Fix processes' toggle buttons not matching their actual state #5632
Conversation
|
I have noticed while testing that process toggles don't work on cell colonies (at least, on other members of those colonies). I hope that it would be fine if I fixed it too with this PR. |
|
It's most likely fine, but I have slight concern if that has understandability problems because #3207 is not done. But I guess in most situations where like for efficiency reasons players want to toggle processes it's fine as the colony compound distribution will make sure that other colony members can basically do the same processes as the lead cell. |
There was a problem hiding this comment.
I think this is a pretty good design for this problem and one that can fix the toggle states getting out of sync. What I have significant concerns about is the change of that one struct to a class which majorly changes all memory usage and allocations related to it, which is something I've relied on greatly in the process system for maximum efficiency. I think as a result the changes here need to be considered more.
|
|
||
| /// <summary> | ||
| /// The processes and their associated speed statistics | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// This uses <see cref="BioProcess"/> rather than <see cref="TweakedProcess"/> as the key so that equality |
There was a problem hiding this comment.
This is another point that I think is causing some pretty major architectural problems here. I didn't yet check every single place, that these changes impact, but my impression is that all of the code depending on the assumptions about the code that has been changed in this PR have not been updated yet.
| @@ -50,7 +50,8 @@ public bool Equals(TweakedProcess other) | |||
| return false; | |||
|
|
|||
| // ReSharper disable once CompareOfFloatsByEqualityOperator | |||
| return Rate == other.Rate && ReferenceEquals(Process, other.Process); | |||
| return Rate == other.Rate && SpeedMultiplier == other.SpeedMultiplier | |||
There was a problem hiding this comment.
Adding comparing the speed multiplier here, I think might break some grouping of process displays...
So I would remove this change.
There was a problem hiding this comment.
I don't think it's right, because if processes with different states are considered equal, then they aren't correctly updated on GUI, unless I'm missing something.
There was a problem hiding this comment.
Have you playtested and confirmed that is a bug? Kind of unhelpfully the process list detection uses equality to combine items, so too strict checking can result in a situation where duplicates get created into the process list.
There was a problem hiding this comment.
Yes, I have play tested it without the speed multiplier check and processes' statuses didn't update.
Are you sure that too strict checking may result in duplicates? If I read the process list code right, then GUI-side processes (ChemicalEquations) that aren't equal to any of the display infos that it is given are simply removed.
There was a problem hiding this comment.
It's true that if this isn't the type of equality used in the GUI combining (and that doesn't call this equality in turn), this should be fine. Though, the last time I tried to improve the process equality checks to be more accurate, I totally messed up the process speed panel to show each instance of a process separately (instead of keeping the functionality where all organelles are summed into one process of each type). I'll need to check this carefully when doing the next review (after the extra temporary memory use is solved).
| @@ -32,6 +32,8 @@ namespace Systems; | |||
| [RuntimeCost(55)] | |||
| public sealed class ProcessSystem : AEntitySetSystem<float> | |||
| { | |||
| private static readonly Dictionary<BioProcess, float> TempSpeedMultipliers = new(); | |||
There was a problem hiding this comment.
I'm pretty sure this is a massive problem as ComputeActiveProcessList has to be thread safe.
There was a problem hiding this comment.
Would it be right to put all of ComputeActiveProcessList's code in a lock statement? Only a single thread would then be able to access the dictionary at a time, though that would worsen performance
There was a problem hiding this comment.
I wouldn't because this then becomes a bottleneck. If there truly is no way to avoid the temp memory, then the approach where the method caller must provide the working memory should be used (look at for example how the hex adding methods in the hex layout class require multiple working memory parameters to work).
Brief Description of What This PR Does
Fixes process toggle buttons being de-synced from their processes' actual state.
Does so by making process displays get the toggle state directly from the process, instead of using a local variable. This also allows those states to be saved.
Also, makes TweakedProcesses' speed multipliers save when they are updated (e.g. when an organelle duplicates).
Related Issues
Fixes #5420
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)
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.