Split auto-evo mutation generation into substeps#6946
Split auto-evo mutation generation into substeps#6946RyuDanuer wants to merge 3 commits intoRevolutionary-Games:masterfrom
Conversation
|
I manually reran the Auto-Evo Exploring Tool with 5 worlds x 15 generations and generated a fresh step timing flamegraph from that run: The key part for this issue is the max single substep time, not the cumulative total. I also checked the log for |
|
Besides the unsuccessful automated check: Before I look in detail, did you make sure the in-game auto-evo results are equivalent to before this change? |
| } | ||
| } | ||
|
|
||
| TotalSteps = 0; |
There was a problem hiding this comment.
TotalSteps should never reduce. That doesn't make sense as the total steps should track the total number of steps that are done and are yet to come. So the code should be refactored to never reduce TotalSteps, it should only be allowed to increment.
| /// -1 means not yet computed | ||
| /// </summary> | ||
| private volatile int totalSteps = -1; | ||
| private int totalSteps = -1; |
There was a problem hiding this comment.
Why did you remove the volatile marking? As far as I can tell you did not remove threading from auto-evo so this is a totally wrong change to make.
The split-step mutation generation preserves the old behavior more closely, especially by keeping the base mutant lifetime/order equivalent to the old recursive path. For validation, I ran a fixed-seed auto-evo comparison against the PR parent for 3 generations. The raw species IDs / generated names can differ, but after remapping those out, the actual auto-evo state matched: species traits, populations, and patch occupants were equivalent. |
|
|
||
| private void UpdateTotalStepsEstimate() | ||
| { | ||
| var estimate = CompleteSteps + runSteps.Sum(s => s.TotalSteps) + 1; |
There was a problem hiding this comment.
I'm pretty sure this would break all other auto-evo steps than the one you modified.
Also rather than locking just for an int, compare exchange or other interlocked methods should be used.
Overall this still looks like a major LLM mess I'm not sure I will have the mental energy to review anytime soon.
There was a problem hiding this comment.
I replaced the lock with an Interlocked.CompareExchange loop, so the total step estimate still only moves upward but doesn’t use a lock just for the int update. This keeps the volatile/threaded behavior intact while addressing the review point.
There was a problem hiding this comment.
You didn't address the fact that other auto-evo step types likely will not work correctly as their CompletedSteps assumes it approaches and reaches the total steps it reports and not like this.
I'm tired of trying to explain to you that bigger changes that touch the overall code architecture of Thrive need a lot more careful manual review than you are doing.
It might be the case that ModifyExistingSpecies.cs is converted reasonably and works without excessive temporary memory usage, but I'm tired of looking at AI PRs again and again when it would be faster for me to just redo the work from scratch.
For human programmers who want to get into Thrive I will try my hardest to provide constructive feedback on their PRs so that they might improve and in the future become a net contributor (or at least walk away from Thrive with some valuable experience on their programming career). But for your PRs I doubt they are going to get any better as I'm still needing to mention that you can't just let the AI run wild as it will not care about code uniformity or properly structuring code changes to fit in with existing code.
|
|
||
| private void UpdateTotalStepsEstimate() | ||
| { | ||
| var estimate = CompleteSteps + runSteps.Sum(s => s.TotalSteps) + 1; |
There was a problem hiding this comment.
You didn't address the fact that other auto-evo step types likely will not work correctly as their CompletedSteps assumes it approaches and reaches the total steps it reports and not like this.
I'm tired of trying to explain to you that bigger changes that touch the overall code architecture of Thrive need a lot more careful manual review than you are doing.
It might be the case that ModifyExistingSpecies.cs is converted reasonably and works without excessive temporary memory usage, but I'm tired of looking at AI PRs again and again when it would be faster for me to just redo the work from scratch.
For human programmers who want to get into Thrive I will try my hardest to provide constructive feedback on their PRs so that they might improve and in the future become a net contributor (or at least walk away from Thrive with some valuable experience on their programming career). But for your PRs I doubt they are going to get any better as I'm still needing to mention that you can't just let the AI run wild as it will not care about code uniformity or properly structuring code changes to fit in with existing code.
|
If you want to be helpful to Thrive, I suggest you would look through our issue backlog and pick out small bugs that are much more likely for an AI to be able to solve on the first go rather than these bigger issues where the AI just clearly fails at refactoring existing code in a satisfactory manner. Small issues even if not high priority or planned to be a priority to work on, would improve Thrive and those would hopefully be ones were the AI agent can create nice and understandable at a glance PRs. |
Brief Description of What This PR Does
This PR splits
ModifyExistingSpeciesmutation generation into resumable substeps, so the expensive mutation recursion no longer happens as one long auto-evo step. The mutation traversal now keeps explicit frame state and continues over manyRunStepcalls instead of recursively finishing a whole species in one call.I also updated
AutoEvoRunto refresh the total step estimate while steps are running, asModifyExistingSpecies.TotalStepsnow changes while its internal mutation work is consumed.I manually tested this with the Auto-Evo Exploring Tool by running 5 worlds for 15 generations. The run completed, and the log had 0 matches for
Single auto-evo step ModifyExistingSpecies. The measured worstModifyExistingSpeciessubstep was 163.764 ms, under the 0.4s warning threshold.I also ran:
Related Issues
Closes #5416
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR). This includes gameplay testing by the PR author.
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.