Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factory potentials & Economies of scale for factories #1892

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

SneakBug8
Copy link
Contributor

No description provided.

@schombert
Copy link
Owner

@ineveraskedforthis you probably want to review this


auto const tax_eff = nations::tax_efficiency(state, n);
auto const rich_effect = (1.0f - tax_eff * float(state.world.nation_get_rich_tax(n)) / 100.0f);

if(desired_types.empty()) {
for(auto type : state.world.in_factory_type) {
if(n.get_active_building(type) || type.get_is_available_from_start()) {
if(command::can_begin_factory_building_construction(state, nid, sid, type, false) || command::can_begin_factory_building_construction(state, nid, sid, type, true)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SneakBug8 This is changing a very cheap check into two very expensive checks. If the expensive checks are necessary, the code should be written so that they are taken a minimal number of times (for example, it shouldn't be doing duplicate work checking for both upgrades and new construction)

@@ -467,6 +467,11 @@ object {
type{ bitfield }
tag{ scenario }
}
property{
name{ uses_potentials }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this being stored in the factory type when everything else is tied to the commodity? I don't think that your syntax for this feature allows it to attach to some factory types and not others if they happen to produce the same output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants