bugfix(shroud): Fix bugged map shroud after Search and Destroy vision bonus is applied at saveload by Caball009 · Pull Request #2892 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-07-18T18:53:16Z

Fixes Map shroud is bugged when Strategy Center vision bonus is applied at saveload #2882
Follow-up for bugfix(energy): Don't increase power production for disabled power plants on save game load #2508

Prior to #2508 the object xfer loading process was as follows:

Call Object::setOrRestoreTeam -> Player::becomingTeamMember and apply battle plan bonuses if needed. If USA Search and Destroy was enabled it would increase the vision range for objects.
Xfer load the stored vision range data, overwriting the data set in step 1 (if any).

After #2508 that process was inverted which leads issues with the map shroud (see issue description). Object::setShroudClearingRange would be called (as part of S&D increased vision) when it shouldn't. Eventually Object::unlook would be called, which would previously return early originally because m_partitionLastLook wasn't xferred yet.

This PR adds an early return to Player::becomingTeamMember so that none of the battle plan bonuses are applied on object xfer load. My understanding is that all the object changes that are applied by the battle plans are already in the xferred data.

See commits for cleaner diffs.

TODO:

Replicate in Generals.

greptile-apps · 2026-07-18T18:55:15Z

Greptile Summary

This PR changes how object team membership effects run during save-load. The main changes are:

Adds an xfer-load flag to Player::becomingTeamMember.
Skips battle-plan and capture side effects during direct object team restoration.
Keeps idle-worker UI updates outside the save-load early return.
Re-enables a shroud undo queue assertion during partition manager load.

Confidence Score: 4/5

This is close, but the load-time assertion should be fixed before merging.

The direct object restore path now avoids reapplying serialized battle-plan vision data.
Other load-time team-change paths can still queue shroud undo work before the partition manager reads the saved queue.
That can make debug builds stop during valid save-load flows.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/Common/Player.h	Adds the optional save-load flag used by team membership handling.
GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp	Skips battle-plan and capture side effects during direct object restore while preserving idle-worker UI updates.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Passes the restore state into team membership notifications for direct object xfer.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp	Re-enables a load-time shroud queue assertion that can still fire for valid load-time team changes.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp:4740-4741
**Load assert still fires**

This assert can still fail during save-load for team changes that do not pass through the new `objectXferLoad` flag. The direct `Object::xfer` restore path now skips battle-plan effects, but other module load paths can call `Object::setTeam()`, which uses `setOrRestoreTeam(team, false)` and reaches `becomingTeamMember(..., objectXferLoad=false)`. With Search and Destroy active, that path can update shroud clearing, queue an undo shroud reveal, and hit this assertion before the saved queue is read. The assertion needs to stay tolerant unless every load-time team-change path is covered by the same suppression.

_{Reviews (3): Last reviewed commit: "Removed function parameter from 'setOrRe..." | Re-trigger Greptile}

xezon · 2026-07-19T08:11:09Z

+
+	// TheSuperHackers @bugfix Caball009 19/07/2026 Return early to avoid overwriting
+	// object data, e.g. the vision range, that may have been loaded during the xfer process.
+	if (objectXferLoad)


There is TheGameLogic->isLoadingSave()

It's still false here when I load a game. This function is also called from other places during loading (with objectXferLoad == false), so a single 'loading' flag is not enough.

xezon · 2026-07-19T08:24:14Z

+
+	// TheSuperHackers @bugfix Caball009 19/07/2026 Return early to avoid overwriting
+	// object data, e.g. the vision range, that may have been loaded during the xfer process.
+	if (objectXferLoad)


After #2508 that process was inverted which leads issues with the map shroud (see issue description). Object::setShroudClearingRange would be called (as part of S&D increased vision) when it shouldn't.

Can you explain why this breaks the shroud? I wonder if something else needs fixing that avoids breaking the shroud if this function is called, such as refreshing some state or so.

In short, SnD triggers a vision bonus, but this data is already xferred and causes incorrect shroud reveal undo calls.

I'm not entirely sure if I've got the entire picture, but here are some details:

bugfix(energy): Don't increase power production for disabled power plants on save game load #2508 moved the call to Object::setOrRestoreTeam, so that Object::m_partitionLastLook is now xferred prior to the call.

Search and Destroy provides objects with vision bonus. This triggers a call to update the shroud:

Object::handleShroud() Line 4916 Object::handlePartitionCellMaintenance() Line 4906 Object::setShroudClearingRange(float newShroudClearingRange) Line 5320 localApplyBattlePlanBonusesToObject(Object * obj, void * userData) Line 3598 Player::applyBattlePlanBonusesForObject(Object * obj) Line 3635 Player::becomingTeamMember(Object * obj, bool yes, bool objectXferLoad) Line 1061 Object::setOrRestoreTeam(Team * team, bool restoring, bool objectXferLoad) Line 931 Object::xfer(Xfer * xfer) Line 4267

Previously Object::unlook would return early because m_partitionLastLook->isInvalid() or m_howFar == 0.0f was still true, because that data wasn't xferred yet. But after bugfix(energy): Don't increase power production for disabled power plants on save game load #2508 m_partitionLastLook is already xferred at this point, so there's a call to PartitionManager::queueUndoShroudReveal.

Just to illustrate my findings in the previous message, the following appears to work as well:

if( xfer->getXferMode() == XFER_LOAD ) { // const Real look = m_partitionLastLook->m_howFar; m_partitionLastLook->m_howFar = 0.0f; // set to zero so that `Object::unlook` returns early assert(m_partitionLastLook->isInvalid()); // Team *team = TheTeamFactory->findTeamByID( teamID ); if( team == nullptr ) { DEBUG_CRASH(( "Object::xfer - Unable to load team" )); throw SC_INVALID_DATA; } const Bool restoring = true; setOrRestoreTeam( team, restoring, true ); // m_partitionLastLook->m_howFar = look; // }

Would it perhaps be possible to decouple the PartitionManager xfer and init ready states so that everything can be xferred at random order but the PartitionManager will not be accepting mutation until after the xfer?

I'm not sure what you mean by that.

My understanding from your write ups so far is that PartitionManager is loaded into good state by xfer, but is then set into broken state by a call to handlePartitionCellMaintenance. I do wonder why exactly handlePartitionCellMaintenance breaks its subsequent state and how it can be avoided that such a call breaks it, for example by not accepting any state mutation until after all xfer is completed.

Basically I think it is absurd that calling handlePartitionCellMaintenance will break it under some circumstances.

I don't think guarding the PartitionManager state in particular is the solution. I have re-enabled an assertion that should get triggered if that state was modified by the Object xferring, though.

Seems to me that the way to fix this is adding a guard as early as possible, long before it would reach to the PartitionManager state, which is what this PR does.

Caball009 · 2026-07-19T17:21:06Z

-			// have to remove this assert, because during load there is a setTeam call for each guy on a sub-team, and that results
-			// in a queued unlook, so we actually have stuff in here at the start.  I am fairly certain that setTeam should wait
-			// until loadPostProcess, but I ain't gonna change it now.
-//			DEBUG_ASSERTCRASH(m_pendingUndoShroudReveals.empty(), ("At load, we appear to not be in a reset state.") );


I strongly suspect the devs experienced the same issue at some point during development, though for the v1.04 code this assertion should not get triggered AFAICT.

I have removed the comment with the assumption that this assertion won't get triggered after this PR. If that's not true, the old comment could / should be restored again.

bobtista · 2026-07-19T20:13:10Z

@@ -901,7 +901,7 @@ void Object::setTemporaryTeam( Team *team )

 //=============================================================================
 //=============================================================================
-void Object::setOrRestoreTeam( Team* team, Bool restoring )
+void Object::setOrRestoreTeam( Team* team, Bool restoring, Bool objectXferLoad)


Do we need the third parameter on setOrRestoreTeam? Only Player::becomingTeamMember actually needs to know it's being called during a load - for setOrRestoreTeam, restoring always equals objectXferLoad right?

setOrRestoreTeam is also called from setTemporaryTeam / setTeam, which is called all over.

All of those paths go through setTemporaryTeam, which hardcodes restoring = false. So setOrRestoreTeam has two direct call sites, and the flags are equal at both - setTemporaryTeam passes (false, false), Object::xfer passes (true, true). Only xfer ever produces true, at least as of now. Anyway, I think it's more correct the way you have it, if someone adds a restoring caller later, we'd want the parameters this way.

That seems reasonable for now. Changed.

I wonder if objectXferLoad should be renamed now that it's just an alias for 'restoring'.

Yeah naming a parameter after who calls it rather than what it means bugs me. restoring works for me.

Moved idle worker code.

94fe5b8

greptile-apps Bot reviewed Jul 18, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp

xezon reviewed Jul 19, 2026

View reviewed changes

Caball009 commented Jul 19, 2026

View reviewed changes

bobtista reviewed Jul 19, 2026

View reviewed changes

Caball009 added 3 commits July 19, 2026 22:16

Fixed the bugged shroud on saveload.

582ea16

Re-added assertion and removed comment.

7ac7b20

Removed function parameter from 'setOrRestoreTeam'.

f54be6f

Caball009 force-pushed the fix_bugged_shroud_saveload branch from 7571db8 to f54be6f Compare July 19, 2026 22:19

greptile-apps Bot reviewed Jul 19, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp

Uh oh!

Conversation

Caball009 commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

Uh oh!

greptile-apps Bot commented Jul 18, 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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Caball009 commented Jul 18, 2026 •

edited

Loading

greptile-apps Bot commented Jul 18, 2026 •

edited

Loading