bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained

[Caball009]

• Closes Restore retail compatibility after crash fix in OpenContain::processDamageToContained #2223
• Follow up for [GEN][ZH] Fix crash in OpenContain::processDamageToContained() when killed occupants kill the host container #1019

The way the crash fix was implemented in #1019 assumes the container size is either unchanged or 0. This is not the case in the attached replay in #2223.

Gameplay without this PR:

processDamageToContained_before2.mp4

Gameplay with this PR:

processDamageToContained_after2.mp4

Due to a different bug, a GLA Worker enters the unmanned Emperor Overlord but becomes part of the crew instead of the pilot. When the Emperor gets destroyed it first removes its Gattling turret and so the container size has changed. The Worker is still inside, though, so breaking out of the loop is not allowed.

TODO:

• Replicate in Generals.
• Polish comments.

[greptile-apps]

Greptile Summary

This PR fixes a retail compatibility regression introduced by PR #1019, where the crash fix for processDamageToContained assumed the container's size would be either unchanged or zero after Object::attemptDamage was called. The specific failure case is an Emperor Overlord whose GattlingTurret is removed from the container first on death — shrinking the container size without emptying it — causing the old guard to fire incorrectly and skip processing remaining occupants (e.g. a GLA Worker still inside).

Key changes:

• Replaces the fragile size-change guard (break/continue on listSize != items->size()) with a full snapshot of m_containList taken before the loop, so modifications to the live list during iteration have no effect.
• Introduces a small-container optimisation: uses a fixed-size stack array for containers with fewer than 16 occupants and falls back to a heap-allocated std::vector for larger ones, avoiding unnecessary allocations in the common case.
• Extracts the per-object damage logic into processDamageToContainedInternal (RETAIL path only) to avoid duplicating it across the two copy strategies.
• The non-RETAIL path is updated only cosmetically (hoists isBurnedDeathToUnits/killContained out of the removed OpenContainModuleData pointer).

Confidence Score: 4/5

• Safe to merge with minor polish — the core snapshot approach correctly fixes the regression with no new crash or UB vectors.
• The snapshot strategy is sound: iterating over a copy of the container while the live list may change is safe given the game's deferred-deletion guarantee. Both the stack-path and vector-path correctly pass m_containListSize / containCopy.size() as the iteration bound. The only actionable findings are a missing DEBUG_ASSERTCRASH null guard in processDamageToContainedInternal (parity with the non-RETAIL path) and the processDamageToContainedInternal declaration being in the public: section despite being an internal helper.
• No files require special attention beyond the minor style notes on OpenContain.h and OpenContain.cpp.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Replaces the fragile size-change break/continue guard in the RETAIL path with a full snapshot copy (stack array for <16 elements, heap vector otherwise), eliminating iterator-invalidation risks. Minor: null-pointer assertion present in the non-RETAIL path is missing from the new processDamageToContainedInternal loop body.
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/OpenContain.h	Adds the processDamageToContainedInternal declaration guarded by RETAIL_COMPATIBLE_CRC. The function is placed in the public: section despite being an internal helper; it should be private.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["processDamageToContained(percentDamage)"] --> B{RETAIL_COMPATIBLE_CRC?}

    B -- Yes --> C["Assert m_containListSize == m_containList.size()"]
    C --> D{m_containListSize < 16?}

    D -- Yes --> E["Stack copy: Object* containCopy[16]\nstd::copy(m_containList → containCopy)"]
    D -- No --> F["Heap copy: std::vector&lt;Object*&gt; containCopy\n(m_containList.begin(), m_containList.end())"]

    E --> G["processDamageToContainedInternal\n(containCopy, m_containListSize, percentDamage)"]
    F --> G

    G --> H["for each object in snapshot"]
    H --> I["object->attemptDamage()"]
    I --> J{isEffectivelyDead\n&& killContained?}
    J -- Yes --> K["object->kill()"]
    J -- No --> L["next object"]
    K --> L
    L --> H

    B -- No --> M["Swap m_containList to local list\nm_containListSize = 0"]
    M --> N["Iterate local list\nattemptDamage each object"]
    N --> O{isEffectivelyDead?}
    O -- Yes --> P["onRemoving / onRemovedFrom\nerase from local list"]
    O -- No --> Q["advance iterator"]
    P --> R["next object"]
    Q --> R
    R --> N
    N --> S["Swap local list back\nupdate m_containListSize"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/GameLogic/Module/OpenContain.h
Line: 222-224

Comment:
**`processDamageToContainedInternal` should be `private`**

The function is named `Internal`, signalling it is an implementation detail not intended for external callers, yet it is placed in the `public:` section. Because it takes a raw `Object* const*` array and a separate `size` parameter, an incorrect external call could bypass the container's snapshot-size safeguards entirely. Moving it to `private:` (or at minimum `protected:`) is a cheap way to enforce the intended usage boundary.

```suggestion
#if RETAIL_COMPATIBLE_CRC
private:
	void processDamageToContainedInternal(Object* const* objects, size_t size, Real percentDamage);
public:
#endif
```

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

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Line: 1473-1475

Comment:
**Missing null guard on `object` in RETAIL path**

The non-RETAIL path has `DEBUG_ASSERTCRASH(object, ("Contain list must not contain null element"))` before dereferencing the pointer. The RETAIL path's `processDamageToContainedInternal` has no equivalent check — a null entry in `m_containList` will silently crash at `object->getBodyModule()` in debug and retail builds alike.

Consistent with the non-RETAIL branch, consider adding the assertion:

```suggestion
	for (size_t i = 0; i < size; ++i)
	{
		Object* object = objects[i];

		DEBUG_ASSERTCRASH( object, ("Contain list must not contain null element") );

		// Calculate the damage to be inflicted on each unit.
		Real damage = object->getBodyModule()->getMaxHealth() * percentDamage;
```

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

_{Last reviewed commit: "Changed comments."}

[greptile-apps]

Assertion assumes object is always the removed item; reset to begin() can double-process occupants

The assertion std::find(items->begin(), items->end(), object) == items->end() fires when object is still in the list after the size change, with the message "Current object was expected to be deleted." However the list can shrink because a different occupant was explicitly removed — not the one currently being processed.

Consider the hypothetical scenario where the container order is [Worker, GattlingTurret] instead of [GattlingTurret, Worker]:

1. object = Worker, it → GattlingTurret
2. attemptDamage(Worker) triggers the Overlord death handler, which explicitly removes GattlingTurret from the container.
3. listSize (2) != items->size() (1) → size changed.
4. std::find(..., Worker) != items->end() → Worker is still in the list → assertion fires (false positive in debug).
5. it = items->begin() → points back to Worker, which was already processed.
6. Worker receives attemptDamage a second time in the same processDamageToContained call.

For the specific scenario described in the PR (GattlingTurret is always first in the container list), this works correctly. But the correctness is implicitly reliant on the container insertion order. A comment documenting this ordering dependency would help future maintainers avoid accidentally triggering double-damage on occupants.

The assertion itself should ideally check that object was either removed from the list or that it still refers to a valid, unprocessed element — since both conditions can legitimately cause the list to shrink without object being the removed item.

Prompt To Fix With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Line: 1504-1510

Comment:
**Assertion assumes `object` is always the removed item; reset to `begin()` can double-process occupants**

The assertion `std::find(items->begin(), items->end(), object) == items->end()` fires when `object` is still in the list after the size change, with the message "Current object was expected to be deleted." However the list can shrink because a *different* occupant was explicitly removed — not the one currently being processed.

Consider the hypothetical scenario where the container order is **[Worker, GattlingTurret]** instead of **[GattlingTurret, Worker]**:

1. `object = Worker`, `it → GattlingTurret`
2. `attemptDamage(Worker)` triggers the Overlord death handler, which explicitly removes **GattlingTurret** from the container.
3. `listSize (2) != items->size() (1)` → size changed.
4. `std::find(..., Worker) != items->end()` → Worker is **still in the list** → assertion fires (false positive in debug).
5. `it = items->begin()` → points back to **Worker**, which was already processed.
6. Worker receives `attemptDamage` a **second time** in the same `processDamageToContained` call.

For the specific scenario described in the PR (GattlingTurret is always first in the container list), this works correctly. But the correctness is implicitly reliant on the container insertion order. A comment documenting this ordering dependency would help future maintainers avoid accidentally triggering double-damage on occupants.

The assertion itself should ideally check that `object` was *either* removed from the list *or* that `it` still refers to a valid, unprocessed element — since both conditions can legitimately cause the list to shrink without `object` being the removed item.

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

[Caball009]

Yes, if an element is removed from the list that's NOT the current item the first element, this code likely won't work correctly.

I'm not aware of a scenario when that could happen, though. It's also challenging to write the code in such a way that this assumption is not necessary.

[Caball009]

@xezon I can't think of any way other than making a copy of the container and iterating over that to avoid this issue. This is what I'd imagine it to look like:

void attemptDamage(std::list<int>& containList, int val)
{
	if (val == 2)
	{
		auto it = std::find(containList.begin(), containList.end(), 2);
		if (it != containList.end())
			containList.erase(it);

		it = std::find(containList.begin(), containList.end(), 3);
		if (it != containList.end())
			containList.erase(it);
	}
}

void processDamageToContained()
{
	std::list<int> containList{ 1, 2, 3, 4 };
	const auto iterateList = containList;
	const auto listSize = containList.size();

	for (auto it = iterateList.begin(); it != iterateList.end(); ++it)
	{ 
		const auto val = *it; 

		if (listSize != containList.size() 
		  && std::find(containList.begin(), containList.end(), val) == containList.end())
			continue;

		attemptDamage(containList, val);
	}
}

The containList search is somewhat inefficient, but that search is unlikely to be needed and the list size is likely small so this keeps the code cleaner.

[xezon]

Yes. You can make a copy into a std::vector<Object*> containCopy(containList.begin(), containList.end()), then iterate that and find the current Object* in the containList to check if that still needs processing.

[Caball009]

Do you think it's worth making a copy on the stack if the list size is small? This could save us some dynamic memory allocations.

[greptile-apps]

Not worth it. The allocation happens only when a container is destroyed/damaged, which is infrequent gameplay. A small heap allocation (~2-5 pointers) is negligible compared to the damage processing logic itself.

The complexity cost of dual code paths (stack buffer for small, heap for large) outweighs any performance gain. Keep it simple with a single std::vector<Object*> copy — the allocator will handle small sizes efficiently anyway.

[Caball009]

The allocation would happen every time processDamageToContained is called and m_containList is not empty.

[Caball009]

Yes. You can make a copy into a std::vector<Object*> containCopy(containList.begin(), containList.end()), then iterate that and find the current Object* in the containList to check if that still needs processing.

On second thought, I'm not sure if this is still needed.

Assuming that

1. removed objects from the contain list are marked as effectively dead, a simple isEffectivelyDead check should suffice.
2. removed objects from the contain list are marked as effectively dead and Object::attemptDamage doesn't do anything meaningful for such objects, there wouldn't be a need for any check at all.

I'm not sure how confident I feel about 2, but 1 seems reasonable enough.

[Mauller]

This comment will want removing since the functionality is changed compared to what the comment is describing.

[Caball009]

I reset the branch and broke the new diff into manageable commits.

[Caball009]

Do we still need this comment?

I'm not sure if we need an issue reproduction description in the code comment. We can use Git history to find out much more information on issue repro than we could possibly cram into a bit of text.