Fix compont props clone bug#2926
Conversation
|
Caution Review failedPull request was closed or merged during review 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:
WalkthroughRemoved srcRoot/targetRoot params from many _cloneTo overrides, added _remap helpers on Entity/Component that delegate to CloneUtils, and refactored CloneManager/ComponentCloner to centralize cloning with early remap, unified ICustomClone shape, and consolidated deep/object cloning logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Requestor as Requestor
participant CloneMgr as CloneManager
participant ComponentCloner as ComponentCloner
participant CloneUtils as CloneUtils
participant Source as Source
participant Target as Target
Requestor->>CloneMgr: start clone(srcRoot, targetRoot)
CloneMgr->>Source: inspect property/value
alt Source implements _remap
Source->>CloneUtils: _remap(srcRoot, targetRoot)
CloneUtils-->>CloneMgr: remapped object or original
CloneMgr-->>Target: assign remapped object
else no _remap
CloneMgr->>Source: determine cloneMode (primitive/assignment/deep)
alt primitive/assignment
CloneMgr-->>Target: direct assign
else deep/object
CloneMgr->>ComponentCloner: ensure/allocate target instance
alt source has copyFrom
ComponentCloner->>Target: target.copyFrom(source)
else source has _cloneTo
ComponentCloner->>Source: source._cloneTo?(target, srcRoot?, targetRoot?)
Source-->>ComponentCloner: custom clone applied
else
ComponentCloner->>Source: recursively clone fields
end
ComponentCloner-->>CloneMgr: assign cloned object
end
end
CloneMgr-->>Requestor: cloning complete
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🧪 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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2926 +/- ##
===========================================
- Coverage 77.29% 77.26% -0.04%
===========================================
Files 884 884
Lines 96638 96585 -53
Branches 9509 9496 -13
===========================================
- Hits 74697 74626 -71
- Misses 21777 21795 +18
Partials 164 164
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:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/mesh/Skin.ts (1)
18-27:⚠️ Potential issue | 🔴 CriticalCloning
Skinnow skips the only_skinMatricesinitialization path.
_bonesis deep-cloned directly, so thebonessetter never runs. That setter is the only place_skinMatricesgets sized, which leaves cloned skins without a backing buffer when_updateSkinMatrices()writes into it.rootBonealso still participates in skinning but is no longer part of the explicit clone setup, so this type still needs a small post-clone hook.Possible direction
- `@ignoreClone` + `@deepClone` private _rootBone: Entity;_cloneTo(target: Skin): void { target._skinMatrices = new Float32Array(target._bones.length * 16); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/mesh/Skin.ts` around lines 18 - 27, Cloned Skin instances miss initialization of the backing buffer because _bones is deep-cloned (bypassing the bones setter) and rootBone isn't re-hooked; update Skin._cloneTo(target: Skin) to allocate target._skinMatrices = new Float32Array(target._bones.length * 16) after cloning bones and ensure the target.rootBone is set/registered the same way original rootBone is (so _updateSkinMatrices and skinning use the same rootBone), preserving any UpdateFlagManager state if needed.
🧹 Nitpick comments (2)
tests/src/core/CloneUtils.test.ts (1)
18-21: Consider engine cleanup after all tests.The engine is created once and
engine.run()is called, but the engine is never destroyed after the test suite completes. While this may not cause issues in the current test environment, adding cleanup ensures resources are properly released.♻️ Suggested improvement
describe("CloneUtils", async () => { const engine = await WebGLEngine.create({ canvas: document.createElement("canvas") }); const scene = engine.sceneManager.activeScene; engine.run(); + + afterAll(() => { + engine.destroy(); + });You'll need to import
afterAllfrom vitest:-import { describe, expect, it } from "vitest"; +import { afterAll, describe, expect, it } from "vitest";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/CloneUtils.test.ts` around lines 18 - 21, The test creates a persistent WebGLEngine via WebGLEngine.create and calls engine.run() but never tears it down; add a vitest afterAll hook (import afterAll) that calls the appropriate teardown method on the engine (e.g., engine.destroy() or engine.dispose() depending on the API) to stop the engine and release resources after the test suite; ensure the engine variable is in a scope accessible to afterAll so the cleanup runs even if tests fail.packages/ui/src/component/UICanvas.ts (1)
20-20: Unused import:CloneUtils.The
CloneUtilsimport is no longer used after removing the camera remapping logic. Consider removing it to keep imports clean.♻️ Proposed fix
import { BoolUpdateFlag, Camera, CameraModifyFlags, Component, DependentMode, DisorderedArray, Entity, EntityModifyFlags, Logger, MathUtil, Matrix, Ray, Vector2, Vector3, assignmentClone, deepClone, dependentComponents, - ignoreClone, - CloneUtils + ignoreClone } from "@galacean/engine";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/component/UICanvas.ts` at line 20, Remove the unused import CloneUtils from the UICanvas module: locate the import statement that includes CloneUtils (symbol: CloneUtils) and delete it, verify there are no remaining references to CloneUtils in UICanvas (e.g., in functions or JSX) and run the linter/build to ensure no unresolved identifiers remain.
🤖 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 106-114: In CloneManager.ts adjust the clone logic so the
CloneMode.Ignore check runs before the _remap fast path: when iterating
properties (sourceProperty / source[k]) first test if cloneMode ===
CloneMode.Ignore and return if so, and only afterwards handle the case where
sourceProperty instanceof Object and (<ICustomClone>sourceProperty)._remap by
calling _remap(srcRoot, targetRoot); this ensures `@ignoreClone` fields (e.g.,
Skin._rootBone, UIInteractive._rootCanvas/_group) are not remapped prematurely.
In `@packages/ui/src/component/interactive/UIInteractive.ts`:
- Around line 53-54: The `@deepClone` on UIInteractive._transitions causes new
Transition instances to be created but their internal mutable fields (_normal,
_pressed, _hover, _disabled, _initialValue, _finalValue, _currentValue) are
still shared; fix this by adding proper clone support on the Transition type
(implement a copyFrom() or _cloneTo() on Transition and its subclasses or add
field-level clone decorators to those mutable fields) so deep cloning copies
their internal state, or alternatively remove `@deepClone` on _transitions and
document/accept shallow cloning semantics—update the Transition class (and any
subclasses) accordingly to ensure cloned UIInteractive instances do not share
mutable transition state.
In `@packages/ui/src/component/UICanvas.ts`:
- Around line 416-418: The _cloneTo method on UICanvas no longer remaps the
`@ignoreClone` _camera so cloned UICanvas instances in ScreenSpaceCamera mode keep
a reference to the original camera; restore camera remapping by invoking the
camera's remap (or _remap) on the cloned instance inside UICanvas._cloneTo
(e.g., if target._camera exists call target._camera =
this._camera?._remap(target) or use the same remapping utility previously used),
and remove the now-unused CloneUtils import if you replace its usage; ensure you
reference _cloneTo, UICanvas, _camera, _remap, ScreenSpaceCamera, and CloneUtils
when making the change.
---
Outside diff comments:
In `@packages/core/src/mesh/Skin.ts`:
- Around line 18-27: Cloned Skin instances miss initialization of the backing
buffer because _bones is deep-cloned (bypassing the bones setter) and rootBone
isn't re-hooked; update Skin._cloneTo(target: Skin) to allocate
target._skinMatrices = new Float32Array(target._bones.length * 16) after cloning
bones and ensure the target.rootBone is set/registered the same way original
rootBone is (so _updateSkinMatrices and skinning use the same rootBone),
preserving any UpdateFlagManager state if needed.
---
Nitpick comments:
In `@packages/ui/src/component/UICanvas.ts`:
- Line 20: Remove the unused import CloneUtils from the UICanvas module: locate
the import statement that includes CloneUtils (symbol: CloneUtils) and delete
it, verify there are no remaining references to CloneUtils in UICanvas (e.g., in
functions or JSX) and run the linter/build to ensure no unresolved identifiers
remain.
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 18-21: The test creates a persistent WebGLEngine via
WebGLEngine.create and calls engine.run() but never tears it down; add a vitest
afterAll hook (import afterAll) that calls the appropriate teardown method on
the engine (e.g., engine.destroy() or engine.dispose() depending on the API) to
stop the engine and release resources after the test suite; ensure the engine
variable is in a scope accessible to afterAll so the cleanup runs even if tests
fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fcd9300e-e2ff-40a4-9976-abeb02cae213
📒 Files selected for processing (23)
packages/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/ComponentCloner.tspackages/core/src/mesh/MeshRenderer.tspackages/core/src/mesh/Skin.tspackages/core/src/mesh/SkinnedMeshRenderer.tspackages/core/src/particle/modules/shape/MeshShape.tspackages/ui/src/component/UICanvas.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.tstests/src/core/CloneUtils.test.ts
| _cloneTo(target: UICanvas): void { | ||
| target.renderMode = this._renderMode; | ||
| const camera = this._camera; | ||
| if (camera) { | ||
| target.camera = CloneUtils.remapComponent(srcRoot, targetRoot, camera); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any other mechanism handling camera remapping for UICanvas
rg -n "_remap|remapComponent" packages/ui/src/component/UICanvas.ts
# Check how camera is used in the cloning context
rg -nA3 "UICanvas.*clone|clone.*UICanvas" packages/Repository: galacean/engine
Length of output: 310
🏁 Script executed:
# Check the _camera field decoration and surrounding code
sed -n '80,90p' packages/ui/src/component/UICanvas.ts
# Check for CloneUtils import
rg -n "import.*CloneUtils|from.*CloneUtils" packages/ui/src/component/UICanvas.ts
# Check if there's a _remap method elsewhere in UICanvas
ast-grep --pattern '$$$
_remap($_) {
$$$
}
$$$' packages/ui/src/component/UICanvas.ts
# Look for CloneManager to understand clone behavior
rg -n "ignoreClone" packages/core/src/Clone/CloneManager.ts -A 2 -B 2Repository: galacean/engine
Length of output: 487
🏁 Script executed:
# Find CloneManager file
fd -t f "CloneManager" packages/
# Check how `@ignoreClone` decorator affects cloning
rg -n "ignoreClone" packages/ -t ts --max-count 10
# Look for the clone implementation and how it handles `@ignoreClone`
rg -n "@ignoreClone" packages/core/src/ -A 3 -B 3 | head -50
# Check if there's camera-specific cloning logic elsewhere
rg -n "camera.*clone|clone.*camera" packages/ui/src/component/UICanvas.ts -i
# Check the import statements in UICanvas.ts
head -30 packages/ui/src/component/UICanvas.tsRepository: galacean/engine
Length of output: 25490
🏁 Script executed:
# Check if CloneUtils is actually used in UICanvas.ts
rg -n "CloneUtils" packages/ui/src/component/UICanvas.ts
# Read the CloneManager to understand how `@ignoreClone` fields are handled
head -150 packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 5243
🏁 Script executed:
# Check if Camera has a _remap method
rg -n "_remap" packages/core/src/Camera.ts
# Also search for _remap methods in the codebase to understand the pattern
rg -n "^\s*_remap\(" packages/core/src/ -A 3 | head -50
# Look for ICustomClone interface to understand _remap contract
rg -n "ICustomClone" packages/core/src/clone/ -A 5Repository: galacean/engine
Length of output: 3807
🏁 Script executed:
# Check if Camera has a _remap method
rg -n "_remap" packages/core/src/Camera.ts
# Also verify what happens when a property has `@ignoreClone` decorator
# The behavior should be: if the property itself is marked `@ignoreClone`, it's skipped
# Let's confirm this by looking at the cloneProperty method more carefully
sed -n '100,120p' packages/core/src/clone/CloneManager.ts
# Check the full signature of cloneProperty to understand the flow
sed -n '100,200p' packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 4097
🏁 Script executed:
# Verify all usages of CloneUtils in UICanvas.ts
rg -n "CloneUtils" packages/ui/src/component/UICanvas.ts -B 2 -A 2
# Let's also check the git history or current state - look at the full _cloneTo method
# and surrounding code to understand the context better
sed -n '410,425p' packages/ui/src/component/UICanvas.ts
# Check what the old _cloneTo looked like by searching for references to remapComponent
rg -n "remapComponent" packages/ --max-count 10Repository: galacean/engine
Length of output: 1122
🏁 Script executed:
# Final check: confirm Camera class doesn't have _remap method
rg -n "_remap" packages/core/src/Camera.ts
# Also verify the render mode behavior - is it intentional that it's `@ignoreClone` yet manually assigned in _cloneTo?
sed -n '80,90p' packages/ui/src/component/UICanvas.tsRepository: galacean/engine
Length of output: 390
Camera remapping was removed without an alternative mechanism.
The previous camera remapping via CloneUtils.remapComponent() in _cloneTo has been removed. Since _camera is marked @ignoreClone, it is skipped by the automatic cloning mechanism and won't be remapped. Without a _remap method on Camera or a manual assignment in _cloneTo, a cloned UICanvas in ScreenSpaceCamera mode will reference the original camera instead of a cloned one.
Additionally, the CloneUtils import is now unused and can be removed.
🤖 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 416 - 418, The _cloneTo
method on UICanvas no longer remaps the `@ignoreClone` _camera so cloned UICanvas
instances in ScreenSpaceCamera mode keep a reference to the original camera;
restore camera remapping by invoking the camera's remap (or _remap) on the
cloned instance inside UICanvas._cloneTo (e.g., if target._camera exists call
target._camera = this._camera?._remap(target) or use the same remapping utility
previously used), and remove the now-unused CloneUtils import if you replace its
usage; ensure you reference _cloneTo, UICanvas, _camera, _remap,
ScreenSpaceCamera, and CloneUtils when making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/src/core/CloneUtils.test.ts (1)
289-303:undefinedpath is declared but not asserted in the null/undefined test.The test name covers both null and undefined, but
undefinedEntityis never checked. Add an explicit assertion to lock this behavior.Minimal assertion addition
it("null entity/component refs should not crash and remain null", () => { @@ expect(cs.nullEntity).eq(null); + expect(cs.undefinedEntity).eq(undefined); expect(cs.nullRenderer).eq(null); expect(cs.someNumber).eq(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/CloneUtils.test.ts` around lines 289 - 303, The test "null entity/component refs should not crash and remain null" asserts null fields but omits asserting the undefined field declared on NullRefScript; after obtaining cs via cloned.getComponent(NullRefScript) add an explicit assertion for the undefined path (e.g., expect(cs.undefinedEntity).eq(undefined)) inside that test so the undefined behavior is locked down alongside null checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 1-9: The test file has Prettier/ESLint formatting violations in
the import list (Entity, MeshRenderer, Script, Signal, assignmentClone,
deepClone, ignoreClone) and an inline closure body later in the file; reformat
the import block into a single-line or properly multiline style per project
Prettier settings and update the inline closure to use the preferred brace/arrow
formatting (or run the project's formatter). Run the project's formatter/ESLint
--fix (or Prettier) on tests/src/core/CloneUtils.test.ts to apply consistent
formatting and ensure the import list and the inline closure body comply with
lint rules before merging.
- Around line 116-705: The tests leak scene state because each it() calls
rootEntity.destroy() only on the happy path; make teardown failure-safe by
ensuring root entities are always destroyed: either add an afterEach() that
tracks and destroys any created root entities (e.g., collect rootEntity
instances created in each it into a local array) or wrap each test body in a
try/finally that calls rootEntity.destroy() in finally; update references in the
test file CloneUtils.test.ts (look for variables named rootEntity created in
multiple describe/it blocks and the existing rootEntity.destroy() calls) to use
the new tear-down pattern so leaked entities cannot persist between tests.
- Around line 111-115: The test currently awaits WebGLEngine.create inside an
async describe block which runs during test collection; move engine
initialization and scene setup into a beforeAll hook and teardown into afterAll:
create the engine via WebGLEngine.create, assign engine and scene
(engine.sceneManager.activeScene), call engine.run() in beforeAll, and
stop/dispose/cleanup the engine in afterAll so tests use a ready engine; update
references to the shared engine variable declared in the describe scope and
ensure beforeAll/afterAll handle async awaits.
---
Nitpick comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 289-303: The test "null entity/component refs should not crash and
remain null" asserts null fields but omits asserting the undefined field
declared on NullRefScript; after obtaining cs via
cloned.getComponent(NullRefScript) add an explicit assertion for the undefined
path (e.g., expect(cs.undefinedEntity).eq(undefined)) inside that test so the
undefined behavior is locked down alongside null checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d36fbba-ae4d-4811-986f-cd4fb9a64c24
📒 Files selected for processing (1)
tests/src/core/CloneUtils.test.ts
| import { | ||
| Entity, | ||
| MeshRenderer, | ||
| Script, | ||
| Signal, | ||
| assignmentClone, | ||
| deepClone, | ||
| ignoreClone | ||
| } from "@galacean/engine-core"; |
There was a problem hiding this comment.
Prettier violations are currently failing lint.
The import list formatting (Line 1-9) and inline closure body formatting (Line 544) match the ESLint/prettier errors. Please run formatter or apply the lint fixes before merge.
Also applies to: 544-544
🧰 Tools
🪛 ESLint
[error] 1-9: Replace ⏎··Entity,⏎··MeshRenderer,⏎··Script,⏎··Signal,⏎··assignmentClone,⏎··deepClone,⏎··ignoreClone⏎ with ·Entity,·MeshRenderer,·Script,·Signal,·assignmentClone,·deepClone,·ignoreClone·
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/core/CloneUtils.test.ts` around lines 1 - 9, The test file has
Prettier/ESLint formatting violations in the import list (Entity, MeshRenderer,
Script, Signal, assignmentClone, deepClone, ignoreClone) and an inline closure
body later in the file; reformat the import block into a single-line or properly
multiline style per project Prettier settings and update the inline closure to
use the preferred brace/arrow formatting (or run the project's formatter). Run
the project's formatter/ESLint --fix (or Prettier) on
tests/src/core/CloneUtils.test.ts to apply consistent formatting and ensure the
import list and the inline closure body comply with lint rules before merging.
| describe("Clone remap", async () => { | ||
| const engine = await WebGLEngine.create({ canvas: document.createElement("canvas") }); | ||
| const scene = engine.sceneManager.activeScene; | ||
| engine.run(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest 2.x, is using an async callback in describe()supported for test definition, or should async setup be done inbeforeAll instead?
💡 Result:
In Vitest 2.x you can technically write describe('x', async () => { ... }) (it’s not considered invalid by Vitest’s own ESLint rule set) [1], and you’ll even see async () => {} used in docs examples (notably under describe.concurrent) [2].
However, you should not rely on await inside describe() to perform async setup that your tests depend on, because describe bodies run during the test collection phase (tests are registered as a side effect of executing the file), and async setup belongs in hooks that run before execution of tests—typically beforeAll for once-per-suite setup [3].
Recommendation: keep describe() for synchronous test definition, and put async setup in beforeAll (or beforeEach if needed). [3]
Sources: [1] [2] [3]
Move engine initialization from describe callback to beforeAll hook.
Line 111 uses an async describe callback. During test collection (when describe bodies execute), the engine creation and assignment are non-blocking—tests can collect and register before the engine is ready. Move const engine = await WebGLEngine.create(...) and scene setup into a beforeAll hook to ensure the engine is initialized before any test runs. Use afterAll for cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/core/CloneUtils.test.ts` around lines 111 - 115, The test currently
awaits WebGLEngine.create inside an async describe block which runs during test
collection; move engine initialization and scene setup into a beforeAll hook and
teardown into afterAll: create the engine via WebGLEngine.create, assign engine
and scene (engine.sceneManager.activeScene), call engine.run() in beforeAll, and
stop/dispose/cleanup the engine in afterAll so tests use a ready engine; update
references to the shared engine variable declared in the describe scope and
ensure beforeAll/afterAll handle async awaits.
| describe("Basic Entity/Component remap", () => { | ||
| it("script undecorated Entity ref should remap to cloned entity", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = parent.addComponent(TestScript); | ||
| script.targetEntity = child; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
| const clonedChild = clonedParent.children[0]; | ||
|
|
||
| expect(clonedScript.targetEntity).not.eq(child); | ||
| expect(clonedScript.targetEntity).eq(clonedChild); | ||
| expect(clonedScript.targetEntity.name).eq("child"); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script undecorated Component ref should remap to cloned component", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const meshRenderer = child.addComponent(MeshRenderer); | ||
| const script = parent.addComponent(TestScript); | ||
| script.targetRenderer = meshRenderer; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
| const clonedChild = clonedParent.children[0]; | ||
| const clonedMeshRenderer = clonedChild.getComponent(MeshRenderer); | ||
|
|
||
| expect(clonedScript.targetRenderer).not.eq(meshRenderer); | ||
| expect(clonedScript.targetRenderer).eq(clonedMeshRenderer); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script ref to entity outside hierarchy should keep original", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const external = rootEntity.createChild("external"); | ||
| const script = parent.addComponent(TestScript); | ||
| script.externalEntity = external; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
|
|
||
| expect(clonedScript.externalEntity).eq(external); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script ref to component outside hierarchy should keep original", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const external = rootEntity.createChild("external"); | ||
| const externalMR = external.addComponent(MeshRenderer); | ||
| const script = parent.addComponent(TestScript); | ||
| script.externalRenderer = externalMR; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
|
|
||
| expect(clonedScript.externalRenderer).eq(externalMR); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("deep hierarchy entity ref should remap correctly", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const grandchild = child.createChild("grandchild"); | ||
| const script = parent.addComponent(TestScript); | ||
| script.deepChild = grandchild; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
| const clonedGrandchild = clonedParent.children[0].children[0]; | ||
|
|
||
| expect(clonedScript.deepChild).not.eq(grandchild); | ||
| expect(clonedScript.deepChild).eq(clonedGrandchild); | ||
| expect(clonedScript.deepChild.name).eq("grandchild"); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script ref to self entity (clone root) should remap", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const script = parent.addComponent(TestScript); | ||
| script.selfRef = parent; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
|
|
||
| expect(clonedScript.selfRef).not.eq(parent); | ||
| expect(clonedScript.selfRef).eq(clonedParent); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("primitive and plain object props should not be affected", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const script = parent.addComponent(TestScript); | ||
| const obj = { x: 1 }; | ||
| script.speed = 42; | ||
| script.name2 = "test"; | ||
| script.flag = true; | ||
| script.data = obj; | ||
|
|
||
| const clonedParent = parent.clone(); | ||
| const clonedScript = clonedParent.getComponent(TestScript); | ||
|
|
||
| expect(clonedScript.speed).eq(42); | ||
| expect(clonedScript.name2).eq("test"); | ||
| expect(clonedScript.flag).eq(true); | ||
| expect(clonedScript.data).eq(obj); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Multiple and duplicate refs", () => { | ||
| it("multiple entity/component refs on same script all remap independently", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const childA = parent.createChild("childA"); | ||
| const childB = parent.createChild("childB"); | ||
| const mrA = childA.addComponent(MeshRenderer); | ||
| const mrB = childB.addComponent(MeshRenderer); | ||
| const script = parent.addComponent(MultiRefScript); | ||
| script.entityA = childA; | ||
| script.entityB = childB; | ||
| script.rendererA = mrA; | ||
| script.rendererB = mrB; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(MultiRefScript); | ||
|
|
||
| expect(cs.entityA).not.eq(childA); | ||
| expect(cs.entityB).not.eq(childB); | ||
| expect(cs.entityA.name).eq("childA"); | ||
| expect(cs.entityB.name).eq("childB"); | ||
| expect(cs.entityA).eq(cloned.children[0]); | ||
| expect(cs.entityB).eq(cloned.children[1]); | ||
| expect(cs.rendererA).eq(cloned.children[0].getComponent(MeshRenderer)); | ||
| expect(cs.rendererB).eq(cloned.children[1].getComponent(MeshRenderer)); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("two properties referencing the same entity both remap to the same cloned entity", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = parent.addComponent(DuplicateRefScript); | ||
| script.ref1 = child; | ||
| script.ref2 = child; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(DuplicateRefScript); | ||
|
|
||
| expect(cs.ref1).not.eq(child); | ||
| expect(cs.ref1).eq(cs.ref2); | ||
| expect(cs.ref1).eq(cloned.children[0]); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Null and undefined refs", () => { | ||
| it("null entity/component refs should not crash and remain null", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const script = parent.addComponent(NullRefScript); | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(NullRefScript); | ||
|
|
||
| expect(cs.nullEntity).eq(null); | ||
| expect(cs.nullRenderer).eq(null); | ||
| expect(cs.someNumber).eq(0); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Sibling entity refs", () => { | ||
| it("ref to sibling entity under clone root should remap", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const childA = parent.createChild("childA"); | ||
| const childB = parent.createChild("childB"); | ||
| const mrB = childB.addComponent(MeshRenderer); | ||
| const script = childA.addComponent(SiblingRefScript); | ||
| script.sibling = childB; | ||
| script.siblingRenderer = mrB; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const clonedChildA = cloned.children[0]; | ||
| const clonedChildB = cloned.children[1]; | ||
| const cs = clonedChildA.getComponent(SiblingRefScript); | ||
|
|
||
| expect(cs.sibling).not.eq(childB); | ||
| expect(cs.sibling).eq(clonedChildB); | ||
| expect(cs.siblingRenderer).not.eq(mrB); | ||
| expect(cs.siblingRenderer).eq(clonedChildB.getComponent(MeshRenderer)); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Clone decorator interaction with _remap", () => { | ||
| it("@assignmentClone entity ref still gets remapped via _remap priority", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = parent.addComponent(DecoratedRefScript); | ||
| script.assignedEntity = child; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(DecoratedRefScript); | ||
|
|
||
| expect(cs.assignedEntity).not.eq(child); | ||
| expect(cs.assignedEntity).eq(cloned.children[0]); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("@ignoreClone entity ref still gets remapped via _remap priority", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = parent.addComponent(DecoratedRefScript); | ||
| script.ignoredEntity = child; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(DecoratedRefScript); | ||
|
|
||
| expect(cs.ignoredEntity).not.eq(child); | ||
| expect(cs.ignoredEntity).eq(cloned.children[0]); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("undecorated entity ref remaps correctly", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = parent.addComponent(DecoratedRefScript); | ||
| script.autoRemapEntity = child; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(DecoratedRefScript); | ||
|
|
||
| expect(cs.autoRemapEntity).not.eq(child); | ||
| expect(cs.autoRemapEntity).eq(cloned.children[0]); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("@ignoreClone entity ref outside hierarchy stays original", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const external = rootEntity.createChild("external"); | ||
| const script = parent.addComponent(DecoratedRefScript); | ||
| script.ignoredEntity = external; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(DecoratedRefScript); | ||
|
|
||
| expect(cs.ignoredEntity).eq(external); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("@deepClone array of entities", () => { | ||
| it("deep cloned entity array should remap internal refs", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const childA = parent.createChild("childA"); | ||
| const childB = parent.createChild("childB"); | ||
| const script = parent.addComponent(ArrayRefScript); | ||
| script.entities = [childA, childB]; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(ArrayRefScript); | ||
|
|
||
| expect(cs.entities).not.eq(script.entities); | ||
| expect(cs.entities.length).eq(2); | ||
| expect(cs.entities[0]).not.eq(childA); | ||
| expect(cs.entities[1]).not.eq(childB); | ||
| expect(cs.entities[0]).eq(cloned.children[0]); | ||
| expect(cs.entities[1]).eq(cloned.children[1]); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("deep cloned entity array with external ref keeps original", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const external = rootEntity.createChild("external"); | ||
| const script = parent.addComponent(ArrayRefScript); | ||
| script.entities = [child, external]; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(ArrayRefScript); | ||
|
|
||
| expect(cs.entities[0]).eq(cloned.children[0]); | ||
| expect(cs.entities[1]).eq(external); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("deep cloned empty entity array stays empty", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const script = parent.addComponent(ArrayRefScript); | ||
| script.entities = []; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(ArrayRefScript); | ||
|
|
||
| expect(cs.entities).not.eq(script.entities); | ||
| expect(cs.entities.length).eq(0); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Component self and cross references", () => { | ||
| it("script referencing itself should remap to cloned script", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const script = parent.addComponent(SelfComponentRefScript); | ||
| script.selfScript = script; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(SelfComponentRefScript); | ||
|
|
||
| expect(cs.selfScript).not.eq(script); | ||
| expect(cs.selfScript).eq(cs); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script referencing another script on child entity should remap", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const childScript = child.addComponent(TestScript); | ||
| const script = parent.addComponent(CrossScriptRefScript); | ||
| script.otherScript = childScript; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(CrossScriptRefScript); | ||
| const clonedChildScript = cloned.children[0].getComponent(TestScript); | ||
|
|
||
| expect(cs.otherScript).not.eq(childScript); | ||
| expect(cs.otherScript).eq(clonedChildScript); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script referencing external script should keep original", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const external = rootEntity.createChild("external"); | ||
| const externalScript = external.addComponent(TestScript); | ||
| const script = parent.addComponent(CrossScriptRefScript); | ||
| script.otherScript = externalScript; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(CrossScriptRefScript); | ||
|
|
||
| expect(cs.otherScript).eq(externalScript); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Nested @deepClone object with entity refs", () => { | ||
| it("entity ref inside deep cloned plain object should remap", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = parent.addComponent(NestedObjectScript); | ||
| script.config = { target: child, label: "hello" }; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(NestedObjectScript); | ||
|
|
||
| expect(cs.config).not.eq(script.config); | ||
| expect(cs.config.label).eq("hello"); | ||
| expect(cs.config.target).not.eq(child); | ||
| expect(cs.config.target).eq(cloned.children[0]); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("entity ref inside deep cloned object pointing outside keeps original", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const external = rootEntity.createChild("external"); | ||
| const script = parent.addComponent(NestedObjectScript); | ||
| script.config = { target: external, label: "ext" }; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(NestedObjectScript); | ||
|
|
||
| expect(cs.config.target).eq(external); | ||
| expect(cs.config.label).eq("ext"); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Signal clone with structured bindings", () => { | ||
| it("@deepClone Signal should not copy closure listeners", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const script = parent.addComponent(SignalScript); | ||
| let called = false; | ||
| script.onFire.on(() => { called = true; }); | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(SignalScript); | ||
|
|
||
| expect(cs.onFire).not.eq(script.onFire); | ||
| cs.onFire.invoke(1); | ||
| expect(called).eq(false); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("@deepClone Signal should remap structured binding target to cloned hierarchy", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const handlerEntity = parent.createChild("handler"); | ||
| const handler = handlerEntity.addComponent(ClickHandler); | ||
| const script = parent.addComponent(SignalScript); | ||
| script.onFire.on(handler, "handleClick"); | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(SignalScript); | ||
| const clonedHandler = cloned.findByName("handler").getComponent(ClickHandler); | ||
|
|
||
| cs.onFire.invoke(1); | ||
| expect(clonedHandler.callCount).eq(1); | ||
| expect(handler.callCount).eq(0); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("@deepClone Signal should keep external structured binding target", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const external = rootEntity.createChild("external"); | ||
| const externalHandler = external.addComponent(ClickHandler); | ||
| const script = parent.addComponent(SignalScript); | ||
| script.onFire.on(externalHandler, "handleClick"); | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(SignalScript); | ||
|
|
||
| cs.onFire.invoke(1); | ||
| expect(externalHandler.callCount).eq(1); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("@deepClone Signal should remap structured binding with pre-resolved args", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const handlerEntity = parent.createChild("handler"); | ||
| const handler = handlerEntity.addComponent(ClickHandler); | ||
| const script = parent.addComponent(SignalScript); | ||
| script.onFire.on(handler, "handleClickWithPrefix", "myPrefix"); | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(SignalScript); | ||
| const clonedHandler = cloned.findByName("handler").getComponent(ClickHandler); | ||
|
|
||
| cs.onFire.invoke(1); | ||
| expect(clonedHandler.callCount).eq(1); | ||
| expect(clonedHandler.lastPrefix).eq("myPrefix"); | ||
| expect(handler.callCount).eq(0); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("@deepClone Signal should preserve once flag on structured binding", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const handlerEntity = parent.createChild("handler"); | ||
| const handler = handlerEntity.addComponent(ClickHandler); | ||
| const script = parent.addComponent(SignalScript); | ||
| script.onFire.once(handler, "handleClick"); | ||
|
|
||
| const cloned = parent.clone(); | ||
| const cs = cloned.getComponent(SignalScript); | ||
| const clonedHandler = cloned.findByName("handler").getComponent(ClickHandler); | ||
|
|
||
| cs.onFire.invoke(1); | ||
| expect(clonedHandler.callCount).eq(1); | ||
| cs.onFire.invoke(2); | ||
| expect(clonedHandler.callCount).eq(1); // once: removed after first call | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Clone hierarchy integrity", () => { | ||
| it("clone preserves children count and names", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| parent.createChild("a"); | ||
| parent.createChild("b"); | ||
| parent.createChild("c"); | ||
|
|
||
| const cloned = parent.clone(); | ||
| expect(cloned.children.length).eq(3); | ||
| expect(cloned.children[0].name).eq("a"); | ||
| expect(cloned.children[1].name).eq("b"); | ||
| expect(cloned.children[2].name).eq("c"); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("clone of deeply nested hierarchy preserves structure", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const a = rootEntity.createChild("a"); | ||
| const b = a.createChild("b"); | ||
| const c = b.createChild("c"); | ||
| const d = c.createChild("d"); | ||
|
|
||
| const cloned = a.clone(); | ||
| expect(cloned.children[0].name).eq("b"); | ||
| expect(cloned.children[0].children[0].name).eq("c"); | ||
| expect(cloned.children[0].children[0].children[0].name).eq("d"); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("script on child entity with ref to parent should remap", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const child = parent.createChild("child"); | ||
| const script = child.addComponent(TestScript); | ||
| script.targetEntity = parent; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const clonedChild = cloned.children[0]; | ||
| const cs = clonedChild.getComponent(TestScript); | ||
|
|
||
| expect(cs.targetEntity).not.eq(parent); | ||
| expect(cs.targetEntity).eq(cloned); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
|
|
||
| it("multiple scripts on different entities with cross-refs all remap correctly", () => { | ||
| const rootEntity = scene.createRootEntity("root"); | ||
| const parent = rootEntity.createChild("parent"); | ||
| const childA = parent.createChild("childA"); | ||
| const childB = parent.createChild("childB"); | ||
|
|
||
| const scriptA = childA.addComponent(TestScript); | ||
| scriptA.targetEntity = childB; | ||
|
|
||
| const scriptB = childB.addComponent(TestScript); | ||
| scriptB.targetEntity = childA; | ||
|
|
||
| const cloned = parent.clone(); | ||
| const clonedA = cloned.children[0]; | ||
| const clonedB = cloned.children[1]; | ||
| const csA = clonedA.getComponent(TestScript); | ||
| const csB = clonedB.getComponent(TestScript); | ||
|
|
||
| expect(csA.targetEntity).eq(clonedB); | ||
| expect(csB.targetEntity).eq(clonedA); | ||
|
|
||
| rootEntity.destroy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Cleanup is not failure-safe; leaked entities can cascade into later tests.
rootEntity.destroy() is only called at the happy path end of each test. If an assertion fails earlier, shared scene state leaks into following tests. Use afterEach (or try/finally) for guaranteed teardown.
Suggested hardening pattern
-import { describe, expect, it } from "vitest";
+import { afterEach, describe, expect, it } from "vitest";
+const rootsToDestroy: Entity[] = [];
+afterEach(() => {
+ for (const root of rootsToDestroy.splice(0)) root.destroy();
+});
-const rootEntity = scene.createRootEntity("root");
+const rootEntity = scene.createRootEntity("root");
+rootsToDestroy.push(rootEntity);🧰 Tools
🪛 ESLint
[error] 544-544: Replace ·called·=·true; with ⏎········called·=·true;⏎·····
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/core/CloneUtils.test.ts` around lines 116 - 705, The tests leak
scene state because each it() calls rootEntity.destroy() only on the happy path;
make teardown failure-safe by ensuring root entities are always destroyed:
either add an afterEach() that tracks and destroys any created root entities
(e.g., collect rootEntity instances created in each it into a local array) or
wrap each test body in a try/finally that calls rootEntity.destroy() in finally;
update references in the test file CloneUtils.test.ts (look for variables named
rootEntity created in multiple describe/it blocks and the existing
rootEntity.destroy() calls) to use the new tear-down pattern so leaked entities
cannot persist between tests.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/clone/CloneManager.ts (1)
108-114:⚠️ Potential issue | 🟠 Major
@ignoreCloneis bypassed by the early_remapfast path.
cloneMode === CloneMode.Ignoreneeds to run before_remap; otherwise ignoredEntity/Componentrefs still get remapped and assigned.Suggested fix
- // Remappable references (Entity/Component) are always remapped, regardless of clone decorator - if (sourceProperty instanceof Object && (<ICustomClone>sourceProperty)._remap) { - target[k] = (<ICustomClone>sourceProperty)._remap(srcRoot, targetRoot); - return; - } - - if (cloneMode === CloneMode.Ignore) return; + if (cloneMode === CloneMode.Ignore) return; + + if (sourceProperty instanceof Object && (<ICustomClone>sourceProperty)._remap) { + target[k] = (<ICustomClone>sourceProperty)._remap(srcRoot, targetRoot); + return; + }🤖 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 108 - 114, The early remap fast path in CloneManager is bypassing the `@ignoreClone` check: move the cloneMode === CloneMode.Ignore guard to run before the remap branch so ignored refs are not processed; specifically, in the cloning loop check if cloneMode === CloneMode.Ignore for the current property (the same condition that now appears after the remap branch) before evaluating sourceProperty instanceof Object and (<ICustomClone>sourceProperty)._remap, and if ignored skip assigning to target[k] so _remap is not called or applied.
🤖 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 in CloneManager (the `case Array:`
block) ignores the `deepInstanceMap`, causing duplicated shared arrays and
infinite recursion for self-referential arrays; fix it by checking
`deepInstanceMap` for an existing mapped target for the `sourceProperty` before
allocating or reusing `target[k]`, return or assign that mapped instance if
present, and when creating a new array instance assign it into `deepInstanceMap`
immediately (before iterating) so subsequent recursive clones reuse it; ensure
the rest of the loop continues to call `CloneManager.cloneProperty` as before
using the mapped/created `targetPropertyA`.
- Around line 124-161: The switch in CloneManager.cloneProperty (handling typed
arrays, Array, and default) declares lexical variables (targetPropertyT,
targetPropertyA, length, targetProperty) directly in case clauses which violates
noSwitchDeclarations; fix by wrapping each case branch that declares let/const
in its own block { ... } so targetPropertyT, targetPropertyA, length and
targetProperty are properly block-scoped, leaving existing logic
(slicing/setting for TypedArrays, resizing/iterating for Array, and the default
object handling) unchanged.
---
Duplicate comments:
In `@packages/core/src/clone/CloneManager.ts`:
- Around line 108-114: The early remap fast path in CloneManager is bypassing
the `@ignoreClone` check: move the cloneMode === CloneMode.Ignore guard to run
before the remap branch so ignored refs are not processed; specifically, in the
cloning loop check if cloneMode === CloneMode.Ignore for the current property
(the same condition that now appears after the remap branch) before evaluating
sourceProperty instanceof Object and (<ICustomClone>sourceProperty)._remap, and
if ignored skip assigning to target[k] so _remap is not called or applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 56072dd0-2e84-43b0-84bd-f855313b4b24
📒 Files selected for processing (1)
packages/core/src/clone/CloneManager.ts
| switch (type) { | ||
| case Uint8Array: | ||
| case Uint16Array: | ||
| case Uint32Array: | ||
| case Int8Array: | ||
| case Int16Array: | ||
| case Int32Array: | ||
| case Float32Array: | ||
| case Float64Array: | ||
| let targetPropertyT = <TypedArray>target[k]; | ||
| if (targetPropertyT == null || targetPropertyT.length !== (<TypedArray>sourceProperty).length) { | ||
| target[k] = (<TypedArray>sourceProperty).slice(); | ||
| } else { | ||
| targetPropertyT.set(<TypedArray>sourceProperty); | ||
| } | ||
| break; | ||
| case Array: | ||
| let targetPropertyA = <Array<any>>target[k]; | ||
| const length = (<Array<any>>sourceProperty).length; | ||
| if (targetPropertyA == null) { | ||
| target[k] = targetPropertyA = new Array<any>(length); | ||
| } else { | ||
| targetPropertyA.length = length; | ||
| } | ||
| for (let i = 0; i < length; i++) { | ||
| CloneManager.cloneProperty( | ||
| <Array<any>>sourceProperty, | ||
| targetPropertyA, | ||
| i, | ||
| cloneMode, | ||
| srcRoot, | ||
| targetRoot, | ||
| deepInstanceMap | ||
| ); | ||
| } | ||
| break; | ||
| default: | ||
| let targetProperty = <Object>target[k]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining switch clauses that declare let/const without a block.
rg -nP 'case\s+[^\n]+:\n\s+(let|const)\s' packages/core/src/clone/CloneManager.ts
# Expected after fix: no matchesRepository: galacean/engine
Length of output: 41
🏁 Script executed:
sed -n '124,161p' packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 1252
Wrap case clauses containing lexical declarations with block scope.
Multiple let and const declarations inside case clauses lack block scoping, violating ESLint's noSwitchDeclarations rule:
case Uint8Array:throughcase Float64Array:(line ~133:let targetPropertyT)case Array:(lines ~141–142:let targetPropertyA,const length)default:(line ~160:let targetProperty)
Each case containing let or const must be wrapped with {} to properly scope variables and avoid lint/CI failures.
🧰 Tools
🪛 Biome (2.4.7)
[error] 133-133: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 141-141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 142-142: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 161-161: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 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 124 - 161, The switch
in CloneManager.cloneProperty (handling typed arrays, Array, and default)
declares lexical variables (targetPropertyT, targetPropertyA, length,
targetProperty) directly in case clauses which violates noSwitchDeclarations;
fix by wrapping each case branch that declares let/const in its own block { ...
} so targetPropertyT, targetPropertyA, length and targetProperty are properly
block-scoped, leaving existing logic (slicing/setting for TypedArrays,
resizing/iterating for Array, and the default object handling) unchanged.
| case Array: | ||
| let targetPropertyA = <Array<any>>target[k]; | ||
| const length = (<Array<any>>sourceProperty).length; | ||
| if (targetPropertyA == null) { | ||
| target[k] = targetPropertyA = new Array<any>(length); | ||
| } else { | ||
| targetPropertyA.length = length; | ||
| } | ||
| for (let i = 0; i < length; i++) { | ||
| CloneManager.cloneProperty( | ||
| <Array<any>>sourceProperty, | ||
| targetPropertyA, | ||
| i, | ||
| cloneMode, | ||
| srcRoot, | ||
| targetRoot, | ||
| deepInstanceMap | ||
| ); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Array cloning skips deepInstanceMap, breaking shared/cyclic graph handling.
case Array always allocates/reuses target[k] directly, but never reads/writes deepInstanceMap. This can duplicate shared arrays and recurse indefinitely on self-referential arrays.
Suggested fix
case Array:
- let targetPropertyA = <Array<any>>target[k];
- const length = (<Array<any>>sourceProperty).length;
- if (targetPropertyA == null) {
- target[k] = targetPropertyA = new Array<any>(length);
- } else {
- targetPropertyA.length = length;
- }
+ const sourceArray = <Array<any>>sourceProperty;
+ let targetPropertyA = <Array<any>>target[k];
+ const length = sourceArray.length;
+ if (targetPropertyA == null) {
+ targetPropertyA = <Array<any>>deepInstanceMap.get(sourceArray);
+ if (!targetPropertyA) {
+ targetPropertyA = new Array<any>(length);
+ deepInstanceMap.set(sourceArray, targetPropertyA);
+ }
+ target[k] = targetPropertyA;
+ } else {
+ deepInstanceMap.set(sourceArray, targetPropertyA);
+ }
+ targetPropertyA.length = length;
for (let i = 0; i < length; i++) {
CloneManager.cloneProperty(
- <Array<any>>sourceProperty,
+ sourceArray,
targetPropertyA,
i,
cloneMode,📝 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.
| case Array: | |
| let targetPropertyA = <Array<any>>target[k]; | |
| const length = (<Array<any>>sourceProperty).length; | |
| if (targetPropertyA == null) { | |
| target[k] = targetPropertyA = new Array<any>(length); | |
| } else { | |
| targetPropertyA.length = length; | |
| } | |
| for (let i = 0; i < length; i++) { | |
| CloneManager.cloneProperty( | |
| <Array<any>>sourceProperty, | |
| targetPropertyA, | |
| i, | |
| cloneMode, | |
| srcRoot, | |
| targetRoot, | |
| deepInstanceMap | |
| ); | |
| } | |
| break; | |
| case Array: | |
| const sourceArray = <Array<any>>sourceProperty; | |
| let targetPropertyA = <Array<any>>target[k]; | |
| const length = sourceArray.length; | |
| if (targetPropertyA == null) { | |
| targetPropertyA = <Array<any>>deepInstanceMap.get(sourceArray); | |
| if (!targetPropertyA) { | |
| targetPropertyA = new Array<any>(length); | |
| deepInstanceMap.set(sourceArray, targetPropertyA); | |
| } | |
| target[k] = targetPropertyA; | |
| } else { | |
| deepInstanceMap.set(sourceArray, targetPropertyA); | |
| } | |
| targetPropertyA.length = length; | |
| for (let i = 0; i < length; i++) { | |
| CloneManager.cloneProperty( | |
| sourceArray, | |
| targetPropertyA, | |
| i, | |
| cloneMode, | |
| srcRoot, | |
| targetRoot, | |
| deepInstanceMap | |
| ); | |
| } | |
| break; |
🧰 Tools
🪛 Biome (2.4.7)
[error] 141-141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 142-142: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 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 140 - 159, The Array
branch in CloneManager (the `case Array:` block) ignores the `deepInstanceMap`,
causing duplicated shared arrays and infinite recursion for self-referential
arrays; fix it by checking `deepInstanceMap` for an existing mapped target for
the `sourceProperty` before allocating or reusing `target[k]`, return or assign
that mapped instance if present, and when creating a new array instance assign
it into `deepInstanceMap` immediately (before iterating) so subsequent recursive
clones reuse it; ensure the rest of the loop continues to call
`CloneManager.cloneProperty` as before using the mapped/created
`targetPropertyA`.
- Remove ineffective @ignoreClone on Entity/Component refs - Fix remapComponent to use index instead of constructor Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/mesh/Skin.ts (1)
18-28:⚠️ Potential issue | 🔴 Critical
_skinMatriceswill beundefinedon clonedSkinobjects with non-empty bones.When
@deepCloneprocesses the_bonesarray, it directly modifiestarget._boneswithout invoking thebonessetter. Since the setter at line 62 is the only place_skinMatricesis initialized (this._skinMatrices = new Float32Array(boneCount * 16)), and_skinMatriceshas@ignoreClone, cloned skins will haveundefinedfor_skinMatricesdespite having a populated_bonesarray.This causes runtime crashes in
_updateSkinMatrices()at lines 85, 87, 89 when accessing the undefined array, and inSkinnedMeshRenderer._update()at lines 198 and 204 when passingskin._skinMatricesto GPU operations.Proposed fix: Change `_skinMatrices` to `@deepClone`
Since
_skinMatricesis a calculation cache and its size must matchbones.length * 16, deep cloning ensures the target has a correctly-sized buffer:/** `@internal` */ - `@ignoreClone` + `@deepClone` _skinMatrices: Float32Array;Alternative: Add a
_cloneTomethod that initializes_skinMatricesbased on_bones.lengthafter cloning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/mesh/Skin.ts` around lines 18 - 28, The cloned Skin instances end up with _skinMatrices undefined because `@deepClone` copies _bones directly without invoking the bones setter which allocates _skinMatrices; fix by making _skinMatrices deep-cloned (change its decorator from `@ignoreClone` to `@deepClone`) so the cloned buffer matches bones.length*16, or alternatively implement a _cloneTo(target: Skin) method invoked during cloning that initializes target._skinMatrices = new Float32Array(target._bones.length * 16) (ensure this runs before any calls to _updateSkinMatrices or SkinnedMeshRenderer._update which access skin._skinMatrices).packages/core/src/Renderer.ts (1)
352-357:⚠️ Potential issue | 🟠 MajorAdd transform entity listener rebinding in
_cloneToto restore listener registration after clone.During component cloning, generic field cloning happens before
_cloneToand assigns_transformEntityviaEntity._remap()without triggering_setTransformEntity(). This bypasses the listener registration in_updateFlagManager, leaving the cloned renderer unable to respond to transform changes.Suggested fix location
_cloneTo(target: Renderer): void { const materials = this._materials; for (let i = 0, n = materials.length; i < n; i++) { target._setMaterial(i, materials[i]); } // Rebind transform entity listeners after field cloning target._setTransformEntity(target._transformEntity); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Renderer.ts` around lines 352 - 357, _cloneTo currently copies materials via _setMaterial but doesn't rebind transform listeners after generic field cloning, so the cloned Renderer won't receive transform updates; after the material loop in _cloneTo, call target._setTransformEntity(target._transformEntity) to re-register listeners created in _updateFlagManager (the _transformEntity was set via Entity._remap previously), ensuring transform change handlers are restored on the clone.
🤖 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/CloneUtils.ts`:
- Around line 17-23: In CloneUtils.remapComponent, avoid blindly indexing into
the target entity's _components array; compute const idx =
srcEntity._components.indexOf(component), call
CloneUtils._getEntityByHierarchyPath(targetRoot, path) to get the target entity,
then check if idx === -1 or targetEntity is falsy or targetEntity._components is
falsy or idx is out of bounds; if any of those are true return the original
component (or a safe fallback) instead of indexing, otherwise return
targetEntity._components[idx] as T. This uses existing helpers
_getEntityHierarchyPath, _getEntityByHierarchyPath, _tempRemapPath and the
_components arrays to locate and safely guard the mapped component.
---
Outside diff comments:
In `@packages/core/src/mesh/Skin.ts`:
- Around line 18-28: The cloned Skin instances end up with _skinMatrices
undefined because `@deepClone` copies _bones directly without invoking the bones
setter which allocates _skinMatrices; fix by making _skinMatrices deep-cloned
(change its decorator from `@ignoreClone` to `@deepClone`) so the cloned buffer
matches bones.length*16, or alternatively implement a _cloneTo(target: Skin)
method invoked during cloning that initializes target._skinMatrices = new
Float32Array(target._bones.length * 16) (ensure this runs before any calls to
_updateSkinMatrices or SkinnedMeshRenderer._update which access
skin._skinMatrices).
In `@packages/core/src/Renderer.ts`:
- Around line 352-357: _cloneTo currently copies materials via _setMaterial but
doesn't rebind transform listeners after generic field cloning, so the cloned
Renderer won't receive transform updates; after the material loop in _cloneTo,
call target._setTransformEntity(target._transformEntity) to re-register
listeners created in _updateFlagManager (the _transformEntity was set via
Entity._remap previously), ensuring transform change handlers are restored on
the clone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e433b54-7958-41f8-944d-85496867e67b
📒 Files selected for processing (11)
packages/core/src/Component.tspackages/core/src/Renderer.tspackages/core/src/Transform.tspackages/core/src/clone/CloneUtils.tspackages/core/src/mesh/Skin.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/physics/shape/ColliderShape.tspackages/ui/src/component/UICanvas.tspackages/ui/src/component/UIGroup.tspackages/ui/src/component/UIRenderer.tspackages/ui/src/component/interactive/UIInteractive.ts
💤 Files with no reviewable changes (4)
- packages/ui/src/component/UIGroup.ts
- packages/core/src/physics/shape/ColliderShape.ts
- packages/core/src/particle/ParticleGenerator.ts
- packages/ui/src/component/UIRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/component/UICanvas.ts
| static remapComponent<T extends Component>(srcRoot: Entity, targetRoot: Entity, component: T): T { | ||
| const paths = CloneUtils._tempRemapPath; | ||
| const success = CloneUtils._getEntityHierarchyPath(srcRoot, component.entity, paths); | ||
| return success | ||
| ? (CloneUtils._getEntityByHierarchyPath(targetRoot, paths)?.getComponent( | ||
| <ComponentConstructor<T>>component.constructor | ||
| ) as T) | ||
| : component; | ||
| const path = CloneUtils._tempRemapPath; | ||
| const srcEntity = component.entity; | ||
| if (!CloneUtils._getEntityHierarchyPath(srcRoot, srcEntity, path)) return component; | ||
| return CloneUtils._getEntityByHierarchyPath(targetRoot, path)._components[ | ||
| srcEntity._components.indexOf(component) | ||
| ] as T; |
There was a problem hiding this comment.
Guard missing component mapping before array index access.
Line 22 can be -1 (or map to an out-of-range slot), which makes Line 21 return undefined cast to T. That can break downstream clone/remap logic at runtime.
💡 Suggested fix
static remapComponent<T extends Component>(srcRoot: Entity, targetRoot: Entity, component: T): T {
const path = CloneUtils._tempRemapPath;
const srcEntity = component.entity;
if (!CloneUtils._getEntityHierarchyPath(srcRoot, srcEntity, path)) return component;
- return CloneUtils._getEntityByHierarchyPath(targetRoot, path)._components[
- srcEntity._components.indexOf(component)
- ] as T;
+ const componentIndex = srcEntity._components.indexOf(component);
+ if (componentIndex < 0) return component;
+
+ const targetEntity = CloneUtils._getEntityByHierarchyPath(targetRoot, path);
+ return (targetEntity._components[componentIndex] ?? component) as T;
}📝 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.
| static remapComponent<T extends Component>(srcRoot: Entity, targetRoot: Entity, component: T): T { | |
| const paths = CloneUtils._tempRemapPath; | |
| const success = CloneUtils._getEntityHierarchyPath(srcRoot, component.entity, paths); | |
| return success | |
| ? (CloneUtils._getEntityByHierarchyPath(targetRoot, paths)?.getComponent( | |
| <ComponentConstructor<T>>component.constructor | |
| ) as T) | |
| : component; | |
| const path = CloneUtils._tempRemapPath; | |
| const srcEntity = component.entity; | |
| if (!CloneUtils._getEntityHierarchyPath(srcRoot, srcEntity, path)) return component; | |
| return CloneUtils._getEntityByHierarchyPath(targetRoot, path)._components[ | |
| srcEntity._components.indexOf(component) | |
| ] as T; | |
| static remapComponent<T extends Component>(srcRoot: Entity, targetRoot: Entity, component: T): T { | |
| const path = CloneUtils._tempRemapPath; | |
| const srcEntity = component.entity; | |
| if (!CloneUtils._getEntityHierarchyPath(srcRoot, srcEntity, path)) return component; | |
| const componentIndex = srcEntity._components.indexOf(component); | |
| if (componentIndex < 0) return component; | |
| const targetEntity = CloneUtils._getEntityByHierarchyPath(targetRoot, path); | |
| return (targetEntity._components[componentIndex] ?? component) as T; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/clone/CloneUtils.ts` around lines 17 - 23, In
CloneUtils.remapComponent, avoid blindly indexing into the target entity's
_components array; compute const idx = srcEntity._components.indexOf(component),
call CloneUtils._getEntityByHierarchyPath(targetRoot, path) to get the target
entity, then check if idx === -1 or targetEntity is falsy or
targetEntity._components is falsy or idx is out of bounds; if any of those are
true return the original component (or a safe fallback) instead of indexing,
otherwise return targetEntity._components[idx] as T. This uses existing helpers
_getEntityHierarchyPath, _getEntityByHierarchyPath, _tempRemapPath and the
_components arrays to locate and safely guard the mapped component.
* fix: compontont props clone
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
Refactor
Behavioral Changes
Tests