Skip to content

bugfix(battleplan): Battle plans are now transferred to the new owner on capture #2257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Stubbjax wants to merge 4 commits into
base: main
Choose a base branch
from
+48 −24
Open

bugfix(battleplan): Battle plans are now transferred to the new owner on capture #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba4f113
bugfix: Battle plans are now transferred to the new owner on capture
Stubbjax Feb 4, 2026
85a414b
refactor: Use new allocation method
Stubbjax Mar 4, 2026
72575e2
docs: Remove obsolete comments
Stubbjax Mar 4, 2026
2f6c72a
fix: Formatting
Stubbjax Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Generals/Code/GameEngine/Include/Common/Player.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ class Player : public Snapshot
//it's possible for multiple strategy centers to have the same plan, so we need
//to keep track of that like radar. Keep in mind multiple strategy centers with
//same plan do not stack, but different strategy centers with different plans do.
void changeBattlePlan( BattlePlanStatus plan, Int delta, BattlePlanBonusesData *bonus );
void changeBattlePlan( BattlePlanStatus plan, Int delta, const BattlePlanBonusesData *bonus );
Int getNumBattlePlansActive() const { return m_bombardBattlePlans + m_holdTheLineBattlePlans + m_searchAndDestroyBattlePlans; }
Int getBattlePlansActiveSpecific( BattlePlanStatus plan ) const;
void applyBattlePlanBonusesForObject( Object *obj ) const; //New object or converted object gaining our current battle plan bonuses.
Expand Down
1 change: 1 addition & 0 deletions Generals/Code/GameEngine/Include/GameLogic/Module/BattlePlanUpdate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ class BattlePlanUpdate : public UpdateModule, public SpecialPowerUpdateInterface
virtual void onObjectCreated() override;
virtual void onDelete() override;
virtual UpdateSleepTime update() override;
virtual void onCapture(Player* oldOwner, Player* newOwner) override;

virtual CommandOption getCommandOption() const override;
protected:
Expand Down
23 changes: 12 additions & 11 deletions Generals/Code/GameEngine/Source/Common/RTS/Player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2921,8 +2921,7 @@ Bool Player::doesObjectQualifyForBattlePlan( Object *obj ) const
}

//-------------------------------------------------------------------------------------------------
// note, bonus is an in-out parm.
void Player::changeBattlePlan( BattlePlanStatus plan, Int delta, BattlePlanBonusesData *bonus )
void Player::changeBattlePlan( BattlePlanStatus plan, Int delta, const BattlePlanBonusesData *bonus )
{
DUMPBATTLEPLANBONUSES(bonus, this, nullptr);
Bool addBonus = false;
Expand Down Expand Up @@ -2976,22 +2975,24 @@ void Player::changeBattlePlan( BattlePlanStatus plan, Int delta, BattlePlanBonus
else if( removeBonus )
{
//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 );
Copy link

@xezon xezon Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@Stubbjax Stubbjax Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@xezon xezon Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if( bonus->m_bombardment > 0 )
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);
if (invertedBonus.m_bombardment > 0)
{
bonus->m_bombardment = -1;
invertedBonus.m_bombardment = -1;
}
if( bonus->m_holdTheLine > 0 )
if (invertedBonus.m_holdTheLine > 0)
{
bonus->m_holdTheLine = -1;
invertedBonus.m_holdTheLine = -1;
}
if( bonus->m_searchAndDestroy > 0 )
if (invertedBonus.m_searchAndDestroy > 0)
{
bonus->m_searchAndDestroy = -1;
invertedBonus.m_searchAndDestroy = -1;
}

applyBattlePlanBonusesForPlayerObjects( bonus );
applyBattlePlanBonusesForPlayerObjects(&invertedBonus);
}
}

Expand Down
10 changes: 10 additions & 0 deletions Generals/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,16 @@ void BattlePlanUpdate::onObjectCreated()
enableTurret( false );
}

//-------------------------------------------------------------------------------------------------
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
Comment on lines +251 to +257
Copy link

@greptile-apps greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@xezon xezon Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care for this?

Copy link

@greptile-apps greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

//-------------------------------------------------------------------------------------------------
Bool BattlePlanUpdate::initiateIntentToDoSpecialPower(const SpecialPowerTemplate *specialPowerTemplate, const Object *targetObj, const Coord3D *targetPos, const Waypoint *way, UnsignedInt commandOptions )
{
Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Include/Common/Player.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ class Player : public Snapshot
//it's possible for multiple strategy centers to have the same plan, so we need
//to keep track of that like radar. Keep in mind multiple strategy centers with
//same plan do not stack, but different strategy centers with different plans do.
void changeBattlePlan( BattlePlanStatus plan, Int delta, BattlePlanBonusesData *bonus );
void changeBattlePlan( BattlePlanStatus plan, Int delta, const BattlePlanBonusesData *bonus );
Int getNumBattlePlansActive() const { return m_bombardBattlePlans + m_holdTheLineBattlePlans + m_searchAndDestroyBattlePlans; }
Int getBattlePlansActiveSpecific( BattlePlanStatus plan ) const;
void applyBattlePlanBonusesForObject( Object *obj ) const; //New object or converted object gaining our current battle plan bonuses.
Expand Down
1 change: 1 addition & 0 deletions GeneralsMD/Code/GameEngine/Include/GameLogic/Module/BattlePlanUpdate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ class BattlePlanUpdate : public SpecialPowerUpdateModule
virtual void onObjectCreated() override;
virtual void onDelete() override;
virtual UpdateSleepTime update() override;
virtual void onCapture(Player* oldOwner, Player* newOwner) override;

virtual CommandOption getCommandOption() const override;
protected:
Expand Down
23 changes: 12 additions & 11 deletions GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3414,8 +3414,7 @@ Bool Player::doesObjectQualifyForBattlePlan( Object *obj ) const
}

//-------------------------------------------------------------------------------------------------
// note, bonus is an in-out parm.
void Player::changeBattlePlan( BattlePlanStatus plan, Int delta, BattlePlanBonusesData *bonus )
void Player::changeBattlePlan( BattlePlanStatus plan, Int delta, const BattlePlanBonusesData *bonus )
{
DUMPBATTLEPLANBONUSES(bonus, this, nullptr);
Bool addBonus = false;
Expand Down Expand Up @@ -3469,22 +3468,24 @@ void Player::changeBattlePlan( BattlePlanStatus plan, Int delta, BattlePlanBonus
else if( removeBonus )
{
//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 );
if( bonus->m_bombardment > 0 )
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);
if (invertedBonus.m_bombardment > 0)
{
bonus->m_bombardment = -1;
invertedBonus.m_bombardment = -1;
}
if( bonus->m_holdTheLine > 0 )
if (invertedBonus.m_holdTheLine > 0)
{
bonus->m_holdTheLine = -1;
invertedBonus.m_holdTheLine = -1;
}
if( bonus->m_searchAndDestroy > 0 )
if (invertedBonus.m_searchAndDestroy > 0)
{
bonus->m_searchAndDestroy = -1;
invertedBonus.m_searchAndDestroy = -1;
}

applyBattlePlanBonusesForPlayerObjects( bonus );
applyBattlePlanBonusesForPlayerObjects(&invertedBonus);
}
}

Expand Down
10 changes: 10 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BattlePlanUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,16 @@ void BattlePlanUpdate::onObjectCreated()
enableTurret( false );
}

//-------------------------------------------------------------------------------------------------
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
}

//-------------------------------------------------------------------------------------------------
Bool BattlePlanUpdate::initiateIntentToDoSpecialPower(const SpecialPowerTemplate *specialPowerTemplate, const Object *targetObj, const Coord3D *targetPos, const Waypoint *way, UnsignedInt commandOptions )
{
Expand Down
Loading