chore(helixcontain): Remove dead code and redundant lookups in HelixContain

[tintinhamans]

Split out of #3282.

The two destroy blocks sit inside m_portableStructureID == INVALID_ID, where getPortableStructure() is always null. The compiled output is the same with and without them.

removeFromContain and isEnclosingContainerFor looked up the object they were just handed by the ID they had just compared. For a live object that lookup returns the same object, so it is dropped. Caball009 ran this against replays without a mismatch.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 60ecd062-2675-4d72-9d05-ac2e945d1bb2

---

📥 Commits

Reviewing files that changed from the base of the PR and between 972489b and 41abf62.

---

📒 Files selected for processing (1)

• GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp

---

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

---

Walkthrough

HelixContain no longer destroys the previously referenced portable structure when adding one. Removal clears containment and experience-sink state on the supplied object and resets the stored ID. The enclosure check compares the stored ID with the queried object’s ID.

Changes

Helix containment

Layer / File(s)	Summary
Portable-structure ID handling GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp	The add methods stop destroying the previously referenced portable structure. Removal clears containment and experience-sink state on the supplied object and resets the stored ID. The enclosure check compares IDs directly.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 41abf

The reviewed changes show no supported behavior regression that needs resolution before merging.

Pre-merge checks | 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check		The title clearly identifies the removal of dead code and redundant lookups in HelixContain, which matches the main changeset.
Description check		The description explains why the destroy blocks and object lookups can be removed and notes replay testing, so it is directly related to the changeset.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issue was found.

Summary

Removes dead destruction blocks and repeated object lookups from HelixContain.

• HelixContain avoids repeat lookups for its portable structure.

_{Reviews (5) · Last reviewed commit: "chore(helixcontain): Remove dead code an..." · Reviewed by Greptile}

[Caball009]

Should be safe. My only mild concern is retail compatible use-after-free bugs invalidating our assumptions about removeFromContain and isEnclosingContainerFor.

I want to test a couple of the replays that previously had me looking at the functions in this class.

[tintinhamans]

Should be safe. My only mild concern is retail compatible use-after-free bugs invalidating our assumptions about removeFromContain and isEnclosingContainerFor.

I want to test a couple of the replays that previously had me looking at the functions in this class.

Good point, a stale pointer has ID 0 and that would match a Helix without a portable. Took the lookup changes out, it's only the two dead blocks now.

No issues detected.

[Caball009]

I just ran the previous changes (including removeFromContain and isEnclosingContainerFor) against a small set of replays and those did not mismatch, so it appears safe to include the previous changes again :)

EDIT: It's up to you whether to include the previous changes again.

[xezon]

You could put them behind RETAIL_COMPATIBLE_CRC if paranoid and put a comment that its just a precaution to leave them.

Open comment

[tintinhamans]

Should be safe. My only mild concern is retail compatible use-after-free bugs invalidating our assumptions about removeFromContain and isEnclosingContainerFor.

I want to test a couple of the replays that previously had me looking at the functions in this class.

Thanks for testing, put the lookup changes back in. Rebased on #3282.

Added an INVALID_ID check in isEnclosingContainerFor, so a stale pointer with ID 0 can’t match a Helix without a portable.