bugfix(battleplan): Battle plans are now transferred to the new owner on capture by Stubbjax · Pull Request #2257 · TheSuperHackers/GeneralsGameCode

Stubbjax · 2026-02-04T16:07:55Z

Fixes USA Strategy Center Battle Plan does not automatically apply its bonus after capturing #2211
Merge after refactor(battleplan): Split data off of BattlePlanBonuses class #2335

This change fixes an issue where battle plans are not transferred from the old owner to the new owner when a Strategy Center is captured. In addition, bonuses in effect upon surrender in team games are also now transferred to the new owner, if applicable.

In retail, capturing a Strategy Center leaves any existing bonus intact for the opponent, while giving the capturing player no bonus.

Run a couple of replays @Skyaero42

greptile-apps · 2026-02-04T16:12:27Z

Greptile Summary

This PR fixes issue #2211 by implementing BattlePlanUpdate::onCapture() so that the active battle plan and its army bonuses are properly transferred from the old owner to the new owner when a Strategy Center is captured. A required prerequisite const correctness fix to Player::changeBattlePlan() — replacing in-place mutation of the bonus parameter with a local invertedBonus copy — makes it safe to call the function twice with the same m_bonuses pointer without corrupting data between the two calls.

Core fix (BattlePlanUpdate.cpp): onCapture() decrements the old owner's plan counter (removing bonuses if their last center of that type) and increments the new owner's (applying bonuses if their first), guarded by #if !RETAIL_COMPATIBLE_CRC to preserve retail-compatible behaviour where bonuses stay with the old owner.
Const refactor (Player.cpp / Player.h): changeBattlePlan() now takes const BattlePlanBonusesData *bonus and works on a local invertedBonus copy when removing bonuses — both a correctness improvement and a prerequisite for the dual-call pattern in onCapture().
Consistent across both codebases: Changes are applied identically to both the Generals/ (vanilla) and GeneralsMD/ (Zero Hour) trees.
Open discussion items from previous threads (date in bugfix comment, null-owner guards aligning with RadarUpgrade/PowerPlantUpgrade patterns, stale "in-out parm" comment) remain to be addressed before merge.

Confidence Score: 4/5

Logic is correct and well-structured; a few open housekeeping points from prior threads should be resolved before merging.
The const refactoring is clean and directly enables the safe dual-call pattern in onCapture(). The counter logic in changeBattlePlan() correctly handles multi-center scenarios (no double-bonus, no premature removal). m_bonuses is always allocated in the constructor so it is never null. The #if !RETAIL_COMPATIBLE_CRC guard correctly preserves retail behaviour as documented. Score is 4 rather than 5 because the unresolved items from previous review threads (null-owner guard alignment with the rest of the codebase, stale comment, date) are acknowledged but not yet fixed.
Both BattlePlanUpdate.cpp files (Generals and GeneralsMD) — the onCapture implementation carries the open discussion items from prior threads.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp	Adds `BattlePlanUpdate::onCapture()` to transfer battle plan bonuses from old to new owner on building capture, guarded by `#if !RETAIL_COMPATIBLE_CRC` to preserve retail parity.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp	Zero Hour mirror of the same `onCapture` addition, identical to the Generals version — same strengths and known open discussion points.
Generals/Code/GameEngine/Source/Common/RTS/Player.cpp	Makes `changeBattlePlan`'s `bonus` parameter `const` and moves mutation into a local `invertedBonus` copy, correctly enabling two back-to-back calls with the same pointer without data corruption.
GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp	Zero Hour mirror of the `const` refactoring to `changeBattlePlan` — clean, no new issues.

Sequence Diagram

sequenceDiagram
    participant Game as Game Engine
    participant Obj as Object
    participant BPU as BattlePlanUpdate
    participant Old as oldOwner (Player)
    participant New as newOwner (Player)

    Game->>Obj: onCapture(oldOwner, newOwner)
    Obj->>BPU: onCapture(oldOwner, newOwner)

    alt RETAIL_COMPATIBLE_CRC defined
        Note over BPU: No transfer — retail behavior preserved.<br/>oldOwner keeps bonus, newOwner gets nothing.
    else !RETAIL_COMPATIBLE_CRC
        BPU->>Old: changeBattlePlan(m_planAffectingArmy, -1, m_bonuses)
        alt oldOwner's plan count drops to 0
            Old->>Old: applyBattlePlanBonusesForPlayerObjects(invertedBonus)
            Note over Old: Bonus removed from army
        else count remains > 0
            Note over Old: Other strategy center still active, no change
        end

        BPU->>New: changeBattlePlan(m_planAffectingArmy, +1, m_bonuses)
        alt newOwner's plan count rises to 1
            New->>New: applyBattlePlanBonusesForPlayerObjects(m_bonuses)
            Note over New: Bonus applied to army
        else count was already > 0
            Note over New: Already had this plan active, no duplicate bonus
        end
    end

_{Last reviewed commit: "fix: Formatting"}

greptile-apps

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

greptile-apps

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp

greptile-apps · 2026-03-04T01:51:56Z

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

+void BattlePlanUpdate::onCapture(Player* oldOwner, Player* newOwner)
+{
+#if !RETAIL_COMPATIBLE_CRC
+	// TheSuperHackers @bugfix Stubbjax 04/11/2025 Transfer battle plan bonuses on capture.
+	oldOwner->changeBattlePlan(m_planAffectingArmy, -1, m_bonuses);
+	newOwner->changeBattlePlan(m_planAffectingArmy, 1, m_bonuses);
+#endif


Missing null pointer guard for oldOwner

The onDelete method (line ~194) has an explicit comment noting that the controlling player can legitimately be null during game reset cycles, and guards accordingly:

// however, player CAN legitimately be null during game reset cycles if( player && m_planAffectingArmy != PLANSTATUS_NONE )

The new onCapture doesn't apply the same safety check. If oldOwner is null (e.g., capturing from a neutral entity during a reset cycle), calling oldOwner->changeBattlePlan(...) will crash. newOwner should similarly be guarded.

Suggested change

void BattlePlanUpdate::onCapture(Player* oldOwner, Player* newOwner)

{

#if !RETAIL_COMPATIBLE_CRC

// TheSuperHackers @bugfix Stubbjax 04/11/2025 Transfer battle plan bonuses on capture.

oldOwner->changeBattlePlan(m_planAffectingArmy, -1, m_bonuses);

newOwner->changeBattlePlan(m_planAffectingArmy, 1, m_bonuses);

#endif

void BattlePlanUpdate::onCapture(Player* oldOwner, Player* newOwner)

{

#if !RETAIL_COMPATIBLE_CRC

// TheSuperHackers @bugfix Stubbjax 04/11/2025 Transfer battle plan bonuses on capture.

if( oldOwner )

oldOwner->changeBattlePlan(m_planAffectingArmy, -1, m_bonuses);

if( newOwner )

newOwner->changeBattlePlan(m_planAffectingArmy, 1, m_bonuses);

#endif

}

The same issue is present in GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp:251.

Prompt To Fix With AI

This is a comment left during a code review. Path: Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp Line: 251-257 Comment: **Missing null pointer guard for `oldOwner`** The `onDelete` method (line ~194) has an explicit comment noting that the controlling player *can legitimately be null* during game reset cycles, and guards accordingly: ```cpp // however, player CAN legitimately be null during game reset cycles if( player && m_planAffectingArmy != PLANSTATUS_NONE ) ``` The new `onCapture` doesn't apply the same safety check. If `oldOwner` is null (e.g., capturing from a neutral entity during a reset cycle), calling `oldOwner->changeBattlePlan(...)` will crash. `newOwner` should similarly be guarded. ```suggestion void BattlePlanUpdate::onCapture(Player* oldOwner, Player* newOwner) { #if !RETAIL_COMPATIBLE_CRC // TheSuperHackers @bugfix Stubbjax 04/11/2025 Transfer battle plan bonuses on capture. if( oldOwner ) oldOwner->changeBattlePlan(m_planAffectingArmy, -1, m_bonuses); if( newOwner ) newOwner->changeBattlePlan(m_planAffectingArmy, 1, m_bonuses); #endif } ``` The same issue is present in `GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp:251`. How can I resolve this? If you propose a fix, please make it concise.

Do we care for this?

Probably not - captures happen during active gameplay between two valid players, not during reset cycles like deletion.

The onDelete null guard exists because object cleanup can happen after the player reference is already cleared during game teardown. But onCapture fires during live gameplay when a player actively captures a building, so both owners should be valid.

However, it's a low-cost safety check (just two if statements) that makes the code more defensive. The real question is: can Strategy Centers spawn as neutral buildings that can be captured, or can they only be built by players?

If they're only player-built, the guards aren't needed. If there's any scenario where a neutral Strategy Center exists and can be captured, then oldOwner could be null.

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp

xezon

Looks risky.

xezon · 2026-03-04T18:41:46Z

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp

+		BattlePlanBonusesData invertedBonus = *bonus;
+
+		invertedBonus.m_armorScalar				= 1.0f / __max( bonus->m_armorScalar, 0.01f );
+		invertedBonus.m_sightRangeScalar		= 1.0f / __max( bonus->m_sightRangeScalar, 0.01f );


While at it, could remove all the excess tabs here.

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

xezon · 2026-03-04T18:47:54Z

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp

+void BattlePlanUpdate::onCapture(Player* oldOwner, Player* newOwner)
+{
+#if !RETAIL_COMPATIBLE_CRC
+	// TheSuperHackers @bugfix Stubbjax 04/11/2025 Transfer battle plan bonuses on capture.
+	oldOwner->changeBattlePlan(m_planAffectingArmy, -1, m_bonuses);
+	newOwner->changeBattlePlan(m_planAffectingArmy, 1, m_bonuses);
+#endif


Do we care for this?

xezon · 2026-03-04T18:51:30Z

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp

 	{
 		//First, inverse the bonuses
-		bonus->m_armorScalar				= 1.0f / __max( bonus->m_armorScalar, 0.01f );
-		bonus->m_sightRangeScalar		= 1.0f / __max( bonus->m_sightRangeScalar, 0.01f );


I do wonder, is this CRC safe?

This function is called like so:

player->changeBattlePlan( m_planAffectingArmy, -1, m_bonuses ); player->changeBattlePlan( PLANSTATUS_BOMBARDMENT, -1, m_bonuses ); player->changeBattlePlan( PLANSTATUS_HOLDTHELINE, -1, m_bonuses ); player->changeBattlePlan( PLANSTATUS_SEARCHANDDESTROY, -1, m_bonuses ); player->changeBattlePlan( PLANSTATUS_BOMBARDMENT, 1, m_bonuses ); player->changeBattlePlan( PLANSTATUS_HOLDTHELINE, 1, m_bonuses ); player->changeBattlePlan( PLANSTATUS_SEARCHANDDESTROY, 1, m_bonuses );

So m_bonuses will be written here by the original code.

But now with this change, m_bonuses will remain unmodified. Does this not carry the risk of mismatch? If not, why?

I just tested a multiplayer match between one client @ 7cb5a62 and another @ 6e41efa and did not observe any mismatches.

I suspect it does not mismatch because

void BattlePlanUpdate::setBattlePlan( BattlePlanStatus plan ) { ... m_bonuses->m_armorScalar = 1.0f; m_bonuses->m_sightRangeScalar = 1.0f; ...

This resets the scalars after every removal to restore a clean slate.

It could only mismatch if this reset was not happening after a battle plan removal.

xezon

Should be ok. Perhaps @Skyaero42 can run it over a few replays?

Needs rebase

Skyaero42 · 2026-03-18T08:40:50Z

Should be ok. Perhaps @Skyaero42 can run it over a few replays?

Needs rebase

Will do, give me a few days.

Stubbjax self-assigned this Feb 4, 2026

Stubbjax added Bug Something is not working right, typically is user facing Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour NoRetail This fix or change is not applicable with Retail game compatibility labels Feb 4, 2026

greptile-apps bot reviewed Feb 4, 2026

View reviewed changes

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp Outdated Show resolved Hide resolved

Stubbjax force-pushed the transfer-captured-battle-plans branch from 3c994e9 to 434df18 Compare February 4, 2026 16:14

greptile-apps bot reviewed Feb 4, 2026

View reviewed changes

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp Show resolved Hide resolved

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp Show resolved Hide resolved

xezon reviewed Feb 21, 2026

View reviewed changes

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp Outdated Show resolved Hide resolved

Generals/Code/GameEngine/Source/Common/RTS/Player.cpp Outdated Show resolved Hide resolved

Stubbjax force-pushed the transfer-captured-battle-plans branch from 434df18 to 6e41efa Compare March 4, 2026 01:48

greptile-apps bot reviewed Mar 4, 2026

View reviewed changes

xezon changed the title ~~bugfix: Battle plans are now transferred to the new owner on capture~~ bugfix(battleplan): Battle plans are now transferred to the new owner on capture Mar 4, 2026

xezon reviewed Mar 4, 2026

View reviewed changes

xezon approved these changes Mar 17, 2026

View reviewed changes

Stubbjax added 4 commits March 18, 2026 19:20

bugfix: Battle plans are now transferred to the new owner on capture

ba4f113

refactor: Use new allocation method

85a414b

docs: Remove obsolete comments

72575e2

fix: Formatting

2f6c72a

Stubbjax force-pushed the transfer-captured-battle-plans branch from 4794155 to 2f6c72a Compare March 18, 2026 08:28

Conversation

Stubbjax commented Feb 4, 2026 • edited by Skyaero42 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

xezon Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xezon Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Stubbjax Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

xezon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skyaero42 commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Stubbjax commented Feb 4, 2026 •

edited by Skyaero42

Loading

greptile-apps bot commented Feb 4, 2026 •

edited

Loading

xezon left a comment •

edited

Loading