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)
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 79.17% 79.28% +0.11%
===========================================
Files 914 919 +5
Lines 101666 102452 +786
Branches 11209 11351 +142
===========================================
+ Hits 80489 81225 +736
- Misses 20990 21039 +49
- Partials 187 188 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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.
| 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
…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.
| * @internal | ||
| */ | ||
| override _onEnable(): void { | ||
| this.generator.subEmitters._bindSlots(); |
There was a problem hiding this comment.
const generator = this.generator;
| @ignoreClone | ||
| _module: SubEmittersModule = null; | ||
|
|
||
| private _emitter: ParticleRenderer = null; |
There was a problem hiding this comment.
私有变量放到 public 变量之后
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-06-17 @ a43fdbdca）
3345da454（上轮基线）是 HEAD 的祖先（git merge-base --is-ancestor 核对通过，线性推进非 force-push）。自上轮新增 5 个 commit，触碰 5 个文件，净 +127/-11：
67fedab6bfix:_onEnable里 reconcile transform-feedback（覆盖反序列化路径）+_setTransformFeedback加isWebGL2 &&门控b98de15e3fix: 把SubEmitter.emitter/type从公开字段改成 reactive setter，路由进 validation1a1eee6b8style: trim 三个@internal方法的注释c1de57577style:_onEnable缓存generator局部变量a43fdbdcastyle:SubEmitterpublic 字段排到 private 之前
总结
b98de15e3 正落地了我上轮 P3 的剩余诉求（sub.emitter 公开可写字段绕过环检测）——把 emitter/type 改成 reactive setter，set emitter 路由进 _validateEmitter（环检测），set type 路由进 _refreshTransformFeedback。67fedab6b 给 _setTransformFeedback 加 isWebGL2 && 门控并在 _onEnable reconcile，正确兜住反序列化路径（_enabled 被直接还原、绕过 setter 的 WebGL1 守卫时不再在 WebGL1 上误建 TF simulator）。逐链核对后，绝大部分正确。唯一新问题是这轮"路由进 validation"只补齐了一半——set type 把 slot 翻成 Death 时缺了 addSubEmitter 已有的 Death+WebGL2 守卫，在 WebGL1 上可静默触发 _onParticleDeath 读 null _feedbackReadback 的崩溃（P1，见下）。
本轮变更核对
① 67fedab6b isWebGL2 && 门控 + _onEnable reconcile — 正确，兜住反序列化 ✓
_setTransformFeedback 的 needed 从 VOL || noise || Death-slot 改成 isWebGL2 && (VOL || noise || Death-slot)。逐链核对其必要性：
- VOL（
LimitVelocityOverLifetimeModuleset enabled，:240）与 Noise（NoiseModuleset enabled，:206）的 setter 本就if (value && !isWebGL2) return静默拒绝——运行时 setter 路径下_enabled在 WebGL1 永不为 true。 - 但反序列化把
_enabled/ Death slot 直接还原（绕过 setter），所以没有这道门控时，_onEnable→_setTransformFeedback会在 WebGL1 上new ParticleTransformFeedbackSimulator→ 后续 draw/program-link 触碰 WebGL2-only API 崩溃。门控落在_setTransformFeedback这个收口点（所有入口都过它）是正确落点。 _onEnable新增_bindSlots()+_setTransformFeedback()：_bindSlots把每个 slot 的_module重新绑定（clone/反序列化后_module是@ignoreClone→ null，需重绑才能让 setter 路由生效）；_setTransformFeedback从当前 config 重算 TF。两者顺序对（先绑定再重算），配回归测试 "Death slot reconciles transform-feedback on enable (deserialize path)"（手动把_useTransformFeedback=false/_feedbackSimulator=null模拟反序列化，断言 enable 后重建）。
② b98de15e3 reactive setter — clone 路径逐链验证无双克隆/无误触发 setter ✓
逐链核对 clone 与 setter 的交互（这是最 subtle 的一处）：
SubEmitter经@deepClone _subEmitters数组深拷贝。CloneManager.cloneProperty的default分支用for (let k in sourceProperty)遍历——ES class field（loose 模式 babel 编译成构造器内this._emitter = null赋值）是 own enumerable，accessor（get/set emitter）在 prototype 上 非枚举。故for...in只遍历_emitter/_type/_module/三个 public 字段，不会走emitter/typesetter，也不会二次克隆 accessor。_emitter：ParticleRenderer（Component）带_remap→cloneProperty优先走_remap（CloneManager.ts:109，先于 cloneMode），跨层级 remap，直接写_emitter不经 setter——此时_module仍 null，即便经 setter 也是 no-op，无误触发 validation。_type：primitive → 直接赋值，不经 setter。_module：@ignoreClone→CloneMode.Ignore→ skip，clone 后为 null，靠_onEnable的_bindSlots重绑。
addSubEmitter内sub.emitter=emitter/sub.type=type都在sub._module=this之前执行，故 setter 里的this._module?._validateEmitter/_refreshTransformFeedback全是 no-op（optional chaining 短路）；addSubEmitter自己做完整的 Death+WebGL2 检查（:73）、环检测（:77）、_setTransformFeedback（:88）。无双重 validation、无premature。绑定_module后的后续slot.emitter=x/slot.type=y才路由进 validation——正是预期的封装。set emitter先_validateEmitter(value)再this._emitter = value，throw 时_emitter不变（测试 "Changing a slot's emitter to form a cycle throws" 断言slot.emitter === c守住）。✓- setter 职责拆分正确：
set emitter只验环（改 emitter 不改_hasSubEmitterOfType(Death)，无需刷 TF）；set type只刷 TF（翻进/出 Death 改变_hasSubEmitterOfType(Death)，需刷；但不涉及环）。
③ 1a1eee6b8 trim 注释 — 符合本文件惯例 ✓
删的是 _bindSlots/_refreshTransformFeedback/_validateEmitter 三个 @internal 方法的机制描述 + _module 的 doc body。本文件其他 @internal 私有方法（_dispatchEvent/_resetRandomSeed/_hasSubEmitterOfType）一律裸 /** @internal */ 无 body——这次 trim 与既有惯例一致。删的是 WHAT/机制描述，非保护非显然不变量的 load-bearing WHY（不同于我历轮提的 >= 概率注释），不再单列。
④ c1de57577 / a43fdbdca style — 行为中性 ✓
generator 局部缓存（getter 调 1 次 vs 4 次，等价）；字段排序（_module @internal 在前、private _emitter/_type 在后，符合 member-ordering 惯例，纯 whitespace）。
问题
[P1] SubEmitter.ts:44 set type 缺 Death+WebGL2 守卫 — 在 WebGL1 上把 slot 翻成 Death 可触发 _onParticleDeath 读 null _feedbackReadback 崩溃
这轮 b98de15e3 的核心目标是"route slot mutations through validation"，但只路由了一半：set emitter 复制了 addSubEmitter 的环检测（经 _validateEmitter），set type 却没有复制 addSubEmitter 的 Death+WebGL2 守卫（addSubEmitter :73 if (type === Death && !isWebGL2) throw）。
逐链核对崩溃路径（WebGL1）：
addSubEmitter(child, Birth)在 WebGL1 合法（不抛），slot 落地、_module绑定。slot.type = ParticleSubEmitterType.Death经set type→ 不抛 →_type=Death+_refreshTransformFeedback→_setTransformFeedback→needed = isWebGL2(false) && ... = false→_feedbackSimulator保持 null。- 父粒子退役：
_retireActiveParticles（ParticleGenerator.ts:1226，在_update内无条件跑，不受 instancedArrays/WebGL2 gate）。hasDeathSlot = _hasSubEmitterOfType(Death) = true。 :1247 if (this._feedbackSimulator && !feedbackLoaded)——_feedbackSimulator为 null →_readbackFeedback被跳过，_feedbackReadback仍是初始 null（:142）。:1251 _onParticleDeath(activeParticleOffset)仍被调用 →:1295 const feedbackData = this._feedbackReadback（null）→:1296 feedbackData[feedbackOffset]→ TypeError: Cannot read properties of null。
即新的"受支持的 validated 变更入口"（PR 把 set type 做成 reactive 并配测试 "Changing a slot's type to Death reconciles transform-feedback"，明确把它当一等 API）破坏了 addSubEmitter 立下的 fail-fast 契约：本该立刻抛 "Death sub-emitter requires WebGL2"，却静默什么都不建、推迟到 _onParticleDeath 深处一个晦涩的 null-deref 崩溃。这正违反"不支持应尽早报错，不应静默跳过"——比 addSubEmitter 的契约严格更差。
注意这与上轮 P3 不是同一问题：上轮是 emitter 字段绕过环检测（已由 set emitter 关闭），本条是 set type 缺Death+WebGL2 检测——不同 setter、不同守卫、且是本轮新代码引入的不对称。
修法（任选）：
- 首选：
set type在value === Death && !isWebGL2时throw new Error("Death sub-emitter requires WebGL2")，与addSubEmitter对称。把 Death+WebGL2 检查抽成_module?._validateType(value)与_validateEmitter并列，两个 setter 各调各的，封装一致。 - 或：补一条 WebGL1 回归测试（mock
isWebGL2=false）覆盖set type = Death，先让崩溃显形再决定守卫落点。
WebGL2 上无此问题（
set type=Death正确建 TF，测试已覆盖）；触发面是 WebGL1 + 经set type（非addSubEmitter）翻成 Death，narrow 但是真崩溃路径，且恰好落在本 commit 主打的"validated mutation"语义里——这是该补的对称守卫。
已关闭问题清单（全部维持）
| 问题 | 状态 |
|---|---|
历史全部 P0/P1/🔴（A→B→A 环递归、多级链 instance scratch、World 出生旋转、Birth/Death velocity 继承、emitCount 不截断、enum 重命名、e2e 时序、setKeys 4-key、emitProbability=0 根因 >=、COL/SOL/ROL 调制、cumulative-curve 搬移、空 gradient →0、Birth dispatch 门控、override 转 args、死 @ignoreClone 删除、间接环 throw 测试、Death readback 窗口收窄、clamp 区间积分） |
✅ 全部闭环，本轮零差异 |
上轮 P3 sub.emitter 公开可写字段绕过环检测 |
✅ 本轮关闭——emitter 改 reactive setter，路由进 _validateEmitter，配 "Changing a slot's emitter to form a cycle throws" 回归 |
上轮 P3 subEmitters getter 单行 JSDoc |
维持（纯格式，不再单列；本轮新 emitter/type getter 单行 doc 与本文件 public 字段惯例一致） |
| 长期遗留 P2：单 key + clamp 区间 CPU/GPU 分叉 / 测试裸 buffer offset | 维持；本轮未触碰 ParticleCurve.ts / 测试断言裸 offset |
结论
5 个 commit：reactive setter 封装（关闭上轮 P3 的 emitter 绕过、clone 路径经 for...in own-enumerable 逐链验证不双克隆/不误触发 setter）+ isWebGL2 && TF 门控（正确兜住反序列化路径，落点在收口的 _setTransformFeedback）+ 3 个行为中性的 style。绝大部分逐链核对正确。 唯一新问题：b98de15e3 的"route slot mutations through validation"只补了一半——set type 缺 addSubEmitter 已有的 Death+WebGL2 守卫，在 WebGL1 上经 set type=Death 可触发 _onParticleDeath 读 null _feedbackReadback 的崩溃（P1，narrow 但真实，建议 set type 对称补守卫或补 WebGL1 回归）。CI 实质全绿（lint / build×3 / e2e×4 / codecov project+patch 本轮均 pass）。补上对称守卫后即可合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
复审 @ a43fdbdca（P2/P3 已闭环，上轮 P1 仍未真正修复，维持 request-changes）
自上轮（1a1eee6b8）以来推了一批 commit，其中三个针对我上轮提的三条：
- [P2 已闭环]
subEmittersgetter 返回内部活数组 →623cc47d5改为get subEmitters(): readonly SubEmitter[]（SubEmittersModule.ts:50），并把 mutation 收口到addSubEmitter/removeSubEmitterByIndex。关闭。 - [P3 已闭环] slot 直接 mutate 绕过校验 →
b98de15e3把 slot 改动路由进_validateEmitter/addSubEmitter的环检测 + WebGL2 守卫。关闭。 - [P1 未真正修复] WebGL1 反序列化加载 Death slot 的 null-deref →
67fedab6b给_setTransformFeedback的needed加了isWebGL2门控，并补了一条 "deserialize 重建 TF" 的回归测试——但那条测试跑在 WebGL2 上、只断言 WebGL2 行为，WebGL1 的崩溃路径仍未覆盖也未修。isWebGL2门控恰恰是把 WebGL1 暴露出来的那一步。
仍开放（阻塞）
[P1] WebGL1 反序列化含 Death sub-emitter 的场景，父粒子一死即 _onParticleDeath 解引用 null _feedbackReadback → TypeError 崩溃。
逐链核对 a43fdbdca 实际源码（每一环都读了真实代码，非推断）：
_subEmitters是@deepClone private（SubEmittersModule.ts:46-47），反序列化/clone 直接还原，绕过addSubEmitter的 WebGL1+Death throw 守卫（SubEmittersModule.ts:72-73）。能力门控只挂在配置入口，序列化字段天然绕过它。ParticleRenderer._onEnable调generator._setTransformFeedback()；WebGL1 下needed = isWebGL2 && (...)=false（ParticleGenerator.ts:684）→ 不进if (needed)分支 →_feedbackSimulator/_feedbackReadback保持 null（ParticleGenerator.ts:134/142）。_retireActiveParticles（每帧从_update跑，无isWebGL2门，ParticleGenerator.ts:354）里hasDeathSlot = subEmitters._hasSubEmitterOfType(Death)（:1232）只看enabled+ slot 类型，与_useTransformFeedback/_feedbackSimulator无关 → WebGL1 下仍为true。:1246-1252：if (hasDeathSlot) { if (this._feedbackSimulator && !feedbackLoaded) { // ← readback 被守住了 this._readbackFeedback(...); feedbackLoaded = true; } this._onParticleDeath(activeParticleOffset); // ← 但这行无条件调 }
_onParticleDeath（:1288）：const feedbackData = this._feedbackReadback;（null）→:1298local.set(feedbackData[feedbackOffset], ...)读 null 的下标 →TypeError: Cannot read properties of null。:1295→:1298之间无任何 null 检查。
零用户错误：正常的 WebGL2 编辑 → WebGL1 运行时降级加载即触发，父粒子越过 lifetime 的第一帧就崩。
修法（能力门控下沉到消费侧，与 _readbackFeedback 的守卫口径统一）：把 :1246 的 if (hasDeathSlot) 收紧为 if (hasDeathSlot && this._feedbackSimulator)（或 && this._useTransformFeedback），这样 WebGL1 下 Death 派发整体跳过，_readbackFeedback 那层守卫也随之冗余可清。或在 _onParticleDeath 入口 if (!this._feedbackReadback) return;。配一条真正跑在 WebGL1 上的回归测试（reconcile 后 _feedbackSimulator===null，父粒子死亡不抛）守住——现有 SubEmitter.test.ts:143 那条只验证 WebGL2，revert 修复也不会 fail。
与上轮 [P3]「运行时 mutate 绕过守卫」是同类（能力门控只挂配置入口）但更严重：那条是用户误用、doc 为 unsupported，这条是零用户错误的正常降级加载路径。
不阻塞 / 已记录
- 同步 readback stall（
@cptbtptpbcptdtptp提）：0f931fab已收窄到「仅退役帧 × 仅活跃区间」，残留同步getBufferSubData是无 fence 基建的固有取舍、读上一帧 buffer、可接受降级，follow-up，不阻塞。 SubEmitter仍export可new（index.ts），但唯一受支持入口是addSubEmitter；标@internal可消除不一致 + 崩溃向量。P3，不阻塞。
结论
request-changes：P2/P3 已正确闭环；唯一阻塞仍是 WebGL1 反序列化 Death slot 的 null-deref（P1，未真正修复）——67fedab6b 修的是 WebGL2 重建路径，WebGL1 崩溃路径的能力门控需下沉到 _retireActiveParticles/_onParticleDeath 的消费侧，并配 WebGL1 回归测试。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
复审 @ a43fdbdca（P2/P3 已闭环，上轮 P1 仍未真正修复，维持 request-changes）
自上轮（1a1eee6b8）以来推了一批 commit，其中三个针对我上轮提的三条：
- [P2 已闭环]
subEmittersgetter 返回内部活数组 →623cc47d5改为get subEmitters(): readonly SubEmitter[]（SubEmittersModule.ts:50），并把 mutation 收口到addSubEmitter/removeSubEmitterByIndex。关闭。 - [P3 已闭环] slot 直接 mutate 绕过校验 →
b98de15e3把 slot 改动路由进_validateEmitter/addSubEmitter的环检测 + WebGL2 守卫。关闭。 - [P1 未真正修复] WebGL1 反序列化加载 Death slot 的 null-deref →
67fedab6b给_setTransformFeedback的needed加了isWebGL2门控，并补了一条 "deserialize 重建 TF" 的回归测试——但那条测试跑在 WebGL2 上、只断言 WebGL2 行为，WebGL1 的崩溃路径仍未覆盖也未修。isWebGL2门控恰恰是把 WebGL1 暴露出来的那一步。
仍开放（阻塞）
[P1] WebGL1 反序列化含 Death sub-emitter 的场景，父粒子一死即 _onParticleDeath 解引用 null _feedbackReadback → TypeError 崩溃。
逐链核对 a43fdbdca 实际源码（每一环都读了真实代码，非推断）：
_subEmitters是@deepClone private（SubEmittersModule.ts:46-47），反序列化/clone 直接还原，绕过addSubEmitter的 WebGL1+Death throw 守卫（SubEmittersModule.ts:72-73）。能力门控只挂在配置入口，序列化字段天然绕过它。ParticleRenderer._onEnable调generator._setTransformFeedback()；WebGL1 下needed = isWebGL2 && (...)=false（ParticleGenerator.ts:684）→ 不进if (needed)分支 →_feedbackSimulator/_feedbackReadback保持 null（ParticleGenerator.ts:134/142）。_retireActiveParticles（每帧从_update跑，无isWebGL2门，ParticleGenerator.ts:354）里hasDeathSlot = subEmitters._hasSubEmitterOfType(Death)（:1232）只看enabled+ slot 类型，与_useTransformFeedback/_feedbackSimulator无关 → WebGL1 下仍为true。:1246-1252：if (hasDeathSlot) { if (this._feedbackSimulator && !feedbackLoaded) { // ← readback 被守住了 this._readbackFeedback(...); feedbackLoaded = true; } this._onParticleDeath(activeParticleOffset); // ← 但这行无条件调 }
_onParticleDeath（:1288）：const feedbackData = this._feedbackReadback;（null）→:1298local.set(feedbackData[feedbackOffset], ...)读 null 的下标 →TypeError: Cannot read properties of null。:1295→:1298之间无任何 null 检查。
零用户错误：正常的 WebGL2 编辑 → WebGL1 运行时降级加载即触发，父粒子越过 lifetime 的第一帧就崩。
修法（能力门控下沉到消费侧，与 _readbackFeedback 的守卫口径统一）：把 :1246 的 if (hasDeathSlot) 收紧为 if (hasDeathSlot && this._feedbackSimulator)（或 && this._useTransformFeedback），这样 WebGL1 下 Death 派发整体跳过，_readbackFeedback 那层守卫也随之冗余可清。或在 _onParticleDeath 入口 if (!this._feedbackReadback) return;。配一条真正跑在 WebGL1 上的回归测试（reconcile 后 _feedbackSimulator===null，父粒子死亡不抛）守住——现有 SubEmitter.test.ts:143 那条只验证 WebGL2，revert 修复也不会 fail。
与上轮 [P3]「运行时 mutate 绕过守卫」是同类（能力门控只挂配置入口）但更严重：那条是用户误用、doc 为 unsupported，这条是零用户错误的正常降级加载路径。
不阻塞 / 已记录
- 同步 readback stall（
@cptbtptpbcptdtptp提）：0f931fab已收窄到「仅退役帧 × 仅活跃区间」，残留同步getBufferSubData是无 fence 基建的固有取舍、读上一帧 buffer、可接受降级，follow-up，不阻塞。 SubEmitter仍export可new（index.ts），但唯一受支持入口是addSubEmitter；标@internal可消除不一致 + 崩溃向量。P3，不阻塞。
结论
request-changes：P2/P3 已正确闭环；唯一阻塞仍是 WebGL1 反序列化 Death slot 的 null-deref（P1，未真正修复）——67fedab6b 修的是 WebGL2 重建路径，WebGL1 崩溃路径的能力门控需下沉到 _retireActiveParticles/_onParticleDeath 的消费侧，并配 WebGL1 回归测试。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
P1 复核 @ a43fdbdca — WebGL1 反序列化 Death slot 仍会崩溃（67fedab6b 未真正修复）
逐链核对当前 HEAD 源码（非推断，每环都读了真实代码），确认这个 P1 仍然存在：
崩溃链
-
Death 才要 WebGL2，Birth 支持 WebGL1（设计正确）——
addSubEmitter只对type === Death && !isWebGL2throw（SubEmittersModule.ts:72-73）。Birth 不读 feedback buffer，WebGL1 能跑；Death 要 TF 读回死亡位置，仅 WebGL2。 -
但这道 throw 只挂在
addSubEmitter配置入口 ——_subEmitters是@deepClone private（SubEmittersModule.ts:46-47）。反序列化/clone（场景从 WebGL2 编辑器导出、含 Death slot，在 WebGL1 运行时加载）直接还原数组，根本不经过addSubEmitter→ 绕过 Death+WebGL1 的 throw → WebGL1 上活了一个本该被拒的 Death slot。 -
WebGL1 下
_feedbackReadback为 null ——_setTransformFeedback的needed = isWebGL2 && (...)（ParticleGenerator.ts:683-684），WebGL1 下 false → 不进if(needed)→_feedbackSimulator/_feedbackReadback保持 null（:134/:142）。67fedab6b加的这个isWebGL2门恰恰把 WebGL1 暴露出来。 -
但 Death 派发不受 TF 状态门控 ——
hasDeathSlot = _hasSubEmitterOfType(Death)（:1232）只看enabled+ slot 类型，与_feedbackSimulator无关 → WebGL1 下仍true：
```ts
if (hasDeathSlot) { // :1246 WebGL1 下为 true
if (this._feedbackSimulator && !feedbackLoaded) { // :1247 readback 被守住
this._readbackFeedback(...);
}
this._onParticleDeath(activeParticleOffset); // :1251 ← 但这行无条件调
}
``` -
_onParticleDeath入口无 null 守卫 ——:1295const feedbackData = this._feedbackReadback;（null）→:1298local.set(feedbackData[feedbackOffset], ...)→TypeError: Cannot read properties of null。
触发场景零用户错误：WebGL2 编辑器创建含 Death sub-emitter 的特效 → WebGL1 设备运行时降级加载 → 父粒子越过 lifetime 的第一帧即崩。
67fedab6b 为什么没修上
它给 _setTransformFeedback 加了 isWebGL2 门 + 一条 "deserialize 重建 TF" 回归测试——但那条测试跑在 WebGL2 上、只断 WebGL2 行为。WebGL1 的崩溃路径既没修也没覆盖。isWebGL2 门本身是对的，但它只解决了"WebGL2 下反序列化要重建 TF"，没解决"WebGL1 下反序列化的 Death slot 该怎么办"。
修法（二选一，建议 fail-loud）
能力门控只挂配置入口、序列化字段天然绕过——根治要么在加载侧校验、要么在消费侧下沉守卫：
- 方案 A（消费侧守卫，最小改动）：
:1246收紧为if (hasDeathSlot && this._feedbackSimulator)，WebGL1 下 Death 派发整体跳过，:1247那层守卫随之冗余可清。但纯静默跳过会让 WebGL1 上的 Death 悄悄不工作，用户对着"没效果"摸不着头脑 → 建议补一条Logger.warn。 - 方案 B（加载侧校验，fail-loud，更对）：反序列化重建/
_onEnablereconcile 时，WebGL1 下检测到 Death slot 就Logger.error+ 丢弃该 slot，跟addSubEmitter配置入口的 throw 口径一致。
按"不支持应尽早报错、不静默跳过"原则，B 优于 A。无论哪个，都要配一条真正跑在 WebGL1 上的回归测试（reconcile 后 _feedbackSimulator === null、父粒子死亡不抛）——现有 SubEmitter.test.ts 那条只验证 WebGL2，revert 修复也不会 fail，守不住这个回归。
结论
request-changes：这是零用户错误的正常降级加载路径上的必现崩溃，必须修。其余 commit（subEmitters 收口 623cc47d5/b98de15e3、style 三连）都干净。3345da454 的 CPU/GPU cumulative 分叉是已知 P2（前轮已记），不阻塞。
补充：P1 的推荐修法 —— 整个 sub-emitter 模块统一要求 WebGL2，别按 Birth/Death 割裂上一条 review 给的两个修法（消费侧下沉守卫 / 加载侧校验）都是在保留 Birth/Death 能力分级的前提下补 P1。但更根本的方向是消除这个分级本身： 为什么"整个模块 WebGL2"优于"Death 才要 WebGL2"
代价（诚实说明）砍掉 WebGL1 上的 Birth sub-emitter（现在它能跑且零成本）。但 sub-emitter 是高级特效功能，用它的项目大概率不卡 WebGL1 长尾；且"统一要 WebGL2"比"WebGL1 上半残"对用户更友好。 这是产品决策，建议团队拍板它改变了功能支持矩阵（WebGL1 失去 Birth sub-emitter），不是纯技术修复。但从"别割裂、消除架构泄漏 + 陷阱状态 + P1 一锅端"的角度，倾向整个模块统一 WebGL2。无论选哪个方向，都要配一条真跑在 WebGL1 上的回归测试守住（现有测试只跑 WebGL2，revert 修复也不 fail）。 |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
P1 阻塞 @ a43fdbdca — WebGL1 下含 Death sub-emitter 会崩溃；建议整个模块统一 WebGL2
逐链核对当前 HEAD 源码（非推断），确认一个零用户错误的必现崩溃，且 67fedab6b 未真正修复。其余新 commit（subEmitters 收口 623cc47d5/b98de15e3、style 三连）干净；3345da454 的 CPU/GPU cumulative 分叉是已知 P2，不阻塞。
[P1] 崩溃链
设计上 Death 才要 WebGL2，Birth 支持 WebGL1（Birth 出生方向 CPU 闭式可得，Death 要 TF 读回死亡位置）。但这道能力门只挂在 addSubEmitter 配置入口：
addSubEmitter仅对type === Death && !isWebGL2throw（SubEmittersModule.ts:72-73）。_subEmitters是@deepClone private（:46-47）——反序列化/clone（WebGL2 编辑器导出含 Death slot 的特效、WebGL1 设备降级加载）直接还原数组，不经过addSubEmitter→ 绕过这道 throw。- WebGL1 下
_setTransformFeedback的needed = isWebGL2 && (...)（ParticleGenerator.ts:683-684）为 false →_feedbackReadback保持 null。 - 但
hasDeathSlot = _hasSubEmitterOfType(Death)（:1232）只看 enabled+slot 类型 → WebGL1 下仍 true →:1251 _onParticleDeath()无条件调（:1247的_feedbackSimulator &&只守住 readback，没守住 dispatch）。 _onParticleDeath入口无 null 守卫 →:1298 local.set(feedbackData[feedbackOffset], ...)读 null →TypeError。
触发零用户错误：WebGL2 编辑器做的含 Death 特效 → WebGL1 设备运行时加载 → 父粒子越过 lifetime 第一帧即崩。67fedab6b 加的 isWebGL2 门 + 回归测试只覆盖 WebGL2 路径，WebGL1 崩溃路径未修未测。
推荐修法：整个 sub-emitter 模块统一要求 WebGL2，别按 Birth/Death 割裂
可以只在消费侧下沉守卫（:1246 改 if (hasDeathSlot && this._feedbackSimulator)）补这个 P1，但那保留了 Birth/Death 能力分级。更根本的方向是消除分级本身：
- 分级是架构泄漏 —— "Birth 能用、Death 不能"纯源于内部实现，用户脑子里 sub-emitter 是一个功能，预期不到这个边界。
- 割裂制造半残陷阱 —— WebGL1 上 Birth 工作、Death 失效/崩，"一半好使"比"整个不支持"更难排查。完全不支持是诚实的，半支持是陷阱——这正是 P1 的土壤。
- 一句契约 > 两句分级 —— "sub-emitter 需要 WebGL2" 记得住守得住。
- P1 顺带消失（消除土壤而非修补） —— 把 WebGL2 门从
addSubEmitter上移到enabledsetter /_onEnable（所有进入路径的收口，含反序列化/clone）。WebGL1 下模块整个不启用 →_onParticleDeath永不被调。addSubEmitter的 Death throw 删除，运行层:1247的_feedbackSimulator &&守卫也因"有 Death slot ⇒ 必 WebGL2 ⇒ simulator 必在"可简化。
代价：砍掉 WebGL1 上的 Birth sub-emitter（现在它能跑且零成本）。但 sub-emitter 是高级特效，用它的项目大概率不卡 WebGL1 长尾，"统一要 WebGL2"比"WebGL1 半残"更友好。这改变功能支持矩阵，属产品决策，建议团队拍板。
行数上两个方案半斤八两（都 ±几行），统一方案的价值在心智一致与消除陷阱，不在省代码。
回归测试（无论哪个方向都必须）
配一条真跑在 WebGL1 上的测试：reconcile 后 _feedbackSimulator === null、父粒子死亡不抛（或模块在 WebGL1 整个不启用）。现有 SubEmitter.test.ts 那条只验证 WebGL2，revert 修复也不会 fail，守不住这个回归。
澄清：统一的是「准入条件」，不是「内部实现」—— Birth 零改动补一句防误解：上面建议的「整个模块统一 WebGL2」统一的只是模块准入门，不是 Birth/Death 的内部实现。两者实现本来就不同、也不该改成一样：
所以统一 WebGL2 后：
换句话说：砍掉 WebGL1 Birth 是准入门顺带挡的（为了不割裂），不是 Birth 技术上做不到。统一的是「谁能进这个模块」这一道门，Birth/Death 各自的实现路径维持不变。 |
A deserialized / cloned Death sub-emitter crashed on WebGL1: the WebGL2 gate lived only in addSubEmitter, which deserialization and clone bypass, so on WebGL1 _setTransformFeedback left the feedback buffer null while _retireActiveParticles still entered _onParticleDeath and dereferenced a null _feedbackReadback. Move the gate to the module's enabled getter — the chokepoint every path reads, including deserialize/clone — so the whole sub-emitter module is inert on WebGL1 and _onParticleDeath is never reached. Drop the per-method Death throw; the Birth/Death capability split is gone (the module simply requires WebGL2).
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
✅ Approve @ b6dced39c — P1 真闭环，整个 sub-emitter 模块统一要求 WebGL2
自上轮 P1 阻塞（a43fdbdca）以来线性推进 1 个 commit b6dced39c fix(particle): require WebGL2 for the whole sub-emitter module，只改 SubEmittersModule.ts（-5/+2）+ 回归测试（+32），正是上轮推荐的「整个模块统一 WebGL2」方向。逐链核对源码（非信 commit message），P1 崩溃路径已从控制流上彻底封死。
P1 [已闭环] WebGL1 反序列化/clone Death slot null-deref
修法 = 把 WebGL2 门从被绕过的配置入口（addSubEmitter）上移到所有路径的收口（enabled getter）：
override get enabled(): boolean {
return this._enabled && this._generator._renderer.engine._hardwareRenderer.isWebGL2;
}崩溃路径已断（逐链验证）：
- WebGL1 ⇒
enabledgetter 恒false。 _hasSubEmitterOfType()首行if (!this.enabled) return false（SubEmittersModule.ts）⇒ WebGL1 下对任意 type 恒返回 false。它是 Death 路径（_setTransformFeedback的needed、_retireActiveParticles的hasDeathSlot）和 Birth 路径（_onParticleBirth派发判定）的唯一判定函数。- ⇒
_retireActiveParticles的hasDeathSlot恒 false ⇒if (hasDeathSlot){ … _onParticleDeath() }整块不进 ⇒_onParticleDeath永不被调 ⇒ 不可能读到 null_feedbackReadback。
收口点选得准：反序列化/clone 还原的 @deepClone _subEmitters 数组也必须过 _hasSubEmitterOfType 才会被消费，所以这道门覆盖了上轮指出的"绕过 addSubEmitter"路径。addSubEmitter 里那段 per-method Death throw 已删——Birth/Death 能力割裂消失，一句契约「sub-emitter 需要 WebGL2」取代两句分级。这正是上轮分析的"消除土壤而非修补补丁"。
Birth 实现零改动确认：本 commit 未触碰 ParticleGenerator.ts（_onParticleBirth/_feedbackReadback 均无 diff）。统一的只是准入条件，不是 Birth 执行逻辑——Birth 仍是纯 CPU 闭式几何、不读 TF buffer，只是 WebGL1 下随整个模块一并 inert。砍掉 WebGL1 上零成本的 Birth sub-emitter 是这个统一方向的预期代价，属已同意的产品决策。
回归测试 [守得住]
新增 Sub-emitter stays inert on WebGL1 instead of crashing on Death（+32）反向证伪成立：
- 父
startLifetime = 0.5s，updateEngine(engine, 10)= 10 帧 × 100ms = 1.0s > 0.5s ⇒ 父粒子必然死亡，确实跑到_retireActiveParticles的hasDeathSlot判定点。 - 关键断言
expect(() => updateEngine(engine, 10)).to.not.throw()命中崩溃点：fix 在位时短路不进_onParticleDeath；若 revert（enabled改回return this._enabled），WebGL1 下hasDeathSlot恢复 true ⇒ 进_onParticleDeath⇒ 读 null_feedbackReadback⇒ TypeError ⇒ 测试真红。不是假绿。 - 另两条
enabled === false/_feedbackSimulator不存在 /child._getAliveParticleCount() === 0正交验证模块在 WebGL1 整个 inert。
仍开放（均非阻塞，沿用历轮定性）
- [P2] CPU
_evaluateCumulativevs GLSLevaluateParticleCurveCumulative实现分叉（3345da454起）：仅当曲线 key 落在 [0,1] 域外才有可见差异，而那是编辑器禁止的配置。运行时信任协议下不阻塞。本 commit 未触及。 - 可选简化（非必须）：消费层
if (hasDeathSlot)块内_feedbackSimulator &&守卫，现在因「有 Death slot ⇒ enabled ⇒ 必 WebGL2 ⇒ simulator 必在」逻辑上恒真，可删；留着是无害的额外防御，不影响合并。
唯一阻塞项 P1 已真修真测，无新增问题。LGTM ✅
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
复审 @ 22d19d5f5（P1 已闭环，--comment 可合并）
自上轮 --request-changes（a43fdbdca）以来推进到 22d19d5f5，核心是 b6dced39c —— 作者采纳了我历轮收敛的处方：把 WebGL2 能力门从配置入口下沉到 enabled 这个所有路径都读的 chokepoint，整个 sub-emitter 模块在 WebGL1 上统一 inert。上轮阻塞的 WebGL1 反序列化 Death null-deref P1 已真正修复并被回归测试守住。
上轮 P1 已闭环（WebGL1 反序列化/clone Death slot null-deref）
逐链亲读源码确认修复正确且最小：
SubEmittersModule.enabledgetter 改为return this._enabled && ...isWebGL2（SubEmittersModule.ts:96-97），删掉addSubEmitter里只挡配置入口的Death requires WebGL2throw。- 关键传导：
_hasSubEmitterOfType首行if (!this.enabled) return false（:160-161）→ WebGL1 下对 Death/Birth 一律返回 false。 - 因此 WebGL1 下：①
_setTransformFeedback（PG.ts:687）的needed为 false →_feedbackSimulator/_feedbackReadback不建（保持 null）；②_retireActiveParticles（PG.ts:1232）hasDeathSlot=false→ 整个if (hasDeathSlot)块（含_onParticleDeath）被跳过 → 不再裸读 nullfeedbackData[feedbackOffset]。崩溃路径从根上消失（消除而非修补）。 - 反序列化/clone 绕过问题随之消解：门挂在 getter 上，
@deepClone _subEmitters还原后照样读这个 getter，不存在"add API 守了、反序列化绕过"的割裂。
回归测试真覆盖崩溃平台（上轮的硬要求已满足）
新增 Sub-emitter stays inert on WebGL1 instead of crashing on Death（SubEmitter.test.ts）从一开始就 hardwareRenderer._isWebGL2 = false，断言 enabled===false、_useTransformFeedback===false、_feedbackSimulator 不存在，并 expect(() => updateEngine(engine, 10)).to.not.throw()（父粒子死亡帧不崩）。revert enabled 门 → hasDeathSlot 复为 true → _onParticleDeath 读 null → 测试 fail，是守得住的真回归测试，不是只跑 WebGL2 的恒过桩。这正补上了上轮"测试只跑 WebGL2 等于没守 WebGL1"的缺口。
方向肯定：统一 WebGL2 > Birth/Death 能力分级
砍掉 Birth(WebGL1 可跑)/Death(仅 WebGL2)的割裂，整模块统一"要 WebGL2、否则静默 inert"，与引擎既有惯例一致（LimitVelocityOverLifetimeModule 的 @remarks This module requires WebGL2, On WebGL1, enabling will be silently ignored）。一句契约胜过两句分级，也消除了"WebGL1 上一半好使"的半残陷阱。
其余 commit（纯文档，已核对合规）
e4e8f375e/69d08c825/22d19d5f5 把 SubEmitter.emitter/type getter 的单行 /** */ 展开为多行 JSDoc 并加句号、删掉描述内部 remap 的 Remapped on clone 备注 —— 符合公开 getter 多行 JSDoc 规范，无功能变化。
结论
P1 闭环，无新 P0/P1/P2，--comment，建议合并。 历史已采纳清单（readonly subEmitters getter / slot mutate 收口 / 环检测 / Birth velocity 继承等）不再复述。同步 readback stall 是无 fence 基建的固有取舍（仅退役帧 × 仅活跃区间，读上一帧），属 follow-up 不阻塞，维持上轮结论。
…eshTransformFeedback passthrough
…_onEnable Cloned slots lose their @ignoreClone _module back-pointer, which _onEnable re-set on every enable via _bindSlots. Move the re-link into _cloneTo so it runs once at clone time (addSubEmitter already wires _module at birth). Add a clone regression test covering the back-pointer.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-06-17 @ c33a07daa）
自我上轮 --comment（22d19d5f5，P1 已闭环）以来线性推进 4 个 commit（git merge-base --is-ancestor 22d19d5f5 c33a07daa 通过，非 force-push）：
e18de0662/70772d059docs:subEmittersgetter JSDoc 多行化后又收成 "The configured sub-emitters."af6b4f913refactor:ParticleGeneratorModule._generatorprotected→@internal，内联删掉_refreshTransformFeedback()一行 passthroughc33a07daarefactor: 把 slot 的_module重链从_onEnable._bindSlots()挪到_cloneTo()
总结
af6b4f913 的两处改动都对：_generator 必须 @internal（SubEmitter.type setter 要跨类读 this._module._generator，protected 读不到，与 MainModule.ts:338 跨实例读 target._generator 一致），_refreshTransformFeedback 单调用者内联是干净简化。doc 改动合规。
但 c33a07daa 把 _module 重链从「每次 enable」收窄到「仅 clone」，丢掉了反序列化路径的覆盖 —— 这恰是作者自己在引入 _bindSlots 时（b98de15e3）注释里点名要覆盖的场景。见下 P2。逐链核对正确，无 P0/P1。
问题
[P2] SubEmittersModule.ts c33a07daa — 把 _module 重链从 _onEnable 挪到 _cloneTo，丢掉了反序列化路径的 _module 绑定（回退了 b98de15e3 显式覆盖的场景）；当前不可达，但 serialization 一落地即成潜伏崩溃
SubEmitter._module 是 slot setter 的唯一拐杖：set type 用它触发 _setTransformFeedback()、set emitter 用它触发 _validateEmitter() 环检测，两者都走 this._module?.… 的可选链——_module 为 null 时静默跳过。
逐链核对 _module 的三条赋值路径（全仓 grep _module = 仅两处赋值点）：
addSubEmitter—sub._module = this（:84）✓- clone —
_cloneTo重链到target（:178）✓（核对 CloneManager：_subEmitters数组在for k in sourceProperty里先被@deepClone完，_cloneTo?.(target)后调，slot 已就位，重链正确） - 反序列化 —
ReflectionParser.parseProps走instance[key] = v/new Class()直填字段，不过addSubEmitter、也不调_cloneTo→ 每个反序列化 slot 的_module恒为 null
git 历史佐证这是回退一个刻意的修复：b98de15e3 引入 _bindSlots() 时注释原文写的是
Slots restored by deserialization / clone arrive unbound, so re-bind on renderer enable.
——明确点名 deserialization 和 clone 都需要重链，并刻意挂在 _onEnable（每次 enable 都跑、与配置构造方式无关）。c33a07daa 用 _cloneTo（只覆盖 clone）替换它，反序列化路径的 _module 不再有任何重链入口。
后果链（反序列化一个 Birth-only 配置 → enable → 运行时把某 slot 翻成 Death）：
set type(Death)→this._module?._generator._setTransformFeedback()→_module===null静默 no-op → TF 不建（_feedbackSimulator仍 null）- 下一帧父粒子死亡：
_retireActiveParticles里hasDeathSlot===true（WebGL2 下 enabled）→_feedbackSimulator===null跳过_readbackFeedback→_onParticleDeath仍跑，裸读this._feedbackReadback（null）→feedbackData[feedbackOffset]null-deref 崩溃 - 同理
set emitter静默跳过_validateEmitter→ 可后门拼出运行时环 → 栈溢出
为什么是 P2 不是 P1/P0：
- 当前不可达 —— 全仓 grep
subEmitters/SubEmitter/addSubEmitter在 loader 包零命中，sub-emitter 的 serialization schema 还没写，所以今天没有任何反序列化 slot 能产生（_module当前必由addSubEmitter/clone 赋值，永不为 null）。 - 上轮 P1 的崩溃没回退 ——
_onEnable._setTransformFeedback()（67fedab6b加、本轮保留）仍从裸type字段 reconcile TF，与_module无关，所以「反序列化 Death 配置 enable 即崩」这条已闭环路径不受影响。本条只影响 enable 之后的运行时 slot 突变。
但这是个会随 serialization 落地即激活的潜伏崩溃，且回退了一个有注释佐证的刻意修复，记此 P2。
修法（任选）：
- 首选：
_onEnable里恢复generator.subEmitters._bindSlots()（幂等、O(n)、与_setTransformFeedback()并列）——_cloneTo与_bindSlots不互斥，clone 走_cloneTo、deserialize+其它走_onEnable，两路都盖住，正是b98de15e3的原设计；或 - 让 setter 不依赖
_module：把_generator反向指针直接挂到SubEmitter（构造/反序列化时由模块回填），但那本质和_module同一个回填问题，不更省。
顺带（不单列）：新增的
Death slot reconciles transform-feedback on enable (deserialize path)测试名不副实——它先调addSubEmitter（已把_module设好）再手动 null 掉_useTransformFeedback/_feedbackSimulator，复现的是「TF 状态丢失」而非反序列化的真实特征「_module===null」。真正的反序列化 slot（_module恒 null）无任何测试覆盖。若按首选修法恢复_bindSlots，建议补一条 honest 测试：构造一个_module=null的 slot 推进_subEmitters，enable 后断言_module被重链、且slot.type=Death能驱动 TF。
已关闭问题清单（全部维持）
| 问题 | 状态 |
|---|---|
历史全部 P0/P1/🔴 + clamp 区间积分 + 反序列化 Death 崩溃 P1（b6dced39c 的 enabled 门 + _onEnable reconcile） |
✅ 全部闭环 |
上轮 P3 subEmitters slot 可变绕过环检测（.push() + sub.emitter） |
✅ 本轮前已关闭（623cc47d5 readonly 视图 + b98de15e3 setter 收口 _validateEmitter/_setTransformFeedback） |
subEmitters getter 单行→多行 JSDoc（上轮 P3 格式项） |
✅ 关闭（e18de0662/70772d059） |
长期遗留 P2（gravityMod slot 19 死 case / 测试裸 offset 42 / CPU-GPU clamp 分叉） |
维持，本轮未触碰 |
结论
4 个 commit：_generator @internal 化 + _refreshTransformFeedback 内联（正确简化）、doc 多行化、_module 重链挪到 _cloneTo。af6b4f913 与 doc 改动干净；c33a07daa 把 _module 重链从 _onEnable（覆盖 deserialize+clone）收窄到 _cloneTo（仅 clone），回退了 b98de15e3 注释明确覆盖的反序列化场景——当前因 serialization schema 未落地而不可达（P2、不阻塞），但会随 serialization 即成潜伏 null-deref，且「deserialize path」测试名不副实未覆盖真实 _module===null。建议恢复 _onEnable._bindSlots()（与 _cloneTo 并存）并补 honest 测试。本轮 --comment，无 P0/P1。reviewDecision 已 APPROVED，TS 编译干净、CI 全绿。
3 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 independentnormalizedAge = 0. Death inherit: parent value with COL/SOL/ROL applied at the actualnormalizedAgeat time of deathOut 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 applied to inherit), self-reference no-recurseparticleRenderer-sub-emittercase + baseline imageSummary by CodeRabbit
New Features
Tests