bugfix(object): Preserve max-health upgrade order - #3285
Conversation
Signed-off-by: tintinhamans <5984296+tintinhamans@users.noreply.github.com>
8f2e1e3 to
fa3cba6
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoPreserve max-health upgrades across veterancy order
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Include/GameLogic/Object.h | Declares the new accessor for the current veterancy health-bonus multiplier. |
| Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp | Implements the accessor by indexing the initialized global health bonuses with the object’s veterancy. |
| Generals/Code/GameEngine/Source/GameLogic/Object/Upgrade/MaxHealthUpgrade.cpp | Adds veterancy-scaled health upgrades, but the repository’s default compatibility setting compiles the unchanged calculation instead. |
| GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h | Declares the equivalent accessor for Zero Hour objects. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp | Implements the Zero Hour accessor using the current veterancy health bonus. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Upgrade/MaxHealthUpgrade.cpp | Mirrors the scaled calculation and the default-build gating issue in Zero Hour. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Apply MaxHealthUpgrade] --> B{RETAIL_COMPATIBLE_CRC}
B -->|Default: 1| C[Add raw AddMaxHealth]
B -->|Explicitly disabled| D[Read current veterancy bonus]
D --> E[Add AddMaxHealth × bonus]
C --> F[Upgrade order remains significant]
E --> G[Upgrade order is preserved]
Prompt To Fix All With AI
### Issue 1
Generals/Code/GameEngine/Source/GameLogic/Object/Upgrade/MaxHealthUpgrade.cpp:89-92
**Fix Disabled by Default**
`RETAIL_COMPATIBLE_CRC` defaults to `1`, so normal builds take this branch and keep adding the raw `AddMaxHealth` value. The new veterancy scaling runs only when that macro is explicitly disabled. As a result, applying veterancy before the armor upgrade still produces 750 health instead of 780 in default builds. The Zero Hour implementation has the same guard and behavior.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "bugfix(object): preserve max-health upgr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #if RETAIL_COMPATIBLE_CRC | ||
| body->setMaxHealth( body->getMaxHealth() + data->m_addMaxHealth, data->m_maxHealthChangeType ); | ||
| #else | ||
| Real maxHealth = body->getMaxHealth() + data->m_addMaxHealth * obj->getMaxHealthUpgradeScale(); |
There was a problem hiding this comment.
RETAIL_COMPATIBLE_CRC defaults to 1, so normal builds take this branch and keep adding the raw AddMaxHealth value. The new veterancy scaling runs only when that macro is explicitly disabled. As a result, applying veterancy before the armor upgrade still produces 750 health instead of 780 in default builds. The Zero Hour implementation has the same guard and behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Upgrade/MaxHealthUpgrade.cpp
Line: 89-92
Comment:
**Fix Disabled by Default**
`RETAIL_COMPATIBLE_CRC` defaults to `1`, so normal builds take this branch and keep adding the raw `AddMaxHealth` value. The new veterancy scaling runs only when that macro is explicitly disabled. As a result, applying veterancy before the armor upgrade still produces 750 health instead of 780 in default builds. The Zero Hour implementation has the same guard and behavior.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
This is by design, this fix is not compatible with non-retail builds.
There was a problem hiding this comment.
Understood—thanks for clarifying. Since this fix is intentionally limited to non-retail builds, the RETAIL_COMPATIBLE_CRC guard is by design and my finding does not apply. I'll withdraw the comment.
Code Review by Qodo
1. Default builds retain health order bug
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| Real maxHealth = body->getMaxHealth() + data->m_addMaxHealth * obj->getMaxHealthUpgradeScale(); | ||
| body->setMaxHealth( maxHealth, data->m_maxHealthChangeType ); |
There was a problem hiding this comment.
2. New veterans receive too much health 🐞 Bug ≡ Correctness
MaxHealthUpgrade::upgradeImplementation uses the tracker's new veterancy bonus even when Object::onVeterancyLevelChanged runs upgrade modules before the body applies the old-to-new health multiplier. If a player health upgrade is already completed when a unit is created with production veterancy, or the module is triggered by promotion, the addition is multiplied at lines 92-93 and again by ActiveBody::onVeterancyLevelChanged, so a +100 elite upgrade at 1.3× contributes 169 health instead of 130 in both games.
Agent Prompt
## Issue description
Max-health upgrades can execute after the experience tracker has entered its new veterancy level but before the body has applied that veterancy transition. The new scaling therefore applies the new bonus to the addition before the body multiplies the same addition again.
## Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp[2835-2851]
- Generals/Code/GameEngine/Source/GameLogic/Object/Upgrade/MaxHealthUpgrade.cpp[89-94]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[3140-3156]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Upgrade/MaxHealthUpgrade.cpp[89-94]
## Recommended Fix
Apply the body's old-to-new veterancy health transition before any upgrade-module pass can execute against the new veterancy level, then grant and process the corresponding upgrade. Mirror the ordering change in both games and cover a unit created with non-regular production veterancy while its player already owns a max-health upgrade.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Closes #3279
This fixes max health depending on upgrade order. Max-health upgrades now scale their added health using the unit's current veterancy bonus.
For a Paladin with 500 base health, Composite Armor (+100), and Elite veterancy (1.3×):
Before, applying veterancy first gave
(500 × 1.3) + 100 = 750, and applying the armor first gave(500 + 100) × 1.3 = 780.After the fix, both orders result in 780 health.