Recalculate particle local bounds when the spawn volume changes - #9192
Conversation
The emitter stored prevEmitterExtents as a reference to the very Vec3 the component handed it, so changing the extents in place compared the object with itself and never triggered a recalculation. The snapshot was also never refreshed after a recalculation, so replacing the Vec3 instead made calculateLocalBounds run on every frame from then on, and the check was skipped entirely for CPU emitters. Take a copy of the spawn volume at the top of calculateLocalBounds, so every recalculation records the inputs it used, and drop the useCpu gate. Curve setters refresh the bounds too, as the velocity and scale graphs feed into them. Also remove prevWorldBoundsSize, prevWorldBoundsCenter and worldBoundsSize, which have been written but never read since pack8 was removed in #8926.
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
mvaligursky
left a comment
There was a problem hiding this comment.
Automated PR review by Codex (GPT-5).
I found one correctness gap in the graph-driven bounds refresh. Apart from that finding, I reviewed the spawn-volume snapshot lifecycle, replacement and in-place mutation paths, CPU/GPU parity, trail/world-AABB propagation, graph setter behavior, obsolete pack8 state removal, allocation/per-frame cost, and test coverage. The snapshot copy and refresh placement are sound, the CPU gate removal is appropriate, and the removed world-bound fields have no remaining consumers.
Validation performed: all current CI checks are green, git diff --check passes, and the five added particle-emitter tests pass locally. I also added a focused scaleGraph2 regression case; it fails on the current head because the recalculation still derives the particle size from only the first scale curve, as detailed inline.
calculateLocalBounds padded the bounds with the maximum of the primary scale curve only, while both the CPU and GPU paths interpolate the rendered size between scaleGraph and scaleGraph2, so a large secondary curve was left out of the bounds. Take the maximum over both curves, and by magnitude, as a negative scale mirrors the particle without shrinking it - the running maximum started at 0, so a fully negative curve used to pad nothing at all.
mvaligursky
left a comment
mvaligursky
left a comment
There was a problem hiding this comment.
Automated re-review by Codex (GPT-5) of changes through 59b840bdaa203e192ec03c87e675be3546800ad9.
The response and update fully address the previous finding. calculateLocalBounds() now takes the maximum magnitude across both quantized scale curves, which safely covers every value produced by interpolation and correctly handles negative scales as mirrored geometry. The new tests cover primary-curve growth, the exact secondary-curve regression, and negative magnitude. The negative-scale correction belongs in this PR because it is the same maxScale under-coverage defect and does not add meaningful scope or risk.
Validation: the focused particle-emitter suite passes 8/8, ESLint passes for the changed implementation and test, git diff --check passes, and all current CI checks are green. The original review thread has been resolved. No additional findings.
1 participant
Fixes #811.
Changing
emitterExtents(oremitterRadius) at runtime did not update the particle emitter's bounds, so the mesh instance kept a stale culling AABB: the emitter can pop out of view while its particles are plainly on screen, and the editor's selection box for a particle system shows the wrong size.The original 2016 issue is about pack8, whose lossy RGBA8 state textures normalized particle positions into that box - a stale box then visibly corrupted the positions. #8926 removed pack8 (v2.20.0) and nothing in the particle shaders depends on emitter bounds any more, so all that is left of the issue is the culling AABB, and it is still broken in three ways:
prevEmitterExtentswas assignedthis.emitterExtents, a reference to the veryVec3the component handed the emitter. Mutating the extents in place -ps.emitterExtents.set(...),.x = n,.copy(v)- compared the object with itself, so the change was never noticed.Vec3(what the component setter and the editor do) worked once and then re-rancalculateLocalBoundson every frame, forever.if (!this.useCpu), added in Particle world bounds cpu fix #2059 with no stated reason, so CPU emitters never tracked the spawn volume at all.Changes:
prevEmitterExtentsis now the emitter's ownVec3, and the snapshot of both it andprevEmitterRadiusmoved to the top ofcalculateLocalBounds, so every recalculation records the inputs it was calculated for. That keeps in place mutation working - which a setter driven fix cannot - andrebuild()gets the bookkeeping for free.useCpugate, so CPU emitters recalculate too._setGraphPropertynow callscalculateLocalBounds()afterrebuildGraphs(). The velocity and scale graphs are its main inputs, and unlike the spawn volume they cannot be cheaply compared each frame, so assigning a curve at runtime used to leave the bounds stale with no check to catch it.Note the bounds stay deliberately conservative:
calculateLocalBoundspads every axis by the integrated world velocity magnitude, which is isotropic because that velocity is in world space while the bounds are local, andworldBoundsis a union over a trail that swaps everylifetime, so growth applies within a frame while shrinking lags. Both predate this PR - they were simply invisible while the bounds never moved. Tightening the padding would need the node rotation folded in and is left out of here.API Changes:
ParticleEmitter#prevWorldBoundsSize,ParticleEmitter#prevWorldBoundsCenterandParticleEmitter#worldBoundsSize. All three were pack8 machinery, added in 2016 to feed theinBoundsSize/outBoundsSizeuniforms that mapped positions into the bounds box, and have been written but never read since refactor(particles): use lossless RGBA32U fallback instead of pack8 #8926. Undocumented, and unused by the editor.Performance:
calculateLocalBoundsno longer runs on every frame for the lifetime of an emitter whose extents were assigned once.