tweak(particlesys): Decouple Particles render update from logic step - #2709
tweak(particlesys): Decouple Particles render update from logic step#2709xezon wants to merge 10 commits into
Conversation
|
| 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
|
I presume this PR fixes #2467. |
| psys->attachToObject(building); | ||
| Drawable *drawable = building->getDrawable(); | ||
| psys->attachToObject(object); | ||
| Drawable *drawable = object->getDrawable(); |
There was a problem hiding this comment.
This rename was just a side quest from when looking around particle things.
| ParticleSystemInfo::WindMotion windMotion = m_system->getWindMotion(); | ||
| // monitor lifetime | ||
| if (m_lifetimeLeft && --m_lifetimeLeft == 0) | ||
| return false; |
There was a problem hiding this comment.
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.
b94750d to
b86a504
Compare
|
The plane trails do not fade out gracefully. That needs looking into. |
|
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. |
b86a504 to
278f319
Compare
|
I will break the dummy particle manager changes off of this pull. |
|
ParticleSystemManagerDummy changes split off to #2740 |
|
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. |
|
@greptileai Particle definition: |
|
The root cause is the interaction between the decoupled logic/render updates, the initial black color of JetLenzflare particles, and the Why it happens: JetLenzflare uses With the decoupled design, At logic = 30 FPS, render = 25 FPS (or lower), the timing is:
The 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 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 |
|
@greptileai |
|
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 Looking more carefully at the decoupling for other potential causes: one thing that stands out is that 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. |
|
I found the hidden particles at sub 30 fps are an explicit feature in //
// 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; |
|
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): |
|
Here are the potential root causes, ranked by likelihood: 1. Spawn position lag due to update-order change (most likely) In the old code, Now This was literally the reason Lorenzen & Wilczynski originally moved particle update into 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. 3.
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 5.
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 |
987f04b to
9a5930a
Compare
PR Summary by QodoDecouple particle rendering from fixed logic updates
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
|
After several review rounds with Claude Opus I have now updated and polished this change further. It is ready for human re-review. |
Code Review by Qodo
1.
|
| { | ||
| m_alpha = m_alphaKey[ m_alphaTargetKey ].value; | ||
| m_alphaTargetKey++; | ||
| computeAlphaRate(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it was intentional but I also cannot remember all the details anymore. I was unable to observe bugs from it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😆
…rticle::update() into additional functions (#2709)
…icle::draw() (#2709)
b911329 to
439c4c9
Compare
439c4c9 to
6143a7a
Compare

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
Initialize particle templates with RETAIL_COMPATIBLE_CRC