refactor(clone): type-driven default clone mode for container elements#3040
refactor(clone): type-driven default clone mode for container elements#3040cptbtptpbcptdtptp wants to merge 55 commits into
Conversation
…ainer elements - Add `@defaultCloneMode(mode)` class decorator for declaring how instances of a type should be cloned when encountered as field values - Change container element cloning: array elements now use `undefined` as cloneMode so each element's type-level `_defaultCloneMode` is consulted. Unknown types (no _defaultCloneMode) default to Assignment (shared reference) instead of Deep, preventing crash on DOM/native objects. - Add `_defaultCloneMode` to ICustomClone interface - Mark RenderState system classes with @defaultCloneMode(Deep): DepthState, StencilState, RasterState, RenderTargetBlendState, BlendState, RenderState Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe clone system now uses class-level default clone modes, clone-map-based identity tracking, and updated resource/reference-count handling across core engine types. Particle, physics, UI, and post-process components were adjusted to the new clone behavior, and the clone-related tests and docs were expanded accordingly. ChangesClone pipeline and core remapping
Clone-mode updates across engine, particle, physics, and UI classes
Clone and ref-count test coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 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/clone/CloneManager.ts`:
- Around line 67-69: The defaultCloneMode decorator accepts any CloneMode value,
but the runtime logic at lines 141-149 only properly handles Deep and Shallow
modes, while Ignore is checked separately earlier at line 138. To fix this,
modify the defaultCloneMode function signature to restrict the mode parameter to
only accept CloneMode.Deep or CloneMode.Shallow (using a union type or
overload), preventing callers from incorrectly decorating with CloneMode.Ignore
which would not be honored at runtime.
🪄 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: 585fad6c-db8f-4c53-9512-ff3e147e9dde
📒 Files selected for processing (8)
packages/core/src/clone/CloneManager.tspackages/core/src/clone/ComponentCloner.tspackages/core/src/shader/state/BlendState.tspackages/core/src/shader/state/DepthState.tspackages/core/src/shader/state/RasterState.tspackages/core/src/shader/state/RenderState.tspackages/core/src/shader/state/RenderTargetBlendState.tspackages/core/src/shader/state/StencilState.ts
…+ type-driven deep clone Core changes: - Clone is now opt-out: all enumerable fields are cloned unless @ignoreClone - Default clone mode for unknown types is Assignment (shared reference) — safe for DOM elements, native handles, and any unrecognized constructor - Types that need independent copies declare @defaultCloneMode(CloneMode.Deep) - Identity-map based deep clone with cycle/shared-subgraph dedup - 3-stage lifecycle: Construct → Populate (copyFrom or for...in) → Finalize (_cloneTo) - Container elements (Array/Map/Set) go through the type-driven gate individually - @deepClone/@assignmentClone/@shallowClone kept as no-op for backward compat - @ignoreClone remains functional RenderState classes marked @defaultCloneMode(Deep): DepthState, StencilState, RasterState, RenderTargetBlendState, BlendState, RenderState Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eep) Particle system: ParticleGeneratorModule (+ all subclasses via inheritance), MainModule, BaseShape (+ all shape subclasses), ParticleGenerator, ParticleCompositeCurve, ParticleCurve, CurveKey, ParticleCompositeGradient, ParticleGradient, GradientColorKey, GradientAlphaKey, Burst Post-process: PostProcessEffect (+ BloomEffect, TonemappingEffect via inheritance), PostProcessEffectParameter (+ Float/Bool/Color/Vector/Enum/Texture) Physics: JointLimits, JointMotor Other: VirtualCamera, Skin Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mark Entity/Component with @defaultCloneMode(Remap) and split Entity.clone() into a register-then-copy two-pass: pre-register every source Entity/Component to its clone in the identity map across the whole subtree, so references nested in arrays/maps/objects are remapped through the clone gate, not just top-level fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Re-register @deepClone/@assignmentClone/@ignoreClone as field-level modes (highest priority). - Containers (Array/Map/Set/TypedArray/plain object) default to deep clone. - Type defaults: ReferResource -> Assignment, math types -> Deep, UpdateFlagManager -> Ignore. - Thread srcRoot/targetRoot through the gate so Signal._cloneTo can remap listeners. - Add CloneMode.Ignore; drop erroneous @deepClone on Joint limits/motor _updateFlagManager. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…efaults - Remove @deepClone/@assignmentClone/@shallowClone field decorators (171) across core & ui. - Fields resolve via container default deep + type-level @defaultCloneMode + Assignment fallback. - Mark ShaderData & Signal Deep; ShaderData adds _cloneTo delegating to cloneTo. - Clean up now-unused clone decorator imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-assignment # Conflicts: # packages/core/src/particle/ParticleGenerator.ts
…l run Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3040 +/- ##
===========================================
+ Coverage 79.37% 80.15% +0.77%
===========================================
Files 903 902 -1
Lines 100632 100803 +171
Branches 11260 11327 +67
===========================================
+ Hits 79879 80797 +918
+ Misses 20569 19828 -741
+ Partials 184 178 -6
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:
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…low) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…remap via identity map - Assignment is now a plain reference share: the gate never touches refCount. Ref-count ownership belongs to each class's own logic (_cloneTo hooks / setters), balanced by that class's destroy path. Fixes double-counting on Camera/Animator/AudioSource clones, Shader leak via _replacementShader, and permanent leaks for user-script-held resources. - Field decorators are highest priority at all depths: the ComponentCloner top-level remap special case is removed; Entity/Component remap unified through the pre-populated cloneMap (O(1)); CloneUtils path-walk deleted. @deepClone on an Entity/Component ref warns and falls back to remap. - Entity.clone() builds the tree and registers identity pairs in one walk; srcRoot/targetRoot threading removed — _cloneTo hooks receive cloneMap. - Container classification centralized (single _isContainer); DataView clones as bytes (was: crash via generic object branch); functions in containers are shared instead of dropped to undefined, while constructor-rebound top-level handlers keep the clone's own binding. - Register ColliderShape / ui Transition as @defaultCloneMode(Deep) so cloned colliders/interactives own independent instances; restore @ignoreClone + setter pattern for TextRenderer/Text fonts and MeshColliderShape mesh; SpriteTransition acquires state-sprite refs on clone. - shallowClone decorator warns on use; dead copyProperty/copyProperties removed; CloneMode exported; orphan imports cleaned. - Tests: decorator-priority semantics updated; new regressions for function fields, DataView, refCount balance (RT/controller/font/sprite-transition), collider-shape and transition instance independence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolve tests/src/core/physics/ColliderShape.test.ts: physics-lite describe removed upstream; move the shape-independence regression into the PhysX describe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed a follow-up that reworks two load-bearing decisions of this PR (design discussion happened offline): 1. The clone gate no longer touches refCount. 2. Remap is unified through the identity map and field decorators win at every depth. The Also in this push: Known pre-existing (not addressed): Tests: core 907+ / physics / ui+loader all green locally; new regressions cover function fields, DataView, refCount balance (RT / controller / font / sprite-transition / script-held textures), and instance independence for collider shapes & transitions. Branch merged with latest 🤖 Generated with Claude Code |
…eleases Component top-level fields sharing a registered ref-counted resource (ReferResource family) acquire one reference at the clone gate (+1, and -1 when replacing an owned preset); the owning component's destroy path releases it (implementation contract). Scripts have no per-field destroy logic, so gate acquisitions are recorded in a ledger and released in Script._onDestroy. Below the top level the gate never counts: container elements and plain-object fields are plain shares, and nested classes pair the acquisition themselves (SpriteTransition._cloneTo +1 paired with Transition.destroy -1, hoisted to the base class so future ReferResource-valued transitions inherit the release). Manual +1 compensations in Camera/Animator/AudioSource._cloneTo are removed (the gate acquisition replaces them); TextRenderer/Text._font return to gate accounting. The counted-resource test is explicit registration (@defaultCloneMode(Assignment)), excluding duck-typed counters like Shader. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…leak Add slot-ownership contract suites asserting the full lifecycle (baseline → clone +1 → setter churn → destroy release) for every counted component slot: Camera.renderTarget, Animator.controller, AudioSource.clip, SpriteRenderer/SpriteMask/ui Image sprite, MeshRenderer.mesh, MeshColliderShape.mesh, SpriteTransition states, and the Script ledger (reassignment still releases the original). Add ShaderData/Material cascade suites: setTexture swap/clear, doubly-referenced hosts (±refCount propagation), renderer clone balance, and instance-material lifecycles. The new suites exposed a pre-existing leak: destroying a ParticleRenderer never released the MeshShape's mesh (+1 in the mesh setter, no release path). Add BaseShape._destroy, called from EmissionModule._destroy, with MeshShape releasing through its setter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shallow is no longer a distinct mode; keeping the decorator silently behaving as deep clone hides the semantic change. Migrate with @deepClone (independent copy) or @assignmentClone (share the reference). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Public API (getFieldModes, deepCloneObject) first, @internal entries (_registerFieldMode, _cloneValue) after, private helpers last. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Script fields are user territory: the engine cannot pair a release for an automatic acquisition (users can't know which slots were acquired), so the gate does no counting for script slots at all — replacing the clone-acquisition ledger. Users who need gc protection manage counts themselves, matching the pre-2.0 semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cloning's only ref-count job is acquiring the count for the slot it copies. Releasing on destroy is the owning class's existing contract: engine components in their destroy paths, script authors in onDestroy. Remove the script exemption flag; Script.ts is untouched by this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_cloneValue is pure cloning again (4 params, Assignment = plain share). ComponentCloner — the only place slots own references — detects the share (result === source value, registered resource only; remapped/deep results never match) and acquires through CloneManager._acquireSlotOwnership. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Not a priority rule: deep-copying an Entity/Component is an unexecutable directive, so it recovers to remap with a warning. Every executable decorator still wins over the type default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keep the model summary and the ref-count contract in two short paragraphs; per-type behavior and deep-clone stages live on the methods themselves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…shared clone [vec3, vec3, vec3] must clone into one NEW Vector3 referenced three times (identity-map dedup), not three copies and not the source instance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-confirmed defects Findings from an exhaustive multi-agent review, each adversarially verified: - _deepClone: the copyFrom value-type branch ran before the container branches and matched any truthy member, so a plain object carrying a copyFrom data key crashed entity.clone(); it now dispatches after the container branches and only for class instances with a callable copyFrom - _deepClone: the typed-array/DataView reuse path lacked the reuse !== value guard, so a preset aliasing the source value (shared static default tables) silently returned the source buffer - physics: cloned MeshColliderShape was attached twice to the native actor (mesh-setter rebuild plus Collider._syncNative); attachment state is now tracked on ColliderShape and _addNativeShape skips re-attach - SkinnedMeshRenderer: the renderer-private joint texture copied by ShaderData's clone kept the source's one-shot destroy() refused and leaked the GC-ignored GPU texture; the clone now drops the entry - tests: cover all four fixes plus the unowned-preset diagnostic branch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/core/CloneUtils.test.ts (1)
1275-1292: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThis test pins a real production defect (negative refCount).
The assertion
expect(UnownedPresetScript.created[1].refCount).eq(-1)locks in the "unconditional -1" release on an unowned counted preset. Pinning is acceptable to lock current behavior, but a resource whoserefCountcan go negative is a genuine ownership bug in the clone gate (outside this file). Consider tracking it so the pin is intentionally revisited rather than silently ossified.Want me to open an issue to track fixing the unconditional release so this expectation can later flip to
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 `@tests/src/core/CloneUtils.test.ts` around lines 1275 - 1292, The test currently pins the negative refCount behavior for UnownedPresetScript by asserting the cloned preset’s refCount is -1, which is a real ownership defect; keep the assertion only as an intentional temporary pin, and add a clear tracking note near the UnownedPresetScript/CloneUtils test case so this expectation is explicitly revisited when the clone gate’s unconditional release is fixed to leave the unowned counted preset at 0.
🤖 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.
Nitpick comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 1275-1292: The test currently pins the negative refCount behavior
for UnownedPresetScript by asserting the cloned preset’s refCount is -1, which
is a real ownership defect; keep the assertion only as an intentional temporary
pin, and add a clear tracking note near the UnownedPresetScript/CloneUtils test
case so this expectation is explicitly revisited when the clone gate’s
unconditional release is fixed to leave the unowned counted preset at 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ff0c247-9129-437e-8b52-c36f3545b334
📒 Files selected for processing (10)
packages/core/src/Signal.tspackages/core/src/clone/CloneManager.tspackages/core/src/mesh/SkinnedMeshRenderer.tspackages/core/src/particle/modules/CustomDataModule.tspackages/core/src/physics/Collider.tspackages/core/src/physics/shape/ColliderShape.tspackages/core/src/physics/shape/MeshColliderShape.tstests/src/core/CloneTextureRefCount.test.tstests/src/core/CloneUtils.test.tstests/src/core/physics/MeshColliderShape.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/physics/shape/MeshColliderShape.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/physics/shape/ColliderShape.ts
- packages/core/src/mesh/SkinnedMeshRenderer.ts
- tests/src/core/physics/MeshColliderShape.test.ts
- packages/core/src/Signal.ts
- packages/core/src/clone/CloneManager.ts
…clone paths Review flagged these three subsystems as mechanism-rewritten with zero clone coverage: skin rootBone/bones remap with independent bind matrices, post-process effect/parameter independence with shared texture refs, and trail curve/gradient/textureScale independence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove the deleted shallowClone decorator from tables, samples and API links (with a migration callout), replace the old 'undecorated fields are shared' default with the type-driven resolution table, and document entity remapping and the asset ref-count contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/en/core/clone.mdx`:
- Around line 148-149: The cloning guidance currently claims the engine
automatically increments ref-counted assets for any component top-level field,
but the revised ownership model is per-class and not a generic clone-wide bump.
Update the “Cloning and Asset Reference Counting” text in clone.mdx to reflect
the decentralized contract used by classes like PostProcess/BloomEffect and
their fields such as dirtTexture, and avoid implying automatic engine-wide
refCount increments unless a specific class explicitly does so in its own
clone/setter/destroy logic.
- Around line 138-139: The typo in the example comments uses “bacause” instead
of “because”; update the inline explanatory text in the clone documentation
example so the wording is correct and consistent. Fix the two comment lines
associated with the script.c and script.d examples, keeping the rest of the
example unchanged.
In `@docs/zh/core/clone.mdx`:
- Around line 149-150: Update the clone and asset reference-counting description
in the documentation section identified by clone-related terms such as
“克隆与资产引用计数” and “ReferResource” to match the corrected English behavior: cloning
should not imply the generic clone flow automatically increments ref counts for
shared resources. Revise the text to say that reference handling is owned by
each class’s own clone/setter/destroy logic, and keep the guidance for built-in
components versus custom scripts aligned with that behavior.
- Around line 139-140: Fix the same typo in the clone example comments by
changing “bacause” to the correct spelling in the lines referencing script.c[0]
and script.d[0]. Update the explanatory comments only, keeping the meaning and
examples intact.
🪄 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: 3eedbefe-38b9-4666-92bf-9afc2307d484
📒 Files selected for processing (7)
docs/en/core/clone.mdxdocs/en/how-to-contribute.mdxdocs/zh/core/clone.mdxdocs/zh/how-to-contribute.mdxtests/src/core/SkinnedMeshRenderer.test.tstests/src/core/Trail.test.tstests/src/core/postProcess/PostProcess.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/en/how-to-contribute.mdx
- docs/zh/how-to-contribute.mdx
| console.log(script.c[0]); // output is (2,2,2). bacause assignmentClone let c use the same array reference with cloneScript's c. | ||
| console.log(script.d[0]); // output is (1,1,1). bacause deepClone let d[0] use the different reference with cloneScript's d[0]. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: "bacause" → "because".
✏️ Proposed fix
-console.log(script.c[0]); // output is (2,2,2). bacause assignmentClone let c use the same array reference with cloneScript's c.
-console.log(script.d[0]); // output is (1,1,1). bacause deepClone let d[0] use the different reference with cloneScript's d[0].
+console.log(script.c[0]); // output is (2,2,2). because assignmentClone let c use the same array reference with cloneScript's c.
+console.log(script.d[0]); // output is (1,1,1). because deepClone let d[0] use the different reference with cloneScript's d[0].📝 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.
| console.log(script.c[0]); // output is (2,2,2). bacause assignmentClone let c use the same array reference with cloneScript's c. | |
| console.log(script.d[0]); // output is (1,1,1). bacause deepClone let d[0] use the different reference with cloneScript's d[0]. | |
| console.log(script.c[0]); // output is (2,2,2). because assignmentClone let c use the same array reference with cloneScript's c. | |
| console.log(script.d[0]); // output is (1,1,1). because deepClone let d[0] use the different reference with cloneScript's d[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 `@docs/en/core/clone.mdx` around lines 138 - 139, The typo in the example
comments uses “bacause” instead of “because”; update the inline explanatory text
in the clone documentation example so the wording is correct and consistent. Fix
the two comment lines associated with the script.c and script.d examples,
keeping the rest of the example unchanged.
| ## 克隆与资产引用计数 | ||
| 克隆时,如果组件的顶层字段共享了一个引用计数资产(`ReferResource`,如 `Texture`、`Mesh`、`Material`),引擎会自动为克隆体增加一次引用计数。持有该字段的类负责在销毁时释放这次引用 —— 内置组件已由引擎处理;自定义脚本请在 `onDestroy` 中释放自己持有的资源。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Same ref-count contradiction as the English doc.
This section claims "克隆时,如果组件的顶层字段共享了一个引用计数资产(ReferResource,如 Texture、Mesh、Material),引擎会自动为克隆体增加一次引用计数。" which conflicts with the PR's stated decision that ref counting is no longer handled by the generic clone mechanism but by each class's own clone/setter/destroy logic — as evidenced by PostProcess.test.ts showing refCount unchanged after cloning a shared texture. Please sync this translation with the corrected English content once fixed.
🤖 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 `@docs/zh/core/clone.mdx` around lines 149 - 150, Update the clone and asset
reference-counting description in the documentation section identified by
clone-related terms such as “克隆与资产引用计数” and “ReferResource” to match the
corrected English behavior: cloning should not imply the generic clone flow
automatically increments ref counts for shared resources. Revise the text to say
that reference handling is owned by each class’s own clone/setter/destroy logic,
and keep the guidance for built-in components versus custom scripts aligned with
that behavior.
…cate walks Simplifications from a five-lens review, each judged for semantic equivalence and net win: - ComponentCloner no longer guards the settlement call: the cloned===sourceValue reasoning moves into _transferSlotOwnership, which now also releases an owned counted preset displaced by a deep-cloned value (closing the last accounting gap) - _deepClone's object branch delegates its field walk to deepCloneObject; the reuse predicate is hoisted into one 'reusable' - the function-branch nested ternary flattens to a single condition - attach/detach primitives move next to their _isShapeAttached flag on ColliderShape; Collider and MeshColliderShape both delegate Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…utions CloneUtils.test.ts was the repo's last reference to the deleted class. Three CloneTextureRefCount comments still attributed gate-acquired references to hooks this PR removed; also cover displaced-preset release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llocation The forEach arrow closures were the only unconditional intermediate allocations on the clone path besides the clone's own output; for-of iterators are erasable by escape analysis, closures are not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查 @ 2a919171b(第六轮)— delta = 1 commit(pure perf, behavior-equivalent)
总结
自上轮 23f0459d6 起 tip 线性推进 1 个 commit(compare 返回 ahead_by:1 / behind_by:0,status ahead,无 force-push):2a919171b perf(clone): iterate Map/Set with for-of to avoid per-clone closure allocation —— 只碰 CloneManager.ts(+10/-5),把 _deepClone 的 Map/Set 两个分支从 forEach(arrow) 改成 for-of。逐链核对为行为逐字节等价的纯 GC 优化(消除每次克隆 Map/Set 时那唯一一个无条件分配的闭包),无新语义。CI 全绿(build×3 macos/ubuntu/windows / e2e×4 视觉回归 / codecov patch+project / lint)。无 P0/P1/P2,可合并。
逐链核对(delta 本身,非信 commit message)
1. Map 分支(_deepClone:280-290)
- 旧
value.forEach((v, key) => dst.set(_cloneValue(key), _cloneValue(v)));forEach回调签名(value, key),故v=值、key=键。 - 新
for (const entry of value)走Map[Symbol.iterator](=entries()),逐项entry = [key, value],dst.set(_cloneValue(entry[0]), _cloneValue(entry[1]))=dst.set(_cloneValue(key), _cloneValue(value))。 - 迭代序:
forEach与for-of对 Map 都走插入序,逐字节相同。键值映射:entry[0]=key /entry[1]=value,无键值互换。参数求值序:JS 从左到右求值dst.set(A,B),两版都先克隆键(左)后克隆值(右)—— 故 key===value(同对象同时作键与值)时,键先克隆入cloneMap、值查表命中同一 clone,别名保留一致。三者全等价。
2. Set 分支(_deepClone:293-298)
- 旧
value.forEach((v) => dst.add(_cloneValue(v)))→ 新for (const v of value) dst.add(_cloneValue(v))。Set[Symbol.iterator]同插入序、单值,逐字节等价。
3. 环 / 共享子图
- 两分支的
cloneMap.set(value, dst)均在 loop 之前(未变),自引用 Map/Set(m.set(m,m)/s.add(s))经 identity map 短路,行为不变。
4. 性能理由核实(非信 commit body)
- commit body 称「
forEacharrow closures were the only unconditional intermediate allocations on the clone path」。核对文件全部forEach:唯一其它命中在getFieldModes:88(own.forEach),但该函数按 type 记忆化(_fieldModeMap.get(type)命中即返回缓存),forEach 只在某 type 首次克隆时跑、后续走缓存,属摊销分配非每克隆分配。故 body 「on the clone path(每克隆路径)」的限定准确——Map/Set 那两个forEach才是每次深拷 Map/Set 值都新建一个闭包的唯一无条件分配点。for-of迭代器可被逃逸分析消除、闭包不可。理由属实。
已核对为「已解决 / 不适用 / 不重提」
- 历史 P3 #2~#6、
_transferSlotOwnership无条件化、copyFrom 重排、TypedArrayreuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理_isShapeAttached上移、归属注释校准、CodeRabbit「ref-count claim contradicts architecture」误报、ParticleCurvekey-listener /ColliderShapePhysicsMaterial orphan(dev/2.0 既有)——均已闭环或明确 out-of-scope,本 delta 未触碰,不重提。 - 上轮 P3
bacause拼写(clone.mdx)—— 本 delta 未触碰该文件,落在未改动行,且已提过一次,不再重提。 - 注释合规:新增单行
// Map — \for-of` instead of `forEach`: no per-clone closure allocation.是完整句、带句号,跟随本文件既有惯例(CloneManager.ts全文整句//` 均带句号::131/:135/:138/:187/:238/:269/:303/:306/:316),跟随项目惯例不作偏离。且非复述代码——它解释的是 为什么(消除闭包分配)而非 做什么,合规。
简化建议
无。delta 是一次干净的等价 perf 收口(两分支 forEach→for-of,行为逐字节不变),把 type-driven 重构的最后一处每克隆闭包分配也消掉了。可合并。
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查 @ 6d724c798(第七轮)— delta = 2 commits(均 docs-only,注释单行改动)
总结
自上轮 2a919171b 起 tip 线性推进 2 个 commit(compare 返回 ahead_by:2 / behind_by:0,status ahead,无 force-push),全部为注释单行清理,零产物逻辑改动:
c13dd08e5docs(clone): drop a change-narrating comment on the Map branch—CloneManager.ts(+1/-1)6d724c798docs(clone): drop tense-relative wording from a lifecycle test comment—CloneTextureRefCount.test.ts(+1/-1)
两处均为严格更好的注释卫生收口(去掉描述「diff 本身/时态相对」的措辞,回到描述「当前不变式」),无任何代码路径改动。CI 全绿(build×3 macos/ubuntu/windows / e2e×4 视觉回归 / codecov patch+project / lint)。无 P0/P1/P2/P3,可合并。
逐条核对(delta 本身,非信 commit message)
1. CloneManager.ts:278 — _deepClone Map 分支标签
- 旧:
// Map — \for-of` instead of `forEach`: no per-clone closure allocation.`(我第六轮批准时新增的) - 新:
// Map - 删掉的部分是叙述「这次改动」的措辞(forEach → for-of 的对比 + 为何改)——它描述的是 diff 相较旧实现的差异,而非当前代码行为。一旦合并,代码里不再有
forEach版本,该注释就变成在描述历史(会成为 stale change-narration,属feedback_aspirational_comments_lie同源的时间维度问题)。删后// Map与两个兄弟标签// Array(:269)/// Set(:293)对齐,成为一组极简 section label。行为零改动(for-of 迭代逻辑第六轮已逐字节核过为等价)。 - 注释合规:
// Map单行//无句号,跟随兄弟 bare-label 惯例(// Set同样无句号;// Array — fresh instance…带句号是因其为完整句而非纯标签,两种形态在本文件并存且各自自洽),无偏离。
2. CloneTextureRefCount.test.ts:44 — joint texture 泄漏回归测试的生命周期注释
- 旧:
// With no stray reference, the source's one-shot destroy() in _onDestroy now succeeds. - 新:
// With no stray reference, the source's one-shot destroy() in _onDestroy succeeds (refCount is zero). - 删掉时态相对词
now(隐含「相较修复前」的 before/after 叙述),补上具体条件(refCount is zero)——与紧邻断言expect(jointTexture.refCount).eq(0)精确吻合。新措辞描述的是当前不变式(无杂散引用 ⇒ refCount 归零 ⇒ one-shot destroy 生效),准确、非 aspirational。 - 注释合规:单行
//带句号,跟随本文件既有惯例(// Simulate the state _update builds…/// The clone must not hold the source's private texture…均为整句带句号),无偏离。
已核对为「已解决 / 不适用 / 不重提」
- 历史 P3 #2~#6、
_transferSlotOwnership无条件化、copyFrom 重排、TypedArrayreuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理_isShapeAttached上移、归属注释校准、Map/Set for-of perf、CodeRabbit「ref-count claim contradicts architecture」误报、ParticleCurvekey-listener /ColliderShapePhysicsMaterial orphan(dev/2.0 既有)——均已闭环或明确 out-of-scope,本 delta 未触碰,不重提。 - 上轮 P3
bacause拼写(clone.mdx)—— 本 delta 未触碰该文件,落在未改动行,且已提过一次,不再重提。 - 自上轮以来无新的 PR 评论 / inline comment / 作者回复需要 reconcile。
简化建议
无。delta 是一次干净的注释卫生收口——把两处「叙述 diff / 时态相对」的注释换成「描述当前不变式」的措辞,方向正确,把 type-driven 重构的注释残留也清干净了。可合并。
cptbtptpbcptdtptp
left a comment
There was a problem hiding this comment.
发现一个可复现的 container clone 回归,建议在合入前处理一下。
CloneManager._deepClone 在没有可复用目标对象时会走 new ctor() 创建 Deep 类型实例:
const dst = reusable ?? (ctor ? new ctor() : Object.create(null));这对一些新注册为 @defaultCloneMode(CloneMode.Deep)、但构造函数需要参数的类型不成立。例子是 ParticleCompositeGradient:它现在是默认 Deep,但构造函数会直接访问 constantOrGradient.constructor。当它作为数组/Map/plain object 里的元素被 clone 时,容器元素没有 reuse,clone gate 会无参构造它并抛错。
我在 PR tip 上用下面这个最小用例复现了:
CloneManager._cloneValue(
[new ParticleCompositeGradient(new Color())],
undefined,
new Map()
);结果:
TypeError: Cannot read properties of undefined (reading 'constructor')
建议修复方向:要么让这些 Deep 类型支持安全的无参构造,要么给 clone 系统提供工厂/自定义构造路径;同时补一个“不带 reuse 的容器元素 clone”回归测试,覆盖数组/Map/plain object 中的 type-default Deep 值。
The clone gate constructs container elements without arguments and then populates every field, but the constructor dereferenced its first parameter unconditionally — a gradient held in an array/map/plain object crashed entity.clone(). The field defaults already form a valid constant-mode state, so an argument-less call now simply keeps them. Audited every Deep-registered type for the same shape: this was the only bare-construction crash. Covers container-element cloning and the previously untested emission-bursts engine path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed types Cocos-style mechanism guard instead of per-class patching: the gate creates preset-less instances with new Type(), so Deep registration implies argument-less constructibility. A guard test enumerates every exported Deep type across core/math/ui and bare-constructs it; the explicit exemption list names the engine-bound structural types the gate only ever clones against a same-type preset, with reasons. The contract is stated on the defaultCloneMode decorator doc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查 @ 16d95d6f3(第八轮)— delta = 2 commits(latent bug fix + mechanism guard test)
总结
自上轮 6d724c798 起 tip 线性推进 2 个 commit(compare 返回 ahead_by:2 / behind_by:0,status ahead,无 force-push):
828008f9bfix(particle): allow bare construction of ParticleCompositeGradient—ParticleCompositeGradient.ts(+9/-1)+ 测试(+42)16d95d6f3test(clone): enforce the bare-construction contract for Deep-registered types—CloneManager.ts(+2 纯文档)+ 测试 guard(+47)
commit 1 是一处真 latent bug 修复(容器元素 bare-construction 崩溃),配两条反向可证伪测试;commit 2 是一处机制级 guard 测试,把「Deep 注册 ⇒ 必须无参可构造」的契约固化成对全仓导出 Deep 类型的可执行枚举,exemption 列表逐条独立核实为精确最小且合法。CI 全绿(build×3 macos/ubuntu/windows / e2e×4 视觉回归 / codecov patch+project / lint)。无 P0/P1/P2/P3,可合并。
逐链核对(delta 本身,非信 commit message)
1. ParticleCompositeGradient bare-construction 崩溃修复(真 bug)
- 根因:gate 的
_deepCloneobject 分支const dst = reusable ?? (ctor ? new ctor() : ...)——当值是容器元素(array/map/set/plain-object)时,容器分支以_cloneValue(element, undefined, ...)传入reuse=undefined→reusable=null→ 走new ctor()裸构造。旧ParticleCompositeGradient构造函数constructor(constantOrGradient: Color | ParticleGradient, ...)首参必填,且无条件constantOrGradient.constructor === Color解引用 → 裸构造时undefined.constructorTypeError。 - 触发面真实存在:
entity.clone()深拷任何把ParticleCompositeGradient放进数组/Map/plain-object 的 slot 时崩溃。 - 修法正确:新增
constructor()重载 + 实现里if (!constantOrGradient) return。核对 falsy 分支只对undefined/null触发(对象恒 truthy,new Color()作参不误伤);早返后字段默认值mode=Constant/constantMin/Max=new Color()/gradientMin/Max=new ParticleGradient()构成合法 constant-mode 态,evaluate走Constant读this.constant(=constantMax) 不崩。而对 clone 路径而言这个 bare 态是瞬态——gate 随后deepCloneObject逐字段用 source 覆盖。行为对所有既有参数化重载逐字节不变(早返只拦无参)。 - 反向可证伪测试:①
clones gradients and curves held in arrays / maps without a reusable preset——config = { gradients: [gradient], curves: Map([["a", curve]]) }走容器元素路径,clone 时对数组元素 bare-construct。旧代码new ParticleCompositeGradient()→undefined.constructor崩溃 → 测试红;新代码 PASS。②clones the emission bursts array through the engine path——真引擎Burst(持ParticleCompositeCurve count)经emission.bursts数组 clone。两测均走公开 API(addComponent/clone/getComponent)。 - 顺带核实:同族
ParticleCompositeCurve无此崩溃——其构造函数typeof constantOrCurve === "number"对undefined为 false → 落 else 分支this.curve = undefined(curveMax=undefined,mode=Curve),产出怪态但不抛异常,且 clone 后被逐字段覆盖,故容器测试对 curve 也 PASS,无需修。
2. Bare-construction guard 测试(机制级护栏)— exemption 列表逐条独立核实
- guard 遍历
EngineCore/EngineMath/EngineUI三个 namespace 全部导出,对prototype._defaultCloneMode === Deep且非 exempt 的类型跑new exported(),断言零抛异常。这是把「Deep 注册 ⇒ gate 会 bare-construct ⇒ 必须无参可构造」的契约固化成可执行枚举——比 commit body 的「audited every Deep-registered type」自证更强。 - exemption 两类逐条核实合法:
- 物理 shapes(
ColliderShape+ Box/Sphere/Capsule/Plane/Mesh 5 子类):核实Collider._shapes: ColliderShape[]在 PR tip 是无装饰器数组字段(Collider.ts:26,两个@ignoreClone落在_index/_nativeCollider)→ gate 深拷该数组 → 每个 shape 确实 bare-construct(container 路径)。exemption 注释「bare-constructible in a real runtime, just not in this physics-less test engine」诚实且准确:ColliderShape构造函数this._material = new PhysicsMaterial()(ColliderShape.ts:130)需初始化的物理后端,测试引擎(WebGLEngine 无物理)才抛,真实 runtime 正常构造。子类的_isShapeAttached双挂守卫(第四轮已核)配套这条 container-clone 路径。 - 模块类(
MainModule/VelocityOverLifetimeModule/SizeOverLifetimeModule/LimitVelocityOverLifetimeModule/NoiseModule):核实均有自己的constructor(generator)且构造期经 curve setter →ParticleGeneratorModule._onCompositeCurveChange→this._generator._renderer→ 裸构造时_generator=undefined崩溃。它们是ParticleGenerator的具名字段(new XModule(this)),clone 时 gate 恒对同型 preset 复用(reusable非空、走 object 分支不new ctor()),永不 bare——exemption 注释「always clones against the component's same-type constructor preset, never bare」准确。
- 物理 shapes(
- exemption 列表精确最小:核实未被 exempt 但同样 Deep 注册(继承自
@defaultCloneMode(Deep)的ParticleGeneratorModule)且导出的模块——ColorOverLifetimeModule/EmissionModule/RotationOverLifetimeModule/SubEmittersModule/CustomDataModule(无自有构造函数,继承基类constructor(generator){ this._generator=generator }只裸赋 undefined 不解引用)+TextureSheetAnimationModule(自有构造函数只碰 field-initialized 的_tiling不碰_generator)——全部 bare-construct 不抛异常,故正确地不在 exemption 里,guard 对它们跑new X()通过。这是 exemption 精确性的正面印证:列表恰好只名有「构造期解引用_generator/ 需物理后端」的类型,一个不多一个不少,由 CI 绿的 guard 测试实证。 - coverage gap(非缺陷,仅记录):
ForceOverLifetimeModule有自有构造函数经 curve setter(会解引用_generator),但未从 core 导出(不在particle/index.tsbarrel)→ guard 的Object.entries看不到 → 既不被测也不需 exempt。它是ParticleGenerator具名字段永不 bare,实践零风险;只是 guard 的枚举面天然局限于「已导出」。不作为问题(guard 覆盖导出 surface 已是最强可行契约)。
已核对为「已解决 / 不适用 / 不重提」
- 历史 P3 #2~#6、
_transferSlotOwnership无条件化、copyFrom 重排+硬化、TypedArrayreuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理_isShapeAttached双挂上移、归属注释校准、Map/Set for-of perf、Map/test 注释卫生、CodeRabbit「ref-count claim contradicts architecture」误报、ParticleCurvekey-listener /ColliderShapePhysicsMaterial orphan(dev/2.0 既有)——均已闭环或明确 out-of-scope,本 delta 未触碰,不重提。 - 上轮 P3
bacause拼写(clone.mdx)—— 本 delta 未触碰该文件,落在未改动行,已提过一次,不再重提。 - 自上轮以来无新的 PR 评论 / inline comment / 作者回复需要 reconcile。
注释合规
ParticleCompositeGradient.ts构造函数 JSDoc/** Create a particle gradient in constant mode with the default color. */——多行块、带句号,跟随同文件兄弟重载 JSDoc 惯例。- 构造函数体内 inline
// No argument: ... populates every field).——整句带句号,跟随同文件_deepClone/本文件既有整句//惯例。 CloneManager.tsdecorator JSDoc 追加句「A type registeredDeepmust construct without arguments...」——落在多行 JSDoc 块内、整块以@param收尾合规。- guard 测试的多行
//说明块(含 exemption 分类注释)——整句带句号,跟随本文件既有整句//惯例(第七轮已确立)。 - 均无偏离。
简化建议
无。delta 是一次高质量收尾:commit 1 修掉 type-driven 重构暴露的最后一处容器元素 bare-construction 崩溃(配两条反向测试),commit 2 用一条机制级 guard 把「Deep ⇒ 无参可构造」的隐性契约变成 CI 可执行的护栏、并把契约写进 decorator 文档——这正是「大道至简」的收口:不再逐类打补丁,而是让机制自证。可合并。
…nnot construct A user type registered Deep with a required-argument constructor used to surface the constructor's raw TypeError from inside the gate; the failure now states the contract (argument-less construction) and the type name, with the original error attached as the cause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查 @ f58d380dc(第九轮)— delta = 1 commit(diagnostic error-wrapping + reverse-falsifiable test)
总结
自上轮 16d95d6f3 起 tip 线性推进 1 个 commit(compare 返回 ahead_by:1 / behind_by:0,status ahead,无 force-push):f58d380dc feat(clone): name the bare-construction contract when a deep clone cannot construct —— 只碰 2 文件:CloneManager.ts(+18/-2)+ CloneManager.test.ts(+21)。把 gate 里两处 new ctor() 裸构造收口进 _bareConstruct(ctor) funnel,用 try/catch 重抛带上下文的错误(命名契约 + 类型名 + 原始错误),配一条反向可证伪测试。CI 全绿(build×3 macos/ubuntu/windows / e2e×4 视觉回归 / codecov patch+project / lint)。无 P0/P1/P2/P3,可合并。
逐链核对(delta 本身,非信 commit message)
1. _bareConstruct funnel 收口正确
- 两处
new ctor()裸构造点——copyFrom 分支(CloneManager.ts:311)+ object 分支(:320)——现均改调CloneManager._bareConstruct(ctor)。grep 全文件确认这是仅有的两个 ctor 驱动裸构造点:Array/Map/Set 分支用的是 typed generic(new Map<any,any>()/new Set<any>()/new Array(len),:273/283/296),非ctor驱动,正确地不收口进来(它们无用户 ctor、永不抛契约错误);Object.create(null)分支(null-proto,:320的 else)也不经_bareConstruct(无 ctor 可抛)。收口精确、无遗漏、无误伤。 reusable ?? _bareConstruct(ctor)的短路语义与旧reusable ?? new ctor()逐字节等价:命中 preset 复用时(reusable非空)根本不进_bareConstruct,只有真正裸构造那一格才走 try/catch。既有所有「gate 对同型 preset 复用」路径(模块类具名字段、物理 shapes 容器复用)零行为改动。
2. try/catch 是合法的「边界层重抛带上下文」,非热路径吞异常反模式
- 落点是 clone 路径(
entity.clone()),属实例化/编辑期操作,非每帧 render/update 热路径——不触发「渲染热路径防御性 try/catch 该删」判据。 - catch 后重新
throw(非静默 swallow / finally 善后),错误照常传播、现场不被擦掉;只是把「用户 Deep 类型带必填参数构造函数」这类契约违规从 gate 内部冒出来的裸TypeError(如Cannot read properties of undefined)换成命名契约的Error(含bare-construct "<TypeName>"+ 契约说明 +Cause: <原错误>)。这正是第八轮 guard 测试固化的「Deep ⇒ 必须无参可构造」隐性契约的运行时诊断补全:guard 覆盖已导出类型,_bareConstruct覆盖运行时任何未被 guard 枚举到的用户类型(如ForceOverLifetimeModule那类未导出的 coverage gap,或用户自定义 Deep 类型)。符合 runtime-trust:不掩盖 bug,反而把 bug 暴露得更可诊断。
3. 反向可证伪测试
a Deep type that cannot construct bare fails with the contract named:ParamDeepConfig(@defaultCloneMode(Deep)+ 必填参构造函数解引用source.id)放进config.items数组(容器元素路径 →reuse=undefined→ 裸构造),parent.clone()触发。- 旧代码
new ctor()直接抛裸TypeError: Cannot read properties of undefined (reading 'id')→ 不匹配/bare-construct "ParamDeepConfig"/→toThrowErrorFAIL; - 新代码抛
... failed to bare-construct "ParamDeepConfig" ...→ 匹配 → PASS。 - 走公开 API(
addComponent/clone/destroy),真反向证伪。
- 旧代码
ParamDeepConfig不污染上轮的「every exported Deep-registered type constructs bare」guard:核实它是本地测试类、不从任何包 namespace 导出(Object.entries(EngineCore/Math/UI)看不到它),故 guard 不会把它跑成new ParamDeepConfig()而误红,也无需加进 exempt 列表。两条测试互不干扰,exemption 列表仍精确最小。
已核对为「已解决 / 不适用 / 不重提」
- 历史全部闭环项(type-driven gate、
_transferSlotOwnership无条件化、copyFrom 重排+硬化、TypedArrayreuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理_isShapeAttached双挂上移、Map/Set for-of perf、注释卫生、上轮 bare-construction 崩溃修复 + guard 测试、ParticleCurvekey-listener /ColliderShapePhysicsMaterial orphan 属 dev/2.0 既有 out-of-scope)——本 delta 未触碰,不重提。 - 上轮 P3
bacause拼写(clone.mdx)—— 本 delta 未触碰该文件,落在未改动行,已提过一次,不再重提。 - 自上轮
16d95d6f3(13:20)以来无新的 PR 评论 / inline comment / 作者回复需要 reconcile(作者 12:01 的回复在我上轮之前已 reconcile)。
注释合规
_bareConstruct的 JSDoc/** ... instead of surfacing the constructor's raw error. */——多行块、带句号、准确描述当前行为(命名契约取代裸错误),非 aspirational(与代码逐行一致)。- 测试类
ParamDeepConfig的单行/** User type registered Deep ... (contract violation). */——跟随同文件兄弟测试类惯例(CopyFromDataScript:240/SharedDefaultTableScript:245同为单行/** ... */),无偏离。 - 均无偏离。
(注:commit body 措辞「attached as the cause」与代码 Cause: ${e} 字符串插值略有出入——代码是把原错误消息插进字符串而非用结构化 Error.cause 选项。但 JSDoc 与代码一致、原错误信息完整保留、全仓无 .cause 消费者,属 commit body 用词的微小不精确,非代码/注释问题,不作 finding。)
简化建议
无。delta 是一次干净的诊断收口:把两处裸构造统一进 _bareConstruct funnel,用重抛把上轮 guard 固化的隐性契约在运行时也命名出来——不掩盖异常、只让它更可诊断,配反向测试守回归。方向正确,可合并。
…mantics The exemption note overstated the preset guarantee: a user can pull a host-bound module off a live generator and put it in a clonable container, where the gate bare-constructs and fails. That rejection is the intended behavior (a host-bound structure cannot exist without its host; sharing must be declared with @assignmentClone) — state it honestly in the exemption list and lock it with a test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…order The previous pin asserted rejection unconditionally, but when the host's engine slot clones before the user container the identity map already holds the module and the container reference remaps onto the clone — the better outcome. Cover both orders: remap when the host precedes, named-contract rejection when it does not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查 @ 9db6c66ced(第十一轮)— delta = 1 commit(test-only:按组件顺序拆分 host-bound 容器行为,修复我上轮误批的坏测试)
总结
自上轮 843ab780e 起 tip 线性推进 1 个 commit(compare 返回 ahead_by:1 / behind_by:0,status ahead,parent=843ab780e,无 force-push):9db6c66ced test(clone): pin both host-bound-in-container behaviors by component order —— 只碰 1 文件 tests/src/core/CloneManager.test.ts(+21/-3)。把上轮那条单测拆成两条按组件添加顺序正确断言的测试。本轮 CI 全绿(含 codecov job 1570/1570 全过),实证修复成功。无 P0/P1/P2/P3,可合并。
⚠️ 更正上一轮的事实错误
我上轮(第十轮 @ 843ab780e)把 codecov 挂红判为「test-only commit 无新增覆盖行 artifact,非真回归」——这是错的。经核 843ab780e 的 CI(run 28707964113):codecov job 的 Test 步骤 conclusion=failure,日志显示单测 a host-bound structural instance in a user container fails with the named error 真 FAIL:
× Clone remap > … > a host-bound structural instance in a user container fails with the named error
→ expected [Function] to throw an error
Test Files 1 failed | 117 passed (118)
Tests 1 failed | 1569 passed (1570)
那条测试 renderer 先加、断言 parent.clone() 会 throw,但实际 renderer-first 会 remap 而非 throw,断言反了。engine CI 的单测跑在名为 codecov 的 job 里(ci.yml 的 codecov job 有 Test=npm run coverage=vitest 步骤 + Upload coverage 步骤),不是 coverage 阈值 artifact。我把它当 artifact 放过是判据过度简化。本轮 commit 正是修复这个断言反了的坏测试。
逐链核对(delta 本身,非信 commit message)
机制(决定 remap-vs-throw 的根因)——独立 trace Entity.clone 链:
Entity._createCloneEntity建树时cloneMap.set(component_i, cloneComponent_i)只注册 entity 直属 component(+entity 本身)。renderer.generator.main是ParticleRenderer.generator内的嵌套MainModule(非 component),故建树期不进 cloneMap。main进 cloneMap 的时机 =_parseCloneEntity→ComponentCloner.cloneComponent(ParticleRenderer, …)深拷 generator 树时(gate_deepClone的cloneMap.set(value, dst),CloneManager.ts:321;main是ParticleGenerator具名字段 ⇒reusable=clone 自己的构造期mainpreset ⇒ 复用它并注册)。- component 迭代序 =
_components数组序 = addComponent 序(_parseCloneEntity:490-492按src._components顺序逐个cloneComponent)。所以「CopyFromDataScript引用ParticleRenderer的嵌套main」能否 remap,取决于谁先 addComponent。
测 1 a host-bound instance in a container remaps when its engine slot clones first(renderer 先加)— 真反向可证伪
renderer先加 ⇒_components=[ParticleRenderer, CopyFromDataScript]⇒ParticleRenderer先cloneComponent⇒generator.main深拷并注册进 cloneMap ⇒ 随后CopyFromDataScript.config.modules[0](=源main)作容器元素走_deepClone⇒:237 cloneMap.get(value)命中existing⇒ remap(dedup)不裸构造。- 断言
clonedModule === cloned.getComponent(ParticleRenderer).generator.main(同一复用 preset)+!== renderer.generator.main(是 clone 自己的)——逐链核对为真。旧代码/坏断言:此路径 remap 不 throw ⇒ 上轮toThrowErrorFAIL;新断言:直接断 remap 结果 ⇒ PASS。
测 2 a host-bound instance in a container fails with the named error when no host precedes(script 先加)— 真反向可证伪
script先加 ⇒_components=[CopyFromDataScript, ParticleRenderer]⇒CopyFromDataScript先cloneComponent⇒config.modules[0](=源main)此时 cloneMap miss ⇒ 落 object 分支、容器元素reuse=undefined⇒reusable=null⇒_bareConstruct(MainModule)⇒new MainModule()首个 curve setter 解引用_generator=undefined抛 ⇒ 包成bare-construct "MainModule"。- 断言
toThrowError(/bare-construct "MainModule"/)——匹配。走公开 API(addComponent/clone/destroy)。
两测逻辑自洽且穷尽了顺序两态:同一份 gate 代码,仅 addComponent 顺序不同就分叉为 remap / throw 两条路径,两测各钉一条,并在注释里显式点明「engine slot clones first ⇒ dedup」/「no host precedes ⇒ bare-construct rejected」的因果。这正是上轮单测漏掉的一半(它只写了 renderer-first 却断言了 script-first 才有的 throw)。上轮 guard 测试(every exported Deep type constructs bare)的 exemption 块与 ParamDeepConfig 反向测试均未被本 delta 触碰,仍精确最小。
已核对为「已解决 / 不适用 / 不重提」
- 历史全部闭环项(type-driven gate、
_transferSlotOwnership、copyFrom 重排+硬化、TypedArrayreuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理_isShapeAttached双挂上移、Map/Set for-of perf、注释卫生、bare-construction 崩溃修复 + guard 测试、_bareConstruct诊断 funnel + 反向测试、ParticleCurvekey-listener /ColliderShapePhysicsMaterial orphan 属 dev/2.0 既有 out-of-scope)——本 delta 未触碰,不重提。 - P3
bacause拼写(clone.mdx)——本 delta 未触碰该文件,落在未改动行,已提过一次,不再重提。 - 自上轮
843ab780e(13:43)以来无新的 PR 评论 / inline comment / 作者回复需要 reconcile。
注释合规
- 两条新测各自的多行
//说明块(// Renderer added first: …/// Script added first: …)——整句带句号,跟随本文件既有整句//惯例,无偏离。 - 均无偏离。
简化建议
无。delta 是一次干净且必要的修正:把上轮那条断言与运行时行为矛盾(因此 CI 实际在挂)的单测,拆成两条按组件顺序正确断言、覆盖 remap 与 throw 两态的反向可证伪测试。方向正确,把 clone 顺序依赖这个真实语义钉成了回归网。可合并。
cptbtptpbcptdtptp
left a comment
There was a problem hiding this comment.
最后一轮看 API / 注释 / 设计自洽性,整体方向我认可:类型驱动默认行为、容器默认结构 deep、未知对象 assignment、Entity/Component 统一走 identity-map remap,ref-count 从 clone gate 移到 component slot settlement,也比递归层里做 ownership 更清楚。
还有几个建议收口:
[P2] ShaderData 的 TextureArray 没进入 ref-count cascade。setTextureArray 会按 _refCount 给数组内 texture 加减引用,说明语义上 texture array 是 counted 的;但 cloneTo 对数组只 slice(),没有给 counted target 加引用,_addReferCount 也只处理 property instanceof Texture,不处理 Texture[]。这和本 PR 明确建立的 ShaderData/Material 资源计数契约不自洽。建议抽一个 helper 统一处理 Texture | Texture[],并补 set before host refCount > 0、cloneTo counted target 两类测试。
[P3] Deep / Assignment 的 API 注释还有过度承诺。defaultCloneMode(Deep) 注释写成所有 Deep 类型必须无参构造,但当前设计允许 host-bound Deep 类型通过 preset clone,preset-less 时具名报错;建议改成「preset-less Deep clone 需要无参构造;host-bound/preset-only 类型无 preset 时会失败」。CloneMode.Assignment 注释写 ref-counted resource 会被 clone keep alive,但实际只在 component top-level slot settlement 做计数,nested level 不计数,也建议写明限定。
[P3] git diff --check 目前不干净:docs/en/core/clone.mdx 64/67 和 docs/zh/core/clone.mdx 65/68 有 trailing whitespace。
验证过核心相关测试:
pnpm exec vitest run tests/src/core/CloneManager.test.ts tests/src/core/CloneTextureRefCount.test.ts tests/src/core/RefCountContract.test.ts tests/src/core/ShaderDataRefCount.test.ts -> 86 passed。
…ract setTextureArray counts its entries by the host's refCount, but the cascade in _addReferCount only handled single textures and cloneTo sliced arrays without acquiring — texture-array entries fell out of the contract this PR pins for ShaderData. A shared helper now applies the same cascade to Texture and Texture[]; covered by cascade and clone lifecycle tests. Also narrows two over-promising docs (Deep bare-construction scoped to preset-less clones; Assignment keep-alive scoped to top-level slots) and strips trailing whitespace in the clone guides. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查 @ f8ad0d758(第十二轮)— delta = 1 commit(真 bug 修复 + 作者自审三项闭环)
总结
自上轮 9db6c66ced 起 tip 线性推进 1 个 commit(compare 返回 ahead_by:1 / behind_by:0,status ahead,parent=9db6c66ced,无 force-push):f8ad0d758 fix(shader): cascade texture-array entries through the ref-count contract —— 碰 6 文件(ShaderData.ts +20/-6、ShaderDataRefCount.test.ts +47、CloneManager.ts/CloneMode.ts 各注释单行、两 clone.mdx 仅剥 trailing whitespace)。本 commit 是作者 14:09 自审三项(1×[P2] + 2×[P3])的同周期闭环:一处真 refCount 泄漏修复(配两条反向可证伪测试)+ 两处过度承诺文档收窄。CI 全绿(build×3 macos/ubuntu/windows / e2e×4 视觉回归 / codecov patch+project / lint / labeler)。无 P0/P1/P2/P3,可合并。
逐链核对(delta 本身,非信 commit message)
1. Texture-array 未进 refCount cascade — 真 bug,两条泄漏路径逐一 trace(对照 tip 树 git show,非本地 worktree)
语义前提核实:setTextureArray(ShaderData.ts:458-473)在 _refCount > 0 时对数组每个 entry _addReferCount(refCount)——即 texture array 语义上确是 counted。但旧代码两处漏配对:
- 路径 A —
_addReferCountcascade(host refCount 变动时):旧_addReferCount(parent 树 :718-726)只if (property instanceof Texture) property._addReferCount(value),Texture[]被整个跳过。故当 host ShaderData 的 refCount 后续变动(如第二个 renderer 引用同一 material →Material._addReferCount(+1)→shaderData._addReferCount(+1),Material.ts:76),单 texture cascade+1、array entry 不 cascade → entry refCount 偏低 → 提前 GC-eligible。 - 路径 B —
cloneToarray 分支(深拷 owning texture array 的 ShaderData 时):旧cloneTo(parent 树 :628-629)单Texture走property._addReferCount(referCount)(acquire),但 array 分支只property.slice()不 acquire。clone 的 array 是持相同 texture 引用的新容器、且 clone 的 shaderDatareferCount>0,本该+referCount却没有。
修法:抽 private static _addTexturesReferCount(property, count)(tip :731-740),对 Texture 直接计数、对 Array 逐元素 element instanceof Texture && 计数,两处泄漏点(cloneTo 数组分支 tip :631、_addReferCount cascade tip :724)共用同一份。逐点核实无误伤:Float32Array/Int32Array 非 instanceof Array → 落空安全(无 texture);Texture[] 内混入非 Texture 由逐元素 guard 挡住。
对称性核实(无双计 / 无泄漏):acquire 端 = setTextureArray set 时 +_refCount per-entry + _addReferCount(delta) host 变动时 +delta per-entry;release 端 = setTextureArray 替换旧数组 -refCount per-entry + _addReferCount(-delta) per-entry;destroy 端 = Material._addReferCount(-value) → shaderData._addReferCount(-value)(Material.ts:73-77)→ 现正确 cascade -value per-entry。与既有单 Texture 路径同构,composes 正确。
反向可证伪测试(两条均走公开 API setMaterial/setTextureArray/clone/destroy,仅读 .refCount 断言):
- 测 1「texture-array entries cascade with the host's refCount」:
entityB加第二个 renderer 引同一 material → cascade。旧代码:array 不 cascade →texA.refCount停 1 →expect(.eq(2))FAIL;新代码 PASS;entityB.destroy()→1、entityA.destroy()→0,对称守住。 - 测 2「cloning a shaderData counts the shared entries」:
entity.clone()深拷持数组的 renderer shaderData。旧代码:slice 不 acquire → 停 1 →expect(.eq(2))FAIL;新代码 PASS;clone.destroy()→1、entity.destroy()→0,对称守住。
2. 文档收窄两处([P3])— 逐条对照 tip 代码核实非 aspirational
CloneManager.tsdecorator JSDoc:旧「A type registeredDeepmust construct without arguments」(绝对,被第八~十一轮的 preset-reuse 反证)→ 新「ADeeptype cloned without a same-type preset (e.g. as a container element) must construct bare … host-bound types lacking a preset fail with a named error」。与_bareConstruct(tip :抛具名bare-construct "<T>")+ preset 复用机制精确吻合。CloneMode.tsAssignment 注释:旧「a ref-counted resource is kept alive by the clone」→ 新「a counted resource shared at a component's top-level slot is kept alive」。与_transferSlotOwnership只在cloneComponent顶层 slot 计数(nested effect 参数 texture 共享不 +1,第四轮 PostProcess 测试已证)吻合。两处均是对被证伪/过宽断言的修正,准确。
3. 其余单行 diff:.mdx 仅剥 trailing whitespace(git diff --check 干净化,[P3]);for (var/let key in ...)→for (const key in ...)、new Map<Object,Object>()→new Map<object,object>() 等 lint 级等价改动——零行为变化。
已核对为「已解决 / 不适用 / 不重提」
- 作者 14:09 自审三项([P2] TextureArray cascade + 2×[P3] docs/whitespace) —— 正是本 commit(authored 14:24,在 14:09 之后)逐条修复的内容,作者请求「抽 helper 统一
Texture | Texture[]+ 补两类测试」被逐字落地,作者本人已实测 86 passed。同周期闭环,不重提。 - 历史全部闭环项(type-driven gate、
_transferSlotOwnership、copyFrom 重排+硬化、TypedArrayreuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理_isShapeAttached双挂上移、Map/Set for-of perf、注释卫生、bare-construction 崩溃修复 + guard 测试、_bareConstruct诊断 funnel + 反向测试、host-bound-in-container 语义钉桩、ParticleCurvekey-listener /ColliderShapePhysicsMaterial orphan 属 dev/2.0 既有 out-of-scope)——本 delta 未触碰,不重提。 - 历史 P3
bacause拼写(clone.mdx)—— 本 delta 虽碰clone.mdx但只剥了 64/67 行的 trailing whitespace,未触碰bacause那行(落在未改动行),已提过一次,不再重提。 - 自上轮
9db6c66ced(13:58)以来除作者这条自审外无其它新评论 / inline comment 需 reconcile。
注释合规
_addTexturesReferCount的多行 JSDoc/** Counted texture content may be a single texture or a texture array; both cascade alike. */——多行块、带句号、准确描述当前行为,无偏离。- 两条新测的单行 inline
// A second owner …/// The cloned array is a fresh container …——整句带句号,跟随本文件既有整句//惯例,无偏离。 - 收窄后的
CloneManager.ts/CloneMode.ts注释——均带句号、准确,无偏离。
简化建议
无。delta 是一次高质量收尾:作者自审揪出 texture-array 未进本 PR 建立的 refCount 契约这一自洽性缺口,同周期用一个 Texture | Texture[] 统一 helper 收口两处泄漏路径(配两条反向测试),并把两处过度承诺的 API 文档收窄成与实际机制吻合的措辞。方向正确,把 ShaderData ref-count 契约的最后一个 array 缺口补上了。可合并。
Summary
Rework the clone system around a clear type-driven priority chain, so most objects clone correctly by their type with little to no per-field annotation.
How a field value is cloned — priority high → low
@deepClone/@assignmentClone/@ignoreClone(explicit per-field override, effective at every depth).@deepCloneon an engine-bound value can't be honored: Entity/Component refs fall back to remap, registered assets to sharing — each with a warning.@defaultCloneMode.Assignment(share the reference).Primitives are copied by value. Functions keep the clone's own constructor-rebound binding when the slot already holds one, otherwise share the reference (so callbacks inside containers survive cloning).
Type defaults (what most fields rely on)
ReferResource(Texture / Mesh / Material / Sprite / Font / …) → Assignment — assets are shared by reference.Entity/Component→ Remap — rewired to the clone within the cloned subtree via the identity map (single remap mechanism; the old path-walkCloneUtilsis removed).CloneManager(the math package can't depend on core's decorator); a completeness test guards that every math export withcopyFromis registered — it already caughtRay, which now implementsclone()/copyFrom().UpdateFlagManager,UpdateFlag,DisorderedArray,SafeLoopArray) → Ignore — the clone keeps its own constructor-built state; their former per-field@ignoreCloneannotations are gone.Ref-count model — slot-ownership contract
ReferResourcefamily; duck-typed counters likeShaderare excluded) owns one reference: the gate acquires it while cloning the slot (+1, and-1when replacing an owned constructor preset). Releasing it on destroy is the owning class's responsibility — a class that doesn't release is a bug in that class.onDestroy), the same way engine components release in their destroy paths.Transition._cloneTo+1 ↔Transition.destroy−1, all four state slots counted in the base class (SpriteTransitioncarries no ref-count code).@ignoreCloneso the setter is the single+1source (MeshRenderer.mesh,Renderermaterials,MeshColliderShape.mesh).+1compensations inCamera/Animator/AudioSource_cloneToare removed — the gate acquisition replaces them.Changes
CloneModegainsIgnoreand is now exported.@assignmentClone/@ignoreCloneon Entity/Component refs are now honored literally (share / keep own) instead of being silently overridden by auto-remap.@defaultCloneMode.@shallowCloneis removed (breaking) — migrate with@deepCloneor@assignmentClone._registerCloneMapmerged into tree construction — two recursive walks instead of three);CloneUtils,Entity._remap,Component._remapand thesrcRoot/targetRootthreading are removed._cloneTohooks receive(target, cloneMap);Signalremaps listener targets/args by map lookup (Entity/Component args only, deterministically)._isContainerpoint shared by the gate and the deep-clone dispatch;DataViewclones as bytes; null-prototype objects are classified, rebuilt (Object.create(null)) and field-walked correctly; typed-array / DataView clones register in the identity map (aliasing topology preserved).CloneManager.deepCloneObjecthonors field-level decorators like the rest of the system.ColliderShape(deep-cloned shapes own their native handles;MeshColliderShaperebuilds via the mesh setter, attaching exactly once — attachment state is tracked onColliderShape) and uiTransitionregistered@defaultCloneMode(Deep)— cloned colliders/interactives no longer share mutable shape/transition instances with the source.copyFromvalue-type dispatch runs after the container branches and only for class instances with a callablecopyFrom(a plain object carrying acopyFromdata key no longer crashes clone);SkinnedMeshRendererdrops the renderer-private joint texture entry from clones (GC-ignored GPU-texture leak); slot settlement (_transferSlotOwnership) is unconditional — an owned counted preset displaced by a deep-cloned value releases its reference too.docs/en|zh/core/clone.mdx, how-to-contribute) rewritten to the type-driven semantics:shallowCloneremoved with a migration callout, default-resolution table, entity-remap example, ref-count contract section.Pre-existing imbalances fixed alongside (exposed by the ref-count rework)
TextRenderer._font/ uiText._font(destroy released a count the clone never acquired) andMeshColliderShape._mesh(same, plus the cloned shape silently lacked a native shape) were unbalanced ondev/2.0before this PR; the slot-ownership contract requires them fixed here to stay self-consistent. Destroying aParticleRendereralso never released aMeshShape's mesh — fixed via aBaseShape._destroyhook on the generator destroy chain.Test plan
copyFromcompleteness guard, collider-shape instance independence, ui transition independence, and refCount balance assertions (camera RT / animator controller / TextRenderer font / script-held texture honoring the slot contract / SpriteTransition full lifecycle)bdcc6353— build ×3 platforms, lint, codecov (patch 96.3%), e2e visual regressions ×4 shards🤖 Generated with Claude Code