fix(saveload): Use getFinalOverride in WeaponSet xfer load to match Object constructor

[bobtista]

Summary

Fixes weapon timing state corruption after loading a saved game by ensuring WeaponSet uses the same template override as Object.

Notes

• Object constructor calls getFinalOverride() on the template (Object.cpp:230)
• WeaponSet::xfer(LOAD) was using TheThingFactory->findTemplate() which returns the base template, not the final override
• This caused m_curWeaponTemplateSet to point to a different ThingTemplate's weapon sets than obj->getTemplate()
• The pointer mismatch in updateWeaponSet() triggered unnecessary weapon reallocation, resetting timing state

Fix

Add getFinalOverride() call in WeaponSet::xfer(LOAD) to match what Object does, ensuring consistent template pointers.

Testing

• Save game with units that have weapons mid-reload
• Load the save and verify weapons maintain their reload state
• Verify normal weapon switching still works correctly

Todo

• Replicate to Generals

[greptile-apps]

Greptile Summary

This PR fixes weapon timing state corruption on savegame load by ensuring WeaponSet::xfer(LOAD) resolves the same final template override that the Object constructor uses. Previously, findTemplate() returned the base template, causing m_curWeaponTemplateSet to point to a different template's weapon sets than obj->getTemplate(), which triggered spurious reallocation in updateWeaponSet() and reset reload timing.

• Adds tt = static_cast<const ThingTemplate*>(tt->getFinalOverride()) after the existing null check in the LOAD branch, directly mirroring the pattern at Object.cpp:211.
• The fix is correctly replicated to both Generals/ and GeneralsMD/ codebases.
• getFinalOverride() always returns at least this (never nullptr), so no additional null guard is needed after the call.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted fix confined to the XFER_LOAD branch with no impact on the save path or normal gameplay codepath.

The one-liner mirrors an identical pattern already used in Object.cpp, getFinalOverride() is guaranteed non-null by its implementation (returns this when no override exists), the existing null check on tt before the call is preserved, and the fix is correctly duplicated to both game codebases.

No files require special attention.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/Object/WeaponSet.cpp	Adds getFinalOverride() call in XFER_LOAD path to match Object constructor behavior, preventing template pointer mismatch and spurious weapon reallocation on savegame load.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/WeaponSet.cpp	Identical fix replicated to Zero Hour (GeneralsMD), maintaining parity between the two game codebases.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant XF as Xfer (LOAD)
    participant WS as WeaponSet::xfer
    participant TF as TheThingFactory
    participant TT as ThingTemplate (base)
    participant FO as ThingTemplate (final override)

    XF->>WS: xfer(LOAD)
    WS->>TF: findTemplate(ttName)
    TF-->>WS: tt (base template)
    Note over WS: Before fix: used base tt directly
    WS->>TT: getFinalOverride()
    TT-->>WS: tt (final override)
    Note over WS: After fix: use final override tt
    WS->>FO: findWeaponTemplateSet(wsFlags)
    FO-->>WS: m_curWeaponTemplateSet
    Note over WS: Now matches Object constructor's template pointer

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant XF as Xfer (LOAD)
    participant WS as WeaponSet::xfer
    participant TF as TheThingFactory
    participant TT as ThingTemplate (base)
    participant FO as ThingTemplate (final override)

    XF->>WS: xfer(LOAD)
    WS->>TF: findTemplate(ttName)
    TF-->>WS: tt (base template)
    Note over WS: Before fix: used base tt directly
    WS->>TT: getFinalOverride()
    TT-->>WS: tt (final override)
    Note over WS: After fix: use final override tt
    WS->>FO: findWeaponTemplateSet(wsFlags)
    FO-->>WS: m_curWeaponTemplateSet
    Note over WS: Now matches Object constructor's template pointer

Loading

_{Reviews (4): Last reviewed commit: "fix(savegame): Replicate WeaponSet getFi..." | Re-trigger Greptile}

[xezon]

My first hunch here is that this change is a hack. Why is m_curWeaponTemplateSet set to something that satisfies the weapon flags but is not actually the real deal? It indicates that the issue is higher up.

[bobtista]

Ok, updated with a better approach.
When an Object is created (Object.cpp:230):
tt = (const ThingTemplate*)tt->getFinalOverride();
The Object uses the final override of the template.

But when WeaponSet::xfer(LOAD) restored weapon data (WeaponSet.cpp:231):
const ThingTemplate* tt = TheThingFactory->findTemplate(ttName);
m_curWeaponTemplateSet = tt->findWeaponTemplateSet(wsFlags);
It used findTemplate() which returns the base template, not the final override.

The fix is to just add t = (const ThingTemplate*)tt->getFinalOverride(); to WeaponSet::xfer(LOAD):

[Caball009]

Looks like a good change, but the title and issue description need to be updated (the summary is no longer in line with the current code change).

[bobtista]

Looks like a good change, but the title and issue description need to be updated (the summary is no longer in line with the current code change).

Updated

[bobtista]

Rebased again, this is ready for re-review

[xezon]

Was there any observable bug from this after save load? Are there potentially more of these mistakes in the code base?

[bobtista]

Was there any observable bug from this after save load? Are there potentially more of these mistakes in the code base?

Yes, but it requires an active template override, eg from mission or map INI data.
ThingFactory::newOverride() copies the final template with *newTemplate = *child, giving the override its own WeaponTemplateSet objects at different addresses while retaining the same template name. findTemplate() still returns the base template because the override is linked through setNextOverride() rather than added to the hash map. Conversely, obj->getTemplate() resolves to the final override through OVERRIDE.

Before this fix, WeaponSet::xfer(LOAD) therefore restored m_curWeaponTemplateSet from the base template. The next updateWeaponSet() compared that pointer with the equivalent set belonging to the final override and treated it as a genuine weapon-set change. It reallocated every weapon and called loadAmmoNow(), replacing the ammo and reload timing restored from the save. If the set does not use WeaponLockSharedAcrossSets, it also releases the weapon lock and selects PRIMARY_WEAPON.

Are there others? Just ScoreKeeper::xferObjectCountMap(). It reconstructs its std::map<const ThingTemplate *, Int> keys using findTemplate() (the base), while addObjectBuilt() and the related methods key the maps using o->getTemplate() (the final override). After loading, subsequent events for an overridden template can therefore create separate base and final-override entries. The problem appears after another save/load cycle. Both entries are saved under the same template name, and loading uses (*map)[thingTemplate] = count, so one entry silently overwrites the other. Could make another PR if it's worth it.

The other close cases I checked already account for overrides: GameClient::xfer() compares final overrides when rebinding drawables, AttackPriorityMap normalizes both insertion and lookup, and matching code uses isEquivalentTo() where appropriate. Other load-time template lookups are passed into newObject() or are not subsequently used as identity keys.

[xezon]

The LLM answer is not good for human consumption. What is the repro in game to see the issue before this fix?

[xezon]

Should findTemplate() maybe return the final override template?

[bobtista]

Should findTemplate() maybe return the final override template?

I don't think so. Map overrides are deleted by ThingFactory::reset(), while several callers cache findTemplate() results in function-local statics (GenericBridge, GenericDebris, and GarrisonGun). Returning the final override would leave those pointers dangling after a match where one was overridden.
The model seems to be: findTemplate() returns the base identity, while OVERRIDE resolves dynamically when accessing object templates. So resolving explicitly in WeaponSet::xfer() is consistent with that model.

Also ScoreKeeper::xferObjectCountMap() has the same base/final mismatch and should probably be fixed separately.

[xezon]

Can you add some code comments where appropriate to document the patterns to avoid future confusions?

[bobtista]

Can you add some code comments where appropriate to document the patterns to avoid future confusions?

done