feat(particle): add SubEmittersModule #2982
Conversation
Sub Emitters fire additional ParticleRenderers on parent particle lifecycle events. Each slot references a target renderer, the trigger event (Birth or Death), an inherit-properties bitmask (Color / Size / Rotation), and a per-event probability. Emit count per event comes from the sub renderer's own Emission module (sum of t=0 burst counts; defaults to 1), aligning with Unity's SubEmittersModule semantics. Birth fires at the parent particle's emission position. Death fires at an approximate end position computed via ballistic formula (a_ShapePos + dir·speed·lifetime + ½·gravity·r0·lifetime²); VOL/FOL/Noise contributions are not accounted for and will introduce drift when those modules are enabled — documented in code. Self-reference is silently bailed; nested sub-emit chains use per-instance scratch buffers so static temps cannot clobber outer dispatches.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces particle sub-emitters, enabling child particle generators to spawn on parent Birth or Death events with optional color, size, and rotation inheritance. The implementation includes lifecycle event dispatch, probability gating, ballistic death-position estimation, and recursive emission prevention. ChangesParticle Sub-Emitters: Lifecycle Event-Driven Spawning
Sequence Diagram(s)sequenceDiagram
participant Parent as ParticleGenerator
participant SubModule as SubEmittersModule
participant Child as Target ParticleGenerator
Parent->>Parent: _addNewParticle() (Birth)
Parent->>SubModule: _onParticleBirth(worldPos, payload)
SubModule->>SubModule: check probability & compute bursts
SubModule->>Child: _emitFromSubEmitter(worldPos, count, overrides)
Child->>Child: transform world->local, set overrides, _addNewParticle()
Parent->>Parent: _retireActiveParticles()
Parent->>Parent: _dispatchDeathEvent() -> SubModule._onParticleDeath
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
hhhhkrx
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2982 +/- ##
===========================================
+ Coverage 78.15% 78.26% +0.11%
===========================================
Files 900 905 +5
Lines 99524 100270 +746
Branches 10254 10345 +91
===========================================
+ Hits 77779 78479 +700
- Misses 21571 21617 +46
Partials 174 174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/src/core/particle/SubEmitter.test.ts (1)
50-83: ⚡ Quick winCleanup leaks on assertion failure across tests.
Each test relies on inline
parent.entity.destroy()/child.entity.destroy()at the bottom. If any earlierexpectfails, those destroy calls never run and the entities remain on the sharedscene, whereengine.update()will keep simulating them. Subsequent tests'_getAliveParticleCount()assertions can then pick up emissions from leaked entities and report misleading failures. Move teardown into anafterEach(or wrap intry/finally) so destruction is guaranteed.♻️ Suggested test scaffolding
describe("SubEmitter", () => { let engine: Engine; + const createdEntities: any[] = []; beforeAll(async function () { engine = await WebGLEngine.create({ canvas: document.createElement("canvas") }); // ... engine.run(); }); + + afterEach(() => { + while (createdEntities.length) createdEntities.pop()?.destroy(); + });Then push every created entity into
createdEntitiesand drop the inline.destroy()calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SubEmitter.test.ts` around lines 50 - 83, Tests leak entities when assertions fail because parent.entity.destroy() and child.entity.destroy() are called only at the end of the test; change teardown to guarantee destruction by either (preferred) collecting created entities from calls to createParticleRenderer (e.g., push returned parent and child into a createdEntities array) and removing inline .destroy() calls, then add an afterEach that iterates createdEntities calling .entity.destroy() (and clearing the array), or wrap each test body in try/finally and destroy parent/child in finally; update the test file's lifecycle hooks (replace per-test inline destroys in the "SubEmitter" spec with an afterEach cleanup) so leaked entities cannot affect subsequent _getAliveParticleCount() assertions.packages/core/src/particle/ParticleGenerator.ts (1)
1230-1240: 💤 Low value
!_suppressSubEmitterDispatchguard here is effectively unreachable.
_retireActiveParticlesis only called from_update, never from inside_emitFromSubEmitter, so_suppressSubEmitterDispatchis alwaysfalsewhen this pre-flight runs. The guard is harmless but adds noise; consider dropping it (or adding a comment that it's defensive against future call sites) to keep the intent of the flag focused on Birth recursion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/particle/ParticleGenerator.ts` around lines 1230 - 1240, In _retireActiveParticles, the !this._suppressSubEmitterDispatch check is effectively unreachable because _retireActiveParticles is only called from _update (not from _emitFromSubEmitter); remove the !_suppressSubEmitterDispatch clause so the conditional reads just on subEmitters.enabled, or if you want to keep a defensive note, add a one-line comment above the check explaining it exists solely for future call sites to avoid confusing readers; the change should be applied in the block that scans subEmitters.subEmitters for ParticleSubEmitterType.Death inside the ParticleGenerator class (method _retireActiveParticles) and must preserve the existing loop and hasDeathSlot behavior.packages/core/src/particle/modules/SubEmittersModule.ts (2)
110-114: 💤 Low valueSelf-reference guard only catches direct cycles.
targetGen === this._generatorblocksA → A, butA → B → A(or longer chains, especially throughDeathslots) will still produce a per-frame feedback loop that grows unbounded as long as both systems can spawn each other. Consider either documenting the limitation (sub-emitter cycles are the user's responsibility) or tracking an_inDispatchset during a top-level dispatch tick to break indirect cycles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 110 - 114, The current guard only prevents direct self-reference (target.generator === this._generator) but allows indirect cycles like A→B→A; modify SubEmittersModule's dispatch logic (the code referencing target.generator and this._generator) to track active generators during a top-level dispatch using a set (e.g., _inDispatch) keyed by generator instance or id and bail if a target generator is already in the set, then remove it when that dispatch branch completes; alternatively, if you prefer not to change runtime behavior, add a clear comment in SubEmittersModule explaining that indirect sub-emitter cycles (A→B→A) are not prevented and are the user's responsibility to avoid.
119-128: ⚡ Quick winBurst count uses the same RNG as the probability gate.
_probabilityRandis consumed conditionally in the probability check (only whenemitProbability < 1) and unconditionally here forburst.count.evaluate(...). That couples two semantically independent random streams, so flippingemitProbabilityfrom1to0.999shifts every subsequent burst-count sample. Consider using a dedicatedRand(e.g. seeded fromParticleRandomSubSeeds.SubEmitter ^ 0x...) for burst count evaluation, or simply passMath.random()/an existing per-renderer rand to keep the probability stream pristine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 119 - 128, The burst count evaluation currently uses the shared RNG _probabilityRand (in SubEmittersModule) which couples it to the probability gate; change burst.count.evaluate(...) to use a dedicated RNG instead of _probabilityRand—either create/obtain a specific rand seeded for sub-emitter burst sampling (e.g. derive from ParticleRandomSubSeeds.SubEmitter ^ 0x... and use that rand) or call a renderer-local RNG/Math.random() when computing burst.count.evaluate(undefined, ...); update the loop that calls burst.count.evaluate to pass this new rand so the probability stream (_probabilityRand) remains untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 1280-1320: The death-event world position can read a stale gravity
modifier because instanceVertices[particleOffset + 19] is only set for
Constant/TwoConstants in _addNewParticle, so either ensure the slot is
initialized there for Curve/TwoCurves (set instanceVertices[offset + 19] to 0 or
the evaluated curve value) or, in the death-position calculation inside
_computeDeathWorldPos (where gravityMod is read), fall back to evaluating
main.gravityModifier at (playTime - particleAge) when the stored value is
invalid; update the doc comment near the death-event handling to mention
curve-mode gravity as a caveat if you choose initialization rather than
runtime-evaluation.
- Around line 1099-1130: The Prettier errors come from multi-line conditionals
and .set(...) calls in ParticleGenerator.ts; collapse the long if condition
starting with "!this._suppressSubEmitterDispatch && subEmitters.enabled &&
subEmitters.subEmitters.length > 0" into a single line, and collapse the
multi-line calls to parentSize.set(...) and parentRotation.set(...) into
single-line invocations; locate these by the symbols
_suppressSubEmitterDispatch, subEmitters, Vector3.transformByQuat,
birthWorldPos, parentColor (reading instanceVertices[offset + N]),
parentSize.set, parentRotation.set, and main.startRotation3D and reformat those
expressions to a single-line style to satisfy Prettier.
- Around line 1145-1188: _emitFromSubEmitter is converting worldPosition into
local emission space but skips scaling; after computing localPos
(ParticleGenerator._tempVector30) and applying inverse rotation
(Quaternion.invert/world transformByQuat), fetch the position scale from
main._getPositionScale() and apply its inverse to localPos (i.e., divide or
multiply by reciprocal per-component) so the emitted shape positions match the
scaled simulation space; ensure this occurs before calling _addNewParticle and
does not mutate the original worldPosition variable.
In `@tests/src/core/particle/SubEmitter.test.ts`:
- Around line 16-29: The updateEngine helper overwrites global performance.now
and never restores it; capture the original (e.g., const origNow =
performance.now) before replacing and restore it after running the frames (or
switch to vi.spyOn/performance.get or vi.useFakeTimers which auto-cleans between
tests). Update the updateEngine function to save performance.now, replace it for
deterministic timing, run the engine.update loop, then restore the saved
original so other tests aren’t affected; reference updateEngine and its use of
performance.now when making the change.
---
Nitpick comments:
In `@packages/core/src/particle/modules/SubEmittersModule.ts`:
- Around line 110-114: The current guard only prevents direct self-reference
(target.generator === this._generator) but allows indirect cycles like A→B→A;
modify SubEmittersModule's dispatch logic (the code referencing target.generator
and this._generator) to track active generators during a top-level dispatch
using a set (e.g., _inDispatch) keyed by generator instance or id and bail if a
target generator is already in the set, then remove it when that dispatch branch
completes; alternatively, if you prefer not to change runtime behavior, add a
clear comment in SubEmittersModule explaining that indirect sub-emitter cycles
(A→B→A) are not prevented and are the user's responsibility to avoid.
- Around line 119-128: The burst count evaluation currently uses the shared RNG
_probabilityRand (in SubEmittersModule) which couples it to the probability
gate; change burst.count.evaluate(...) to use a dedicated RNG instead of
_probabilityRand—either create/obtain a specific rand seeded for sub-emitter
burst sampling (e.g. derive from ParticleRandomSubSeeds.SubEmitter ^ 0x... and
use that rand) or call a renderer-local RNG/Math.random() when computing
burst.count.evaluate(undefined, ...); update the loop that calls
burst.count.evaluate to pass this new rand so the probability stream
(_probabilityRand) remains untouched.
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 1230-1240: In _retireActiveParticles, the
!this._suppressSubEmitterDispatch check is effectively unreachable because
_retireActiveParticles is only called from _update (not from
_emitFromSubEmitter); remove the !_suppressSubEmitterDispatch clause so the
conditional reads just on subEmitters.enabled, or if you want to keep a
defensive note, add a one-line comment above the check explaining it exists
solely for future call sites to avoid confusing readers; the change should be
applied in the block that scans subEmitters.subEmitters for
ParticleSubEmitterType.Death inside the ParticleGenerator class (method
_retireActiveParticles) and must preserve the existing loop and hasDeathSlot
behavior.
In `@tests/src/core/particle/SubEmitter.test.ts`:
- Around line 50-83: Tests leak entities when assertions fail because
parent.entity.destroy() and child.entity.destroy() are called only at the end of
the test; change teardown to guarantee destruction by either (preferred)
collecting created entities from calls to createParticleRenderer (e.g., push
returned parent and child into a createdEntities array) and removing inline
.destroy() calls, then add an afterEach that iterates createdEntities calling
.entity.destroy() (and clearing the array), or wrap each test body in
try/finally and destroy parent/child in finally; update the test file's
lifecycle hooks (replace per-test inline destroys in the "SubEmitter" spec with
an afterEach cleanup) so leaked entities cannot affect subsequent
_getAliveParticleCount() assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b496579f-b5cf-4dce-99ce-a3e5edb8cf82
📒 Files selected for processing (8)
packages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/enums/ParticleRandomSubSeeds.tspackages/core/src/particle/enums/ParticleSubEmitterProperty.tspackages/core/src/particle/enums/ParticleSubEmitterType.tspackages/core/src/particle/index.tspackages/core/src/particle/modules/SubEmitter.tspackages/core/src/particle/modules/SubEmittersModule.tstests/src/core/particle/SubEmitter.test.ts
| _emitFromSubEmitter( | ||
| count: number, | ||
| worldPosition: Vector3, | ||
| inheritColor: Color, | ||
| inheritSize: Vector3, | ||
| inheritRotation: Vector3 | ||
| ): void { | ||
| if (count <= 0) return; | ||
|
|
||
| const main = this.main; | ||
| const notRetired = this._getNotRetiredParticleCount(); | ||
| const available = main.maxParticles - notRetired; | ||
| if (available <= 0) return; | ||
| if (count > available) count = available; | ||
|
|
||
| const transform = this._renderer.entity.transform; | ||
| const worldPos = transform.worldPosition; | ||
| const worldRot = transform.worldRotationQuaternion; | ||
|
|
||
| // Convert event world position into local emission space for a_ShapePos | ||
| const localPos = ParticleGenerator._tempVector30; | ||
| Vector3.subtract(worldPosition, worldPos, localPos); | ||
| const invRot = ParticleGenerator._tempQuat0; | ||
| Quaternion.invert(worldRot, invRot); | ||
| Vector3.transformByQuat(localPos, invRot, localPos); | ||
|
|
||
| const direction = ParticleGenerator._tempVector31; | ||
| direction.set(0, 0, -1); | ||
|
|
||
| this._subEmitColorOverride = inheritColor; | ||
| this._subEmitSizeOverride = inheritSize; | ||
| this._subEmitRotationOverride = inheritRotation; | ||
| this._suppressSubEmitterDispatch = true; | ||
|
|
||
| const playTime = this._playTime; | ||
| for (let i = 0; i < count; i++) { | ||
| this._addNewParticle(localPos, direction, transform, playTime); | ||
| } | ||
|
|
||
| this._subEmitColorOverride = null; | ||
| this._subEmitSizeOverride = null; | ||
| this._subEmitRotationOverride = null; | ||
| this._suppressSubEmitterDispatch = false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# See how positionScale is applied elsewhere so the inverse path can mirror it.
ast-grep --pattern '_getPositionScale() { $$$ }'
rg -nP -C3 'positionScale' --type=ts -g 'packages/core/src/particle/**'Repository: galacean/engine
Length of output: 4352
🏁 Script executed:
rg -nA10 '_getPositionScale\(\)' packages/core/src/particle/modules/MainModule.tsRepository: galacean/engine
Length of output: 427
🏁 Script executed:
rg -nA5 '_emitFromSubEmitter' packages/core/src/particle/ParticleGenerator.tsRepository: galacean/engine
Length of output: 546
🏁 Script executed:
# Check the full context of _emit to understand positionScale application
sed -n '300,330p' packages/core/src/particle/ParticleGenerator.tsRepository: galacean/engine
Length of output: 1304
🏁 Script executed:
sed -n '1145,1188p' packages/core/src/particle/ParticleGenerator.tsRepository: galacean/engine
Length of output: 1561
🏁 Script executed:
rg -i 'subemit|sub.emit' packages/core/src/particle --type=ts -lRepository: galacean/engine
Length of output: 429
🏁 Script executed:
# Check if there are any tests for sub-emitters
fd -i 'test|spec' packages/core --type f -path '*particle*' | head -20Repository: galacean/engine
Length of output: 228
🏁 Script executed:
# Look for any scale-related sub-emitter logic or comments
rg -n -C2 'sub.*scale|scale.*sub' packages/core/src/particle --type=tsRepository: galacean/engine
Length of output: 41
🏁 Script executed:
rg -n -B5 -A10 '_emitFromSubEmitter' packages/core/src/particle/modules/SubEmittersModule.tsRepository: galacean/engine
Length of output: 563
🏁 Script executed:
# Also check if positionScale is referenced anywhere in sub-emitter logic
rg -n 'positionScale' packages/core/src/particle/modules/SubEmitter*.tsRepository: galacean/engine
Length of output: 41
Sub-emitter position conversion missing positionScale scaling.
_emit applies position.multiply(positionScale) via main._getPositionScale() to scale shape positions into the correct simulation space. _emitFromSubEmitter converts the event's world position to local space via translation and inverse rotation only—it never accounts for scale. When the target generator has non-unit scale (e.g., via ScaleMode.World, ScaleMode.Local, or ScaleMode.Shape with scaled parent), the emitted particle's shape position will be off by the scale factor, causing visible offset when particles move under velocity-over-lifetime, force-over-lifetime, or in Local simulation space.
Fix: Apply inverse of main._getPositionScale() to localPos after the rotation transformation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/particle/ParticleGenerator.ts` around lines 1145 - 1188,
_emitFromSubEmitter is converting worldPosition into local emission space but
skips scaling; after computing localPos (ParticleGenerator._tempVector30) and
applying inverse rotation (Quaternion.invert/world transformByQuat), fetch the
position scale from main._getPositionScale() and apply its inverse to localPos
(i.e., divide or multiply by reciprocal per-component) so the emitted shape
positions match the scaled simulation space; ensure this occurs before calling
_addNewParticle and does not mutate the original worldPosition variable.
| const lifetime = instanceVertices[particleOffset + 3]; | ||
| const startSpeed = instanceVertices[particleOffset + 18]; | ||
| const gravityMod = instanceVertices[particleOffset + 19]; | ||
|
|
||
| // Local-space end position before world rotation: a_ShapePos + dir·speed·lifetime | ||
| const local = this._eventWorldPos; | ||
| local.set( | ||
| instanceVertices[particleOffset + 0] + instanceVertices[particleOffset + 4] * startSpeed * lifetime, | ||
| instanceVertices[particleOffset + 1] + instanceVertices[particleOffset + 5] * startSpeed * lifetime, | ||
| instanceVertices[particleOffset + 2] + instanceVertices[particleOffset + 6] * startSpeed * lifetime | ||
| ); | ||
|
|
||
| let worldRotation: Quaternion; | ||
| if (simSpaceLocal) { | ||
| worldRotation = transform.worldRotationQuaternion; | ||
| } else { | ||
| const tempQ = ParticleGenerator._tempQuat0; | ||
| tempQ.set( | ||
| instanceVertices[particleOffset + 30], | ||
| instanceVertices[particleOffset + 31], | ||
| instanceVertices[particleOffset + 32], | ||
| instanceVertices[particleOffset + 33] | ||
| ); | ||
| worldRotation = tempQ; | ||
| } | ||
| Vector3.transformByQuat(local, worldRotation, local); | ||
|
|
||
| if (simSpaceLocal) { | ||
| local.add(transform.worldPosition); | ||
| } else { | ||
| local.x += instanceVertices[particleOffset + 27]; | ||
| local.y += instanceVertices[particleOffset + 28]; | ||
| local.z += instanceVertices[particleOffset + 29]; | ||
| } | ||
|
|
||
| // Gravity contribution: 0.5 · gravity · gravityMod · lifetime² (world-space) | ||
| const gravity = this._renderer.scene.physics.gravity; | ||
| const halfTSquaredR = 0.5 * lifetime * lifetime * gravityMod; | ||
| local.x += gravity.x * halfTSquaredR; | ||
| local.y += gravity.y * halfTSquaredR; | ||
| local.z += gravity.z * halfTSquaredR; |
There was a problem hiding this comment.
Stale gravityMod for curve-mode gravityModifier.
_addNewParticle only writes instanceVertices[offset + 19] in the Constant / TwoConstants branches of the switch at lines 971-978. For Curve / TwoCurves gravity modes the slot is left at whatever value the previous occupant of this circular-buffer index wrote, so gravityMod here will pick up garbage and the death-event world position will drift unpredictably. Either fall back to evaluating main.gravityModifier against playTime - particleAge here, or initialize the slot to 0/curve-evaluated value in _addNewParticle for completeness.
The doc comment at lines 1267-1273 currently lists VOL/FOL/Noise as caveats — please also call out curve-mode gravity (or fix it) so users aren't surprised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/particle/ParticleGenerator.ts` around lines 1280 - 1320,
The death-event world position can read a stale gravity modifier because
instanceVertices[particleOffset + 19] is only set for Constant/TwoConstants in
_addNewParticle, so either ensure the slot is initialized there for
Curve/TwoCurves (set instanceVertices[offset + 19] to 0 or the evaluated curve
value) or, in the death-position calculation inside _computeDeathWorldPos (where
gravityMod is read), fall back to evaluating main.gravityModifier at (playTime -
particleAge) when the stored value is invalid; update the doc comment near the
death-event handling to mention curve-mode gravity as a caveat if you choose
initialization rather than runtime-evaluation.
| function updateEngine(engine: Engine, frames: number, deltaTime = 100) { | ||
| //@ts-ignore | ||
| engine._vSyncCount = Infinity; | ||
| //@ts-ignore | ||
| engine._time._lastSystemTime = 0; | ||
| let times = 0; | ||
| performance.now = function () { | ||
| times++; | ||
| return times * deltaTime; | ||
| }; | ||
| for (let i = 0; i < frames; i++) { | ||
| engine.update(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any tests that read performance.now and might be affected by the global override.
rg -nP -C2 'performance\.now' --type=ts -g '!**/dist/**' -g '!**/build/**'Repository: galacean/engine
Length of output: 10866
Restore performance.now after test execution or use test framework mocking.
The updateEngine function overwrites the global performance.now without restoration. If particle tests run before other time-sensitive tests (e.g., shader compilation benchmarks, engine timing), those tests will receive stale deterministic values instead of real timestamps, silently breaking their behavior. Capture the original and restore it in cleanup, or use vi.spyOn(performance, 'now', 'get') or vi.useFakeTimers() which automatically cleans up between tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/particle/SubEmitter.test.ts` around lines 16 - 29, The
updateEngine helper overwrites global performance.now and never restores it;
capture the original (e.g., const origNow = performance.now) before replacing
and restore it after running the frames (or switch to vi.spyOn/performance.get
or vi.useFakeTimers which auto-cleans between tests). Update the updateEngine
function to save performance.now, replace it for deterministic timing, run the
engine.update loop, then restore the saved original so other tests aren’t
affected; reference updateEngine and its use of performance.now when making the
change.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…bursts Previously each slot read its target renderer's t=0 bursts to derive per-event count. That path had two problems: - If the target renderer also plays on its own (playOnEnabled, manual play), its own EmissionModule fires those bursts independently — the sub-emit path re-fires them, double-counting. - The same `_probabilityRand` was consumed for both the probability roll and the burst-count lerp factor, so adding/removing a burst shifted the entire downstream RNG stream (seed-unstable). Replace with an explicit `emitCount: number = 1` per slot. The target renderer's EmissionModule is left fully alone (bursts / rate / playOnEnabled co-exist with sub-emit driving with no overlap). `_probabilityRand` now serves only the probability roll.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/src/core/particle/SubEmitter.test.ts (1)
16-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
performance.nowafter the helper completes.
updateEnginepermanently overwrites the globalperformance.now. Any later test in the same Vitest run that depends on real wall-clock time will silently see the deterministic stub, and once this test file finishes the global stays in its last state. Capture and restore the original within the helper (or usevi.spyOn(globalThis, 'performance', 'get')/vi.useFakeTimers()), and pair the override withafterEachcleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SubEmitter.test.ts` around lines 16 - 29, The helper updateEngine in SubEmitter.test.ts replaces global performance.now permanently; capture the original (e.g. const originalNow = performance.now) before overwriting and restore it after the helper runs (or switch to vi.spyOn(globalThis, 'performance', 'get')/vi.useFakeTimers()), and ensure tests include an afterEach cleanup that restores performance.now so later tests see the real clock; update the updateEngine function and add an afterEach restore to fix this in the test file.
🧹 Nitpick comments (5)
packages/core/src/particle/modules/SubEmittersModule.ts (2)
121-122: 💤 Low value
| 0is a sharp truncation; preferMath.truncorMath.floor.Using
sub.emitCount | 0coerces via a 32-bit signed conversion: anyemitCount≥ 2³¹ wraps to a negative value and is silently dropped by thecount <= 0guard, and any fractional value above2³¹ − 1similarly explodes. For a user-facing field,Math.trunc(sub.emitCount)(orMath.max(0, Math.floor(sub.emitCount))) gives more predictable semantics with effectively the same cost. Not a release blocker, but worth tightening sinceemitCountis a public field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 121 - 122, Replace the 32-bit bitwise coercion on sub.emitCount with a safer numeric truncation and non-negative guard: instead of using "sub.emitCount | 0" compute a truncated integer (e.g. Math.trunc(sub.emitCount)) and ensure it's clamped to zero or above (e.g. Math.max(0, Math.trunc(sub.emitCount))). Update the count assignment in SubEmittersModule.ts where "const count = sub.emitCount | 0" is set so fractional values and very large numbers are handled predictably and won't wrap into negatives.
30-32: 💤 Low valueRedundant constructor.
The explicit constructor only forwards to
super(generator)without adding behavior; it can be removed and the base-class constructor will be used directly. Pure nit.♻️ Proposed cleanup
- constructor(generator: ParticleGenerator) { - super(generator); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 30 - 32, Remove the redundant constructor in the SubEmittersModule class: the explicit constructor that only calls super(generator) is unnecessary because the base class constructor will be used automatically; delete the constructor definition in SubEmittersModule so the class relies on the inherited constructor (referencing SubEmittersModule, constructor, ParticleGenerator, and super).tests/src/core/particle/SubEmitter.test.ts (3)
50-61: ⚡ Quick winMissing
afterAllto destroy the engine.The shared
engineis created inbeforeAllbut never disposed. WithWebGLEngine, this leaks the canvas/GL context across the test runner's lifetime and, when combined with other particle suites that also create engines, can exhaust browser/jsdom resources or cross-contaminate global state (audio context, render frame loops, etc.). Add anafterAll(() => engine.destroy()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SubEmitter.test.ts` around lines 50 - 61, The test creates a shared WebGLEngine in beforeAll but never disposes it, leaking GL/canvas resources; add an afterAll hook that calls engine.destroy() to clean up. Specifically, in the SubEmitter test suite where beforeAll creates the engine via WebGLEngine.create and assigns it to the engine variable, add afterAll(() => engine.destroy()) (or await engine.destroy() if destroy is async) to ensure the created engine, canvas and GL context are torn down after tests.
207-211: 💤 Low valueMagic offsets into
_instanceVerticescouple the test to the GPU buffer layout.Indexing
buf[8],buf[9],buf[10]for the color components is brittle — any reshuffle of the particle vertex stride/attribute order silently breaks this test (and the failure mode is "wrong number" rather than "wrong attribute"). Consider naming the offsets via a small local constant tied to a documented stride, or asserting via a higher-level read path (e.g., a debug accessor onParticleGenerator) so the assertion describes intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SubEmitter.test.ts` around lines 207 - 211, The test is using magic indices into child.generator._instanceVertices (buf[8], buf[9], buf[10]) which couples the test to the GPU buffer layout; add a stable, self-documenting way to read color components instead: either introduce local constants in the test (e.g., PARTICLE_VERTEX_STRIDE and COLOR_OFFSET) and compute base = particleIndex * PARTICLE_VERTEX_STRIDE + COLOR_OFFSET before asserting buf[base + 0/1/2], or (preferred) add a debug/read accessor on ParticleGenerator (e.g., ParticleGenerator.getInstanceColor(instanceIndex) or getInstanceAttribute(instanceIndex, "color")) that returns the RGBA components and use that in the test (reference symbols: _instanceVertices, ParticleGenerator, child.generator). Ensure the test uses the new constants/accessor so it asserts intent rather than raw indices.
53-61: ⚡ Quick winClearer engine initialization: avoid relying on vSync settings to suppress the rAF loop.
engine.run()starts a rAF loop that callsengine.update()each tick. TheupdateEngine()function mitigates double-ticking by setting_vSyncCount = Infinity(line 18), which prevents the rAF-driven updates. However, this implicit workaround is fragile and makes the test harder to understand.For clarity, either omit
engine.run()and drive everything via manualengine.update(), or explicitly callengine.pause()before enteringupdateEngine()to halt the rAF loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SubEmitter.test.ts` around lines 53 - 61, The test currently calls engine.run() which starts a rAF loop and relies on setting _vSyncCount = Infinity (in updateEngine) to prevent double updates; instead, stop the rAF loop explicitly by either removing the engine.run() call and driving frames only via updateEngine(), or call engine.pause() immediately after engine.run() before entering updateEngine(); update the test setup to use WebGLEngine.create(...) and then either omit engine.run() or call engine.pause() so updateEngine() no longer depends on mutating _vSyncCount to suppress rAF updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/particle/modules/SubEmittersModule.ts`:
- Around line 103-114: Move the self-reference and early-exit guards so they run
before consuming the probability RNG: in SubEmittersModule, check target (const
target = sub.emitter), ensure target !== null && !target.destroyed, then check
target.generator !== this._generator (the self-reference guard) and the
emitCount <= 0 guard (where emitCount is used) before calling
this._probabilityRand.random() or evaluating sub.emitProbability; this prevents
wasting calls to this._probabilityRand.random() for slots that are going to be
skipped.
- Around line 106-108: The current check in SubEmittersModule uses
this._probabilityRand.random() > sub.emitProbability which can allow a rare
firing when emitProbability is 0 because random() can return 0; update the logic
to explicitly handle non-positive probabilities before calling the RNG—e.g.,
return immediately if sub.emitProbability <= 0, otherwise keep the existing
probabilistic branch (or alternatively change the comparison to >= to treat 0 as
never emit). Ensure you reference SubEmittersModule and the
_probabilityRand.random() call and avoid calling random() when emitProbability
is non-positive.
- Around line 111-114: The current guard in SubEmittersModule only blocks direct
self-reference (targetGen === this._generator) but not indirect cycles; fix by
tracking visited generators per emission chain: add a visited Set (e.g.,
Set<ParticleGenerator> or WeakSet) passed into the sub-emitter dispatch path
(methods in SubEmittersModule that perform emission, the block containing
targetGen and _suppressSubEmitterDispatch) and before recursing check if
visited.has(targetGen) — if so, bail early; otherwise add targetGen to visited
before invoking its emission and remove after, or use a depth counter with a
maxDepth fallback; ensure you reference and update _suppressSubEmitterDispatch
behavior consistently so Birth/Death suppression still applies.
---
Duplicate comments:
In `@tests/src/core/particle/SubEmitter.test.ts`:
- Around line 16-29: The helper updateEngine in SubEmitter.test.ts replaces
global performance.now permanently; capture the original (e.g. const originalNow
= performance.now) before overwriting and restore it after the helper runs (or
switch to vi.spyOn(globalThis, 'performance', 'get')/vi.useFakeTimers()), and
ensure tests include an afterEach cleanup that restores performance.now so later
tests see the real clock; update the updateEngine function and add an afterEach
restore to fix this in the test file.
---
Nitpick comments:
In `@packages/core/src/particle/modules/SubEmittersModule.ts`:
- Around line 121-122: Replace the 32-bit bitwise coercion on sub.emitCount with
a safer numeric truncation and non-negative guard: instead of using
"sub.emitCount | 0" compute a truncated integer (e.g. Math.trunc(sub.emitCount))
and ensure it's clamped to zero or above (e.g. Math.max(0,
Math.trunc(sub.emitCount))). Update the count assignment in SubEmittersModule.ts
where "const count = sub.emitCount | 0" is set so fractional values and very
large numbers are handled predictably and won't wrap into negatives.
- Around line 30-32: Remove the redundant constructor in the SubEmittersModule
class: the explicit constructor that only calls super(generator) is unnecessary
because the base class constructor will be used automatically; delete the
constructor definition in SubEmittersModule so the class relies on the inherited
constructor (referencing SubEmittersModule, constructor, ParticleGenerator, and
super).
In `@tests/src/core/particle/SubEmitter.test.ts`:
- Around line 50-61: The test creates a shared WebGLEngine in beforeAll but
never disposes it, leaking GL/canvas resources; add an afterAll hook that calls
engine.destroy() to clean up. Specifically, in the SubEmitter test suite where
beforeAll creates the engine via WebGLEngine.create and assigns it to the engine
variable, add afterAll(() => engine.destroy()) (or await engine.destroy() if
destroy is async) to ensure the created engine, canvas and GL context are torn
down after tests.
- Around line 207-211: The test is using magic indices into
child.generator._instanceVertices (buf[8], buf[9], buf[10]) which couples the
test to the GPU buffer layout; add a stable, self-documenting way to read color
components instead: either introduce local constants in the test (e.g.,
PARTICLE_VERTEX_STRIDE and COLOR_OFFSET) and compute base = particleIndex *
PARTICLE_VERTEX_STRIDE + COLOR_OFFSET before asserting buf[base + 0/1/2], or
(preferred) add a debug/read accessor on ParticleGenerator (e.g.,
ParticleGenerator.getInstanceColor(instanceIndex) or
getInstanceAttribute(instanceIndex, "color")) that returns the RGBA components
and use that in the test (reference symbols: _instanceVertices,
ParticleGenerator, child.generator). Ensure the test uses the new
constants/accessor so it asserts intent rather than raw indices.
- Around line 53-61: The test currently calls engine.run() which starts a rAF
loop and relies on setting _vSyncCount = Infinity (in updateEngine) to prevent
double updates; instead, stop the rAF loop explicitly by either removing the
engine.run() call and driving frames only via updateEngine(), or call
engine.pause() immediately after engine.run() before entering updateEngine();
update the test setup to use WebGLEngine.create(...) and then either omit
engine.run() or call engine.pause() so updateEngine() no longer depends on
mutating _vSyncCount to suppress rAF updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 79635f31-7e0d-4a9b-ac6b-aa7618fed8e7
📒 Files selected for processing (3)
packages/core/src/particle/modules/SubEmitter.tspackages/core/src/particle/modules/SubEmittersModule.tstests/src/core/particle/SubEmitter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/particle/modules/SubEmitter.ts
| if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the return range of Rand.random() in the engine-math package.
ast-grep --pattern $'class Rand {
$$$
random() {
$$$
}
$$$
}'
rg -nP -C3 '\brandom\s*\(\s*\)\s*[:{]' --type=ts -g '*Rand*'Repository: galacean/engine
Length of output: 422
emitProbability = 0 can allow unexpected firings.
The implementation of Rand.random() returns randomInt32() / 0xffffffff, producing a range of [0, 1.0] (including both boundaries). When emitProbability = 0, the condition random() > 0 evaluates to false if the RNG returns exactly 0, allowing the slot to fire despite the "never emit" configuration. While the probability is extremely low (~1 in 2³²), the contract is broken at the boundary.
Fix by adding an explicit check for non-positive probabilities or changing the comparison operator:
🛡️ Suggested fix
+ if (sub.emitProbability <= 0) return;
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) { | |
| return; | |
| } | |
| if (sub.emitProbability <= 0) return; | |
| if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 106 -
108, The current check in SubEmittersModule uses this._probabilityRand.random()
> sub.emitProbability which can allow a rare firing when emitProbability is 0
because random() can return 0; update the logic to explicitly handle
non-positive probabilities before calling the RNG—e.g., return immediately if
sub.emitProbability <= 0, otherwise keep the existing probabilistic branch (or
alternatively change the comparison to >= to treat 0 as never emit). Ensure you
reference SubEmittersModule and the _probabilityRand.random() call and avoid
calling random() when emitProbability is non-positive.
| if (targetGen === this._generator) { | ||
| // Self-reference would recurse infinitely on Birth; bail. | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for recursion/cycle protection in the sub-emit dispatch path.
fd -t f 'ParticleGenerator.ts' packages/core | xargs rg -n -C3 '_emitFromSubEmitter|_onParticleBirth|_subEmitDepth|_inSubEmit|scratch'
ast-grep --pattern $'_emitFromSubEmitter($$$) {
$$$
}'Repository: galacean/engine
Length of output: 1256
🏁 Script executed:
# Get full implementation of _emitFromSubEmitter method
fd -t f 'ParticleGenerator.ts' packages/core -x cat {} | grep -A 50 '_emitFromSubEmitter' | head -80Repository: galacean/engine
Length of output: 2567
🏁 Script executed:
# Search for depth limit or visited tracking patterns in sub-emitter logic
rg -n 'depth|visited|stack|recursion|cycle' packages/core/src/particle/ -i -C 2Repository: galacean/engine
Length of output: 9502
🏁 Script executed:
# Look at SubEmittersModule._onParticleBirth implementation
fd -t f 'SubEmittersModule.ts' packages/core -x cat {} | grep -A 30 '_onParticleBirth'Repository: galacean/engine
Length of output: 913
🏁 Script executed:
# Get complete _emitFromSubEmitter method
fd -t f 'ParticleGenerator.ts' packages/core -x cat -n {} | awk '/^[[:space:]]*1133.*_emitFromSubEmitter/,/^[[:space:]]*[0-9]+.*^[[:space:]]*(private|public|protected|_|\/\/)/ {print}' | head -100Repository: galacean/engine
Length of output: 4188
🏁 Script executed:
# Search for _depth or recursion tracking variables
rg -n '_depth|_recursion|_level|_inRecursion|_emitDepth' packages/core/src/particle/Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Look at _fireSlot implementation in SubEmittersModule
fd -t f 'SubEmittersModule.ts' packages/core -x cat {} | grep -A 20 '_fireSlot'Repository: galacean/engine
Length of output: 1484
🏁 Script executed:
# Search for where _suppressSubEmitterDispatch is checked during particle creation/birth
rg -n '_suppressSubEmitterDispatch' packages/core/src/particle/ -B 3 -A 3Repository: galacean/engine
Length of output: 3545
🏁 Script executed:
# Look at _addNewParticle to see if it checks _suppressSubEmitterDispatch before dispatching Birth
fd -t f 'ParticleGenerator.ts' packages/core -x cat -n {} | awk '/^[[:space:]]*1094/,/^[[:space:]]*1126/ {print}'Repository: galacean/engine
Length of output: 1744
Indirect sub-emitter cycles are not prevented; only direct self-reference is guarded.
The check targetGen === this._generator at line 111 blocks immediate self-reference (A → A), but allows indirect cycles like A → B → A. The _suppressSubEmitterDispatch flag only suppresses Birth/Death dispatch during a specific generator's sub-emitter emission and does not track visited generators across the call chain. While the finite particle pool will eventually exhaust and stop recursion, a user-authored cycle will recurse until particles run out, potentially causing stack overflow or significant performance degradation. Consider tracking recursion depth or maintaining a visited-set per frame to explicitly break cycles earlier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 111 -
114, The current guard in SubEmittersModule only blocks direct self-reference
(targetGen === this._generator) but not indirect cycles; fix by tracking visited
generators per emission chain: add a visited Set (e.g., Set<ParticleGenerator>
or WeakSet) passed into the sub-emitter dispatch path (methods in
SubEmittersModule that perform emission, the block containing targetGen and
_suppressSubEmitterDispatch) and before recursing check if
visited.has(targetGen) — if so, bail early; otherwise add targetGen to visited
before invoking its emission and remove after, or use a depth counter with a
maxDepth fallback; ensure you reference and update _suppressSubEmitterDispatch
behavior consistently so Birth/Death suppression still applies.
…bability roll target null/destroyed / self-reference / emitCount<=0 are static slot filters that previously sat after `_probabilityRand.random()`. With useAutoRandomSeed off, adding or toggling a no-op slot would consume one rand and shift every downstream probability check, breaking deterministic playback. Move all non-RNG filters above the roll so only slots that actually participate in the event consume rand.
…ributes Replace the test's direct `_instanceVertices[8..10]` poke (which required `@ts-ignore` and hardcoded the buffer's float-offset layout) with three @internal accessors on ParticleGenerator: `_readParticleStartColor`, `_readParticleStartSize`, `_readParticleStartRotation`. Same conventional pattern as `_getAliveParticleCount` — underscore-prefixed, @internal-tagged. Color inherit test updated; Size / Rotation inherit tests can use the matching accessors when added.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@GuoLei1990 P0 这个 关键: A→B→A 走一遍:
调用栈深度 = 1。A↔B↔C 同理,每层 你说"B 的 flag 为 false"那段——在 A fire B 期间 B 的 flag 是 true,是动态栈标志不是静态属性。 跨帧 B 自播再 fire A 是独立路径不是递归调用,每帧一次没有指数增长。 Unity 全局 Set 是因为它"重新实例化子系统"会同栈帧再跑 EmissionModule;我们的 |
Engine bitmask enums (Layer / PointerButton / CameraClearFlags / ColorWriteMask / SpriteMaskLayer / ActiveChangeFlag) all use 0x1, 0x2, 0x4 ... convention. Align ParticleSubEmitterProperty to the same style — 1 << n was an outlier.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… add Rotation Remove the CPU-side OverLifetime modulation mirror used by sub-emitter inherit and instead pass the parent particle's raw start values to the sub system. ColorOverLifetime/SizeOverLifetime evaluation now lives only in the GPU shader, keeping the engine's single-source-of-truth for lifetime modulation. Add Rotation back to the inherit bitmask alongside Color and Size; parent's start rotation is added (not multiplied) onto the sub particle's start rotation.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
提议：继承父粒子在事件时刻的"当前调制色/尺寸/旋转"，而非 raw start values简述当前 PR 选择继承父粒子的 raw 想提议一条更符合用户视觉直觉的语义：继承父粒子在事件那一刻的当前调制值（startColor × COL(currentAge)、startSize × SOL(currentAge)、startRotation 含 ROL 累积）。 典型差异场景:父粒子 lifetime 内 COL 从白渐变到深红，死亡时触发 sub-emit。当前行为：子粒子拿到的 inheritColor = 白（父的 startColor）。提议行为：子粒子拿到的 inheritColor = 深红（父在死亡瞬间的视觉色）。用户在编辑器里看到「红火球死了喷出来的火星却是白色」会感到困惑，特别是当 COL 渐变明显时——继承字段的意图是"子嗣继承父的样子"，把"样子"理解成「死亡瞬间的样子」比「出生瞬间冻结的样子」更符合直觉。 Birth 事件下两种语义几乎等价（normalizedAge ≈ 0 时 COL evaluate ≈ startColor），差异主要在 Death 事件上显现。 Galacean 现有数据已经能支持这件事所有需要的输入都已经持久化在 instance buffer，CPU 端能直接读：
normalizedAge 用 改动范围（约 100 行）1. 补
|
| 场景 | CPU evaluate 次数 | 单帧耗时 |
|---|---|---|
| 没启用 SubEmitter | 0 | 0 |
| 普通持续雨：1000 粒子、1 秒 lifetime、60fps | ~17 次/帧 | < 5 µs |
| 极端：10000 粒子、0.5 秒 lifetime | ~333 次/帧 | < 100 µs |
| 同步 burst 1000 颗同帧死亡 | 1000 次集中一帧 | < 300 µs，下一帧归零 |
跟同帧 GPU 在做的事相比（GPU 那边每帧 N instance × 每 instance 4 vertex 跑同样的 evaluate，量级是 CPU 的 100×+），CPU 这一刀属于纯增量、不显眼。
关于「CPU 和 GPU 各 evaluate 一次重不重复」：会，但消除不掉。让 GPU 把 currentColor 写回 buffer 再回读，WebGL 同步 getBufferSubData 阻塞主线程（毫秒级），异步 fence 又会引入 1-2 帧延迟、拿到的是旧色；让 CPU 每帧算所有活粒子的当前色又会撤掉 Galacean procedural 路径「每帧 CPU 不动」的核心优势。两份 evaluate 服务不同消费者（GPU 那份画屏幕、CPU 那份传给子系统作为 inheritColor），这是「GPU 主导模拟 + CPU 事件派发」架构下的天然代价。
bounds（独立问题，建议 follow-up）
sub-emit 注入的子粒子出生在父粒子事件位置，而非子系统自己的 emission shape 内——这会让子系统 Local 空间下 _calculateGeneratorBounds 的解析推导 bounds 偏小（不覆盖 sub-emit 注入的粒子），可能导致 frustum culling 误杀。
跟本次 evaluate 改动正交，建议本 PR 范围内先在注释里加一条 caveat：「sub-emit 注入的子粒子可能位于子系统 generatorBounds 之外，建议在子系统上将 culling mode 设为 AlwaysSimulate 或手动 bounds override」。完整修复（子系统反向感知父系统、bounds 叠加父贡献）可走 follow-up PR，约 100 行。
二选一
A. 实现上面 1-4 步（推荐）
视觉行为：子粒子继承的是父粒子在事件那一刻的实际显示色/尺寸/旋转。
B. 保留当前 raw start values 路线
如果不打算补 evaluate，建议把 3 个新单测的命名和注释中带歧义的措辞调整一下，并在 PR description / 用户文档里把"继承的是 start values（不经 OverLifetime 调制）"这条作为有意的设计决定明确写出来——避免用户在 Death 场景下踩到「子粒子色和父粒子死亡视觉色不一致」的坑时不知道为什么。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…values
Children spawned via Birth/Death sub-emit events now inherit the parent's
currently-visible Color/Size/Rotation at the event moment, instead of the
raw start values stored in the instance buffer. Matches user intuition
("the parent was deep-red when it died, the sparks should be deep-red too")
and Unity's sub-emitter inheritance semantics.
Implementation:
- ParticleGradient._evaluate: CPU mirror of shader's evaluateParticleGradient
(independent color/alpha max-time clamps, linear key interpolation).
- ParticleCompositeGradient.evaluate: fill Gradient / TwoGradients branches
(previously silent default-break — a real bug independent of sub-emitter).
- ParticleCurve._evaluateCumulative + ParticleCompositeCurve._evaluateCumulative:
CPU mirror of shader's evaluateParticleCurveCumulative (trapezoidal
integration). Used by ROL accumulation.
- ParticleGenerator._modulateInheritByLifetime: applies COL multiplicatively,
SOL multiplicatively (Curve/TwoCurves only, matching shader gating), and
ROL additively (cumulative × lifetime). Random factors are read from the
same instance-buffer slots the shader samples (a_Random0.y/z/w).
- _dispatchDeathEvent / _addNewParticle Birth dispatch call the helper at
normalizedAge ≈ 1 / 0 respectively.
Birth-event note: at normalizedAge=0 modulation is usually identity, but if
the user explicitly sets non-identity COL gradient[0] (or SOL curve[0]) the
Birth event will pick that up — by design, for symmetry with Death.
Tests:
- 'Color inherit uses parent's raw startColor' / 'Size inherit uses parent's
raw startSize' tests rewritten to assert the modulated value (was locking
in the old semantics).
…D modes The previous commit unconditionally added the ROL cumulative to parentRotation.z, but in 2D start-rotation mode (main.startRotation3D=false, the default) the shader stores and reads the Z angle in a_StartRotation0.x — not .z. Mirror the same dispatch in CPU: - separateAxes ROL → each axis gets its own cumulative (3D implied) - 3D start rotation + single-axis ROL → cumulative goes to .z - 2D start rotation (default) + single-axis ROL → cumulative goes to .x Add a Death + ROL-enabled test that locks this in (parent ROL rate 2/s, lifetime 0.5s → cumulative 1.0; child startRotZ 0.25 → inherits 1.25).
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-14 第二轮）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭 |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复 |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复 |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
| Birth 位置 World 空间双重变换 | ✅ 已关闭（误报，position 始终为 shape 局部坐标） |
| ROL cumulative 写入错误 slot（2D 模式应写 .x 而非 .z） | ✅ 已修复（commit 54bd132） |
| COL/SOL/ROL 继承仅传原始 start 值，未反映 OverLifetime 调制 | ✅ 已修复（commit 6175a85 新增 _modulateInheritByLifetime） |
以下两个问题经当前 HEAD（54bd132）确认仍未修复，请处理后合并。
[P1] SubEmittersModule.ts — emitProbability = 0 仍可意外触发
// 当前代码（未变）
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}Rand.random() 实现为 randomInt32() / 0xffffffff，返回闭区间 [0, 1]（含两端点）。当 emitProbability = 0 且 randomInt32() 恰好返回 0（概率 1/2³² ≈ 2.3×10⁻¹⁰）时，random() 返回 0.0，0.0 > 0.0 为 false，子发射器意外触发。
emitProbability = 0 语义上应表示"从不触发"，不应依赖 RNG 实现细节。修复一行：
if (sub.emitProbability <= 0) return; // 语义明确：0 = 从不触发
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;[P2] ParticleGenerator.ts — gravityMod 在 Curve/TwoCurves 模式下读取未初始化 slot
_dispatchDeathEvent 无条件读取 instanceVertices[particleOffset + 19] 作为 gravityMod，用于计算死亡位置的重力项 ½·g·gravityMod·lifetime²。但 _initParticleVertices 的 switch 只在 Constant 和 TwoConstants 分支写入 slot 19，Curve 和 TwoCurves 分支没有 case，slot 19 保持为 buffer 初始值（0 或环形缓冲区中前一粒子的残留值）。
Curve gravity 模式下，Death sub-emitter 的生成位置会引入非确定性误差。建议补 default 分支：
default:
// Curve/TwoCurves: t=0 sample as approximation; Death position deviates when gravity varies over lifetime
instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, 0);
break;若不希望改变行为，至少在 _dispatchDeathEvent 顶部注释中补充说明此限制（与 VOL/FOL/Noise 并列）。
新增的 _modulateInheritByLifetime 逻辑经 shader 对照验证：COL/SOL/ROL 三条路径的随机因子读取均与 GPU 端 a_Random0.y/z/w 的实际使用一致，设计正确。ROL 2D/3D 模式分支（commit 54bd132）的修复亦属正确。
`_evaluateCumulative` on ParticleCurve / ParticleCompositeCurve is only called by the sub-emitter inherit modulation in ParticleGenerator. Moving the math next to its sole caller as two private statics (_curveCumulative + _curveKeysIntegral) keeps the curve classes free of methods that no other code reads.
|
@GuoLei1990 分析准确,边界确实存在。但我倾向这次不改: 触发概率: 修复的副作用:
API 语义: emitProbability=0 表达"永不触发"本身就是边缘用法——想禁用 slot 应该走 |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-14 第三轮，HEAD d380d71b）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭 |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复 |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复 |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
| Birth 位置 World 空间双重变换 | ✅ 已关闭（误报，position 始终为 shape 局部坐标） |
| ROL cumulative 写入错误 slot（2D 模式应写 .x 而非 .z） | ✅ 已修复（54bd132） |
| COL/SOL/ROL 继承仅传原始 start 值，未反映 OverLifetime 调制 | ✅ 已修复（6175a85 新增 _modulateInheritByLifetime） |
_evaluateCumulative 内联到 ParticleGenerator |
✅ 已重构（d380d71b 移入 _curveCumulative / _curveKeysIntegral，逻辑一致） |
以下两个问题经 HEAD d380d71b 确认仍未修复，请处理后合并。
[P1] SubEmittersModule.ts — emitProbability = 0 仍可意外触发
// 当前代码（第 115 行，未变）
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}Rand.random() 实现为 randomInt32() / 0xffffffff，返回闭区间 [0, 1]（含两端点）。当 emitProbability = 0 且 randomInt32() 恰好返回 0（概率 1/2³² ≈ 2.3×10⁻¹⁰）时，random() 返回 0.0，0.0 > 0.0 为 false，条件不成立，子发射器意外触发。
emitProbability = 0 语义上应表示"从不触发"，不应依赖 RNG 实现细节。修复一行：
if (sub.emitProbability <= 0) return; // 语义明确：0 = 从不触发
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;[P2] ParticleGenerator.ts — gravityMod 在 Curve/TwoCurves 模式下读取未初始化 slot
_initParticleVertices 的 switch 只在 Constant 和 TwoConstants 分支写入 slot 19，Curve 和 TwoCurves 分支没有 case，slot 19 保持为 buffer 初始值（0 或环形缓冲区中前一粒子的残留值）：
switch (main.gravityModifier.mode) {
case ParticleCurveMode.Constant:
instanceVertices[offset + 19] = main.gravityModifier.constant;
break;
case ParticleCurveMode.TwoConstants:
instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random());
break;
// Curve / TwoCurves: 无写入，slot 19 保留脏数据
}_dispatchDeathEvent 第 1308 行无条件读取 slot 19 计算死亡位置重力项 ½·g·gravityMod·lifetime²，Curve 模式下读到的是非确定性脏数据，导致 Death sub-emitter 的生成位置随机漂移。
建议补 default 分支：
default:
// Curve/TwoCurves: t=0 sample as approximation; Death position deviates when gravity varies over lifetime
instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, 0);
break;若不希望改变行为，至少在 _dispatchDeathEvent 顶部注释中补充此限制（与 VOL/FOL/Noise 并列说明）。
…rent Enable parent ColorOverLifetime and SizeOverLifetime so the e2e case verifies that sub-emitters inherit the parent's visible (modulated) values at Death, not the raw start values.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-14 第四轮，HEAD ebc3169）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭 |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复 |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复 |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
| Birth 位置 World 空间双重变换 | ✅ 已关闭（误报） |
| ROL cumulative 写入错误 slot（2D 模式应写 .x 而非 .z） | ✅ 已修复 |
| COL/SOL/ROL 继承仅传原始 start 值，未反映 OverLifetime 调制 | ✅ 已修复（_modulateInheritByLifetime） |
_evaluateCumulative 内联到 ParticleGenerator |
✅ 已重构 |
以下三个问题仍未解决。
[P1] SubEmittersModule.ts:115 — emitProbability = 0 仍可意外触发
作者回复把修复描述为 > → >=，但历次 review 提出的修复方案并非如此。我建议的是在现有逻辑之前增加一行独立的早退检查：
// 语义明确：0 = 永不触发，不进入 RNG 路径
if (sub.emitProbability <= 0) return;
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;这一改动：
- 不改变
>的 RNG 语义，现有的> emitProbability逻辑完全不动 - 不影响 任何
emitProbability > 0的正常路径 - 精确匹配了
emitProbability = 0的语义：用户设置 0 意味着"从不触发"，这应该在不消耗 RNG 状态的前提下成立
作者的反驳（"用 0 来表示禁用是用户绕了远路"）不成立。emitProbability 在 Unity 中是公开的可动画化属性，合法取值范围是 [0, 1]，0 表示"从不触发"是自然语义。用户完全可能通过动画曲线或运行时脚本将其降到 0 来动态关闭 slot，这不是"绕路"——这是预期用法。
两行中的第一行：单测无法直接验证该行为（RNG 极小概率），但可以加一个 emitProbability = 0 的 确定性单测：100 次调用，验证 0 次触发。
[P2] ParticleGenerator.ts:1012 — gravityMod 在 Curve/TwoCurves 模式下读取未初始化 slot
当前代码（经 HEAD ebc3169 确认，未变）：
switch (main.gravityModifier.mode) {
case ParticleCurveMode.Constant:
instanceVertices[offset + 19] = main.gravityModifier.constant;
break;
case ParticleCurveMode.TwoConstants:
instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random());
break;
// Curve / TwoCurves：无写入，slot 19 保持为 0 或环形缓冲区中前一粒子的残留值
}_dispatchDeathEvent:1308 无条件读取 instanceVertices[particleOffset + 19] 作为 gravityMod，用于计算死亡位置的重力项 ½·g·gravityMod·lifetime²。Curve/TwoCurves 模式下该 slot 是不确定的脏数据，Death sub-emitter 的生成位置会随机偏移。
最小修复——补 default 分支，t=0 采样作为近似值：
default:
// Curve/TwoCurves: t=0 sample as approximation; Death position deviates when gravity varies over lifetime
instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, 0);
break;若不改行为，至少在 _dispatchDeathEvent 顶部注释中补充说明（与 VOL/FOL/Noise 并列）。
[P3] PR 描述与实现不符 — COL/SOL/ROL 继承行为
PR 描述 Behavior 表格中写：
ColorOverLifetime / SizeOverLifetime / RotationOverLifetime are NOT applied to the inherited values
但 commit 6175a85 添加的 _modulateInheritByLifetime 方法已经将 COL/SOL/ROL 应用于继承值——Birth 事件使用 normalizedAge = 0 采样，Death 事件使用粒子实际 normalizedAge。
实现行为与描述相反。请更新 PR 描述，将该行改为：
Birth inherit: values are taken from parent's start values (COL/SOL/ROL at normalizedAge = 0)
Death inherit: values reflect the parent's OverLifetime modulation at the moment of death (COL/SOL/ROL applied at actual normalizedAge)
另外，checklist 中"incl. parent's COL/SOL not applied to inherit"同样已失效，建议一并修正。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-14 第四轮，HEAD ebc3169）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭 |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复 |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复 |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
| Birth 位置 World 空间双重变换 | ✅ 已关闭（误报） |
| ROL cumulative 写入错误 slot（2D 模式应写 .x 而非 .z） | ✅ 已修复 |
| COL/SOL/ROL 继承仅传原始 start 值，未反映 OverLifetime 调制 | ✅ 已修复（_modulateInheritByLifetime） |
_evaluateCumulative 内联到 ParticleGenerator |
✅ 已重构 |
以下三个问题仍未解决。
[P1] SubEmittersModule.ts:115 — emitProbability = 0 仍可意外触发
作者回复把修复描述为 > → >=，但历次 review 提出的修复方案并非如此。建议的是在现有逻辑之前增加一行独立的早退检查：
// 语义明确：0 = 永不触发，不进入 RNG 路径
if (sub.emitProbability <= 0) return;
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;这一改动：
- 不改变
>的 RNG 语义，现有的> emitProbability逻辑完全不动 - 不影响 任何
emitProbability > 0的正常路径 - 精确匹配
emitProbability = 0的用户语义：动画曲线或运行时脚本完全可能将其降到0来动态关闭 slot，这是预期用法而非"绕路"
两行中的第一行可以写一个确定性单测验证：固定 emitProbability = 0，调用 100 次，验证触发次数为 0。
[P2] ParticleGenerator.ts:1012 — gravityMod 在 Curve/TwoCurves 模式下读取未初始化 slot
当前代码（经 HEAD ebc3169 确认，未变）：
switch (main.gravityModifier.mode) {
case ParticleCurveMode.Constant:
instanceVertices[offset + 19] = main.gravityModifier.constant;
break;
case ParticleCurveMode.TwoConstants:
instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random());
break;
// Curve / TwoCurves：无写入，slot 19 保持为 0 或环形缓冲区中前一粒子的残留值
}_dispatchDeathEvent:1308 无条件读取 instanceVertices[particleOffset + 19] 作为 gravityMod，用于死亡位置重力项计算。Curve/TwoCurves 模式下 slot 19 是不确定的脏数据，Death sub-emitter 生成位置会随机偏移。
最小修复：
default:
// Curve/TwoCurves: t=0 sample as approximation; Death position deviates when gravity varies over lifetime
instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, 0);
break;若不改行为，至少在 _dispatchDeathEvent 顶部注释中补充说明（与 VOL/FOL/Noise 并列）。
[P3] PR 描述与实现不符 — COL/SOL/ROL 继承行为
PR 描述 Behavior 表格中写：
ColorOverLifetime / SizeOverLifetime / RotationOverLifetime are NOT applied to the inherited values
但 _modulateInheritByLifetime 已经将 COL/SOL/ROL 应用于继承值——Birth 事件使用 normalizedAge = 0 采样，Death 事件使用粒子实际 normalizedAge。
实现行为与描述相反。建议将该行改为：
Birth inherit: parent start values with COL/SOL/ROL sampled at normalizedAge = 0
Death inherit: parent values with COL/SOL/ROL applied at actual normalizedAge at time of death
checklist 中"incl. parent's COL/SOL not applied to inherit"同样已失效，建议一并修正。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15 第五轮，HEAD 38a7fd3d）
新增变更
HEAD 38a7fd3d 相比上一轮审查（ebc3169）只有一处改动：将 e2e/config.ts 中 particleRenderer-sub-emitter 的 diffPercentage 从 0 放宽到 0.06。这是跨 GPU/驱动器浮点差异导致的基准截图微小偏差，调整合理，无需额外审查。
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭 |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复 |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复 |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
| Birth 位置 World 空间双重变换 | ✅ 已关闭（误报） |
| ROL cumulative 写入错误 slot（2D 模式应写 .x 而非 .z） | ✅ 已修复 |
| COL/SOL/ROL 继承仅传原始 start 值，未反映 OverLifetime 调制 | ✅ 已修复（_modulateInheritByLifetime） |
_evaluateCumulative 内联到 ParticleGenerator |
✅ 已重构 |
以下三个问题经 HEAD 38a7fd3d 确认仍未修复，请处理后合并。
[P1] SubEmittersModule.ts:115 — emitProbability = 0 仍可意外触发
// 当前代码（未变）
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}Rand.random() 实现为 randomInt32() / 0xffffffff，返回闭区间 [0, 1]（含两端点）。当 emitProbability = 0 且 randomInt32() 恰好返回 0（概率 1/2³² ≈ 2.3×10⁻¹⁰）时，random() 返回 0.0，0.0 > 0.0 为 false，子发射器意外触发。
现有测试（SubEmitter.test.ts:150）只运行了 20 次 Birth 事件，概率上几乎必然通过——但这是概率正确，不是确定性正确。emitProbability = 0 语义上应表示"从不触发"，不应依赖 RNG 细节。
最小修复（在现有 RNG 检查之前加一行早退）：
if (sub.emitProbability <= 0) return; // 0 = 永不触发，语义确定，无需消耗 RNG
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;[P2] ParticleGenerator.ts:1012 — gravityMod 在 Curve/TwoCurves 模式下读取未初始化 slot
// 当前代码（未变）
switch (main.gravityModifier.mode) {
case ParticleCurveMode.Constant:
instanceVertices[offset + 19] = main.gravityModifier.constant;
break;
case ParticleCurveMode.TwoConstants:
instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random());
break;
// Curve / TwoCurves：无写入，slot 19 保持为 0 或环形缓冲区中前一粒子的残留值
}_dispatchDeathEvent:1308 无条件读取 instanceVertices[particleOffset + 19] 作为 gravityMod，用于死亡位置重力项计算 ½·g·gravityMod·lifetime²。Curve/TwoCurves 模式下 slot 19 是不确定的脏数据，Death sub-emitter 生成位置会随机偏移。
建议补 default 分支（t=0 采样作为近似，对静态曲线完全精确，对动态曲线是已知近似误差）：
default:
// Curve/TwoCurves: t=0 sample as approximation; Death position deviates when gravity varies over lifetime
instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, 0);
break;若不希望改变行为，至少在 _dispatchDeathEvent 顶部注释中补充说明此限制（与 VOL/FOL/Noise 并列）。
[P3] PR 描述与实现不符 — COL/SOL/ROL 继承行为
PR 描述 Behavior 表格中写：
ColorOverLifetime / SizeOverLifetime / RotationOverLifetime are NOT applied to the inherited values
但 _modulateInheritByLifetime（ParticleGenerator.ts:1389）已经将 COL/SOL/ROL 应用于继承值——Birth 事件使用 normalizedAge = 0 采样，Death 事件使用粒子实际 normalizedAge。
Checklist 中 incl. parent's COL/SOL not applied to inherit 与实现同样相反。
建议将 Behavior 行改为：
Birth inherit: parent start values with COL/SOL/ROL at normalizedAge = 0 applied
Death inherit: parent values with COL/SOL/ROL applied at actual normalizedAge at time of death
并更新 checklist 条目描述为 incl. parent's COL/SOL **applied** to inherit。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭（_suppressSubEmitterDispatch 设置在 target 上，链深度恒为 1） |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复（SubEmitter 新增 emitCount 字段） |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复 |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
以下两个问题经当前 HEAD 确认仍未修复。
[P1] SubEmittersModule.ts — emitProbability = 0 仍可意外触发
// 当前代码
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}Rand.random() 实现为 randomInt32() / 0xffffffff，返回闭区间 [0, 1]（含两端点）。当 emitProbability = 0 且 randomInt32() 恰好返回 0 时，0.0 > 0.0 为 false，子发射器意外触发。
emitProbability = 0 在语义上应表示「从不触发」，不应依赖 RNG 的底层实现细节。一行修复：
if (sub.emitProbability <= 0) return;
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;[P1] ParticleGenerator.ts — Birth dispatch 在 World 空间模拟时对位置做双重变换
Birth dispatch 当前代码无条件将 position 当作局部坐标处理（旋转 + 平移到世界空间）：
const birthWorldPos = this._eventWorldPos;
Vector3.transformByQuat(position, transform.worldRotationQuaternion, birthWorldPos);
birthWorldPos.add(transform.worldPosition);但在 World 空间模拟（simulationSpace === ParticleSimulationSpace.World）时，position 已经是世界坐标，再做旋转 + 平移会产生双重变换，导致 Birth 子发射的位置偏离实际粒子位置。
对比 _dispatchDeathEvent 的正确实现——它区分了 simSpaceLocal 分支，Local 时使用 transform.worldPosition/Rotation，World 时从 instance buffer 读取存储的世界坐标。Birth dispatch 缺少同等处理。
参照 _dispatchDeathEvent 模式修复：
const birthWorldPos = this._eventWorldPos;
if (main.simulationSpace === ParticleSimulationSpace.Local) {
Vector3.transformByQuat(position, transform.worldRotationQuaternion, birthWorldPos);
birthWorldPos.add(transform.worldPosition);
} else {
// World space: position from shape is already local-space; read the emitted
// world position from the instance buffer (slots 27–29) as _dispatchDeathEvent does.
birthWorldPos.set(
instanceVertices[offset + 27],
instanceVertices[offset + 28],
instanceVertices[offset + 29]
);
}以上两个 P1 修复后可合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭（_suppressSubEmitterDispatch 设置在 target 上，链深度恒为 1） |
| self-reference 检查在概率 roll 之后 | ✅ 已修复 |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复（改为显式 sub.emitCount 字段） |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复（改为 _readParticleStartColor() accessor） |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
| Birth dispatch 位置双重变换 | ✅ 已关闭（position 在两种模式下始终是 shape 局部坐标） |
| 继承 raw start values 而非调制后值 | ✅ 已实现（_modulateInheritByLifetime 在 Birth/Death dispatch 时应用 COL/SOL/ROL） |
[P1] SubEmittersModule.ts — emitProbability = 0 仍可意外触发
当前代码：
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}作者回复将此问题定性为"概率 1/2³²，忽略不计"，但这是对我上轮提案的误读。我的建议不是把 > 改为 >=（那会改变 RNG 语义），而是在 RNG 调用之前增加一个独立的零值守卫：
if (sub.emitProbability <= 0) return; // 零值守卫，不消耗随机数
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) return;这两行互相独立：
- 第一行处理
emitProbability = 0（语义：永不触发），不触碰 RNG 状态 - 第二行的
>语义完全不变
emitProbability = 0 是一个有意义的公开 API 值（用户可以在运行时动态设为 0 来禁用某个 slot），而当前实现在这个值上有一个极小概率（但真实存在）的语义违约。一行守卫即可消除，没有任何副作用。
当前 emitProbability = 0 skips all events 测试并不能守住这个 bug（_probabilityRand.random() = 0.0 的概率 1/2³²，测试的 20 次 emit 极小概率触发）。
[P2] ParticleGenerator.ts — gravityMod 在 Curve/TwoCurves 模式下读取未初始化 slot
_dispatchDeathEvent 无条件读取 instanceVertices[particleOffset + 19] 作为 gravityMod。但 _initParticleVertices 的 gravityModifier switch 只在 Constant 和 TwoConstants 分支写入 slot 19，Curve/TwoCurves 分支无对应 case，slot 19 保持为 buffer 初始值（0 或环形缓冲区中前一粒子的残留值）。
在 Curve gravity 模式下，Death sub-emitter 的生成位置会有非确定性误差。建议至少在 _dispatchDeathEvent 顶部注释中将此列为已知限制（与 VOL/FOL/Noise 并列），或补 default 分支记录 t=0 采样值作为近似：
default:
// Curve/TwoCurves: store t=0 approximation for Death position; deviation expected when gravity varies over lifetime
instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, 0);
break;P1 请修复后合入，P2 建议修复（不阻塞）。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| A→B→A 间接循环递归 | ✅ 已关闭（_suppressSubEmitterDispatch 设置在 target 上，链深度恒为 1） |
| self-reference 检查在概率 roll 之后 | ✅ 已修复（null/destroyed/self/emitCount≤0 均在 roll 前） |
| emitCount 从子系统 t=0 burst 读取 | ✅ 已修复（改为显式 sub.emitCount 字段） |
Color inherit 测试戳私有 _instanceVertices |
✅ 已修复（改为 _readParticleStartColor() accessor） |
_tempQuat0 时序脆弱性 |
✅ 已关闭 |
performance.now 覆盖未恢复 |
✅ 已关闭 |
以下问题当前 HEAD（38a7fd3）确认仍未修复：
[P1] SubEmittersModule.ts — emitProbability = 0 仍可意外触发
// 当前代码
if (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability) {
return;
}Rand.random() 实现为 randomInt32() / 0xffffffff，返回闭区间 [0, 1]（含两端点）。当 emitProbability = 0 且 randomInt32() 恰好返回 0 时，random() 返回 0.0，0.0 > 0.0 为 false，子发射器意外触发。概率约 1/2³²（≈2.3×10⁻¹⁰），看似极低，但在帧率 60fps 的场景中仍会在数十分钟内出现。
emitProbability = 0 语义上是"从不触发"，不应依赖 RNG 实现细节。一行修复：
if (sub.emitProbability <= 0 || (sub.emitProbability < 1.0 && this._probabilityRand.random() > sub.emitProbability)) {
return;
}[P2] SubEmitter.emitter 克隆后静默失效
SubEmitter.emitter 标注了 @ignoreClone，这意味着克隆 ParticleRenderer（如在 prefab 实例化时）后，克隆出的 subEmitters slot 的 emitter 为 null，子发射器静默不工作。PR 描述中提到"行为在文档中说明，属于合理设计限制"，但当前 API 文档中无任何说明，用户会感到困惑。建议至少在 SubEmitter.emitter 的 JSDoc 中注明："克隆时不随 clone 传播，实例化后需手动赋值。"
请处理 P1 后重新提交。
2 participants
Summary
Add
SubEmittersModuleto ParticleGenerator: parent particles can fire additionalParticleRenderers on Birth/Death lifecycle events, mirroring Unity'sParticleSystem.SubEmittersModulesemantics.subEmitterson everyParticleGeneratorSubEmittersModule/SubEmitter/ParticleSubEmitterType/ParticleSubEmitterPropertyParticleRenderer, trigger type (Birth/Death),inheritPropertiesbitmask (Color | Size | Rotation),emitProbability,emitCountBehavior
_addNewParticle_retireActiveParticlesbefore each particle's slot is freeda_ShapePos + dir·speed·lifetime + ½·gravity·r0·lifetime²— does NOT include VOL/FOL/Noise contributions; documented in codeemitCount(defaults to 1); the target renderer's ownEmissionmodule is independentOut of scope (follow-ups)
Collision/Trigger/Manualevent typesLifetime/Durationinherit flagsUsage
Test plan
npm run b:module— all 12 packages build cleanlyvitest run tests/src/core/particle— 87 / 87 (77 existing + 10 new inSubEmitter.test.ts)emitProbability = 0skip, module disabled skip, Color/Size/Rotation inherit value correctness (incl. parent's COL/SOL not applied to inherit), self-reference no-recurseparticleRenderer-sub-emittercase + baseline imageSummary by CodeRabbit
New Features
Tests