Fix double refunding of organelle upgrade MP #5779
Conversation
|
The lead programmer for Thrive is currently on vacation until 2025-01-07. Until then other programmers will try to make pull request reviews, but please be patient if your PR is not getting reviewed. PRs may be merged after multiple programmers have approved the changes (especially making sure to ensure style guide conformance and gameplay testing are good). If there are no active experienced programmers who can perform merges, PRs may need to wait until the lead programmer is back to be merged. |
|
|
||
| // The replacing action is in the remove organelle action | ||
| // Organelle upgrades can never combine since the old choice will have already been refunded. | ||
| // https://github.com/Revolutionary-Games/Thrive/issues/5524 |
There was a problem hiding this comment.
I'm a bit hesitant on this as if you have an upgrade like the chemoreceptor and it is made to cost MP, then wouldn't the combine be required for MP to work correctly?
For example:
- Player edits detection distance from 200 to 500 and it costs 3 MP
- The player then edits the distance from 500 to 200 again costing 3 MP
- The combined action should then be the change being from 200 to 200 and not costing any MP, but if the combining system is told that upgrade changes cannot combine, then this would cost 6 MP.
There was a problem hiding this comment.
I think that the original problem comes down to the fact that removing upgrades refunds MP, which causes weirdness with combined upgrades.
cost += newUpgrades.Sum(u => u.MPCost);
cost -= removedUpgrades.Sum(u => u.MPCost);Since #4095 is an issue, I believe that this refunding wasn't a desired behavior in the first place, as refunding MP by reverting upgrades in the same editor cycle should be done by action combining.
There was a problem hiding this comment.
reverting upgrades in the same editor cycle should be done by action combining.
That's probably true and a mistake I didn't realize at the time when I was writing the general upgrades system for the editor.
Brief Description of What This PR Does
When changing the organelle upgrade repeatedly in the same editor cycle, MP would be refunded twice (as detailed in #5524). This is because the cost of an upgrade also includes the negated cost of the current upgrade, meanwhile the editor refunds MP where
ActionInterferenceMode.Combinableis returned as the interference mode.Related Issues
Closes #5524
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.