bugfix(contain): Occupants killed by their containers now kill their occupants

[Stubbjax]

Closes #184

This change fixes an issue where occupants killed via the destruction of their container do not kill their occupants, and so on. For example, if a Chinook containing a Troop Crawler dies, the Red Guards inside the Troop Crawler survive and fall to the ground, which looks silly and can leave them floating in the air.

Before

A Troop Crawler dies inside a Chinook, but its occupants survive and fall to the ground or float in the air

YES_SURVIVORS.mp4

After

A Troop Crawler dies inside a Chinook, and kills its own occupants as a result

NO_SURVIVORS.mp4

[greptile-apps]

Greptile Summary

This PR fixes a nested-container death propagation bug: when a transport (e.g. Troop Crawler) dies while held inside an outer container (e.g. Chinook), its own occupants (e.g. Red Guards) were previously free to exit and would fall to the ground or float in the air. The fix has two complementary parts:

• Reorder killRidersWhoAreNotFreeToExit before processDamageToContained (non-retail only, in OpenContain::onDie): this ensures that riders are killed — and their own onDie cascades fire — before the container attempts to apply proportional damage, allowing the recursive kill to propagate down the container hierarchy.
• DISABLED_HELD check in TransportContain::isSpecificRiderFreeToExit (non-retail only): if the immediate parent container of a rider has the DISABLED_HELD flag set (i.e., it is itself being carried), then the rider is considered not free to exit and will be killed rather than released. This cleanly handles the recursive case: Red Guards see their container (Troop Crawler) is DISABLED_HELD, so they cannot exit.

Additional changes include:

• processDamageToContained signature updated across ContainModule, OpenContain (header + both impls) to pass percentDamage explicitly, removing the implicit data->m_damagePercentageToUnits lookup inside the function.
• killContained boolean pre-computed from percentDamage == 1.0f to avoid repeating the floating-point comparison.
• Non-retail killRidersWhoAreNotFreeToExit (GeneralsMD) now uses DEATH_BURNED / DEATH_NORMAL based on m_isBurnedDeathToUnits to prevent infantry corpse drop-out.

Confidence Score: 4/5

• Safe to merge with a minor concern about redundant damage application to already-dead objects in the non-retail path.
• The core logic is sound: the DISABLED_HELD check correctly identifies the nested-container scenario, and the reordering in onDie properly triggers recursive death propagation. The main minor concern is that in the non-retail path, riders killed by killRidersWhoAreNotFreeToExit() (whose removal is deferred) will still be present in the contain list when processDamageToContained() runs, causing a redundant attemptDamage call on already-dead objects. This is unlikely to produce visible bugs but is unnecessary work and may trigger on-damage side-effects. The onRemoving/onRemovedFrom cleanup path is not doubled since the deferred-removal design ensures each dead object is cleaned up exactly once by processDamageToContained.
• Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp and GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp — the non-retail processDamageToContained path processes objects that may already be dead after the new killRidersWhoAreNotFreeToExit pre-call.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Include/GameLogic/Module/ContainModule.h	Pure-virtual signature of processDamageToContained updated to accept a Real percentDamage parameter, aligning it with the GeneralsMD version. Minimal change, no issues found.
Generals/Code/GameEngine/Include/GameLogic/Module/OpenContain.h	Override declaration of processDamageToContained updated to match the new base signature. No logic, no issues found.
Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	In onDie, killRidersWhoAreNotFreeToExit is moved before processDamageToContained in non-retail builds to trigger nested container death propagation. processDamageToContained now accepts and uses percentDamage directly (removing the now-unused data variable). Redundant attemptDamage calls on already-dead objects may occur in the non-retail path after the reordering.
Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp	Added DISABLED_HELD check in isSpecificRiderFreeToExit (non-retail only): if the rider's direct container is held, the rider is not free to exit, preventing occupants of nested containers from escaping when the outer container dies.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Same onDie reordering as the Generals version. killContained optimization added to avoid repeating the float comparison. Retains data variable for m_isBurnedDeathToUnits usage. Same dead-object redundant-damage concern applies.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp	Non-retail killRidersWhoAreNotFreeToExit now calls obj->kill(DAMAGE_UNRESISTABLE, DEATH_BURNED/NORMAL) to prevent infantry corpses from falling out of containers. Same DISABLED_HELD check added to isSpecificRiderFreeToExit as in the Generals version. Both changes are clean.

Sequence Diagram

sequenceDiagram
    participant Chinook as Chinook (OpenContain/onDie)
    participant TC_KR as Chinook killRidersWhoAreNotFreeToExit
    participant TroopCrawler as TroopCrawler (TransportContain)
    participant TC_IS as TroopCrawler isSpecificRiderFreeToExit
    participant TC_KR2 as TroopCrawler killRidersWhoAreNotFreeToExit
    participant RedGuard as Red Guard

    Note over Chinook: Chinook dies (#if !RETAIL_COMPATIBLE_CRC path)
    Chinook->>TC_KR: killRidersWhoAreNotFreeToExit()
    TC_KR->>TroopCrawler: isSpecificRiderFreeToExit(TroopCrawler)?
    TroopCrawler->>TC_IS: check AI + DISABLED_HELD on Chinook
    TC_IS-->>TC_KR: FALSE (Chinook may be DISABLED_HELD)
    TC_KR->>TroopCrawler: obj->kill(DAMAGE_UNRESISTABLE, DEATH_BURNED/NORMAL)
    Note over TroopCrawler: TroopCrawler onDie fires
    TroopCrawler->>TC_KR2: killRidersWhoAreNotFreeToExit()
    TC_KR2->>RedGuard: isSpecificRiderFreeToExit(RedGuard)?
    Note over TC_KR2: specificObject->getContainedBy() = TroopCrawler<br/>TroopCrawler.isDisabledByType(DISABLED_HELD) = TRUE
    TC_KR2-->>TC_KR2: FALSE — TroopCrawler is DISABLED_HELD (held by Chinook)
    TC_KR2->>RedGuard: obj->kill(...)
    Note over RedGuard: Red Guard is killed ✓
    Chinook->>Chinook: processDamageToContained(percentDamage)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Line: 1367-1384

Comment:
**Dead objects may receive redundant damage in non-retail path**

In the non-retail build, `killRidersWhoAreNotFreeToExit()` is now called before `processDamageToContained()`. Because rider removal is deferred (`obj->kill()` does not remove the occupant from `m_containList` immediately, as noted by the existing `@info` comment), those freshly-killed riders will still be present in `m_containList` when `processDamageToContained()` swaps the list and iterates it. They will receive a second round of `DAMAGE_UNRESISTABLE` + `DEATH_BURNED` damage via `object->attemptDamage(&damageInfo)` before `isEffectivelyDead()` finally catches them and triggers the explicit `onRemoving` / `onRemovedFrom` cleanup.

While `onRemoving`/`onRemovedFrom` are each called exactly once (the kill-path defers removal, so `processDamageToContained` handles cleanup), the redundant `attemptDamage` call on an already-dead object is wasted work and may produce unexpected side effects (e.g. re-triggering any on-damage callbacks). Consider guarding the damage application with a liveness check:

```cpp
if (!object->isEffectivelyDead())
{
    Real damage = object->getBodyModule()->getMaxHealth() * percentDamage;
    DamageInfo damageInfo;
    // ...
    object->attemptDamage( &damageInfo );

    if( !object->isEffectivelyDead() && killContained )
        object->kill();
}
```

The same pattern applies to both the `#if RETAIL_COMPATIBLE_CRC` and `#else` branches in both `Generals` and `GeneralsMD` versions of this function.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "tweak: Add assertion..."}

[greptile-apps]

_{6 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{2 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Should use percentDamage parameter instead of data->m_damagePercentageToUnits to match the GeneralsMD implementation and ensure nested containers receive the correct damage value.

		Real damage = object->getBodyModule()->getMaxHealth() * percentDamage;

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Line: 1307:1307

Comment:
Should use `percentDamage` parameter instead of `data->m_damagePercentageToUnits` to match the GeneralsMD implementation and ensure nested containers receive the correct damage value.

```suggestion
		Real damage = object->getBodyModule()->getMaxHealth() * percentDamage;
```

How can I resolve this? If you propose a fix, please make it concise.

[xezon]

Does this also happen with Troopcrawler in helix?

[Stubbjax]

Does this also happen with Troopcrawler in helix?

A Troop Crawler takes up eight slots and thus cannot fit inside a Helix. But we can demonstrate the same behaviour with a Helix by loading an Ambulance or Technical.

AMBO_LIX.mp4

[xezon]

This change will need to be Merged With Rebase, because the unify commit needs to be kept separate. The commit titles need to be fit to standard.

[xezon]

Is this condition correct? What if the damage dealt to occupants is just 0.5? Shouldn't that also transfer the damage?

[Stubbjax]

Why? If a Humvee could be stored inside an Overlord, and the Overlord was destroyed, would it make sense to apply that 0.5 damage to the Humvee's passengers in addition to the Humvee itself? There is no precedent for this.

[xezon]

Yes if the health of the contained transport is 0.4 and the damage is 0.5, then we want to kill the passengers of the transport as well right? Or will they be killed anyway? If they are, then it begs the question why is the damage applied in that event but not when applying 1.0 damage?

[Stubbjax]

Upon further investigation, it turns out that the DamagePercentToUnits defined in the Chinook / Helix TransportContain modules is a red herring. Setting either of these fields to a value < 100% still results in the contained units dying, which is due to a call to killRidersWhoAreNotFreeToExit (though this causes the infantry corpses to drop out like vehicles do due to the lack of the burned death type via this logic path).

Only the direct occupants of airborne containers are considered not FREE_TO_EXIT (see ChinookAIUpdate::getAiFreeToExit), which prevents the killRidersWhoAreNotFreeToExit logic from triggering for successive occupants of contained transports such as those inside contained Troop Crawlers. It would seem killRidersWhoAreNotFreeToExit was never called in retail anyway due to the DamagePercentToUnits applying first, which is always set to 100% for airborne containers (where the FREE_TO_EXIT status applies).

I've solved this by returning false if the parent container has the held status in the isSpecificRiderFreeToExit function. I've also swapped the order of killRidersWhoAreNotFreeToExit and processDamageToContained, which avoids calling redundant logic in cases where riders are not FREE_TO_EXIT and DamagePercentToUnits < 100%.

[greptile-apps]

Missing null check before dereferencing getContainedBy(). While objects in the contain list should have a valid container, defensive programming suggests checking for nullptr to prevent potential crashes.

Suggested change

	if (specificObject->getContainedBy()->isDisabledByType(DISABLED_HELD))
	if (specificObject->getContainedBy() && specificObject->getContainedBy()->isDisabledByType(DISABLED_HELD))

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp
Line: 462

Comment:
Missing null check before dereferencing `getContainedBy()`. While objects in the contain list should have a valid container, defensive programming suggests checking for nullptr to prevent potential crashes.

```suggestion
	if (specificObject->getContainedBy() && specificObject->getContainedBy()->isDisabledByType(DISABLED_HELD))
```

How can I resolve this? If you propose a fix, please make it concise.

[xezon]

Code logic makes sense.

Needs rebase.