From 045d6193bb41b1b1e913392deaf1cadc4fcaae3a Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Wed, 22 Apr 2026 19:24:47 +0800 Subject: [PATCH 1/5] fix: clear children bug --- packages/core/src/Entity.ts | 7 +++++++ tests/src/core/Entity.test.ts | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/packages/core/src/Entity.ts b/packages/core/src/Entity.ts index 9470d05817..4ee6796939 100644 --- a/packages/core/src/Entity.ts +++ b/packages/core/src/Entity.ts @@ -403,6 +403,11 @@ export class Entity extends EngineObject { for (let i = children.length - 1; i >= 0; i--) { const child = children[i]; child._parent = null; + child._siblingIndex = -1; + // Dispatch `Child` to the old parent before `_processInActive` (which unregisters + // UI listeners via `cleanRootCanvas`), so subscribers such as UICanvas can react + // to the hierarchy change while still attached. + this._dispatchModify(EntityModifyFlags.Child, this); let activeChangeFlag = ActiveChangeFlag.None; child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy); @@ -410,6 +415,8 @@ export class Entity extends EngineObject { activeChangeFlag && child._processInActive(activeChangeFlag); Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive(). + + child._setParentChange(); } children.length = 0; } diff --git a/tests/src/core/Entity.test.ts b/tests/src/core/Entity.test.ts index 0a428f4b60..edfed9fe4b 100644 --- a/tests/src/core/Entity.test.ts +++ b/tests/src/core/Entity.test.ts @@ -328,8 +328,29 @@ describe("Entity", async () => { child.parent = parent; const child2 = new Entity(engine, "child2"); child2.parent = parent; + + const parentModifyCount = [0, 0, 0]; + const childModifyCount = [0, 0, 0]; + const child2ModifyCount = [0, 0, 0]; + // @ts-ignore + parent._registerModifyListener((flag: EntityModifyFlags) => ++parentModifyCount[flag]); + // @ts-ignore + child._registerModifyListener((flag: EntityModifyFlags) => ++childModifyCount[flag]); + // @ts-ignore + child2._registerModifyListener((flag: EntityModifyFlags) => ++child2ModifyCount[flag]); + parent.clearChildren(); expect(parent.children.length).eq(0); + + // Parent should receive a single `Child` modify event for the whole clear so + // listeners (e.g. UICanvas) can invalidate their cached state. + expect(parentModifyCount[EntityModifyFlags.Child]).eq(1); + // Each detached child should receive a `Parent` modify event. + expect(childModifyCount[EntityModifyFlags.Parent]).eq(1); + expect(child2ModifyCount[EntityModifyFlags.Parent]).eq(1); + // Sibling index must be reset so the entity is treated as lonely afterwards. + expect(child.siblingIndex).eq(-1); + expect(child2.siblingIndex).eq(-1); }); it("sibling index", () => { const root = scene.createRootEntity(); From 44e725781826253c6ad8e99d1d4a9f8e89cee148 Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Mon, 20 Apr 2026 15:58:33 +0800 Subject: [PATCH 2/5] fix: entity set sibling error when without parent --- packages/core/src/Entity.ts | 8 +++----- packages/core/src/SceneManager.ts | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/core/src/Entity.ts b/packages/core/src/Entity.ts index 4ee6796939..2419f4c401 100644 --- a/packages/core/src/Entity.ts +++ b/packages/core/src/Entity.ts @@ -9,7 +9,7 @@ import { Script } from "./Script"; import { Transform } from "./Transform"; import { UpdateFlagManager } from "./UpdateFlagManager"; import { ReferResource } from "./asset/ReferResource"; -import { EngineObject } from "./base"; +import { EngineObject, Logger } from "./base"; import { CloneUtils } from "./clone/CloneUtils"; import { ComponentCloner } from "./clone/ComponentCloner"; import { ActiveChangeFlag } from "./enums/ActiveChangeFlag"; @@ -212,16 +212,14 @@ export class Entity extends EngineObject { } set siblingIndex(value: number) { - if (this._siblingIndex === -1) { - throw `The entity ${this.name} is not in the hierarchy`; - } - if (this._isRoot) { this._setSiblingIndex(this._scene._rootEntities, value); } else { const parent = this._parent; this._setSiblingIndex(parent._children, value); parent._dispatchModify(EntityModifyFlags.Child, parent); + } else { + Logger.warn(`The entity ${this.name} is not in the hierarchy`); } } diff --git a/packages/core/src/SceneManager.ts b/packages/core/src/SceneManager.ts index 92de60d66b..984850fed4 100644 --- a/packages/core/src/SceneManager.ts +++ b/packages/core/src/SceneManager.ts @@ -94,7 +94,7 @@ export class SceneManager { const scenePromise = this.engine.resourceManager.load({ url, type: AssetType.Scene }); scenePromise.then((scene: Scene) => { if (destroyOldScene) { - const scenes = this._scenes.getArray(); + const scenes = this._scenes.getLoopArray(); for (let i = 0, n = scenes.length; i < n; i++) { scenes[i].destroy(); } From c0ae2b01b6e2533ee6c839732ff6857dcf89884a Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Tue, 30 Jun 2026 15:53:18 +0800 Subject: [PATCH 3/5] fix(entity): valid else-if guard for siblingIndex & single Child dispatch in clearChildren The siblingIndex setter used an invalid `else ... else` chain that broke compilation; restore the `else if (this._parent)` guard so orphan entities warn instead of crashing. clearChildren now dispatches a single `Child` modify event after all children are detached instead of once per child (idempotent for UICanvas, matches the test). Update the stale sibling-index test to expect a warning rather than a throw. Co-Authored-By: Claude Opus 4.8 --- packages/core/src/Entity.ts | 9 ++++----- tests/src/core/Entity.test.ts | 7 ++++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/core/src/Entity.ts b/packages/core/src/Entity.ts index 2419f4c401..a55158bd35 100644 --- a/packages/core/src/Entity.ts +++ b/packages/core/src/Entity.ts @@ -214,7 +214,7 @@ export class Entity extends EngineObject { set siblingIndex(value: number) { if (this._isRoot) { this._setSiblingIndex(this._scene._rootEntities, value); - } else { + } else if (this._parent) { const parent = this._parent; this._setSiblingIndex(parent._children, value); parent._dispatchModify(EntityModifyFlags.Child, parent); @@ -402,10 +402,6 @@ export class Entity extends EngineObject { const child = children[i]; child._parent = null; child._siblingIndex = -1; - // Dispatch `Child` to the old parent before `_processInActive` (which unregisters - // UI listeners via `cleanRootCanvas`), so subscribers such as UICanvas can react - // to the hierarchy change while still attached. - this._dispatchModify(EntityModifyFlags.Child, this); let activeChangeFlag = ActiveChangeFlag.None; child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy); @@ -417,6 +413,9 @@ export class Entity extends EngineObject { child._setParentChange(); } children.length = 0; + // Dispatch a single `Child` modify event for the whole clear so subscribers + // (e.g. UICanvas) can invalidate their cached hierarchy state once. + this._dispatchModify(EntityModifyFlags.Child, this); } /** diff --git a/tests/src/core/Entity.test.ts b/tests/src/core/Entity.test.ts index edfed9fe4b..ad9cdac5eb 100644 --- a/tests/src/core/Entity.test.ts +++ b/tests/src/core/Entity.test.ts @@ -416,12 +416,13 @@ describe("Entity", async () => { }; expect(siblingIndexBadFn).to.throw(); - // thorw error when set lonely entity + // setting sibling index on a lonely entity (no parent, not in scene root) warns instead of throwing const entityX = new Entity(engine, "entityX"); - var lonelyBadFn = function () { + var lonelyFn = function () { entityX.siblingIndex = 1; }; - expect(lonelyBadFn).to.throw(); + expect(lonelyFn).not.to.throw(); + expect(entityX.siblingIndex).eq(-1); }); it("isRoot", () => { From 8923242de9b17124fafffa9193a6a1e38c6484bb Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Tue, 30 Jun 2026 16:32:29 +0800 Subject: [PATCH 4/5] test(scene): cover loadScene destroyOldScene safe iteration Load a new scene while several old scenes are present and assert all previous scenes are destroyed. Without the getLoopArray fix, destroying a scene shrinks `_scenes` mid-loop, so live-array iteration skips scenes and reads past the end (verified: this test fails on getArray, passes on getLoopArray). Co-Authored-By: Claude Opus 4.8 --- tests/src/core/Scene.test.ts | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/src/core/Scene.test.ts b/tests/src/core/Scene.test.ts index c932fbd9db..22c3dbbf55 100644 --- a/tests/src/core/Scene.test.ts +++ b/tests/src/core/Scene.test.ts @@ -1,6 +1,6 @@ import { BackgroundMode, Engine, Entity, Scene, TextureFormat, Texture2D } from "@galacean/engine-core"; import { WebGLEngine } from "@galacean/engine"; -import { describe, beforeAll, beforeEach, expect, it } from "vitest"; +import { describe, beforeAll, beforeEach, expect, it, vi } from "vitest"; describe("Scene", () => { let engine: Engine; @@ -202,4 +202,37 @@ describe("Scene", () => { expect(scene.rootEntitiesCount).eq(0); }); }); + + describe("loadScene", () => { + it("destroyOldScene destroys every previous scene during safe iteration", async () => { + // Dedicated engine so destroying old scenes does not disturb the shared one. + const localEngine = await WebGLEngine.create({ canvas: document.createElement("canvas") }); + const sceneManager = localEngine.sceneManager; + + // Engine starts with one default scene; add two more so multiple scenes exist. + const old0 = sceneManager.scenes[0]; + const old1 = new Scene(localEngine, "old1"); + const old2 = new Scene(localEngine, "old2"); + sceneManager.addScene(old1); + sceneManager.addScene(old2); + expect(sceneManager.scenes.length).eq(3); + + // Resolve the load with a fresh scene without hitting real resources. + const newScene = new Scene(localEngine, "newScene"); + vi.spyOn(localEngine.resourceManager, "load").mockReturnValue(Promise.resolve(newScene) as any); + + await sceneManager.loadScene("mock://scene", true); + + // Destroying a scene removes it from `_scenes` mid-loop; iterating the live array + // (`getArray`) would skip scenes, so every previous scene must still be destroyed. + expect(old0.destroyed).eq(true); + expect(old1.destroyed).eq(true); + expect(old2.destroyed).eq(true); + // Only the newly loaded scene remains. + expect(sceneManager.scenes.length).eq(1); + expect(sceneManager.scenes[0]).eq(newScene); + + localEngine.destroy(); + }); + }); }); From 80e07bdfebda9db1ea7d42b4c4a4da6fc330e255 Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Sun, 12 Jul 2026 23:54:11 +0800 Subject: [PATCH 5/5] fix(entity): dispatch clearChildren Child event before detaching children `_processInActive` unregisters the removed subtrees' modify listeners (e.g. UIRenderer's root-canvas listener via cleanRootCanvas), so the post-loop dispatch never reached them when the cleared parent had no UI component of its own, leaving UICanvas's ordered renderer cache stale -- detached renderers kept rendering and stayed raycastable. Move the single dispatch before the loop (mirroring `_removeFromParent`, which dispatches before `_processInActive`) and skip it entirely when there are no children. Document the siblingIndex no-op semantics for out-of-hierarchy entities. Add a UICanvas regression test covering renderer-cache invalidation and tighten the clearChildren test to pin the dispatch-before-detach contract and the empty-clear early return. Co-Authored-By: Claude Fable 5 --- packages/core/src/Entity.ts | 12 +++++++++--- tests/src/core/Entity.test.ts | 13 ++++++++++++- tests/src/ui/UICanvas.test.ts | 24 +++++++++++++++++++++++- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/core/src/Entity.ts b/packages/core/src/Entity.ts index a55158bd35..03ed6dbc1f 100644 --- a/packages/core/src/Entity.ts +++ b/packages/core/src/Entity.ts @@ -206,6 +206,8 @@ export class Entity extends EngineObject { /** * The sibling index. + * @remarks Assigning it on an entity that is not in the hierarchy (no parent and not a scene root) + * is a no-op and logs a warning. */ get siblingIndex(): number { return this._siblingIndex; @@ -398,6 +400,13 @@ export class Entity extends EngineObject { */ clearChildren(): void { const children = this._children; + if (children.length === 0) { + return; + } + // Dispatch a single `Child` modify event for the whole clear before detaching: + // `_processInActive` unregisters the removed subtrees' listeners (e.g. UIRenderer's + // root-canvas listener), so a later dispatch could no longer reach them. + this._dispatchModify(EntityModifyFlags.Child, this); for (let i = children.length - 1; i >= 0; i--) { const child = children[i]; child._parent = null; @@ -413,9 +422,6 @@ export class Entity extends EngineObject { child._setParentChange(); } children.length = 0; - // Dispatch a single `Child` modify event for the whole clear so subscribers - // (e.g. UICanvas) can invalidate their cached hierarchy state once. - this._dispatchModify(EntityModifyFlags.Child, this); } /** diff --git a/tests/src/core/Entity.test.ts b/tests/src/core/Entity.test.ts index ad9cdac5eb..dffc4639d5 100644 --- a/tests/src/core/Entity.test.ts +++ b/tests/src/core/Entity.test.ts @@ -332,8 +332,12 @@ describe("Entity", async () => { const parentModifyCount = [0, 0, 0]; const childModifyCount = [0, 0, 0]; const child2ModifyCount = [0, 0, 0]; + let childCountAtDispatch = -1; // @ts-ignore - parent._registerModifyListener((flag: EntityModifyFlags) => ++parentModifyCount[flag]); + parent._registerModifyListener((flag: EntityModifyFlags) => { + ++parentModifyCount[flag]; + flag === EntityModifyFlags.Child && (childCountAtDispatch = parent.children.length); + }); // @ts-ignore child._registerModifyListener((flag: EntityModifyFlags) => ++childModifyCount[flag]); // @ts-ignore @@ -345,12 +349,19 @@ describe("Entity", async () => { // Parent should receive a single `Child` modify event for the whole clear so // listeners (e.g. UICanvas) can invalidate their cached state. expect(parentModifyCount[EntityModifyFlags.Child]).eq(1); + // The event must fire before the children are detached, while listeners + // registered from the removed subtrees are still able to receive it. + expect(childCountAtDispatch).eq(2); // Each detached child should receive a `Parent` modify event. expect(childModifyCount[EntityModifyFlags.Parent]).eq(1); expect(child2ModifyCount[EntityModifyFlags.Parent]).eq(1); // Sibling index must be reset so the entity is treated as lonely afterwards. expect(child.siblingIndex).eq(-1); expect(child2.siblingIndex).eq(-1); + + // Clearing an entity that has no children should not dispatch any event. + parent.clearChildren(); + expect(parentModifyCount[EntityModifyFlags.Child]).eq(1); }); it("sibling index", () => { const root = scene.createRootEntity(); diff --git a/tests/src/ui/UICanvas.test.ts b/tests/src/ui/UICanvas.test.ts index d9eca198d6..53aa9ccc59 100644 --- a/tests/src/ui/UICanvas.test.ts +++ b/tests/src/ui/UICanvas.test.ts @@ -1,7 +1,7 @@ import { Camera } from "@galacean/engine-core"; import { Vector2 } from "@galacean/engine-math"; import { WebGLEngine } from "@galacean/engine"; -import { CanvasRenderMode, ResolutionAdaptationMode, UICanvas, UITransform } from "@galacean/engine-ui"; +import { CanvasRenderMode, Image, ResolutionAdaptationMode, UICanvas, UITransform } from "@galacean/engine-ui"; import { describe, expect, it } from "vitest"; describe("UICanvas", async () => { @@ -352,4 +352,26 @@ describe("UICanvas", async () => { // @ts-ignore expect(rootCanvas._canDispatchEvent(camera2)).to.be.false; }); + + it("clearChildren on a plain container invalidates the ordered renderer cache", () => { + // The container itself has no UI component, so after its children are detached + // the only chance to reach the root canvas is the `Child` modify event of the clear. + const container = canvasEntity.createChild("container"); + const image1 = container.createChild("image1").addComponent(Image); + const image2 = container.createChild("image2").addComponent(Image); + + // @ts-ignore + let renderers = rootCanvas._getRenderers(); + expect(renderers).to.include(image1); + expect(renderers).to.include(image2); + + container.clearChildren(); + + // @ts-ignore + renderers = rootCanvas._getRenderers(); + expect(renderers).to.not.include(image1); + expect(renderers).to.not.include(image2); + + container.destroy(); + }); });