Skip to content

refactor(clone): type-driven default clone mode for container elements#3040

Open
cptbtptpbcptdtptp wants to merge 55 commits into
galacean:dev/2.0from
cptbtptpbcptdtptp:fix/clone-opt-out-assignment
Open

refactor(clone): type-driven default clone mode for container elements#3040
cptbtptpbcptdtptp wants to merge 55 commits into
galacean:dev/2.0from
cptbtptpbcptdtptp:fix/clone-opt-out-assignment

Conversation

@cptbtptpbcptdtptp

@cptbtptpbcptdtptp cptbtptpbcptdtptp commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

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

  1. Field decorator@deepClone / @assignmentClone / @ignoreClone (explicit per-field override, effective at every depth). @deepClone on an engine-bound value can't be honored: Entity/Component refs fall back to remap, registered assets to sharing — each with a warning.
  2. Container — Array / Map / Set / TypedArray / DataView / plain object (incl. null-prototype) → deep clone (each element re-enters the gate).
  3. Type default — the value type's @defaultCloneMode.
  4. FallbackAssignment (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 / ComponentRemap — rewired to the clone within the cloned subtree via the identity map (single remap mechanism; the old path-walk CloneUtils is removed).
  • Math value types (Vector / Matrix / Quaternion / Color / Rect / Bounding* / Ray / …) → Deep — registered centrally in CloneManager (the math package can't depend on core's decorator); a completeness test guards that every math export with copyFrom is registered — it already caught Ray, which now implements clone() / copyFrom().
  • Value-semantic data (RenderState, particle modules, joint config, PostProcess, ShaderData, Signal, ColliderShape, ui Transition, …) → Deep.
  • Runtime containers (UpdateFlagManager, UpdateFlag, DisorderedArray, SafeLoopArray) → Ignore — the clone keeps its own constructor-built state; their former per-field @ignoreClone annotations are gone.

Ref-count model — slot-ownership contract

  • Every component top-level field sharing an explicitly registered ref-counted resource (the ReferResource family; duck-typed counters like Shader are excluded) owns one reference: the gate acquires it while cloning the slot (+1, and -1 when 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.
  • The contract is uniform — user scripts included: a cloned script slot owns one reference like any component slot; releasing it on destroy is the script author's responsibility (onDestroy), the same way engine components release in their destroy paths.
  • Below the top level the gate never counts. The gate only auto-counts where the destroy contract is uniform and auditable — component slots; a nested host's release contract is unknowable to the gate (many nested hosts have no destroy at all), so nested classes pair the acquisition themselves in class-local code: Transition._cloneTo +1 ↔ Transition.destroy −1, all four state slots counted in the base class (SpriteTransition carries no ref-count code).
  • Slots rebuilt through setters stay @ignoreClone so the setter is the single +1 source (MeshRenderer.mesh, Renderer materials, MeshColliderShape.mesh).
  • Manual +1 compensations in Camera / Animator / AudioSource _cloneTo are removed — the gate acquisition replaces them.

Changes

  • Field decorators register as field-level modes (highest priority); CloneMode gains Ignore and is now exported.
  • ⚠️ @assignmentClone / @ignoreClone on Entity/Component refs are now honored literally (share / keep own) instead of being silently overridden by auto-remap.
  • −191 / +13 field decorators (the 13 additions are genuine runtime-state opt-outs); 37 classes registered via @defaultCloneMode. @shallowClone is removed (breaking) — migrate with @deepClone or @assignmentClone.
  • Entity/Component remap unified through the clone identity map (_registerCloneMap merged into tree construction — two recursive walks instead of three); CloneUtils, Entity._remap, Component._remap and the srcRoot/targetRoot threading are removed. _cloneTo hooks receive (target, cloneMap); Signal remaps listener targets/args by map lookup (Entity/Component args only, deterministically).
  • Container classification centralized in a single _isContainer point shared by the gate and the deep-clone dispatch; DataView clones 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.deepCloneObject honors field-level decorators like the rest of the system.
  • ColliderShape (deep-cloned shapes own their native handles; MeshColliderShape rebuilds via the mesh setter, attaching exactly once — attachment state is tracked on ColliderShape) and ui Transition registered @defaultCloneMode(Deep) — cloned colliders/interactives no longer share mutable shape/transition instances with the source.
  • Review hardening: the copyFrom value-type dispatch runs after the container branches and only for class instances with a callable copyFrom (a plain object carrying a copyFrom data key no longer crashes clone); SkinnedMeshRenderer drops 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 (docs/en|zh/core/clone.mdx, how-to-contribute) rewritten to the type-driven semantics: shallowClone removed 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 / ui Text._font (destroy released a count the clone never acquired) and MeshColliderShape._mesh (same, plus the cloned shape silently lacked a native shape) were unbalanced on dev/2.0 before this PR; the slot-ownership contract requires them fixed here to stay self-consistent. Destroying a ParticleRenderer also never released a MeshShape's mesh — fixed via a BaseShape._destroy hook on the generator destroy chain.

Test plan

  • Full local suite: 1565 tests / 118 files pass (core / ui / loader / math)
  • Multi-agent exhaustive review (6 dimensions, adversarially verified): confirmed regressions all fixed with regression tests; Skin / PostProcess / TrailRenderer clone paths now covered
  • refCount lifecycle suites (slot-ownership: clone +1 → setter churn → destroy release, per counted slot; ShaderData/Material cascade; instance materials)
  • New regressions: decorator-priority semantics, functions in containers, DataView byte-clone, null-prototype containers, math copyFrom completeness 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)
  • CI green on bdcc6353 — build ×3 platforms, lint, codecov (patch 96.3%), e2e visual regressions ×4 shards

🤖 Generated with Claude Code

…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>
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The 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.

Changes

Clone pipeline and core remapping

Layer / File(s) Summary
Clone infrastructure
packages/core/src/clone/CloneManager.ts, packages/core/src/clone/ComponentCloner.ts, packages/core/src/clone/enums/CloneMode.ts, packages/core/src/index.ts, packages/core/src/clone/CloneUtils.ts
CloneMode adds Remap, removes Shallow, and updates its docs. CloneManager adds defaultCloneMode, per-field mode registration, getFieldModes, _cloneValue, _deepClone, and counted-resource helpers, while CloneUtils is removed. ComponentCloner switches to cloneMap-based cloning and the revised ICustomClone contract.
Entity, component, and signal remapping
packages/core/src/Component.ts, packages/core/src/Entity.ts, packages/core/src/Signal.ts, packages/core/src/particle/modules/CustomDataModule.ts
Component and Entity move to @defaultCloneMode(CloneMode.Remap) and use a shared clone map through subtree cloning. Signal now remaps listeners and structured-binding arguments from the same identity map, and CustomDataModule threads that map through deep-cloned curves and gradients.

Clone-mode updates across engine, particle, physics, and UI classes

Layer / File(s) Summary
Core clone-mode updates
packages/core/src/shader/state/*, packages/core/src/Camera.ts, packages/core/src/Renderer.ts, packages/core/src/Transform.ts, packages/core/src/UpdateFlag*.ts, packages/core/src/VirtualCamera.ts, packages/core/src/animation/Animator.ts, packages/core/src/asset/ReferResource.ts, packages/core/src/audio/AudioSource.ts, packages/core/src/lighting/Light.ts, packages/core/src/mesh/Skin.ts, packages/core/src/mesh/SkinnedMeshRenderer.ts, packages/core/src/postProcess/*, packages/core/src/shader/ShaderData.ts, packages/core/src/trail/TrailRenderer.ts, packages/core/src/utils/*, packages/math/src/Ray.ts
Core engine types move to class-level default clone modes or remove explicit field decorators. Several classes add or remove clone-time cleanup and resource ownership behavior, and Ray now implements clone() and copyFrom().
Particle, physics, and UI clone behavior
packages/core/src/particle/*, packages/core/src/particle/modules/*, packages/core/src/particle/modules/shape/*, packages/core/src/physics/*, packages/ui/src/component/*
Particle modules and shape classes migrate to class-level defaults, runtime-only fields are ignored, and mesh/state teardown hooks are added. Physics and UI components also update clone decorators, cleanup, and reference-count handling to match the new pipeline.

Clone and ref-count test coverage

Layer / File(s) Summary
Clone, ref-count, and copy coverage
tests/src/core/*, tests/src/math/Ray.test.ts, tests/src/ui/*
New and expanded tests cover clone-map remapping, slot ownership and ref-count balancing, binary data cloning, function handling, math value-type copy/clone, and UI/physics transition behavior.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: GuoLei1990

Poem

🐇 I hopped through clone maps, tidy and bright,
With deep-clone defaults and remaps just right.
Ref counts now dance and the tests sing along,
The bunny says: “cloning feels sturdy and strong!”

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main refactor toward type-driven default clone mode and container cloning behavior.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5d74c1d and 79e9105.

📒 Files selected for processing (8)
  • packages/core/src/clone/CloneManager.ts
  • packages/core/src/clone/ComponentCloner.ts
  • packages/core/src/shader/state/BlendState.ts
  • packages/core/src/shader/state/DepthState.ts
  • packages/core/src/shader/state/RasterState.ts
  • packages/core/src/shader/state/RenderState.ts
  • packages/core/src/shader/state/RenderTargetBlendState.ts
  • packages/core/src/shader/state/StencilState.ts

Comment thread packages/core/src/clone/CloneManager.ts
cptbtptpbcptdtptp and others added 7 commits June 17, 2026 19:31
…+ 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

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.96450% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.15%. Comparing base (721b42a) to head (f8ad0d7).

Files with missing lines Patch % Lines
packages/core/src/clone/CloneManager.ts 97.73% 7 Missing ⚠️
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     
Flag Coverage Δ
unittests 80.15% <98.96%> (+0.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cptbtptpbcptdtptp and others added 6 commits June 23, 2026 15:40
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>
@cptbtptpbcptdtptp

Copy link
Copy Markdown
Collaborator Author

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. Assignment is a plain reference share again. The gate's unconditional +1 was structurally unbalanceable: it double-counted with the manual +1s in Camera/Animator/AudioSource._cloneTo, leaked on fields no destroy path releases (user Script resources, Camera._replacementShader — Shader isn't even a ReferResource), and couldn't reach non-component hosts (SpriteTransition state sprites, Signal args). Ownership now lives entirely in each class's own logic (_cloneTo hooks / setters), balanced by that class's destroy path — the proven pre-existing pattern (MeshRenderer, Renderer, ShaderData.cloneTo). Fixed the classes that had drifted from it: TextRenderer._font / ui Text._font / MeshColliderShape runtime state are @ignoreClone + setter-in-_cloneTo; SpriteTransition._cloneTo acquires its state-sprite refs.

2. Remap is unified through the identity map and field decorators win at every depth. The ComponentCloner "_remap first" special case and the CloneUtils path-walk are gone — Entity.clone() registers every source→clone pair while building the tree (2 walks instead of 3), and the gate's Remap branch resolves in O(1) with identical out-of-subtree semantics. srcRoot/targetRoot threading is removed; _cloneTo hooks receive the cloneMap (only Signal uses it). @deepClone on an Entity/Component ref warns and falls back to remap instead of fabricating an engine-less instance. ⚠️ Semantic change: @assignmentClone/@ignoreClone on entity refs are now honored literally instead of being overridden by auto-remap (tests updated).

Also in this push: DataView clones as bytes (crashed before via the generic object branch); functions inside containers are shared instead of silently becoming undefined, while constructor-rebound top-level handlers keep the clone's own binding; container classification is a single _isContainer predicate; ColliderShape / ui Transition are registered @defaultCloneMode(Deep) so cloned colliders/buttons own independent instances (previously the clone shared and re-parented the source's shapes/transitions); dead copyProperty/copyProperties removed; shallowClone warns on use; CloneMode is exported.

Known pre-existing (not addressed): ColliderShape's constructor presets a native PhysicsMaterial that gets orphaned whenever the field is later replaced (clone or user shape.material = x) — same behavior on dev/2.0, worth its own issue.

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 dev/2.0 (resolved ColliderShape.test.ts against the physics-lite removal).

🤖 Generated with Claude Code

cptbtptpbcptdtptp and others added 10 commits July 3, 2026 11:09
…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>
GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

…-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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/src/core/CloneUtils.test.ts (1)

1275-1292: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

This 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 whose refCount can 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

📥 Commits

Reviewing files that changed from the base of the PR and between e696681 and d095812.

📒 Files selected for processing (10)
  • packages/core/src/Signal.ts
  • packages/core/src/clone/CloneManager.ts
  • packages/core/src/mesh/SkinnedMeshRenderer.ts
  • packages/core/src/particle/modules/CustomDataModule.ts
  • packages/core/src/physics/Collider.ts
  • packages/core/src/physics/shape/ColliderShape.ts
  • packages/core/src/physics/shape/MeshColliderShape.ts
  • tests/src/core/CloneTextureRefCount.test.ts
  • tests/src/core/CloneUtils.test.ts
  • tests/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

cptbtptpbcptdtptp and others added 2 commits July 3, 2026 22:18
…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>
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jul 3, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d095812 and 78ec199.

📒 Files selected for processing (7)
  • docs/en/core/clone.mdx
  • docs/en/how-to-contribute.mdx
  • docs/zh/core/clone.mdx
  • docs/zh/how-to-contribute.mdx
  • tests/src/core/SkinnedMeshRenderer.test.ts
  • tests/src/core/Trail.test.ts
  • tests/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

Comment thread docs/en/core/clone.mdx
Comment on lines +138 to 139
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].

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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.

Comment thread docs/en/core/clone.mdx
Comment thread docs/zh/core/clone.mdx
Comment thread docs/zh/core/clone.mdx
Comment on lines +149 to +150
## 克隆与资产引用计数
克隆时,如果组件的顶层字段共享了一个引用计数资产(`ReferResource`,如 `Texture`、`Mesh`、`Material`),引擎会自动为克隆体增加一次引用计数。持有该字段的类负责在销毁时释放这次引用 —— 内置组件已由引擎处理;自定义脚本请在 `onDestroy` 中释放自己持有的资源。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Same ref-count contradiction as the English doc.

This section claims "克隆时,如果组件的顶层字段共享了一个引用计数资产(ReferResource,如 TextureMeshMaterial),引擎会自动为克隆体增加一次引用计数。" 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.

GuoLei1990

This comment was marked as outdated.

cptbtptpbcptdtptp and others added 2 commits July 4, 2026 08:57
…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>
GuoLei1990

This comment was marked as outdated.

…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 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查 @ 2a919171b(第六轮)— delta = 1 commit(pure perf, behavior-equivalent)

总结

自上轮 23f0459d6 起 tip 线性推进 1 个 commitcompare 返回 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))
  • 迭代序forEachfor-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 称「forEach arrow closures were the only unconditional intermediate allocations on the clone path」。核对文件全部 forEach:唯一其它命中在 getFieldModes:88own.forEach),但该函数按 type 记忆化_fieldModeMap.get(type) 命中即返回缓存),forEach 只在某 type 首次克隆时跑、后续走缓存,属摊销分配非每克隆分配。故 body 「on the clone path(每克隆路径)」的限定准确——Map/Set 那两个 forEach 才是每次深拷 Map/Set 值都新建一个闭包的唯一无条件分配点。for-of 迭代器可被逃逸分析消除、闭包不可。理由属实。

已核对为「已解决 / 不适用 / 不重提」

  • 历史 P3 #2~#6_transferSlotOwnership 无条件化、copyFrom 重排、TypedArray reuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理 _isShapeAttached 上移、归属注释校准、CodeRabbit「ref-count claim contradicts architecture」误报、ParticleCurve key-listener / ColliderShape PhysicsMaterial 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 收口(两分支 forEachfor-of,行为逐字节不变),把 type-driven 重构的最后一处每克隆闭包分配也消掉了。可合并。

cptbtptpbcptdtptp and others added 2 commits July 4, 2026 10:05
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@GuoLei1990 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查 @ 6d724c798(第七轮)— delta = 2 commits(均 docs-only,注释单行改动)

总结

自上轮 2a919171b 起 tip 线性推进 2 个 commitcompare 返回 ahead_by:2 / behind_by:0,status ahead,无 force-push),全部为注释单行清理,零产物逻辑改动:

  1. c13dd08e5 docs(clone): drop a change-narrating comment on the Map branchCloneManager.ts(+1/-1)
  2. 6d724c798 docs(clone): drop tense-relative wording from a lifecycle test commentCloneTextureRefCount.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 重排、TypedArray reuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理 _isShapeAttached 上移、归属注释校准、Map/Set for-of perf、CodeRabbit「ref-count claim contradicts architecture」误报、ParticleCurve key-listener / ColliderShape PhysicsMaterial orphan(dev/2.0 既有)——均已闭环或明确 out-of-scope,本 delta 未触碰,不重提。
  • 上轮 P3 bacause 拼写(clone.mdx)—— 本 delta 未触碰该文件,落在未改动行,且已提过一次,不再重提。
  • 自上轮以来无新的 PR 评论 / inline comment / 作者回复需要 reconcile。

简化建议

无。delta 是一次干净的注释卫生收口——把两处「叙述 diff / 时态相对」的注释换成「描述当前不变式」的措辞,方向正确,把 type-driven 重构的注释残留也清干净了。可合并。

@cptbtptpbcptdtptp cptbtptpbcptdtptp left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

发现一个可复现的 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 值。

cptbtptpbcptdtptp and others added 2 commits July 4, 2026 20:18
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 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查 @ 16d95d6f3(第八轮)— delta = 2 commits(latent bug fix + mechanism guard test)

总结

自上轮 6d724c798 起 tip 线性推进 2 个 commitcompare 返回 ahead_by:2 / behind_by:0,status ahead,无 force-push):

  1. 828008f9b fix(particle): allow bare construction of ParticleCompositeGradientParticleCompositeGradient.ts(+9/-1)+ 测试(+42)
  2. 16d95d6f3 test(clone): enforce the bare-construction contract for Deep-registered typesCloneManager.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 的 _deepClone object 分支 const dst = reusable ?? (ctor ? new ctor() : ...)——当值是容器元素(array/map/set/plain-object)时,容器分支以 _cloneValue(element, undefined, ...) 传入 reuse=undefinedreusable=null → 走 new ctor() 裸构造。旧 ParticleCompositeGradient 构造函数 constructor(constantOrGradient: Color | ParticleGradient, ...) 首参必填,且无条件 constantOrGradient.constructor === Color 解引用 → 裸构造时 undefined.constructor TypeError
  • 触发面真实存在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 态,evaluateConstantthis.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 = undefinedcurveMax=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 两类逐条核实合法
    • 物理 shapesColliderShape + 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._onCompositeCurveChangethis._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」准确
  • 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.ts barrel)→ guard 的 Object.entries 看不到 → 既不被测也不需 exempt。它是 ParticleGenerator 具名字段永不 bare,实践零风险;只是 guard 的枚举面天然局限于「已导出」。不作为问题(guard 覆盖导出 surface 已是最强可行契约)。

已核对为「已解决 / 不适用 / 不重提」

  • 历史 P3 #2~#6_transferSlotOwnership 无条件化、copyFrom 重排+硬化、TypedArray reuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理 _isShapeAttached 双挂上移、归属注释校准、Map/Set for-of perf、Map/test 注释卫生、CodeRabbit「ref-count claim contradicts architecture」误报、ParticleCurve key-listener / ColliderShape PhysicsMaterial 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.ts decorator JSDoc 追加句「A type registered Deep must 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 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查 @ f58d380dc(第九轮)— delta = 1 commit(diagnostic error-wrapping + reverse-falsifiable test)

总结

自上轮 16d95d6f3 起 tip 线性推进 1 个 commitcompare 返回 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 namedParamDeepConfig@defaultCloneMode(Deep) + 必填参构造函数解引用 source.id)放进 config.items 数组(容器元素路径 → reuse=undefined → 裸构造),parent.clone() 触发。
    • 旧代码 new ctor() 直接抛裸 TypeError: Cannot read properties of undefined (reading 'id')不匹配 /bare-construct "ParamDeepConfig"/toThrowError FAIL
    • 新代码... 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 重排+硬化、TypedArray reuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理 _isShapeAttached 双挂上移、Map/Set for-of perf、注释卫生、上轮 bare-construction 崩溃修复 + guard 测试、ParticleCurve key-listener / ColliderShape PhysicsMaterial 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 固化的隐性契约在运行时也命名出来——不掩盖异常、只让它更可诊断,配反向测试守回归。方向正确,可合并。

cptbtptpbcptdtptp and others added 2 commits July 4, 2026 21:37
…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

This comment was marked as outdated.

@GuoLei1990 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查 @ 9db6c66ced(第十一轮)— delta = 1 commit(test-only:按组件顺序拆分 host-bound 容器行为,修复我上轮误批的坏测试

总结

自上轮 843ab780e 起 tip 线性推进 1 个 commitcompare 返回 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.mainParticleRenderer.generator 内的嵌套 MainModule(非 component),故建树期不进 cloneMap
  • main 进 cloneMap 的时机 = _parseCloneEntityComponentCloner.cloneComponent(ParticleRenderer, …) 深拷 generator 树时(gate _deepClonecloneMap.set(value, dst)CloneManager.ts:321mainParticleGenerator 具名字段 ⇒ reusable=clone 自己的构造期 main preset ⇒ 复用它并注册)。
  • component 迭代序 = _components 数组序 = addComponent 序_parseCloneEntity:490-492src._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]ParticleRenderercloneComponentgenerator.main 深拷并注册进 cloneMap ⇒ 随后 CopyFromDataScript.config.modules[0](=源 main)作容器元素走 _deepClone:237 cloneMap.get(value) 命中 existingremap(dedup)不裸构造
  • 断言 clonedModule === cloned.getComponent(ParticleRenderer).generator.main(同一复用 preset)+ !== renderer.generator.main(是 clone 自己的)——逐链核对为真。旧代码/坏断言:此路径 remap 不 throw ⇒ 上轮 toThrowError FAIL;新断言:直接断 remap 结果 ⇒ PASS。

测 2 a host-bound instance in a container fails with the named error when no host precedes(script 先加)— 真反向可证伪

  • script 先加 ⇒ _components=[CopyFromDataScript, ParticleRenderer]CopyFromDataScriptcloneComponentconfig.modules[0](=源 main)此时 cloneMap miss ⇒ 落 object 分支、容器元素 reuse=undefinedreusable=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 重排+硬化、TypedArray reuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理 _isShapeAttached 双挂上移、Map/Set for-of perf、注释卫生、bare-construction 崩溃修复 + guard 测试、_bareConstruct 诊断 funnel + 反向测试、ParticleCurve key-listener / ColliderShape PhysicsMaterial 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 cptbtptpbcptdtptp left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最后一轮看 API / 注释 / 设计自洽性,整体方向我认可:类型驱动默认行为、容器默认结构 deep、未知对象 assignment、Entity/Component 统一走 identity-map remap,ref-count 从 clone gate 移到 component slot settlement,也比递归层里做 ownership 更清楚。

还有几个建议收口:

[P2] ShaderDataTextureArray 没进入 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 > 0cloneTo 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 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量审查 @ f8ad0d758(第十二轮)— delta = 1 commit(真 bug 修复 + 作者自审三项闭环)

总结

自上轮 9db6c66ced 起 tip 线性推进 1 个 commitcompare 返回 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)

语义前提核实:setTextureArrayShaderData.ts:458-473)在 _refCount > 0 时对数组每个 entry _addReferCount(refCount)——即 texture array 语义上确是 counted。但旧代码两处漏配对:

  • 路径 A — _addReferCount cascade(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 — cloneTo array 分支(深拷 owning texture array 的 ShaderData 时):旧 cloneTo(parent 树 :628-629)单 Textureproperty._addReferCount(referCount)(acquire),但 array 分支只 property.slice() 不 acquire。clone 的 array 是持相同 texture 引用的新容器、且 clone 的 shaderData referCount>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.ts decorator JSDoc:旧「A type registered Deep must construct without arguments」(绝对,被第八~十一轮的 preset-reuse 反证)→ 新「A Deep type 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.ts Assignment 注释:旧「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 重排+硬化、TypedArray reuse !== value、SkinnedMeshRenderer joint texture 泄漏、物理 _isShapeAttached 双挂上移、Map/Set for-of perf、注释卫生、bare-construction 崩溃修复 + guard 测试、_bareConstruct 诊断 funnel + 反向测试、host-bound-in-container 语义钉桩、ParticleCurve key-listener / ColliderShape PhysicsMaterial 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 缺口补上了。可合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants