bugfix: Preserve occupants when transferring assets to allies

[Stubbjax]

This change preserves contained occupants when a player's assets are transferred to an ally player.

Transferred assets run through the onCapture logic, which features safeguards against weird circumstances like enemy occupants being left inside a captured building or vehicle. However, this does not take assets transferred to allies on surrender into consideration, in which case preserved containment is always desirable*. Also note that not all occupants are consistently evacuated (at least prior to #1885).

*This change does leave a possible scenario where a container object is transferred to an ally under different circumstances, such as via a scripting event. In such cases, any of the original owner's occupants would hypothetically remain inside the container rather than being ejected, which might cause weird or unexpected behaviour.

I'm thinking it might be necessary to implement an optional capture path / reason into the onCapture signatures and make the change conditional on that reason.

void TransportContain::onCapture(Player* oldOwner, Player* newOwner, CaptureType captureType = CT_DEFAULT)
{
  if (captureType = CT_TRANSFER_ALLIES)
  {
    // ...
  }
}

Or we could simply ignore such scenarios; they might not even be valid and they're certainly not likely. I'm open to suggestions.

Before

Units are evacuated when transferred to an ally

YES_EVAC.mp4

And with #1885 applied:
https://github.com/user-attachments/assets/7ee1b10b-4d9e-4eac-848d-6ff71622ceec

After

Units remain contained when transferred to an ally

NO_EVAC.mp4

And with #1885 applied:
https://github.com/user-attachments/assets/adbcf7f1-5fa2-49dc-b614-bddc9d8804a5

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Preserve transport occupants when assets transfer to allied players

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Prevent allied ownership transfers from ejecting passengers from transports.
• Keep retail-compatible CRC builds unchanged by retaining forced passenger evacuation.
• Apply the fix consistently to both Generals and GeneralsMD engine trees.

Diagram

graph TD
  A["Asset ownership change"] --> B[["TransportContain::onCapture"]] --> C{"Unmanned?"}
  C -- "yes" --> D["removeAllContained()"]
  C -- "no" --> E{"Retail CRC build?"}
  E -- "yes" --> G["orderAllPassengersToExit"]
  E -- "no" --> F{"Old/New are allies?"}
  F -- "no" --> G
  F -- "yes" --> H["Preserve containment"]

  subgraph Legend
    direction LR
    _sub[["Function"]] ~~~ _dec{"Decision"} ~~~ _proc["Action"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add explicit capture/transfer reason (CaptureType) to onCapture

• ➕ Makes allied-transfer behavior explicit and future-proof (e.g., surrender vs script-driven transfers).
• ➕ Avoids overloading 'ALLIES' relationship as a proxy for 'safe to preserve occupants'.
• ➕ Can centralize rules for other contain modules (garrison/tunnel/etc.) if needed.
• ➖ Requires signature changes and call-site updates across capture/transfer code paths.
• ➖ Higher risk of regressions and larger diff than this targeted fix.

2. Split entry points: onCapture() vs onTransferToAlly()

• ➕ Keeps existing capture semantics intact and isolates ally-transfer behavior.
• ➕ Minimal branching/conditionals inside onCapture.
• ➖ Still needs new call paths and plumbing to choose the right hook.
• ➖ May duplicate logic across hooks unless refactored.

3. Gate the behavior behind a rule/config flag

• ➕ Allows modders/projects to choose between retail-like evacuation and ally-preserve behavior.
• ➕ Reduces risk if some game modes rely on the old ejection behavior.
• ➖ Adds configuration surface area and testing matrix.
• ➖ Does not address the conceptual ambiguity between capture vs transfer events.

Recommendation: The current approach is a pragmatic, low-risk fix for the surrender/ally-transfer bug, especially since RETAIL_COMPATIBLE_CRC builds remain unchanged. If future gameplay/scripts can transfer containers to allies in contexts where ejection is desired, consider introducing an explicit transfer/capture reason (CaptureType) rather than inferring intent from team relationship.

Files changed (2) +10 / -0

Bug fix (2) +10 / -0

TransportContain.cppSkip passenger ejection on allied transfers (non-retail builds) +5/-0

Skip passenger ejection on allied transfers (non-retail builds)

• Adjusts TransportContain::onCapture to only order passengers to exit when the new owner is not an ally. Retains the existing unconditional ejection behavior under RETAIL_COMPATIBLE_CRC.

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp

TransportContain.cppMirror allied-transfer containment fix for GeneralsMD +5/-0

Mirror allied-transfer containment fix for GeneralsMD

• Applies the same onCapture relationship gate to prevent evacuating passengers when ownership transfers to an allied player. Keeps the RETAIL_COMPATIBLE_CRC path unchanged.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Mixed-owner passengers preserved 🐞 Bug ≡ Correctness

Description

When the new ALLIES check skips orderAllPassengersToExit, a transport can remain containing
passengers owned by a different (allied) player if only the container is transferred (e.g., scripted
single-object transfers). This violates TransportContain’s own containment rule that riders must
have the same controlling player as the transport.

Code

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[R509-510]

+			if (oldOwner->getRelationship(newOwner->getDefaultTeam()) != ALLIES)
+				orderAllPassengersToExit(CMD_FROM_AI);

Evidence

TransportContain enforces that contained riders must share the transport’s controlling player (entry
invariant). ScriptActions can transfer only the transport via setTeam without transferring its
contained riders, and the PR change now skips evacuation for allies, allowing this invariant to be
violated after such transfers.

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[494-513]
Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[149-187]
Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp[2769-2785]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new ally-only capture path preserves containment by skipping evacuation, but it can leave occupants owned by the old player inside the newly owned transport in cases where only the container is transferred (not the passengers). This contradicts `TransportContain::isValidContainerFor`, which enforces “only our own units can be transported.”

### Issue Context
`ScriptActions::doNamedTransferAssetsToPlayer` transfers only the named object via `setTeam(...)` and does not transfer contained riders. With the new onCapture behavior, allied transfers no longer force a clean state via evacuation.

### Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[494-513]
- Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[149-187]
- Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp[2769-2785]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[635-655]
- GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp[2865-2881]

### Suggested fix
Pick one consistent rule and enforce it:
- **Option A (keep invariant):** If `oldOwner` and `newOwner` are allies and you skip evacuation, also iterate contained riders and `setTeam(getObject()->getTeam())` (or `newOwner->getDefaultTeam()`) so the transport never contains units from a different controlling player.
- **Option B (scope the change):** Keep the old evacuation behavior for ally captures unless you can determine the capture is part of bulk asset transfer where riders will also be transferred.

Implement the same fix in both Generals and GeneralsMD TransportContain.cpp to keep behavior consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Fix compiled out by default 🐞 Bug ⚙ Maintainability

Description

The new ALLIES-based suppression of passenger ejection is under #else of `#if
RETAIL_COMPATIBLE_CRC, but RETAIL_COMPATIBLE_CRC` defaults to 1, so default builds still always
eject passengers on capture. This makes the PR’s intended behavior change inactive unless the build
overrides that macro.

Code

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[R506-508]

+#if RETAIL_COMPATIBLE_CRC
			orderAllPassengersToExit( CMD_FROM_AI );
+#else

Evidence

TransportContain::onCapture only uses the new ALLIES check in the #else branch, while
RETAIL_COMPATIBLE_CRC is defined as 1 by default in GameDefines.h, so the new logic is not
compiled into default builds.

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[494-513]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[635-655]
Core/GameEngine/Include/Common/GameDefines.h[23-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The ally-transfer behavior change is unreachable in default builds because it is compiled only when `RETAIL_COMPATIBLE_CRC` is disabled, while the default project setting enables it.

### Issue Context
This can lead to confusion during testing/usage: developers may expect the new behavior, but still observe passenger ejection.

### Fix Focus Areas
- Core/GameEngine/Include/Common/GameDefines.h[23-36]
- Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[494-513]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp[635-655]

### Suggested fix
Do one of:
1) If the behavior should *not* apply under CRC-compatibility, add an explicit comment near the `#if RETAIL_COMPATIBLE_CRC` block stating the fix is intentionally disabled in retail-compatible builds and how to enable it.
2) If the behavior *should* apply by default, move the ALLIES check outside the `RETAIL_COMPATIBLE_CRC` guard (accepting the compatibility break) or adjust the project macro defaults accordingly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Skyaero42]

This looks good to me

[xezon]

*This change does leave a possible scenario where a container object is transferred to an ally under different circumstances, such as via a scripting event. In such cases, any of the original owner's occupants would hypothetically remain inside the container rather than being ejected, which might cause weird or unexpected behaviour.

Can we continue to eject if the passengers do not match the containers team after capture? That would solve the edge case.

[bobtista]

*This change does leave a possible scenario where a container object is transferred to an ally under different circumstances, such as via a scripting event. In such cases, any of the original owner's occupants would hypothetically remain inside the container rather than being ejected, which might cause weird or unexpected behaviour.

Can we continue to eject if the passengers do not match the containers team after capture? That would solve the edge case.

I don’t think we can reliably check that here. transferAssetsFromThat() transfers objects one at a time, so the container could be processed before its passengers and temporarily appear mismatched. Could we pass an explicit transfer reason, eg CT_TRANSFER_ALL_ASSETS? Or expose a temporary bulk-transfer target on the source player and only preserve occupants when it matches newOwner? That would avoid changing every onCapture() signature while keeping individual scripted transfers on the existing behavior

[xezon]

So does this change need more work or not? It is not clear to me.

[Stubbjax]

So does this change need more work or not? It is not clear to me.

It depends if we're okay with the minuscule possibility that a map script transfers a contained unit to an ally. Covering this one scenario introduces quite a bit of complexity.

[xezon]

If it is technically possible then it better be handled at some point.

[Skyaero42]

It depends if we're okay with the minuscule possibility that a map script transfers a contained unit to an ally. Covering this one scenario introduces quite a bit of complexity.

I think a contained unit should not be transferable to an ally. The script should fail (i.e. do nothing) at that point.
That would fall outside of the scope of this PR, so recommend - if agreed - to make an issue for it.

[xezon]

Bugfix comment is missing