[Caball009]

• Fixes Particle::loadPostProcess looks odd #842

The conversion from from ParticleSystem* to ParticleSystemID and back is currently bugged during the Particle xfer process. ParticleSystemID m_systemUnderControlID is never set, so the lookup in Particle::loadPostProcess never succeeds. This PR fixes that.

---

GeneralsGameCode/Core/GameEngine/Include/GameClient/ParticleSys.h

Lines 252 to 256 in f02fcd3

	union
	{
	ParticleSystem * m_systemUnderControl; ///< the particle system attached to this particle (not the system that created us)
	ParticleSystemID m_systemUnderControlID; ///< id of system attached to this particle (not the system that created us);
	};

I added a union tag to keep track which union member is currently active to keep the behavior well-defined. I also did assignments to both union members just to be sure. This way removing the union / changing the pointer to 64 bit should not introduce new issues.

[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.

• Last reviewed commit: 1039118
• Latest commit, not reviewed: 9e4e0dd

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

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d657e679-8016-4e90-9ec3-0b659c655b88

Commits

Reviewing files that changed from the base of the PR and between 23a6e13 and 1039118.

Files selected for processing: 2

• Core/GameEngine/Include/GameClient/ParticleSys.h
• Core/GameEngine/Source/GameClient/System/ParticleSys.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.

Walkthrough

Particle serialization now tracks whether the controlled-system union contains a transfer ID or runtime pointer. Loading defers system lookup until post-processing, which restores the control-particle relationship or throws SC_INVALID_DATA if the system is missing.

Changes

Particle load restoration

Layer / File(s)	Summary
Store the controlled-system transfer ID Core/GameEngine/Include/GameClient/ParticleSys.h, Core/GameEngine/Source/GameClient/System/ParticleSys.cpp	Particle now tracks whether the union contains a transfer ID or runtime pointer. Serialization saves an ID only when the controlled system exists, is not destroyed, and is saveable. Loading defers pointer resolution, and the destructor skips cleanup while restoration is pending.
Restore the controlled particle system Core/GameEngine/Source/GameClient/System/ParticleSys.cpp	Post-processing resolves and clears the deferred ID. It throws SC_INVALID_DATA if the system is missing. Otherwise, it links the particle to the system.

Priority: Normal

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

Change: Bug fix

Suggested reviewers: bobtista

Merge Risk: Minimal · up to 10391

Controlled-system references omitted from saves no longer trigger the identified load failure. The change is mergeable after normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Issue #842 requires the particle-system ID to resolve during Particle::loadPostProcess() and requires safe handling when the lookup fails. Particle::xfer() now stores the loaded ID in `m_xferSyste…
Out of Scope Changes check		The changes are limited to the controlled particle-system transfer state, serialization, post-load restoration, and destructor safety. The saveability checks and transfer tag support the particle-syst…
Description check		The pull request description directly explains the particle transfer bug and the changes to the union tag, transfer ID, and load restoration logic.
Title check		The title names the primary change: Particle::loadPostProcess() now loads the particle system under control. The raw summary confirms this. The loadPostProcess path looks up the saved ID, throws SC_IN…

Full details: Linked Issues check

Explanation

Issue #842 requires the particle-system ID to resolve during Particle::loadPostProcess() and requires safe handling when the lookup fails. Particle::xfer() now stores the loaded ID in m_xferSystemUnderControlID. loadPostProcess() calls findParticleSystem(), checks for nullptr before use, throws SC_INVALID_DATA when needed, and restores m_systemUnderControl for a valid system. The transfer tag also prevents the destructor from treating transfer data as a runtime pointer. Issue #842 has no stated automated-test requirement.

Full details: Out of Scope Changes check

Explanation

The changes are limited to the controlled particle-system transfer state, serialization, post-load restoration, and destructor safety. The saveability checks and transfer tag support the particle-system reference fix and interrupted-load safety described by the PR and issue #842. No unrelated change is shown.

Full details: Title check

Explanation

The title names the primary change: Particle::loadPostProcess() now loads the particle system under control. The raw summary confirms this. The loadPostProcess path looks up the saved ID, throws SC_INVALID_DATA when no system is found, and links the control particle. The title does not mention the rename to m_xferSystemUnderControlID, the new transfer flag, or the destructor change. Those details are secondary and need not appear in the title.

• 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 issues were found.

Summary

Fixes saved particles losing their link to the particle systems they control.

• Loaded particles reconnect to the systems they control.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Save particle] --> B{Controlled system will be saved?}
  B -->|Yes| C[Write system ID]
  B -->|No| D[Write invalid ID]
  E[Load particle] --> F[Store ID and set tag]
  F --> G[Finish loading and registering systems]
  G --> H[Restore pointer and clear tag]
  H --> I[Reconnect system to its control particle]

Loading

_{Reviews (8) · Last reviewed commit: "fix(particle): Load particle system unde..." · Reviewed by Greptile}

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: a17d8048-1120-44e3-82fc-a77ea203aaed

📥 Commits

Reviewing files that changed from the base of the PR and between f02fcd3 and d9798d0.

📒 Files selected for processing: 2

• Core/GameEngine/Include/GameClient/ParticleSys.h
• Core/GameEngine/Source/GameClient/System/ParticleSys.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.

[stephanmeesters]

I think it looks good, but some concerns about legacy saves now more often leading to SC_INVALID_DATA because the loader is more strict now. I haven't tested it on any saves though.

[xezon]

Looks technically right. Style can be polished.

[Caball009]

Looks technically right. Style can be polished.

I made some non-style changes since your review, so you may want to give it another good look.

There's some redundancy in the code (writing to both union members), but I tried to make sure that changing the union to struct or changing the pointer from 32 bit to 64 bit doesn't introduce subtle problems.

[xezon]

Makes sense.