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

Apply newly added techs before final meter estimation #2600

Conversation

agrrr3
Copy link
Contributor

@agrrr3 agrrr3 commented Oct 21, 2019

Move ApplyNewTechs call Before Final Meter Estimate Update in PostCombatProcessTurns in order to do correct predictions for next turn values on server side.

Forum discussion

Test target meter estimation and it looks good for empire and planetary output.
When i play humans i have 9 RP for all, i research first Algorithmic Elegance and get the "tech researched" message on turn 4. Planetary values are: currently 9RP, target 11RP and next turn 10RP. Empire values are 10RP used/output and 11RP target. Then the values grow as expected each turn until 11RP.

By moving it up we also move it before the following calls
UpdateEmpireSupply(true);
m_universe.UpdateEmpireLatestKnownObjectsAndVisibilityTurns();
m_universe.UpdateStatRecords();
for (auto& entry : empires)
entry.second->UpdateOwnedObjectCounters();
GetSpeciesManager().UpdatePopulationCounter();

Reasoning that there are no unintended side-effects by this move:
supply update is meter based - OK (meter backpropagation already happened)
visibility is based on stealth and detection meters - OK
stat records - probably OK; FOCS statistics are evaluated here those should not change game state. Number of technologies would include newly added techs (which is ok)
update object counters - should be OK, tech does not directly add/remove/change objects
population counters - should be OK, tech does not directly add/remove population

@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 21, 2019

Somebody broke the build - or we moved to c++14 without me noticing.

Condition.cpp:9550:23: Fehler: Verwendung von »auto« in Lambda-Parameterdeklaration ist nur mit »-std=c++14« oder »-std=gnu++14« verfügbar

@agrrr3 agrrr3 force-pushed the 201910_MeterEstimatesShouldConsiderNewlyAddedTech branch from 1aabcdb to df4f70a Compare October 21, 2019 11:24
@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 21, 2019

Reset to the weekly-test-build and cherry-picked the commit in order to evade the c++14 issue. Builds and test fine now

@geoffthemedio
Copy link
Member

Somebody broke the build - or we moved to c++14 without me noticing.

Was me. Should be able to change the return od the lambda to std::string.

@geoffthemedio geoffthemedio added category:bug The Issue/PR describes or solves a perceived malfunction within the game. component:internal The Issue/PR deals with any project component that has no explicit `component` label. priority:high The Issue/PR is very urgent or important and should be addressed/finished as soon as possible. labels Oct 21, 2019
@geoffthemedio geoffthemedio added this to the v0.4.9 milestone Oct 21, 2019
Move ApplyNewTechs call Before Final Meter Estimate Update in PostCombatProcessTurns

We move it also before the following calls
    UpdateEmpireSupply(true);
    m_universe.UpdateEmpireLatestKnownObjectsAndVisibilityTurns();
    m_universe.UpdateStatRecords();
    for (auto& entry : empires)
        entry.second->UpdateOwnedObjectCounters();
    GetSpeciesManager().UpdatePopulationCounter();

This should be
  supply update is meter based - OK (meter backpropagation already happened)
  visibility is based on stealth and detection meters - OK
  stat records - probably OK; FOCS statistics are evaluated here those should not change game state. Number of technologies would include newly added techs (which is ok)
  update object counters - should be OK, tech does not directly add/remove/change objects
  population counters - should be OK, tech does not directly add/remove population
@agrrr3 agrrr3 force-pushed the 201910_MeterEstimatesShouldConsiderNewlyAddedTech branch from df4f70a to 8254802 Compare October 22, 2019 23:20
@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 29, 2019

Is there any reason not to merge this?

@geoffthemedio
Copy link
Member

Probably more testing reports would be good

@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 29, 2019

I have the impression people only test code if it is in the test builds.
And I sadly do not have time to play the game to look for issues.
I'll ask on the forum sigh

@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 29, 2019

Or is there a please-test-everybody tag on our github?

@o01eg o01eg added the status:testing requested The Implementation can't be tested sufficiently by the developer and require support. label Oct 29, 2019
@Vezzra
Copy link
Member

Vezzra commented Nov 1, 2019

@agrrr3,

Or is there a please-test-everybody tag on our github?

Yep, see the label that @o01eg has added.

@geoffthemedio
Copy link
Member

If we can't get any test feedback by asking, then this can be merged to force this issue, though I'd prefer pre-merge testing.

@agrrr3
Copy link
Contributor Author

agrrr3 commented Nov 5, 2019

No feedback for a week. I guess we had some implicit testing because we put the code closer to the state before moving the application so far down the code (in order to be after turn increase).

@Oberlus
Copy link
Contributor

Oberlus commented Nov 5, 2019

I've tested the effect on population techs.
Initial population is 20.
I got on turn 7 the sitrep for Planetary Ecology researched.
In that turn, target population is 21 (correct) and predicted population for next turn is 20 (no increase, I don't know if this is correct because tech needs one more turn to kick in or if it is wrong).
In turn 8 population is 20 (consistent with prediction from previous turn), target population is 21, predicted population is 20.4.
In turn 9 population is 20.4 (consistent with predicted), predicted population in 20.73.
In turn 10 population is 20.73.

Everything seems right, unless we should be getting the first population increase in the turn right after the tech sitrep (turn 8).

This probably can be merged.

@Vezzra
Copy link
Member

Vezzra commented Nov 7, 2019

For the sake of making progress with the release, I'm going to merge this.

@Vezzra Vezzra merged commit 48cd0b9 into freeorion:master Nov 7, 2019
@Vezzra Vezzra added status:merged All relevant commits of this PR were merged into the master development branch. and removed status:testing requested The Implementation can't be tested sufficiently by the developer and require support. labels Nov 7, 2019
@Vezzra Vezzra removed the priority:high The Issue/PR is very urgent or important and should be addressed/finished as soon as possible. label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bug The Issue/PR describes or solves a perceived malfunction within the game. component:internal The Issue/PR deals with any project component that has no explicit `component` label. status:merged All relevant commits of this PR were merged into the master development branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants