fix(entity): clearChildren bug + setSiblingIndex without parent#2991
fix(entity): clearChildren bug + setSiblingIndex without parent#2991cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
WalkthroughEntity hierarchy operations now emit modify events in a deterministic sequence. ChangesEntity Hierarchy Modification Behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
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/Entity.ts (1)
399-420:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
EntityModifyFlags.Childdispatched per child contradicts the new test (and the documented "single event for the whole clear" intent).
this._dispatchModify(EntityModifyFlags.Child, this)is inside theforloop, so the parent receives oneChildevent per child cleared. The new test assertsparentModifyCount[EntityModifyFlags.Child]).eq(1)(seetests/src/core/Entity.test.ts), which will fail wheneverclearChildren()runs over more than one child (the test itself uses two children).If the intent is a single bulk event per the test's comment ("a single
Childmodify event for the whole clear"), hoist the dispatch out of the loop. If the intent really is one event per child (more consistent with_removeFromParent/_addToChildrenList), the test assertion needs updating instead — but please pick one.🛠️ Suggested fix (single bulk event, matches the test)
clearChildren(): void { const children = this._children; + if (children.length === 0) return; + // 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. A single bulk event represents + // the entire clear operation. + this._dispatchModify(EntityModifyFlags.Child, this); 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; }🤖 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 399 - 420, In clearChildren(), move the call to this._dispatchModify(EntityModifyFlags.Child, this) out of the per-child loop so the parent emits a single bulk Child modify event for the entire clear operation; preserve the existing child cleanup sequence (clearing _parent/_siblingIndex, computing activeChangeFlag and calling child._processInActive(activeChangeFlag), Entity._traverseSetOwnerScene(child, null), and child._setParentChange()) for each child, then after the loop (before or after children.length = 0) invoke this._dispatchModify(EntityModifyFlags.Child, this) once to match the test and documented behavior.tests/src/core/Entity.test.ts (1)
419-425:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStale assertion: lonely entity no longer throws on
siblingIndexset.This test predates the new orphan-entity behavior in
Entity.siblingIndex(theLogger.warnpath added in this PR).entityXhas no parent and is not a root, so the setter now warns instead of throwing —expect(lonelyBadFn).to.throw()will fail.Either assert on the warning (spy
Logger.warn) or remove thethrowexpectation.🛠️ Suggested fix
- // thorw error when set lonely entity - const entityX = new Entity(engine, "entityX"); - var lonelyBadFn = function () { - entityX.siblingIndex = 1; - }; - expect(lonelyBadFn).to.throw(); + // Lonely entity should warn (not throw) when siblingIndex is set + const entityX = new Entity(engine, "entityX"); + const warnSpy = vi.spyOn(Logger, "warn").mockImplementation(() => {}); + expect(() => { entityX.siblingIndex = 1; }).not.toThrow(); + expect(warnSpy).toHaveBeenCalled(); + warnSpy.mockRestore();Note: this also requires importing
Loggerfrom@galacean/engine-coreat the top of the file.🤖 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 419 - 425, The test currently expects Entity.siblingIndex setter to throw for an orphan entity (entityX), but the setter now warns instead; update the test to spy on Logger.warn (import Logger from '@galacean/engine-core' at the top), set up a spy around Logger.warn, call the setter (e.g., entityX.siblingIndex = 1), assert that Logger.warn was called with an appropriate message, and restore the spy; alternatively remove the expect(lonelyBadFn).to.throw() assertion and replace it with the Logger.warn assertion referencing entityX and the siblingIndex setter.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/core/src/Entity.ts`:
- Around line 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`).
In `@tests/src/core/Entity.test.ts`:
- Around line 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.
---
Outside diff comments:
In `@packages/core/src/Entity.ts`:
- Around line 399-420: In clearChildren(), move the call to
this._dispatchModify(EntityModifyFlags.Child, this) out of the per-child loop so
the parent emits a single bulk Child modify event for the entire clear
operation; preserve the existing child cleanup sequence (clearing
_parent/_siblingIndex, computing activeChangeFlag and calling
child._processInActive(activeChangeFlag), Entity._traverseSetOwnerScene(child,
null), and child._setParentChange()) for each child, then after the loop (before
or after children.length = 0) invoke
this._dispatchModify(EntityModifyFlags.Child, this) once to match the test and
documented behavior.
In `@tests/src/core/Entity.test.ts`:
- Around line 419-425: The test currently expects Entity.siblingIndex setter to
throw for an orphan entity (entityX), but the setter now warns instead; update
the test to spy on Logger.warn (import Logger from '@galacean/engine-core' at
the top), set up a spy around Logger.warn, call the setter (e.g.,
entityX.siblingIndex = 1), assert that Logger.warn was called with an
appropriate message, and restore the spy; alternatively remove the
expect(lonelyBadFn).to.throw() assertion and replace it with the Logger.warn
assertion referencing entityX and the siblingIndex setter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8edd7d34-fe19-44e8-8217-73c91be6bae2
📒 Files selected for processing (3)
packages/core/src/Entity.tspackages/core/src/SceneManager.tstests/src/core/Entity.test.ts
| 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`); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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`).
| 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); |
There was a problem hiding this comment.
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.
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
clearChildren 缺少 child._siblingIndex = -1 重置 |
✅ 已修复 |
clearChildren 缺少 child._setParentChange() |
✅ 已修复 |
SceneManager.loadScene destroy 遍历时修改正在遍历的数组 |
✅ 已修复(getLoopArray()) |
总结
方向正确,三项历史 bug 均已处理。但仍有两个问题未修复。
问题
[P0] Entity.ts:220 — set siblingIndex 中存在 if/else/else 语法错误,无法通过 TypeScript 编译
当前代码结构:
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 { // ← 第二个 else,非法
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}这是编译级错误,会导致 CI 全线失败。正确结构:
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`);
}[P0] Entity.ts:409 — clearChildren 循环内每次 dispatch Child 事件,测试断言必然 fail
当前实现:
for (let i = children.length - 1; i >= 0; i--) {
...
this._dispatchModify(EntityModifyFlags.Child, this); // 每个子节点各触发一次
...
}有 2 个子节点时,parentModifyCount[EntityModifyFlags.Child] 值为 2,但测试在第 350 行断言:
expect(parentModifyCount[EntityModifyFlags.Child]).eq(1);clearChildren 是批量操作,父节点只应收到一次 Child 事件。实现与测试自相矛盾,两者必有一个是错的。
测试语义是正确的,应当修改实现:将 _dispatchModify 移到循环之前(不是之后),让订阅者在所有子节点被移除前收到一次通知,之后在循环内执行清理:
clearChildren(): void {
const children = this._children;
if (children.length === 0) return;
// Notify listeners once before detaching any child, so subscribers (e.g. UICanvas)
// can react while all children are still accessible.
this._dispatchModify(EntityModifyFlags.Child, this);
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
// ... activeChangeFlag logic ...
child._processInActive(activeChangeFlag);
Entity._traverseSetOwnerScene(child, null);
child._setParentChange();
}
children.length = 0;
}这同时解决了注释里提到的时序问题(dispatch 在 _processInActive 之前),又保证只触发一次。
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
修复 Entity.clearChildren() 中两个 bug:
- 子节点
_siblingIndex未重置:detach 后子节点仍持有旧 siblingIndex(>= 0),setSiblingIndex的检测逻辑会认为它仍在层级中,引发错误行为 _setParentChange()未调用:子节点 detach 后未通知订阅者父节点变更,订阅了 Parent modify 事件的系统(如 Transform dirty)无法感知
同时修复 setSiblingIndex setter 中死代码 if (this._siblingIndex === -1) throw 因 else 错位导致的逻辑断裂,改为 Logger.warn(非 throw)语义也更合理——孤立节点设置 siblingIndex 是用户误操作,不是程序错误。
测试通过公开 API 触发,断言公开输出(parent.children.length、子节点 siblingIndex、modify 事件计数),正确的链路测试。
问题
[P2] clearChildren 中 _dispatchModify 在循环内被重复调用
当前代码在每个子节点的循环体内都调用 this._dispatchModify(EntityModifyFlags.Child, this),父节点会被通知 N 次(有几个子节点就通知几次)。如果订阅者(如 UICanvas)在每次通知时做复杂操作(如 layout invalidation),会有 O(N) 重复开销。
Unity 在 ClearChildren 等批量操作中只在完成后发送一次 dirty 通知。
测试断言 parentModifyCount[EntityModifyFlags.Child] === 1,但实际实现在 N 个子节点时发送 N 次,测试会失败(除非测试数据只有 1 个子节点时巧合通过)。——验证:测试用了两个子节点(child + child2),若当前实现发 2 次,parentModifyCount[Child] === 1 断言会 fail。建议将 _dispatchModify 移到循环外,在 children.length = 0 后统一发一次。
// 修复:移出循环
for (let i = ...) {
child._siblingIndex = -1;
// 不在这里 dispatch
// ...
child._setParentChange();
}
children.length = 0;
this._dispatchModify(EntityModifyFlags.Child, this); // 一次性通知LGTM(P2 处理后合入)。
Summary
Two small
Entitybugs found during prefab/scene editing:clearChildren缺失边界处理 — fix: ensure children-related state is cleaned up correctly whenclearChildrenis invoked. Adds regression test inEntity.test.ts.siblingIndexsetter 在无父节点时报错 —_setSiblingIndexpath assumed_parentexists when_isRootis false; the new guardelse if (this._parent)skips the operation rather than crashing.Test Plan
Entity.test.tscovers the clearChildren regressionsiblingIndexset without errorSummary by CodeRabbit
Release Notes
Bug Fixes
Tests