bugfix(helixcontain): Grant experience to the Helix from attachment gun kills

[tintinhamans]

Closes #87

A Helix never gains experience from kills made by its Gattling Cannon. The cannon is a separate object with its own weapon, so every kill is credited to the cannon, and its experience tracker accepts nothing: it is not trainable and has no experience sink. The Overlord has the same setup, but OverlordContain forwards the cannon's experience to the tank through ExperienceSinkForRider. HelixContain has no equivalent, so the experience is lost.

Now HelixContain::addToContain sets the portable structure's experience sink to the Helix, so the cannon's kills count for it. This covers both ways a Helix gets an attachment: the initial PayloadTemplateName payload and the upgrade OCLs with ContainInsideSourceObject. addToContainList does not need it, because for the Helix it is only reached from OpenContain::loadPostProcess, and ExperienceTracker::xfer already saves the sink. A portable structure never leaves a Helix alive, so nothing has to reset the sink, same as the Overlord.

helix-xp-compare-github.mp4

• Behind !RETAIL_COMPATIBLE_CRC
• Not replicated to Generals (no Helix)

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
🔒 Security Review	✅ Completed 2026-09-13T07:17:24.045860Z	6be0d03	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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

PR Summary by Qodo

Route Helix attachment experience to containing Helix

🐞 Bug fix 🕐 Less than 10 minutes

AI Description

• Routes portable Helix attachment kill experience to the containing Helix.
• Limits corrected gameplay behavior to non-retail-compatible builds.

Diagram

sequenceDiagram
  participant C as Helix Contain
  participant A as Portable Attachment
  participant T as XP Tracker
  participant H as Helix
  C->>A: Detect containment
  C->>T: Set Helix sink
  A->>T: Report kill XP
  T->>H: Forward experience

Loading

High-Level Assessment

The scoped on-containment sink assignment is the appropriate approach and mirrors the established Overlord attachment experience-routing pattern. Moving attribution into generic transport or weapon logic would affect unrelated containers and introduce unnecessary gameplay risk.

Files changed (1) +6 / -0

Bug fix (1) +6 / -0

HelixContain.cppRedirect portable attachment experience to the Helix +6/-0

Redirect portable attachment experience to the Helix

• Includes the experience tracker API and assigns the containing Helix as the experience sink when a portable structure is contained. The behavior is excluded from retail-compatible CRC builds.

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

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

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Detached cannons reward former Helixes 📎 Requirement gap ≡ Correctness

Description

onContaining assigns the portable structure's experience sink to the Helix, but onRemoving never
resets that sink. When the cannon is detached and later kills an eligible target,
ExperienceTracker::addExperiencePoints continues forwarding its experience to the former
container.

Code

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[R407-408]

+  if ( obj->isKindOf( KINDOF_PORTABLE_STRUCTURE ) && obj->getExperienceTracker() )
+    obj->getExperienceTracker()->setExperienceSink( getObject()->getID() );

Evidence

Rule 1 requires attachment experience to follow the owning Helix. The changed code establishes a
persistent sink, while the removal callback only restores weapon and disabled state;
ExperienceTracker retains and uses that object ID for later awards.

Award Helix XP for Gattling Cannon Attachment Kills
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[405-408]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[428-435]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/ExperienceTracker.cpp[91-99]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/ExperienceTracker.cpp[162-177]

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 experience routing persists after a portable structure is removed from its Helix, causing subsequent experience to be forwarded to the former container.
## Fix Focus Areas
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[407-408]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[428-435]
## Recommended Fix
In `onRemoving`, detect portable structures with an experience tracker and clear the sink with `INVALID_ID` when it still points to the current Helix. Apply the same compatibility guard used when assigning the sink.

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

---

2. Helixes still lose attachment experience 🐞 Bug ≡ Correctness

Description

HelixContain::onContaining assigns the experience sink, but the portable-structure branch of
HelixContain::addToContain returns without reaching that callback. Initial Gattling Cannons and
other configured portable payloads take this branch, so their kills continue providing no experience
to the Helix.

Code

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[R407-408]

+  if ( obj->isKindOf( KINDOF_PORTABLE_STRUCTURE ) && obj->getExperienceTracker() )
+    obj->getExperienceTracker()->setExperienceSink( getObject()->getID() );

Evidence

createPayload sends configured attachments through the virtual addToContain call. The Helix
override handles the first portable attachment solely by storing its ID and setting containedBy,
whereas the base implementation is what invokes onContaining; therefore the changed sink
assignment is unreachable for the intended payload path.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[109-112]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[149-173]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[266-282]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp[362-382]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[396-408]

Agent prompt

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

## Issue description
Portable Helix attachments bypass `HelixContain::onContaining`, so the newly added experience-sink assignment never runs for the intended attachment creation path.
## Fix Focus Areas
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[246-283]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[396-409]
## Recommended Fix
Move or share the experience-sink assignment with the portable-specific insertion branches in `addToContain` and `addToContainList`, ensuring it executes when the first portable structure is attached without changing Helix's specialized containment bookkeeping.

ⓘ 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 start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; the only new finding is non-blocking dead cleanup code.

Summary

This PR routes experience earned by a Helix portable attachment to its containing Helix in non-retail-compatible builds and clears the route when the attachment is removed.

• Adds a preservation flag for the legacy no-XP behavior.
• Assigns the Helix as the attachment's experience sink.
• Clears the sink during attachment removal.
• Adds ineffective attachment-cleanup branches whose lookups are guaranteed to return null.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Portable attachment scores a kill] --> B[Attachment ExperienceTracker]
  B -->|experience sink| C[Containing Helix]
  D[Attachment removed] --> E[Clear experience sink]

Loading

_{Reviews (4) · Last reviewed commit: "bugfix(contain): Route Helix attachment ..." · Reviewed by Greptile}

[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

Walkthrough

HelixContain now assigns the Helix as the experience sink for a contained portable structure with an experience tracker, and clears the sink when the structure is removed. This logic is compiled only when neither RETAIL_COMPATIBLE_CRC nor PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS is defined.

Changes

Helix portable structure containment

Layer / File(s)	Summary
Update portable structure containment Core/GameEngine/Include/Common/GameDefines.h, GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp	PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS defaults to 1 when not already defined. When neither this macro nor RETAIL_COMPATIBLE_CRC is defined, addToContain assigns the Helix ID as the experience sink for a contained portable structure with an experience tracker. removeFromContain resets that sink to INVALID_ID under the same condition.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: 🟡 Moderate · up to 7a7b2

Helix attachment kills can still fail to award the intended experience in non-retail builds, and older saves may retain that behavior after loading. Resolve these gaps before relying on the fix.

Pre-merge checks | 3 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #87 requires Helixes to gain experience from Gattling Cannon kills. HelixContain::addToContain assigns the portable structure's experience sink to the Helix only when both `RETAIL_COMPATIBLE_C…	Enable the experience routing for the supported build configuration. Remove the new preservation default or set it to 0, while keeping any intentional retail-compatibility override explicit. Add or report an automated test for experience …

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check		The pull request changes only Helix portable-structure experience routing, its reset on removal, the required include, and a compatibility configuration macro. These changes directly support issue #87…
Title check		The title clearly identifies the Helix experience bug and the fix for attachment gun kills.
Description check		The description directly explains the Helix experience issue, the experience sink fix, affected attachment paths, compatibility conditions, and implementation rationale.

---

Full details: Linked Issues check

Explanation

Issue #87 requires Helixes to gain experience from Gattling Cannon kills. HelixContain::addToContain assigns the portable structure's experience sink to the Helix only when both RETAIL_COMPATIBLE_CRC and PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS are false. GameDefines.h defaults PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS to 1, so the new routing remains disabled even in a non-retail-compatible build unless another configuration overrides the macro. The default configuration also sets RETAIL_COMPATIBLE_CRC to 1.

Resolution

Enable the experience routing for the supported build configuration. Remove the new preservation default or set it to 0, while keeping any intentional retail-compatibility override explicit. Add or report an automated test for experience transfer if the repository supports one.

---

• 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.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 963d740a-7529-4095-816d-4ea90d81c814

📥 Commits

Reviewing files that changed from the base of the PR and between 86ba6b5 and 287bc9d.

📒 Files selected for processing (2)

• Core/GameEngine/Include/Common/GameDefines.h
• 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; 3 remain after this review.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Set PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS to 0. · GameDefines.h:43-45

Core/GameEngine/Include/Common/GameDefines.h:43-45
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Set PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS to 0.

When RETAIL_COMPATIBLE_CRC is 0, the default value 1 still excludes both Helix sink operations. Portable-structure kills therefore do not route experience to the Helix unless the build also defines this undocumented macro as 0. Change only this default and retain the RETAIL_COMPATIBLE_CRC guard.

Suggested fix

 #ifndef PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS
-#define PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS (1)
+#define PRESERVE_NO_XP_FROM_HELIX_ATTACHMENT_KILLS (0)
 #endif

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1cf142a1-2bff-4da6-b3a6-1c800f0c2f85

📥 Commits

Reviewing files that changed from the base of the PR and between 287bc9d and 7a7b2a1.

📒 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; 2 remain after this review.

[xezon]

This makes sense to me.