Skip to content

fix(entity): clearChildren bug + setSiblingIndex without parent #2991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cptbtptpbcptdtptp wants to merge 2 commits into
base: dev/2.0
Choose a base branch
from
+32 −6
Open

fix(entity): clearChildren bug + setSiblingIndex without parent #2991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
045d619
fix: clear children bug
cptbtptpbcptdtptp Apr 22, 2026
44e7257
fix: entity set sibling error when without parent
cptbtptpbcptdtptp Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions packages/core/src/Entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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`);
}
}
Comment on lines 214 to 224
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical: invalid if/else/else chain — blocks all CI builds.

The block goes if (this._isRoot) { … } else { … } else { … }, which is not valid TypeScript and breaks every CI job (rollup/swc, ESLint, Biome) at line 221. Per the PR description, the second branch should be else if (this._parent) so orphan entities fall through to the Logger.warn path.

🛠️ Proposed fix 
   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);
     } else {
       Logger.warn(`The entity ${this.name} is not in the hierarchy`);
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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`);
}
}
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
} else if (this._parent) {
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`);
}
}
🧰 Tools
🪛 Biome (2.4.14)

[error] 221-221: Expected a statement but instead found 'else'.

(parse)

🪛 GitHub Actions: CI / 0_e2e (22.x, 2_4).txt

[error] 221-221: Rollup build failed via (plugin swc) with syntax error: "Expression expected" at line 221 (} else {).

🪛 GitHub Actions: CI / 1_build (22.x, ubuntu-latest).txt

[error] 221-221: Build failed during Rollup (plugin swc) with syntax error: "Expression expected" at "} else {".

🪛 GitHub Actions: CI / 2_e2e (22.x, 3_4).txt

[error] 221-223: Build failed in rollup (plugin swc): Syntax Error: "Expression expected" at the closing brace before else.

🪛 GitHub Actions: CI / 3_e2e (22.x, 1_4).txt

[error] 221-221: Build failed in rollup (plugin swc): Syntax Error: 'Expression expected' near '}' / start of 'else' block. See swc error at /Users/runner/work/engine/engine/packages/core/src/Entity.ts:221:1.

🪛 GitHub Actions: CI / 5_lint.txt

[error] 221-221: ESLint parsing error: Declaration or statement expected (Parsing error: Declaration or statement expected).

🪛 GitHub Actions: CI / 6_codecov.txt

[error] 221-221: Build failed during Rollup (plugin swc). Syntax error: Expression expected at line 221 near 'else'.

🪛 GitHub Actions: CI / 7_build (22.x, macos-latest).txt

[error] 221-221: rollup (plugin swc) build failed with Syntax Error: "Expression expected" at } else {

🪛 GitHub Actions: CI / 8_e2e (22.x, 4_4).txt

[error] 221-221: Build failed in rollup (plugin swc): Syntax Error — "Expression expected" at line 221 near "} else {".

🪛 GitHub Actions: CI / build (22.x, macos-latest)

[error] 221-221: Build failed in rollup (plugin swc): Syntax Error: "Expression expected" at line 221 near "} else {".

🪛 GitHub Actions: CI / build (22.x, ubuntu-latest)

[error] 221-221: Rollup (plugin swc) failed with syntax error: "Expression expected" at Entity.ts:221:1 near } else {.

🪛 GitHub Actions: CI / codecov

[error] 221-221: Build failed during rollup compilation with (plugin swc) syntax error: "Expression expected" at the } else { line. Error reported as: "(plugin swc) Error: Expression expected".

🪛 GitHub Actions: CI / e2e (22.x, 1_4)

[error] 221-224: Build failed during Rollup (plugin swc): "Expression expected" Syntax Error at "/Users/runner/work/engine/engine/packages/core/src/Entity.ts:221:1" near "+ } else {"

🪛 GitHub Actions: CI / e2e (22.x, 2_4)

[error] 221-221: Build failed during rollup with plugin swc: Syntax Error — "Expression expected" at line 221 near "} else {".

🪛 GitHub Actions: CI / e2e (22.x, 3_4)

[error] 221-221: Build failed in rollup (plugin swc): 'Expression expected' syntax error at Entity.ts line 221 near '} else {'.

🪛 GitHub Actions: CI / e2e (22.x, 4_4)

[error] 221-223: Build failed during Rollup (plugin swc). swc syntax error in /Users/runner/work/engine/engine/packages/core/src/Entity.ts:221:1: "Expression expected" near "} else {".

🪛 GitHub Actions: CI / lint

[error] 221-221: ESLint parsing error: Declaration or statement expected

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/Entity.ts` around lines 214 - 224, The setter siblingIndex
has an invalid if/else/else chain; update the second branch to be an else if
that checks for this._parent so orphans fall through to the Logger.warn path:
keep the this._isRoot branch that calls
this._setSiblingIndex(this._scene._rootEntities, value), change the middle
branch to else if (this._parent) which calls
this._setSiblingIndex(parent._children, value) and
parent._dispatchModify(EntityModifyFlags.Child, parent), and let the final else
call Logger.warn(`The entity ${this.name} is not in the hierarchy`).


Expand Down Expand Up @@ -403,13 +401,20 @@ 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);
child._isActiveInScene && (activeChangeFlag |= ActiveChangeFlag.Scene);
activeChangeFlag && child._processInActive(activeChangeFlag);

Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive().

child._setParentChange();
}
children.length = 0;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/SceneManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export class SceneManager {
const scenePromise = this.engine.resourceManager.load<Scene>({ 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();
}
Expand Down
21 changes: 21 additions & 0 deletions tests/src/core/Entity.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +332 to +353
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | 💤 Low value

Heads up: parentModifyCount[EntityModifyFlags.Child]).eq(1) only holds if Entity.clearChildren() dispatches once for the whole operation.

The current implementation in packages/core/src/Entity.ts dispatches the Child event inside the per-child loop, so this assertion will see 2. See the corresponding comment on Entity.clearChildren(); aligning the two is required before this test will pass.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/Entity.test.ts` around lines 332 - 353, Test expects
Entity.clearChildren() to dispatch a single EntityModifyFlags.Child event for
the whole operation, but current implementation emits Child inside the per-child
loop; update the Entity.clearChildren() method to accumulate removals and call
the modify/dispatch for EntityModifyFlags.Child exactly once after all children
are detached (while still emitting the Parent modify event per detached child
and resetting siblingIndex to -1 for each child); locate the dispatch call(s) in
Entity.clearChildren() and move/condense them so only one Child event is emitted
for the overall clear.

});
it("sibling index", () => {
const root = scene.createRootEntity();
Expand Down
Loading