optimize TrailRenderer texture scale and remove widthMultiplier#2889
optimize TrailRenderer texture scale and remove widthMultiplier#2889GuoLei1990 wants to merge 5 commits intogalacean:dev/2.0from
Conversation
WalkthroughTrailRenderer API and internals updated: public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2889 +/- ##
===========================================
- Coverage 83.03% 83.03% -0.01%
===========================================
Files 796 796
Lines 90449 90450 +1
Branches 9503 9497 -6
===========================================
- Hits 75107 75101 -6
- Misses 15260 15267 +7
Partials 82 82
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
🤖 Fix all issues with AI agents
In `@packages/core/src/trail/TrailRenderer.ts`:
- Around line 161-167: The cloned TrailRenderer's deep-cloned _textureScale
Vector2 loses its _onValueChanged handler so changes to textureScale won't
propagate; add an override of _cloneTo on TrailRenderer that calls
super._cloneTo(target, srcRoot, targetRoot) and then re-wire the handler by
setting target._textureScale._onValueChanged =
target._onTextureScaleChanged.bind(target) (use the _textureScale,
_onValueChanged, and _onTextureScaleChanged symbols to locate fields/methods).
🧹 Nitpick comments (1)
tests/src/core/Trail.test.ts (1)
107-119: Good test coverage of both mutation paths (setter and direct.set()).This validates that both
textureScale = new Vector2(...)andtextureScale.set(...)correctly update the property. Consider also adding a test that mutates individual components (e.g.,textureScale.x = 3.0) to fully cover the_onValueChangedhandler, though this is optional.
| constructor(entity: Entity) { | ||
| super(entity); | ||
| // @ts-ignore | ||
| this._textureScale._onValueChanged = this._onTextureScaleChanged.bind(this); | ||
| this._onTextureScaleChanged(); | ||
| this._initGeometry(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the TrailRenderer class structure
cat -n packages/core/src/trail/TrailRenderer.ts | head -200Repository: galacean/engine
Length of output: 8156
🏁 Script executed:
# Check the parent class (Renderer) to understand cloning behavior
fd -e ts -p "Renderer" packages/core/src | head -20Repository: galacean/engine
Length of output: 442
🏁 Script executed:
# Look for _onClone or similar clone lifecycle methods
rg -n "_onClone|_cloneTo|clone\(" --type=ts packages/core/src | head -30Repository: galacean/engine
Length of output: 2937
🏁 Script executed:
# Check for _onValueChanged usage patterns
rg -n "_onValueChanged" --type=ts -C 2 packages/core/src | head -50Repository: galacean/engine
Length of output: 4078
🏁 Script executed:
# Search for _cloneTo in TrailRenderer
rg -n "_cloneTo" packages/core/src/trail/TrailRenderer.tsRepository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check the Renderer base class for cloning
cat -n packages/core/src/Renderer.ts | head -100Repository: galacean/engine
Length of output: 4595
🏁 Script executed:
# Look at CloneManager to understand how `@deepClone` works
rg -n "deepClone|@deepClone" packages/core/src/clone/CloneManager.ts | head -40Repository: galacean/engine
Length of output: 239
🏁 Script executed:
# Check ComponentCloner to see how cloning is done for components
cat -n packages/core/src/clone/ComponentCloner.tsRepository: galacean/engine
Length of output: 1667
🏁 Script executed:
# Search for Renderer._cloneTo method
rg -n "_cloneTo" packages/core/src/Renderer.ts -A 15Repository: galacean/engine
Length of output: 498
🏁 Script executed:
# Examine CloneManager.cloneProperty to understand how `@deepClone` works
rg -n "cloneProperty" packages/core/src/clone/CloneManager.ts -A 20 | head -100Repository: galacean/engine
Length of output: 2380
🏁 Script executed:
# Look for the deepCloneObject implementation
rg -n "deepCloneObject" packages/core/src/clone/CloneManager.ts -A 30 | head -80Repository: galacean/engine
Length of output: 323
Cloning bug: _onValueChanged handler won't be re-wired on the cloned _textureScale.
When a TrailRenderer is cloned, the deep-cloned _textureScale Vector2 lacks the _onValueChanged callback that's set up in the constructor. Since the constructor is not invoked during cloning, mutations to the clone's textureScale (e.g., .set(...) or .x = ...) will silently fail to sync to _trailParams.z/w.
Add a _cloneTo override to re-wire the callback on the cloned instance:
override _cloneTo(target: TrailRenderer, srcRoot: Entity, targetRoot: Entity): void {
super._cloneTo(target, srcRoot, targetRoot);
// `@ts-ignore`
target._textureScale._onValueChanged = target._onTextureScaleChanged.bind(target);
}🤖 Prompt for AI Agents
In `@packages/core/src/trail/TrailRenderer.ts` around lines 161 - 167, The cloned
TrailRenderer's deep-cloned _textureScale Vector2 loses its _onValueChanged
handler so changes to textureScale won't propagate; add an override of _cloneTo
on TrailRenderer that calls super._cloneTo(target, srcRoot, targetRoot) and then
re-wire the handler by setting target._textureScale._onValueChanged =
target._onTextureScaleChanged.bind(target) (use the _textureScale,
_onValueChanged, and _onTextureScaleChanged symbols to locate fields/methods).
GuoLei1990
changed the title
… adjust shader usage
GuoLei1990
changed the title
GuoLei1990
changed the title
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 604-609: Remove the meaningless `@ignoreClone` decorator from the
method declaration of _onTextureScaleChanged because `@ignoreClone` is a property
decorator for instance fields and has no effect on prototype methods; edit the
TrailRenderer class to delete the `@ignoreClone` annotation above the private
method _onTextureScaleChanged so the method remains unchanged in behavior but no
longer includes an incorrect decorator.
🧹 Nitpick comments (1)
tests/src/core/Trail.test.ts (1)
94-106: Good coverage of both assignment and mutation paths.Consider adding a clone test for
TrailRendererthat verifiestextureScalechanges on the cloned instance propagate correctly — this would catch the_onValueChangedre-wiring bug flagged above.
|
|
||
| @ignoreClone | ||
| private _onTextureScaleChanged(): void { | ||
| this._trailParams.y = this._textureScale.x; | ||
| this._trailParams.z = this._textureScale.y; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the `@ignoreClone` decorator definition and understand its implementation
rg -n 'export.*ignoreClone|function.*ignoreClone|const.*ignoreClone' --type=ts -A 5 packages/coreRepository: galacean/engine
Length of output: 534
🏁 Script executed:
# Check the provided script - search for `@ignoreClone` on methods
rg -nP '@ignoreClone\s*\n\s*(private|protected|public)?\s+\w+\(' --type=ts packages/core/src | head -20Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Also look for `@ignoreClone` usage on properties vs methods to understand the pattern
rg -n '@ignoreClone' --type=ts -B 1 packages/core/src | head -40Repository: galacean/engine
Length of output: 2567
🏁 Script executed:
# Verify how CloneManager.registerCloneMode works - check if it could affect methods
rg -n 'registerCloneMode|CloneMode\.Ignore' --type=ts -B 2 -A 2 packages/core/src/clone/CloneManager.ts | head -30Repository: galacean/engine
Length of output: 986
🏁 Script executed:
# Check if there's any special handling for methods in the clone logic
rg -n 'clone|cloneMode' --type=ts packages/core/src/clone/CloneManager.ts | head -20Repository: galacean/engine
Length of output: 1114
🏁 Script executed:
# Double-check the specific method in TrailRenderer.ts to confirm it's a method
sed -n '604,609p' packages/core/src/trail/TrailRenderer.tsRepository: galacean/engine
Length of output: 217
Remove @ignoreClone from the method — it has no effect.
@ignoreClone is a property decorator designed for instance properties. Methods are shared across instances via the prototype and are never cloned, so this decorator is a no-op and should be removed.
Proposed fix
- `@ignoreClone`
private _onTextureScaleChanged(): void {
this._trailParams.y = this._textureScale.x;
this._trailParams.z = this._textureScale.y;
}📝 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.
| @ignoreClone | |
| private _onTextureScaleChanged(): void { | |
| this._trailParams.y = this._textureScale.x; | |
| this._trailParams.z = this._textureScale.y; | |
| } | |
| private _onTextureScaleChanged(): void { | |
| this._trailParams.y = this._textureScale.x; | |
| this._trailParams.z = this._textureScale.y; | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/src/trail/TrailRenderer.ts` around lines 604 - 609, Remove the
meaningless `@ignoreClone` decorator from the method declaration of
_onTextureScaleChanged because `@ignoreClone` is a property decorator for instance
fields and has no effect on prototype methods; edit the TrailRenderer class to
delete the `@ignoreClone` annotation above the private method
_onTextureScaleChanged so the method remains unchanged in behavior but no longer
includes an incorrect decorator.
2 participants
Summary
textureScalefromnumbertoVector2, enabling independent X/Y texture scaling (along the trail vs across the trail)widthMultiplierAPI — trail width is now fully controlled bywidthCurve, simplifying the APIrenderer_TrailParamsshader uniform layout: x → textureMode, y → textureScaleX, z → textureScaleYBreaking Changes
widthMultiplierremoved: UsewidthCurveinstead. e.g.trail.widthMultiplier = 2.0withCurveKey(0, 1)→CurveKey(0, 2)directlytextureScaletype changed: FromnumbertoVector2, enabling separate X/Y scale controlTest plan
Summary by CodeRabbit