fix(core): add @ignoreClone to Transform._parentTransformCache#3036
Conversation
…d UITransform _parentChange() previously called _updateAllWorldFlag(), which has an early-exit when the current node already has all target world dirty flags set. After reparent, a descendant whose WorldMatrix flag was previously cleared keeps returning stale cached worldMatrix. Replace with _propagateReparentDirty / _propagateReparentDirtyUI that unconditionally recurse through all descendants, setting _isParentDirty and world dirty flags regardless of the current node's flag state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Walkthrough
ChangesFix stale parent-cache on clone
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
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 docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3036 +/- ##
===========================================
+ Coverage 77.31% 79.76% +2.45%
===========================================
Files 914 914
Lines 101723 101797 +74
Branches 10437 11198 +761
===========================================
+ Hits 78647 81203 +2556
+ Misses 22892 20413 -2479
+ Partials 184 181 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
During clone, ComponentCloner.cloneProperty overwrites _parentTransformCache after a listener has already resolved it, producing a stale state (_isParentDirty=false, cache=null). On subsequent reparent, _getParentTransform returns null from cache and worldMatrix falls back to localMatrix. Root cause: _parentTransformCache lacked @ignoreClone, so cloneProperty copied the source's value over the already-resolved cache. With @ignoreClone, clone keeps the default null + _isParentDirty=true (also @ignoreClone), so the first _getParentTransform call after clone resolves correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
force-pushed
the
fix/transform-reparent-propagation
branch
from
ac3a325 to
147d178
Compare
cptbtptpbcptdtptp
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Transform.ts`:
- Line 567: The _parentChange() method at the specified location delegates to
_updateAllWorldFlag, which has an early exit optimization that prevents it from
recursively invalidating descendants when the modification flags are already
set. This causes stale worldMatrix caches to persist after reparent operations.
Replace the call to _updateAllWorldFlag with a method that unconditionally
propagates invalidation to all descendants regardless of whether the flags are
currently set, ensuring descendants receive the reparent invalidation even if
the parent's flags were already marked as modified.
🪄 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: 7c67b07a-14e2-42ba-ba78-6f6d0dc04002
📒 Files selected for processing (2)
packages/core/src/Transform.tstests/src/core/Transform.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/Transform.ts`:
- Line 567: The _parentChange() method at the specified location delegates to
_updateAllWorldFlag, which has an early exit optimization that prevents it from
recursively invalidating descendants when the modification flags are already
set. This causes stale worldMatrix caches to persist after reparent operations.
Replace the call to _updateAllWorldFlag with a method that unconditionally
propagates invalidation to all descendants regardless of whether the flags are
currently set, ensuring descendants receive the reparent invalidation even if
the parent's flags were already marked as modified.
🪄 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: 7c67b07a-14e2-42ba-ba78-6f6d0dc04002
📒 Files selected for processing (2)
packages/core/src/Transform.tstests/src/core/Transform.test.ts
🛑 Comments failed to post (1)
packages/core/src/Transform.ts (1)
567-567:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReparent invalidation still short-circuits descendant propagation.
At Line 567,
_parentChange()still delegates to_updateAllWorldFlag(...), but_updateAllWorldFlag(Line 770+) exits early when current flags are already set, which prevents recursive invalidation of descendants. This directly leaves staleworldMatrixcaches reachable after reparent in the scenario described by this PR.Suggested fix
_parentChange(): void { this._isParentDirty = true; - this._updateAllWorldFlag(TransformModifyFlags.WmWpWeWqWsWus); + this._propagateReparentDirty(TransformModifyFlags.WmWpWeWqWsWus); } + private _propagateReparentDirty(flags: TransformModifyFlags): void { + this._isParentDirty = true; + this._worldAssociatedChange(flags); + const children = this._entity._children; + for (let i = 0, n = children.length; i < n; i++) { + children[i].transform?._propagateReparentDirty(flags); + } + }📝 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._parentChange(): void { this._isParentDirty = true; this._propagateReparentDirty(TransformModifyFlags.WmWpWeWqWsWus); } private _propagateReparentDirty(flags: TransformModifyFlags): void { this._isParentDirty = true; this._worldAssociatedChange(flags); const children = this._entity._children; for (let i = 0, n = children.length; i < n; i++) { children[i].transform?._propagateReparentDirty(flags); } }🤖 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/Transform.ts` at line 567, The _parentChange() method at the specified location delegates to _updateAllWorldFlag, which has an early exit optimization that prevents it from recursively invalidating descendants when the modification flags are already set. This causes stale worldMatrix caches to persist after reparent operations. Replace the call to _updateAllWorldFlag with a method that unconditionally propagates invalidation to all descendants regardless of whether the flags are currently set, ensuring descendants receive the reparent invalidation even if the parent's flags were already marked as modified.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reproduces the real trigger: a component reading worldMatrix inside _cloneTo (as DynamicCollider does) resolves a parent's _parentTransformCache before the parent transform's cache is cloned. Without the fix the copy stomps it back to the source's null while _isParentDirty stays false, so the node renders at the origin after reparent. Fails on dev/2.0, passes with this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root causeAfter How the inconsistency is created (during
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/src/core/Transform.test.ts`:
- Around line 184-186: The test is incomplete as it only validates the x and y
translation components of the worldMatrix (elements[12] and elements[13]) but is
missing the z-coordinate assertion (elements[14]), while the earlier test
validates all three components. Add an expectation statement that asserts
world.elements[14] equals 0 to ensure complete and consistent validation of all
three translation coordinates, matching the thoroughness of the first test case.
🪄 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: 1f284098-797a-4afc-86d3-ae364cd78103
📒 Files selected for processing (1)
tests/src/core/Transform.test.ts
| const world = cloneLayer.transform.worldMatrix; | ||
| expect(world.elements[12]).to.equal(1010); | ||
| expect(world.elements[13]).to.equal(2020); |
There was a problem hiding this comment.
Consider asserting the z-coordinate for consistency and completeness.
The first test validates all three worldMatrix translation components (lines 151-153), but this test only checks elements[12] (x) and elements[13] (y). For consistency and complete coverage, consider adding:
expect(world.elements[14]).to.equal(0);This ensures all three coordinates are validated and matches the thoroughness of the first test case.
🤖 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/Transform.test.ts` around lines 184 - 186, The test is
incomplete as it only validates the x and y translation components of the
worldMatrix (elements[12] and elements[13]) but is missing the z-coordinate
assertion (elements[14]), while the earlier test validates all three components.
Add an expectation statement that asserts world.elements[14] equals 0 to ensure
complete and consistent validation of all three translation coordinates,
matching the thoroughness of the first test case.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
新增 commit d597e886（相对上轮 HEAD 147d17880 只多了 33 行测试，源码一行 @ignoreClone 未变）补了第二个测试 WorldReaderOnClone——一个在 _cloneTo 里读 worldMatrix 的 Script，忠实复刻了 DynamicCollider._cloneTo → _syncNative → _addNativeShape 读 lossyWorldScale 的真实触发路径。
这正是我上两轮缺的那块拼图。 我用 PR HEAD 的 core dist 跑了反向证伪（CloneManager.getCloneMode(Transform) 运行时切换 _parentTransformCache 的 clone-mode：0=有 @ignoreClone / delete=无，等价 undecorated）：
=== WITH @ignoreClone (fix) ===
_cloneTo reads during clone: 1
post-clone cloneLayer: _isParentDirty=false cache=set
worldMatrix tx/ty = [1010, 2020] => PASS (follows holder)
=== WITHOUT @ignoreClone (toggled, pre-fix) ===
_cloneTo reads during clone: 1
post-clone cloneLayer: _isParentDirty=false cache=null ← stale
worldMatrix tx/ty = [10, 20] => FAIL (falls back to local)
链路完全打通，我上轮的两条 P1 全部关闭：
- 上轮 [P1]「fix 是 no-op，构造不出失败态」—— 当时我的论证（clone 期间 listener 零 dispatch）对 listener 路径成立、现已复测仍 0 fire；但作者的真实路径是
_cloneTo，它由ComponentCloner.cloneComponent(ComponentCloner.ts:40) 同步直接调，不依赖 dispatch，所以 clone 期间确实 fire（1 次读）。_parseCloneEntity(Entity.ts:483-490) 先递归 clone 整棵子树、后 clone 本节点组件——于是 leaf 的_cloneTo读worldMatrix时，父节点 layer 的 Transform 尚未被 clone，这次读 resolve 了cloneLayer._parentTransformCache（设_isParentDirty=false），随后 layer.Transform 被 clone、cloneProperty(CloneManager.ts:117) 对null源值走裸 assign 把已 resolve 的 cache 覆盖回null→ 留下(cache=null, _isParentDirty=false)的 stale 态。@ignoreClone让 cloneProperty 在 :114 提前 return、不覆盖 → 修复成立。 - 上轮 [P1]「测试不守回归」—— 新测试 revert
@ignoreClone后 FAIL、加回 PASS，断言是强形式（equal(1010)而非「非 garbage」），真守回归。
落点也对：root cause 是 _isParentDirty(@ignoreClone) 与 _parentTransformCache(未装饰) 在 cloner 眼里的不对称，一行让二者对称是本质解，非输出端擦屁股。非 null cache 那条本就走 Component._remap(Component.ts:160) 正确重映射，只有 null 源值会掉进裸 assign——所以这一行精准只补了真正破的那个 case。
整体可合。下面两条都不阻塞。
问题
[P2] tests/src/core/Transform.test.ts:127 第一个测试 WorldMatrixListener 不守回归，建议删除
我同样复测了它：clone 期间 listener 零次 fire（与上轮结论一致），revert @ignoreClone 后它照样 PASS（tx=1010）。它没在守本 bug——_updateFlagManager 的 listener 走的是 dispatch 路径，而 clone 期间 Transform 收尾用 _setDirtyFlagTrue（只 |= 不 dispatch），所以 listener 不触发、cache 不被 resolve、失败态根本构造不出来。作者本人在 PR 评论里也已确认「the pre-existing WorldMatrixListener test passes with or without the fix … can be removed in favor of the test above」。
既然 WorldReaderOnClone 已经真守回归，留着这个 0-fire 的 listener 测试只会给人「两个用例都在守 bug」的错觉、稀释信号。建议直接删掉，只留 WorldReaderOnClone 一个。（与上轮 :125/:155 那对是同性质问题，但指向的是本轮新代码，故重提一次。）
[P3] PR 描述的 Root cause 段落与真实机制不符 / 评论里的 _propagateReparentDirty 不在本 PR
- PR body 的「Root cause」第 2 步写的是 listener 路径（「某个属性赋值触发 dirty dispatch → listener 读 worldMatrix」），而这条路径上面已证 clone 期间不 fire。真实机制是
_cloneTo同步读——这一点WorldReaderOnClone的 inline 注释（:174-180）写得完全正确，但 PR body 仍停留在被证伪的 listener 理论。建议把 body 的 Root cause 更新成与测试注释一致的_cloneTo版本，免得后人对照 body 复现失败。 - 2026-06-17 的评论里「The companion
_propagateReparentDirtychange additionally re-dirties the whole subtree on reparent」——这个_propagateReparentDirty改动在上一轮已被整体 revert，不在当前 diff 里（git diff 147d17880 d597e886 --stat只有测试文件）。当前 PR 只有「一行@ignoreClone」单一 fix，没有 companion 改动。评论这句会让 reviewer 误以为 reparent 侧也改了，建议订正措辞。
简化建议
无源码侧简化（一行 fix 已是最小形态，落点正确）。测试侧如 [P2]：删掉不守回归的 WorldMatrixListener，只留 WorldReaderOnClone 这一个真正反向证伪的用例。
2 participants
Summary
Transform._parentTransformCache缺少@ignoreClone，导致 clone 过程中cloneProperty覆盖了已被 listener resolve 过的 cache，产生 stale state（_isParentDirty=false,_parentTransformCache=null）。_getParentTransform直接返回 null →worldMatrix = localMatrix，不反映新 parent 的偏移。Root cause
_createCloneEntity创建 clone 树时，Script 构造函数注册了 transform 变化 listener_parseCloneEntityclone Transform 属性时，某个属性赋值触发 dirty dispatch → listener 读worldMatrix→_getParentTransformresolve →_isParentDirty=falsecloneProperty继续遍历到_parentTransformCache→ 用源的值覆盖 → 写入 null_isParentDirty=false+_parentTransformCache=null→ staleFix
给
_parentTransformCache加@ignoreClone（一行）。clone 后保持默认值 null +_isParentDirty=true（也是@ignoreClone），首次_getParentTransform调用会正确 resolve。Test plan
pnpm run test— Transform tests pass (5/5)🤖 Generated with Claude Code
Summary by CodeRabbit