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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| clearChildren 缺少 child._siblingIndex 重置 | ✅ 已修复 |
| clearChildren 缺少 child._setParentChange() | ✅ 已修复 |
| SceneManager.loadScene destroy 遍历时修改正在遍历的数组 | ✅ 已修复（getLoopArray()） |
| siblingIndex setter 无父节点时应 warn 而非 throw | ✅ 已修复（Logger.warn） |
总结
两处 Entity hierarchy bug 修复方向均正确，clearChildren 的状态清理完整，SceneManager 并发迭代问题已修复。但 P0 语法错误和 P1 dispatch 过度仍存在，需修复后再合入。
问题
[P0] Entity.ts:215 — siblingIndex setter 存在 if/else/else 语法错误，TypeScript 无法编译
当前 HEAD 代码（已通过读取分支确认）：
set siblingIndex(value: number) {
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`);
}
}结构应为三路分支：_isRoot / _parent != null / 孤立。修复：
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`);
}
}[P1] Entity.ts:401 — clearChildren 在循环内每次调用 _dispatchModify(EntityModifyFlags.Child, this)，触发 N 次
removeChild 只触发一次 EntityModifyFlags.Child。clearChildren 改后在 N 个子节点的循环内触发 N 次，UICanvas 等订阅者执行 N 次无效刷新。测试也断言 parentModifyCount[EntityModifyFlags.Child] === 1，但当前实现会触发 N 次，测试自身也会失败（除非 N=1）。
建议将 _dispatchModify 移出循环，在 children.length = 0 之后调用一次：
clearChildren(): void {
const children = this._children;
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
let activeChangeFlag = ActiveChangeFlag.None;
child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy);
child._isActiveInScene && (activeChangeFlag |= ActiveChangeFlag.Scene);
activeChangeFlag && child._processInActive(activeChangeFlag);
Entity._traverseSetOwnerScene(child, null);
child._setParentChange();
}
children.length = 0;
this._dispatchModify(EntityModifyFlags.Child, this); // ← 移到循环外，触发一次
}注意：_dispatchModify 需要在 children.length = 0 之后调用，而不是之前——让订阅者看到已清空的状态。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| clearChildren 缺少 child._siblingIndex 重置 | ✅ 已修复 |
| clearChildren 缺少 child._setParentChange() | ✅ 已修复 |
| SceneManager.loadScene destroy 遍历时修改正在遍历的数组 | ✅ 已修复（getLoopArray()） |
| siblingIndex setter 无父节点时应 warn 而非 throw | ✅ 意图正确，但实现有语法错误 |
问题
[P0] Entity.ts:213 — siblingIndex setter 存在 if/else/else 语法错误，TypeScript 无法编译
当前 diff 生成的代码结构：
set siblingIndex(value: number) {
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，TypeScript 语法错误
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}这是一个有三路分支的逻辑，应该使用 if / else if / else：
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
} else if (this._parent) {
this._setSiblingIndex(this._parent._children, value);
this._parent._dispatchModify(EntityModifyFlags.Child, this._parent);
} else {
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}此错误已在第 3 轮 review 指出，多轮未修复。
[P1] Entity.ts:404 — clearChildren 在循环内每次调用 _dispatchModify(EntityModifyFlags.Child, this)
当前循环体内 dispatch N 次，但：
- 测试断言
parentModifyCount[EntityModifyFlags.Child] === 1，实际会得到 N（2 个 child 就是 2）—— 测试自身会失败 removeChild的语义是每次 dispatch 1 次，clearChildren整体只需 dispatch 1 次
建议将 _dispatchModify 移出循环，在 children.length = 0 之后调用：
clearChildren(): void {
const children = this._children;
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
let activeChangeFlag = ActiveChangeFlag.None;
child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy);
child._isActive && (activeChangeFlag |= ActiveChangeFlag.Self);
activeChangeFlag && child._processInActive(activeChangeFlag);
Entity._traverseSetOwnerScene(child, null);
child._setParentChange();
}
children.length = 0;
this._dispatchModify(EntityModifyFlags.Child, this); // ← 移到循环外，整体 dispatch 1 次
}
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
clearChildren 缺少 child._siblingIndex 重置 |
✅ 已修复 |
clearChildren 缺少 child._setParentChange() |
✅ 已修复 |
SceneManager.loadScene destroy 遍历时修改正在遍历的数组 |
✅ 已修复（getLoopArray()） |
以下两个问题确认仍未修复：
[P0] Entity.ts:213 — siblingIndex setter 存在 else/else 语法错误，TypeScript 无法编译
diff 生成的代码结构：
set siblingIndex(value: number) {
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`);
}
}三路分支应使用 if / else if / else：
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
} else if (this._parent) {
this._setSiblingIndex(this._parent._children, value);
this._parent._dispatchModify(EntityModifyFlags.Child, this._parent);
} else {
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}此问题已在第三轮 review 指出，当前 PR diff 仍携带错误结构。
[P1] Entity.ts:404 — clearChildren 在循环内每次都调用 _dispatchModify(EntityModifyFlags.Child, this)
当前实现在循环体内 dispatch N 次（每个 child 各一次），但：
- 测试断言
parentModifyCount[EntityModifyFlags.Child]).eq(1)，实际结果等于子节点数量——测试自身会失败 clearChildren整体只需一次Childnotify，应移出循环
建议：将 _dispatchModify 移到循环之外，在 children.length = 0 之后调用：
clearChildren(): void {
const children = this._children;
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
let activeChangeFlag = ActiveChangeFlag.None;
child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy);
child._isActive && (activeChangeFlag |= ActiveChangeFlag.Self);
activeChangeFlag && child._processInActive(activeChangeFlag);
Entity._traverseSetOwnerScene(child, null);
child._setParentChange();
}
children.length = 0;
this._dispatchModify(EntityModifyFlags.Child, this); // 一次，在循环外
}以上 P0/P1 修复后可合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
clearChildren 缺少 child._siblingIndex 重置 |
✅ 已修复 |
clearChildren 缺少 child._setParentChange() |
✅ 已修复 |
SceneManager.loadScene destroy 遍历时修改正在遍历的数组 |
✅ 已修复（getLoopArray()） |
以下两个问题确认仍未修复。
[P0] Entity.ts:213 — siblingIndex setter 存在 if/else/else 语法错误，TypeScript 无法编译
diff 生成的代码结构为：
set siblingIndex(value: number) {
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`);
}
}这是无效的 TypeScript 语法，会导致 tsc、rollup 等所有构建工具报错，CI 全线失败。
正确写法：
set siblingIndex(value: number) {
if (this._isRoot) {
this._setSiblingIndex(this._scene._rootEntities, value);
} else if (this._parent) {
this._setSiblingIndex(this._parent._children, value);
this._parent._dispatchModify(EntityModifyFlags.Child, this._parent);
} else {
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}[P1] clearChildren — _dispatchModify 在循环内被调用 n 次，但测试断言只期望 1 次
当前循环结构：
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
// ...
this._dispatchModify(EntityModifyFlags.Child, this); // ← 每个子节点都触发一次
// ...
}clearChildren 是一个批量操作，订阅者（如 UICanvas）在收到 Child 事件后会触发完整的重新计算。对 n 个子节点触发 n 次，会导致 O(n²) 的更新开销。
测试文件 Entity.test.ts:351 中：
expect(parentModifyCount[EntityModifyFlags.Child]).eq(1);该断言当前因为循环内调用了 n 次而会失败（n=2 时，parentModifyCount[EntityModifyFlags.Child] 为 2）。
正确做法：将 dispatch 移出循环，在所有子节点处理完毕后调用一次：
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
// ... processInActive, traverseSetOwnerScene, _setParentChange
}
children.length = 0;
this._dispatchModify(EntityModifyFlags.Child, this); // ← 移出循环请修复以上两个问题后再合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
clearChildren 缺少 child._siblingIndex 重置 |
✅ 已修复 |
clearChildren 缺少 child._setParentChange() |
✅ 已修复 |
SceneManager.loadScene destroy 遍历时修改正在遍历的数组 |
✅ 已修复（getLoopArray()） |
以下两个问题确认仍未修复。
[P0] Entity.ts:215 — siblingIndex setter 存在 if/else/else 语法错误，TypeScript 无法编译
当前 diff 生成的代码结构：
set siblingIndex(value: number) {
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 挂在同一个 if 上，TS 语法错误
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) {
this._setSiblingIndex(this._parent._children, value);
this._parent._dispatchModify(EntityModifyFlags.Child, this._parent);
} else {
Logger.warn(`The entity ${this.name} is not in the hierarchy`);
}
}[P1] clearChildren 在循环内每次都调用 _dispatchModify(EntityModifyFlags.Child, this)，触发 N 次而非 1 次
当前代码在 for 循环体内调用 this._dispatchModify(EntityModifyFlags.Child, this)，有 N 个子节点就触发 N 次 Child modify 事件。测试断言 parentModifyCount[EntityModifyFlags.Child].eq(1) 在只有 2 个子节点时偶然通过是不可能的——实际上会是 2，测试本身也会失败。
正确做法是把 _dispatchModify 移到循环外，整个 clearChildren 完成后发一次：
_clearChildren(): void {
const children = this._children;
for (let i = children.length - 1; i >= 0; i--) {
const child = children[i];
child._parent = null;
child._siblingIndex = -1;
// ... processInActive, traverseSetOwnerScene, _setParentChange ...
}
children.length = 0;
this._dispatchModify(EntityModifyFlags.Child, this); // ← 一次，在循环外
}请修复以上两个问题后重新提交。
2 participants
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