bugfix: Fix transferred in-progress upgrades

[Stubbjax]

This change fixes two issues with in-progress upgrades when they are transferred to another player.

1. If a team mate surrenders, the surviving team mate's m_upgradesInProgress mask does not append any of the surrendering player's in-progress upgrades. This incorrectly allows the surviving player to purchase a duplicate instance of any upgrades that the surrendering player had in progress.

   For example, Bob starts researching Black Napalm and then surrenders. Fred selects a different War Factory and is able to research a new/separate instance of Black Napalm, despite it already being in progress in another War Factory.

2. The second problem arises when it comes to transferring in-progress upgrades that the surviving player already has in progress or completed. Once an upgrade has been completed, any duplicate in-progress upgrades can no longer be cancelled, and attempting to do so leads to strange UI behaviour.

   For example, Bob starts researching Black Napalm and then surrenders. But Fred already has Black Napalm researched. Fred discovers the second instance of Black Napalm that Bob had begun researching and tries to cancel it, but the upgrade cannot be cancelled.

The solution is to cancel any of the old player's in-progress upgrades if the new player already has them in progress or completed (and the refund is a nice bonus). The m_upgradesInProgress mask is also appropriately updated so that the new player cannot purchase any duplicate upgrades that the old player had in progress.

Before

UPG_BROKEN.mp4

After

UPG_FIXED.mp4

[greptile-apps]

Greptile Summary

This PR fixes two related bugs in Player::transferAssetsFromThat — the code path executed when a teammate surrenders:

1. Duplicate upgrade prevention — that player's m_upgradesInProgress is now OR'd into this player's mask, so the surviving player can no longer start a second instance of an upgrade that the surrendered player already had in-flight.
2. Un-cancellable upgrade fix — if the surviving player already owns (in-progress or completed) an upgrade that the surrendering player was also researching, the duplicate is now cancelled (with a refund) before the objects are transferred, preventing a dangling un-cancellable production entry.

Both changes are gated under #if !RETAIL_COMPATIBLE_CRC to avoid altering replay-compatible CRC behaviour. The fixes are applied symmetrically across both Generals/ and GeneralsMD/. The core logic — build a cancel list, fire iterateObjects with the cancelUpgradeInProduction callback, then OR the updated in-progress mask — is sound: BitFlags::set(const BitFlags&) performs a bitwise OR (not a replace), and ProductionUpdate::cancelUpgrade calls Player::removeUpgrade which correctly clears the cancelled bit from that->m_upgradesInProgress before the mask is merged.

Key points:

• The static helper cancelUpgradeInProduction is defined outside the #if !RETAIL_COMPATIBLE_CRC guard in both files, making it dead code in retail builds and likely to produce -Wunused-function warnings. (Previously noted in review threads.)
• Object transfers happen after the cancellation + mask-merge block, so getControllingPlayer() inside the callback correctly resolves to that at cancellation time.
• The include of ProductionUpdate.h is necessary and correctly added to both translation units.

Confidence Score: 4/5

• Safe to merge — the fix is logically correct and well-ordered; the only outstanding concern (dead code outside the preprocessor guard) is a compiler-warning issue already flagged in prior review threads.
• The upgrade-cancellation logic is correct: the cancel list is populated before any transfers, iterateObjects is called on that's still-owned objects, removeUpgrade clears bits from that->m_upgradesInProgress before the mask OR, and the BitFlags::set implementation performs a non-destructive OR. No new critical logic bugs were found beyond what was already discussed in prior threads. Score is 4 (not 5) because the cancelUpgradeInProduction function remains defined outside the #if !RETAIL_COMPATIBLE_CRC guard in both files, which will silently produce dead code / compiler warnings in retail builds.
• No files require special attention beyond the already-discussed dead-code placement of cancelUpgradeInProduction outside the preprocessor guard in both Player.cpp files.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/Common/RTS/Player.cpp	Adds cancelUpgradeInProduction callback and pre-transfer logic to cancel/mask duplicate in-progress player upgrades; cancelUpgradeInProduction is defined outside the #if !RETAIL_COMPATIBLE_CRC guard despite only being used inside it (dead code in retail builds).
GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp	Mirrors the Generals fix for Zero Hour — same cancelUpgradeInProduction dead-code placement concern outside #if !RETAIL_COMPATIBLE_CRC guard, otherwise correct logic.

Sequence Diagram

sequenceDiagram
    participant TA as transferAssetsFromThat()
    participant ThatPlayer as that (surrendering)
    participant ThisPlayer as this (surviving)
    participant PUI as ProductionUpdateInterface
    participant PM as Player::removeUpgrade

    TA->>ThatPlayer: iterate m_upgradeList
    ThatPlayer-->>TA: upgrades with UPGRADE_STATUS_IN_PRODUCTION

    loop for each in-progress upgrade
        TA->>ThisPlayer: hasUpgradeComplete(upgrade)?
        TA->>ThisPlayer: hasUpgradeInProduction(upgrade)?
        alt this player already has it
            TA->>TA: add to upgradesToCancel
        end
    end

    loop for each upgrade in upgradesToCancel
        TA->>ThatPlayer: iterateObjects(cancelUpgradeInProduction, upgradeTemplate)
        ThatPlayer->>PUI: isUpgradeInQueue(upgradeTemplate)?
        PUI-->>ThatPlayer: true
        ThatPlayer->>PUI: cancelUpgrade(upgradeTemplate)
        PUI->>PM: removeUpgrade(upgrade) on that player
        PM->>ThatPlayer: m_upgradesInProgress.clear(mask)
    end

    TA->>ThisPlayer: m_upgradesInProgress.set(that->m_upgradesInProgress)
    Note over ThisPlayer: OR in that's remaining in-progress bits

    TA->>ThatPlayer: iterate objects for transfer
    ThatPlayer-->>TA: list of objects
    TA->>ThisPlayer: setTeam(defaultTeam) for each object

    TA->>ThatPlayer: getMoney()->withdraw(allMoney)
    TA->>ThisPlayer: getMoney()->deposit(allMoney)

Loading

_{Last reviewed commit: "fix: Add static cast"}

[xezon]

Nice find and the implementation of the fix makes sense too.

[xezon]

This will not compile in VC6.

[xezon]

static_cast

[xezon]

void* cast should be implicit