Skip to content

tweak(particlesys): Decouple Particles render update from logic step#2709

Open
xezon wants to merge 7 commits into
TheSuperHackers:mainfrom
xezon:xezon/decouple-particle-update
Open

tweak(particlesys): Decouple Particles render update from logic step#2709
xezon wants to merge 7 commits into
TheSuperHackers:mainfrom
xezon:xezon/decouple-particle-update

Conversation

@xezon
Copy link
Copy Markdown

@xezon xezon commented May 14, 2026

Merge with Rebase

This change decouples the Particles render update from the logic step.

Split into 7 commits for ease of understanding and review.

TODO

  • Replicate in Generals
  • Add pull ids to commit titles

@xezon xezon added Enhancement Is new feature or request Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Rendering Is Rendering related labels May 14, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR decouples the particle system's render update from the fixed logic step: lifetime management and key-frame advancement stay on the logic step (ParticleSystem::update), while physics integration, colour/alpha accumulation, and transform interpolation are moved to a new render-step path (ParticleSystem::draw(Real timeScale)). Frame-rate-independent damping helpers (scaleDamping, scaleAccelDamping) are introduced so particle trajectories remain consistent at varying render rates.

  • ParticleSystemManager::update() is moved from GameClient into GameLogic::update(), and a new draw() is called from W3DDisplay at render time; ParticleSystemManagerDummy gains explicit no-op overrides for both, removing the headless updateHeadless() workaround.
  • m_pos/m_lastPos on ParticleSystem are renamed to m_logicalPos/m_lastLogicalPos and m_lastPos on Particle is removed; a PRESERVE_RETAIL_PARTICLES compile flag controls a one-frame key-frame delay to preserve the original visual look.
  • BaseType.h gains a full set of RGBColor arithmetic operators consumed by the new draw() colour accumulation.

Confidence Score: 4/5

The core logic/render split is sound; only a harmless duplicate initializer and two pre-existing open questions remain.

The physics decoupling and frame-rate scaling math are solid, and the dummy manager overrides cleanly handle headless mode. A duplicate m_velCoeff.zero() call is the only new defect introduced by the diff. Two previously documented concerns — updateTransform() executing on both paths per frame, and alpha/colour-based particle death not firing in fully headless runs — are carried forward.

Core/GameEngine/Source/GameClient/System/ParticleSys.cpp and Core/GameEngine/Include/GameClient/ParticleSys.h

Important Files Changed

Filename Overview
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp Major refactor splitting particle logic into update() (logic step: lifetime/keyframes) and draw() (render step: physics/color/alpha). Introduces scaleDamping/scaleAccelDamping helpers for frame-rate-independent physics. Contains a duplicate m_velCoeff.zero() call introduced by the diff.
Core/GameEngine/Include/GameClient/ParticleSys.h Adds draw(Real timeScale) methods to Particle, ParticleSystem, and ParticleSystemManager; replaces m_pos/m_lastPos with m_logicalPos/m_lastLogicalPos; introduces VisibilityState and refactored transform helpers; adds isDummy() virtual to ParticleSystemManagerDummy.
Core/Libraries/Include/Lib/BaseType.h Adds a full set of arithmetic operators for RGBColor. All binary operators correctly return by value.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp Moves TheParticleSystemManager->update() from GameClient to GameLogic::update(), anchoring particle lifetime management to the logic step.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp Switches call from ->update() to ->draw() in the render path, completing the logic/render decoupling.

Sequence Diagram

sequenceDiagram
    participant GL as GameLogic::update()
    participant PSM as ParticleSystemManager
    participant PS as ParticleSystem
    participant P as Particle
    participant W3D as W3DDisplay (render)

    GL->>PSM: setLocalPlayerIndex()
    GL->>PSM: update()
    PSM->>PS: update(localPlayerIndex)
    PS->>PS: updateVisibility()
    PS->>PS: updateLastLogicalPos()
    PS->>PS: updateTransform()
    PS->>PS: updateLogicalPos()
    PS->>PS: generateParticles()
    PS->>P: update()
    Note over P: lifetime decrement / keyframe advance / isInvisible() cull

    W3D->>PSM: draw()
    Note over PSM: timeScale = FramePacer ratio
    PSM->>PS: draw(timeScale)
    PS->>PS: updateWindMotion(timeScale)
    PS->>PS: updateTransform()
    PS->>P: applyForce(gravity)
    PS->>P: draw(timeScale)
    Note over P: velocity/position integration / alpha/colour accumulation / accel reset
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp:1125-1128
Duplicate `m_velCoeff.zero()` call introduced by this diff. The line was already present in the original constructor body; the new `+` hunk added a second identical call immediately before it. The later assignment to `m_velCoeff.x/y/z = 1.0f` means both zeroing calls are effectively dead, but having two consecutive identical statements is misleading noise worth removing.

```suggestion
	m_logicalPos.zero();
	m_lastLogicalPos.zero();
	m_velCoeff.zero();
```

Reviews (2): Last reviewed commit: "tweak(particlesys): Decouple Particles r..." | Re-trigger Greptile

Comment thread Core/Libraries/Include/Lib/BaseType.h Outdated
@Caball009
Copy link
Copy Markdown

Caball009 commented May 14, 2026

I presume this PR fixes #2467.

psys->attachToObject(building);
Drawable *drawable = building->getDrawable();
psys->attachToObject(object);
Drawable *drawable = object->getDrawable();
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rename was just a side quest from when looking around particle things.

// vel += accel;
// vel *= damping;
//
static inline Real scaleAccelDamping(Real damping, Real timeScale)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was crafted by prompting Chat Gippy many times. I do not understand exactly why it works but it works.

ParticleSystemInfo::WindMotion windMotion = m_system->getWindMotion();
// monitor lifetime
if (m_lifetimeLeft && --m_lifetimeLeft == 0)
return false;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the lifetime check from the bottom of the update to the very top, because there is no reason to go through all the trouble of updating the particle when the lifetime hits zero anyway. So this is a very minor performance improvement maybe.

@xezon
Copy link
Copy Markdown
Author

xezon commented May 17, 2026

The plane trails do not fade out gracefully. That needs looking into.

@Caball009
Copy link
Copy Markdown

Caball009 commented May 17, 2026

The changes in this PR don't seem remotely retail compatible. Like many other replays, this PR makes GR1 mismatch with headless mode for me. The mismatch happens at frame 14227, but only with headless mode. It looks like the CI replay checker is silently broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Is new feature or request Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Rendering Is Rendering related ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants