particle support limit velocity over lifetime#2925
Merged
cptbtptpbcptdtptp merged 68 commits intogalacean:dev/2.0from Mar 18, 2026
Merged
particle support limit velocity over lifetime#2925cptbtptpbcptdtptp merged 68 commits intogalacean:dev/2.0from
cptbtptpbcptdtptp merged 68 commits intogalacean:dev/2.0from
Conversation
|
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:
WalkthroughAdds GPU transform‑feedback support and a new LimitVelocityOverLifetime particle module, integrates TF into particle generator/renderer, adds WebGL2 TF platform/primitive implementations, shader/tooling updates, buffer copy APIs, extensive clone/remap refactor, plus unit and e2e tests and version bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant Renderer as ParticleRenderer
participant Generator as ParticleGenerator
participant TFSim as TFSimulator
participant Shader as ShaderProgram
participant GL as WebGLContext
App->>Renderer: update(deltaTime)
Renderer->>Generator: _update(shaderData, deltaTime)
alt transformFeedback enabled
Generator->>TFSim: update(shaderData, particleCount, firstActive, firstFree, deltaTime)
TFSim->>Shader: compile/retrieve(program with feedback varyings)
TFSim->>GL: bind buffers & transformFeedback
GL->>TFSim: beginTransformFeedback(primitive)
TFSim->>GL: drawArrays(firstActive, count)
GL-->>TFSim: capture feedback (position, velocity)
GL->>TFSim: endTransformFeedback()
TFSim->>TFSim: swap read/write buffers
TFSim-->>Generator: provide readBinding
end
Renderer->>GL: render using instance buffers (may use TF read binding)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2925 +/- ##
===========================================
+ Coverage 77.18% 77.20% +0.01%
===========================================
Files 884 896 +12
Lines 96609 98245 +1636
Branches 9520 9667 +147
===========================================
+ Hits 74565 75846 +1281
- Misses 21880 22233 +353
- Partials 164 166 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 27-65: The test never uses the imported E2E helpers so the
screenshot harness may capture before particles/textures finish; at the end of
createScalarLimitParticle add calls to updateForE2E(engine, 500) and
initScreenshot(engine, camera) so the engine has time to settle and the camera
frame is registered before the reference screenshot is taken — locate the calls
inside the async particle setup sequence (referencing createScalarLimitParticle,
engine and camera) and append updateForE2E and initScreenshot accordingly.
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 254-269: _isRandomMode currently only considers
_limitX/_limitY/_limitZ and thus returns false when drag uses
TwoConstants/TwoCurves, causing ParticleGenerator to skip per-particle randoms
(Random2.w) and produce deterministic drag; update _isRandomMode (and the
equivalent checks in the ranges 274-323 and 427-448) to include the module's
drag random modes as well (i.e., check the drag-related curve/mode fields used
by _uploadDrag for TwoConstants and TwoCurves) so the module-wide "needs
per-particle random" decision returns true whenever drag is in a random mode and
ParticleGenerator will allocate Random2.w accordingly. Ensure you reference and
update the same logic in the _isRandomMode helper(s) and harmonize it with
_uploadDrag's accepted modes so behavior is consistent.
- Around line 360-425: The function _uploadSeparateAxisLimits currently coerces
mixed per-axis modes to the constant path; update it to handle each axis
individually by checking limitX.mode, limitY.mode, limitZ.mode separately and
uploading the correct representation per axis: use
LimitVelocityOverLifetimeModule._limitXMaxCurveProperty/_limitXMinCurveProperty
(and Y/Z equivalents) when an axis is Curve/TwoCurves, and write the
corresponding component into
LimitVelocityOverLifetimeModule._limitMaxConstVecProperty/_limitMinConstVecProperty
for Constant/TwoConstants axes; set per-axis random flags and mode macros
(introduce or reuse per-axis macros if needed instead of the single
_limitCurveModeMacro/_limitConstantModeMacro) so the shader can distinguish
which axes are curves vs constants, and ensure _uploadSeparateAxisLimits returns
the appropriate macros (modeMacro/randomMacro) reflecting per-axis state rather
than forcing all-constant fallback.
In `@tests/src/core/particle/LimitVelocityOverLifetime.test.ts`:
- Around line 216-239: The tests override the global performance.now in multiple
places (e.g., inside the "enabling module triggers shader update without error"
case that manipulates engine._vSyncCount and engine._time._lastSystemTime and
calls engine.update()), but never restore it; capture the original
performance.now before replacing it and ensure you restore it in a finally block
or an afterEach hook so the global clock is returned to its original function
after each smoke test; update all occurrences that set performance.now
(including the other similar test blocks) to follow this pattern so later specs
are not affected by the mocked clock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e9b43c5e-3212-4921-8132-1ec2c353b844
⛔ Files ignored due to path filters (3)
packages/core/src/shaderlib/extra/particle.vs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/force_over_lifetime_module.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/limit_velocity_over_lifetime_module.glslis excluded by!**/*.glsl
📒 Files selected for processing (8)
e2e/case/particleRenderer-limitVelocity.tse2e/config.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/enums/ParticleRandomSubSeeds.tspackages/core/src/particle/index.tspackages/core/src/particle/modules/LimitVelocityOverLifetimeModule.tspackages/core/src/shaderlib/particle/index.tstests/src/core/particle/LimitVelocityOverLifetime.test.ts
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+216
to
+239
| it("enabling module triggers shader update without error", function () { | ||
| const lvl = particleRenderer.generator.limitVelocityOverLifetime; | ||
| lvl.enabled = true; | ||
| lvl.limit = new ParticleCompositeCurve(5); | ||
| lvl.dampen = 0.8; | ||
| lvl.drag = new ParticleCompositeCurve(0.5); | ||
|
|
||
| // Should not throw when updating shader data | ||
| particleRenderer.generator.play(); | ||
| expect(() => { | ||
| //@ts-ignore | ||
| engine._vSyncCount = Infinity; | ||
| //@ts-ignore | ||
| engine._time._lastSystemTime = 0; | ||
| let times = 0; | ||
| performance.now = function () { | ||
| times++; | ||
| return times * 100; | ||
| }; | ||
| for (let i = 0; i < 10; ++i) { | ||
| engine.update(); | ||
| } | ||
| }).to.not.throw(); | ||
| }); |
There was a problem hiding this comment.
Restore performance.now after these smoke tests.
Line 231, Line 257, and Line 282 overwrite the global clock and never restore it. That leaks into later specs and makes the browser suite order-dependent. Save the original function and put it back in finally or afterEach.
Suggested pattern
+ const originalNow = performance.now;
expect(() => {
- performance.now = function () {
- times++;
- return times * 100;
- };
- for (let i = 0; i < 10; ++i) {
- engine.update();
- }
+ try {
+ performance.now = function () {
+ times++;
+ return times * 100;
+ };
+ for (let i = 0; i < 10; ++i) {
+ engine.update();
+ }
+ } finally {
+ performance.now = originalNow;
+ }
}).to.not.throw();Also applies to: 241-265, 267-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/core/particle/LimitVelocityOverLifetime.test.ts` around lines 216 -
239, The tests override the global performance.now in multiple places (e.g.,
inside the "enabling module triggers shader update without error" case that
manipulates engine._vSyncCount and engine._time._lastSystemTime and calls
engine.update()), but never restore it; capture the original performance.now
before replacing it and ensure you restore it in a finally block or an afterEach
hook so the global clock is returned to its original function after each smoke
test; update all occurrences that set performance.now (including the other
similar test blocks) to follow this pattern so later specs are not affected by
the mocked clock.
GuoLei1990
force-pushed
the
feat/limitParticle
branch
from
272bf29 to
2193eb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
e2e/case/particleRenderer-limitVelocity.ts (1)
27-27:⚠️ Potential issue | 🟡 MinorHook this case into the screenshot harness.
initScreenshotandupdateForE2Eare still unused, so the capture can happen before the texture load and first burst settle. Please callupdateForE2E(engine, 500)andinitScreenshot(engine, camera)after the particle setup completes, even if that means threadingcameraintocreateScalarLimitParticle().Also applies to: 63-65, 68-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/case/particleRenderer-limitVelocity.ts` at line 27, After the particle setup completes, hook the test into the screenshot harness by calling updateForE2E(engine, 500) and then initScreenshot(engine, camera); if camera isn't available where you build the particle case, thread the camera through (e.g. add a camera parameter to createScalarLimitParticle or return the camera from the setup) so you can call initScreenshot(engine, camera) immediately after setup; apply the same change to the other similar cases referenced (the blocks that import initScreenshot/updateForE2E).packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (2)
256-270:⚠️ Potential issue | 🟠 MajorTreat randomized drag as a random-mode configuration.
_uploadDrag()supportsTwoConstantsandTwoCurves, but_isRandomMode()still only checks the limit curves. In separate-axes mode it also requires every axis to be randomized. The module-level random check should flip on whenever any limit axis or drag uses a randomized mode; otherwise the generator skips the per-particle random this shader path needs and drag becomes deterministic.Also applies to: 429-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around lines 256 - 270, The _isRandomMode() function currently only inspects _limitX/_limitY/_limitZ modes (and requires all axes randomized when _separateAxes is true), but it must also treat randomized drag as random-mode so the per-particle random shader path is enabled; update _isRandomMode() to return true if any of _limitX/_limitY/_limitZ is in ParticleCurveMode.TwoConstants or TwoCurves OR if the drag configuration inspected by _uploadDrag() is in TwoConstants/TwoCurves (and when _separateAxes is true, consider each axis ORed rather than requiring all three), ensuring the method references the same drag mode checks used by _uploadDrag() so randomized drag flips the random-mode flag; apply the same fix to the other _isRandomMode() copy around the 429-448 area.
362-424:⚠️ Potential issue | 🟠 MajorDon't silently fall back to constants for mixed per-axis modes.
Anything other than “all curves” or “all constants” drops into the constant upload branch. A setup like
limitX = Curve,limitY = Constant,limitZ = TwoConstantsloses the authored curve/random data instead of matching the per-axis configuration. Either reject mixed setups or upload each axis independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around lines 362 - 424, In _uploadSeparateAxisLimits detect and handle each axis mode individually instead of falling back to the “all curves” vs “all constants” branches: inspect limitX.limitY.limitZ modes and for each axis upload the appropriate data (for curves use _limit{X|Y|Z}MaxCurveProperty and _limit{X|Y|Z}MinCurveProperty when TwoCurves, for constants use _limitMaxConstVecProperty/_limitMinConstVecProperty but only set the corresponding component), set modeMacro to reflect per-axis curve/constant usage (or compute a composite shader macro logic) and set randomMacro only if any axis is random (TwoCurves/TwoConstants) while preserving each axis’s authored values; modify _uploadSeparateAxisLimits, the uses of LimitVelocityOverLifetimeModule._limitXMaxCurveProperty/_limitXMinCurveProperty/_limitY.../_limitZ... and the constant vector packing logic to write per-component values rather than globally switching branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts`:
- Around line 89-105: updateVAOs currently skips rebuilding VAOs when only
program.id matches _lastProgramId, causing stale VAO bindings if the underlying
buffers are reallocated; update the cache key logic in updateVAOs (and related
fields) to include the current transform buffers (this._readBuffer and
this._writeBuffer) so the VAOs are recreated when either buffer changes: compare
buffer identity (e.g., buffer.id or the buffer object references) against stored
lastReadBuffer/lastWriteBuffer (or store a composite _lastKey) and call
_deleteVAOs()/_createVAO(...) whenever program.id or either buffer has changed,
then update those stored buffer ids/refs along with _lastProgramId.
- Around line 58-84: resize() must reset the ping-pong state so an odd number of
prior swap() calls doesn't leave _useA false and cause read/write to the same
buffer; after recreating and assigning _readBuffer and _writeBuffer in
TransformFeedbackPrimitive.resize(), set the ping-pong flag (this._useA) to a
known side (e.g., true) and ensure the current render binding
(_renderBufferBinding) points at the new read buffer so subsequent draw()/swap()
behavior is consistent with the new buffers.
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 625-651: When enabling TF at runtime in _setTFMode, resizing the
transform feedback buffers is insufficient because existing live particles'
state must be copied into TF storage; update _setTFMode so after
creating/resizing this._transformFeedback (ParticleTransformFeedbackSimulator)
you backfill the TF buffers from the current CPU/GPU particle state (e.g. read
the geometry/attribute buffers or the particle system's CPU arrays for position,
velocity, life, etc.) into the transform feedback buffers before enabling the TF
macro (renderer.shaderData.enableMacro(ParticleGenerator._tfModeMacro)),
ensuring the first TF update sees correct data; keep the call to
_reorganizeGeometryBuffers() but perform the backfill immediately after resize
and before any TF-driven update.
- Around line 1023-1036: The world-space TF init currently uses identity
rotation; instead read the captured a_SimulationWorldRotation from the instance
buffer (stored at offset+30) when main.simulationSpace !==
ParticleSimulationSpace.Local and pass that quaternion into _initTFParticle so
the emitted shape position is rotated the same way as the non-TF path; update
the call sites that currently set qx/qy/qz/qw to identity to extract the
quaternion from the instance buffer (or forward it) and ensure _initTFParticle
consumes that quaternion rather than assuming identity, keeping the existing
local-space branch that uses transform.worldRotationQuaternion.
In `@packages/core/src/shader/ShaderProgram.ts`:
- Around line 267-274: The Transform Feedback setup in ShaderProgram currently
casts gl to WebGL2RenderingContext without validation; update the block in the
method that handles transformFeedbackVaryings to first assert that the context
is WebGL2 (e.g., using a type guard like "gl instanceof WebGL2RenderingContext"
or checking an existing "isWebGL2" flag) and early-return if it's not WebGL2,
then call (<WebGL2RenderingContext>gl).transformFeedbackVaryings only when that
guard passes to avoid unsafe casts and runtime errors for future callers.
In `@packages/rhi-webgl/src/GLTransformFeedback.ts`:
- Around line 13-17: The GLTransformFeedback constructor calls
createTransformFeedback() on rhi.gl without ensuring it's a WebGL2 context,
causing a TypeError for WebGL1; update the GLTransformFeedback constructor to
check that rhi.gl is an instance of WebGL2RenderingContext (or that
rhi.supportsTransformFeedback/WebGL2 flag is true) before calling
createTransformFeedback(), and if not supported either set _glTransformFeedback
to null/undefined and mark the instance as unsupported or throw a clear error;
make sure callers via WebGLGraphicDevice.createPlatformTransformFeedback(),
TransformFeedback, and TransformFeedbackPrimitive handle the unsupported case
consistently.
---
Duplicate comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Line 27: After the particle setup completes, hook the test into the screenshot
harness by calling updateForE2E(engine, 500) and then initScreenshot(engine,
camera); if camera isn't available where you build the particle case, thread the
camera through (e.g. add a camera parameter to createScalarLimitParticle or
return the camera from the setup) so you can call initScreenshot(engine, camera)
immediately after setup; apply the same change to the other similar cases
referenced (the blocks that import initScreenshot/updateForE2E).
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 256-270: The _isRandomMode() function currently only inspects
_limitX/_limitY/_limitZ modes (and requires all axes randomized when
_separateAxes is true), but it must also treat randomized drag as random-mode so
the per-particle random shader path is enabled; update _isRandomMode() to return
true if any of _limitX/_limitY/_limitZ is in ParticleCurveMode.TwoConstants or
TwoCurves OR if the drag configuration inspected by _uploadDrag() is in
TwoConstants/TwoCurves (and when _separateAxes is true, consider each axis ORed
rather than requiring all three), ensuring the method references the same drag
mode checks used by _uploadDrag() so randomized drag flips the random-mode flag;
apply the same fix to the other _isRandomMode() copy around the 429-448 area.
- Around line 362-424: In _uploadSeparateAxisLimits detect and handle each axis
mode individually instead of falling back to the “all curves” vs “all constants”
branches: inspect limitX.limitY.limitZ modes and for each axis upload the
appropriate data (for curves use _limit{X|Y|Z}MaxCurveProperty and
_limit{X|Y|Z}MinCurveProperty when TwoCurves, for constants use
_limitMaxConstVecProperty/_limitMinConstVecProperty but only set the
corresponding component), set modeMacro to reflect per-axis curve/constant usage
(or compute a composite shader macro logic) and set randomMacro only if any axis
is random (TwoCurves/TwoConstants) while preserving each axis’s authored values;
modify _uploadSeparateAxisLimits, the uses of
LimitVelocityOverLifetimeModule._limitXMaxCurveProperty/_limitXMinCurveProperty/_limitY.../_limitZ...
and the constant vector packing logic to write per-component values rather than
globally switching branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 135872d8-619c-4b54-83ae-4a092ee18b8f
⛔ Files ignored due to path filters (4)
packages/core/src/shaderlib/extra/particle.vs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/limit_velocity_over_lifetime_module.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/particle_common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/particle_transform_feedback_update.glslis excluded by!**/*.glsl
📒 Files selected for processing (16)
e2e/case/particleRenderer-limitVelocity.tspackages/core/src/graphic/Buffer.tspackages/core/src/graphic/TransformFeedback.tspackages/core/src/graphic/TransformFeedbackPrimitive.tspackages/core/src/graphic/enums/BufferBindFlag.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/ParticleRenderer.tspackages/core/src/particle/ParticleTransformFeedbackSimulator.tspackages/core/src/particle/modules/LimitVelocityOverLifetimeModule.tspackages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.tspackages/core/src/renderingHardwareInterface/index.tspackages/core/src/shader/ShaderProgram.tspackages/core/src/shaderlib/particle/index.tspackages/rhi-webgl/src/GLBuffer.tspackages/rhi-webgl/src/GLTransformFeedback.tspackages/rhi-webgl/src/WebGLGraphicDevice.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/graphic/enums/BufferBindFlag.ts
Comment on lines
+267
to
+274
| // Set Transform Feedback varyings before linking (WebGL2 only) | ||
| if (transformFeedbackVaryings?.length) { | ||
| (<WebGL2RenderingContext>gl).transformFeedbackVaryings( | ||
| program, | ||
| transformFeedbackVaryings, | ||
| (<WebGL2RenderingContext>gl).INTERLEAVED_ATTRIBS | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all ShaderProgram callsites that pass a 4th argument are WebGL2-gated.
rg -nP --type=ts -C3 'new\s+ShaderProgram\s*\([^,]+,\s*[^,]+,\s*[^,]+,\s*[^)]+\)'
rg -nP --type=ts -C3 '\bisWebGL2\b|transformFeedbackVaryings'Repository: galacean/engine
Length of output: 44452
Consider adding WebGL2 validation in the Transform Feedback varyings setup.
The code at lines 268–273 unconditionally casts gl to WebGL2RenderingContext. While the only known call site in ParticleTransformFeedbackSimulator.ts:168 is already guarded with if (!engine._hardwareRenderer.isWebGL2) return;, the function itself lacks internal protection. A future caller could pass transformFeedbackVaryings without the guard, causing a runtime error. Adding a defensive check (e.g., type guard or early return) would improve safety and prevent potential bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/shader/ShaderProgram.ts` around lines 267 - 274, The
Transform Feedback setup in ShaderProgram currently casts gl to
WebGL2RenderingContext without validation; update the block in the method that
handles transformFeedbackVaryings to first assert that the context is WebGL2
(e.g., using a type guard like "gl instanceof WebGL2RenderingContext" or
checking an existing "isWebGL2" flag) and early-return if it's not WebGL2, then
call (<WebGL2RenderingContext>gl).transformFeedbackVaryings only when that guard
passes to avoid unsafe casts and runtime errors for future callers.
Comment on lines
+13
to
+17
| constructor(rhi: WebGLGraphicDevice) { | ||
| const gl = <WebGL2RenderingContext>rhi.gl; | ||
| this._gl = gl; | ||
| this._glTransformFeedback = gl.createTransformFeedback(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "WebGL2-only transform feedback API usage:"
rg -n -C2 'createTransformFeedback|bindTransformFeedback|beginTransformFeedback|endTransformFeedback' packages/rhi-webgl/src
echo
echo "Creation path into GLTransformFeedback:"
rg -n -C2 'createPlatformTransformFeedback|new GLTransformFeedback|new TransformFeedback\(' packagesRepository: galacean/engine
Length of output: 3528
🏁 Script executed:
# Check WebGLGraphicDevice.createPlatformTransformFeedback and surrounding context
sed -n '265,280p' packages/rhi-webgl/src/WebGLGraphicDevice.ts
# Check TransformFeedback constructor and surrounding context
sed -n '10,25p' packages/core/src/graphic/TransformFeedback.ts
# Check TransformFeedbackPrimitive constructor and surrounding context
sed -n '40,60p' packages/core/src/graphic/TransformFeedbackPrimitive.ts
# Check if isWebGL2 is used elsewhere for feature gating
rg -n 'isWebGL2' packagesRepository: galacean/engine
Length of output: 13563
Guard this constructor against WebGL1 contexts.
rhi.gl can still be a WebGLRenderingContext, and createTransformFeedback() is WebGL2-only. On WebGL1 this becomes a TypeError immediately when instantiated.
The creation path through WebGLGraphicDevice.createPlatformTransformFeedback() and the public TransformFeedback and TransformFeedbackPrimitive constructors have no upstream WebGL2 checks, so the guard must be placed here.
Suggested fix
constructor(rhi: WebGLGraphicDevice) {
+ if (!rhi.isWebGL2) {
+ throw new Error("Transform Feedback requires a WebGL2 context.");
+ }
const gl = <WebGL2RenderingContext>rhi.gl;
this._gl = gl;
this._glTransformFeedback = gl.createTransformFeedback();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rhi-webgl/src/GLTransformFeedback.ts` around lines 13 - 17, The
GLTransformFeedback constructor calls createTransformFeedback() on rhi.gl
without ensuring it's a WebGL2 context, causing a TypeError for WebGL1; update
the GLTransformFeedback constructor to check that rhi.gl is an instance of
WebGL2RenderingContext (or that rhi.supportsTransformFeedback/WebGL2 flag is
true) before calling createTransformFeedback(), and if not supported either set
_glTransformFeedback to null/undefined and mark the instance as unsupported or
throw a clear error; make sure callers via
WebGLGraphicDevice.createPlatformTransformFeedback(), TransformFeedback, and
TransformFeedbackPrimitive handle the unsupported case consistently.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (2)
256-265:⚠️ Potential issue | 🟠 Major
_isRandomMode()doesn't account for drag's random modes.The method only checks
_limitX,_limitY,_limitZmodes but ignores_drag. When drag usesTwoConstantsorTwoCurveswhile limits use non-random modes, the particle generator won't allocate per-particle random values for drag interpolation, causing all particles to use the same drag factor.Suggested fix to include drag mode check
_isRandomMode(): boolean { + const dragRandom = this._drag.mode === ParticleCurveMode.TwoConstants || + this._drag.mode === ParticleCurveMode.TwoCurves; if (this._separateAxes) { return ( (this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves) && (this._limitY.mode === ParticleCurveMode.TwoConstants || this._limitY.mode === ParticleCurveMode.TwoCurves) && (this._limitZ.mode === ParticleCurveMode.TwoConstants || this._limitZ.mode === ParticleCurveMode.TwoCurves) - ); + ) || dragRandom; } - return this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves; + return this._limitX.mode === ParticleCurveMode.TwoConstants || + this._limitX.mode === ParticleCurveMode.TwoCurves || + dragRandom; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around lines 256 - 265, The _isRandomMode() method currently checks only _limitX/_limitY/_limitZ modes and misses _drag; update _isRandomMode in LimitVelocityOverLifetimeModule to include a check that _drag.mode is also TwoConstants or TwoCurves (when deciding random mode), i.e., when _separateAxes is true require both all three limit axes and _drag be random-mode, and when _separateAxes is false require either _limitX or _drag to be TwoConstants/TwoCurves so per-particle random values are allocated for drag interpolation.
356-421:⚠️ Potential issue | 🟠 MajorMixed per-axis modes silently fall back to constant upload.
When
separateAxesis enabled with mixed modes (e.g.,limitX = Curve,limitY = Constant,limitZ = Curve), lines 368-373 require all axes to be curves, so the code falls through to the constant path (line 402+). This silently discards curve data for axes configured with curves.Consider either:
- Supporting per-axis mode representation in the shader
- Rejecting/warning on mixed-mode configurations
- Documenting this as expected behavior
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around lines 356 - 421, In _uploadSeparateAxisLimits detect mixed per-axis modes (using limitX/limitY/limitZ and ParticleCurveMode) and avoid silently falling into the constant branch: either (A) implement per-axis uploads by setting per-axis curve/constant shader data (call shaderData.setFloatArray for curve axes using limitX.curveMax/_curveMin and shaderData.setVector3 for constant axes) and introduce per-axis mode macros (e.g., add/check new macros like _limitXCurveMacro/_limitYCurveMacro/_limitZCurveMacro and still set _limitIsRandomMacro where applicable), or (B) if you prefer not to support mixed modes, throw/log a clear warning/error from _uploadSeparateAxisLimits when modes differ instead of uploading constants only; update the function _uploadSeparateAxisLimits, references to LimitVelocityOverLifetimeModule._limitCurveModeMacro/_limitConstantModeMacro/_limitIsRandomMacro, and the shaderData.setFloatArray/setVector3 calls accordingly so curve data is not discarded silently.
🧹 Nitpick comments (5)
e2e/case/particleRenderer-limitVelocity.ts (1)
136-147: Remove commented-out code.Lines 136 and 143-146 contain commented-out alternative configurations. These should either be removed or, if they serve as documentation for alternative test scenarios, converted to separate test functions.
Suggested cleanup
limitVelocityOverLifetime.limitZ = new ParticleCompositeCurve(0); - // limitVelocityOverLifetime.limit = new ParticleCompositeCurve(1); limitVelocityOverLifetime.space = ParticleSimulationSpace.World; limitVelocityOverLifetime.dampen = 0.25; limitVelocityOverLifetime.drag = new ParticleCompositeCurve(0.0); limitVelocityOverLifetime.multiplyDragByParticleSize = true; limitVelocityOverLifetime.multiplyDragByParticleVelocity = true; - - // limitVelocityOverLifetime.enabled = true; - // limitVelocityOverLifetime.separateAxes = false; - // limitVelocityOverLifetime.limit = new ParticleCompositeCurve(0); - // limitVelocityOverLifetime.dampen = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/case/particleRenderer-limitVelocity.ts` around lines 136 - 147, Remove the commented-out alternative configurations around the limitVelocityOverLifetime block: delete the commented lines referencing ParticleCompositeCurve, enabled, separateAxes, and dampen (the comments at and around the limitVelocityOverLifetime assignment), or if those variants are useful as documented alternatives, extract them into separate test functions instead of leaving them commented; locate the limitVelocityOverLifetime usage and ParticleCompositeCurve / ParticleSimulationSpace references in this file to remove or refactor the commented alternatives accordingly.packages/core/src/graphic/TransformFeedbackPrimitive.ts (2)
154-160: Consider reusingVertexBufferBindingobjects to reduce allocations.
swap()creates a newVertexBufferBindinginstance on every call. Since TF updates happen every frame for active particle systems, this causes per-frame allocations. Consider maintaining two pre-allocated binding objects and swapping between them.Suggested optimization
+ private _renderBufferBindingA: VertexBufferBinding; + private _renderBufferBindingB: VertexBufferBinding; + private _useBindingA = true; resize(vertexCount: number): void { // ... existing code ... this._readBuffer = readBuffer; this._writeBuffer = writeBuffer; - this._renderBufferBinding = new VertexBufferBinding(this._readBuffer, this._byteStride); + this._renderBufferBindingA = new VertexBufferBinding(readBuffer, this._byteStride); + this._renderBufferBindingB = new VertexBufferBinding(writeBuffer, this._byteStride); + this._renderBufferBinding = this._renderBufferBindingA; + this._useBindingA = true; // ... } swap(): void { const temp = this._readBuffer; this._readBuffer = this._writeBuffer; this._writeBuffer = temp; this._currentVAO = this._currentVAO === this._vaoA ? this._vaoB : this._vaoA; - this._renderBufferBinding = new VertexBufferBinding(this._readBuffer, this._byteStride); + this._useBindingA = !this._useBindingA; + this._renderBufferBinding = this._useBindingA ? this._renderBufferBindingA : this._renderBufferBindingB; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts` around lines 154 - 160, The swap() method in TransformFeedbackPrimitive currently allocates a new VertexBufferBinding each call (this._renderBufferBinding = new VertexBufferBinding(...)); instead, pre-allocate two VertexBufferBinding instances (e.g., this._bindingA / this._bindingB) as members alongside _vaoA/_vaoB and initialize them for the corresponding buffers and _byteStride, then in swap() simply toggle which binding is active (assign this._renderBufferBinding = this._bindingA or this._bindingB) and update their internal buffer reference/stride if necessary; update any initialization code that creates _renderBufferBinding to use the pre-allocated pair and ensure _renderBufferBinding is swapped when _readBuffer/_writeBuffer and _currentVAO are swapped.
61-86: Consider resetting_currentVAOreference inresize()to avoid stale reference.After
resize()sets_vaoDirty = true, the old VAOs will be deleted inrebuildVAOsIfNeeded(). However, ifbindVAO()were called beforerebuildVAOsIfNeeded(),_currentVAOwould reference a deleted VAO. While the current usage pattern appears safe (rebuild is always called before bind), explicitly nullifying_currentVAOinresize()would make the code more defensive.Suggested defensive fix
this._vertexCount = vertexCount; this._initialized = true; this._vaoDirty = true; + this._currentVAO = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts` around lines 61 - 86, The resize() method should clear the cached VAO reference to avoid holding a pointer to a VAO that will be destroyed; add a line to set this._currentVAO = null (or undefined consistent with class) when resizing—ideally right after setting this._vaoDirty = true—so that rebuildVAOsIfNeeded() can safely recreate VAOs and bindVAO() cannot reference a deleted object.packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (2)
127-127: Replace magic number with GL constant for clarity.
POINTS = 0x0000is the raw WebGL constant value. Consider usinggl.POINTSfor better readability and maintainability.Suggested fix
+ const gl = this._engine._hardwareRenderer.gl; // Bind VAO and execute TF for alive particles this._primitive.bindVAO(); - const POINTS = 0x0000; if (firstActive < firstFree) { - this._primitive.draw(rhi, POINTS, firstActive, firstFree - firstActive); + this._primitive.draw(rhi, gl.POINTS, firstActive, firstFree - firstActive); } else { - this._primitive.draw(rhi, POINTS, firstActive, particleCount - firstActive); + this._primitive.draw(rhi, gl.POINTS, firstActive, particleCount - firstActive); if (firstFree > 0) { - this._primitive.draw(rhi, 0, firstFree); + this._primitive.draw(rhi, gl.POINTS, 0, firstFree); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` at line 127, Replace the magic numeric literal used for POINTS with the WebGL constant from the existing GL context: change the declaration that sets POINTS = 0x0000 to use the in-scope WebGLRenderingContext constant (e.g., POINTS = gl.POINTS or this.gl.POINTS depending on how the context is referenced in ParticleTransformFeedbackSimulator) so the code uses the named constant instead of the raw hex value.
109-113: Per-frameVertexBufferBindingallocation in update loop.
VertexBufferBindingis instantiated on everyupdate()call (line 110). Since this runs every frame for active particle systems, it creates unnecessary GC pressure. Consider caching and reusing the binding when the instance buffer hasn't changed.Suggested optimization
+ private _cachedInstanceBuffer: Buffer; + private _cachedInstanceBinding: VertexBufferBinding; update( instanceBuffer: Buffer, // ... ): void { // ... - const instanceBinding = new VertexBufferBinding(instanceBuffer, ParticleBufferUtils.instanceVertexStride); + if (this._cachedInstanceBuffer !== instanceBuffer) { + this._cachedInstanceBuffer = instanceBuffer; + this._cachedInstanceBinding = new VertexBufferBinding(instanceBuffer, ParticleBufferUtils.instanceVertexStride); + } + const instanceBinding = this._cachedInstanceBinding; this._primitive.rebuildVAOsIfNeeded(this._tfProgram, ParticleTransformFeedbackSimulator._tfElements, [ { binding: instanceBinding, elements: ParticleTransformFeedbackSimulator._instanceElements } ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around lines 109 - 113, The code currently allocates a new VertexBufferBinding each frame inside update(), causing GC churn; cache and reuse the binding instead: add a class field (e.g., this._instanceBinding) and in update() only create a new VertexBufferBinding when the instanceBuffer reference changes (compare instanceBuffer to the cached binding's buffer or null), otherwise reuse the cached binding when calling this._primitive.rebuildVAOsIfNeeded(this._tfProgram, ParticleTransformFeedbackSimulator._tfElements, [{ binding: this._instanceBinding, elements: ParticleTransformFeedbackSimulator._instanceElements }]); ensure the cache is cleared/updated whenever the instanceBuffer is replaced or disposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 256-265: The _isRandomMode() method currently checks only
_limitX/_limitY/_limitZ modes and misses _drag; update _isRandomMode in
LimitVelocityOverLifetimeModule to include a check that _drag.mode is also
TwoConstants or TwoCurves (when deciding random mode), i.e., when _separateAxes
is true require both all three limit axes and _drag be random-mode, and when
_separateAxes is false require either _limitX or _drag to be
TwoConstants/TwoCurves so per-particle random values are allocated for drag
interpolation.
- Around line 356-421: In _uploadSeparateAxisLimits detect mixed per-axis modes
(using limitX/limitY/limitZ and ParticleCurveMode) and avoid silently falling
into the constant branch: either (A) implement per-axis uploads by setting
per-axis curve/constant shader data (call shaderData.setFloatArray for curve
axes using limitX.curveMax/_curveMin and shaderData.setVector3 for constant
axes) and introduce per-axis mode macros (e.g., add/check new macros like
_limitXCurveMacro/_limitYCurveMacro/_limitZCurveMacro and still set
_limitIsRandomMacro where applicable), or (B) if you prefer not to support mixed
modes, throw/log a clear warning/error from _uploadSeparateAxisLimits when modes
differ instead of uploading constants only; update the function
_uploadSeparateAxisLimits, references to
LimitVelocityOverLifetimeModule._limitCurveModeMacro/_limitConstantModeMacro/_limitIsRandomMacro,
and the shaderData.setFloatArray/setVector3 calls accordingly so curve data is
not discarded silently.
---
Nitpick comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 136-147: Remove the commented-out alternative configurations
around the limitVelocityOverLifetime block: delete the commented lines
referencing ParticleCompositeCurve, enabled, separateAxes, and dampen (the
comments at and around the limitVelocityOverLifetime assignment), or if those
variants are useful as documented alternatives, extract them into separate test
functions instead of leaving them commented; locate the
limitVelocityOverLifetime usage and ParticleCompositeCurve /
ParticleSimulationSpace references in this file to remove or refactor the
commented alternatives accordingly.
In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts`:
- Around line 154-160: The swap() method in TransformFeedbackPrimitive currently
allocates a new VertexBufferBinding each call (this._renderBufferBinding = new
VertexBufferBinding(...)); instead, pre-allocate two VertexBufferBinding
instances (e.g., this._bindingA / this._bindingB) as members alongside
_vaoA/_vaoB and initialize them for the corresponding buffers and _byteStride,
then in swap() simply toggle which binding is active (assign
this._renderBufferBinding = this._bindingA or this._bindingB) and update their
internal buffer reference/stride if necessary; update any initialization code
that creates _renderBufferBinding to use the pre-allocated pair and ensure
_renderBufferBinding is swapped when _readBuffer/_writeBuffer and _currentVAO
are swapped.
- Around line 61-86: The resize() method should clear the cached VAO reference
to avoid holding a pointer to a VAO that will be destroyed; add a line to set
this._currentVAO = null (or undefined consistent with class) when
resizing—ideally right after setting this._vaoDirty = true—so that
rebuildVAOsIfNeeded() can safely recreate VAOs and bindVAO() cannot reference a
deleted object.
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Line 127: Replace the magic numeric literal used for POINTS with the WebGL
constant from the existing GL context: change the declaration that sets POINTS =
0x0000 to use the in-scope WebGLRenderingContext constant (e.g., POINTS =
gl.POINTS or this.gl.POINTS depending on how the context is referenced in
ParticleTransformFeedbackSimulator) so the code uses the named constant instead
of the raw hex value.
- Around line 109-113: The code currently allocates a new VertexBufferBinding
each frame inside update(), causing GC churn; cache and reuse the binding
instead: add a class field (e.g., this._instanceBinding) and in update() only
create a new VertexBufferBinding when the instanceBuffer reference changes
(compare instanceBuffer to the cached binding's buffer or null), otherwise reuse
the cached binding when calling
this._primitive.rebuildVAOsIfNeeded(this._tfProgram,
ParticleTransformFeedbackSimulator._tfElements, [{ binding:
this._instanceBinding, elements:
ParticleTransformFeedbackSimulator._instanceElements }]); ensure the cache is
cleared/updated whenever the instanceBuffer is replaced or disposed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d59df521-c2b6-4d48-b48b-0b1f6cd258e6
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-limitVelocity.jpgis excluded by!**/*.jpg
📒 Files selected for processing (6)
e2e/case/particleRenderer-limitVelocity.tse2e/config.tspackages/core/src/graphic/TransformFeedbackPrimitive.tspackages/core/src/particle/ParticleRenderer.tspackages/core/src/particle/ParticleTransformFeedbackSimulator.tspackages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/config.ts
GuoLei1990
force-pushed
the
feat/limitParticle
branch
from
3c88bf0 to
d023c18
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (1)
44-44: Inconsistent shader property naming.
_dragConstantPropertyuses"u_DragConstant"while all other properties in this module use the"renderer_LVL*"prefix (e.g.,renderer_LVLDampen,renderer_LVLDragMaxCurve). Consider renaming to"renderer_LVLDragConstant"for consistency.Suggested fix
- static readonly _dragConstantProperty = ShaderProperty.getByName("u_DragConstant"); + static readonly _dragConstantProperty = ShaderProperty.getByName("renderer_LVLDragConstant");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` at line 44, Update the shader property name for consistency: change the string passed to ShaderProperty.getByName in the static member _dragConstantProperty from "u_DragConstant" to "renderer_LVLDragConstant" and ensure any other usages that reference the shader property by name align with this new identifier (look for references to _dragConstantProperty and any direct "u_DragConstant" usages to update them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 423-445: In _uploadDrag, constant-based drag modes never set a
"random" macro so the shader can't tell TwoConstants (random) from Constant;
update _uploadDrag to set and return a dedicated macro (e.g.,
LimitVelocityOverLifetimeModule._dragIsRandomMacro) when drag.mode ===
ParticleCurveMode.TwoConstants (in addition to writing _dragConstantProperty
using _dragConstantVec), keep existing behavior of returning _dragCurveModeMacro
for curve modes, and ensure the shader code uses that new _dragIsRandomMacro
(and related symbol _isRandomMode()) to choose interpolation; reference symbols:
_uploadDrag, _drag, _dragConstantVec, _dragConstantProperty,
_dragCurveModeMacro, _dragMin/Max curve properties, and add/return
LimitVelocityOverLifetimeModule._dragIsRandomMacro when appropriate.
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 64-83: The code marks the transform-feedback simulator initialized
even when resized to zero, allowing writeParticleData to write into
zero-capacity buffers; fix by only setting this->_initialized (in resize) when
particleCount > 0 (or set it false when particleCount === 0), and/or add a
defensive check at the start of writeParticleData to return if the primitive
buffers have insufficient capacity (use
ParticleTransformFeedbackSimulator._byteStride and the index to compute/validate
byteOffset before calling _primitive.readBuffer.setData and
_primitive.writeBuffer.setData) so writes never occur into zero-sized ping-pong
buffers.
---
Nitpick comments:
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Line 44: Update the shader property name for consistency: change the string
passed to ShaderProperty.getByName in the static member _dragConstantProperty
from "u_DragConstant" to "renderer_LVLDragConstant" and ensure any other usages
that reference the shader property by name align with this new identifier (look
for references to _dragConstantProperty and any direct "u_DragConstant" usages
to update them).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2703be57-bb7a-4843-8e3a-adad99afc684
⛔ Files ignored due to path filters (1)
packages/core/src/shaderlib/particle/particle_transform_feedback_update.glslis excluded by!**/*.glsl
📒 Files selected for processing (3)
packages/core/src/particle/ParticleRenderer.tspackages/core/src/particle/ParticleTransformFeedbackSimulator.tspackages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
Comment on lines
+423
to
+445
| private _uploadDrag(shaderData: ShaderData): ShaderMacro { | ||
| const drag = this._drag; | ||
| let dragCurveMacro: ShaderMacro = null; | ||
|
|
||
| const isRandomCurveMode = drag.mode === ParticleCurveMode.TwoCurves; | ||
| if (isRandomCurveMode || drag.mode === ParticleCurveMode.Curve) { | ||
| shaderData.setFloatArray(LimitVelocityOverLifetimeModule._dragMaxCurveProperty, drag.curveMax._getTypeArray()); | ||
| dragCurveMacro = LimitVelocityOverLifetimeModule._dragCurveModeMacro; | ||
| if (isRandomCurveMode) { | ||
| shaderData.setFloatArray(LimitVelocityOverLifetimeModule._dragMinCurveProperty, drag.curveMin._getTypeArray()); | ||
| } | ||
| } else { | ||
| const dragVec = this._dragConstantVec; | ||
| if (drag.mode === ParticleCurveMode.TwoConstants) { | ||
| dragVec.set(drag.constantMin, drag.constantMax); | ||
| } else { | ||
| dragVec.set(drag.constantMax, drag.constantMax); | ||
| } | ||
| shaderData.setVector2(LimitVelocityOverLifetimeModule._dragConstantProperty, dragVec); | ||
| } | ||
|
|
||
| return dragCurveMacro; | ||
| } |
There was a problem hiding this comment.
Drag constant modes lack a random indicator macro.
For constant-based drag modes (TwoConstants vs Constant), no macro is returned to inform the shader whether to interpolate between min and max using a random value. The shader receives a Vector2 but cannot distinguish between "use both with random" and "use single value".
This is related to the _isRandomMode() issue—without proper random mode detection and a dedicated macro (similar to _limitIsRandomMacro), drag randomization may not work as expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 423 - 445, In _uploadDrag, constant-based drag modes never set a "random"
macro so the shader can't tell TwoConstants (random) from Constant; update
_uploadDrag to set and return a dedicated macro (e.g.,
LimitVelocityOverLifetimeModule._dragIsRandomMacro) when drag.mode ===
ParticleCurveMode.TwoConstants (in addition to writing _dragConstantProperty
using _dragConstantVec), keep existing behavior of returning _dragCurveModeMacro
for curve modes, and ensure the shader code uses that new _dragIsRandomMacro
(and related symbol _isRandomMode()) to choose interpolation; reference symbols:
_uploadDrag, _drag, _dragConstantVec, _dragConstantProperty,
_dragCurveModeMacro, _dragMin/Max curve properties, and add/return
LimitVelocityOverLifetimeModule._dragIsRandomMacro when appropriate.
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
Outdated
Show resolved
Hide resolved
GuoLei1990
force-pushed
the
feat/limitParticle
branch
from
68b2e13 to
2024ae1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts (1)
27-35:⚠️ Potential issue | 🟠 MajorRebuild VAOs when buffer bindings change, not only when program changes.
Line 27 currently keys cache on
program.idonly. IfreadBinding,writeBinding, orinputBindingpoints to a new buffer with the same program, VAOs keep stale bindings.Suggested fix
export class GLTransformFeedbackPrimitive implements IPlatformTransformFeedbackPrimitive { private _gl: WebGL2RenderingContext; private _vaoA: WebGLVertexArrayObject; private _vaoB: WebGLVertexArrayObject; private _lastProgramId = -1; + private _lastReadBuffer: unknown = null; + private _lastWriteBuffer: unknown = null; + private _lastInputBuffer: unknown = null; @@ updateVertexLayout( program: any, readBinding: VertexBufferBinding, writeBinding: VertexBufferBinding, @@ ): void { - if (program.id === this._lastProgramId) return; + const readBuffer = readBinding?.buffer; + const writeBuffer = writeBinding?.buffer; + const inputBuffer = inputBinding?.buffer; + if ( + program.id === this._lastProgramId && + readBuffer === this._lastReadBuffer && + writeBuffer === this._lastWriteBuffer && + inputBuffer === this._lastInputBuffer + ) { + return; + } @@ this._vaoA = this._createVAO(attribs, readBinding, feedbackElements, inputBinding, inputElements); this._vaoB = this._createVAO(attribs, writeBinding, feedbackElements, inputBinding, inputElements); this._lastProgramId = program.id; + this._lastReadBuffer = readBuffer; + this._lastWriteBuffer = writeBuffer; + this._lastInputBuffer = inputBuffer; @@ private _deleteVAOs(): void { @@ this._lastProgramId = -1; + this._lastReadBuffer = null; + this._lastWriteBuffer = null; + this._lastInputBuffer = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts` around lines 27 - 35, The code only rebuilds VAOs when program.id changes (checked via this._lastProgramId), so VAOs stay stale if readBinding, writeBinding, or inputBinding change; update the logic to also detect binding changes by tracking unique identifiers for readBinding, writeBinding and inputBinding (e.g., buffer id or a composite key) and include them in the cache check before returning, and when any of those ids differ call this._deleteVAOs() and recreate this._vaoA/this._vaoB via this._createVAO(attribs, ...), then update the stored last-program and last-binding ids (e.g., this._lastReadBindingId, this._lastWriteBindingId, this._lastInputBindingId) alongside this._lastProgramId.packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (1)
260-269:⚠️ Potential issue | 🟠 MajorInclude drag random modes in module random-mode detection.
Line 260-Line 269 currently ignores drag random modes. This can skip per-particle random generation when
dragisTwoConstants/TwoCurvesbut limits are non-random.Suggested fix
_isRandomMode(): boolean { + const dragRandom = + this._drag.mode === ParticleCurveMode.TwoConstants || this._drag.mode === ParticleCurveMode.TwoCurves; + + const limitRandom = this._separateAxes + ? (this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves) && + (this._limitY.mode === ParticleCurveMode.TwoConstants || this._limitY.mode === ParticleCurveMode.TwoCurves) && + (this._limitZ.mode === ParticleCurveMode.TwoConstants || this._limitZ.mode === ParticleCurveMode.TwoCurves) + : this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves; + - if (this._separateAxes) { - return ( - (this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves) && - (this._limitY.mode === ParticleCurveMode.TwoConstants || this._limitY.mode === ParticleCurveMode.TwoCurves) && - (this._limitZ.mode === ParticleCurveMode.TwoConstants || this._limitZ.mode === ParticleCurveMode.TwoCurves) - ); - } - return this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves; + return limitRandom || dragRandom; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around lines 260 - 269, The _isRandomMode method currently only checks limit curves and ignores drag random modes; update it to also consider drag being in random modes (ParticleCurveMode.TwoConstants or ParticleCurveMode.TwoCurves). Specifically, in the branch where this._separateAxes is true, include checks for this._dragX.mode, this._dragY.mode and this._dragZ.mode (alongside _limitX/_limitY/_limitZ) and in the non-separate branch include this._drag.mode in the OR logic with this._limitX.mode so that any drag TwoConstants/TwoCurves causes _isRandomMode to return true.packages/rhi-webgl/src/GLTransformFeedback.ts (1)
13-17:⚠️ Potential issue | 🔴 CriticalAdd explicit WebGL2 and handle-creation guards in constructor.
This constructor still assumes WebGL2 and does not check
createTransformFeedback()result, which can fail and produce invalid state.Suggested patch
constructor(rhi: WebGLGraphicDevice) { + if (!rhi.isWebGL2) { + throw new Error("Transform Feedback requires a WebGL2 context."); + } const gl = <WebGL2RenderingContext>rhi.gl; this._gl = gl; - this._glTransformFeedback = gl.createTransformFeedback(); + const tf = gl.createTransformFeedback(); + if (!tf) { + throw new Error("Failed to create WebGLTransformFeedback."); + } + this._glTransformFeedback = tf; }#!/bin/bash set -e # Confirm guard presence and constructor call paths sed -n '1,120p' packages/rhi-webgl/src/GLTransformFeedback.ts rg -nP --type=ts 'new GLTransformFeedback\s*\(' packages rg -nP --type=ts -C3 'createPlatformTransformFeedback|isWebGL2' packages/rhi-webgl/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rhi-webgl/src/GLTransformFeedback.ts` around lines 13 - 17, The GLTransformFeedback constructor assumes WebGL2 and doesn't validate createTransformFeedback() output; update the constructor in class GLTransformFeedback to assert/guard that rhi.gl is a WebGL2RenderingContext (use isWebGL2 or instanceof WebGL2RenderingContext) and check the result of gl.createTransformFeedback(); if it returns null handle it by throwing or returning a clear error (or set a safe fallback and log) and ensure _glTransformFeedback is only assigned when non-null, referencing the constructor, WebGL2RenderingContext, createTransformFeedback, and _glTransformFeedback symbols.e2e/case/particleRenderer-limitVelocity.ts (1)
27-28:⚠️ Potential issue | 🟡 MinorWire the screenshot harness in this case setup.
initScreenshot/updateForE2Eare imported but unused; the case never explicitly settles before capture. This can make snapshots timing-sensitive.Suggested patch
- .then((texture) => { - createScalarLimitParticle(engine, rootEntity, <Texture2D>texture); + .then((texture) => { + createScalarLimitParticle(engine, rootEntity, camera, <Texture2D>texture); }); }); -function createScalarLimitParticle(engine: Engine, rootEntity: Entity, texture: Texture2D): void { +function createScalarLimitParticle(engine: Engine, rootEntity: Entity, camera: Camera, texture: Texture2D): void { @@ limitVelocityOverLifetime.drag = new ParticleCompositeCurve(0.0); limitVelocityOverLifetime.multiplyDragByParticleSize = true; limitVelocityOverLifetime.multiplyDragByParticleVelocity = true; + + updateForE2E(engine, 500); + initScreenshot(engine, camera); }Also applies to: 64-68, 148-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/case/particleRenderer-limitVelocity.ts` around lines 27 - 28, The test imports initScreenshot and updateForE2E but never uses them, so wire the screenshot harness into the case: call initScreenshot() near the start of the test setup to initialize the harness and, after any actions that change state but before taking a snapshot, call await updateForE2E() to let the scene settle; update the particleRenderer-limitVelocity test case (references to initScreenshot and updateForE2E in the file) to include these calls in the setup and immediately before captures so snapshots are not timing-sensitive.
🧹 Nitpick comments (2)
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (1)
84-84: Avoid per-frameVertexBufferBindingallocation in hot update loop.Line 84 allocates every frame. Reusing a cached binding (or mutating an existing one) will reduce GC churn during particle-heavy scenes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` at line 84, The per-frame allocation of a VertexBufferBinding (variable instanceBinding) inside ParticleTransformFeedbackSimulator causes GC churn; cache a reusable binding on the simulator instance instead of constructing it each update: add a class field (e.g., this.instanceBinding) created once using instanceBuffer and ParticleBufferUtils.instanceVertexStride and reuse it every frame, updating its buffer reference only if instanceBuffer changes (or mutate the existing binding) so you stop allocating a new VertexBufferBinding each frame.packages/core/src/shader/ShaderPass.ts (1)
153-163: Consider centralizing capability macro collection.
_getShaderLabProgram()rebuilds the same renderer-capability macro list that the non-ShaderLab path already needs. Pulling that into a shared helper would reduce drift if new capability macros get added later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shader/ShaderPass.ts` around lines 153 - 163, The code in _getShaderLabProgram rebuilds renderer capability macros (e.g., GRAPHICS_API_WEBGL2/1, HAS_TEX_LOD, HAS_DERIVATIVES) into shaderMacroList; extract that logic into a shared helper (for example a new private function like collectRendererCapabilityMacros(engine: Engine, target: ShaderMacro[])) and call it from _getShaderLabProgram and the non-ShaderLab path so both use the same single source of truth; ensure the helper uses engine._hardwareRenderer and ShaderMacro.getByName to push the same macros and update callers to remove the duplicated push logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 123-129: Remove the leftover debug logging inside the setTimeout
callback: locate the anonymous callback that sets velocityOverLifetime.enabled
and its velocityX/Y/Z constants and delete the console.log("s") call so the E2E
path no longer emits debug output; ensure only the velocityOverLifetime changes
remain in that callback.
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 987-1002: The initial velocity is not rotated into world space
when this.main.simulationSpace !== ParticleSimulationSpace.Local, so rotate the
local direction by transform.worldRotationQuaternion before passing to
writeParticleData: after computing world position (using
ParticleGenerator._tempVector32 and transform.worldRotationQuaternion),
transform the local direction vector (the existing direction variable) by
transform.worldRotationQuaternion into a temp vector (e.g., a ParticleGenerator
temp Vector3), then use that rotated vector multiplied by startSpeed for the
x/y/z velocity args in this._feedbackSimulator.writeParticleData so
world-simulated particles respect emitter rotation.
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 85-105: beginUpdate()/endUpdate() pair in
ParticleTransformFeedbackSimulator is not exception-safe: if any
this._simulator.draw(...) throws, endUpdate() is skipped; wrap the draw logic in
a try/finally so endUpdate() always executes when beginUpdate(...) returned
true. Concretely, after the successful beginUpdate(...) call on this._simulator,
enclose the conditional draw(...) calls (including both branches using
MeshTopology.Points and the two draw calls) inside a try block and call
this._simulator.endUpdate() in the finally block; preserve the early return when
beginUpdate(...) returns false so endUpdate() is only invoked when
beginUpdate(...) succeeded.
In `@packages/rhi-webgl/src/WebGLGraphicDevice.ts`:
- Around line 279-281: createPlatformTransformFeedbackPrimitive() assumes WebGL2
and will crash on WebGL1; add a guard using this._isWebGL2 before instantiating
GLTransformFeedbackPrimitive. In createPlatformTransformFeedbackPrimitive(),
check this._isWebGL2 and if true return new
GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); otherwise either
throw a clear Error (e.g. "Transform feedback requires WebGL2") or return a
no-op/fallback IPlatformTransformFeedbackPrimitive implementation to match how
other WebGL2-only methods are protected.
---
Duplicate comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 27-28: The test imports initScreenshot and updateForE2E but never
uses them, so wire the screenshot harness into the case: call initScreenshot()
near the start of the test setup to initialize the harness and, after any
actions that change state but before taking a snapshot, call await
updateForE2E() to let the scene settle; update the
particleRenderer-limitVelocity test case (references to initScreenshot and
updateForE2E in the file) to include these calls in the setup and immediately
before captures so snapshots are not timing-sensitive.
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 260-269: The _isRandomMode method currently only checks limit
curves and ignores drag random modes; update it to also consider drag being in
random modes (ParticleCurveMode.TwoConstants or ParticleCurveMode.TwoCurves).
Specifically, in the branch where this._separateAxes is true, include checks for
this._dragX.mode, this._dragY.mode and this._dragZ.mode (alongside
_limitX/_limitY/_limitZ) and in the non-separate branch include this._drag.mode
in the OR logic with this._limitX.mode so that any drag TwoConstants/TwoCurves
causes _isRandomMode to return true.
In `@packages/rhi-webgl/src/GLTransformFeedback.ts`:
- Around line 13-17: The GLTransformFeedback constructor assumes WebGL2 and
doesn't validate createTransformFeedback() output; update the constructor in
class GLTransformFeedback to assert/guard that rhi.gl is a
WebGL2RenderingContext (use isWebGL2 or instanceof WebGL2RenderingContext) and
check the result of gl.createTransformFeedback(); if it returns null handle it
by throwing or returning a clear error (or set a safe fallback and log) and
ensure _glTransformFeedback is only assigned when non-null, referencing the
constructor, WebGL2RenderingContext, createTransformFeedback, and
_glTransformFeedback symbols.
In `@packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts`:
- Around line 27-35: The code only rebuilds VAOs when program.id changes
(checked via this._lastProgramId), so VAOs stay stale if readBinding,
writeBinding, or inputBinding change; update the logic to also detect binding
changes by tracking unique identifiers for readBinding, writeBinding and
inputBinding (e.g., buffer id or a composite key) and include them in the cache
check before returning, and when any of those ids differ call this._deleteVAOs()
and recreate this._vaoA/this._vaoB via this._createVAO(attribs, ...), then
update the stored last-program and last-binding ids (e.g.,
this._lastReadBindingId, this._lastWriteBindingId, this._lastInputBindingId)
alongside this._lastProgramId.
---
Nitpick comments:
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Line 84: The per-frame allocation of a VertexBufferBinding (variable
instanceBinding) inside ParticleTransformFeedbackSimulator causes GC churn;
cache a reusable binding on the simulator instance instead of constructing it
each update: add a class field (e.g., this.instanceBinding) created once using
instanceBuffer and ParticleBufferUtils.instanceVertexStride and reuse it every
frame, updating its buffer reference only if instanceBuffer changes (or mutate
the existing binding) so you stop allocating a new VertexBufferBinding each
frame.
In `@packages/core/src/shader/ShaderPass.ts`:
- Around line 153-163: The code in _getShaderLabProgram rebuilds renderer
capability macros (e.g., GRAPHICS_API_WEBGL2/1, HAS_TEX_LOD, HAS_DERIVATIVES)
into shaderMacroList; extract that logic into a shared helper (for example a new
private function like collectRendererCapabilityMacros(engine: Engine, target:
ShaderMacro[])) and call it from _getShaderLabProgram and the non-ShaderLab path
so both use the same single source of truth; ensure the helper uses
engine._hardwareRenderer and ShaderMacro.getByName to push the same macros and
update callers to remove the duplicated push logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4fc14c71-df4e-45c3-86f7-9d341f40ef7b
⛔ Files ignored due to path filters (3)
packages/core/src/shaderlib/extra/particle.vs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/limit_velocity_over_lifetime_module.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/particle_feedback_simulation.glslis excluded by!**/*.glsl
📒 Files selected for processing (19)
e2e/case/particleRenderer-limitVelocity.tspackages/core/src/graphic/TransformFeedback.tspackages/core/src/graphic/TransformFeedbackPrimitive.tspackages/core/src/graphic/TransformFeedbackSimulator.tspackages/core/src/particle/ParticleBufferUtils.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/ParticleRenderer.tspackages/core/src/particle/ParticleTransformFeedbackSimulator.tspackages/core/src/particle/enums/attributes/ParticleFeedbackVertexAttribute.tspackages/core/src/particle/modules/LimitVelocityOverLifetimeModule.tspackages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.tspackages/core/src/renderingHardwareInterface/IPlatformTransformFeedbackPrimitive.tspackages/core/src/renderingHardwareInterface/index.tspackages/core/src/shader/ShaderPass.tspackages/core/src/shaderlib/ShaderFactory.tspackages/core/src/shaderlib/particle/index.tspackages/rhi-webgl/src/GLTransformFeedback.tspackages/rhi-webgl/src/GLTransformFeedbackPrimitive.tspackages/rhi-webgl/src/WebGLGraphicDevice.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.ts
- packages/core/src/graphic/TransformFeedback.ts
Comment on lines
+987
to
+1002
| let position: Vector3; | ||
| if (this.main.simulationSpace === ParticleSimulationSpace.Local) { | ||
| position = shapePosition; | ||
| } else { | ||
| position = ParticleGenerator._tempVector32; | ||
| Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position); | ||
| position.add(transform.worldPosition); | ||
| } | ||
|
|
||
| this._feedbackSimulator.writeParticleData( | ||
| index, | ||
| position, | ||
| direction.x * startSpeed, | ||
| direction.y * startSpeed, | ||
| direction.z * startSpeed | ||
| ); |
There was a problem hiding this comment.
Rotate initial TF velocity in world simulation mode.
In world mode, position is rotated into world space (Lines 991–994), but velocity at Lines 999–1001 still uses local direction. This causes incorrect initial motion when emitter rotation is non-identity.
Suggested patch
private _addFeedbackParticle(
@@
): void {
let position: Vector3;
+ let velocity = direction;
if (this.main.simulationSpace === ParticleSimulationSpace.Local) {
position = shapePosition;
} else {
position = ParticleGenerator._tempVector32;
Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position);
position.add(transform.worldPosition);
+ velocity = ParticleGenerator._tempVector31;
+ Vector3.transformByQuat(direction, transform.worldRotationQuaternion, velocity);
}
this._feedbackSimulator.writeParticleData(
index,
position,
- direction.x * startSpeed,
- direction.y * startSpeed,
- direction.z * startSpeed
+ velocity.x * startSpeed,
+ velocity.y * startSpeed,
+ velocity.z * startSpeed
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/particle/ParticleGenerator.ts` around lines 987 - 1002, The
initial velocity is not rotated into world space when this.main.simulationSpace
!== ParticleSimulationSpace.Local, so rotate the local direction by
transform.worldRotationQuaternion before passing to writeParticleData: after
computing world position (using ParticleGenerator._tempVector32 and
transform.worldRotationQuaternion), transform the local direction vector (the
existing direction variable) by transform.worldRotationQuaternion into a temp
vector (e.g., a ParticleGenerator temp Vector3), then use that rotated vector
multiplied by startSpeed for the x/y/z velocity args in
this._feedbackSimulator.writeParticleData so world-simulated particles respect
emitter rotation.
Comment on lines
+85
to
+105
| if ( | ||
| !this._simulator.beginUpdate( | ||
| shaderData, | ||
| ParticleBufferUtils.feedbackVertexElements, | ||
| instanceBinding, | ||
| ParticleBufferUtils.feedbackInstanceElements | ||
| ) | ||
| ) | ||
| return; | ||
|
|
||
| if (firstActive < firstFree) { | ||
| this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive); | ||
| } else { | ||
| this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive); | ||
| if (firstFree > 0) { | ||
| this._simulator.draw(MeshTopology.Points, 0, firstFree); | ||
| } | ||
| } | ||
|
|
||
| this._simulator.endUpdate(); | ||
| } |
There was a problem hiding this comment.
Make TF update exception-safe with try/finally.
If any draw-path call throws after beginUpdate(), endUpdate() is skipped and TF state cleanup is missed.
Suggested patch
if (
!this._simulator.beginUpdate(
shaderData,
ParticleBufferUtils.feedbackVertexElements,
instanceBinding,
ParticleBufferUtils.feedbackInstanceElements
)
)
return;
- if (firstActive < firstFree) {
- this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
- } else {
- this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
- if (firstFree > 0) {
- this._simulator.draw(MeshTopology.Points, 0, firstFree);
+ try {
+ if (firstActive < firstFree) {
+ this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
+ } else {
+ this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
+ if (firstFree > 0) {
+ this._simulator.draw(MeshTopology.Points, 0, firstFree);
+ }
}
+ } finally {
+ this._simulator.endUpdate();
}
-
- this._simulator.endUpdate();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around
lines 85 - 105, beginUpdate()/endUpdate() pair in
ParticleTransformFeedbackSimulator is not exception-safe: if any
this._simulator.draw(...) throws, endUpdate() is skipped; wrap the draw logic in
a try/finally so endUpdate() always executes when beginUpdate(...) returned
true. Concretely, after the successful beginUpdate(...) call on this._simulator,
enclose the conditional draw(...) calls (including both branches using
MeshTopology.Points and the two draw calls) inside a try block and call
this._simulator.endUpdate() in the finally block; preserve the early return when
beginUpdate(...) returns false so endUpdate() is only invoked when
beginUpdate(...) succeeded.
Comment on lines
+279
to
+281
| createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive { | ||
| return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
# Verify whether this factory can be reached without explicit WebGL2 gating
rg -nP --type=ts '\bcreatePlatformTransformFeedbackPrimitive\s*\(' packages
rg -nP --type=ts -C3 'new GLTransformFeedbackPrimitive|isWebGL2' packages/rhi-webgl/src
sed -n '1,140p' packages/rhi-webgl/src/GLTransformFeedbackPrimitive.tsRepository: galacean/engine
Length of output: 37600
Guard TF primitive creation behind WebGL2 capability.
createPlatformTransformFeedbackPrimitive() currently assumes a WebGL2 context without checking. In a WebGL1 runtime, this will fail at first API call. Other methods in the same class (e.g., enableRasterizerDiscard, blitFrameBuffer) properly guard WebGL2-only operations using this._isWebGL2.
Suggested patch
createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive {
+ if (!this._isWebGL2) {
+ throw new Error("Transform Feedback primitive requires a WebGL2 context.");
+ }
return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl);
}📝 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.
Suggested change
| createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive { | |
| return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); | |
| } | |
| createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive { | |
| if (!this._isWebGL2) { | |
| throw new Error("Transform Feedback primitive requires a WebGL2 context."); | |
| } | |
| return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rhi-webgl/src/WebGLGraphicDevice.ts` around lines 279 - 281,
createPlatformTransformFeedbackPrimitive() assumes WebGL2 and will crash on
WebGL1; add a guard using this._isWebGL2 before instantiating
GLTransformFeedbackPrimitive. In createPlatformTransformFeedbackPrimitive(),
check this._isWebGL2 and if true return new
GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); otherwise either
throw a clear Error (e.g. "Transform feedback requires WebGL2") or return a
no-op/fallback IPlatformTransformFeedbackPrimitive implementation to match how
other WebGL2-only methods are protected.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 122-123: Before enabling limitVelocityOverLifetime, add an
explicit WebGL2 capability guard that checks the renderer/context for WebGL2
(e.g. renderer.capabilities.isWebGL2 or gl instanceof WebGL2RenderingContext)
and fail or skip the test if WebGL2 is not available; specifically, insert the
guard just before the lines that set limitVelocityOverLifetime.enabled and
.separateAxes so the test does not silently no-op on WebGL1—use a clear early
exit such as throwing an Error("WebGL2 required for limit-velocity test") or
invoking the test runner's skip mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8fb17f70-130d-494e-add2-dc0b53b550af
📒 Files selected for processing (1)
e2e/case/particleRenderer-limitVelocity.ts
Comment on lines
+122
to
+123
| limitVelocityOverLifetime.enabled = true; | ||
| limitVelocityOverLifetime.separateAxes = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify module behavior: enabling is gated by WebGL2.
module_file="$(fd 'LimitVelocityOverLifetimeModule.ts$' | head -n1)"
case_file="$(fd 'particleRenderer-limitVelocity.ts$' | head -n1)"
echo "Module file: ${module_file}"
echo "Case file: ${case_file}"
rg -n -C3 'override set enabled|isWebGL2|_setTransformFeedback' "$module_file"
rg -n -C3 'limitVelocityOverLifetime\.enabled|isWebGL2' "$case_file"
# Optional: inspect similar E2E patterns for capability gating.
rg -n --type=ts -C2 'isWebGL2|TransformFeedback|limitVelocityOverLifetime' e2e/caseRepository: galacean/engine
Length of output: 2685
Fail fast when WebGL2 is unavailable so this test actually exercises limit-velocity.
The module's enabled setter silently returns without enabling on WebGL1 (it checks isWebGL2 and returns early). Add an explicit WebGL2 capability guard before line 122 to ensure the test fails or skips if WebGL2 is not available, rather than silently passing without covering the intended transform-feedback behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/case/particleRenderer-limitVelocity.ts` around lines 122 - 123, Before
enabling limitVelocityOverLifetime, add an explicit WebGL2 capability guard that
checks the renderer/context for WebGL2 (e.g. renderer.capabilities.isWebGL2 or
gl instanceof WebGL2RenderingContext) and fail or skip the test if WebGL2 is not
available; specifically, insert the guard just before the lines that set
limitVelocityOverLifetime.enabled and .separateAxes so the test does not
silently no-op on WebGL1—use a clear early exit such as throwing an
Error("WebGL2 required for limit-velocity test") or invoking the test runner's
skip mechanism.
GuoLei1990
approved these changes
Mar 17, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/core/src/particle/ParticleGenerator.ts (2)
979-1001:⚠️ Potential issue | 🟠 MajorRotate initial TF velocity for world simulation.
In world mode, Line 991-992 rotates/emits position in world space, but Line 998-1000 still uses local
direction. This produces incorrect initial motion when emitter rotation is non-identity.Suggested patch
private _addFeedbackParticle( index: number, shapePosition: Vector3, direction: Vector3, startSpeed: number, transform: Transform ): void { let position: Vector3; + let velocity: Vector3 = direction; if (this.main.simulationSpace === ParticleSimulationSpace.Local) { position = shapePosition; } else { position = ParticleGenerator._tempVector32; Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position); position.add(transform.worldPosition); + velocity = ParticleGenerator._tempVector31; + Vector3.transformByQuat(direction, transform.worldRotationQuaternion, velocity); } this._feedbackSimulator.writeParticleData( index, position, - direction.x * startSpeed, - direction.y * startSpeed, - direction.z * startSpeed + velocity.x * startSpeed, + velocity.y * startSpeed, + velocity.z * startSpeed ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleGenerator.ts` around lines 979 - 1001, The feedback particle's initial velocity isn't rotated into world space in ParticleGenerator._addFeedbackParticle when this.main.simulationSpace !== ParticleSimulationSpace.Local; keep the existing world-space position transform but also rotate the local direction by transform.worldRotationQuaternion (e.g., into a temporary Vector3) and use that rotated direction's x/y/z multiplied by startSpeed in the call to this._feedbackSimulator.writeParticleData so emitted velocities respect emitter rotation.
614-628:⚠️ Potential issue | 🟠 MajorBackfill alive particles when enabling TF at runtime.
Line 621 only resizes TF buffers. Existing alive slots are not copied into TF state, so the first TF step can read default position/velocity for already-active particles.
Suggested patch sketch
_setTransformFeedback(enabled: boolean): void { this._useTransformFeedback = enabled; if (enabled) { if (!this._feedbackSimulator) { this._feedbackSimulator = new ParticleTransformFeedbackSimulator(this._renderer.engine); } this._feedbackSimulator.resize(this._currentParticleCount, this._instanceVertexBufferBinding); + this._syncAliveParticlesToFeedback(); this._renderer.shaderData.enableMacro(ParticleGenerator._transformFeedbackMacro); } else { this._renderer.shaderData.disableMacro(ParticleGenerator._transformFeedbackMacro); } this._reorganizeGeometryBuffers(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleGenerator.ts` around lines 614 - 628, When enabling transform-feedback in _setTransformFeedback, after creating/resizing this._feedbackSimulator call a method to initialize/copy the current alive particle state from the instance vertex data into the TF buffers so the first TF step doesn't read default values; use this._instanceVertexBufferBinding and this._currentParticleCount as the source and add/invoke a method on ParticleTransformFeedbackSimulator (e.g. initializeFromInstanceBinding or copyFromInstanceBuffers) to transfer per-particle attributes before enabling the macro and before calling _reorganizeGeometryBuffers().packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (1)
84-104:⚠️ Potential issue | 🟠 MajorGuard TF state cleanup with
try/finally.Line 85 enters TF update, but if any draw on Line 95/Line 97/Line 99 throws, Line 103 is skipped and TF state is left unclosed.
Suggested patch
if ( !this._simulator.beginUpdate( shaderData, ParticleBufferUtils.feedbackVertexElements, this._instanceBinding, ParticleBufferUtils.feedbackInstanceElements ) ) return; - if (firstActive < firstFree) { - this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive); - } else { - this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive); - if (firstFree > 0) { - this._simulator.draw(MeshTopology.Points, 0, firstFree); + try { + if (firstActive < firstFree) { + this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive); + } else { + this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive); + if (firstFree > 0) { + this._simulator.draw(MeshTopology.Points, 0, firstFree); + } } + } finally { + this._simulator.endUpdate(); } - - this._simulator.endUpdate();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around lines 84 - 104, The TF (transform feedback) update calls into this._simulator beginUpdate/draw/endUpdate must be wrapped so endUpdate always runs even if a draw throws: capture the boolean result of this._simulator.beginUpdate(...) into a local (e.g., began), and if began then execute the draw logic inside a try block and call this._simulator.endUpdate() in a finally block; do not call endUpdate when beginUpdate returned false. Update the code around _simulator.beginUpdate / _simulator.draw / _simulator.endUpdate to use this try/finally pattern so TF state is always cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 979-1001: The feedback particle's initial velocity isn't rotated
into world space in ParticleGenerator._addFeedbackParticle when
this.main.simulationSpace !== ParticleSimulationSpace.Local; keep the existing
world-space position transform but also rotate the local direction by
transform.worldRotationQuaternion (e.g., into a temporary Vector3) and use that
rotated direction's x/y/z multiplied by startSpeed in the call to
this._feedbackSimulator.writeParticleData so emitted velocities respect emitter
rotation.
- Around line 614-628: When enabling transform-feedback in
_setTransformFeedback, after creating/resizing this._feedbackSimulator call a
method to initialize/copy the current alive particle state from the instance
vertex data into the TF buffers so the first TF step doesn't read default
values; use this._instanceVertexBufferBinding and this._currentParticleCount as
the source and add/invoke a method on ParticleTransformFeedbackSimulator (e.g.
initializeFromInstanceBinding or copyFromInstanceBuffers) to transfer
per-particle attributes before enabling the macro and before calling
_reorganizeGeometryBuffers().
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 84-104: The TF (transform feedback) update calls into
this._simulator beginUpdate/draw/endUpdate must be wrapped so endUpdate always
runs even if a draw throws: capture the boolean result of
this._simulator.beginUpdate(...) into a local (e.g., began), and if began then
execute the draw logic inside a try block and call this._simulator.endUpdate()
in a finally block; do not call endUpdate when beginUpdate returned false.
Update the code around _simulator.beginUpdate / _simulator.draw /
_simulator.endUpdate to use this try/finally pattern so TF state is always
cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f9965229-ef8a-46c0-9911-ab36dbc8e709
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Particle_particleRenderer-force.jpgis excluded by!**/*.jpgpackages/core/src/shaderlib/extra/particle.vs.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
packages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/ParticleTransformFeedbackSimulator.ts
packages/core/src/shaderlib/particle/particle_feedback_simulation.glsl
Outdated
Show resolved
Hide resolved
| feedbackVaryings: string[] | ||
| ) { | ||
| this._engine = engine; | ||
| this._primitive = new TransformFeedbackPrimitive(engine, byteStride); |
Collaborator
There was a problem hiding this comment.
P1 这条 Transform Feedback 路径还没有真正接进设备恢复链路。TransformFeedbackSimulator 自己维护了一套独立的 ShaderProgramPool，不会随着 devicerestored 一起清空；恢复后 beginUpdate() 可能继续复用底层 GL program 已失效的 ShaderProgram 包装对象，而 GLTransformFeedbackPrimitive.updateVertexLayout() 又只在 program.id 变化时才重建 VAO。另外，feedback ping-pong buffer 恢复后是空的，但粒子路径只会重传 instance buffer，不会恢复已存活粒子的当前位置/速度。既然引擎显式支持 context lost/restore，这条 TF 模拟路径需要明确的 invalidate/恢复策略。
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui/src/component/interactive/UIInteractive.ts (1)
21-22:⚠️ Potential issue | 🔴 CriticalMark
_rootCanvasand_groupwith@ignoreClonedecorator to prevent reference to original objects on clone.These fields lack
@ignoreClonedecorators and will be cloned by reference. Since UICanvas and UIGroup do not define_remapmethods, the cloned component will retain references to the original canvas and group. Additionally, the corresponding dirty flags (_isRootCanvasDirty,_isGroupDirty) are marked with@ignoreCloneand initialize tofalse, preventing automatic re-discovery viaUtils.searchRootCanvasInParentsandUtils.searchGroupInParentsin the getter methods. Add@ignoreCloneto these field declarations so they're rediscovered on the cloned instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/component/interactive/UIInteractive.ts` around lines 21 - 22, In UIInteractive, mark the fields _rootCanvas and _group with the `@ignoreClone` decorator (matching the existing `@ignoreClone` on _isRootCanvasDirty and _isGroupDirty) so cloned instances do not retain references to the original UICanvas/UIGroup; this ensures the boolean flags (_isRootCanvasDirty, _isGroupDirty) remain false and the getters will trigger re-discovery via Utils.searchRootCanvasInParents and Utils.searchGroupInParents instead of pointing to the original objects.packages/ui/src/component/UICanvas.ts (1)
48-49:⚠️ Potential issue | 🟠 MajorRestore
@ignoreCloneon_cameraObserverfield.The
_cameraObserverfield is derived state calculated from_camera,_isRootCanvas, and_renderMode. Removing@ignoreClonecauses it to be remapped as a component reference during cloning. This breaks state consistency: if the source canvas'srenderModeis the defaultWorldSpaceand remainsWorldSpaceafter cloning, the_updateCameraObserver()method is never invoked (setter only triggers on change). The cloned canvas retains a reference to the remapped camera without registering the necessary event listeners (_onCameraTransformListenerand_onCameraModifyListener), creating stale state that can cause incorrect listener management later.Also applies to: lines 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/component/UICanvas.ts` around lines 48 - 49, Re-add the `@ignoreClone` annotation to the _cameraObserver field so it is not treated as a remapped component reference during cloning; this ensures cloned canvases keep derived state and trigger _updateCameraObserver() properly instead of inheriting a remapped camera reference without listeners. Specifically, mark the _cameraObserver declaration with `@ignoreClone` and do the same for the related field referenced at lines 81-82 (also a derived/internal field) so cloning does not remap those component references; keep _camera, _isRootCanvas and _renderMode unchanged and rely on _updateCameraObserver() to register _onCameraTransformListener and _onCameraModifyListener after clone initialization.
♻️ Duplicate comments (2)
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (1)
84-104:⚠️ Potential issue | 🟠 MajorMake TF update exception-safe with
try/finally.If any
draw()call throws afterbeginUpdate()succeeds,endUpdate()is skipped. This leaves GL state corrupted (rasterizer discard enabled, transform feedback bound), which would break subsequent rendering.Suggested fix
if ( !this._simulator.beginUpdate( shaderData, ParticleBufferUtils.feedbackVertexElements, this._instanceBinding, ParticleBufferUtils.feedbackInstanceElements ) ) return; - if (firstActive < firstFree) { - this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive); - } else { - this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive); - if (firstFree > 0) { - this._simulator.draw(MeshTopology.Points, 0, firstFree); + try { + if (firstActive < firstFree) { + this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive); + } else { + this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive); + if (firstFree > 0) { + this._simulator.draw(MeshTopology.Points, 0, firstFree); + } } + } finally { + this._simulator.endUpdate(); } - - this._simulator.endUpdate();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around lines 84 - 104, The code calls this._simulator.beginUpdate(...), then performs one or more this._simulator.draw(...) calls and finally this._simulator.endUpdate(); make the update exception-safe by wrapping the draw logic in a try/finally so that this._simulator.endUpdate() is always invoked even if any draw() throws; locate the beginUpdate / draw / endUpdate calls in ParticleTransformFeedbackSimulator and move the draw branches into the try block with endUpdate() in the finally block to guarantee GL state is cleaned up.packages/core/src/particle/ParticleGenerator.ts (1)
978-1001:⚠️ Potential issue | 🟠 MajorRotate initial TF velocity in world simulation mode.
Position is correctly transformed to world space (Lines 989-991), but velocity still uses local-space
direction. This causes incorrect initial motion when the emitter has non-identity rotation.Suggested fix
private _addFeedbackParticle( index: number, shapePosition: Vector3, direction: Vector3, startSpeed: number, transform: Transform ): void { let position: Vector3; + let velocity = direction; if (this.main.simulationSpace === ParticleSimulationSpace.Local) { position = shapePosition; } else { position = ParticleGenerator._tempVector32; Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position); position.add(transform.worldPosition); + velocity = ParticleGenerator._tempVector31; + Vector3.transformByQuat(direction, transform.worldRotationQuaternion, velocity); } this._feedbackSimulator.writeParticleData( index, position, - direction.x * startSpeed, - direction.y * startSpeed, - direction.z * startSpeed + velocity.x * startSpeed, + velocity.y * startSpeed, + velocity.z * startSpeed ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/particle/ParticleGenerator.ts` around lines 978 - 1001, The initial velocity isn't rotated into world space in ParticleGenerator._addFeedbackParticle when this.main.simulationSpace !== ParticleSimulationSpace.Local; rotate the local-space direction by transform.worldRotationQuaternion before scaling by startSpeed and passing components to this._feedbackSimulator.writeParticleData. Use a temp vector (don't overwrite shapePosition temp used for position—e.g., a dedicated temp like ParticleGenerator._tempVector33 or a new local Vector3), call Vector3.transformByQuat(direction, transform.worldRotationQuaternion, rotatedDir), multiply rotatedDir by startSpeed, then pass rotatedDir.x/y/z to writeParticleData instead of direction.x/y/z.
🧹 Nitpick comments (1)
packages/core/src/Transform.ts (1)
54-54: Verify intentional removal of@ignoreClonefrom_parentTransformCache.The
@ignoreClonedecorator was removed from_parentTransformCache. While_isParentDirty(line 53) is still@ignoreCloneand defaults totrue, ensuring the cache is recalculated on first access, the removal allows the cached parent reference to be processed during cloning.Since Transform extends Component and has
_remap, the reference should be correctly remapped if the parent is within the cloned subtree. However, this adds unnecessary cloning work since_isParentDirty = truewill force recalculation anyway.Consider keeping
@ignoreCloneon_parentTransformCacheto avoid unnecessary remapping overhead:+ `@ignoreClone` private _parentTransformCache: Transform = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Transform.ts` at line 54, The _parentTransformCache field lost its `@ignoreClone` decorator allowing it to be processed during cloning and causing unnecessary remap work even though _isParentDirty (still `@ignoreClone`) will force recompute; restore the `@ignoreClone` decorator on the _parentTransformCache property in the Transform class so the cached reference is skipped by the clone machinery (keeping _isParentDirty true ensures recalculation) and mention/verify that _remap on Component still correctly handles cases where the parent is inside the cloned subtree.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/clone/CloneManager.ts`:
- Around line 140-159: The array branch fails to register the source array in
deepInstanceMap and reuses the parent cloneMode for every element; before
iterating set deepInstanceMap.set(sourceProperty, targetPropertyA) to
register/reuse shared or self-referential arrays, and compute an
element-specific clone mode rather than reusing the parent cloneMode when
calling CloneManager.cloneProperty (e.g., derive elementCloneMode from the
array's own clone metadata or a dedicated helper and pass that to
CloneManager.cloneProperty instead of cloneMode).
- Around line 175-181: The recursive clone call in CloneManager is not
propagating a parent Deep setting into undecorated nested fields, so in
CloneManager.cloneProperty (called from the loop using
sourceProperty/targetProperty and cloneModes[k]) update the logic to derive an
effective mode: if the parent field's mode (from
CloneManager.getCloneMode(sourceProperty.constructor) or the caller's current
mode) is CloneMode.Deep and cloneModes[k] is undefined, treat the effective mode
as CloneMode.Deep before calling CloneManager.cloneProperty; ensure the
effectiveMode (not just cloneModes[k]) is passed down into recursive calls so
nested plain objects/arrays inherit Deep when their parent was annotated.
- Around line 161-188: The code currently continues to recurse even when
deepInstanceMap already contains sourceProperty, causing duplicate traversal and
infinite recursion on cycles; modify the block in CloneManager.cloneProperty so
that after fetching targetProperty = deepInstanceMap.get(sourceProperty) and
assigning target[k] = targetProperty you immediately return (or otherwise skip
the subsequent copy/iteration logic) instead of proceeding to call copyFrom,
iterate properties, or invoke _cloneTo; this ensures when deepInstanceMap has
sourceProperty you reuse the mapped target instance and do not walk the same
source object again.
In `@packages/math/src/CollisionUtil.ts`:
- Around line 148-156: The JSDoc for intersectsRayAndPlane (in CollisionUtil.ts)
and the Ray return description (in Ray.ts) must be updated to state that when a
ray is parallel to the plane but its origin lies on the plane the function
returns 0 (intersection at origin) instead of -1; update the comments to
explicitly describe the parallel-on-plane edge case and that -1 is returned only
when there is no intersection. Also verify downstream behavior in XRHitTest.ts
(the if (distance >= 0) check) and UIRenderer.ts (the if (curDistance >= 0 &&
curDistance < distance) check) to ensure treating parallel-on-plane as a valid
hit at distance 0 is intended—either leave as-is if intended or adjust those
conditional checks to exclude parallel-origin cases, and add/update
unit/integration tests to cover this scenario.
In `@packages/ui/src/component/advanced/Text.ts`:
- Around line 270-275: Remove the `@assignmentClone` decorator from the _font
field and mark it `@ignoreClone` like _subFont, then rely on the existing override
_cloneTo(target: Text) to assign via target.font = this._font so the font setter
runs and reference counting occurs; specifically update the _font declaration to
use `@ignoreClone` (remove `@assignmentClone`) and keep the _cloneTo method as-is so
the font setter's guard (in the font property/method) increments/decrements refs
correctly during cloning.
---
Outside diff comments:
In `@packages/ui/src/component/interactive/UIInteractive.ts`:
- Around line 21-22: In UIInteractive, mark the fields _rootCanvas and _group
with the `@ignoreClone` decorator (matching the existing `@ignoreClone` on
_isRootCanvasDirty and _isGroupDirty) so cloned instances do not retain
references to the original UICanvas/UIGroup; this ensures the boolean flags
(_isRootCanvasDirty, _isGroupDirty) remain false and the getters will trigger
re-discovery via Utils.searchRootCanvasInParents and Utils.searchGroupInParents
instead of pointing to the original objects.
In `@packages/ui/src/component/UICanvas.ts`:
- Around line 48-49: Re-add the `@ignoreClone` annotation to the _cameraObserver
field so it is not treated as a remapped component reference during cloning;
this ensures cloned canvases keep derived state and trigger
_updateCameraObserver() properly instead of inheriting a remapped camera
reference without listeners. Specifically, mark the _cameraObserver declaration
with `@ignoreClone` and do the same for the related field referenced at lines
81-82 (also a derived/internal field) so cloning does not remap those component
references; keep _camera, _isRootCanvas and _renderMode unchanged and rely on
_updateCameraObserver() to register _onCameraTransformListener and
_onCameraModifyListener after clone initialization.
---
Duplicate comments:
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 978-1001: The initial velocity isn't rotated into world space in
ParticleGenerator._addFeedbackParticle when this.main.simulationSpace !==
ParticleSimulationSpace.Local; rotate the local-space direction by
transform.worldRotationQuaternion before scaling by startSpeed and passing
components to this._feedbackSimulator.writeParticleData. Use a temp vector
(don't overwrite shapePosition temp used for position—e.g., a dedicated temp
like ParticleGenerator._tempVector33 or a new local Vector3), call
Vector3.transformByQuat(direction, transform.worldRotationQuaternion,
rotatedDir), multiply rotatedDir by startSpeed, then pass rotatedDir.x/y/z to
writeParticleData instead of direction.x/y/z.
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 84-104: The code calls this._simulator.beginUpdate(...), then
performs one or more this._simulator.draw(...) calls and finally
this._simulator.endUpdate(); make the update exception-safe by wrapping the draw
logic in a try/finally so that this._simulator.endUpdate() is always invoked
even if any draw() throws; locate the beginUpdate / draw / endUpdate calls in
ParticleTransformFeedbackSimulator and move the draw branches into the try block
with endUpdate() in the finally block to guarantee GL state is cleaned up.
---
Nitpick comments:
In `@packages/core/src/Transform.ts`:
- Line 54: The _parentTransformCache field lost its `@ignoreClone` decorator
allowing it to be processed during cloning and causing unnecessary remap work
even though _isParentDirty (still `@ignoreClone`) will force recompute; restore
the `@ignoreClone` decorator on the _parentTransformCache property in the
Transform class so the cached reference is skipped by the clone machinery
(keeping _isParentDirty true ensures recalculation) and mention/verify that
_remap on Component still correctly handles cases where the parent is inside the
cloned subtree.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b210b67-5995-421c-8173-24d724de3a3b
⛔ Files ignored due to path filters (1)
packages/core/src/shaderlib/particle/particle_feedback_simulation.glslis excluded by!**/*.glsl
📒 Files selected for processing (51)
e2e/package.jsonexamples/package.jsonpackage.jsonpackages/core/package.jsonpackages/core/src/2d/sprite/SpriteMask.tspackages/core/src/2d/sprite/SpriteRenderer.tspackages/core/src/2d/text/TextRenderer.tspackages/core/src/Camera.tspackages/core/src/Component.tspackages/core/src/Entity.tspackages/core/src/Renderer.tspackages/core/src/Transform.tspackages/core/src/animation/Animator.tspackages/core/src/audio/AudioSource.tspackages/core/src/clone/CloneManager.tspackages/core/src/clone/CloneUtils.tspackages/core/src/clone/ComponentCloner.tspackages/core/src/graphic/Buffer.tspackages/core/src/graphic/TransformFeedbackPrimitive.tspackages/core/src/mesh/MeshRenderer.tspackages/core/src/mesh/Skin.tspackages/core/src/mesh/SkinnedMeshRenderer.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/ParticleTransformFeedbackSimulator.tspackages/core/src/particle/modules/shape/MeshShape.tspackages/core/src/physics/shape/ColliderShape.tspackages/core/src/renderingHardwareInterface/IPlatformBuffer.tspackages/design/package.jsonpackages/galacean/package.jsonpackages/loader/package.jsonpackages/math/package.jsonpackages/math/src/CollisionUtil.tspackages/physics-lite/package.jsonpackages/physics-physx/package.jsonpackages/rhi-webgl/package.jsonpackages/rhi-webgl/src/GLBuffer.tspackages/shader-lab/package.jsonpackages/shader/package.jsonpackages/ui/package.jsonpackages/ui/src/component/UICanvas.tspackages/ui/src/component/UIGroup.tspackages/ui/src/component/UIRenderer.tspackages/ui/src/component/UITransform.tspackages/ui/src/component/advanced/Button.tspackages/ui/src/component/advanced/Image.tspackages/ui/src/component/advanced/Text.tspackages/ui/src/component/interactive/UIInteractive.tspackages/xr-webxr/package.jsonpackages/xr/package.jsontests/package.jsontests/src/core/CloneUtils.test.ts
💤 Files with no reviewable changes (3)
- packages/core/src/physics/shape/ColliderShape.ts
- packages/ui/src/component/UIRenderer.ts
- packages/ui/src/component/UIGroup.ts
✅ Files skipped from review due to trivial changes (14)
- packages/shader-lab/package.json
- packages/shader/package.json
- examples/package.json
- packages/physics-physx/package.json
- packages/xr/package.json
- packages/loader/package.json
- tests/package.json
- e2e/package.json
- packages/physics-lite/package.json
- packages/design/package.json
- packages/galacean/package.json
- package.json
- packages/core/package.json
- packages/rhi-webgl/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rhi-webgl/src/GLBuffer.ts
- packages/core/src/graphic/Buffer.ts
Comment on lines
+175
to
+181
| const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor); | ||
| for (let k in sourceProperty) { | ||
| CloneManager.cloneProperty( | ||
| <Array<any>>sourceProperty, | ||
| targetPropertyA, | ||
| i, | ||
| cloneMode, | ||
| <Object>sourceProperty, | ||
| targetProperty, | ||
| k, | ||
| cloneModes[k], |
There was a problem hiding this comment.
Propagate CloneMode.Deep into undecorated nested fields.
The recursive call only uses cloneModes[k]. For a @deepClone plain object, nested plain objects/arrays have no field metadata, so they fall back to cloneMode === undefined and get assigned by reference. That makes deep-clone effectively one level deep unless every nested field is separately decorated.
Possible fix
- cloneModes[k],
+ cloneModes[k] ?? (cloneMode === CloneMode.Deep ? CloneMode.Deep : undefined),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/clone/CloneManager.ts` around lines 175 - 181, The
recursive clone call in CloneManager is not propagating a parent Deep setting
into undecorated nested fields, so in CloneManager.cloneProperty (called from
the loop using sourceProperty/targetProperty and cloneModes[k]) update the logic
to derive an effective mode: if the parent field's mode (from
CloneManager.getCloneMode(sourceProperty.constructor) or the caller's current
mode) is CloneMode.Deep and cloneModes[k] is undefined, treat the effective mode
as CloneMode.Deep before calling CloneManager.cloneProperty; ensure the
effectiveMode (not just cloneModes[k]) is passed down into recursive calls so
nested plain objects/arrays inherit Deep when their parent was annotated.
zhuxudong
reviewed
Mar 18, 2026
| gl.bindBufferBase(gl.TRANSFORM_FEEDBACK_BUFFER, index, null); | ||
| } | ||
|
|
||
| begin(primitiveMode: number): void { |
Member
There was a problem hiding this comment.
P2: begin(primitiveMode: number) 直接将引擎的 MeshTopology 枚举值传给
gl.beginTransformFeedback()。需要确认 MeshTopology 枚举值与 GL 常量对齐（MeshTopology.Points = 0 ===
gl.POINTS = 0）。
Member
There was a problem hiding this comment.
WebGL 常量：gl.POINTS = 0, gl.LINES = 1, gl.LINE_STRIP = 3, gl.TRIANGLES = 4, gl.TRIANGLE_STRIP = 5, gl.TRIANGLE_FAN = 6。
完全对齐，不存在这个问题。
| // Transform Feedback: render all slots (instance buffer not compacted, dead particles discarded in shader) | ||
| // Non-Transform Feedback: render only alive particles (instance buffer compacted) | ||
| generator._primitive.instanceCount = generator._useTransformFeedback | ||
| ? generator._currentParticleCount |
Member
There was a problem hiding this comment.
P1: ParticleRenderer._render() 中 instanceCount 设为 _currentParticleCount 存在性能浪费
ParticleRenderer.ts:236-239：TF 模式下 instanceCount 设为全部 slot 数而非 alive 数量，依赖 shader 中 normalizedAge >= 1.0 来 discard 死粒子。这意味着 大量死粒子 slot 也会执行完整的顶点着色，只是最终被 discard。
| * Platform interface for Transform Feedback operations. | ||
| * @internal | ||
| */ | ||
| export interface IPlatformTransformFeedback { |
Member
There was a problem hiding this comment.
增加 pauseTransformFeedback() / resumeTransformFeedback()
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/shader/ShaderPass.ts (1)
179-186: Minor formatting inconsistency in template literals.The ShaderLab path has a leading space before the version string (
` ${versionStr}), whileShaderFactory.compilePlatformSourceuses`${versionStr}without the leading space. Although GLSL allows whitespace before#version, aligning the formatting would improve consistency.Additionally, the vertex shader here doesn't include an explicit
precision highp float;declaration, unlike the platform source path. While GLSL ES vertex shaders default tohighpfor floats, verify that ShaderLab-parsed shaders include their own precision declarations.🔧 Suggested formatting alignment
- const vertexSource = ` ${versionStr} - ${noIncludeVertex} - `; - const fragmentSource = ` ${versionStr} - ${isWebGL2 ? "" : ShaderFactory._shaderExtension} - ${precisionStr} - ${noIncludeFrag} - `; + const vertexSource = `${versionStr} +${noIncludeVertex}`; + const fragmentSource = `${versionStr} +${isWebGL2 ? "" : ShaderFactory._shaderExtension}${precisionStr} +${noIncludeFrag}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shader/ShaderPass.ts` around lines 179 - 186, In ShaderPass.ts adjust the template literals for vertexSource and fragmentSource to match the formatting used by ShaderFactory.compilePlatformSource by removing the leading space before ${versionStr} (i.e., change ` ${versionStr}` to `${versionStr}`) and ensure the vertex shader includes an explicit precision declaration similar to fragmentSource (add `precision highp float;` when generating vertexSource for GLSL ES or otherwise confirm ShaderLab-parsed shaders provide that declaration); update the variables vertexSource and fragmentSource accordingly so formatting and precision handling are consistent with ShaderFactory.compilePlatformSource and ShaderLab expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts`:
- Around line 58-80: The resize method unconditionally sets this._readIsA = true
which can flip the active read buffer after resizing; capture the current read
state before reallocating (e.g. const prevReadIsA = this._readIsA) and restore
it after creating the new VertexBufferBinding objects (this._readIsA =
prevReadIsA), or alternatively when copying choose the copy direction based on
prevReadIsA (copy the currently-read old buffer into the new read buffer) so the
active read buffer remains unchanged; modify resize to use prevReadIsA and
update bufferA/bufferB copies or restore the flag accordingly using the existing
symbols resize, _readIsA, oldBindingA, oldBindingB, bufferA, bufferB, and
VertexBufferBinding.
- Around line 121-124: In TransformFeedbackPrimitive.draw, guard against
zero/negative capture sizes by returning early when count <= 0 before calling
this._transformFeedback.bindBufferRange and begin; specifically, in the
draw(mode: MeshTopology, first: number, count: number) method add an early check
(if (count <= 0) return;) so you do not call bindBufferRange(...) or begin(...)
with a zero-sized range.
---
Nitpick comments:
In `@packages/core/src/shader/ShaderPass.ts`:
- Around line 179-186: In ShaderPass.ts adjust the template literals for
vertexSource and fragmentSource to match the formatting used by
ShaderFactory.compilePlatformSource by removing the leading space before
${versionStr} (i.e., change ` ${versionStr}` to `${versionStr}`) and ensure the
vertex shader includes an explicit precision declaration similar to
fragmentSource (add `precision highp float;` when generating vertexSource for
GLSL ES or otherwise confirm ShaderLab-parsed shaders provide that declaration);
update the variables vertexSource and fragmentSource accordingly so formatting
and precision handling are consistent with ShaderFactory.compilePlatformSource
and ShaderLab expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7a6e18f-6f65-490f-8edc-88bd15c9dc34
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-limitVelocity.jpgis excluded by!**/*.jpg
📒 Files selected for processing (4)
packages/core/src/Engine.tspackages/core/src/graphic/TransformFeedbackPrimitive.tspackages/core/src/graphic/TransformFeedbackSimulator.tspackages/core/src/shader/ShaderPass.ts
Comment on lines
+58
to
+80
| resize(vertexCount: number): void { | ||
| const oldBindingA = this._bindingA; | ||
| const oldBindingB = this._bindingB; | ||
|
|
||
| const byteLength = this._byteStride * vertexCount; | ||
| const bufferA = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false); | ||
| bufferA.isGCIgnored = true; | ||
| const bufferB = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false); | ||
| bufferB.isGCIgnored = true; | ||
|
|
||
| // GPU copy old data to new buffers before destroying old ones | ||
| if (oldBindingA) { | ||
| const copyLength = Math.min(oldBindingA.buffer.byteLength, byteLength); | ||
| bufferA.copyFromBuffer(oldBindingA.buffer, 0, 0, copyLength); | ||
| bufferB.copyFromBuffer(oldBindingB.buffer, 0, 0, copyLength); | ||
| oldBindingA.buffer.destroy(); | ||
| oldBindingB.buffer.destroy(); | ||
| } | ||
|
|
||
| this._bindingA = new VertexBufferBinding(bufferA, this._byteStride); | ||
| this._bindingB = new VertexBufferBinding(bufferB, this._byteStride); | ||
| this._readIsA = true; | ||
| this._platformPrimitive.invalidate(); |
There was a problem hiding this comment.
Preserve active read state when resizing ping-pong buffers.
Line 79 always resets _readIsA to true after copying old A→new A and old B→new B. If resize occurs while _readIsA was false, the next frame reads stale state from old A (state rollback). Copy the current read buffer into both new buffers (or preserve previous _readIsA).
💡 Suggested fix
resize(vertexCount: number): void {
+ const wasReadA = this._readIsA;
const oldBindingA = this._bindingA;
const oldBindingB = this._bindingB;
const byteLength = this._byteStride * vertexCount;
const bufferA = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false);
bufferA.isGCIgnored = true;
const bufferB = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false);
bufferB.isGCIgnored = true;
// GPU copy old data to new buffers before destroying old ones
- if (oldBindingA) {
- const copyLength = Math.min(oldBindingA.buffer.byteLength, byteLength);
- bufferA.copyFromBuffer(oldBindingA.buffer, 0, 0, copyLength);
- bufferB.copyFromBuffer(oldBindingB.buffer, 0, 0, copyLength);
+ if (oldBindingA && oldBindingB) {
+ const sourceBuffer = wasReadA ? oldBindingA.buffer : oldBindingB.buffer;
+ const copyLength = Math.min(sourceBuffer.byteLength, byteLength);
+ bufferA.copyFromBuffer(sourceBuffer, 0, 0, copyLength);
+ bufferB.copyFromBuffer(sourceBuffer, 0, 0, copyLength);
oldBindingA.buffer.destroy();
oldBindingB.buffer.destroy();
}
this._bindingA = new VertexBufferBinding(bufferA, this._byteStride);
this._bindingB = new VertexBufferBinding(bufferB, this._byteStride);
this._readIsA = true;
this._platformPrimitive.invalidate();
}📝 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.
Suggested change
| resize(vertexCount: number): void { | |
| const oldBindingA = this._bindingA; | |
| const oldBindingB = this._bindingB; | |
| const byteLength = this._byteStride * vertexCount; | |
| const bufferA = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false); | |
| bufferA.isGCIgnored = true; | |
| const bufferB = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false); | |
| bufferB.isGCIgnored = true; | |
| // GPU copy old data to new buffers before destroying old ones | |
| if (oldBindingA) { | |
| const copyLength = Math.min(oldBindingA.buffer.byteLength, byteLength); | |
| bufferA.copyFromBuffer(oldBindingA.buffer, 0, 0, copyLength); | |
| bufferB.copyFromBuffer(oldBindingB.buffer, 0, 0, copyLength); | |
| oldBindingA.buffer.destroy(); | |
| oldBindingB.buffer.destroy(); | |
| } | |
| this._bindingA = new VertexBufferBinding(bufferA, this._byteStride); | |
| this._bindingB = new VertexBufferBinding(bufferB, this._byteStride); | |
| this._readIsA = true; | |
| this._platformPrimitive.invalidate(); | |
| resize(vertexCount: number): void { | |
| const wasReadA = this._readIsA; | |
| const oldBindingA = this._bindingA; | |
| const oldBindingB = this._bindingB; | |
| const byteLength = this._byteStride * vertexCount; | |
| const bufferA = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false); | |
| bufferA.isGCIgnored = true; | |
| const bufferB = new Buffer(this._engine, BufferBindFlag.VertexBuffer, byteLength, BufferUsage.Dynamic, false); | |
| bufferB.isGCIgnored = true; | |
| // GPU copy old data to new buffers before destroying old ones | |
| if (oldBindingA && oldBindingB) { | |
| const sourceBuffer = wasReadA ? oldBindingA.buffer : oldBindingB.buffer; | |
| const copyLength = Math.min(sourceBuffer.byteLength, byteLength); | |
| bufferA.copyFromBuffer(sourceBuffer, 0, 0, copyLength); | |
| bufferB.copyFromBuffer(sourceBuffer, 0, 0, copyLength); | |
| oldBindingA.buffer.destroy(); | |
| oldBindingB.buffer.destroy(); | |
| } | |
| this._bindingA = new VertexBufferBinding(bufferA, this._byteStride); | |
| this._bindingB = new VertexBufferBinding(bufferB, this._byteStride); | |
| this._readIsA = true; | |
| this._platformPrimitive.invalidate(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts` around lines 58 -
80, The resize method unconditionally sets this._readIsA = true which can flip
the active read buffer after resizing; capture the current read state before
reallocating (e.g. const prevReadIsA = this._readIsA) and restore it after
creating the new VertexBufferBinding objects (this._readIsA = prevReadIsA), or
alternatively when copying choose the copy direction based on prevReadIsA (copy
the currently-read old buffer into the new read buffer) so the active read
buffer remains unchanged; modify resize to use prevReadIsA and update
bufferA/bufferB copies or restore the flag accordingly using the existing
symbols resize, _readIsA, oldBindingA, oldBindingB, bufferA, bufferB, and
VertexBufferBinding.
Comment on lines
+121
to
+124
| draw(mode: MeshTopology, first: number, count: number): void { | ||
| const transformFeedback = this._transformFeedback; | ||
| transformFeedback.bindBufferRange(0, this.writeBinding.buffer, first * this._byteStride, count * this._byteStride); | ||
| transformFeedback.begin(mode); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WebGL2, is gl.bindBufferRange(gl.TRANSFORM_FEEDBACK_BUFFER, index, buffer, offset, size) valid when size is 0?
💡 Result:
No, gl.bindBufferRange(gl.TRANSFORM_FEEDBACK_BUFFER, index, buffer, offset, 0) is not valid in WebGL2. It generates a GL_INVALID_VALUE error.
Citations:
- 1: https://docs.gl/es3/glBindBufferRange
- 2: https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/bindBufferRange
- 3: https://registry.khronos.org/webgl/specs/latest/2.0/
🏁 Script executed:
fd TransformFeedbackPrimitive.tsRepository: galacean/engine
Length of output: 253
🏁 Script executed:
cat -n packages/core/src/graphic/TransformFeedbackPrimitive.ts | sed -n '110,135p'Repository: galacean/engine
Length of output: 1153
🏁 Script executed:
# Check if there are any guards against zero-count draws elsewhere in the file
rg "count\s*(?:<=|<)\s*0" packages/core/src/graphic/TransformFeedbackPrimitive.ts -A 2 -B 2Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check how this method is called to understand if count=0 is expected
rg "\.draw\(" packages/core/src/graphic/TransformFeedbackPrimitive.ts -B 3 -A 3Repository: galacean/engine
Length of output: 356
🏁 Script executed:
# Search for calls to draw() on TransformFeedbackPrimitive instances
rg "\.draw\s*\(" --type ts -B 2 -A 2 | head -50Repository: galacean/engine
Length of output: 2678
🏁 Script executed:
# Check if there are guards in calling code
rg "TransformFeedbackPrimitive" packages/core/src -A 5 -B 5 | grep -A 10 -B 10 "\.draw"Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check the interface/base class to understand contracts
cat -n packages/core/src/renderingHardwareInterface/IPlatformTransformFeedbackPrimitive.tsRepository: galacean/engine
Length of output: 1765
Guard zero-count TF draws before binding output range.
The draw() method at line 123 computes TF capture size as count * this._byteStride without validating count. When count ≤ 0, this binds a zero-sized range to the transform feedback buffer, triggering a GL_INVALID_VALUE error in WebGL2. Call sites in ParticleTransformFeedbackSimulator can pass zero counts in certain execution paths.
Add an early return guard before bindBufferRange():
Suggested fix
draw(mode: MeshTopology, first: number, count: number): void {
+ if (count <= 0) return;
const transformFeedback = this._transformFeedback;
transformFeedback.bindBufferRange(0, this.writeBinding.buffer, first * this._byteStride, count * this._byteStride);
transformFeedback.begin(mode);
this._platformPrimitive.draw(mode, first, count);
transformFeedback.end();
}📝 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.
Suggested change
| draw(mode: MeshTopology, first: number, count: number): void { | |
| const transformFeedback = this._transformFeedback; | |
| transformFeedback.bindBufferRange(0, this.writeBinding.buffer, first * this._byteStride, count * this._byteStride); | |
| transformFeedback.begin(mode); | |
| draw(mode: MeshTopology, first: number, count: number): void { | |
| if (count <= 0) return; | |
| const transformFeedback = this._transformFeedback; | |
| transformFeedback.bindBufferRange(0, this.writeBinding.buffer, first * this._byteStride, count * this._byteStride); | |
| transformFeedback.begin(mode); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts` around lines 121 -
124, In TransformFeedbackPrimitive.draw, guard against zero/negative capture
sizes by returning early when count <= 0 before calling
this._transformFeedback.bindBufferRange and begin; specifically, in the
draw(mode: MeshTopology, first: number, count: number) method add an early check
(if (count <= 0) return;) so you do not call bindBufferRange(...) or begin(...)
with a zero-sized range.
- Implement per-frame TF-based particle simulation matching Unity's ClampVelocityModule (dampen, drag, execution order, world space handling) - Add TransformFeedbackPrimitive for reusable ping-pong buffer/VAO management - Add TransformFeedback/GLTransformFeedback platform abstraction layer - Remove inaccurate 8-step approximation fallback (WebGL1 ignores module) - Optimize TF pass to only process alive particles via bindBufferRange - Optimize instance buffer upload with Discard mode for TF Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use VertexElement instead of custom TFAttributeDescriptor - Auto-detect VAO rebuild via _lastProgramId (no manual markVAODirty) - Extract attribute descriptors as static readonly arrays - Remove unnecessary comments and simplify code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix animatedVelocity composition: only VOL, not FOL/gravity - Fix gravity timing: add to base velocity before dampen/drag - Fix drag math: scale total then subtract animated (not scale base only) - Remove Unity references from comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…als in particle modules
- Unbind TF buffer from TRANSFORM_FEEDBACK_BUFFER target after TF draw to prevent conflict when render VAO rebuilds (e.g., VOL enabled at runtime) - Fix drag math: scale total velocity then subtract animated - Fix gravity: add to base velocity before dampen/drag - Fix animatedVelocity: only VOL, not FOL/gravity Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unify TransformFeedbackSimulator's ShaderProgramPool with Engine's global pool so device lost/restore correctly invalidates TF programs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Residual bindings on COPY_READ_BUFFER and COPY_WRITE_BUFFER caused GL_INVALID_OPERATION during transform feedback draw calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GuoLei1990
force-pushed
the
feat/limitParticle
branch
from
30af3da to
afa18a5
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When firstActive <= firstFree, use firstFree as instanceCount instead of _currentParticleCount, skipping trailing dead slots. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| return; | ||
| } | ||
| this._enabled = value; | ||
| this._generator._setTransformFeedback(value); |
Collaborator
There was a problem hiding this comment.
P1 clone 之后 TF 开关会和模块 enabled 状态失去同步。
这里 _setTransformFeedback() 只在 enabled setter 里触发，但 clone 流程是直接拷字段，不会重走 setter。当前 PR 里：
ParticleRenderer.generator是@deepCloneLimitVelocityOverLifetimeModule._enabled会被直接拷成trueParticleGenerator._useTransformFeedback和_feedbackSimulator都是@ignoreClone
最小复现：
const src = root.createChild("src");
const srcRenderer = src.addComponent(ParticleRenderer);
srcRenderer.generator.limitVelocityOverLifetime.enabled = true;
const clone = src.clone();
const cloneRenderer = clone.getComponent(ParticleRenderer);
console.log(cloneRenderer.generator.limitVelocityOverLifetime.enabled); // true
console.log(cloneRenderer.generator._useTransformFeedback); // false这样 clone 看起来启用了 LVL，但实际上不会走 TF 路径，除非之后再手动把 enabled 先切成 false 再切回 true。如果这条路径需要支持组件 clone，这里需要在 clone 后重放 TF 初始化副作用，或者避免把 enabled clone 成一个与运行时开关不一致的状态。
| // Another choice is just add new particles to vertex buffer and render all particles ignore the retired particle in shader, especially billboards | ||
| // But webgl don't support map buffer range, so this choice don't have performance advantage even less set data to GPU | ||
| if ( | ||
| const isContentLost = this._instanceVertexBufferBinding._buffer.isContentLost; |
Collaborator
There was a problem hiding this comment.
P2 content lost 时把粒子退休放在 world-volume bookkeeping 后面，可能导致旧 bounds 残留。
现在 lastAlive/isAlive 对比和 _onWorldVolumeChanged() 发生在前面，而 isContentLost 时真正把 _firstActiveElement/_firstFreeElement/... 清零在后面。这样如果 restore 发生在“系统已经 stop emitting，但场上还有活粒子”的时刻，这一帧会先按 alive=true 走完 bounds/world-volume 逻辑，再在后面把粒子变成 dead；renderer.bounds 之后就可能继续复用 restore 前缓存下来的包围盒。
最小复现：
const renderer = emitter.addComponent(ParticleRenderer);
const generator = renderer.generator;
generator.limitVelocityOverLifetime.enabled = true;
generator.main.playOnEnabled = false;
generator.main.isLoop = false;
generator.main.duration = 0.05;
generator.main.startLifetime = new ParticleCompositeCurve(5);
generator.emission.rateOverTime.constant = 0;
generator.emission.addBurst(new Burst(0, new ParticleCompositeCurve(1)));
generator.play(false);
for (let i = 0; i < 6; i++) engine.update(); // !generator._isPlaying && generator.isAlive
const staleBounds = renderer.bounds.clone();
engine._onDeviceLost();
engine._onDeviceRestored();
engine.update();
console.log(staleBounds);
console.log(renderer.bounds); // 这里仍可能是 restore 前的旧 bounds更稳的做法是把 content-loss retirement 提前到 alive/bounds bookkeeping 之前，或者在清空 ring-buffer 指针时显式调用 _onWorldVolumeChanged()。
Also add _cloneTo to sync TF state after deep clone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| if (!this._feedbackSimulator) { | ||
| this._feedbackSimulator = new ParticleTransformFeedbackSimulator(this._renderer.engine); | ||
| } | ||
| this._feedbackSimulator.resize(this._currentParticleCount, this._instanceVertexBufferBinding); |
Collaborator
There was a problem hiding this comment.
P1 这里重新 enable LVL 会重新分配一对 feedback buffer，但旧 buffer 没有被销毁。
关键链路是：_setTransformFeedback(true) 每次都会调用 this._feedbackSimulator.resize(...)，而 ParticleTransformFeedbackSimulator.resize() 会先把旧的 read/write buffer 暂存到 _oldReadBuffer/_oldWriteBuffer，再分配新的 buffer。正常 resize 路径后面还会走 copyOldBufferData() + destroyOldBuffers()，但这个 enable 路径不会。
关键复现代码：
const renderer = root.createChild("p").addComponent(ParticleRenderer);
const lvl = renderer.generator.limitVelocityOverLifetime;
const resourcePool = (renderer.engine.resourceManager as any)._graphicResourcePool;
const countGraphics = () => Object.keys(resourcePool).length;
lvl.enabled = true;
const sim = (renderer.generator as any)._feedbackSimulator;
const oldRead = sim.readBinding.buffer;
const before = countGraphics();
lvl.enabled = false;
lvl.enabled = true;
console.log(oldRead.destroyed); // false
console.log(before, countGraphics()); // 第二次 enable 后通常会多出 2 个 GraphicsResource因为这些 feedback buffers 都是 isGCIgnored = true，这里不是一次性的抖动，而是会随着反复开关模块持续泄漏 GPU buffer。更稳的做法是：如果粒子容量没变就复用现有 feedback buffer；如果必须重建，也要在 enable 路径里显式销毁旧的那一对 buffer。
| ) { | ||
| this._engine = engine; | ||
| this._primitive = new TransformFeedbackPrimitive(engine, byteStride); | ||
| this._simulatorId = ShaderPass._shaderPassCounter++; |
Collaborator
There was a problem hiding this comment.
P2 这里会导致 TF shader program 按粒子系统实例重复编译/缓存，而不是按 shader 内容共享。
TransformFeedbackSimulator 每创建一个实例都会分配一个新的 _simulatorId，后面又用这个 id 去索引 engine._getShaderProgramPool(this._simulatorId)。对于粒子路径来说，TF 的 vertex source / fragment source / feedback varyings 在不同 emitter 之间通常是相同的，所以多个相同 LVL 粒子系统不会共享 program pool。
关键复现代码：
const r1 = root.createChild("a").addComponent(ParticleRenderer);
const r2 = root.createChild("b").addComponent(ParticleRenderer);
r1.generator.limitVelocityOverLifetime.enabled = true;
r2.generator.limitVelocityOverLifetime.enabled = true;
const s1 = (r1.generator as any)._feedbackSimulator._simulatorId;
const s2 = (r2.generator as any)._feedbackSimulator._simulatorId;
console.log(s1 === s2); // false
console.log((engine as any)._shaderProgramPools[s1] === (engine as any)._shaderProgramPools[s2]); // false也就是说，两个配置完全相同的 TF emitter 仍然会各自维护一份独立的 program pool，并在第一次 update 时各自编译同一套 TF program。这个问题不会造成功能错误，但会把 shader compile stall 和 GL program 内存占用按 emitter 数量线性放大。
Only resize feedback buffers on first creation, reuse existing buffers on subsequent enable toggles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract TransformFeedbackShader as shared shader definition registered in ShaderPool. Multiple LVL particle systems now share a single program pool instead of compiling the same shader per emitter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resize checks byteLength before allocating new buffers. This fixes both the leak (no resize when capacity unchanged) and the capacity desync (resize when capacity changed while LVL was disabled). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the resize-needed check from ParticleTransformFeedbackSimulator.resize() up to ParticleGenerator to avoid unnecessary buffer re-creation when the existing feedback buffer already has the correct capacity. When no resize is needed, only update the instance binding reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a_Random2.w was only written when limit velocity was in random mode, but the drag random branch in the TF shader also reads this component. Now also write it when drag.mode is TwoConstants or TwoCurves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…OverLifetimeModule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… state - Clamp TF simulation dt to min(renderer_DeltaTime, age) so particles emitted mid-frame only integrate their actual elapsed time on the first TF pass, matching the analytical path - Extract _clearActiveParticles() helper from StopEmittingAndClear logic - Call _clearActiveParticles() when switching TF mode (LVL enabled/disabled) to avoid one-frame position jump caused by incompatible buffer state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Direct array assignment bypassed _bufferStructChanged, causing WebGL2 to reuse the stale VAO and render from the previous feedback buffer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
VAO caches GL buffer handles at build time. Since the feedback buffer swaps every frame, a cached VAO would always read the buffer from the frame it was first registered — causing the render pass to lag one frame behind the simulation. Disabling VAO in TF mode falls back to per-frame bindBufferAndAttrib, which is cheaper than rebuilding the VAO each frame. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_updateFeedback was receiving raw engine.time.deltaTime while _playTime advances by elapsedTime * simulationSpeed, causing gravity, drag, dampen and position integration to run at the wrong rate when simulationSpeed != 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Content lost reset was clearing the particle ring buffer but leaving _firstActiveTransformedBoundingBox untouched, causing stale world-space bounds to persist until they naturally expired. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_clearActiveParticles only reset the particle ring buffer but left _firstActiveTransformedBoundingBox untouched. This caused stale world-space bounds to persist after TF mode switches and StopEmittingAndClear until they naturally expired via _retireTransformedBounds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概述
为粒子系统新增 LimitVelocityOverLifetime 模块，通过 WebGL2 Transform Feedback 实现 GPU 端逐帧速度限制（dampen）和阻力（drag）模拟。
架构设计
三层 Transform Feedback 架构：
TransformFeedback— WebGL2 TF 对象封装（bind/unbind/begin/end）TransformFeedbackPrimitive— ping-pong 双缓冲 + VAO 管理TransformFeedbackSimulator— 通用 TF 模拟器（shader 编译、program 缓存、simulate 调度）ParticleTransformFeedbackSimulator— 粒子专用，继承通用模拟器平台抽象：
IPlatformTransformFeedback/IPlatformTransformFeedbackPrimitive— 核心接口GLTransformFeedback/GLTransformFeedbackPrimitive— WebGL2 实现TF Shader 算法（particle_feedback_simulation.glsl）
每帧对每个存活粒子执行四步：
drag * dt，支持 multiplyBySize / multiplyByVelocityBug 修复
curVOL - prevVOL计算，常量模式下结果为零RENDERER_LVL_DRAG_IS_RANDOM_TWO宏保护其他改动
ShaderPass.ts— 拆分 ShaderLab / 非 ShaderLab 编译路径，使手写 GLSL（TF shader）能正确编译ShaderFactory.ts— 新增compilePlatformSource统一非 ShaderLab shader 编译流程getStartPosition/u_DragConstant），drag 现在仅在 TF 模式下生效Summary by CodeRabbit
New Features
Tests
Chores