Skip to content

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

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

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

Conversation

@xezon

@xezon xezon commented May 14, 2026

Copy link
Copy Markdown

Merge with Rebase

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

Split into 6 commits for ease of understanding and review.

TODO

  • Replicate in Generals
  • Add pull ids to commit titles
  • Investigate plane trail particles
  • Initialize particle templates with RETAIL_COMPATIBLE_CRC

@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

greptile-apps Bot commented May 14, 2026

Copy link
Copy Markdown

Greptile Summary

This PR decouples particle rendering work from particle logic updates. The main changes are:

  • New frame-phase tracking in FramePacer.
  • Particle motion, color, alpha, and damping updates moved into render-time draw() paths.
  • Particle lifetime and system cleanup kept on logic-time update() paths.
  • Variant render loops updated to call particle draw separately.
  • RGBColor arithmetic operators changed to return safe value results.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp Splits particle logic cleanup from render-time motion, alpha, color, and force integration.
Core/GameEngine/Include/GameClient/ParticleSys.h Adds draw/update declarations, validation helpers, and the dummy manager draw no-op.
Core/GameEngine/Source/Common/FramePacer.cpp Tracks render progress through the current logic frame for interpolation.
Core/Libraries/Include/Lib/BaseType.h Adds safe value-returning RGBColor arithmetic operators.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp Runs particle logic once after a logic update and particle draw during the render pass.
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp Adds the particle draw call to the render pass.

Reviews (10): Last reviewed commit: "fixup! tweak(particlesys): Decouple Part..." | Re-trigger Greptile

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

Caball009 commented May 14, 2026

Copy link
Copy Markdown

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.

Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp Outdated
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
xezon force-pushed the xezon/decouple-particle-update branch from b94750d to b86a504 Compare May 17, 2026 11:40
@xezon

xezon commented May 17, 2026

Copy link
Copy Markdown
Author

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

@Caball009

Caball009 commented May 17, 2026

Copy link
Copy Markdown

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.

Comment thread Core/Libraries/Include/Lib/BaseType.h
@xezon

xezon commented May 21, 2026

Copy link
Copy Markdown
Author

I will break the dummy particle manager changes off of this pull.

@xezon

xezon commented May 21, 2026

Copy link
Copy Markdown
Author

ParticleSystemManagerDummy changes split off to #2740

Skyaero42
Skyaero42 previously approved these changes Jun 16, 2026
Comment thread Core/Libraries/Include/Lib/BaseType.h
@xezon

xezon commented Jun 16, 2026

Copy link
Copy Markdown
Author

This still needs work. Plane trail issue I have locally fixed but there was another issue that I already forgot. Maybe need to watch old videos.

Edit: I remember it was plane exhaust and low frame rate (USA 02 mission intro)

@Skyaero42

Copy link
Copy Markdown

This still needs work. Plane trail issue I have locally fixed but there was another issue that I already forgot. Maybe need to watch old videos.

Edit: I remember it was plane exhaust and low frame rate (USA 02 mission intro)

Would have liked this PR in draft then, so I didn't spend time reviewing it.
I cannot withdraw my review until branch conflicts are resolved - long live Github

@xezon
xezon marked this pull request as draft June 17, 2026 07:53
@xezon

xezon commented Jul 11, 2026

Copy link
Copy Markdown
Author

@greptileai
With this change, JetLenzflare does not render at render frame rate 25 or lower. Why?

Particle definition:

ParticleSystem JetLenzflare
  Priority = WEAPON_EXPLOSION
  IsOneShot = NO
  Shader = ADDITIVE
  Type = PARTICLE
  ParticleName = EXLnzFlar2.tga
  AngleX = 0.00 0.00
  AngleY = 0.00 0.00
  AngleZ = 0.00 0.00
  AngularRateX = 0.00 0.00
  AngularRateY = 0.00 0.00
  AngularRateZ = 0.00 0.00
  AngularDamping = 1.00 1.00
  VelocityDamping = 1.00 1.00
  Gravity = 0.00
  SlaveSystem = JetLenzflareExhaust
  SlavePosOffset = X:0.00 Y:0.00 Z:0.00
  Lifetime = 3.00 3.00
  SystemLifetime = 0
  Size = 20.00 20.00
  StartSizeRate = 0.00 0.00
  SizeRate = 0.20 0.20
  SizeRateDamping = 1.00 1.00
  Alpha1 = 1.00 1.00 0
  Alpha2 = 0.00 0.00 2
  Alpha3 = 0.00 0.00 0
  Alpha4 = 0.00 0.00 0
  Alpha5 = 0.00 0.00 0
  Alpha6 = 0.00 0.00 0
  Alpha7 = 0.00 0.00 0
  Alpha8 = 0.00 0.00 0
  Color1 = R:0 G:0 B:0 0
  Color2 = R:255 G:255 B:255 1
  Color3 = R:0 G:0 B:0 3
  Color4 = R:0 G:0 B:0 0
  Color5 = R:0 G:0 B:0 0
  Color6 = R:0 G:0 B:0 0
  Color7 = R:0 G:0 B:0 0
  Color8 = R:0 G:0 B:0 0
  ColorScale = 0.00 0.00
  BurstDelay = 0.00 0.00
  BurstCount = 1.00 1.00
  InitialDelay = 0.00 0.00
  DriftVelocity = X:0.00 Y:0.00 Z:0.25
  VelocityType = ORTHO
  VelOrthoX = -1.00 0.00
  VelOrthoY = 0.00 0.00
  VelOrthoZ = 0.00 0.00
  VolumeType = LINE
  VolLineStart = X:0.00 Y:0.00 Z:0.00
  VolLineEnd = X:0.00 Y:0.00 Z:0.00
  IsHollow = YES
  IsGroundAligned = NO
  IsEmitAboveGroundOnly = NO
  IsParticleUpTowardsEmitter = NO
  WindMotion = UNUSED
  WindAngleChangeMin = 0.149924
  WindAngleChangeMax = 0.449946
  WindPingPongStartAngleMin = 0.000000
  WindPingPongStartAngleMax = 0.785398
  WindPingPongEndAngleMin = 5.497787
  WindPingPongEndAngleMax = 6.283185
End

@greptile-apps

greptile-apps Bot commented Jul 11, 2026

Copy link
Copy Markdown

The root cause is the interaction between the decoupled logic/render updates, the initial black color of JetLenzflare particles, and the isInvisible() check in Particle::update().

Why it happens:

JetLenzflare uses Shader = ADDITIVE with:

Color1 = R:0 G:0 B:0 0   ← starts BLACK
Color2 = R:255 G:255 B:255 1

With the decoupled design, m_color is only advanced in Particle::draw() (the render step), not in Particle::update() (the logic step). The logic step only manages key frame index advancement and the isInvisible() death check.

At logic = 30 FPS, render = 25 FPS (or lower), the timing is:

Time Event
t=0ms Logic frame N: particle created, m_color = (0,0,0) (black)
t=33ms Logic frame N+1: update() runs with frameCount=1. draw() has NOT been called yet — render frame isn't due until t=40ms. isInvisible() sees color is still black → particle dies
t=40ms Render frame: draw() would have made the color white — but the particle is already gone

The frameCount == 0 guard in update() was intended to protect against this:

if (frameCount == 0)
{
    // Pass one full logic frame before trying to update and delete this potentially now
    // invisible particle, because the later render update may fade it in and make it visible.
    return true;
}

But it only protects the creation frame. It does not protect against the case where render FPS < logic FPS and no draw() call has yet occurred by frameCount=1. At 25 FPS render, draw() simply hasn't been called between frameCount=0 and frameCount=1, so the particle is still black when isInvisible() fires.

The existing todo in the code already captures this:

// TheSuperHackers @todo This check is shady for particles that fade in first. A more robust logic would be good.
if (isInvisible())
    return false;

A robust fix would need to track whether at least one draw() call has occurred before enabling the isInvisible() early-exit in update(). For example, a m_hasBeenDrawn flag on the particle: skip the isInvisible() check until the flag is set by the first draw() call.

@xezon

xezon commented Jul 11, 2026

Copy link
Copy Markdown
Author

@greptileai
This is not the cause of the issue. The Logic step cannot run faster than the Render step. And I already tested without the isInvisible() test and it the issue still happens. Please check for other reasons.

@greptile-apps

greptile-apps Bot commented Jul 11, 2026

Copy link
Copy Markdown

You're right, I was wrong on both counts — the logic step can't run faster than render in this engine's design, and the isInvisible() check being removed didn't reproduce the fix. I apologize for the incorrect diagnosis.

Looking more carefully at the decoupling for other potential causes: one thing that stands out is that m_alpha and m_color are now only accumulated in draw(), but the keyframe rate recomputation (computeAlphaRate() / computeColorRate()) happens in update(). So there's an asymmetry — the rate changes on logic ticks, but the accumulated value only advances on render ticks. If a keyframe boundary is crossed in update() and then draw() overshoots because of a large timeScale, the alpha/color could drift past the intended keyframe target value with no correction, since the direct-snap m_alpha = m_alphaKey[...].value was removed.

Could the issue be related to particle position or alpha/color values going out of expected range due to this rate/accumulation split, rather than the lifetime check? If you can describe what specific visual artifact you're seeing, I can narrow down the analysis.

@xezon

xezon commented Jul 11, 2026

Copy link
Copy Markdown
Author

I found the hidden particles at sub 30 fps are an explicit feature in ParticleSystem::createParticle

		//
		// Check if particle is below priorities we allow for this FPS or if it being skipped because
		// all particles are being skipped (excluding special fps independent particles at
		// getMinDynamicParticleSkipPriority())
		//
		if( priority < TheGameLODManager->getMinDynamicParticlePriority() ||
				(priority < TheGameLODManager->getMinDynamicParticleSkipPriority() &&
				 TheGameLODManager->isParticleSkipped()) )
			return nullptr;

@xezon

xezon commented Jul 11, 2026

Copy link
Copy Markdown
Author

I cannot figure out why the USA02 Cine JetLenzflare is lagging behind.

Jet Lenzflare in normal gameplay looks normal.

jetlenzflare

@xezon

xezon commented Jul 16, 2026

Copy link
Copy Markdown
Author

Brother @greptileai, please list potential reasons why this change could have introduced lagging-behind particles on fast moving objects or drawables.

It is not observable with all particles, but is 100% with JetLenzflare in a cutscene, that runs at 22 fps.

The effect(s):

ParticleSystem JetLenzflare
  Priority = WEAPON_EXPLOSION
  IsOneShot = NO
  Shader = ADDITIVE
  Type = PARTICLE
  ParticleName = EXLnzFlar2.tga
  AngleZ = 0.00 0.00
  AngularRateZ = 0.00 0.00
  AngularDamping = 1.00 1.00
  VelocityDamping = 1.00 1.00
  Gravity = 0.00
  SlaveSystem = JetLenzflareExhaust
  SlavePosOffset = X:0.00 Y:0.00 Z:0.00
  Lifetime = 3.00 3.00
  SystemLifetime = 0
  Size = 20.00 20.00
  StartSizeRate = 0.00 0.00
  SizeRate = 0.20 0.20
  SizeRateDamping = 1.00 1.00
  Alpha1 = 1.00 1.00 0
  Alpha2 = 0.00 0.00 2
  Alpha3 = 0.00 0.00 0
  Alpha4 = 0.00 0.00 0
  Alpha5 = 0.00 0.00 0
  Alpha6 = 0.00 0.00 0
  Alpha7 = 0.00 0.00 0
  Alpha8 = 0.00 0.00 0
  Color1 = R:0 G:0 B:0 0
  Color2 = R:255 G:255 B:255 1
  Color3 = R:0 G:0 B:0 3
  Color4 = R:0 G:0 B:0 0
  Color5 = R:0 G:0 B:0 0
  Color6 = R:0 G:0 B:0 0
  Color7 = R:0 G:0 B:0 0
  Color8 = R:0 G:0 B:0 0
  ColorScale = 0.00 0.00
  BurstDelay = 0.00 0.00
  BurstCount = 1.00 1.00
  InitialDelay = 0.00 0.00
  DriftVelocity = X:0.00 Y:0.00 Z:0.25
  VelocityType = ORTHO
  VelOrthoX = -1.00 0.00
  VelOrthoY = 0.00 0.00
  VelOrthoZ = 0.00 0.00
  VolumeType = LINE
  VolLineStart = X:0.00 Y:0.00 Z:0.00
  VolLineEnd = X:0.00 Y:0.00 Z:0.00
  IsHollow = YES
  IsGroundAligned = NO
  IsEmitAboveGroundOnly = NO
  IsParticleUpTowardsEmitter = NO
End

ParticleSystem JetLenzflareExhaust
  Priority = WEAPON_EXPLOSION
  IsOneShot = NO
  Shader = ALPHA
  Type = PARTICLE
  ParticleName = EXCloud01.tga
  AngleZ = 0.00 1.00
  AngularRateZ = 0.00 0.00
  AngularDamping = 1.00 1.00
  VelocityDamping = 0.00 0.00
  Gravity = 0.00
  Lifetime = 35.00 35.00
  SystemLifetime = 0
  Size = 0.50 0.50
  StartSizeRate = 0.00 0.00
  SizeRate = 0.60 2.00
  SizeRateDamping = 0.93 0.94
  Alpha1 = 0.50 0.50 0
  Alpha2 = 0.00 0.50 25
  Alpha3 = 0.00 0.00 35
  Alpha4 = 0.00 0.00 0
  Alpha5 = 0.00 0.00 0
  Alpha6 = 0.00 0.00 0
  Alpha7 = 0.00 0.00 0
  Alpha8 = 0.00 0.00 0
  Color1 = R:245 G:204 B:141 0
  Color2 = R:128 G:128 B:128 10
  Color3 = R:0 G:0 B:0 0
  Color4 = R:0 G:0 B:0 0
  Color5 = R:0 G:0 B:0 0
  Color6 = R:0 G:0 B:0 0
  Color7 = R:0 G:0 B:0 0
  Color8 = R:0 G:0 B:0 0
  ColorScale = 0.00 0.00
  BurstDelay = 2.00 2.00
  BurstCount = 1.00 1.00
  InitialDelay = 0.00 0.00
  DriftVelocity = X:0.00 Y:0.00 Z:0.00
  VelocityType = ORTHO
  VelOrthoX = 0.00 0.00
  VelOrthoY = 0.00 0.00
  VelOrthoZ = 0.00 0.00
  VolumeType = POINT
  IsHollow = NO
  IsGroundAligned = NO
  IsEmitAboveGroundOnly = NO
  IsParticleUpTowardsEmitter = NO
  WindMotion = UNUSED
  WindAngleChangeMin = 0.149924
  WindAngleChangeMax = 0.449946
  WindPingPongStartAngleMin = 0.000000
  WindPingPongStartAngleMax = 0.785398
  WindPingPongEndAngleMin = 5.497787
  WindPingPongEndAngleMax = 6.283185
End

@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown

Here are the potential root causes, ranked by likelihood:


1. Spawn position lag due to update-order change (most likely)

In the old code, ParticleSystemManager::update() (which includes spawning) ran from W3DDisplay::draw()after GameClient::update() had already refreshed all drawable bone/animation transforms. New particles therefore spawned at the current render-frame transform.

Now update() runs from GameLogic::update()before GameClient::update(). When updateTransform() calls attachedTo->getTransformMatrix(), it reads the transform from the previous client frame. Particles spawn one render-frame behind the drawable's true visual position. For a fast jet this is a visible displacement, especially because JetLenzflare particles have near-zero own velocity (VelOrthoX = -1.00 0.00, DriftVelocity = 0.25 Z), so they essentially mark where the jet was, not where it is.

This was literally the reason Lorenzen & Wilczynski originally moved particle update into W3DDisplay::draw() — to capture the render-object's transform "while it is still valid."


2. Bone/animated transform vs. logical object position

The particle system is attached to a drawable whose bone positions are resolved during the W3D client pipeline, not during game logic. m_logicalPos captures the object's raw world-space origin, but the lensflare should track the exhaust bone. If the drawable's getTransformMatrix() returns the bone-resolved transform only after client update, then logic-time updateTransform() will get an outdated bone position regardless of object speed.


3. timeScale doesn't represent actual elapsed wall-clock time per render frame

getActualLogicTimeScaleOverFpsRatio() appears to be a fixed-ratio value (logic step duration / target FPS frame duration), not a measurement of actual elapsed time since the last draw() call. At genuinely variable fps (like 22fps in a heavy cutscene), the actual render-frame wall-clock time fluctuates. If timeScale is a fixed ratio (e.g., logicHz / targetRenderHz), particle velocity integration will over- or under-shoot the correct world displacement each frame, and the error accumulates as visible lag against the emitter.


4. Multiple logic steps between render frames are not compensated

At 22fps render / 30fps logic, there will be render frames where 1 logic step fires and others where 2 fire before the next draw() call. If draw() is called once per render frame with a fixed timeScale that assumes 1 logic step, and 2 logic steps occurred, particles integrate only half the needed physics displacement for that frame, lagging behind the emitter.


5. emissionAdjustment interpolation uses only the logic-frame delta

frameDeltaPos = m_logicalPos - m_lastLogicalPos spreads burst particles along the path between two consecutive logic positions. At 22fps render / 30fps logic, this is fine for the spread itself, but the spread starts at m_lastLogicalPos (one full logic frame back) rather than at the emitter's current render-frame position. For fast-moving emitters the entire spawn cluster is displaced backward.


In short: The most actionable cause is #1 — particles now spawn with one client-update-cycle of position lag that the original design deliberately avoided by running update() inside the render pipeline. The fix would be to either run updateTransform() / updateLogicalPos() a second time at the start of draw() before any new particles are spawned (but spawning already happened), or to record and use the render-time emitter position separately from m_logicalPos for the purpose of computing the visual emitter origin in draw().

@xezon
xezon force-pushed the xezon/decouple-particle-update branch 2 times, most recently from 987f04b to 9a5930a Compare August 29, 2026 17:13
@xezon
xezon marked this pull request as ready for review August 29, 2026 17:17
@xezon
xezon requested a review from Skyaero42 August 29, 2026 17:17
@xezon
xezon dismissed Skyaero42’s stale review August 29, 2026 17:17

Code has changed

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Decouple particle rendering from fixed logic updates

✨ Enhancement 🐞 Bug fix 🕐 40+ Minutes

Grey Divider

AI Description

• Separates particle lifetime logic from frame-paced visual simulation.
• Makes motion, damping, wind, color, and size updates frame-rate independent.
• Preserves attachment, save compatibility, and particle behavior across both game variants.
Diagram

sequenceDiagram
    participant GL as Game Logic
    participant GC as Game Client
    participant PM as Particle Manager
    participant PS as Particle System
    participant FP as Frame Pacer
    participant RD as W3D Display
    participant P as Particle
    GL->>GC: Complete logic frame
    GC->>PM: Logic update
    PM->>PS: Update lifetime
    PS->>P: Advance keys
    RD->>FP: Read time scale
    FP-->>RD: Delta ratio
    RD->>PM: Render update
    PM->>PS: Draw scaled
    PS->>P: Integrate visuals
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Fixed-step simulation with interpolation
  • ➕ Keeps rendering side-effect free and simulation deterministic.
  • ➕ Avoids visual state changing multiple times per logic frame.
  • ➖ Requires previous/current particle snapshots and interpolation support.
  • ➖ Adds memory and complexity for short-lived particles, emissions, and keyframes.
2. Accumulated fixed render substeps
  • ➕ Reuses original fixed-step equations with predictable behavior.
  • ➕ Avoids fractional powers for damping calculations.
  • ➖ Can perform multiple particle traversals per display frame.
  • ➖ Needs remainder handling and may produce visible stepping.

Recommendation: The PR's frame-scaled render update is pragmatic because particles are visual effects and avoids snapshot storage or repeated substeps. Keep this approach, but validate unusual frame ratios, paused rendering, negative damping inputs, and retail keyframe behavior; interpolation would be preferable only if render-side mutation later causes determinism or multi-view rendering issues.

Files changed (10) +602 / -272

Enhancement (6) +589 / -251
ParticleSys.hDefine separate particle logic and render update interfaces +35/-6

Define separate particle logic and render update interfaces

• Adds draw paths for particles, systems, and the manager; extracts transform and visibility helpers; and removes the unused particle position member. Declares damping validation and updates the dummy manager interface.

Core/GameEngine/Include/GameClient/ParticleSys.h

ParticleSys.cppImplement frame-paced particle rendering and compatibility safeguards +414/-235

Implement frame-paced particle rendering and compatibility safeguards

• Moves motion, gravity, wind, rotation, size, alpha, and color integration into render updates using frame-rate-independent damping. Retains lifetime and keyframe progression in logic updates, extracts attachment transform and visibility handling, validates damping, and versions serialization after removing the unused field.

Core/GameEngine/Source/GameClient/System/ParticleSys.cpp

BaseType.hAdd RGBColor arithmetic operators +120/-0

Add RGBColor arithmetic operators

• Adds component-wise and scalar arithmetic operators used to simplify frame-scaled particle color updates.

Core/Libraries/Include/Lib/BaseType.h

GameClient.cppGate Generals particle logic updates on completed logic frames +2/-2

Gate Generals particle logic updates on completed logic frames

• Runs the particle manager logic update only when game logic advances, using the subsystem update wrapper.

Generals/Code/GameEngine/Source/GameClient/GameClient.cpp

W3DDisplay.cppRun Generals particle render updates from W3D draw +2/-0

Run Generals particle render updates from W3D draw

• Invokes the particle manager render path after view updates so visual particle state advances each display frame.

Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp

W3DDisplay.cppSeparate GeneralsMD particle logic and render dispatch +16/-8

Separate GeneralsMD particle logic and render dispatch

• Runs particle logic only after game logic advances, resolves the observed player for shroud checks, and dispatches render updates every display frame.

GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp

Bug fix (1) +6 / -6
ParticleEditorDialog.cppPrevent negative particle damping in editor inputs +6/-6

Prevent negative particle damping in editor inputs

• Clamps size-rate, angular, and velocity damping values to non-negative ranges when applying editor changes.

Core/Tools/ParticleEditor/ParticleEditorDialog.cpp

Refactor (3) +7 / -15
BoneFXUpdate.cppClarify bone effect object attachment +3/-4

Clarify bone effect object attachment

• Renames the local attached object variable while preserving particle attachment and hidden-drawable stopping behavior.

Generals/Code/GameEngine/Source/GameLogic/Object/Update/BoneFXUpdate.cpp

GameClient.cppRemove obsolete GeneralsMD client particle update block +1/-7

Remove obsolete GeneralsMD client particle update block

• Removes the inactive client-side update scaffold because particle logic and rendering are coordinated in W3DDisplay.

GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp

BoneFXUpdate.cppClarify GeneralsMD bone effect object attachment +3/-4

Clarify GeneralsMD bone effect object attachment

• Renames the local attached object variable while preserving particle attachment and hidden-drawable stopping behavior.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/BoneFXUpdate.cpp

@xezon

xezon commented Aug 29, 2026

Copy link
Copy Markdown
Author

After several review rounds with Claude Opus I have now updated and polished this change further. It is ready for human re-review.

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 29, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Particle logic updates repeatedly ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new GeneralsMD display-loop call invokes ParticleSystemManager::UPDATE() on every iteration
while hasUpdated() remains true, and removal of the manager's logic-frame guard allows one game
frame to age and emit particles multiple times during tactical-camera freeze loops. This shortens
particle/system lifetimes and consumes burst delays without corresponding logic advancement.
Code

GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp[1961]

+				TheParticleSystemManager->UPDATE();
Evidence
The display code places the update inside a repeating freeze loop, while m_hasUpdated is only
cleared by the next logic pre-update. Each manager update advances emission delays, particle
lifetimes, and system lifetime.

GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp[1886-1886]
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp[1940-1963]
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp[2100-2105]
GeneralsMD/Code/GameEngine/Source/Common/GameEngine.cpp[316-320]
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp[3403-3416]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[2043-2055]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[2091-2115]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[2124-2129]

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

## Issue description
Prevent particle logic from updating multiple times during one game-logic frame when the GeneralsMD render loop repeats.
## Issue Context
`GameLogic::hasUpdated()` stays true until the next logic `preUpdate()`, so it is not a once-only consumption flag. Restore a per-logic-frame guard in the particle manager or move the update to a location that executes exactly once per logic frame.
## Fix Focus Areas
- GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp[1957-1962]
- Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[3183-3195]

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


2. Gravity displacement remains FPS-dependent ✓ Resolved 🐞 Bug ≡ Correctness
Description
Particle::draw() updates velocity from acceleration and then integrates position from that
post-acceleration velocity, so subdividing one logic interval changes the endpoint. With damping 1,
zero initial velocity, and gravity a, one step of scale 1 moves by a, while two half-steps move
by only 0.75a, making particle trajectories depend on render FPS.
Code

Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[R499-501]

+	m_pos.x += (m_vel.x + driftVel->x) * timeScale;
+	m_pos.y += (m_vel.y + driftVel->y) * timeScale;
+	m_pos.z += (m_vel.z + driftVel->z) * timeScale;
Evidence
FramePacer supplies a logic-FPS/render-FPS ratio, gravity is reapplied before every fractional draw,
and acceleration is reset afterward. The added position equation therefore yields the demonstrated
1.0 versus 0.75 displacement discrepancy solely from render subdivision.

Core/GameEngine/Source/Common/FramePacer.cpp[205-210]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[457-482]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[491-501]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[562-565]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[2300-2310]
Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[3237-3248]

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

## Issue description
Make acceleration-driven particle displacement produce the same endpoint regardless of render-step subdivision.
## Issue Context
The velocity damping transition is subdivision-compatible, but position currently uses only the post-update velocity. Implement the matching closed-form/semigroup displacement transition, including acceleration and damping edge cases, and test one full step against multiple fractional steps.
## Fix Focus Areas
- Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[491-501]
- Core/GameEngine/Source/GameClient/System/ParticleSys.cpp[2300-2310]

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


Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp Outdated
Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp
Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp Outdated
Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp Outdated
Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp
@xezon
xezon requested review from Skyaero42 and bobtista August 30, 2026 10:48
{
m_alpha = m_alphaKey[ m_alphaTargetKey ].value;
m_alphaTargetKey++;
computeAlphaRate();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Was removing the alpha key snap intentional? Retail sets m_alpha to the reached key value before computing the next rate. computeAlphaRate() is key-to-key, so any render-step drift now carries into later segments.

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 think it was intentional but I also cannot remember all the details anymore. I was unable to observe bugs from it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I dug into it more. This is a one-frame phase lead, not drift. We could restore m_alpha = m_alphaKey[m_alphaTargetKey].value before incrementing the target, then have draw() skip alpha integration when m_alphaTargetKey > 1 && frameCount == m_alphaKey[m_alphaTargetKey - 1].frame.
frameCount can be derived from TheGameClient->getFrame() - m_createTimestamp. This holds the snapped value for the boundary frame and starts the new rate on the next frame without adding state.
I don't think this is materially noticeable in normal play. The fix would be more retail-compatible, but I'm happy to approve either way.

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 am not convinced the above observations are correct. I remember I extensively tested this and the + KeyFrameDelay on one of these advances and not on the other already correctly migrated it.

If there are observable issues with the code as presented, please say with which particle to observe this with.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I measured this with ToxinSpray in three builds: the base, this branch, and this branch with the snap restored:

age base head head+snap
4 0.312500 0.312500 0.312500
5 0.250000 0.305357 0.242857
20 0.142857 0.198214 0.135714
38 0.014286 0.069643 0.007143

The missing snap leaves alpha 0.055357 above the base for the remaining 34 frames. Restoring it removes that persistent offset; the remaining 0.007143 is the next rate being applied in draw() on the boundary frame, which the skip described above handles.
The same alpha keys are used by ToxinBuildingClearSpray, ToxinUpgradedBuildingClearSpray, AnthraxSpray, AnthraxGammaSpray and ScudStormBuildingGoo. The KeyFrameDelay comment exempts alpha because retail overwrote the accumulated value at the key—the line removed here.

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.

Ok. I did a long chat session with Claude Opus about this and we settled on a solution that moves the alpha frame computation entirely to the render update, which then solves this problem. The Particle class loses its m_alphaRate member, but is preserved for RETAIL_COMPATIBLE_XFER_SAVE. The implementations for alpha rate and color rate are now different, but it was not easily possible to do the same for the color rate and not introduce a visual divergence. Maybe it can be revisited in the future.

The logic for key frames not starting at 0 has changed for not PRESERVER_RETAIL_PARTICLES. They now legitimately hold the value before reaching the first frame instead of advancing to the first key frame and then holding its value towards the second frame. This is more true to what the INI setup says.

It's all part of the last fixup commit.

The code is only getting more complicated with every new comment added to this pull 😆

@Skyaero42 Skyaero42 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me.

@xezon
xezon force-pushed the xezon/decouple-particle-update branch from b911329 to 439c4c9 Compare September 2, 2026 12:00
@xezon
xezon force-pushed the xezon/decouple-particle-update branch from 439c4c9 to 6143a7a Compare September 2, 2026 15:32
Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp
Comment thread Core/GameEngine/Source/GameClient/System/ParticleSys.cpp
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.

6 participants