feat(particle): implement rateOverDistance emission#3011
Conversation
The `EmissionModule.rateOverDistance` field has been declared since the initial particle implementation but never consumed — the emitter only ran the rateOverTime path. This wires it up. Each frame the emitter samples its world position, accumulates the delta against the previous sample, and emits `ratePerUnit × distanceMoved` particles (Unity-aligned semantics). The sub-interval distance fragment is carried across frames so long, fine-grained pulls integrate correctly instead of getting truncated per frame. Two correctness details: - Use `Math.floor(accumulator / interval + zeroTolerance)` plus a single subtraction instead of a `while (acc >= interval)` subtract-loop, so `2.0 - 19 * 0.1` style float drift doesn't drop a particle near exact boundaries. - When the rate is `<= 0` or the baseline position is uninitialized, sync the position and drop the accumulator. A later rate flip from `0 → N` then starts from the current position rather than dumping every pre-flip frame of movement into a one-shot burst. `_reset` (called from `stop(StopEmittingAndClear)`) clears the baseline flag and accumulator so a play-after-clear re-syncs from the current emitter position. Unit test (5 cases) covers: default-zero no-emit, ratePerUnit × distance, sub-interval accumulation across frames, static-emitter no-emit, and stop+clear reset. e2e case `particleRenderer-rateOverDistance` sweeps a World-space emitter horizontally with rateOverTime=0 and rateOverDistance=2, leaving a deterministic particle trail.
|
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:
WalkthroughImplements distance-driven particle emission (rateOverDistance), integrates world-position overrides into the generator and stop handling, adds unit tests for accumulator and world-space distribution, and provides an E2E visual case plus a config entry. ChangesDistance-based particle emission feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/src/core/particle/RateOverDistance.test.ts`:
- Around line 1-8: The import block for Camera, Engine, Entity,
ParticleMaterial, ParticleRenderer, and ParticleStopMode is misformatted for
Prettier; replace the multi-line import with a single-line import (import {
Camera, Engine, Entity, ParticleMaterial, ParticleRenderer, ParticleStopMode }
from "`@galacean/engine-core`";) or run the project's Prettier formatter to
reformat that import so ESLint passes.
- Around line 13-23: The test mutates global performance.now inside tick
(function tick) in a way that increments times.value on every call (so multiple
reads during engine.update advance time too far) and never restores the original
(global polluted after suite); fix by saving the original performance.now before
overriding, replace the mock with a closure that increments times.value only
once per tick and returns the same value for subsequent calls during that tick
(so engine.update can call performance.now multiple times safely), and ensure
the original performance.now is restored (either at the end of tick or in
afterAll) so the global is not left mocked after tests run; refer to the tick
function and the afterAll teardown mentioned in the comment when making changes.
🪄 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: a14b5a49-e433-443a-aa7a-2950e42426de
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-rateOverDistance.jpgis excluded by!**/*.jpg
📒 Files selected for processing (4)
e2e/case/particleRenderer-rateOverDistance.tse2e/config.tspackages/core/src/particle/modules/EmissionModule.tstests/src/core/particle/RateOverDistance.test.ts
| import { | ||
| Camera, | ||
| Engine, | ||
| Entity, | ||
| ParticleMaterial, | ||
| ParticleRenderer, | ||
| ParticleStopMode | ||
| } from "@galacean/engine-core"; |
There was a problem hiding this comment.
Fix the import formatting to unblock lint.
This import block currently fails the Prettier rule reported by ESLint.
Suggested fix
-import {
- Camera,
- Engine,
- Entity,
- ParticleMaterial,
- ParticleRenderer,
- ParticleStopMode
-} from "`@galacean/engine-core`";
+import { Camera, Engine, Entity, ParticleMaterial, ParticleRenderer, ParticleStopMode } from "`@galacean/engine-core`";📝 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.
| import { | |
| Camera, | |
| Engine, | |
| Entity, | |
| ParticleMaterial, | |
| ParticleRenderer, | |
| ParticleStopMode | |
| } from "@galacean/engine-core"; | |
| import { Camera, Engine, Entity, ParticleMaterial, ParticleRenderer, ParticleStopMode } from "`@galacean/engine-core`"; |
🧰 Tools
🪛 ESLint
[error] 1-8: Replace ⏎··Camera,⏎··Engine,⏎··Entity,⏎··ParticleMaterial,⏎··ParticleRenderer,⏎··ParticleStopMode⏎ with ·Camera,·Engine,·Entity,·ParticleMaterial,·ParticleRenderer,·ParticleStopMode·
(prettier/prettier)
🤖 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/RateOverDistance.test.ts` around lines 1 - 8, The
import block for Camera, Engine, Entity, ParticleMaterial, ParticleRenderer, and
ParticleStopMode is misformatted for Prettier; replace the multi-line import
with a single-line import (import { Camera, Engine, Entity, ParticleMaterial,
ParticleRenderer, ParticleStopMode } from "`@galacean/engine-core`";) or run the
project's Prettier formatter to reformat that import so ESLint passes.
| function tick(engine: Engine, times: { value: number }, deltaMs: number = 100): void { | ||
| //@ts-ignore | ||
| engine._vSyncCount = Infinity; | ||
| //@ts-ignore | ||
| engine._time._lastSystemTime = 0; | ||
| performance.now = function () { | ||
| times.value += deltaMs; | ||
| return times.value; | ||
| }; | ||
| engine.update(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify global time mocking sites and restoration in this suite.
rg -nP --type=ts 'performance\.now\s*=' tests/src/core/particle/RateOverDistance.test.ts -C2
rg -nP --type=ts 'beforeAll\(|afterAll\(' tests/src/core/particle/RateOverDistance.test.ts -A8 -B2Repository: galacean/engine
Length of output: 972
Stabilize and restore the performance.now mock.
The mock at line 18 increments times.value on each call, causing engine.update() to advance more than deltaMs if performance.now() is read multiple times. It is never restored in afterAll (line 62), leaving the global polluted after the suite runs.
Suggested fix
+let originalPerformanceNow: typeof performance.now;
+
function tick(engine: Engine, times: { value: number }, deltaMs: number = 100): void {
//@ts-ignore
engine._vSyncCount = Infinity;
//@ts-ignore
engine._time._lastSystemTime = 0;
- performance.now = function () {
- times.value += deltaMs;
- return times.value;
- };
+ const next = times.value + deltaMs;
+ times.value = next;
+ performance.now = function () {
+ return next;
+ };
engine.update();
}
@@
beforeAll(async function () {
+ originalPerformanceNow = performance.now.bind(performance);
engine = await WebGLEngine.create({
canvas: document.createElement("canvas")
});
@@
afterAll(function () {
+ performance.now = originalPerformanceNow;
engine.destroy();
});📝 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.
| function tick(engine: Engine, times: { value: number }, deltaMs: number = 100): void { | |
| //@ts-ignore | |
| engine._vSyncCount = Infinity; | |
| //@ts-ignore | |
| engine._time._lastSystemTime = 0; | |
| performance.now = function () { | |
| times.value += deltaMs; | |
| return times.value; | |
| }; | |
| engine.update(); | |
| } | |
| function tick(engine: Engine, times: { value: number }, deltaMs: number = 100): void { | |
| //@ts-ignore | |
| engine._vSyncCount = Infinity; | |
| //@ts-ignore | |
| engine._time._lastSystemTime = 0; | |
| const next = times.value + deltaMs; | |
| times.value = next; | |
| performance.now = function () { | |
| return next; | |
| }; | |
| engine.update(); | |
| } |
🤖 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/RateOverDistance.test.ts` around lines 13 - 23, The
test mutates global performance.now inside tick (function tick) in a way that
increments times.value on every call (so multiple reads during engine.update
advance time too far) and never restores the original (global polluted after
suite); fix by saving the original performance.now before overriding, replace
the mock with a closure that increments times.value only once per tick and
returns the same value for subsequent calls during that tick (so engine.update
can call performance.now multiple times safely), and ensure the original
performance.now is restored (either at the end of tick or in afterAll) so the
global is not left mocked after tests run; refer to the tick function and the
afterAll teardown mentioned in the comment when making changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3011 +/- ##
===========================================
+ Coverage 78.02% 78.29% +0.27%
===========================================
Files 901 902 +1
Lines 99608 99853 +245
Branches 10295 10318 +23
===========================================
+ Hits 77718 78180 +462
+ Misses 21715 21499 -216
+ Partials 175 174 -1
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:
|
Replace the linear horizontal sweep with a Lissajous-style orbit (`OrbitScript`) and a sprite texture so the captured frame reads as a clear curving trail instead of a thin straight line. Bumps the deterministic sim window to 4 s (40 × 100 ms) so the orbit path is fully visible at the configured 2 s lifetime, and regenerates the baseline image to match. The TS-side `EmissionModule` change is unaffected — this is presentation only.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/case/particleRenderer-rateOverDistance.ts (1)
37-49: ⚡ Quick winAdd error handling for texture loading to improve test reliability.
If the texture fails to load, the promise rejection is unhandled, which could cause the test to timeout or fail with an unclear error message. Adding a
.catch()block would provide immediate feedback and simplify debugging.♻️ Proposed error handling
engine.resourceManager .load({ url: "https://mdn.alipayobjects.com/huamei_9ahbho/afts/img/A*OiP_RLwuFqAAAAAAQBAAAAgAegDwAQ/original", type: AssetType.Texture }) .then((texture) => { const particleEntity = createTrailParticle(engine, <Texture2D>texture); particleEntity.addComponent(OrbitScript); rootEntity.addChild(particleEntity); updateForE2E(engine, 100, 40); initScreenshot(engine, camera); + }) + .catch((error) => { + console.error("Failed to load particle texture:", error); + throw error; });🤖 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 `@e2e/case/particleRenderer-rateOverDistance.ts` around lines 37 - 49, The promise returned by engine.resourceManager.load is missing rejection handling, so add a .catch handler to the load(...) chain to log/report the error and fail the test gracefully; locate the engine.resourceManager.load call that passes the texture URL and type, and after the existing .then(...) attach .catch(err => { processLogger.error or console.error with context like "failed to load particle texture" and the err; ensure test teardown or a fail/throw is invoked so updateForE2E, initScreenshot, and createTrailParticle/OrbitScript are not called when the texture load fails }).
🤖 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.
Nitpick comments:
In `@e2e/case/particleRenderer-rateOverDistance.ts`:
- Around line 37-49: The promise returned by engine.resourceManager.load is
missing rejection handling, so add a .catch handler to the load(...) chain to
log/report the error and fail the test gracefully; locate the
engine.resourceManager.load call that passes the texture URL and type, and after
the existing .then(...) attach .catch(err => { processLogger.error or
console.error with context like "failed to load particle texture" and the err;
ensure test teardown or a fail/throw is invoked so updateForE2E, initScreenshot,
and createTrailParticle/OrbitScript are not called when the texture load fails
}).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 97545876-be1b-499b-b095-da25f1814f24
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-rateOverDistance.jpgis excluded by!**/*.jpg
📒 Files selected for processing (1)
e2e/case/particleRenderer-rateOverDistance.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/core/particle/RateOverDistance.test.ts (1)
171-205: 💤 Low valueHardcoded buffer layout constants are fragile.
The test uses magic numbers
stride = 42and offset27for world position. IfParticleBufferUtils.instanceVertexFloatStrideor the world position offset changes, this test will silently produce incorrect assertions rather than failing to compile.Consider importing the constants from
ParticleBufferUtilsif they are accessible, or add a comment referencing the source of these values to ease maintenance.🤖 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/RateOverDistance.test.ts` around lines 171 - 205, The test uses fragile magic numbers for per-instance stride and world-position offset (stride = 42, offset = 27); replace those hardcoded values by importing and using the buffer layout constants from ParticleBufferUtils (e.g., ParticleBufferUtils.instanceVertexFloatStride and the exported world-position offset constant) in the test so the loop reads verts[i * instanceVertexFloatStride + worldPositionOffset]; if the offset constant isn't exported, add a descriptive comment linking to ParticleBufferUtils and/or export the offset there and then reference it from this test to avoid silent breakage when the layout changes.
🤖 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.
Nitpick comments:
In `@tests/src/core/particle/RateOverDistance.test.ts`:
- Around line 171-205: The test uses fragile magic numbers for per-instance
stride and world-position offset (stride = 42, offset = 27); replace those
hardcoded values by importing and using the buffer layout constants from
ParticleBufferUtils (e.g., ParticleBufferUtils.instanceVertexFloatStride and the
exported world-position offset constant) in the test so the loop reads verts[i *
instanceVertexFloatStride + worldPositionOffset]; if the offset constant isn't
exported, add a descriptive comment linking to ParticleBufferUtils and/or export
the offset there and then reference it from this test to avoid silent breakage
when the layout changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 19e6a9c8-e0f4-4deb-912c-6c14f5192770
📒 Files selected for processing (3)
packages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/modules/EmissionModule.tstests/src/core/particle/RateOverDistance.test.ts
GuoLei1990
force-pushed
the
feat/particle-rate-over-distance
branch
from
560e1d1 to
ba92e73
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
force-pushed
the
feat/particle-rate-over-distance
branch
2 times, most recently
from
1ce2438 to
6a533ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/src/core/particle/RateOverDistance.test.ts (1)
14-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStabilize and restore the
performance.nowmock.At Lines 19-22,
tick()advancestimes.valueon every read ofperformance.now, so oneengine.update()can consume more thandeltaMsif it queries the clock multiple times. The mock is also never restored inafterAll(), which leaks global state into later suites.Also applies to: 63-65
🤖 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/RateOverDistance.test.ts` around lines 14 - 24, The tick helper advances times.value on every call to the mocked performance.now which lets a single engine.update() consume multiple deltaMs and leaks global state; change the mock in tick (function tick(engine: Engine, times: { value: number }, deltaMs = 100)) so it advances times.value only once per tick (e.g., capture a local called called=false and increment on the first call, returning the same value for subsequent calls during that tick) and ensure the original performance.now is saved before mocking and restored in an afterAll/afterEach cleanup (add restoration in the test file's afterAll/afterEach) so engine.update() behavior is stable and global state is not leaked.
🤖 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/EmissionModule.ts`:
- Around line 229-244: Particles emitted along the movement segment are all
using the same generator._playTime, so compute a per-particle emitTime from the
segment fraction t and pass it into generator._emit instead of playTime; inside
the World-space branch (where moveLength > MathUtil.zeroTolerance) after
computing t and before calling generator._emit, derive emitTime =
generator._playTime - (1 - t) * frameDelta (use the engine's frame delta field
on generator or its main object — e.g. generator._deltaTime or
generator.main.deltaTime) and call generator._emit(emitTime, 1, emitPos) so
earlier particles get older ages consistent with their position along the
segment.
---
Duplicate comments:
In `@tests/src/core/particle/RateOverDistance.test.ts`:
- Around line 14-24: The tick helper advances times.value on every call to the
mocked performance.now which lets a single engine.update() consume multiple
deltaMs and leaks global state; change the mock in tick (function tick(engine:
Engine, times: { value: number }, deltaMs = 100)) so it advances times.value
only once per tick (e.g., capture a local called called=false and increment on
the first call, returning the same value for subsequent calls during that tick)
and ensure the original performance.now is saved before mocking and restored in
an afterAll/afterEach cleanup (add restoration in the test file's
afterAll/afterEach) so engine.update() behavior is stable and global state is
not leaked.
🪄 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: 77fd1ef3-daba-4d26-9622-7c5a73ad80cc
📒 Files selected for processing (3)
packages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/modules/EmissionModule.tstests/src/core/particle/RateOverDistance.test.ts
GuoLei1990
force-pushed
the
feat/particle-rate-over-distance
branch
2 times, most recently
from
c4a0693 to
e18d9cf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
force-pushed
the
feat/particle-rate-over-distance
branch
3 times, most recently
from
224be15 to
c275c97
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
force-pushed
the
feat/particle-rate-over-distance
branch
from
c275c97 to
f4f536a
Compare
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.
…path The initial implementation burst all N emissions at the frame-end position, which collapses the trail into clumps at high speed / low rate / low fps — not aligned with Unity's per-particle subFrameOffset semantics. Spatially distribute the N emissions along [lastPos → currentPos] in World simulation space using accumulator-based parameter t. Local space keeps frame-end emit (particles are parented to the emitter anyway). Also clear the rateOverDistance baseline on stop(StopEmitting) so emitter movement during the stopped interval doesn't dump a one-shot burst on resume.
GuoLei1990
force-pushed
the
feat/particle-rate-over-distance
branch
from
f4f536a to
2e030a5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@hhhhkrx 顺着 CodeRabbit 指出的 emit time 那条想了一下,本地写了个补丁验证,贴出来给你做参考。 问题目前 在 对比 Unity 的 修法把 private _emitByRateOverDistance(lastPlayTime: number, playTime: number): void {
const ratePerUnit = this.rateOverDistance.evaluate(undefined, undefined);
const generator = this._generator;
if (ratePerUnit <= 0) {
this._invalidateDistanceBaseline();
return;
}
if (!this._hasLastEmitPosition) {
this._lastEmitPosition.copyFrom(generator._renderer.entity.transform.worldPosition);
this._hasLastEmitPosition = true;
return;
}
const lastPos = this._lastEmitPosition;
const currentPos = generator._renderer.entity.transform.worldPosition;
const dx = currentPos.x - lastPos.x;
const dy = currentPos.y - lastPos.y;
const dz = currentPos.z - lastPos.z;
const moveLength = Math.sqrt(dx * dx + dy * dy + dz * dz);
this._distanceAccumulator += moveLength;
const emitInterval = 1.0 / ratePerUnit;
// `+ zeroTolerance` absorbs float divide error so an exact `N*interval` accumulator doesn't drop 1
const count = Math.floor(this._distanceAccumulator / emitInterval + MathUtil.zeroTolerance);
if (count > 0) {
this._distanceAccumulator -= count * emitInterval;
if (generator.main.simulationSpace === ParticleSimulationSpace.World && moveLength > MathUtil.zeroTolerance) {
// Distribute N emissions along [lastPos → currentPos]. The same normalized
// back-distance `s ∈ [0, 1]` controls both spatial offset and emit-time offset:
// s = 0 → spawn at currentPos with emitTime = playTime (just born),
// s = 1 → spawn at lastPos with emitTime = lastPlayTime (born a frame ago).
// Without the time interpolation, all N particles would share `playTime`, so any
// age-driven module (COL / SOL / FOL) would render them as a uniform stamp
// instead of a smooth fade — the very high-speed case spatial distribution is
// meant to fix.
const invMoveLength = 1.0 / moveLength;
const sStep = emitInterval * invMoveLength;
const dt = playTime - lastPlayTime;
let s = this._distanceAccumulator * invMoveLength;
const emitPos = EmissionModule._tempEmitPosition;
for (let i = 0; i < count; i++) {
emitPos.set(currentPos.x - dx * s, currentPos.y - dy * s, currentPos.z - dz * s);
generator._emit(playTime - dt * s, 1, emitPos);
s += sStep;
}
} else {
generator._emit(playTime, count);
}
}
lastPos.copyFrom(currentPos);
}调用处: _emit(lastPlayTime: number, playTime: number): void {
this._emitByRateOverTime(playTime);
this._emitByRateOverDistance(lastPlayTime, playTime);
this._emitByBurst(lastPlayTime, playTime);
}配套测试在 const times: number[] = [];
for (let i = 0; i < 4; i++) {
times.push(verts[i * stride + 7]); // ParticleBufferUtils.timeOffset
}
times.sort((a, b) => a - b);
expect(times[3] - times[0]).to.be.greaterThan(0);
expect(times[2] - times[1]).to.be.greaterThan(0);取舍
本地 84/84 测试过。 |
|
|
||
| const emitInterval = 1.0 / ratePerUnit; | ||
| // `+ zeroTolerance` absorbs float divide error so an exact `N*interval` accumulator doesn't drop 1 | ||
| const count = Math.floor(this._distanceAccumulator / emitInterval + MathUtil.zeroTolerance); |
There was a problem hiding this comment.
count 没有上限。分布循环会调用 count 次 _emit,而 _emit 内部对 maxParticles 的提前 return 只保护了「粒子数」,没保护「循环次数」。
如果播放过程中用脚本把 emitter 瞬移到远处(场景切换、跟随快速物体、respawn 等并不罕见),moveLength 无界 → count 可达百万级,缓冲填满后每次 _emit 仍要做一次函数调用 + _getNotRetiredParticleCount() + 比较,造成空转的 CPU 尖刺。_emitByRateOverTime 也是循环,但时间增量受帧时间天然有界;距离增量在瞬移时无界——这是区别。
注意:只把 count clamp 到 maxParticles 不够——_distanceAccumulator -= count * emitInterval 之后仍会残留巨大的 accumulator,下一帧继续 burst,尖刺只是被推迟。瞬移这种情况应同时把残留 accumulator 丢弃(置 0 或只保留 % emitInterval 的小数部分),因为单帧发射超过 maxParticles 的粒子本就没有意义。
The previous implementation distributed N emissions spatially along [lastPos → currentPos] but stamped all of them with the frame-end `generator._playTime`. In shader, `age = renderer_CurrentTime - a_DirectionTime.w`, so every same-frame particle ages in lockstep — age-driven modules (COL / SOL / FOL) then render the trail as a uniform block instead of a smooth fade, exactly in the high-speed case the spatial fix targets. Unify position and time under a single normalized back-distance `s ∈ [0, 1]` (0 → currentPos / playTime; 1 → lastPos / lastPlayTime). For each emission, `emitPos = currentPos − s · Δpos` and `emitTime = playTime − s · dt`, both stepping by `sStep = emitInterval / moveLength` per particle. Same loop, both offsets, one variable. Local simulation space keeps the lump-sum `generator._emit(playTime, count)` path — particles are parented to the emitter there, so sub-frame time/position interpolation has no visible effect. Test (8 cases total) covers per-particle emit time spacing by reading `a_DirectionTime.w` from the instance buffer and asserting the four emit times form an arithmetic sequence with non-zero step.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…me fix Trail rendering changes once emit time is interpolated along the movement path — at high speed / low fps the COL fade is now a smooth gradient instead of a uniform stamp, so the prior baseline (captured before the time-interpolation fix) no longer matches.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…loop _emit was the only emission entry point but only guarded `notRetire >= maxParticles` at the door — once inside, the count-sized loop kept calling _addNewParticle even after the buffer filled. A setPosition jump on a rateOverDistance emitter can push count into the millions, turning each frame into millions of no-op iterations. Move the budget clamp into _emit (count = min(count, maxParticles - alive)) and return the actual emitted count. All entry points (rateOverTime / burst / public emit() / rateOverDistance) now share the same protection. The rateOverDistance loop reads the return value: hitting 0 means the buffer just saturated, so settle the frame's distance budget instead of carrying it over. While unifying the loop, fold the moveLength ≈ 0 fallback into the main path: invMoveLength = 0 collapses subFrameAge to 0 (frame-end emit), the initial Math.min handles the `accumulator / moveLength > 1` edge a tiny move-across-an- emitInterval can produce. Local simulation space rides the same loop — the position override is ignored but the per-particle emitTime gives COL/SOL/FOL fades a smooth gradient instead of a uniform block.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Regression guard for the budget clamp landed in cb7dc26: a single setPosition jump shouldn't expand into millions of no-op _emit iterations or a multi-frame burst. Simulates a 10000-unit teleport with rateOverDistance=10 (would otherwise demand 100,000 emissions in one frame), asserts alive count stays bounded by main.maxParticles, and verifies the accumulator is drained — the next idle frame emits nothing instead of dripping out the residual budget.
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.
| let subFrameAge = Math.min(this._distanceAccumulator * invMoveLength, 1.0); | ||
| const emitPos = EmissionModule._tempEmitPosition; | ||
| for (let i = 0; i < count; i++) { | ||
| if (isWorld) { | ||
| emitPos.set( | ||
| currentPos.x - dx * subFrameAge, | ||
| currentPos.y - dy * subFrameAge, | ||
| currentPos.z - dz * subFrameAge | ||
| ); | ||
| } | ||
| if (generator._emit(playTime - dt * subFrameAge, 1, isWorld ? emitPos : undefined) === 0) { | ||
| // Buffer full: settle the frame's distance budget instead of carrying it over | ||
| this._distanceAccumulator = 0; | ||
| break; | ||
| } | ||
| subFrameAge += ageStep; |
There was a problem hiding this comment.
subFrameAge 的钳制只作用于初值(i=0),循环里 subFrameAge += ageStep 不再钳制——一旦初值被钳到 1.0,i≥1 全部 subFrameAge > 1。
subFrameAge > 1 表示该粒子属于更早的帧,在本帧区间 [lastPos → currentPos] 上没有合法落点:emitPos 会外推到 lastPos 之前,emitTime = playTime - dt*subFrameAge < lastPlayTime。
触发条件:rateOverDistance 在两帧之间被调高 → emitInterval 变小,而之前以低正速率积累的 previousCarry 超过了新的 emitInterval,此时 residual/moveLength > 1,初值钳制生效,后续粒子溢出。
后果:调高速率的那一帧喷出一小撮粒子,World 空间下位置外推到 lastPos 后方(可达 ~段长×count),发射时间回拨到过去多帧;emitTime 回拨量若小于 startLifetime,粒子会以错误的 normalizedAge 相位渲染(SOL/COL/FOL 取错值),否则被着色器按 normalizedAge > 1 discard 但仍白占一个发射名额。低频边界 bug,但确实存在。
建议:把循环末尾改为单调钳制,与已有的初值 Math.min 一致:
subFrameAge = Math.min(subFrameAge + ageStep, 1.0);正常路径 subFrameAge < 1,Math.min 为 no-op,零代价;只有边界场景才把溢出粒子收敛到 lastPos/lastPlayTime。count 不变,不影响 Unity 语义。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Cherry-picked from #3011 (e2e parts dropped). - Wire up EmissionModule.rateOverDistance: each frame accumulates the delta of the emitter's world position and emits ratePerUnit × distance particles (Unity-aligned). - Sub-interval distance fragment carried across frames; floor-based count instead of subtract-loop to avoid float drift dropping a particle at exact boundaries. - Distribute the N per-frame emissions spatially along [lastPos → currentPos] in World simulation space, and interpolate emit time the same way so age-driven modules (COL/SOL/FOL) render a smooth gradient instead of a uniform block. - Clamp _emit at the maxParticles budget and return the actual count, so a setPosition teleport on a rateOverDistance emitter can't expand into millions of no-op iterations. - Reset baseline + accumulator on stop(StopEmitting*) so a play-after- clear re-syncs from the current position. Includes unit tests covering zero-rate, ratePerUnit × distance, sub-interval accumulation across frames, static emitter, stop+clear reset, spatial distribution, per-particle emit-time spacing, and teleport-induced burst guard.
The initial `Math.min(accumulator * invMoveLength, 1.0)` only guards i=0. Once `+= ageStep` runs, subsequent iterations re-cross 1.0 and emitPos extrapolates past lastPos (in the opposite direction of motion) while emitTime rewinds before lastPlayTime. Trigger: rateOverDistance is increased between two frames. The previous-frame accumulator carry (legal under the old, larger emitInterval) now exceeds the new emitInterval, so `count` pays out multiple particles in a frame whose own moveLength is only ~one interval wide. `residual / moveLength > 1` → the clamp engages at i=0 (correctly settling at lastPos), but i≥1 escapes. Mirror the clamp inside the loop: `subFrameAge = Math.min(subFrameAge + ageStep, 1.0)`. On the normal path (subFrameAge stays well under 1) this is a no-op; on the rate-hike edge the overflowed particles collapse to lastPos / lastPlayTime — a physically legal degenerate spot instead of an unreachable one. Count and Unity semantics are preserved. Regression test reproduces the scenario (rate 0.5 → 5 mid-frame, seven emissions, all expected within [lastPos.x, currentPos.x]). Verified by temporarily rolling the clamp back and confirming the test catches i=1 at x = 1.30 (≈ 0.2 units behind lastPos).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const dt = playTime - lastPlayTime; | ||
| let subFrameAge = Math.min(this._distanceAccumulator * invMoveLength, 1.0); | ||
| const emitPos = EmissionModule._tempEmitPosition; | ||
| for (let i = 0; i < count; i++) { |
There was a problem hiding this comment.
把这个声明提到循环外
const {x: cx , y: cy, z: cz} = currentPos;
| /** | ||
| * @internal | ||
| */ | ||
| _invalidateDistanceBaseline(): void { |
There was a problem hiding this comment.
_invalidateDistanceBaseline 的调用时机有两处遗漏,会产生实际可见的视觉异常,本地已复现并修通。
不变量:任何「不调 _emit 的连续帧」结束后,resume 时第一帧的位移不该被计入距离。当前 PR 只在显式 stop() 的两条分支里失效 baseline,漏了两个同样会跳过 _emit 的路径。
Defect A:emission.enabled = false → true 后幻影 burst
enabled setter 只切 shader macro,没碰 baseline。_update 在 enabled=false 时跳过 _emit → _lastEmitPosition 冻结。「关 emission → 移动 emitter → 开 emission」就会在重开那一帧 diff 出 rate × 关闭期间总位移 颗粒子。
典型场景:角色技能蓄力/隐身/进出某状态时临时停发粒子;LOD/性能优化 emitter 出屏时关粒子;编辑器面板 toggle 预览;多个粒子组件交替显示。
Defect B:非循环系统自动停 + play() 重播后幻影 burst
ParticleGenerator._update 里 !isLoop && _playTime > duration 时直接 _isPlaying = false,不走 stop(),所以不失效 baseline。之后直接 play()(没先 stop(StopEmittingAndClear))→ 那一帧爆出 rate × 自动停到重播之间的位移 颗粒子。
典型场景:对象池复用一次性粒子(子弹命中、爆炸、技能 hit、跳跃落地烟尘等),池子直接 play() 不 stop+clear 是非常常见的写法;挂在移动物体上的一次性特效。
视觉表现
两个 defect 的症状一样:那一帧 World 空间下沿「跳过 _emit 期间的轨迹」一字排开一截突然出现的拖尾;Local 空间下糊一团在 emitter 当前位置。前提是用了 rateOverDistance > 0,只用 rateOverTime/burst 无关。
复现 + 验证
在 worktree 上写两个最小用例(rate=10、移动 3 单位)跑在当前 PR head 1efb7dca2:
× (A) emission.enabled false→true → expected 0, got 30
× (B) non-loop auto-stop + play() → expected 0, got 30
精确爆出 30 颗,与 rate × moveLength 完全吻合。
修法(6 行,本地验证有效且现有 11 个 RateOverDistance 用例无回归)
EmissionModule.ts:
override set enabled(value: boolean) {
if (value !== this._enabled) {
if (value) {
// 重新启用前 emitter 可能动过,丢弃过时基准
this._invalidateDistanceBaseline();
}
this._enabled = value;
...
}
}ParticleGenerator.ts play() 单粒子分支末尾:
this._playStartDelay = this.main.startDelay.evaluate(...);
// 覆盖 auto-stop 后 replay 以及其它不经 stop() 翻转 _isPlaying 的情况
this.emission._invalidateDistanceBaseline();更彻底的方向:把失效统一放在「重新开始发射」入口(play() + enabled setter),stop() 现有的两次失效就变成冗余但无害的防御——可保留也可删,看取舍。
There was a problem hiding this comment.
追加发现:同款 bug 在 _emitByRateOverTime 路径上预存在(本 PR 之前就有),原因同根。
_frameRateTime 也只在 _reset(被 stop(StopEmittingAndClear) 调)时归零,play() 入口不重置;而 _update 里 _playTime += deltaTime 没有 _isPlaying 门(回收逻辑需要)。所以非循环自动停后,_playTime 继续涨,_frameRateTime 卡在停时位置——一旦不经 stop(Clear) 直接 play() 重播,_emitByRateOverTime 的 cumulativeTime = playTime - _frameRateTime 会展成整个停摆区间,一帧内 catch-up 补发 rate × gap 颗粒子。同理 rate 切 0 又切回 N(_frameRateTime 也不动)。
在 base 分支 dev/2.0 上验过 + worktree 上写了可视化 example:
- 单测:
rateOverTime=10/sec,非循环,自动停后等 3 秒play()→ 一帧爆 100 颗(预期 <5)。 - Example(
examples/src/particle-replay-burst-bug.ts):两个 emitter 设置完全相同,左边stop(Clear) + play()标准重播,右边只调play()。每 3 秒触发一次,右边明显一帧噗一团 ~15 颗,左边平滑。
这个 bug 影响面比新引入的 rateOverDistance Defect A/B 更广——rateOverTime 是默认且最常用的发射方式,对象池+一次性特效的典型用法都会撞。只是视觉表现是「重播多了几颗粒子」,用户大多忍了或养成 stop(Clear)+play() 的习惯绕过去。
所以「单一真理」的修法可以一次到位:把所有「下一颗粒子从哪开始」的游标(_frameRateTime、_currentBurstIndex、_lastEmitPosition/baseline)统一搬到 entry 层(play() + enabled setter false→true),_reset 和 _invalidateDistanceBaseline 都删,stop() 出口不再失效。_emitByRateOverTime 在 rate≤0 时也同步 _frameRateTime = playTime 防回升时催更(和 rateOverDistance 现有处理对称)。
净代码量减少 3 行,三条发射路径行为一致,没有任何冗余。
Scope 取舍:rateOverTime 这条是预存在 bug,严格说不属于本 PR 范围。两种合理选择:
- 本 PR 走窄修(只动 rateOverDistance + emission.enabled 那 6 行),rateOverTime 的预存在 bug 开 follow-up issue/PR 单独修。审起来轻、blast radius 小,推荐如果 PR 想快合。
- 本 PR 顺手宽修一次到位,把三条路径的 entry-resync 统一。架构干净,但 PR 描述需要明确说「也修了 rateOverTime 的预存在 catch-up bug」,reviewer 心智压力大一点。
我倾向 1(窄修先合 + follow-up)。把 rateOverTime 这个长期 bug 拿出来单独 review、单独写 regression 测试,合规性和可追溯性都更好;rateOverDistance 这个新功能也不被拖。但如果你倾向一次性收干净,2 也完全成立。
Move all "next emission" cursors (_frameRateTime, _currentBurstIndex, _lastEmitPosition / _distanceAccumulator) to a single entry-side resync called from play() and the emission.enabled false→true transition. The old layout invalidated them on the stop side, which only covered the explicit stop() paths and missed every other gap where _emit was skipped while _playTime kept advancing. Three concrete bugs this resolves: 1. rateOverDistance — emission.enabled toggled off, emitter moves, toggled back on: bursts rate × moved-distance on the resume frame. New in this PR (rateOverDistance is wired up here for the first time). 2. rateOverDistance — non-loop generator auto-stops (`!isLoop && _playTime > duration` flips _isPlaying directly without calling stop()), emitter moves, replay via bare play(): same burst. New in this PR. 3. rateOverTime — non-loop auto-stop + bare play() replay: catch-up burst spanning the whole stopped interval (_update advances _playTime every frame regardless of _isPlaying because the retire path needs it, but _frameRateTime sat still). Pre-existing since the original particle implementation; verified at rate=10/s with a 3 s gap dumping ~100 particles into one frame. emission.enabled toggle and rate 0→N transitions had the same shape. Implementation: - EmissionModule._resyncCursors(playTime): single helper called from play() and the enabled setter (true branch). Replaces the deleted _reset() and _invalidateDistanceBaseline() methods. - _emitByRateOverTime now syncs _frameRateTime = playTime when rate ≤ 0, mirroring the existing rate-transition guard in _emitByRateOverDistance. - ParticleGenerator.stop() no longer touches emission cursors — entry side owns it. The buffer clear and _playTime = 0 stay in stop(Clear). - _emitByRateOverDistance: hoist currentPos.x/y/z to locals before the per-particle loop (transform.worldPosition is a live reference; the JIT can't prove the loop body doesn't alias it). Net code: 113 insertions, 41 deletions across the two source files; three deleted "@internal" helpers (_reset, _invalidateDistanceBaseline, the stop-side else branch), one added helper, one centralized invariant. Tests: - RateOverDistance.test.ts: +2 cases (emission.enabled toggle, non-loop auto-stop replay) on top of the existing 11. - RateOverTimeReplay.test.ts: new file, 3 cases (auto-stop replay, rate 0→N transition, enabled toggle) covering the pre-existing rateOverTime catch-up bug. - Full particle suite: 93 tests / 10 files pass, no regressions.
cptbtptpbcptdtptp
force-pushed
the
feat/particle-rate-over-distance
branch
from
0e4bea6 to
1520610
Compare
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
本轮新增了 1 个 commit（1520610）：entry-side resync 统一重构。cptbtptpbcptdtptp 在 03:17Z 和 04:03Z 指出的三个问题（Defect A、Defect B、以及 rateOverTime 的同根预存在 bug）全部正确落地。
_reset() + _invalidateDistanceBaseline() 合并为 _resyncCursors(playTime)，调用点统一移到 play() 和 enabled setter 的进入侧，stop() 出口不再碰 cursors。_emitByRateOverTime 补上 rate≤0 时 _frameRateTime = playTime 的对称守卫。代码量净减，三条发射路径行为一致，无冗余。实现干净，与 cptbtptpbcptdtptp 建议的"单一真理"方向完全吻合。宽修（方向 2）是对的：这个 rateOverTime 预存在 bug 影响面远比 rateOverDistance 大，放在同一 PR 里一次性收干净比 follow-up 更好。
已关闭问题
EmissionModule.ts逻辑分支冗余 — 已修复 ✓StopEmitting时 burst — 已修复 ✓- World 空间
playTime未回填 — 已修复 ✓ count无上限 / teleport CPU 尖刺 — 已修复 ✓subFrameAge循环末尾溢出 — 已修复 ✓- Defect A:
emission.enabledfalse→true 幻影 burst — 本轮修复 ✓ - Defect B: 非循环自动停 +
play()幻影 burst — 本轮修复 ✓ rateOverTimecatch-up burst 预存在 bug — 本轮修复 ✓（宽修，含 rate 0→N 和 enabled toggle 两条变体）const {x: cx, y: cy, z: cz}循环外提升 — 本轮修复 ✓
问题
[P2] RateOverDistance.test.ts 和 RateOverTimeReplay.test.ts — performance.now 覆盖未恢复（第十三轮；新文件延续同一写法）
两个测试文件的 tick() 每次调用都用 closure 替换全局 performance.now（且每次读取都推进 times.value），afterAll 均只有 engine.destroy()。新文件 RateOverTimeReplay.test.ts 是全新引入，完全可以直接用正确写法。建议两个文件统一：
let origPerformanceNow: () => number;
beforeAll(async function () {
origPerformanceNow = performance.now.bind(performance);
// ...
});
afterAll(function () {
performance.now = origPerformanceNow;
engine.destroy();
});tick() 改为入口一次性固定值：
function tick(engine: Engine, times: { value: number }, deltaMs: number = 100): void {
engine._vSyncCount = Infinity;
engine._time._lastSystemTime = 0;
const next = times.value + deltaMs;
times.value = next;
performance.now = function () { return next; };
engine.update();
}[P2] RateOverDistance.test.ts:192, 225, 308 — 魔法数字（第十二轮）
stride = 42、+ 27、timeFloatOffset = 7 仍为手算值，rate-hike-clamp 测试（约第 308 行）延续同一写法。layout 改变时断言静默失败。建议：
const stride = ParticleBufferUtils.instanceVertexFloatStride; // 42
const worldPosFloatOffset = 108 / 4; // SimulationWorldPosition byte offset = 108
const timeFloatOffset = 16 / 4 + 3; // a_DirectionTime byte offset = 16, .w = +3[P2] RateOverDistance.test.ts:276 — emit-returns-actual 直接戳 _emit 合约（第六轮）
buffer 满时返回 0 的外部可观察效果已由 teleport-clamp 和 no-burst-on-replay 链路测试充分覆盖。直接戳 generator._emit（@internal）把内部返回值契约锁死在测试里。建议删除该用例。
简化建议
_resyncCursors 单一入口替代两个分散的失效方法，净减代码量，三条路径行为统一，是本 PR 最终版最干净的改动。三个 P2 可合入后改进。
3 participants
Summary
EmissionModule.rateOverDistancehas been declared since the initial particle implementation but never consumed — only the time-based emission path ran. This wires it up.Each frame the emitter samples its world position, accumulates the delta against the previous sample, and emits
ratePerUnit × distanceMovedparticles (Unity-aligned semantics). Sub-interval distance is carried across frames so long, fine-grained pulls integrate correctly.Implementation notes
Math.floor(accumulator / interval + zeroTolerance)+ single subtraction, not awhile (acc >= interval)loop.2.0 - 19 * 0.1style drift would otherwise drop a particle at exact boundaries (the unit test caught this).<= 0or the baseline position is uninitialized, sync the position and drop the accumulator. A later0 → Nrate flip starts fresh from the current position instead of dumping pre-flip frames of movement into a burst._resetclears state: stop(StopEmittingAndClear) → play re-syncs from the current emitter position.Test plan
RateOverDistance.test.ts(5 cases): default-zero, ratePerUnit×distance, sub-interval accumulation, static-emitter no-emit, stop+clear resetparticleRenderer-rateOverDistance: World-space emitter sweeps horizontally withrateOverTime=0andrateOverDistance=2, leaving a deterministic particle trail; baseline capturedSummary by CodeRabbit
New Features
Bug Fixes
Tests