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

Population effect priorities are broken #2153

Closed
Morlic-fo opened this Issue Jun 15, 2018 · 18 comments

Comments

Projects
None yet
6 participants
@Morlic-fo
Contributor

Morlic-fo commented Jun 15, 2018

9f02a55 by @geoffthemedio changed the order of growth effects. At the very least, species modifier priorities are changed.

There is a comment by @Dilvish-fo pointing that out but I can't see the issue resolved...(9f02a55#r27525956)

Even if the changes are intended for whatever reason, this means that at the very least, the AI calculations are currently significantly broken and must be updated.

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 16, 2018

Member

Just to double check, @Morlic-fo had you noted any discrepancies between the AI processing and the priorities as noted in our wiki page for this? i.e., had you seen any other changes needed besides getting the GOOD_POPULATION handling in sync?

Member

Dilvish-fo commented Jun 16, 2018

Just to double check, @Morlic-fo had you noted any discrepancies between the AI processing and the priorities as noted in our wiki page for this? i.e., had you seen any other changes needed besides getting the GOOD_POPULATION handling in sync?

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Jun 17, 2018

Contributor

Yes, the issue is quite more complex than that one line.

Previously, there were 3 effective timings:
Early: Before species effects (i.e. affected by them)
Default: Species effects
Late: After species effects (i.e. unaffected by them)

Some of the "Late" effects were changed to scale with species effects, some of the early effects seem now to be unaffected. There were some "Very Late" effects previously but I am not sure yet if that had any gameplay impact or if they could just be moved into the Late category... Commit notes it happened for "cosmetic" purposes.

Relevant commit: 7169e27
Forum thread: http://www.freeorion.org/forum/viewtopic.php?f=15&t=9749&hilit=Priorities

TL;DR:
Affected by species traits are should be:

  • Environmental modifiers
  • "Biological" techs (symb. Biology, Xeno Genetics, Xeno Hybridization, Cyborgs)

Everything else should not be affected by species.

Currently, it is just a mess.

Contributor

Morlic-fo commented Jun 17, 2018

Yes, the issue is quite more complex than that one line.

Previously, there were 3 effective timings:
Early: Before species effects (i.e. affected by them)
Default: Species effects
Late: After species effects (i.e. unaffected by them)

Some of the "Late" effects were changed to scale with species effects, some of the early effects seem now to be unaffected. There were some "Very Late" effects previously but I am not sure yet if that had any gameplay impact or if they could just be moved into the Late category... Commit notes it happened for "cosmetic" purposes.

Relevant commit: 7169e27
Forum thread: http://www.freeorion.org/forum/viewtopic.php?f=15&t=9749&hilit=Priorities

TL;DR:
Affected by species traits are should be:

  • Environmental modifiers
  • "Biological" techs (symb. Biology, Xeno Genetics, Xeno Hybridization, Cyborgs)

Everything else should not be affected by species.

Currently, it is just a mess.

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Jun 17, 2018

Member

Well, that sounds serious. How bad is this mess? Any ideas/estimates how quickly that can be fixed?

Member

Vezzra commented Jun 17, 2018

Well, that sounds serious. How bad is this mess? Any ideas/estimates how quickly that can be fixed?

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 17, 2018

Member

There are two primary parts to the discussion-- the first is agreeing how we really want it all to work, and the second is ensuring that the AI calculations match that. Whether we change the scripting priorities, or the AI calculations, or some mix, it will be a fair amount of changes overall, but I'll recommend we do the same treatment for both master and 0.4.8 rather than trying to come up with some smaller set of changes just for 0.4.8.

On the first point, it looks to me like Morlic's summary of the end conclusion of that planning thread is correct. On reviewing the AI code, it also looks to me like the AI code matches that summary and the associated commit (except that it looks to me like the AI still has no handling for the xenophobic effect). As that discussion seems to be our last consensus, with MatGB tasked with the revamp, and ingeneral changing scripting priorities is somewhat simpler than adjusting the AI calculations, I think it the relative priorities of his commit should be our starting reference/default and we only deviate from it for some specific important reason. (We don't necessarily need to stick with the exact priority levels, if having more levels somehow simplifies the scripting but we'd need to examine any such thing to see if it really represented an extra constraint that the AI would need to be aware of.) I'll hypothesize that MatGB's reference to adding the very late priority for cosmetic purposes may have been related to some idea that it would affect their order of presentation in tooltips or the pedia someplace, but I'm not seeing that actually happen anywhere.

For a summary of the current priorities actually in the script, I was pretty careful when making my summary for the Wiki. My focus there was to help the planning for new species like the Sly, it seems it hadn't even crossed my mind that the AI code and the scripted effects might have gotten out of sync.

So @geoffthemedio I would imagine you had some reason to have made these priority changes, but I'm not seeing it documented anywhere. What do you recall about this?

Member

Dilvish-fo commented Jun 17, 2018

There are two primary parts to the discussion-- the first is agreeing how we really want it all to work, and the second is ensuring that the AI calculations match that. Whether we change the scripting priorities, or the AI calculations, or some mix, it will be a fair amount of changes overall, but I'll recommend we do the same treatment for both master and 0.4.8 rather than trying to come up with some smaller set of changes just for 0.4.8.

On the first point, it looks to me like Morlic's summary of the end conclusion of that planning thread is correct. On reviewing the AI code, it also looks to me like the AI code matches that summary and the associated commit (except that it looks to me like the AI still has no handling for the xenophobic effect). As that discussion seems to be our last consensus, with MatGB tasked with the revamp, and ingeneral changing scripting priorities is somewhat simpler than adjusting the AI calculations, I think it the relative priorities of his commit should be our starting reference/default and we only deviate from it for some specific important reason. (We don't necessarily need to stick with the exact priority levels, if having more levels somehow simplifies the scripting but we'd need to examine any such thing to see if it really represented an extra constraint that the AI would need to be aware of.) I'll hypothesize that MatGB's reference to adding the very late priority for cosmetic purposes may have been related to some idea that it would affect their order of presentation in tooltips or the pedia someplace, but I'm not seeing that actually happen anywhere.

For a summary of the current priorities actually in the script, I was pretty careful when making my summary for the Wiki. My focus there was to help the planning for new species like the Sly, it seems it hadn't even crossed my mind that the AI code and the scripted effects might have gotten out of sync.

So @geoffthemedio I would imagine you had some reason to have made these priority changes, but I'm not seeing it documented anywhere. What do you recall about this?

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Jun 17, 2018

Contributor

As far as I understood, Geoff introduced the new priorities because the target population effects have to happen before the current population stuff and messing up the order of effects was an unintended side-effect.

So, we probably want to have something like
TARGET_POPULATION_BEFORE_SCALING_PRIORITY
TARGET_POPULATION_SCALING_PRIORITY
TARGET_POPULATION_AFTER_SCALING_PRIORITY
TARGET_POPULATION_OVERRIDE_PRIORITY

assuming that we do not need the separation of late/very late stuff. If we need that, then (at least) a 5th priority between AFTER_SCALING and OVERRIDE will be needed.

Contributor

Morlic-fo commented Jun 17, 2018

As far as I understood, Geoff introduced the new priorities because the target population effects have to happen before the current population stuff and messing up the order of effects was an unintended side-effect.

So, we probably want to have something like
TARGET_POPULATION_BEFORE_SCALING_PRIORITY
TARGET_POPULATION_SCALING_PRIORITY
TARGET_POPULATION_AFTER_SCALING_PRIORITY
TARGET_POPULATION_OVERRIDE_PRIORITY

assuming that we do not need the separation of late/very late stuff. If we need that, then (at least) a 5th priority between AFTER_SCALING and OVERRIDE will be needed.

@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Jun 17, 2018

Member

I'll hypothesize that MatGB's reference to adding the very late priority for cosmetic purposes may have been related to some idea that it would affect their order of presentation in tooltips

Correct. (unintentional absense dragged on, apologies)

The "very late" macro was entirely to ensure that similar effects, specifically the Growth Specials, were grouped together in the tooltip and not spread amongst the others. I haven't played at all in months but this definitely worked when I implemented it and tested it.

The macros are entirely there to give a readable number, it was Marcel's idea to use them I just implemented them where we wanted them, if they're not working it's an underlying problem with the scripting priorities system and given how important that is for current balance: without playing, this may be related to #2162, Good Population as a trait is way overpowered if it applies to all population bonuses, I only considered making Scylior playable after we'd fixed GP.

There are many other ways it can happen but if priorities aren't working here, are they not working in other places as well? Because for some of them it really does matter what order things happen in.

Member

MatGB commented Jun 17, 2018

I'll hypothesize that MatGB's reference to adding the very late priority for cosmetic purposes may have been related to some idea that it would affect their order of presentation in tooltips

Correct. (unintentional absense dragged on, apologies)

The "very late" macro was entirely to ensure that similar effects, specifically the Growth Specials, were grouped together in the tooltip and not spread amongst the others. I haven't played at all in months but this definitely worked when I implemented it and tested it.

The macros are entirely there to give a readable number, it was Marcel's idea to use them I just implemented them where we wanted them, if they're not working it's an underlying problem with the scripting priorities system and given how important that is for current balance: without playing, this may be related to #2162, Good Population as a trait is way overpowered if it applies to all population bonuses, I only considered making Scylior playable after we'd fixed GP.

There are many other ways it can happen but if priorities aren't working here, are they not working in other places as well? Because for some of them it really does matter what order things happen in.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Jun 17, 2018

Member

Geoff introduced the new priorities because the target population effects have to happen before the current population stuff

and also before the effect that depend on current population that determine various other resource meters.

#2159 (comment)

Member

geoffthemedio commented Jun 17, 2018

Geoff introduced the new priorities because the target population effects have to happen before the current population stuff

and also before the effect that depend on current population that determine various other resource meters.

#2159 (comment)

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 17, 2018

Member

Geoff introduced the new priorities because the target population effects have to happen before the current population stuff

and also before the effect that depend on current population that determine various other resource meters.

@geoffthemedio The general framework you describe here makes sense and sounds fine (edit except ... nm about that question, I just looked up the handful of additional direct Population effects like from bombardment, ConcCamps, etc) . I'd add the additional detail that within the set of TargetPopulation effects, the order of effects should match that which Morlic noted above. But I am seeing a lot of mismatch between the current scripting and this framework.

When I look over that wiki page I had put together listing the order of these priorities & which effects are under each, I'm not seeing how those priorities fit with this explanation. And that wiki page had been reviewed and corrected on a couple points already by other people, and I also did double check a sampling of those right now to verify they were right. Examples of things which seem to be scripted wrongly in light of your explanation, are growth specials and non-growth techs like CON_NDIM_STRC getting EARLY_TARGET_POPULATION_PRIORITY. And some of those target population effects are given VERY_LATE_PRIORITY and so I don't see how that fits at all with this framework you've now described (particularly given that some of the other resource meter effects you mention as needing to come after all the population stuff, like the Industrial Center population based effect, have EARLY_PRIORITY).

If I am still just misunderstanding something important, please clarify.

Member

Dilvish-fo commented Jun 17, 2018

Geoff introduced the new priorities because the target population effects have to happen before the current population stuff

and also before the effect that depend on current population that determine various other resource meters.

@geoffthemedio The general framework you describe here makes sense and sounds fine (edit except ... nm about that question, I just looked up the handful of additional direct Population effects like from bombardment, ConcCamps, etc) . I'd add the additional detail that within the set of TargetPopulation effects, the order of effects should match that which Morlic noted above. But I am seeing a lot of mismatch between the current scripting and this framework.

When I look over that wiki page I had put together listing the order of these priorities & which effects are under each, I'm not seeing how those priorities fit with this explanation. And that wiki page had been reviewed and corrected on a couple points already by other people, and I also did double check a sampling of those right now to verify they were right. Examples of things which seem to be scripted wrongly in light of your explanation, are growth specials and non-growth techs like CON_NDIM_STRC getting EARLY_TARGET_POPULATION_PRIORITY. And some of those target population effects are given VERY_LATE_PRIORITY and so I don't see how that fits at all with this framework you've now described (particularly given that some of the other resource meter effects you mention as needing to come after all the population stuff, like the Industrial Center population based effect, have EARLY_PRIORITY).

If I am still just misunderstanding something important, please clarify.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Jun 17, 2018

Member

I meant for cases where the modification to a resource meter depends on the population meter, those effects should happen after all population setting effects (first target pop, then current pop, then stuff that depends on population). I think I was wrong about this though, as currently I think all such cases use the non Value(...) wrapped value of Target.Population, which will be the value from the previous turn (or last backpropagation). But I'd still keep them separate and ordered as above to be safe.

What's wrong with growth specials' target population setting effects having EARLY_TARGET_POPULATION_PRIORITY? It doesn't conflict with what I wrote above about the order of target pop, current pop, and then other potentially pop-dependent effects. If you want to order the target population setting effects more precisely, feel free to add another priority. The current one just control the base target pop setting, scaling of that, and then stuff that overrides what came before, as the names indicate.

CON_NDIM_STRC has a SetTargetPopulation effect. Thus it's in EARLY_TARGET_POPULATION_PRIORITY. I don't understand your concern.

I also don't understand your concern about industrial centre effect being EARLY_PRIORITY. This is after all the population effects, as it should be.

Member

geoffthemedio commented Jun 17, 2018

I meant for cases where the modification to a resource meter depends on the population meter, those effects should happen after all population setting effects (first target pop, then current pop, then stuff that depends on population). I think I was wrong about this though, as currently I think all such cases use the non Value(...) wrapped value of Target.Population, which will be the value from the previous turn (or last backpropagation). But I'd still keep them separate and ordered as above to be safe.

What's wrong with growth specials' target population setting effects having EARLY_TARGET_POPULATION_PRIORITY? It doesn't conflict with what I wrote above about the order of target pop, current pop, and then other potentially pop-dependent effects. If you want to order the target population setting effects more precisely, feel free to add another priority. The current one just control the base target pop setting, scaling of that, and then stuff that overrides what came before, as the names indicate.

CON_NDIM_STRC has a SetTargetPopulation effect. Thus it's in EARLY_TARGET_POPULATION_PRIORITY. I don't understand your concern.

I also don't understand your concern about industrial centre effect being EARLY_PRIORITY. This is after all the population effects, as it should be.

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 18, 2018

Member

(unintentional absense dragged on, apologies)

@MatGB that sounds like an avowal that said absence no longer drags on, woot!

Member

Dilvish-fo commented Jun 18, 2018

(unintentional absense dragged on, apologies)

@MatGB that sounds like an avowal that said absence no longer drags on, woot!

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 18, 2018

Member

Some conversations can be painfully difficult. I remind myself that super-busy people sometimes feel compelled to shoot from the hip, without reviewing the memos and references that their underlings are pleading with them to read. Sometimes they still hit their target, but sometimes they miss; that's just how life is. To help break free from the suboptimal local minimum we appear trapped in, I invoke Plato's Fifth Lost Dialogue:

G: we must ensure that in all cases A < B < C
D: but in case X, A > C
G: Pfft, B < C, all is as it should be
D: %$#^???

What does it mean? We'll never know, the remainder of the Dialogue is lost to us, but sometimes it can still be illuminating to meditate upon fragments such as this; we may discern their essence reflected in multitudes around us like dew in the morning grass.

= * = * = * = * = *

...(first target pop, then current pop, then stuff that depends on population). I think I was wrong about this though, as currently I think all such cases use the non Value(...) wrapped value of Target.Population, which will be the value from the previous turn (or last backpropagation).

@geoffthemedio I heartily agree with both parts of the full quoted sentence, and I think it is an excellent contribution towards a reasonable resolution to this issue. Regarding the factual premise, I did a thorough review of all uses of "Value(", and I found no Value(...) wrapped forms of Target.Population or Target.TargetPopulation being used. Regarding the logical conclusions, and which value will be used, and that it means we don't really need to try to ensure that all TargetPop effects precede all Pop effects precede all pop-derived resource effects, I also agree.

Although you did proceed to note,

But I'd still keep them separate and ordered as above to be safe.

it appears to me that the word 'keep' is inoperative there because there are already so many violations of that ordering. It might be nice to achieve that ordering someday, but it is entirely unnecessary for resolving the present Issue, and I think it would both substantially delay and jeopardize the reliability of this Issue's resolution if we were to immediately pursue that broader ordering.

All we really need here is to address the relative ordering of the TargetPopulation effects.

If you want to order the target population setting effects more precisely, feel free to add another priority.

That may very well be helpful, yes. It might also be that for clarity's sake we'd want to rename some, or just make more new ones.

The current one[s] just control the base target pop setting, scaling of that, and then stuff that overrides what came before, as the names indicate.

Well, those are the ones that actually use the phrase "TARGET_POPULATION" in their macro name, but there are quite a few other priorities that are also currently used for SetTargetPopulation effects, as noted on that wiki page I have cited numerous times now. It really would be helpful if you could find time to review that (to more clearly see the current state of things) and to review the summary and links that Morlic provided above (to better understand our target state of things).

@Morlic-fo I am unlikely to be able to devote much more time for this until Friday. If you have time to implement your

TARGET_POPULATION_BEFORE_SCALING_PRIORITY
TARGET_POPULATION_SCALING_PRIORITY
TARGET_POPULATION_AFTER_SCALING_PRIORITY
TARGET_POPULATION_OVERRIDE_PRIORITY

suggestion I think that would be great. Please be aware that there are still some TargetPopulation effects not noted on that wikie page, such as for the Worldtree, Banforo, Lembalalam, and others. To be sure to catch all those (if you do have time for this), be sure to run the following in a git bash window opened to the FO root directory

grep -Ri "SetTargetPopulation" default/scripting/
Member

Dilvish-fo commented Jun 18, 2018

Some conversations can be painfully difficult. I remind myself that super-busy people sometimes feel compelled to shoot from the hip, without reviewing the memos and references that their underlings are pleading with them to read. Sometimes they still hit their target, but sometimes they miss; that's just how life is. To help break free from the suboptimal local minimum we appear trapped in, I invoke Plato's Fifth Lost Dialogue:

G: we must ensure that in all cases A < B < C
D: but in case X, A > C
G: Pfft, B < C, all is as it should be
D: %$#^???

What does it mean? We'll never know, the remainder of the Dialogue is lost to us, but sometimes it can still be illuminating to meditate upon fragments such as this; we may discern their essence reflected in multitudes around us like dew in the morning grass.

= * = * = * = * = *

...(first target pop, then current pop, then stuff that depends on population). I think I was wrong about this though, as currently I think all such cases use the non Value(...) wrapped value of Target.Population, which will be the value from the previous turn (or last backpropagation).

@geoffthemedio I heartily agree with both parts of the full quoted sentence, and I think it is an excellent contribution towards a reasonable resolution to this issue. Regarding the factual premise, I did a thorough review of all uses of "Value(", and I found no Value(...) wrapped forms of Target.Population or Target.TargetPopulation being used. Regarding the logical conclusions, and which value will be used, and that it means we don't really need to try to ensure that all TargetPop effects precede all Pop effects precede all pop-derived resource effects, I also agree.

Although you did proceed to note,

But I'd still keep them separate and ordered as above to be safe.

it appears to me that the word 'keep' is inoperative there because there are already so many violations of that ordering. It might be nice to achieve that ordering someday, but it is entirely unnecessary for resolving the present Issue, and I think it would both substantially delay and jeopardize the reliability of this Issue's resolution if we were to immediately pursue that broader ordering.

All we really need here is to address the relative ordering of the TargetPopulation effects.

If you want to order the target population setting effects more precisely, feel free to add another priority.

That may very well be helpful, yes. It might also be that for clarity's sake we'd want to rename some, or just make more new ones.

The current one[s] just control the base target pop setting, scaling of that, and then stuff that overrides what came before, as the names indicate.

Well, those are the ones that actually use the phrase "TARGET_POPULATION" in their macro name, but there are quite a few other priorities that are also currently used for SetTargetPopulation effects, as noted on that wiki page I have cited numerous times now. It really would be helpful if you could find time to review that (to more clearly see the current state of things) and to review the summary and links that Morlic provided above (to better understand our target state of things).

@Morlic-fo I am unlikely to be able to devote much more time for this until Friday. If you have time to implement your

TARGET_POPULATION_BEFORE_SCALING_PRIORITY
TARGET_POPULATION_SCALING_PRIORITY
TARGET_POPULATION_AFTER_SCALING_PRIORITY
TARGET_POPULATION_OVERRIDE_PRIORITY

suggestion I think that would be great. Please be aware that there are still some TargetPopulation effects not noted on that wikie page, such as for the Worldtree, Banforo, Lembalalam, and others. To be sure to catch all those (if you do have time for this), be sure to run the following in a git bash window opened to the FO root directory

grep -Ri "SetTargetPopulation" default/scripting/
@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Jun 18, 2018

Member

The "ones" I was referring to were the priorities that were supposed to be used for target population effects.

"Keep" in the sense of how they were supposed to be arranged. I considered cases where target population setting effects have a much later priority to be errors / oversights.

I looked over the wiki list. I didn't change how I thought things were supposed to be arranged, which was what I was describing above.

My noting that the specific examples (CON_NDIM_STRC and growth specials) you referred to had the priority I expect doesn't mean that I'm disagreeing with you noting that various other cases don't have the right priority. Those specific cases are correct, not "all is well".

Member

geoffthemedio commented Jun 18, 2018

The "ones" I was referring to were the priorities that were supposed to be used for target population effects.

"Keep" in the sense of how they were supposed to be arranged. I considered cases where target population setting effects have a much later priority to be errors / oversights.

I looked over the wiki list. I didn't change how I thought things were supposed to be arranged, which was what I was describing above.

My noting that the specific examples (CON_NDIM_STRC and growth specials) you referred to had the priority I expect doesn't mean that I'm disagreeing with you noting that various other cases don't have the right priority. Those specific cases are correct, not "all is well".

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 18, 2018

Member

Those specific cases are correct

Those specific cases may somehow be as you expect and seem correct to you, but I can only understand such an assessment as being made through the narrow lens of the very particular structural change you had been trying to accomplish, rather than the broader view of applicable constraints that is discussed above. Those two will not be remaining in the earliest priority used for target population effects.

Member

Dilvish-fo commented Jun 18, 2018

Those specific cases are correct

Those specific cases may somehow be as you expect and seem correct to you, but I can only understand such an assessment as being made through the narrow lens of the very particular structural change you had been trying to accomplish, rather than the broader view of applicable constraints that is discussed above. Those two will not be remaining in the earliest priority used for target population effects.

@Oberlus

This comment has been minimized.

Show comment
Hide comment
@Oberlus

Oberlus Jun 18, 2018

@Morlic-fo You may find useful this list of files with TargetPopulation effets posted in the forum: http://www.freeorion.org/forum/viewtopic.php?f=6&t=10988&p=92979#p92979

Priorities reordering suggestion here: http://www.freeorion.org/forum/viewtopic.php?f=6&t=10988&p=92981#p92981

Oberlus commented Jun 18, 2018

@Morlic-fo You may find useful this list of files with TargetPopulation effets posted in the forum: http://www.freeorion.org/forum/viewtopic.php?f=6&t=10988&p=92979#p92979

Priorities reordering suggestion here: http://www.freeorion.org/forum/viewtopic.php?f=6&t=10988&p=92981#p92981

Dilvish-fo added a commit to Dilvish-fo/freeorion that referenced this issue Jun 18, 2018

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 18, 2018

Member

@Morlic-fo I wound up finding some time to make the set of changes & thought it best to get them up for review/discussion as soon as possible; I hope I got this out before you started on the task as well.

Member

Dilvish-fo commented Jun 18, 2018

@Morlic-fo I wound up finding some time to make the set of changes & thought it best to get them up for review/discussion as soon as possible; I hope I got this out before you started on the task as well.

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Jun 18, 2018

Contributor

Well, I actually went through the entire thing as well... Which isn't necessarily bad, I suppose.

For reference, my changes are: https://github.com/Morlic-fo/freeorion/pull/17/files

Contributor

Morlic-fo commented Jun 18, 2018

Well, I actually went through the entire thing as well... Which isn't necessarily bad, I suppose.

For reference, my changes are: https://github.com/Morlic-fo/freeorion/pull/17/files

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo Jun 18, 2018

Member

Well, I actually went through the entire thing as well... Which isn't necessarily bad, I suppose.

OK, right, I think it's an important enough issue that it is good to be able to compare two different implementations. The comparison between yours and mine is attached below.

There are not many differences I find between our implementations. The only two really significant one is I had prioritized CON_ORBITAL_HAB as a growth tech (which is its category, despite its name) and therefor subject to scaling, but I now see that in Mat's discussion he was treating it as a construction tech not subject to scaling, and so mine is a mistake there. I also see you added a TARGET_POPULATION_LAST_BEFORE_OVERRIDE_PRIORITY, whereas I just had all those things in my post-scaling priority. For all the phototropic uses I don't think it actually matters (but is not a problem), and for the xenophobic case it does look useful/important.

I had also accidentally shifted the temporal anomaly research effect to be EARLY_TARGET_POPULATION_POST_SCALING_PRIORITY, whereas it should have remained EARLY_PRIORITY. I don't think the change would actually matter, but my change is still a mistake.

On more minor notes, your priority names are a slightly different naming convention (I was sticking more closely to the existing naming convention, but yours are more clear), you added a few more comments, and you added a priority for the Adequate environment (which is purely for labeling purposes and is fine as default or as you put it, I think).

So, it's looking to me like yours is the superior fix. Please put up yours as a PR and I'll close mine as superseded.

I think this comparison between our two versions has been very useful, and makes me very confident of your fix.

pop_fix_compare.diff.zip

Member

Dilvish-fo commented Jun 18, 2018

Well, I actually went through the entire thing as well... Which isn't necessarily bad, I suppose.

OK, right, I think it's an important enough issue that it is good to be able to compare two different implementations. The comparison between yours and mine is attached below.

There are not many differences I find between our implementations. The only two really significant one is I had prioritized CON_ORBITAL_HAB as a growth tech (which is its category, despite its name) and therefor subject to scaling, but I now see that in Mat's discussion he was treating it as a construction tech not subject to scaling, and so mine is a mistake there. I also see you added a TARGET_POPULATION_LAST_BEFORE_OVERRIDE_PRIORITY, whereas I just had all those things in my post-scaling priority. For all the phototropic uses I don't think it actually matters (but is not a problem), and for the xenophobic case it does look useful/important.

I had also accidentally shifted the temporal anomaly research effect to be EARLY_TARGET_POPULATION_POST_SCALING_PRIORITY, whereas it should have remained EARLY_PRIORITY. I don't think the change would actually matter, but my change is still a mistake.

On more minor notes, your priority names are a slightly different naming convention (I was sticking more closely to the existing naming convention, but yours are more clear), you added a few more comments, and you added a priority for the Adequate environment (which is purely for labeling purposes and is fine as default or as you put it, I think).

So, it's looking to me like yours is the superior fix. Please put up yours as a PR and I'll close mine as superseded.

I think this comparison between our two versions has been very useful, and makes me very confident of your fix.

pop_fix_compare.diff.zip

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Jun 18, 2018

Contributor

So, it's looking to me like yours is the superior fix. Please put up yours as a PR and I'll close mine as superseded.

Done.

Contributor

Morlic-fo commented Jun 18, 2018

So, it's looking to me like yours is the superior fix. Please put up yours as a PR and I'll close mine as superseded.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment