[codex] preserve Transform replacement dependencies#3073
[codex] preserve Transform replacement dependencies#3073cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughEntity transform handling now selects transform subclasses during construction and replaces existing transforms while preserving slots and state. Dependency validation, destruction, cloning, removal safety, and core/UI regression tests were updated. ChangesTransform replacement flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Entity
participant ComponentsDependencies
participant ReplacementTransform
Entity->>ComponentsDependencies: validate replacement dependencies
Entity->>ReplacementTransform: create replacement transform
Entity->>Entity: copy state and preserve component slot
Entity->>ReplacementTransform: destroy previous transform
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
cptbtptpbcptdtptp
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3073 +/- ##
===========================================
- Coverage 79.82% 79.64% -0.18%
===========================================
Files 904 904
Lines 101297 101257 -40
Branches 11423 11434 +11
===========================================
- Hits 80865 80651 -214
- Misses 20250 20422 +172
- Partials 182 184 +2
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:
|
![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
🧹 Nitpick comments (1)
tests/src/core/Transform.test.ts (1)
173-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert cloned Transform slot identity, not just constructor ordering.
Matching constructors can still hide a detached same-class Transform in
clone.transform, which is the mapping regression this PR targets.
tests/src/core/Transform.test.ts#L173-L177: assertclone._components[0] === clone.transform.tests/src/ui/UITransform.test.ts#L415-L421: assertcloned._components[0] === cloned.transform.🤖 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 173 - 177, Strengthen the clone identity assertions by verifying the Transform component itself is reused, not merely that constructors match: in tests/src/core/Transform.test.ts lines 173-177, assert clone._components[0] === clone.transform; apply the same assertion in tests/src/ui/UITransform.test.ts lines 415-421 for cloned._components[0] === cloned.transform.
🤖 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 244-258: The Entity constructor’s component ordering must resolve
Transform dependencies from the queued component types before validation and
installation. Update the logic around _isTransformType, addComponent, and
AutoAddDependentTransform so queued dependencies are recognized and not
auto-added again; install the selected Transform in slot 0, then add each
remaining requested component exactly once. Add constructor regressions covering
CheckOnlyDependentTransform and AutoAddDependentTransform with MeshRenderer.
---
Nitpick comments:
In `@tests/src/core/Transform.test.ts`:
- Around line 173-177: Strengthen the clone identity assertions by verifying the
Transform component itself is reused, not merely that constructors match: in
tests/src/core/Transform.test.ts lines 173-177, assert clone._components[0] ===
clone.transform; apply the same assertion in tests/src/ui/UITransform.test.ts
lines 415-421 for cloned._components[0] === cloned.transform.
🪄 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 Plus
Run ID: 91207389-0e89-42d4-91d9-c4b44eefd453
📒 Files selected for processing (4)
packages/core/src/ComponentsDependencies.tspackages/core/src/Entity.tstests/src/core/Transform.test.tstests/src/ui/UITransform.test.ts
| let transformType: ComponentConstructor = Transform; | ||
| const n = components.length; | ||
| for (let i = n - 1; i >= 0; i--) { | ||
| const componentType = components[i]; | ||
| if (Entity._isTransformType(componentType)) { | ||
| transformType = componentType; | ||
| break; | ||
| } | ||
| } | ||
| this._transform = <Transform>this.addComponent(transformType); | ||
| for (let i = 0; i < n; i++) { | ||
| const componentType = components[i]; | ||
| if (!Entity._isTransformType(componentType)) { | ||
| this.addComponent(componentType); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve constructor-supplied Transform dependencies before validation.
Line 253 installs the Transform before queued components. new Entity(engine, "x", MeshRenderer, CheckOnlyDependentTransform) now throws because MeshRenderer is not installed yet. With AutoAddDependentTransform, auto-add inserts MeshRenderer before the Transform, and the loop then adds the requested renderer again—also moving the Transform out of slot 0.
Resolve Transform dependencies against pending constructor component types and avoid auto-adding dependencies already queued; add CheckOnly and AutoAdd constructor regressions.
🤖 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 244 - 258, The Entity constructor’s
component ordering must resolve Transform dependencies from the queued component
types before validation and installation. Update the logic around
_isTransformType, addComponent, and AutoAddDependentTransform so queued
dependencies are recognized and not auto-added again; install the selected
Transform in slot 0, then add each remaining requested component exactly once.
Add constructor regressions covering CheckOnlyDependentTransform and
AutoAddDependentTransform with MeshRenderer.
🤖 Augment PR SummarySummary: Fixes Transform subclass installation/replacement so each Changes:
Why: Prevents erroneous “remove Renderer before remove Transform” errors and preserves component ordering assumptions used by cloning and constructor-based mapping. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| // Keep the unique Transform in the same component slot. Detach the old | ||
| // instance before destroy because destroy can be deferred during a frame. | ||
| const components = this._components; | ||
| const previousIndex = components.indexOf(previous); |
There was a problem hiding this comment.
_replaceTransform() assumes the current this._transform is still present in this._components; if the Transform was previously destroyed/removed (or otherwise detached), previousIndex becomes -1 and components[previousIndex] = value writes to a non-index property, leaving the replacement Transform untracked in the component list.
This can corrupt invariants like getComponent(Transform)/clone ordering, so it may be worth guarding against a missing previous slot (or ensuring _transform can’t be detached without being replaced).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
总结
整体方向正确：Transform slot、局部姿态和延迟销毁期间的唯一可见实例都应由 Entity 统一维护；99c357a 相对上一提交仅补了一处花括号，没有改变主体行为。但完整链路仍有 3 个 P1：两条现有未解决 thread 均可由公开 API 或 clone 真实触发，此外替换依赖校验还会提交违反声明契约的最终组件图，因此当前不应合入。
问题
-
[P1] packages/core/src/ComponentsDependencies.ts:49 / packages/core/src/Entity.ts:270 — replacement 的依赖校验读取了两个不同的组件快照。 Entity 先执行 remove check，随后 add check 和组件构造都可以通过 AutoAdd 或重入新增组件，最后又先 detach old Transform，使 destroy callback 因 index < 0 跳过最终校验。可复现例：当前是 UITransform，NewTransform extends Transform 声明 dependentComponents(Image, AutoAdd)；初次 remove check 时没有 Image，add check 会在旧 UITransform 尚在时成功添加 Image，swap 后却只剩 NewTransform + Image，最终违反 Image → UITransform，UIRenderer 下游读取 transform.size 时可崩。应保留 ComponentsDependencies 作为最终组件图的权威 owner，删除“提前 remove check + 按旧 live state add check”这组双阶段事实源；从 planned final set 机械派生一次校验结果，再由 Entity 原子提交 slot，并用反向测试保证失败时旧 Transform、slot 和本次 AutoAdd 均不漂移。
-
[P1] 已有 constructor dependency thread 仍成立： #3073 (comment) 。合法实体可先安装 MeshRenderer 再替换成 CheckOnlyDependentTransform，但 clone 把 [CheckOnlyDependentTransform, MeshRenderer] 交给新 constructor 后会先校验 Transform，直接因 MeshRenderer 尚未安装而抛错；AutoAdd(MeshRenderer) 的实际失败点更早，是 MeshRenderer 自身在初始 Transform 尚未可见时的依赖检查。Entity constructor 应保留整批安装计划的 owner，按依赖安全顺序真实安装；不要在 clone 端加特判，也不要把 queued type 当成已安装的第三份状态。
-
[P1] 已有 detached-slot thread 仍成立： #3073 (comment) 。公开路径 entity.transform.destroy() → entity.addComponent(SubTransform) 会让 previousIndex 为 -1，数组只得到名为 “-1” 的属性；replacement 不进入 membership，随后 getComponent、clone 和 Entity.destroy 全部丢失它。这里应保留 _components 为 membership/order 的权威 owner，_transform 只作 O(1) 派生 cache；把 remove/install 收口到同一 mutation funnel，删除“_transform 非空就必有有效 slot”的隐式双源假设，而不是再加 detached flag 或只做 return guard。
-
[P2] tests/src/core/Transform.test.ts:195 — deferred-destruction 用例没有断言回调后的状态。 所有断言都在 finally 中 previous.destroy() 之前；若 detached guard 回退，旧逻辑会在这里 splice(-1, 1) 删除 replacement，但该用例仍通过。销毁/flush 后应再次断言 previous 已销毁、entity.transform 仍是 replacement，且组件数组中唯一 Transform 仍是 replacement。
-
[P2] packages/core/src/ComponentsDependencies.ts:43-44 等 — 一次收口新增代码的样式契约。 第 44 行 early return 仍缺花括号，而末提交只修了 Entity 中同类写法；第 43 行以及 Entity.ts:556-557、796-797 的新增 // 注释末尾仍有句号。请统一补花括号并去掉这些 // 末尾句号。
简化建议
constructor 与 replacement 最终都应走同一条最小管线：从请求机械派生最终 component set → ComponentsDependencies 一次校验 → Entity 一次提交 _components 与派生 _transform → 再销毁 outgoing component。这样可以同时删掉 clone 特判诱因、双阶段依赖状态和 slot/cache 的平行写入点。
2 participants
Summary
This PR fixes Transform subclass installation and replacement so an Entity always has exactly one Transform-compatible component in a stable component slot.
Problem
Adding UI components can auto-add
UITransformto an Entity that already containsTransformand aRenderer. The previous flow treated this as removing the only Transform before the replacement was visible, which incorrectly raisedShould remove Renderer before remove Transform.The old replacement flow could also append the new Transform and destroy the previous one afterward. When destruction was deferred, both Transform instances remained observable in the component list, and after destruction the Transform slot could move. That broke component ordering assumptions used by cloning and constructor-based component mapping.
Root cause
Component dependency removal checks did not understand that a replacement subtype can continue satisfying dependencies on the old Transform type. Entity construction also processed component constructors before guaranteeing the final Transform subtype, and Transform replacement did not preserve the original component-array slot.
Fix
Transformand all Transform subclasses consistently.CheckOnlyandAutoAdddependencies.Validation
pnpm -F @galacean/engine-core run b:typespnpm run b:moduleCI=true HEADLESS=true pnpm exec vitest --run tests/src/core tests/src/ui/UITransform.test.ts --reporter=dotgit diff --checkpassed.Regression coverage includes constructor ordering, duplicate Transform arguments, base/subclass replacement in both directions, deferred destruction, dependency rollback, replacement Transform
CheckOnlyandAutoAdddependencies, renderer-to-UITransform replacement, and clone component mapping.Summary by CodeRabbit
New Features
Transformwith a compatible transform type while preserving the transform’s slot/component position.Bug Fixes
Tests