Support trail render #2873
Support trail render #2873
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the mesh-based TrailRenderer with a Renderer-driven circular-buffer trail system, adds EffectMaterial and migrates TrailMaterial/ParticleMaterial to extend it, registers a new "trail" shader and TrailTextureMode enum, adds an e2e Trail example, and introduces unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Entity as Entity
participant Move as TrailMoveScript
participant TrailR as TrailRenderer
participant Shader as ShaderPool/Shader
participant GPU as GPU
rect rgba(250,250,255,0.5)
Note over App,Entity: Initialization
App->>Entity: create entity & attach TrailRenderer
App->>TrailR: configure widthCurve, colorGradient, texture, emitting
TrailR->>Shader: Shader.find("trail") → prepare shaderData
end
rect rgba(245,255,245,0.5)
Note over Move,TrailR: Per-frame update & emission
App->>Move: update(dt)
Move->>Entity: update transform (position)
Entity->>TrailR: report world position
TrailR->>TrailR: emit/retire points, manage ring-buffer & bridge
TrailR->>Shader: upload uniforms/arrays (time, widthCurve, color/alpha keys)
Shader->>GPU: draw calls for sub-primitives (main + wrap)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 @@
## main #2873 +/- ##
==========================================
+ Coverage 78.41% 78.57% +0.15%
==========================================
Files 867 870 +3
Lines 94394 95002 +608
Branches 9399 9485 +86
==========================================
+ Hits 74022 74646 +624
+ Misses 20212 20196 -16
Partials 160 160
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: 1
🧹 Nitpick comments (5)
e2e/case/trailRenderer-basic.ts (1)
22-24: Consider adding error handling for engine creation.The
WebGLEngine.create()promise lacks a.catch()handler. For an e2e example this is minor, but adding error handling would make failures more diagnosable.🔎 Proposed fix
WebGLEngine.create({ canvas: "canvas" -}).then((engine) => { +}).then((engine) => { + // ... existing code ... +}).catch((error) => { + console.error("Failed to create WebGL engine:", error); +});packages/core/src/trail/TrailRenderer.ts (4)
52-88: Consider adding property validation for safety.Public properties like
time,width, andminVertexDistanceaccept any number without validation. Negative or zero values could cause issues (e.g., division by zero, infinite loops). While acceptable for initial implementation, consider adding guards.
462-520: Per-frame Float32Array allocations could cause GC pressure.Lines 480, 487, 493 create new
Float32Arrayviews each frame. While views are lightweight, in performance-critical rendering paths, consider caching these views or using a different upload strategy.
526-548: Use named constants instead of magic numbers for curve modes.The values
0and2representParticleCompositeCurvemodes. Using named constants or an enum would improve readability and maintainability.- if (curve.mode === 0) { + if (curve.mode === ParticleCurveMode.Constant) { // Constant mode ... - } else if (curve.mode === 2 && curve.curve) { + } else if (curve.mode === ParticleCurveMode.Curve && curve.curve) { // Curve mode
535-541: Document the 4-key limit for curves and gradients.Width curves and color gradients are silently capped at 4 keys. This shader limitation should be documented in the public API (on
widthCurveandcolorGradientproperties) so users understand the constraint.Also applies to: 566-573
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/core/src/trail/trail.fs.glslis excluded by!**/*.glslpackages/core/src/trail/trail.vs.glslis excluded by!**/*.glsl
📒 Files selected for processing (5)
e2e/case/trailRenderer-basic.tspackages/core/src/trail/TrailMaterial.tspackages/core/src/trail/TrailRenderer.tspackages/core/src/trail/enums/TrailTextureMode.tspackages/core/src/trail/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/trail/enums/TrailTextureMode.ts (1)
packages/core/src/trail/index.ts (1)
TrailTextureMode(3-3)
e2e/case/trailRenderer-basic.ts (5)
packages/core/src/base/EngineObject.ts (1)
engine(21-23)packages/core/src/index.ts (1)
Camera(16-16)packages/core/src/trail/TrailRenderer.ts (1)
TrailRenderer(27-588)packages/core/src/trail/index.ts (1)
TrailRenderer(1-1)packages/core/src/base/Time.ts (1)
deltaTime(40-42)
🪛 GitHub Check: lint
packages/core/src/trail/TrailRenderer.ts
[failure] 445-445:
Delete ····
[failure] 444-444:
Delete ·················
[failure] 443-443:
Delete ······
[failure] 442-442:
Delete ·····
[failure] 428-428:
Delete ·
[failure] 309-309:
Replace ⏎······this._vertexBuffer,⏎······TrailRenderer.VERTEX_STRIDE⏎···· with this._vertexBuffer,·TrailRenderer.VERTEX_STRIDE
[failure] 297-297:
Replace ⏎······engine,⏎······BufferBindFlag.VertexBuffer,⏎······byteLength,⏎······BufferUsage.Dynamic,⏎······false⏎···· with engine,·BufferBindFlag.VertexBuffer,·byteLength,·BufferUsage.Dynamic,·false
[failure] 237-237:
Replace ⏎········worldPosition.x·+·halfWidth,⏎········worldPosition.y·+·halfWidth,⏎········worldPosition.z·+·halfWidth⏎······ with worldPosition.x·+·halfWidth,·worldPosition.y·+·halfWidth,·worldPosition.z·+·halfWidth
[failure] 232-232:
Replace ⏎········worldPosition.x·-·halfWidth,⏎········worldPosition.y·-·halfWidth,⏎········worldPosition.z·-·halfWidth⏎······ with worldPosition.x·-·halfWidth,·worldPosition.y·-·halfWidth,·worldPosition.z·-·halfWidth
packages/core/src/trail/TrailMaterial.ts
[failure] 86-86:
Delete ⏎
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e (22.x, 2/4)
- GitHub Check: codecov
🔇 Additional comments (12)
packages/core/src/trail/index.ts (1)
1-4: LGTM!Clean barrel export that properly exposes the new
TrailTextureModeenum alongside existing trail exports.packages/core/src/trail/enums/TrailTextureMode.ts (1)
1-10: LGTM!Well-documented enum with explicit values. The JSDoc comments clearly explain the purpose of each mode.
packages/core/src/trail/TrailMaterial.ts (1)
71-83: LGTM!Appropriate render state configuration for trail rendering: additive blending, depth write disabled, and double-sided rendering enabled.
packages/core/src/trail/TrailRenderer.ts (8)
45-50: LGTM!Vertex stride calculation is correct: 3 + 1 + 1 + 4 + 1 + 3 = 13 floats = 52 bytes. Static temp vector avoids per-frame allocations.
158-185: LGTM!Clean update flow that properly retires expired points and manages shader uniforms. The per-frame uniform updates are acceptable for correctness.
190-219: LGTM!Proper early-exit when insufficient points, null/destroyed checks on material, and correct use of render element pools.
224-276: LGTM!Bounds calculation correctly handles the ring buffer wrap-around and expands by half-width to account for the trail's visual extent.
288-347: LGTM!Geometry initialization is well-structured with clear comments documenting the vertex layout. Buffer sizes are appropriate for the max point count.
349-374: LGTM!Correct retirement logic that advances through expired points and properly resets state when trail is empty.
393-400: Verify point count management when buffer is full.When the buffer is full,
_currentPointCountis decremented here before adding the new point. This correctly maintains the count, but the interaction with_retireActivePointscould be subtle—if a time-based retirement and buffer-full condition occur in the same frame, ensure counts remain consistent.
410-456: LGTM!Point addition logic correctly handles tangent calculation with proper first-point update when the second point is added. Vertex attribute offsets match the documented layout.
e2e/case/trailRenderer-basic.ts (1)
71-88: No changes needed—position mutation via.set()properly triggers transform updates.The code correctly uses
this.entity.transform.position.set(x, y, z). Vector3 has an_onValueChangedcallback mechanism that Transform registers during construction (this._position._onValueChanged = this._onPositionChanged). When.set()is called, it triggers this callback, which calls_setDirtyFlagTrue(TransformModifyFlags.LocalMatrix)and_updateWorldPositionFlag(), properly marking the transform as dirty. This pattern is used consistently throughout the codebase and works correctly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/config.ts (1)
436-443: Consider allowing a small diff tolerance for visual stability.The
diffPercentage: 0is quite strict for trail rendering, which may exhibit minor anti-aliasing or timing-related variations across different runs or environments. Other similar visual tests (e.g.,particleDreamat 0.015,shadow-basicat 0.008) allow small tolerances to prevent flaky tests.🔎 Suggested adjustment
Trail: { basic: { category: "Trail", caseFileName: "trailRenderer-basic", threshold: 0, - diffPercentage: 0 + diffPercentage: 0.01 } },
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/case/trailRenderer-basic.tse2e/config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e (22.x, 1/4)
- GitHub Check: e2e (22.x, 2/4)
- GitHub Check: codecov
🔇 Additional comments (5)
e2e/case/trailRenderer-basic.ts (5)
1-20: LGTM!Imports are appropriate for the test case. The curve and gradient types (prefixed with
Particle*) appear to be shared utilities also used by the trail renderer.
22-38: LGTM!Standard engine and camera setup follows the existing E2E test patterns.
40-69: LGTM!The TrailRenderer configuration demonstrates the key features (time, width, curves, gradients) effectively.
Minor note: The
Colorobjects inGradientColorKey(lines 59-61) include alpha values, but alpha is separately controlled byGradientAlphaKey. The alpha in the color is likely ignored, making it slightly redundant—but this follows the API design and is not an issue.
91-98: LGTM!The E2E test finalization follows the standard pattern with
updateForE2Efor controlled frame updates andinitScreenshotfor capturing the result.
78-88: No action needed. Theposition.set()method properly triggers transform updates in @galacean/engine through its internal_onValueChangedcallback, which is registered during Transform initialization. This pattern is widely used across the E2E test suite with no issues.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/trail/TrailMaterial.ts (1)
37-42: Add null check tobaseColorsetter for defensive coding.The setter assumes
getColor()returns a validColorinstance. While the constructor initializes this at line 69, defensive coding should guard againstundefinedto prevent potential runtime errors if called in unexpected scenarios.
🧹 Nitpick comments (3)
packages/core/src/trail/TrailRenderer.ts (3)
106-114: Consider making_maxPointCountconfigurable.The maximum point count is hardcoded to 256. For different use cases (short/fast trails vs. long/slow trails), users may benefit from adjusting this value to optimize memory usage or support longer trails.
🔎 Suggested approach
+ /** Maximum number of trail points. */ + maxPointCount: number = 256; + // Ring buffer pointers @ignoreClone private _firstActiveElement: number = 0; @ignoreClone private _firstFreeElement: number = 0; @ignoreClone private _currentPointCount: number = 0; - @ignoreClone - private _maxPointCount: number = 256;Note: This would require reinitializing geometry when changed, so a setter with validation might be more appropriate.
505-505: Consider moving inline import types to top-level imports.The inline dynamic import syntax
import("../shader/ShaderData").ShaderDataat lines 505 and 534 works but is unconventional. SinceShaderDatais used in multiple methods, importing it at the top with other imports would be cleaner.🔎 Suggested change
Add to imports at the top of the file:
import { ShaderData } from "../shader/ShaderData";Then update the method signatures:
- private _updateWidthCurve(shaderData: import("../shader/ShaderData").ShaderData): void { + private _updateWidthCurve(shaderData: ShaderData): void { - private _updateColorGradient(shaderData: import("../shader/ShaderData").ShaderData): void { + private _updateColorGradient(shaderData: ShaderData): void {Also applies to: 534-534
505-532: Replace magic numbers with named constants for curve mode.The mode values
0and2at lines 509 and 515 correspond toParticleCurveMode.ConstantandParticleCurveMode.Curve. ImportParticleCurveModefrom../particle/enums/ParticleCurveModeand use the enum directly for clarity, or define local constants if preferred.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/trail/TrailMaterial.tspackages/core/src/trail/TrailRenderer.tspackages/core/src/trail/enums/TrailTextureMode.tspackages/core/src/trail/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/trail/index.ts
- packages/core/src/trail/enums/TrailTextureMode.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/trail/TrailMaterial.ts (2)
packages/core/src/base/EngineObject.ts (1)
engine(21-23)packages/core/src/shader/index.ts (2)
BlendFactor(1-1)CullMode(5-5)
packages/core/src/trail/TrailRenderer.ts (3)
packages/math/src/Vector3.ts (8)
Vector3(11-612)x(327-329)x(331-334)y(339-341)y(343-346)z(351-353)z(355-358)distance(102-107)packages/core/src/graphic/Buffer.ts (2)
Buffer(13-250)byteLength(34-36)packages/core/src/trail/TrailMaterial.ts (1)
TrailMaterial(14-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e (22.x, 4/4)
- GitHub Check: codecov
🔇 Additional comments (7)
packages/core/src/trail/TrailMaterial.ts (1)
64-84: LGTM - Constructor properly initializes shader and render states.The lazy shader creation pattern, base color initialization, and render state configuration (additive blending, depth write disabled, culling off) are all appropriate for transparent trail rendering.
packages/core/src/trail/TrailRenderer.ts (6)
27-50: LGTM - Well-organized static shader properties and layout constants.The shader property bindings are clearly named, and the vertex stride constants (52 bytes / 13 floats) are correctly documented matching the vertex layout defined in
_initGeometry.
158-185: LGTM - Update loop correctly manages point lifecycle and shader uniforms.The update flow properly retires expired points, adds new points when emitting, and updates all necessary shader uniforms for the trail rendering.
280-330: LGTM - Geometry initialization with well-documented vertex layout.The vertex buffer setup correctly defines the 52-byte vertex layout with all required attributes (position, birth time, normalized width, color, corner, tangent). The comments at lines 313-320 clearly document the layout.
332-357: LGTM - Ring buffer retirement logic is correct.The
_retireActivePointsmethod correctly iterates from the head of the ring buffer, retiring points whose age exceeds the lifetime, and properly resets_hasLastPositionwhen all points expire.
393-439: LGTM - Point addition with correct vertex data population.The
_addPointmethod correctly:
- Calculates tangent direction from previous position
- Back-patches the first point's tangent when the second point is added (lines 406-413)
- Populates both vertices (top/bottom) for each trail point with correct offsets
273-278: LGTM - Resource cleanup is properly implemented.The
_onDestroymethod correctly destroys the vertex buffer, index buffer, and primitive using optional chaining to handle cases where they might not be initialized.
GuoLei1990
added
rendering
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/trail/TrailRenderer.ts (4)
113-113: Consider making_maxPointCountconfigurable.The maximum point count is hardcoded to 256. For longer trails or higher frame rates, users might need more points. Consider exposing this as a public configuration property (e.g.,
maxPointCount) set via constructor or property, allowing users to balance memory usage vs. trail length.🔎 Suggested approach
Add a public property to configure max point count:
+ /** Maximum number of trail points to store. */ + maxPointCount: number = 256; + // Ring buffer pointers @ignoreClone private _firstActiveElement: number = 0; @ignoreClone private _firstFreeElement: number = 0; @ignoreClone private _currentPointCount: number = 0; - @ignoreClone - private _maxPointCount: number = 256;Then use
this.maxPointCountinstead ofthis._maxPointCountthroughout, and call_initGeometry()when the value changes (with proper cleanup of old buffers).
192-197: Optimize buffer updates to occur only when data changes.The vertex and index buffers are updated every frame in
_render(), even when no points were added or removed. This causes unnecessary GPU uploads and could impact performance, especially for trails with many points.🔎 Suggested optimization
Track whether the buffer needs updating and only upload when necessary:
+ @ignoreClone + private _bufferDirty: boolean = false; + protected override _update(context: RenderContext): void { super._update(context); const deltaTime = this.engine.time.deltaTime; this._playTime += deltaTime; // Retire old points + const oldCount = this._currentPointCount; this._retireActivePoints(); + if (this._currentPointCount !== oldCount) { + this._bufferDirty = true; + } // Add new point if emitting and moved enough if (this.emitting) { this._tryAddNewPoint(); } // ... rest of update } protected override _render(context: RenderContext): void { const activeCount = this._getActivePointCount(); if (activeCount < 2) { return; } - // Update vertex buffer with new points - this._updateVertexBuffer(); - - // Update index buffer to handle ring buffer wrap-around - const indexCount = this._updateIndexBuffer(activeCount); - this._subPrimitive.count = indexCount; + if (this._bufferDirty) { + this._updateVertexBuffer(); + const indexCount = this._updateIndexBuffer(activeCount); + this._subPrimitive.count = indexCount; + this._bufferDirty = false; + } // ... rest of render }Set
_bufferDirty = truein_addPoint()as well.
514-514: Extract magic number 4 to a named constant.The maximum number of curve/gradient keys (4) appears multiple times. Extract this to a named constant to improve maintainability and make the limitation explicit.
🔎 Proposed refactor
Add a constant after the vertex stride constants:
private static readonly VERTEX_STRIDE = 52; // bytes per vertex private static readonly VERTEX_FLOAT_STRIDE = 13; // floats per vertex + private static readonly MAX_CURVE_KEYS = 4; // Maximum number of keys for curves and gradientsThen use it throughout:
- const count = Math.min(keys.length, 4); + const count = Math.min(keys.length, TrailRenderer.MAX_CURVE_KEYS); - const colorCount = Math.min(colorKeys.length, 4); + const colorCount = Math.min(colorKeys.length, TrailRenderer.MAX_CURVE_KEYS); - const alphaCount = Math.min(alphaKeys.length, 4); + const alphaCount = Math.min(alphaKeys.length, TrailRenderer.MAX_CURVE_KEYS);Also update the array allocations:
- const colorKeysData = this._colorKeysData || (this._colorKeysData = new Float32Array(16)); - const alphaKeysData = this._alphaKeysData || (this._alphaKeysData = new Float32Array(8)); + const colorKeysData = this._colorKeysData || (this._colorKeysData = new Float32Array(TrailRenderer.MAX_CURVE_KEYS * 4)); + const alphaKeysData = this._alphaKeysData || (this._alphaKeysData = new Float32Array(TrailRenderer.MAX_CURVE_KEYS * 2));Also applies to: 545-545, 558-558
410-413: Consider using entity's forward direction for initial tangent.The first point uses a hardcoded tangent of
(0, 0, 1)until the second point is added. While this is temporary and corrected when the second point arrives, using the entity's forward direction would provide a more accurate initial orientation.🔎 Suggested improvement
} else { - // First point - use forward direction (will be updated when second point is added) - tangent.set(0, 0, 1); + // First point - use entity's forward direction (will be updated when second point is added) + this.entity.transform.getWorldForward(tangent); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Trail_trailRenderer-basic.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
e2e/case/trailRenderer-basic.tspackages/core/src/trail/TrailRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/case/trailRenderer-basic.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e (22.x, 3/4)
- GitHub Check: e2e (22.x, 4/4)
- GitHub Check: codecov
🔇 Additional comments (5)
packages/core/src/trail/TrailRenderer.ts (5)
220-264: LGTM! Bounds calculation correctly handles ring buffer wraparound.The bounds calculation properly:
- Falls back to entity position when no points exist
- Iterates through the ring buffer with wraparound handling
- Expands bounds by half-width to account for billboard rendering
- Uses the correct vertex offsets for position data
328-435: LGTM! Ring buffer management correctly handles point lifecycle and wraparound.The ring buffer implementation properly:
- Retires expired points by advancing
_firstActiveElement- Handles buffer overflow by retiring the oldest point
- Maintains correct wraparound logic at buffer boundaries
- Updates tangents for proper trail orientation, including the special case when adding the second point
441-499: LGTM! Buffer update logic correctly handles ring buffer and GPU uploads.The buffer update implementation properly:
- Handles contiguous ranges with a single upload
- Handles wrapped ranges with two uploads
- Correctly converts between float offsets and byte offsets
- Builds triangle strip indices with proper wraparound
- Uploads the correct byte lengths to GPU buffers
269-274: No action needed.SubPrimitiveis a simple data structure holding drawing parameters (start, count, topology) and does not manage GPU resources. It has nodestroy()method and does not require explicit cleanup, unlikePrimitive,Buffer, andIndexBufferwhich manage GPU memory.Likely an incorrect or invalid review comment.
28-42: Shader property names are correctly matched to the corresponding uniforms in trail.vs.glsl. All 11 properties defined in lines 28-42 have exact matches in the vertex shader uniforms.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/core/src/shaderlib/extra/trail.fs.glslis excluded by!**/*.glsl
📒 Files selected for processing (3)
packages/core/src/material/EffectMaterial.tspackages/core/src/particle/ParticleMaterial.tspackages/core/src/trail/TrailMaterial.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/trail/TrailMaterial.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/material/EffectMaterial.ts (1)
packages/core/src/base/EngineObject.ts (1)
engine(21-23)
packages/core/src/particle/ParticleMaterial.ts (1)
packages/core/src/material/EffectMaterial.ts (1)
EffectMaterial(10-85)
🪛 GitHub Check: lint
packages/core/src/material/EffectMaterial.ts
[failure] 86-86:
Delete ⏎
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e (22.x, 1/4)
- GitHub Check: e2e (22.x, 3/4)
- GitHub Check: e2e (22.x, 4/4)
🔇 Additional comments (9)
packages/core/src/particle/ParticleMaterial.ts (4)
2-2: LGTM: Import updated correctly.The import change from
BaseMaterialtoEffectMaterialcorrectly aligns with the class extension refactoring.
8-8: LGTM: Class hierarchy refactored correctly.The change to extend
EffectMaterialaligns with the PR's goal to share common material properties between particle and trail materials.
10-10: LGTM: Documentation improved.The JSDoc now accurately describes the constructor as creating a particle material instance rather than an "unlit material instance."
13-15: LGTM: Constructor simplified correctly.The constructor now correctly delegates initialization to the
EffectMaterialbase class, which handles default color values and transparency settings.packages/core/src/material/EffectMaterial.ts (5)
14-23: LGTM: Color property pattern is correct.The baseColor setter uses a reference check to avoid unnecessary copying when the same Color instance is set. This is a reasonable optimization.
28-39: LGTM: Texture property with macro management.The baseTexture setter correctly manages the associated shader macro, enabling it when a texture is present and disabling it otherwise.
44-53: LGTM: Consistent color property pattern.The emissiveColor property follows the same safe pattern as baseColor, using
copyFromto avoid external mutations.
58-69: LGTM: Consistent texture property pattern.The emissiveTexture property follows the same macro management pattern as baseTexture for shader conditional compilation.
76-84: LGTM: Constructor initializes sensible defaults.The constructor properly initializes default colors (white base, black emissive) and sets transparency, which is appropriate for effect materials like particles and trails.
…ing in TrailRenderer
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/trail/TrailRenderer.ts (2)
562-590: Verify handling of all ParticleCompositeCurve modes.The code handles mode 0 (constant) and mode 2 (curve) but doesn't explicitly handle mode 1. If
ParticleCompositeCurvehas additional modes (e.g., mode 1 for random between two curves), they will fall through to the default constant 1.0 case. Please verify this is intentional.Additionally, the curve is limited to 4 keys (line 575), likely due to shader uniform constraints. Consider documenting this limitation in the
widthCurveproperty JSDoc comments so users understand why additional keys are ignored.
592-628: Document the gradient key limitations.The color and alpha keys are limited to 4 each (lines 607, 620), likely due to shader uniform array size constraints. Consider documenting this limitation in the
colorGradientproperty JSDoc comments to inform users that additional keys beyond the first 4 will be silently ignored.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Trail_trailRenderer-basic.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
e2e/case/trailRenderer-basic.tspackages/core/src/trail/TrailRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/case/trailRenderer-basic.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/trail/TrailRenderer.ts (1)
packages/math/src/Vector3.ts (4)
Vector3(11-612)min(168-173)max(155-160)distance(102-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x, 2/4)
🔇 Additional comments (15)
packages/core/src/trail/TrailRenderer.ts (15)
1-27: LGTM! Clean architecture with appropriate base class.The imports are well-organized and the class correctly extends
Rendererto integrate with the broader rendering pipeline. The base class change fromMeshRenderertoRendereris appropriate for a custom buffer-driven renderer.
28-51: LGTM! Well-organized shader bindings and constants.The shader properties are correctly declared as static, and the vertex layout constants are properly documented. The stride calculations are consistent (13 floats = 52 bytes).
52-89: LGTM! Well-designed public API with sensible defaults.The public properties provide a comprehensive interface for trail configuration. The use of
@deepClonedecorators for complex types (Color, ParticleCompositeCurve, ParticleGradient) ensures proper cloning behavior.
90-141: LGTM! Well-organized internal state with proper clone handling.The internal state is properly decorated with
@ignoreCloneto prevent cloning runtime-managed resources. The ring buffer pointers are clearly named, and the bounds dirty-flag optimization is a good practice for performance.
265-315: LGTM! Correct buffer allocation and vertex layout.The vertex buffer, index buffer, and primitive setup are correct. The vertex layout is properly documented and matches the VERTEX_STRIDE constant (52 bytes). Using
BufferUsage.Dynamicis appropriate for frequently updated trail geometry.
317-347: LGTM! Correct ring buffer point retirement logic.The ring buffer advancement and wrap-around handling are correct. Setting
_boundsDirtywhen points are retired (line 340) ensures bounds are recalculated. Resetting_hasLastPositionwhen all points expire (line 345) correctly handles the start of a new trail segment.
349-380: LGTM! Proper distance check and buffer overflow handling.The minimum distance check prevents excessive point generation, and the buffer overflow handling correctly retires the oldest point when the buffer is full. The ring buffer wrap-around logic is correct.
382-432: LGTM! Correct vertex data generation and tangent handling.The tangent calculation and special handling for the second point (updating the first point's tangent at lines 395-404) is a nice touch that ensures the trail tail doesn't have incorrect orientation. The vertex data layout matches the stride constants, and the bounds expansion is correctly called.
434-493: LGTM! Correct ring buffer math and bounds optimization.The active point count calculation handles ring buffer wrap-around correctly. The bounds optimization with dirty flag and incremental expansion (
_expandBounds) avoids full recalculation on every update. The full recalculation (_recalculateBounds) correctly iterates through wrapped buffer ranges.
495-537: LGTM! Correct buffer upload with wrap-around handling.The buffer upload logic correctly handles both contiguous and wrapped ranges. The content-loss recovery (line 501) ensures proper behavior after context loss. The byte offset calculations for
Float32Arrayviews andsetDatacalls are correct.
539-560: LGTM! Correct index generation for triangle strip.The index buffer generation correctly handles ring buffer wrap-around using modulo arithmetic. Each point contributes two indices (bottom and top vertices) for proper triangle strip rendering.
164-191: LGTM! Clean update loop with proper shader uniform updates.The update method correctly maintains trail state by retiring old points, adding new points when emitting, and updating shader uniforms. The call to
super._updateensures base class behavior is preserved.
196-227: LGTM! Robust rendering with proper validation.The rendering logic correctly handles edge cases (< 2 points), content loss recovery, and material validation. The checks for destroyed material and shader (lines 216-218) are good defensive programming to prevent crashes during teardown.
232-253: LGTM! Efficient bounds calculation with caching.The bounds update uses the dirty-flag optimization to avoid recalculating on every frame. The halfWidth expansion correctly accounts for the trail's visual width. The fallback to entity position when the trail is empty (lines 236-242) is appropriate.
145-148: LGTM! Proper initialization and cleanup.The constructor, clear method, and destruction logic are correct. The use of optional chaining in
_onDestroy(lines 260-262) safely handles cases where resources may not be initialized. Theclearmethod correctly resets ring buffer state and marks bounds as dirty.Also applies to: 153-159, 258-263
| } | ||
| } | ||
|
|
||
| private _mergePointPosition(vertices: Float32Array, offset: number, min: Vector3, max: Vector3): void { |
There was a problem hiding this comment.
In the trail.vs.glsl, this point needs to be multiplied by widthMultiplier, but this is ignored here.
| min.set(Infinity, Infinity, Infinity); | ||
| max.set(-Infinity, -Infinity, -Infinity); | ||
|
|
||
| const wrapped = firstActive > firstFree; |
There was a problem hiding this comment.
In _addActivePointsToVertexBuffer and _render, wrapped is defined as spansBoundary && firstFree > 0;, while here wrapped is defined as firstActive > firstFree;. It would be better if these were consistent.
| } else { | ||
| Vector3.subtract(points[i + 1], p, perpVector); | ||
| } | ||
| protected override _updateBounds(worldBounds: BoundingBox): void { |
There was a problem hiding this comment.
The dirty flag of TrailRenderer's bounds is entirely controlled by Transform. If stop moving after a certain period, its bounding box will remain permanently at the point of stopping.
|
|
||
| // age: time since birth, normalizedAge: 0=new, 1=expired | ||
| float age = renderer_TimeDistParams.x - birthTime; | ||
| float normalizedAge = age / renderer_TimeDistParams.y; |
There was a problem hiding this comment.
现在的淡出用不到时间衰减的话可以删除这个计算，还是说两者叠加效果会更好
…urve in TrailRenderer
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/trail/TrailRenderer.ts`:
- Around line 436-521: In _addPoint(), the computed tangent can be zero-length
causing normalize() to produce NaNs; before calling tangent.normalize() (the
block that computes segmentLength from Vector3.subtract in _addPoint), guard
against segmentLength === 0 and supply a safe fallback (e.g., set tangent to a
default vector like (0,0,1) or reuse the previous valid tangent) so you never
call normalize() on a zero vector and thus avoid poisoning vertices; update any
logic that depends on the tangent (including the first-point placeholder
handling and bridge copy paths) to use the guarded tangent.
♻️ Duplicate comments (1)
packages/core/src/trail/TrailRenderer.ts (1)
164-170: Reset cumulative distance on clear().
Line 169:_cumulativeDistanceshould reset so UV/gradient distance restarts cleanly afterclear().💡 Proposed fix
clear(): void { this._firstActiveElement = 0; this._firstNewElement = 0; this._firstFreeElement = 0; this._firstRetiredElement = 0; this._hasLastPosition = false; + this._cumulativeDistance = 0; }
| private _emitNewPoint(playTime: number): void { | ||
| const worldPosition = this.entity.transform.worldPosition; | ||
|
|
||
| if (this._hasLastPosition && Vector3.distance(worldPosition, this._lastPosition) < this.minVertexDistance) { | ||
| return; | ||
| } | ||
|
|
||
| this._prePointsNum = this._curPointNum; | ||
| // Using 'nextFreeElement' instead of 'freeElement' when comparing with '_firstRetiredElement' | ||
| // aids in definitively identifying the head and tail of the circular queue. | ||
| // Failure to adopt this approach may impede growth initiation | ||
| // due to the initial alignment of 'freeElement' and 'firstRetiredElement'. | ||
| const nextFreeElement = (this._firstFreeElement + 1) % this._currentPointCapacity; | ||
| if (nextFreeElement === this._firstRetiredElement) { | ||
| this._resizeBuffer(TrailRenderer.POINT_INCREASE_COUNT); | ||
| } | ||
|
|
||
| const count = this._curPointNum; | ||
| const texDelta = 1.0 / count; | ||
| this._addPoint(worldPosition, playTime); | ||
| this._lastPosition.copyFrom(worldPosition); | ||
| this._hasLastPosition = true; | ||
| } | ||
|
|
||
| private _addPoint(position: Vector3, playTime: number): void { | ||
| const pointIndex = this._firstFreeElement; | ||
| const floatStride = TrailRenderer.VERTEX_FLOAT_STRIDE; | ||
| const pointStride = TrailRenderer.POINT_FLOAT_STRIDE; | ||
| const vertices = this._vertices; | ||
| for (let i = 0; i < count; i++) { | ||
| const d = 1.0 - i * texDelta; | ||
| const p0 = (i * 2 * this._vertexStride) / 4; | ||
| const p1 = ((i * 2 + 1) * this._vertexStride) / 4; | ||
| const capacity = this._currentPointCapacity; | ||
|
|
||
| const tangent = TrailRenderer._tempVector3; | ||
| if (this._hasLastPosition) { | ||
| Vector3.subtract(position, this._lastPosition, tangent); | ||
| const segmentLength = tangent.length(); | ||
| tangent.normalize(); | ||
|
|
||
| // First point has placeholder tangent, update it when second point is added | ||
| if (this._getActivePointCount() === 1) { | ||
| const firstPointOffset = this._firstActiveElement * pointStride; | ||
| tangent.copyToArray(vertices, firstPointOffset + 5); // Top vertex tangent | ||
| tangent.copyToArray(vertices, firstPointOffset + floatStride + 5); // Bottom vertex tangent | ||
| // Mark first point for re-upload since its tangent changed | ||
| this._firstNewElement = this._firstActiveElement; | ||
| } | ||
|
|
||
| vertices[p0] = 0; | ||
| vertices[p0 + 1] = d; | ||
| // Update cumulative distance | ||
| this._cumulativeDistance += segmentLength; | ||
| } else { | ||
| // First point uses placeholder tangent (will be corrected when second point arrives) | ||
| tangent.set(0, 0, 1); | ||
| } | ||
|
|
||
| vertices[p1] = 1.0; | ||
| vertices[p1 + 1] = d; | ||
| // Write top vertex (corner = -1) and bottom vertex (corner = 1) | ||
| // Store absolute cumulative distance (written once, never updated) | ||
| const distOffset = TrailRenderer.DISTANCE_OFFSET; | ||
| const cumulativeDist = this._cumulativeDistance; | ||
| const topOffset = pointIndex * pointStride; | ||
| position.copyToArray(vertices, topOffset); | ||
| vertices[topOffset + 3] = playTime; | ||
| vertices[topOffset + 4] = -1; | ||
| tangent.copyToArray(vertices, topOffset + 5); | ||
| vertices[topOffset + distOffset] = cumulativeDist; | ||
|
|
||
| const bottomOffset = topOffset + floatStride; | ||
| position.copyToArray(vertices, bottomOffset); | ||
| vertices[bottomOffset + 3] = playTime; | ||
| vertices[bottomOffset + 4] = 1; | ||
| tangent.copyToArray(vertices, bottomOffset + 5); | ||
| vertices[bottomOffset + distOffset] = cumulativeDist; | ||
|
|
||
| // Write to bridge position when writing point 0 (bridge = copy of point 0 to connect wrap-around) | ||
| if (pointIndex === 0) { | ||
| const bridgeTopOffset = capacity * pointStride; | ||
| const bridgeBottomOffset = bridgeTopOffset + floatStride; | ||
| position.copyToArray(vertices, bridgeTopOffset); | ||
| vertices[bridgeTopOffset + 3] = playTime; | ||
| vertices[bridgeTopOffset + 4] = -1; | ||
| tangent.copyToArray(vertices, bridgeTopOffset + 5); | ||
| vertices[bridgeTopOffset + distOffset] = cumulativeDist; | ||
| position.copyToArray(vertices, bridgeBottomOffset); | ||
| vertices[bridgeBottomOffset + 3] = playTime; | ||
| vertices[bridgeBottomOffset + 4] = 1; | ||
| tangent.copyToArray(vertices, bridgeBottomOffset + 5); | ||
| vertices[bridgeBottomOffset + distOffset] = cumulativeDist; | ||
| } | ||
|
|
||
| this._firstFreeElement = (this._firstFreeElement + 1) % capacity; | ||
| } |
There was a problem hiding this comment.
Guard zero-length tangents to avoid NaNs.
Lines 464-469: if segmentLength is 0 (e.g., minVertexDistance set to 0 or object stationary), normalize() can yield NaNs and poison vertex data. Add a zero-length guard/fallback.
🐛 Proposed guard
if (this._hasLastPosition) {
Vector3.subtract(position, this._lastPosition, tangent);
const segmentLength = tangent.length();
- tangent.normalize();
+ if (segmentLength > 0) {
+ tangent.normalize();
+ } else {
+ // Fallback to a stable direction when there is no movement
+ tangent.set(0, 0, 1);
+ }
// First point has placeholder tangent, update it when second point is added
if (this._getActivePointCount() === 1) {
const firstPointOffset = this._firstActiveElement * pointStride;
tangent.copyToArray(vertices, firstPointOffset + 5); // Top vertex tangent🤖 Prompt for AI Agents
In `@packages/core/src/trail/TrailRenderer.ts` around lines 436 - 521, In
_addPoint(), the computed tangent can be zero-length causing normalize() to
produce NaNs; before calling tangent.normalize() (the block that computes
segmentLength from Vector3.subtract in _addPoint), guard against segmentLength
=== 0 and supply a safe fallback (e.g., set tangent to a default vector like
(0,0,1) or reuse the previous valid tangent) so you never call normalize() on a
zero vector and thus avoid poisoning vertices; update any logic that depends on
the tangent (including the first-point placeholder handling and bridge copy
paths) to use the guarded tangent.
…ariables in TrailRenderer
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive trail rendering system for the Galacean engine, replacing the deprecated trail implementation. The trail system enables visual effects that follow moving objects with customizable width, color, lifetime, and texture mapping.
Changes:
- Complete rewrite of TrailRenderer with per-trail time control, emission toggling, vertex distance thresholds, width curves, color gradients, and texture scaling modes
- Introduction of EffectMaterial base class to unify material handling for particles and trails
- New GLSL shaders for trail rendering with billboard-based geometry and gradient evaluation
- Comprehensive test suite including unit tests and E2E visual tests
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/trail/TrailRenderer.ts | Complete rewrite implementing circular buffer-based trail rendering with billboard geometry |
| packages/core/src/trail/TrailMaterial.ts | Simplified to extend EffectMaterial with clone support |
| packages/core/src/trail/enums/TrailTextureMode.ts | New enum defining Stretch and Tile texture mapping modes |
| packages/core/src/trail/index.ts | Added TrailTextureMode export |
| packages/core/src/material/EffectMaterial.ts | New base class consolidating effect material properties (baseColor, baseTexture, emissive) |
| packages/core/src/particle/ParticleMaterial.ts | Refactored to extend EffectMaterial, removing duplicated code |
| packages/core/src/shaderlib/extra/trail.vs.glsl | New vertex shader implementing billboard rendering with curves and gradients |
| packages/core/src/shaderlib/extra/trail.fs.glsl | New fragment shader with base and emissive texture support |
| packages/core/src/shaderlib/particle/particle_common.glsl | Added evaluateParticleGradient function for gradient evaluation |
| packages/core/src/shaderlib/particle/color_over_lifetime_module.glsl | Removed duplicate gradient evaluation code |
| packages/core/src/shader/ShaderPool.ts | Registered trail shader |
| tests/src/core/Trail.test.ts | Comprehensive unit tests for TrailRenderer and TrailMaterial |
| e2e/case/trailRenderer-basic.ts | Visual test with multiple animated trails demonstrating features |
| e2e/config.ts | Added trail test configuration |
| e2e/fixtures/originImage/Trail_trailRenderer-basic.jpg | Reference image for E2E test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/src/core/Trail.test.ts
Outdated
| expect(trailRenderer.bounds.max).to.deep.include({ x: 0, y: 0, z: 0 }); | ||
|
|
||
| // Move entity to (5, 0, 0) - distance > minVertexDistance, creates trail point | ||
| engine.update(); //@todo:删除会触发包围盒无法更新的bug |
There was a problem hiding this comment.
The comment contains Chinese text and a TODO marker indicating a known bug. The comment states "删除会触发包围盒无法更新的bug" which translates to "removing this will trigger a bug where the bounding box cannot be updated." This suggests there's a workaround for a bug that should be properly fixed rather than worked around in tests. Consider either fixing the underlying bug or documenting why this engine.update() call is necessary for the test to work correctly.
| engine.update(); //@todo:删除会触发包围盒无法更新的bug | |
| engine.update(); // Perform an initial engine update to ensure TrailRenderer bounds are initialized and can update correctly |
| export class EffectMaterial extends BaseMaterial { | ||
| /** | ||
| * Base color. | ||
| */ | ||
| get baseColor(): Color { | ||
| return this.shaderData.getColor(BaseMaterial._baseColorProp); | ||
| } | ||
|
|
||
| set baseColor(value: Color) { | ||
| const baseColor = this.shaderData.getColor(BaseMaterial._baseColorProp); | ||
| if (value !== baseColor) { | ||
| baseColor.copyFrom(value); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Base texture. | ||
| */ | ||
| get baseTexture(): Texture2D { | ||
| return <Texture2D>this.shaderData.getTexture(BaseMaterial._baseTextureProp); | ||
| } | ||
|
|
||
| set baseTexture(value: Texture2D) { | ||
| this.shaderData.setTexture(BaseMaterial._baseTextureProp, value); | ||
| if (value) { | ||
| this.shaderData.enableMacro(BaseMaterial._baseTextureMacro); | ||
| } else { | ||
| this.shaderData.disableMacro(BaseMaterial._baseTextureMacro); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Emissive color. | ||
| */ | ||
| get emissiveColor(): Color { | ||
| return this.shaderData.getColor(BaseMaterial._emissiveColorProp); | ||
| } | ||
|
|
||
| set emissiveColor(value: Color) { | ||
| const emissiveColor = this.shaderData.getColor(BaseMaterial._emissiveColorProp); | ||
| if (value !== emissiveColor) { | ||
| emissiveColor.copyFrom(value); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Emissive texture. | ||
| */ | ||
| get emissiveTexture(): Texture2D { | ||
| return <Texture2D>this.shaderData.getTexture(BaseMaterial._emissiveTextureProp); | ||
| } | ||
|
|
||
| set emissiveTexture(value: Texture2D) { | ||
| this.shaderData.setTexture(BaseMaterial._emissiveTextureProp, value); | ||
| if (value) { | ||
| this.shaderData.enableMacro(BaseMaterial._emissiveTextureMacro); | ||
| } else { | ||
| this.shaderData.disableMacro(BaseMaterial._emissiveTextureMacro); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create an effect material instance. | ||
| * @param engine - Engine to which the material belongs | ||
| * @param shader - Shader used by the material | ||
| */ | ||
| constructor(engine: Engine, shader: Shader) { | ||
| super(engine, shader); | ||
|
|
||
| const shaderData = this.shaderData; | ||
| shaderData.setColor(BaseMaterial._baseColorProp, new Color(1, 1, 1, 1)); | ||
| shaderData.setColor(BaseMaterial._emissiveColorProp, new Color(0, 0, 0, 1)); | ||
|
|
||
| this.isTransparent = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The EffectMaterial class is not exported from the material module's index.ts file. While TrailMaterial and ParticleMaterial are exported via their respective modules, EffectMaterial serves as a base class for effect-type materials and should be exported from the material module for consistency and to allow users to extend it for custom effect materials. Add export { EffectMaterial } from "./EffectMaterial"; to packages/core/src/material/index.ts.
There was a problem hiding this comment.
EffectMaterial is internal material
| vec4 evaluateParticleGradient(in vec4 colorKeys[4], in float colorMaxTime, in vec2 alphaKeys[4], in float alphaMaxTime, in float t) { | ||
| vec4 value; | ||
|
|
||
| float alphaT = min(t, alphaMaxTime); | ||
| for (int i = 0; i < 4; i++) { | ||
| vec2 key = alphaKeys[i]; | ||
| if (alphaT <= key.x) { | ||
| if (i == 0) { | ||
| value.a = alphaKeys[0].y; | ||
| } else { | ||
| vec2 lastKey = alphaKeys[i - 1]; | ||
| float age = (alphaT - lastKey.x) / (key.x - lastKey.x); | ||
| value.a = mix(lastKey.y, key.y, age); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| float colorT = min(t, colorMaxTime); | ||
| for (int i = 0; i < 4; i++) { | ||
| vec4 key = colorKeys[i]; | ||
| if (colorT <= key.x) { | ||
| if (i == 0) { | ||
| value.rgb = colorKeys[0].yzw; | ||
| } else { | ||
| vec4 lastKey = colorKeys[i - 1]; | ||
| float age = (colorT - lastKey.x) / (key.x - lastKey.x); | ||
| value.rgb = mix(lastKey.yzw, key.yzw, age); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return value; |
There was a problem hiding this comment.
The evaluateParticleGradient function has a potential issue when no alpha keys match the condition (alphaT <= key.x). If all alpha keys have time values less than alphaT, the loop will complete without setting value.a, leaving it uninitialized. The same issue exists for the color loop. The original implementation in color_over_lifetime_module.glsl had the same structure, but this should still be addressed. Consider initializing value to a default (e.g., vec4(1.0)) or handling the case when t exceeds all key times by using the last key's value.
There was a problem hiding this comment.
因为 alphaMaxTime 就是所有 key.x 的最大值，所以至少会等于
| /** | ||
| * The fade-out duration in seconds. | ||
| */ | ||
| get time(): number { | ||
| return this._timeDistParams.y; | ||
| } | ||
|
|
||
| this._strapPoints.push(new Vector3()); | ||
| this._strapPoints.push(new Vector3()); | ||
| } | ||
| this._curPointNum = 0; | ||
| set time(value: number) { | ||
| this._timeDistParams.y = value; | ||
| } |
There was a problem hiding this comment.
The documentation for the 'time' property says "The fade-out duration in seconds" but this is not entirely accurate. Based on the implementation, 'time' represents the lifetime of trail points - how long each point persists before fading out. Consider updating the documentation to be more precise: "The lifetime of trail points in seconds. Each point will fade out after this duration."
There was a problem hiding this comment.
Too technically advanced, not easy to understand
| import { | ||
| AssetType, | ||
| BlendMode, | ||
| BloomEffect, | ||
| Camera, | ||
| Color, | ||
| CurveKey, | ||
| GradientAlphaKey, | ||
| GradientColorKey, | ||
| Logger, | ||
| ParticleCurve, | ||
| ParticleGradient, | ||
| PostProcess, | ||
| Script, | ||
| Texture2D, | ||
| TonemappingEffect, | ||
| TonemappingMode, | ||
| TrailMaterial, | ||
| TrailRenderer, | ||
| Vector3, | ||
| WebGLEngine | ||
| } from "@galacean/engine"; |
There was a problem hiding this comment.
Unused imports BloomEffect, PostProcess, TonemappingEffect, TonemappingMode.
… trail point generation
…erer initialization
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/src/core/Trail.test.ts`:
- Around line 15-23: The async initialization inside the describe block should
be moved into a beforeAll hook and cleaned up in afterAll: remove the async from
describe("Trail", ...) and instead perform await WebGLEngine.create({ canvas })
and assign engine, scene (sceneManager.activeScene) inside a beforeAll block,
call engine.run() there as well, and add an afterAll that calls engine.destroy()
to stop the animation loop; update tests to reference the engine/scene variables
initialized by beforeAll.
- Around line 192-229: The TrailRenderer width setter currently updates the
width but does not invalidate bounds, so change the TrailRenderer.width setter
to mark bounds dirty so they get recalculated immediately: when setting width in
the width setter (symbol: TrailRenderer.width), set the internal dirty flag
(e.g., this._boundsDirty = true) or call the existing bounds invalidation helper
(e.g., this._markBoundsDirty() or this.entity.transform.markWorldVolumeDirty())
so subsequent assertions/readers get updated bounds without relying on a later
position change.
🧹 Nitpick comments (1)
tests/src/core/Trail.test.ts (1)
24-30: Clean up the per-test root entity to keep tests isolated.Each test creates a new root + camera but never disposes it. This can accumulate state across tests and affect later assertions. Track the root entity created in
beforeEachand destroy it inafterEach(or use the scene reset API if available).🧹 Suggested cleanup
-import { describe, it, expect, beforeEach, beforeAll, afterAll } from "vitest"; +import { describe, it, expect, beforeEach, beforeAll, afterAll, afterEach } from "vitest"; + let rootEntity: any; // type as Entity if available beforeEach(() => { - const rootEntity = scene.createRootEntity("root"); + rootEntity = scene.createRootEntity("root"); const cameraEntity = rootEntity.createChild("camera"); cameraEntity.addComponent(Camera); cameraEntity.transform.setPosition(0, 0, -10); cameraEntity.transform.lookAt(new Vector3()); }); + + afterEach(() => { + rootEntity.destroy(); + });
| import { describe, it, expect, beforeEach } from "vitest"; | ||
|
|
||
| describe("Trail", async () => { | ||
| const canvas = document.createElement("canvas"); | ||
| const engine = await WebGLEngine.create({ canvas: canvas }); | ||
| const scene = engine.sceneManager.activeScene; | ||
|
|
||
| engine.run(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/src/core/Trail.test.ts | head -50Repository: galacean/engine
Length of output: 2074
🏁 Script executed:
fd -t f "WebGLEngine" --exec grep -l "class WebGLEngine\|export.*WebGLEngine" {} \;Repository: galacean/engine
Length of output: 129
🏁 Script executed:
rg "class WebGLEngine|export.*WebGLEngine" -A 20 -t ts -t jsRepository: galacean/engine
Length of output: 2690
🏁 Script executed:
rg "destroy\(" --type ts -B 2 -A 2 | head -30Repository: galacean/engine
Length of output: 1580
🏁 Script executed:
rg "class Engine" -A 30 -t ts | head -80Repository: galacean/engine
Length of output: 5065
🏁 Script executed:
rg "\.destroy\(\)|_destroy\(\)" packages/core/src -t ts -B 2 -A 2 | head -40Repository: galacean/engine
Length of output: 2359
🏁 Script executed:
cat -n packages/core/src/Engine.ts | head -100Repository: galacean/engine
Length of output: 4419
🏁 Script executed:
rg "destroy\(\)|_destroy\(\)" packages/core/src/Engine.ts -B 2 -A 5Repository: galacean/engine
Length of output: 1081
🏁 Script executed:
rg "public.*destroy|public.*\._destroy|_onDestroy" packages/core/src/Engine.ts -B 2 -A 10Repository: galacean/engine
Length of output: 41
🏁 Script executed:
cat -n packages/core/src/Engine.ts | sed -n '200,350p'Repository: galacean/engine
Length of output: 5667
🏁 Script executed:
rg "describe\(" tests/src -A 5 -t ts | head -100Repository: galacean/engine
Length of output: 6805
🏁 Script executed:
rg "beforeAll|afterAll" tests/src -B 2 -A 2 -t ts | head -50Repository: galacean/engine
Length of output: 2821
🏁 Script executed:
cat -n tests/src/core/Trail.test.ts | sed -n '1,100p'Repository: galacean/engine
Length of output: 4124
Move engine setup from async describe to beforeAll and add afterAll cleanup.
Vitest collects tests synchronously; async describe blocks prevent proper test registration. Additionally, engine.run() starts an animation loop that requires cleanup. Use beforeAll for async initialization and afterAll to call engine.destroy().
🛠️ Suggested refactor
-import { describe, it, expect, beforeEach } from "vitest";
+import { describe, it, expect, beforeEach, beforeAll, afterAll } from "vitest";
-describe("Trail", async () => {
- const canvas = document.createElement("canvas");
- const engine = await WebGLEngine.create({ canvas: canvas });
- const scene = engine.sceneManager.activeScene;
-
- engine.run();
+describe("Trail", () => {
+ let engine: WebGLEngine;
+ let scene: any; // type as Scene if available
+
+ beforeAll(async () => {
+ const canvas = document.createElement("canvas");
+ engine = await WebGLEngine.create({ canvas });
+ scene = engine.sceneManager.activeScene;
+ engine.run();
+ });
+
+ afterAll(() => {
+ engine.destroy();
+ });🤖 Prompt for AI Agents
In `@tests/src/core/Trail.test.ts` around lines 15 - 23, The async initialization
inside the describe block should be moved into a beforeAll hook and cleaned up
in afterAll: remove the async from describe("Trail", ...) and instead perform
await WebGLEngine.create({ canvas }) and assign engine, scene
(sceneManager.activeScene) inside a beforeAll block, call engine.run() there as
well, and add an afterAll that calls engine.destroy() to stop the animation
loop; update tests to reference the engine/scene variables initialized by
beforeAll.
| engine.update(); // Trail generates new vertices only after engine.update(), so we need to call it first to record initial position | ||
| trailEntity.transform.position = new Vector3(5, 0, 0); | ||
|
|
||
| // Now has trail geometry, bounds should encompass (0,0,0) to (5,0,0) expanded by halfWidth | ||
| // min: (-1, -1, -1), max: (6, 1, 1) | ||
| expect(trailRenderer.bounds.min.x).to.closeTo(-halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.min.y).to.closeTo(-halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.min.z).to.closeTo(-halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.x).to.closeTo(5 + halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.y).to.closeTo(halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.z).to.closeTo(halfWidth, 0.01); | ||
|
|
||
| // Move entity to (5, 3, 0) and update | ||
| trailEntity.transform.position = new Vector3(5, 3, 0); | ||
|
|
||
| // Bounds should encompass all points: (0,0,0), (5,0,0), (5,3,0) | ||
| // min: (-1, -1, -1), max: (6, 4, 1) | ||
| expect(trailRenderer.bounds.min.x).to.closeTo(-halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.min.y).to.closeTo(-halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.min.z).to.closeTo(-halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.x).to.closeTo(5 + halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.y).to.closeTo(3 + halfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.z).to.closeTo(halfWidth, 0.01); | ||
|
|
||
| // Test width change affects bounds | ||
| trailRenderer.width = 4.0; | ||
| const newHalfWidth = 2.0; | ||
| trailEntity.transform.position = new Vector3(5, 4, 0); | ||
|
|
||
| // Bounds with new halfWidth (2.0), encompass (0,0,0) to (5,4,0) | ||
| // min: (-2, -2, -2), max: (7, 6, 2) | ||
| expect(trailRenderer.bounds.min.x).to.closeTo(-newHalfWidth, 0.01); | ||
| expect(trailRenderer.bounds.min.y).to.closeTo(-newHalfWidth, 0.01); | ||
| expect(trailRenderer.bounds.min.z).to.closeTo(-newHalfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.x).to.closeTo(5 + newHalfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.y).to.closeTo(4 + newHalfWidth, 0.01); | ||
| expect(trailRenderer.bounds.max.z).to.closeTo(newHalfWidth, 0.01); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Trail test file and examine the full test context
fd "Trail.test.ts" --type fRepository: galacean/engine
Length of output: 87
🏁 Script executed:
# Look at the test file and surrounding code
head -300 tests/src/core/Trail.test.ts | tail -150Repository: galacean/engine
Length of output: 6060
🏁 Script executed:
# Find TrailRenderer implementation to understand bounds computation
fd -e ts -e js | xargs grep -l "class TrailRenderer" | head -5Repository: galacean/engine
Length of output: 498
🏁 Script executed:
# Examine the TrailRenderer implementation to understand bounds computation
cat -n packages/core/src/trail/TrailRenderer.ts | head -200Repository: galacean/engine
Length of output: 8164
🏁 Script executed:
# Continue reading to find the bounds property
cat -n packages/core/src/trail/TrailRenderer.ts | sed -n '200,400p'Repository: galacean/engine
Length of output: 9836
🏁 Script executed:
# Find where _updateBounds is called and how bounds property works
fd -e ts "Renderer.ts" | xargs grep -l "class Renderer" | head -5Repository: galacean/engine
Length of output: 88
🏁 Script executed:
# Check the base Renderer class to understand bounds property behavior
find packages/core/src -name "Renderer.ts" -type fRepository: galacean/engine
Length of output: 88
🏁 Script executed:
# Examine the Renderer base class, particularly the bounds property
cat -n packages/core/src/Renderer.ts | head -250Repository: galacean/engine
Length of output: 9613
🏁 Script executed:
# Search for where WorldVolume flag is set
rg "WorldVolume" packages/core/src --type ts -A 2 -B 2Repository: galacean/engine
Length of output: 15694
🏁 Script executed:
# Check if TrailRenderer overrides _onTransformChanged or handles width changes
rg "_onTransformChanged|set width" packages/core/src/trail/TrailRenderer.ts -B 2 -A 5Repository: galacean/engine
Length of output: 181
🏁 Script executed:
# Check if Transform changes automatically update vertex data or if it's deferred
cat -n packages/core/src/trail/TrailRenderer.ts | sed -n '176,200p'Repository: galacean/engine
Length of output: 1258
🏁 Script executed:
# Search for _emitNewPoint to understand when vertices are updated
rg "_emitNewPoint|_addActivePointsToVertexBuffer" packages/core/src/trail/TrailRenderer.ts -B 2 -A 8Repository: galacean/engine
Length of output: 1571
🏁 Script executed:
# Verify when TrailRenderer marks bounds dirty and check if width setter marks it
rg "set width|_dirtyUpdateFlag.*WorldVolume" packages/core/src/trail/TrailRenderer.ts -B 1 -A 1Repository: galacean/engine
Length of output: 253
🏁 Script executed:
# Check the full _emitNewPoint and understand when vertices get updated
cat -n packages/core/src/trail/TrailRenderer.ts | sed -n '400,500p'Repository: galacean/engine
Length of output: 5097
🏁 Script executed:
# Check if _onTransformChanged is overridden in TrailRenderer or if it uses base implementation
cat -n packages/core/src/trail/TrailRenderer.ts | grep -A 5 "_onTransformChanged"Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check the full _updateBounds implementation and understand the flow
cat -n packages/core/src/trail/TrailRenderer.ts | sed -n '258,315p'Repository: galacean/engine
Length of output: 2781
🏁 Script executed:
# Check if there are any engine.update() calls between assertions in the actual test
cat -n tests/src/core/Trail.test.ts | sed -n '185,235p'Repository: galacean/engine
Length of output: 2993
🏁 Script executed:
# Check when Transform changes trigger _onTransformChanged callback
fd -e ts "Transform.ts" | head -1 | xargs cat | head -150Repository: galacean/engine
Length of output: 4767
🏁 Script executed:
# Look for how position setter triggers transform changed events
rg "position.*set.*{|_onTransformChanged" packages/core/src/Transform.ts -A 3 -B 1 | head -40Repository: galacean/engine
Length of output: 193
🏁 Script executed:
# Look for _onPositionChanged callback
rg "_onPositionChanged|_onTransformChanged" packages/core/src/Transform.ts -B 2 -A 5 | head -60Repository: galacean/engine
Length of output: 1534
🏁 Script executed:
# Find how Transform changes notify Renderer components
rg "_onTransformChanged|TransformModifyFlags" packages/core/src/Renderer.ts -B 2 -A 3Repository: galacean/engine
Length of output: 1114
🏁 Script executed:
# Verify that width setter does NOT set WorldVolume flag and check if anything else does
cat -n packages/core/src/trail/TrailRenderer.ts | sed -n '125,135p'Repository: galacean/engine
Length of output: 320
🏁 Script executed:
# Double-check: does changing width eventually trigger _updateBounds?
rg "width|halfWidth" packages/core/src/trail/TrailRenderer.ts | grep -v "minWidth\|maxWidth"Repository: galacean/engine
Length of output: 1202
Width setter needs to mark bounds dirty for immediate recalculation.
The bounds assertions are correct for position changes (Transform updates automatically trigger WorldVolume flag recalculation), but the width change at line 217 doesn't invalidate bounds. Since the subsequent position change at line 219 coincidentally triggers recalculation with the new width, the test passes—but it's fragile and could fail if width changes without a following position change.
Only the width change needs to be addressed by either marking bounds dirty in the setter or calling engine.update() after width changes. Position changes automatically trigger bounds recalculation through the Transform callback system.
✅ Invalidate bounds on width change
set width(value: number) {
this._trailParams.x = value;
+ this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume;
}Alternatively, add engine.update() only after width changes:
trailRenderer.width = 4.0;
+ engine.update();🤖 Prompt for AI Agents
In `@tests/src/core/Trail.test.ts` around lines 192 - 229, The TrailRenderer width
setter currently updates the width but does not invalidate bounds, so change the
TrailRenderer.width setter to mark bounds dirty so they get recalculated
immediately: when setting width in the width setter (symbol:
TrailRenderer.width), set the internal dirty flag (e.g., this._boundsDirty =
true) or call the existing bounds invalidation helper (e.g.,
this._markBoundsDirty() or this.entity.transform.markWorldVolumeDirty()) so
subsequent assertions/readers get updated bounds without relying on a later
position change.
4 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Materials
New Example
Tests
Other
✏️ Tip: You can customize this high-level summary in your review settings.