refactor(clone): opt-in @property model with identity-map remap#3018
refactor(clone): opt-in @property model with identity-map remap#3018cptbtptpbcptdtptp wants to merge 12 commits into
Conversation
|
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:
WalkthroughRefactors cloning to use a shared identity-map (cloneMap), introduces class-level default clone modes (including Remap), replaces per-field clone-mode decorators with an opt-in ChangesClone System Architecture Refactoring
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/Signal.ts (1)
125-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun structured listener args through the normal clone classifier.
cloneMap.get(arg) ?? argonly fixes top-level entity/component arguments. If a listener is registered with something like an array, object,Map, orSetthat contains cloned entities/components, that wrapper is reused as-is and still points back to the source graph after clone. Please clone each structured-listener argument with the same deep-clone/remap path used for normal properties instead of doing a shallow map lookup here.
🧹 Nitpick comments (1)
tests/src/core/CloneUtils.test.ts (1)
97-100: ⚡ Quick winCover
Mapkeys andSetelements too.These new inference tests only lock down
Mapvalues with primitive keys. The new clone path remaps bothMapkeys and values, and it has a separateSetbranch, so regressions there would still pass this suite. Please add one case with entity keys and one withSet<Entity>to fully cover the container remap contract.Also applies to: 1011-1031
🤖 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 97 - 100, The tests currently only assert Map values are remapped; add two cases: one that uses a Map with Entity keys (e.g., a Map<Entity, Entity> or Map<Entity, string>) and one that uses a Set<Entity> to ensure the clone path remaps keys and Set elements as well. Create small helper scripts/classes analogous to MapRefScript (e.g., MapKeyScript and SetRefScript) that populate a Map with an Entity key and a Set<Entity>, run the same clone/serialize roundtrip used by the existing tests, and assert the cloned Map keys and Set elements are remapped (different identities from the originals but referentially consistent with the cloned entities).
🤖 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 161-180: The Map/Set branches only call
CloneManager._remapIfPossible on keys/values, leaving undecorated nested
objects/arrays shared; instead run those items through the same
classification+clone gate used elsewhere so container contents are deep-cloned
by runtime type. Update the Map and Set loops to remap then pass each key/value
through the classify+clone helper (the same function used for object/array
properties) rather than only calling CloneManager._remapIfPossible, i.e.,
replace the direct _remapIfPossible usage with the classify-and-clone path for
both keys and values so nested plain objects/arrays are cloned by value.
- Around line 134-140: The code treats CloneMode.Shallow as a deep clone because
any mode not Ignore/Assignment falls through to CloneManager._deepClone; change
the branching in the property copy logic to explicitly handle CloneMode.Shallow
by assigning the source reference into the target (i.e., target[k] =
sourceProperty and return) instead of calling _deepClone, so shallow-marked
arrays/objects keep shared references; leave Ignore and Assignment behavior
unchanged and keep calling _deepClone only for modes that require recursive
cloning.
- Around line 153-205: The Map/Set/Array branches in CloneManager.cloneProperty
never register the new destination container in cloneMap before iterating, so
shared or cyclic containers get duplicated or cause infinite recursion; fix by
inserting cloneMap.set(sourceProperty, dst) immediately after creating (or
resolving) dst for Map, Set, and Array (and after assigning target[k] for
typed-array branches if you want consistency), before any forEach/for loop, so
subsequent encounters use the memoized dst; refer to CloneManager.cloneProperty,
the sourceProperty / target[k] local variables, and cloneMap when adding this
memoization.
In `@packages/core/src/clone/enums/CloneRefMode.ts`:
- Around line 18-20: The comment for the `@remapClone` precedence is incorrect:
CloneManager.cloneProperty() only consults the field's _cloneRefMode when no
explicit field-level clone decorator exists, so `@ignoreClone` and
`@assignmentClone` still override type-level remap. Update the docblock on
CloneRefMode/@remapClone to state that type-level remap is the default only for
fields without an explicit per-field decorator, and mention
CloneManager.cloneProperty(), _cloneRefMode, `@ignoreClone` and `@assignmentClone`
as the relevant symbols so callers understand that explicit field decorators
take precedence over the type-level remap.
In `@packages/core/src/Entity.ts`:
- Around line 455-462: The bug is mapping source components to clones by array
index (cloneMap.set(components[i], cloneComponents[i])), which breaks when a
component constructor adds components and reorders cloneEntity._components;
instead, build the mapping from the actual clone instances created during clone
construction: change the clone creation flow (where cloneEntity components are
instantiated—e.g., inside the clone constructor or Entity.addComponent
invocation used for cloning) to call cloneMap.set(sourceComponentInstance,
createdCloneInstance) at the moment each clone component is
constructed/returned, and then change any later lookups that use
target._components[i] to consult cloneMap.get(sourceComponent) rather than
positional indexing; apply the same fix for the other occurrence referenced
(lines 486-488).
---
Nitpick comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 97-100: The tests currently only assert Map values are remapped;
add two cases: one that uses a Map with Entity keys (e.g., a Map<Entity, Entity>
or Map<Entity, string>) and one that uses a Set<Entity> to ensure the clone path
remaps keys and Set elements as well. Create small helper scripts/classes
analogous to MapRefScript (e.g., MapKeyScript and SetRefScript) that populate a
Map with an Entity key and a Set<Entity>, run the same clone/serialize roundtrip
used by the existing tests, and assert the cloned Map keys and Set elements are
remapped (different identities from the originals but referentially consistent
with the cloned entities).
🪄 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: a183297d-965f-4eae-af17-095bc4bd403c
📒 Files selected for processing (15)
packages/core/src/2d/text/SubFont.tspackages/core/src/Component.tspackages/core/src/Entity.tspackages/core/src/Signal.tspackages/core/src/asset/ReferResource.tspackages/core/src/clone/CloneManager.tspackages/core/src/clone/CloneUtils.tspackages/core/src/clone/ComponentCloner.tspackages/core/src/clone/enums/CloneRefMode.tspackages/core/src/index.tspackages/core/src/shader/Shader.tspackages/core/src/shader/ShaderMacro.tspackages/core/src/shader/ShaderTagKey.tspackages/ui/src/component/interactive/UIInteractive.tstests/src/core/CloneUtils.test.ts
💤 Files with no reviewable changes (2)
- packages/core/src/clone/CloneUtils.ts
- packages/core/src/index.ts
| if (cloneMode === CloneMode.Ignore) return; | ||
|
|
||
| // Primitives, undecorated, or @assignmentClone: direct assign | ||
| if (!(sourceProperty instanceof Object) || cloneMode === undefined || cloneMode === CloneMode.Assignment) { | ||
| if (cloneMode === CloneMode.Assignment) { | ||
| target[k] = sourceProperty; | ||
| return; | ||
| } | ||
| CloneManager._deepClone(sourceProperty, target, k, cloneMap); | ||
| } |
There was a problem hiding this comment.
CloneMode.Shallow is currently treated as deep clone.
Line 139 sends every non-Ignore/Assignment mode into _deepClone(), and _deepClone() recursively classifies child fields/elements again. That breaks the public @shallowClone contract for arrays and plain objects, because nested references stop being shared.
Suggested direction
if (cloneMode === CloneMode.Ignore) return;
if (cloneMode === CloneMode.Assignment) {
target[k] = sourceProperty;
return;
}
+ if (cloneMode === CloneMode.Shallow) {
+ CloneManager._shallowClone(sourceProperty, target, k, cloneMap);
+ return;
+ }
CloneManager._deepClone(sourceProperty, target, k, cloneMap);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 134 - 140, The code
treats CloneMode.Shallow as a deep clone because any mode not Ignore/Assignment
falls through to CloneManager._deepClone; change the branching in the property
copy logic to explicitly handle CloneMode.Shallow by assigning the source
reference into the target (i.e., target[k] = sourceProperty and return) instead
of calling _deepClone, so shallow-marked arrays/objects keep shared references;
leave Ignore and Assignment behavior unchanged and keep calling _deepClone only
for modes that require recursive cloning.
| * Class decorator: instances are remappable references (Entity / Component), resolved to their | ||
| * clone via the identity map. Highest clone priority — even @ignoreClone / @assignmentClone on | ||
| * the referencing field do not bypass it (prevents carrying source-subtree references into a clone). |
There was a problem hiding this comment.
Fix the @remapClone precedence comment.
CloneManager.cloneProperty() only reads _cloneRefMode when the field has no explicit clone decorator, so @ignoreClone and @assignmentClone still override type-level remap. The current doc says the opposite and will mislead callers about the actual contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/enums/CloneRefMode.ts` around lines 18 - 20, The
comment for the `@remapClone` precedence is incorrect:
CloneManager.cloneProperty() only consults the field's _cloneRefMode when no
explicit field-level clone decorator exists, so `@ignoreClone` and
`@assignmentClone` still override type-level remap. Update the docblock on
CloneRefMode/@remapClone to state that type-level remap is the default only for
fields without an explicit per-field decorator, and mention
CloneManager.cloneProperty(), _cloneRefMode, `@ignoreClone` and `@assignmentClone`
as the relevant symbols so callers understand that explicit field decorators
take precedence over the type-level remap.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cptbtptpbcptdtptp
force-pushed
the
fix/clone-prefab-rearchitect
branch
from
27b8a5a to
5b9589b
Compare
cptbtptpbcptdtptp
changed the title
Reworks the prefab/entity clone classifier so reference handling is correct by default and decided by one consistent, declarative mechanism. - Replace CloneUtils hierarchy-path remap with a single source->clone identity map (merged with the dedup map). Fixes the destroyed-entity NPE and the multi-same-type-component indexOf fragility; O(1) remap instead of O(depth). - Add CloneMode.Remap and a type-level @defaultCloneMode(mode) class decorator (set non-enumerably on the prototype) as the default for undecorated fields: Entity/Component -> Remap, ReferResource + Shader/ShaderMacro/ShaderTagKey/ SubFont -> Assignment. Explicit field decorators take precedence. - Decouple populate (copyFrom) from finalize (_cloneTo); _cloneTo always runs. - Delete CloneUtils. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
changed the title
cptbtptpbcptdtptp
force-pushed
the
fix/clone-prefab-rearchitect
branch
from
5b9589b to
15af1cb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Flip the clone system from opt-out (clone every enumerable field unless @ignoreClone) to opt-in: only fields marked @Property are cloned, and HOW is type-driven via @defaultCloneMode (Entity/Component -> Remap, ReferResource / interned flyweights -> Assignment, otherwise deep). Aligns with Unity [SerializeField] / UE UPROPERTY / Cocos @Property: one neutral "managed property" marker, behaviors as orthogonal concerns. - CloneManager walks a cached per-class @Property field set (getProperties) instead of `for k in source`; drops per-field CloneMode plumbing, the legacy bridges, and CloneMode.Shallow/Ignore. - Migrate ~70 files: @deepClone/@assignmentClone -> @Property, delete @ignoreClone, and add @Property to currently-cloned-but-undecorated fields (Camera, Renderer, lighting, physics, particle, postprocess, ui, ...). - Gap-fill decorator-less cloneable classes found via the test suite + a static @property-field-type sweep: physics shape/joint subclasses, PhysicsMaterial, PostProcessEffect(+Parameter), Transition (_target/_interactive), PointLight, particle emitter shapes. - Skin.name made a @Property field + parameterless-constructible; function-typed fields are skipped (transient, re-established by the constructor). - Rewrite clone spec tests for the opt-in contract (incl. "unmarked field is not cloned"); fixtures mark @Property; Signal tests use the identity-map _cloneTo. Verified: 986 browser tests pass (core 826, ui 64, loader 96). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
changed the title
After merging dev/2.0 (custom particle data galacean#3004), adapt its clone-using code to the opt-in @Property model: - CustomDataModule: drop @ignoreClone on its fields — its `_cloneTo` deep-clones the curve/gradient maps, so the walker must not touch them. - ParticleGenerator.customData: @deepClone -> @Property. Build + clone/custom-data tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/Entity.ts (1)
447-478:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIndex-based component mapping assumes no constructor side-effects.
Lines 459-463 map source components to clone components by array index, relying on the assumption that
new Entity(...componentConstructors)creates components in exactly the same order. If any component constructor internally callsentity.addComponent(), the clone's_componentsarray will have a different ordering, causing incorrect mappings.The comment on line 458 acknowledges this constraint. Consider adding a defensive check (e.g., assert
components[i].constructor === cloneComponents[i].constructor) during development builds to catch violations early.🛡️ Suggested defensive check (development only)
cloneMap.set(this, cloneEntity); const cloneComponents = cloneEntity._components; for (let i = 0, n = components.length; i < n; i++) { + // `@ifdef` DEBUG + if (components[i].constructor !== cloneComponents[i].constructor) { + console.warn( + `Clone component mismatch at index ${i}: expected ${components[i].constructor.name}, got ${cloneComponents[i].constructor.name}. ` + + `A component constructor may be adding additional components.` + ); + } + // `@endif` cloneMap.set(components[i], cloneComponents[i]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Entity.ts` around lines 447 - 478, The index-based component mapping in Entity._createCloneEntity assumes components are created in identical order; add a development-only defensive check after obtaining cloneComponents to verify that for each index i components[i].constructor === cloneComponents[i].constructor (or otherwise assert true) and surface a clear error (throw or logger) if a mismatch is detected so violations (e.g., constructors that call entity.addComponent()) are caught early; keep this check gated behind a dev flag or NODE_ENV !== 'production' and reference Entity._createCloneEntity, componentConstructors, cloneMap, components and cloneComponents when adding the assertion.packages/core/src/physics/PhysicsMaterial.ts (1)
9-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix PhysicsMaterial native desync on deep clone
PhysicsMaterialnow marks_bounciness,_dynamicFriction,_staticFriction,_bounceCombine, and_frictionCombinewith@property, but it still only creates_nativeMaterialin the constructor and has no_cloneTo/native sync hook. During deep cloning,CloneManagerreuses the pre-allocatedPhysicsMaterialinstance created byColliderShape’s constructor, populates only the JS@propertyfields, and thenColliderShape._syncNative()bindsthis._material._nativeMaterial—which still holds the default constructor values—so the native physics material can diverge from the cloned JS values.Add a
PhysicsMaterial._cloneTo(...)(or equivalent) to recreate/synctarget._nativeMaterialfrom the cloned JS fields after populate, or changePhysicsMaterial’s clone mode toAssignmentif native sharing is intended.
packages/core/src/physics/PhysicsMaterial.ts (fields + constructor)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/physics/PhysicsMaterial.ts` around lines 9 - 33, PhysicsMaterial currently leaves _nativeMaterial tied to the constructor defaults during deep clone, causing native/JS desync; implement a clone hook to recreate/sync the native material: add a method PhysicsMaterial._cloneTo(target: PhysicsMaterial) that uses target._staticFriction, target._dynamicFriction, target._bounciness, target._bounceCombine and target._frictionCombine to recreate or update target._nativeMaterial via Engine._nativePhysics.createPhysicsMaterial (or appropriate native setters), so when CloneManager/ColliderShape._syncNative binds the material the native instance matches the cloned JS properties; alternatively, set PhysicsMaterial’s clone mode to Assignment if native sharing is intended.
🧹 Nitpick comments (1)
packages/core/src/clone/CloneManager.ts (1)
119-138: 💤 Low valueMap/Set contents are remapped but not deep-cloned, unlike Array elements.
Arrays recursively call
clonePropertyfor each element (line 148), which applies the full classify-then-clone gate. In contrast,MapandSetonly call_remapIfPossible, which remaps Entity/Component references but leaves plain objects and arrays shared with the source.If this asymmetry is intentional (per the design doc: "Map/Set → new container + per-element remap"), consider adding a brief comment clarifying that Map/Set values are shallow-copied except for Entity/Component remaps, while Arrays are deep-cloned element-wise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/clone/CloneManager.ts` around lines 119 - 138, Map and Set contents are only remapped via CloneManager._remapIfPossible instead of being passed through the full clone pipeline (CloneManager.cloneProperty), causing plain objects/arrays to remain shared; update the Map branch to call CloneManager.cloneProperty for both key and value and the Set branch to call CloneManager.cloneProperty for each value (instead of _remapIfPossible) so their elements are cloned the same way Arrays are, or if shallow-remap behavior is intentional, add a concise comment in CloneManager near the Map/Set handling explaining that Map/Set elements are intentionally shallow-copied except for entity/component remaps done by _remapIfPossible.
🤖 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/2d/text/TextRenderer.ts`:
- Around line 38-39: The class-level `@property` decorators on _font and _subFont
cause populate to copy references directly and bypass the font setter/refcount
logic, leaking the constructor-incremented engine._textDefaultFont; remove the
`@property` annotation from _font (and _subFont as suggested) so that assignments
go through the public font setter, keep the constructor as-is (it increments the
default font refcount) and ensure _cloneTo continues to perform target.font =
this._font (and target._subFont = this._subFont only if necessary) so refcounts
are correctly released/retained via the setter logic.
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 58-90: The cloned ParticleGenerator currently only calls
target._setTransformFeedback() in ParticleGenerator._cloneTo(), so module
instances retain references to the original generator; update the module cloning
so each cloned module's internal _generator points to the new generator. Modify
ParticleGeneratorModule cloning behavior (e.g., its _cloneTo or clone metadata
for the _generator field) so when MainModule._cloneTo,
RotationOverLifetimeModule, ColorOverLifetimeModule, etc. are cloned their
_generator property is rebound to the cloned ParticleGenerator (use target as
the generator reference), and ensure any module clone paths that rely on
target._generator._generateTransformedBounds() will call the cloned generator's
_generateTransformedBounds() rather than the original.
In `@packages/core/src/physics/joint/HingeJoint.ts`:
- Around line 22-25: Cloning currently bypasses the motor/limits setters so the
cloned HingeJoint's listeners (_onMotorChanged/_onLimitsChanged) aren't
re-registered for the cloned _jointMotor/_limits; override the clone hook
(implement _cloneTo or the component post-clone hook used by your
CloneManager/ComponentCloner) in HingeJoint to re-establish listeners on the
cloned instance by either assigning via the public setters (this.motor =
source._jointMotor; this.limits = source._limits) or by directly adding
listeners to the cloned _jointMotor._updateFlagManager and
_limits._updateFlagManager; also ensure any prior listeners are removed before
re-adding to avoid duplicates (use the same removal logic as in the motor/limits
setters).
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 14-15: The cloned MeshColliderShape doesn't increment the shared
ModelMesh reference or recreate its native shape because the clone-populate path
assigns _mesh directly instead of using the setter; override
MeshColliderShape._cloneTo to call target._mesh._addReferCount(1) when
target._mesh is present, then if target._mesh and
target._extractMeshData(target._mesh) return true call
target._createNativeShape(), and finally call super._cloneTo(target) to preserve
existing clone behavior.
In `@packages/core/src/postProcess/PostProcessEffectParameter.ts`:
- Around line 14-19: The _needLerp field lacks the `@property` decorator causing
clones to reset it to false; add the `@property` decorator to the protected
_needLerp declaration in the PostProcessEffectParameter class so cloned
instances preserve the source's _needLerp value and _lerp() behaves correctly
(update the line defining protected _needLerp = false to include `@property`).
In `@packages/core/src/shader/state/RenderState.ts`:
- Around line 17-31: RenderState's depthState/stencilState/rasterState are
`@property` but their classes (DepthState, StencilState, RasterState) expose no
`@property` fields or copyFrom, so CloneManager falls back to constructor
defaults; to fix, update each of DepthState, StencilState and RasterState to
either mark their internal serializable fields with `@property` (so CloneManager
will copy them) or implement a copyFrom(source:
DepthState|StencilState|RasterState) that copies all relevant settings, ensuring
these classes correctly transfer state when RenderState is cloned (leave
RenderState.renderQueueType and its `@property` intact).
In `@packages/ui/src/component/advanced/Button.ts`:
- Around line 6-7: The issue is that declaring readonly onClick = new Signal()
makes CloneManager reuse the existing Signal instance on the destination and
Signal._cloneTo only copies structured-binding listeners (methodName) so plain
function listeners are lost when Buttons are cloned; to fix, stop pre-allocating
a shared Signal on the prototype and instead initialize onClick per-instance
(e.g., remove the inline new Signal() and assign this.onClick = new Signal()
inside the Button constructor or an init method) so each Button gets its own
Signal instance and listeners (including plain function listeners) are preserved
when cloning; reference symbols: onClick property, Signal class/_cloneTo, and
CloneManager.cloneProperty.
In `@packages/ui/src/component/interactive/transition/Transition.ts`:
- Around line 18-25: The cloned Transition/SpriteTransition instances can lose
refercount balance because CloneManager assigns ReferResource fields (_normal,
_pressed, _hover, _disabled) directly (CloneMode.Assignment) without invoking
setters, so SpriteTransition.destroy() may decrement refs that were never
incremented; fix by adding a finalize/clone hook (e.g., add a protected
_cloneTo(target: this) or finalizeClone method on Transition/SpriteTransition)
that runs after CloneManager assignment and explicitly calls _addReferCount(1)
for each ReferResource field copied, or alternatively update CloneManager's
handling of CloneMode.Assignment for ReferResource to increment refs; reference
Transition, SpriteTransition, CloneManager, CloneMode.Assignment,
_onStateValueDirty, _addReferCount, and the _normal/_pressed/_hover/_disabled
fields when making the change.
---
Outside diff comments:
In `@packages/core/src/Entity.ts`:
- Around line 447-478: The index-based component mapping in
Entity._createCloneEntity assumes components are created in identical order; add
a development-only defensive check after obtaining cloneComponents to verify
that for each index i components[i].constructor ===
cloneComponents[i].constructor (or otherwise assert true) and surface a clear
error (throw or logger) if a mismatch is detected so violations (e.g.,
constructors that call entity.addComponent()) are caught early; keep this check
gated behind a dev flag or NODE_ENV !== 'production' and reference
Entity._createCloneEntity, componentConstructors, cloneMap, components and
cloneComponents when adding the assertion.
In `@packages/core/src/physics/PhysicsMaterial.ts`:
- Around line 9-33: PhysicsMaterial currently leaves _nativeMaterial tied to the
constructor defaults during deep clone, causing native/JS desync; implement a
clone hook to recreate/sync the native material: add a method
PhysicsMaterial._cloneTo(target: PhysicsMaterial) that uses
target._staticFriction, target._dynamicFriction, target._bounciness,
target._bounceCombine and target._frictionCombine to recreate or update
target._nativeMaterial via Engine._nativePhysics.createPhysicsMaterial (or
appropriate native setters), so when CloneManager/ColliderShape._syncNative
binds the material the native instance matches the cloned JS properties;
alternatively, set PhysicsMaterial’s clone mode to Assignment if native sharing
is intended.
---
Nitpick comments:
In `@packages/core/src/clone/CloneManager.ts`:
- Around line 119-138: Map and Set contents are only remapped via
CloneManager._remapIfPossible instead of being passed through the full clone
pipeline (CloneManager.cloneProperty), causing plain objects/arrays to remain
shared; update the Map branch to call CloneManager.cloneProperty for both key
and value and the Set branch to call CloneManager.cloneProperty for each value
(instead of _remapIfPossible) so their elements are cloned the same way Arrays
are, or if shallow-remap behavior is intentional, add a concise comment in
CloneManager near the Map/Set handling explaining that Map/Set elements are
intentionally shallow-copied except for entity/component remaps done by
_remapIfPossible.
🪄 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: 1578ccd5-f08d-4545-b461-f6b606aa5bb0
📒 Files selected for processing (93)
packages/core/src/2d/sprite/Sprite.tspackages/core/src/2d/sprite/SpriteMask.tspackages/core/src/2d/sprite/SpriteRenderer.tspackages/core/src/2d/text/SubFont.tspackages/core/src/2d/text/TextRenderer.tspackages/core/src/Camera.tspackages/core/src/Component.tspackages/core/src/Entity.tspackages/core/src/Renderer.tspackages/core/src/Script.tspackages/core/src/Signal.tspackages/core/src/Transform.tspackages/core/src/VirtualCamera.tspackages/core/src/animation/Animator.tspackages/core/src/asset/ReferResource.tspackages/core/src/audio/AudioSource.tspackages/core/src/base/EngineObject.tspackages/core/src/clone/CloneManager.tspackages/core/src/clone/ComponentCloner.tspackages/core/src/clone/enums/CloneMode.tspackages/core/src/env-probe/Probe.tspackages/core/src/lighting/DirectLight.tspackages/core/src/lighting/Light.tspackages/core/src/lighting/PointLight.tspackages/core/src/lighting/SpotLight.tspackages/core/src/mesh/MeshRenderer.tspackages/core/src/mesh/Skin.tspackages/core/src/mesh/SkinnedMeshRenderer.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/ParticleRenderer.tspackages/core/src/particle/modules/Burst.tspackages/core/src/particle/modules/ColorOverLifetimeModule.tspackages/core/src/particle/modules/EmissionModule.tspackages/core/src/particle/modules/ForceOverLifetimeModule.tspackages/core/src/particle/modules/LimitVelocityOverLifetimeModule.tspackages/core/src/particle/modules/MainModule.tspackages/core/src/particle/modules/NoiseModule.tspackages/core/src/particle/modules/ParticleCompositeCurve.tspackages/core/src/particle/modules/ParticleCompositeGradient.tspackages/core/src/particle/modules/ParticleCurve.tspackages/core/src/particle/modules/ParticleGeneratorModule.tspackages/core/src/particle/modules/ParticleGradient.tspackages/core/src/particle/modules/RotationOverLifetimeModule.tspackages/core/src/particle/modules/SizeOverLifetimeModule.tspackages/core/src/particle/modules/TextureSheetAnimationModule.tspackages/core/src/particle/modules/VelocityOverLifetimeModule.tspackages/core/src/particle/modules/shape/BaseShape.tspackages/core/src/particle/modules/shape/BoxShape.tspackages/core/src/particle/modules/shape/CircleShape.tspackages/core/src/particle/modules/shape/ConeShape.tspackages/core/src/particle/modules/shape/HemisphereShape.tspackages/core/src/particle/modules/shape/MeshShape.tspackages/core/src/particle/modules/shape/SphereShape.tspackages/core/src/physics/CharacterController.tspackages/core/src/physics/Collider.tspackages/core/src/physics/DynamicCollider.tspackages/core/src/physics/PhysicsMaterial.tspackages/core/src/physics/joint/HingeJoint.tspackages/core/src/physics/joint/Joint.tspackages/core/src/physics/joint/JointLimits.tspackages/core/src/physics/joint/JointMotor.tspackages/core/src/physics/joint/SpringJoint.tspackages/core/src/physics/shape/BoxColliderShape.tspackages/core/src/physics/shape/CapsuleColliderShape.tspackages/core/src/physics/shape/ColliderShape.tspackages/core/src/physics/shape/MeshColliderShape.tspackages/core/src/physics/shape/SphereColliderShape.tspackages/core/src/postProcess/PostProcess.tspackages/core/src/postProcess/PostProcessEffect.tspackages/core/src/postProcess/PostProcessEffectParameter.tspackages/core/src/postProcess/effects/BloomEffect.tspackages/core/src/postProcess/effects/TonemappingEffect.tspackages/core/src/shader/Shader.tspackages/core/src/shader/ShaderData.tspackages/core/src/shader/ShaderMacro.tspackages/core/src/shader/ShaderTagKey.tspackages/core/src/shader/state/BlendState.tspackages/core/src/shader/state/RenderState.tspackages/core/src/trail/TrailRenderer.tspackages/ui/src/component/UICanvas.tspackages/ui/src/component/UIGroup.tspackages/ui/src/component/UIRenderer.tspackages/ui/src/component/UITransform.tspackages/ui/src/component/advanced/Button.tspackages/ui/src/component/advanced/Image.tspackages/ui/src/component/advanced/Text.tspackages/ui/src/component/interactive/UIInteractive.tspackages/ui/src/component/interactive/transition/Transition.tstests/src/core/CloneUtils.test.tstests/src/core/Signal.test.tstests/src/core/Transform.test.tstests/src/core/particle/ParticleShapeTransform.test.tstests/src/loader/PrefabResource.test.ts
💤 Files with no reviewable changes (4)
- packages/core/src/base/EngineObject.ts
- packages/core/src/Script.ts
- packages/core/src/clone/enums/CloneMode.ts
- packages/core/src/2d/sprite/Sprite.ts
✅ Files skipped from review due to trivial changes (1)
- tests/src/core/particle/ParticleShapeTransform.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/asset/ReferResource.ts
- packages/core/src/2d/text/SubFont.ts
- packages/core/src/shader/ShaderTagKey.ts
- packages/core/src/shader/Shader.ts
- packages/core/src/shader/ShaderMacro.ts
- packages/core/src/Component.ts
- packages/core/src/clone/ComponentCloner.ts
- packages/core/src/Signal.ts
| @property | ||
| useAutoRandomSeed = true; | ||
|
|
||
| /** Main module. */ | ||
| @deepClone | ||
| @property | ||
| readonly main: MainModule; | ||
| /** Emission module. */ | ||
| @deepClone | ||
| @property | ||
| readonly emission = new EmissionModule(this); | ||
| /** Velocity over lifetime module. */ | ||
| @deepClone | ||
| @property | ||
| readonly velocityOverLifetime: VelocityOverLifetimeModule; | ||
| /** Force over lifetime module. */ | ||
| @deepClone | ||
| @property | ||
| readonly forceOverLifetime: ForceOverLifetimeModule; | ||
| /** Limit velocity over lifetime module. */ | ||
| @deepClone | ||
| @property | ||
| readonly limitVelocityOverLifetime: LimitVelocityOverLifetimeModule; | ||
| /** Size over lifetime module. */ | ||
| @deepClone | ||
| @property | ||
| readonly sizeOverLifetime: SizeOverLifetimeModule; | ||
| /** Rotation over lifetime module. */ | ||
| @deepClone | ||
| @property | ||
| readonly rotationOverLifetime = new RotationOverLifetimeModule(this); | ||
| /** Color over lifetime module. */ | ||
| @deepClone | ||
| @property | ||
| readonly colorOverLifetime = new ColorOverLifetimeModule(this); | ||
| /** Texture sheet animation module. */ | ||
| @deepClone | ||
| @property | ||
| readonly textureSheetAnimation = new TextureSheetAnimationModule(this); | ||
| /** Noise module. */ | ||
| @deepClone | ||
| @property | ||
| readonly noise: NoiseModule; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ParticleGeneratorModule and subclasses handle _generator remapping in _cloneTo
rg -nP -A10 '_cloneTo\s*\(' --glob '**/particle/modules/*.ts' --glob '**/ParticleGenerator.ts'
# Check for `@defaultCloneMode` on ParticleGenerator to understand clone behavior
rg -nP '`@defaultCloneMode`|class ParticleGenerator' packages/core/src/particle/ParticleGenerator.ts
# Look for _generator field declarations and usage patterns
ast-grep --pattern $'class $_ extends ParticleGeneratorModule {
$$$
}'Repository: galacean/engine
Length of output: 50372
Fix cloned particle module generator back-references
ParticleGenerator._cloneTo() only calls target._setTransformFeedback() and doesn’t rebind module instances to the cloned ParticleGenerator. Meanwhile module cloning code (e.g., MainModule._cloneTo()) uses target._generator._generateTransformedBounds(), so cloned modules must have their internal _generator pointing at the cloned generator. Update ParticleGeneratorModule cloning (e.g., via _cloneTo and/or cloning metadata for the _generator field / default clone mode) to preserve this back-reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/particle/ParticleGenerator.ts` around lines 58 - 90, The
cloned ParticleGenerator currently only calls target._setTransformFeedback() in
ParticleGenerator._cloneTo(), so module instances retain references to the
original generator; update the module cloning so each cloned module's internal
_generator points to the new generator. Modify ParticleGeneratorModule cloning
behavior (e.g., its _cloneTo or clone metadata for the _generator field) so when
MainModule._cloneTo, RotationOverLifetimeModule, ColorOverLifetimeModule, etc.
are cloned their _generator property is rebound to the cloned ParticleGenerator
(use target as the generator reference), and ensure any module clone paths that
rely on target._generator._generateTransformedBounds() will call the cloned
generator's _generateTransformedBounds() rather than the original.
| @property | ||
| private _mesh: ModelMesh = null; |
There was a problem hiding this comment.
Missing reference count increment for cloned mesh.
The _mesh field is marked with @property for cloning. Based on the PR architecture (ModelMesh as ReferResource → Assignment mode), the mesh reference will be shared between source and cloned shapes. However, the mesh setter (lines 78-79) manages reference counting via _addReferCount, and during the clone populate phase, the field is directly assigned without calling the setter.
Override _cloneTo to increment the reference count for the shared mesh and handle native shape recreation:
override _cloneTo(target: MeshColliderShape): void {
// Increment ref count for shared mesh
if (target._mesh) {
target._mesh._addReferCount(1);
}
// Recreate native shape if mesh data is available
if (target._mesh && target._extractMeshData(target._mesh)) {
target._createNativeShape();
}
super._cloneTo(target);
}As per the CloneManager contract: "_cloneTo post-clone hook... (native sync, refcount, update-flag, derived state, cache rebuild)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/physics/shape/MeshColliderShape.ts` around lines 14 - 15,
The cloned MeshColliderShape doesn't increment the shared ModelMesh reference or
recreate its native shape because the clone-populate path assigns _mesh directly
instead of using the setter; override MeshColliderShape._cloneTo to call
target._mesh._addReferCount(1) when target._mesh is present, then if
target._mesh and target._extractMeshData(target._mesh) return true call
target._createNativeShape(), and finally call super._cloneTo(target) to preserve
existing clone behavior.
| @property | ||
| enabled = true; | ||
|
|
||
| protected _needLerp = false; | ||
| @property | ||
| protected _value: T; |
There was a problem hiding this comment.
Missing @property on _needLerp may cause incorrect clone behavior.
The _needLerp field controls interpolation behavior in _lerp(). Without @property, cloned instances will use the field initializer (false) instead of the source's value. For parameters constructed with needLerp = true (e.g., PostProcessEffectFloatParameter), clones will incorrectly skip lerping.
Proposed fix
+ `@property`
protected _needLerp = false;
`@property`
protected _value: T;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/postProcess/PostProcessEffectParameter.ts` around lines 14
- 19, The _needLerp field lacks the `@property` decorator causing clones to reset
it to false; add the `@property` decorator to the protected _needLerp declaration
in the PostProcessEffectParameter class so cloned instances preserve the
source's _needLerp value and _lerp() behaves correctly (update the line defining
protected _needLerp = false to include `@property`).
| @property | ||
| readonly blendState: BlendState = new BlendState(); | ||
| /** Depth state. */ | ||
| @deepClone | ||
| @property | ||
| readonly depthState: DepthState = new DepthState(); | ||
| /** Stencil state. */ | ||
| @deepClone | ||
| @property | ||
| readonly stencilState: StencilState = new StencilState(); | ||
| /** Raster state. */ | ||
| @deepClone | ||
| @property | ||
| readonly rasterState: RasterState = new RasterState(); | ||
|
|
||
| /** Render queue type. */ | ||
| @property | ||
| renderQueueType: RenderQueueType = RenderQueueType.Opaque; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm nested render-state classes opt fields into the new clone model.
for f in BlendState DepthState StencilState RasterState; do
echo "===== $f ====="
fd -i "${f}.ts" --type f --exec sh -c 'rg -nP "`@property`|copyFrom\s*\(" "{}" || echo " (no `@property` / copyFrom found)"'
doneRepository: galacean/engine
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect RenderState.ts clone/deepClone/populate logic
echo "===== RenderState.ts relevant clone logic ====="
fd -a "RenderState.ts" packages/core/src/shader/state --type f --exec sh -c 'echo "--- {} ---"; rg -n "deepClone|clone|populate|Populate|copyFrom|`@property`" "{}" -n' \; || true
# Show top portion around lines 1-220 (should include clone-related code if present)
fd -a "RenderState.ts" packages/core/src/shader/state --type f -0 | xargs -0 -I{} sh -c 'echo "===== {} (head 240) ====="; sed -n "1,240p" "{}"'
# 2) Locate class definitions for DepthState/StencilState/RasterState and check for copyFrom or other clone helpers
for f in DepthState StencilState RasterState; do
echo "===== $f.ts content (first 220 lines) ====="
fd -i "${f}.ts" --type f --exec sh -c 'echo "--- {} ---"; sed -n "1,220p" "{}"' \;
done
# 3) Also search for copyFrom usage for each state in the repo (in case they have copyFrom in other files)
for f in DepthState StencilState RasterState; do
echo "===== repo rg for '${f}.*copyFrom' or 'copyFrom.*${f}' ====="
rg -n "copyFrom.*${f}|${f}.*copyFrom|class ${f}|new ${f}\(" packages/core/src/shader/state packages/core/src -S || true
doneRepository: galacean/engine
Length of output: 22929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect RenderState.ts clone/deepClone/populate logic
echo "===== RenderState.ts relevant clone logic ====="
fd -a "RenderState.ts" packages/core/src/shader/state --type f --exec sh -c 'echo "--- {} ---"; rg -n "deepClone|clone|populate|Populate|copyFrom|`@property`" "{}" -n' \; || true
# Show top portion around lines 1-240 (should include clone-related code if present)
fd -a "RenderState.ts" packages/core/src/shader/state --type f -0 | xargs -0 -I{} sh -c 'echo "===== {} (head 240) ====="; sed -n "1,240p" "{}"'
# 2) Locate class definitions for DepthState/StencilState/RasterState and check for copyFrom or other clone helpers
for f in DepthState StencilState RasterState; do
echo "===== $f.ts content (first 220 lines) ====="
fd -i "${f}.ts" --type f --exec sh -c 'echo "--- {} ---"; sed -n "1,220p" "{}"' \;
done
# 3) Also search for copyFrom usage for each state in the repo (in case they have copyFrom in other files)
for f in DepthState StencilState RasterState; do
echo "===== repo rg for '${f}.*copyFrom' or 'copyFrom.*${f}' ====="
rg -n "copyFrom.*${f}|${f}.*copyFrom|class ${f}|new ${f}\(" packages/core/src/shader/state packages/core/src -S || true
doneRepository: galacean/engine
Length of output: 22929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate CloneManager implementation
echo "===== Locate CloneManager.ts ====="
fd -a "CloneManager.ts" packages/core/src --type f
# Inspect CloneManager + clone/deepClone logic (limit output)
echo "===== Inspect CloneManager.ts (first 260 lines) ====="
fd -a "CloneManager.ts" packages/core/src --type f -0 | xargs -0 -I{} sh -c 'echo "---- {} ----"; sed -n "1,260p" "{}"'
# Search for deepClone / populate / copyFrom usage in clone system
echo "===== Search clone system for deepClone / populate / copyFrom ====="
rg -n "deepClone|populate|Populate|copyFrom\s*\(|`@property`" packages/core/src/clone packages/core/src --S || true
# Find where `property` decorator stores metadata and how it decides which fields to copy
echo "===== Search for decorator handling: 'property' in clone system ====="
rg -n "function property|export function property|const property|property\s*\(" packages/core/src/clone packages/core/src -S || trueRepository: galacean/engine
Length of output: 13049
Fix RenderState deep-clone of depth/stencil/raster state
CloneManager only copies class-instance fields that are either copyFrom’able or marked with @property. While RenderState.ts marks depthState/stencilState/rasterState as @property, DepthState.ts, StencilState.ts, and RasterState.ts contain no @property fields and no copyFrom, so cloned RenderState will keep default constructor values instead of the source’s modified depth/stencil/raster settings. Add @property to the relevant fields (or implement copyFrom) in each of these state classes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/shader/state/RenderState.ts` around lines 17 - 31,
RenderState's depthState/stencilState/rasterState are `@property` but their
classes (DepthState, StencilState, RasterState) expose no `@property` fields or
copyFrom, so CloneManager falls back to constructor defaults; to fix, update
each of DepthState, StencilState and RasterState to either mark their internal
serializable fields with `@property` (so CloneManager will copy them) or implement
a copyFrom(source: DepthState|StencilState|RasterState) that copies all relevant
settings, ensuring these classes correctly transfer state when RenderState is
cloned (leave RenderState.renderQueueType and its `@property` intact).
| @property | ||
| readonly onClick = new Signal<[PointerEventData]>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Signal's clone mode/default and copyFrom semantics.
fd -t f 'Signal.ts' -p 'packages/core' --exec cat {}
rg -nP 'defaultCloneMode|copyFrom|_cloneTo' -g '**/Signal.ts' -C2Repository: galacean/engine
Length of output: 6976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Button.ts and inspect around onClick
fd -t f 'Button.ts' -p 'packages/ui/src/component/advanced' --exec sh -c 'echo "---- $1"; sed -n "1,120p" "$1"' _ {}
# Find where Signal._cloneTo is referenced (copyFrom/clone logic)
fd -t f '*.ts' -p 'packages' --exec rg -n "(_cloneTo\\(|Signal\\.)" {} \; | head -n 200
# Search for copyFrom / deepClone / Populate / Finalize in the clone/copy system
rg -n "copyFrom\\(|deepClone\\(|Populate\\b|Finalize\\b" packages | head -n 200
# Narrow search for any cloning adapter that handles Signal specially
rg -n "Signal<|instanceof Signal|_cloneTo\\(target: Signal|cloneMap" packages | head -n 200Repository: galacean/engine
Length of output: 1383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Button.ts around onClick
sed -n '1,120p' packages/ui/src/component/advanced/Button.ts
# Find cloning/copy pipeline pieces
rg -n --glob='*.ts' "copyFrom\\(|deepClone\\(|\\bPopulate\\b|\\bFinalize\\b" packages | head -n 200
# Find Signal cloning internals usage
rg -n --glob='*.ts' "_cloneTo\\(|_cloneArguments\\(|_cloneTo\\(target: Signal|cloneMap" packages | head -n 200
# Find any usage of Signal in clone/copy adapters
rg -n --glob='*.ts' "instanceof Signal|new Signal\\(|Signal<" packages | head -n 200Repository: galacean/engine
Length of output: 27066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 8530
Fix rationale: onClick cloning reuses the existing Signal, but only clones structured-binding listeners
CloneManager.cloneProperty deep-clones @property fields by reusing the pre-allocated destination instance when target[k] already has the same constructor, then calls the source object's _cloneTo hook on it (not via copyFrom). For Signal, _cloneTo only recreates listeners that have methodName (structured binding via target.on(targetComponent, methodName, ...)) and skips plain function listeners where methodName is absent—so cloned Buttons won’t inherit the source’s function callbacks, while structured-binding listeners are preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/component/advanced/Button.ts` around lines 6 - 7, The issue
is that declaring readonly onClick = new Signal() makes CloneManager reuse the
existing Signal instance on the destination and Signal._cloneTo only copies
structured-binding listeners (methodName) so plain function listeners are lost
when Buttons are cloned; to fix, stop pre-allocating a shared Signal on the
prototype and instead initialize onClick per-instance (e.g., remove the inline
new Signal() and assign this.onClick = new Signal() inside the Button
constructor or an init method) so each Button gets its own Signal instance and
listeners (including plain function listeners) are preserved when cloning;
reference symbols: onClick property, Signal class/_cloneTo, and
CloneManager.cloneProperty.
| @property | ||
| protected _normal: T; | ||
| @property | ||
| protected _pressed: T; | ||
| @property | ||
| protected _hover: T; | ||
| @property | ||
| protected _disabled: T; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect clone populate/finalize handling and any refCount adjustment for assignment-mode ReferResource fields.
fd -t f 'CloneManager.ts' --exec cat {}
fd -t f 'ComponentCloner.ts' --exec cat {}
fd -t f 'ReferResource.ts' -p 'packages/core' --exec cat {}
# Look for refCount handling tied to clone
rg -nP '_addReferCount|_cloneTo|CloneMode\.Assignment' -g '!**/*.test.ts' -C2Repository: galacean/engine
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on transition implementation and refcount handling
echo "---- Transition.ts key sections (clone/finalize/state dirty) ----"
rg -n "_cloneTo|_onStateValueDirty|_normal|_pressed|_hover|_disabled|_finalState|InteractiveState" packages/ui/src/component/interactive/transition/Transition.ts
echo
# Print relevant line ranges around clone/finalize and dirty handler
python3 - <<'PY'
import re, itertools, os, sys, pathlib
path="packages/ui/src/component/interactive/transition/Transition.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
keys=["_cloneTo","_onStateValueDirty","_normal","_pressed","_hover","_disabled","_finalState"]
# find line numbers for any key
hits=[]
for i,l in enumerate(lines, start=1):
if any(k in l for k in keys):
hits.append(i)
# cluster and print around key areas
printed=set()
for i in sorted(set(hits)):
start=max(1,i-20); end=min(len(lines),i+20)
if any((s,e) and (max(s,i-20)<=min(e,i+20))) for s,e in []:
pass
rng=(start,end)
if rng in printed:
continue
printed.add(rng)
print(f"\n### {path}:{start}-{end}")
for ln in range(start,end+1):
sys.stdout.write(f"{ln:4d}: {lines[ln-1]}")
PY
echo
echo "---- SpriteTransition.ts (clone/destroy/refcount) ----"
rg -n "_cloneTo|destroy|_addReferCount|_normal|_pressed|_hover|_disabled" packages/ui/src/component/interactive/transition/SpriteTransition.ts
echo
python3 - <<'PY'
path="packages/ui/src/component/interactive/transition/SpriteTransition.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
# print whole file if small
print(f"File lines: {len(lines)}")
start=1; end=min(len(lines),250)
for ln in range(start,end+1):
sys.stdout.write(f"{ln:4d}: {lines[ln-1]}")
PY
echo
echo "---- Find any other _cloneTo in transition folder ----"
fd -t f packages/ui/src/component/interactive/transition -x echo {} > /tmp/transition_files.txt || true
# list
cat /tmp/transition_files.txt
rg -n "_cloneTo" packages/ui/src/component/interactive/transition -SRepository: galacean/engine
Length of output: 1731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/ui/src/component/interactive/transition/Transition.ts"
echo "==== $FILE (head) ===="
sed -n '1,240p' "$FILE" | nl -ba | sed -n '1,240p'
echo
echo "==== $FILE (_cloneTo / clone / finalize / _setState / _onStateValueDirty) ===="
rg -n "_cloneTo|_clone|finalize|_onStateValueDirty|_setState|_finalState|_finalValue" "$FILE"
echo
echo "==== SpriteTransition.ts ===="
FILE2="packages/ui/src/component/interactive/transition/SpriteTransition.ts"
sed -n '1,260p' "$FILE2" | nl -ba | sed -n '1,260p'
echo
echo "==== transition folder: any _cloneTo overrides ===="
rg -n "override _cloneTo|_cloneTo\(" packages/ui/src/component/interactive/transition -S
echo
echo "==== search for _onStateValueDirty call sites ===="
rg -n "_onStateValueDirty\(" packages/ui/src/component/interactive/transition -SRepository: galacean/engine
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/ui/src/component/interactive/transition/Transition.ts"
FILE2="packages/ui/src/component/interactive/transition/SpriteTransition.ts"
echo "== $FILE =="
wc -l "$FILE"
cat -n "$FILE" | sed -n '1,260p'
echo
echo "== refs around dirty handler =="
rg -n "onStateValueDirty|_finalState|_finalValue|_setState|_cloneTo|clone|finalize" "$FILE" -n -S || true
echo
echo "== $FILE2 =="
wc -l "$FILE2"
cat -n "$FILE2" | sed -n '1,260p'
echo
echo "== clone overrides inside transition folder =="
fd -t f packages/ui/src/component/interactive/transition -x echo {} | head -n 50
rg -n "override _cloneTo|_cloneTo\(|_clone\(" packages/ui/src/component/interactive/transition -S || true
echo
echo "== refcount calls inside transition folder =="
rg -n "_addReferCount\(|_onDestroy|destroy\(" packages/ui/src/component/interactive/transition -S || trueRepository: galacean/engine
Length of output: 9438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where transitions are stored in UIInteractive and how cloning is handled
rg -n "class UIInteractive|_cloneTo\(|clone|Transition|SpriteTransition|normal|pressed|hover|disabled" packages/ui/src/component/interactive/UIInteractive.ts packages/ui/src/component/interactive -S || true
# List possible clone hooks in interactive components
rg -n "_cloneTo\\(" packages/ui/src/component/interactive -S || true
# Search for Transition references
rg -n "SpriteTransition|new SpriteTransition|removeTransition\\(|addTransition\\(|_transitions" packages/ui/src/component/interactive -S || true
# Also search UIInteractive.ts itself if path differs
fd -t f "UIInteractive.ts" packages/ui/src/component/interactive -d 4
if [ -f "packages/ui/src/component/interactive/UIInteractive.ts" ]; then
wc -l packages/ui/src/component/interactive/UIInteractive.ts
cat -n packages/ui/src/component/interactive/UIInteractive.ts | sed -n '1,260p'
fi
# search for clone implementation in UIInteractive's owner (if any)
rg -n "_cloneTo\\(" packages/ui/src/component -S | rg -n "UIInteractive|Interactive" || trueRepository: galacean/engine
Length of output: 19432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate UIInteractive implementation
fd -t f UIInteractive packages/ui/src/component | head -n 20
# Inspect UIInteractive and its clone logic
UIFILE=$(fd -t f -i "UIInteractive.ts" packages/ui/src/component | head -n 1 || true)
echo "UIFILE=$UIFILE"
if [ -n "${UIFILE}" ] && [ -f "${UIFILE}" ]; then
wc -l "${UIFILE}"
cat -n "${UIFILE}" | sed -n '1,320p'
fi
echo
echo "---- Search for transition storage + clone hooks ----"
# Search whole interactive directory for transition members & clone
rg -n "Transition<|SpriteTransition|_transit|removeTransition|addTransition|_cloneTo\\(|clone" packages/ui/src/component/interactive -S || true
echo
echo "---- Search for destroy/cleanup patterns that might balance refcounts on clone ----"
rg -n "refCount|_addReferCount\\(|destroy\\(" packages/ui/src/component/interactive -S || trueRepository: galacean/engine
Length of output: 11681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate UIInteractive file
UIFILE=$(fd -t f -i "UIInteractive.ts" packages/ui/src/component | head -n 1 || true)
echo "UIFILE=$UIFILE"
if [ -n "${UIFILE}" ] && [ -f "${UIFILE}" ]; then
wc -l "${UIFILE}"
cat -n "${UIFILE}" | sed -n '1,360p'
fi
echo
echo "---- Find transition storage + clone hooks in interactive folder ----"
rg -n "SpriteTransition|Transition<|removeTransition\\(|addTransition\\(|_transit|_cloneTo\\(|clone" packages/ui/src/component/interactive -S || true
echo
echo "---- Find any refcount fixups related to transitions ----"
rg -n "_addReferCount\\(|refCount" packages/ui/src/component/interactive -S || trueRepository: galacean/engine
Length of output: 11244
Fix possible ReferResource refcount imbalance when cloning Transition state fields (SpriteTransition).
Transitionmarks_normal/_pressed/_hover/_disabledas@property, soCloneManagerclones them when cloning aUIInteractive(its@property _transitionsarray).CloneManagerhandlesReferResource/SpritewithCloneMode.Assignmentby direct assignment (target[k] = sourceProperty) without invokingTransitionsetters, so_onStateValueDirtynever runs for the clone.SpriteTransition.destroy()unconditionally calls_addReferCount(-1)for_normal/_hover/_pressed/_disabled, so a clonedSpriteTransitioncan release a sharedSpritewithout having incremented its refcount, risking premature release / incorrect refcounts.
Add a _cloneTo (or equivalent finalize) on SpriteTransition/Transition to _addReferCount(1) for copied ReferResource state fields (or adjust cloning behavior for CloneMode.Assignment ReferResource). [packages/ui/src/component/interactive/transition/Transition.ts:163-171, packages/ui/src/component/interactive/transition/SpriteTransition.ts:12-34]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/component/interactive/transition/Transition.ts` around lines
18 - 25, The cloned Transition/SpriteTransition instances can lose refercount
balance because CloneManager assigns ReferResource fields (_normal, _pressed,
_hover, _disabled) directly (CloneMode.Assignment) without invoking setters, so
SpriteTransition.destroy() may decrement refs that were never incremented; fix
by adding a finalize/clone hook (e.g., add a protected _cloneTo(target: this) or
finalizeClone method on Transition/SpriteTransition) that runs after
CloneManager assignment and explicitly calls _addReferCount(1) for each
ReferResource field copied, or alternatively update CloneManager's handling of
CloneMode.Assignment for ReferResource to increment refs; reference Transition,
SpriteTransition, CloneManager, CloneMode.Assignment, _onStateValueDirty,
_addReferCount, and the _normal/_pressed/_hover/_disabled fields when making the
change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3018 +/- ##
===========================================
+ Coverage 77.31% 77.50% +0.18%
===========================================
Files 914 913 -1
Lines 101723 101510 -213
Branches 10437 10400 -37
===========================================
+ Hits 78647 78674 +27
+ Misses 22892 22652 -240
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… through one gate Addresses CR feedback on the clone system: - `_cloneTo` only fired for deep clone, so a shared (Assignment) ReferResource never got a ref-count bump on clone -> underflow on the clone's destroy. Bump +1 centrally in the cloner's Assignment branch (no-op for non-ref-counted flyweights) and drop the now-redundant manual bumps in Camera/AudioSource `_cloneTo`. Fixes the same latent leak in TextRenderer/SpriteTransition/etc. for free. - Map/Set members went through a remap-only shortcut (values shared, never deep-cloned or ref-counted). Unify property / Array / Map / Set members through one `_cloneValue` gate. - Register Array/Map/Set in the identity map before recursing, so shared containers dedup and self-referential containers no longer infinite-loop. Verified: full build + core suite (845 tests) pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Structure-only cleanup of the clone core (no behavior change): - `_cloneValue` is now a thin per-reference gate (primitive / function / share / remap / deep); the recursion lives entirely in `_deepClone`, so nested-object handling reads in one place. - Containers (Array/Map/Set) always allocate fresh; `reuse` is kept only for value types (copyFrom in place, preserving constructor-bound callbacks) and typed-array buffers. Verified: build + core suite (845) pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…tor._cloneTo The clone gate now bumps shared (Assignment) ReferResources centrally, so the manual bump in Animator._cloneTo double-counted -> AnimatorController leaked (+1, never freed). Keep only the change-flag re-registration; add a clone ref-count regression test. Fixes the P1 from review on 3b12859. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Advanced users authoring a custom shareable type need to declare its clone behavior, so expose `@defaultCloneMode` + `CloneMode` (re-exported from the clone entry; reference docs added). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Opt-in populate writes @Property fields directly, bypassing setters that perform refcount/native side effects; a compensating _cloneTo setter then no-ops because populate already made source and target equal. - TextRenderer/Text `_font` (+ `_subFont`): drop @Property so `_cloneTo`'s `target.font = this._font` setter is the sole, refcount-correct path. Previously leaked engine._textDefaultFont +1 per clone (the constructor bumps the default font, the no-op setter never released it). - PhysicsMaterial: add `_cloneTo` to push the cloned JS values into the clone's own native material; populate bypasses the native-syncing setters, so a cloned collider's friction/bounciness otherwise stayed at native defaults. - Add a reverse-falsification regression test for the default-font ref-count leak. Verified: 107 related tests pass (TextRenderer / ui-Text / PhysicsMaterial PhysX / CloneUtils / PrefabResource); core + ui type-check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s copy* - Export CloneMode directly from index.ts and drop the re-export laundered through CloneManager. Public API is unchanged (CloneMode still appears once at the package root); internal consumers already import it from ./enums/CloneMode. - Rename the "write into an existing target" helpers to copy*, matching the cross-engine convention where clone/duplicate/instantiate mean "return a new instance" and copy/assign mean "fill an existing target" (W3C Object.assign, Unity EditorUtility.CopySerialized, UE CopyPropertiesForUnrelatedObjects): cloneProperty -> copyProperty, deepCloneObject/cloneAllProperties -> copyProperties. Value-returning helpers keep clone (_cloneValue, _deepClone, entity.clone()). Pure export/naming change, no behavior change; type-check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The Assignment branch now releases the clone's reused slot value before acquiring the shared source value (release-old + acquire-new), so the gate alone keeps ref counts balanced for any @Property Assignment field whose constructor pre-set a default. With that, TextRenderer/Text `_font`/`_subFont` go back to plain @Property and their `_cloneTo` overrides are deleted — they only re-applied the font through the setter to release the constructor-allocated default font, which the gate now does. Supersedes the per-field opt-out from the previous fix; the default-font ref-count regression test now guards the gate path instead. Verified: 146 related tests pass (TextRenderer / ui-Text / CloneUtils / PrefabResource / Animator); type-check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cptbtptpbcptdtptp
left a comment
cptbtptpbcptdtptp
left a comment
There was a problem hiding this comment.
Code Review 深度审计
整体方向非常好——opt-in 模型比 opt-out 干净得多，identity-map 替代 path-based remap 也更优雅。以下是逐点审计结果。
1. ✅ RenderTarget 分支验证 — 通过
Camera._renderTarget 加了 @property，旧 _cloneTo 中的手动 _addReferCount(1) 被删除。验证了继承链：
RenderTarget → GraphicsResource → ReferResource (@defaultCloneMode(Assignment)) → EngineObject
原型链查 _defaultCloneMode 拿到 Assignment，走 _addReferCount(1); return value 分支。语义等价，更干净。
同时验证了所有常见资产类（Texture/Texture2D/TextureCube/Material/Mesh/ModelMesh/AnimatorController/AudioClip/Sprite/SpriteAtlas/Font）都继承自 ReferResource，全部走 Assignment。Shader/ShaderMacro/ShaderTagKey/SubFont 自身挂了 @defaultCloneMode(Assignment)。全部正确。
2. ⚠️ opt-in 静默丢字段 — Camera 有 3 个回归
对 PR 涉及的所有非测试文件做了 decorator 计数对比（旧版 @assignmentClone + @deepClone + @shallowClone 数量 vs 新版 @property 数量），所有文件 DIFF 均为正（新版标注更多字段），系统性遗漏风险低。
但 Camera 中有 3 个旧版无装饰的字段（在旧 for k in source 下会被直接赋值克隆）未补 @property：
| 字段 | 行号 | 旧行为 | 新行为 |
|---|---|---|---|
_replacementShader |
L133 | 无装饰 → 直接赋值（共享引用） | 不克隆 → null |
_replacementSubShaderTag |
L135 | 同上 | 不克隆 → null |
_replacementFailureStrategy |
L137 | 同上 | 不克隆 → null |
这三个字段通过公开 API setReplacementShader() 设置。如果用户在 clone 前调了这个 API，clone 出来的 Camera 会丢失 replacement shader 设置。
建议补 @property：Shader 和 ShaderTagKey 都有 @defaultCloneMode(Assignment)，会走正确分支。ReplacementFailureStrategy 是 enum（primitive），直接赋值。
另外手动抽查了 Renderer、Light、SpotLight、PointLight、Transition、Skin 等文件——未加 @property 的字段全部是 transient runtime state（dirty flag、native handle、计时器、缓存等），新版不克隆反而更正确。
附带一个好消息：Skin._skinMatrices 旧版 @ignoreClone 导致的已知 bug（cloneEditor 中有明确的 _fixClonedSkins workaround），在新版中因为 _skinMatrices 不带 @property（transient），而 bones 带 @property 且 setter 会重建 _skinMatrices，所以这个旧 bug 大概率被修了——值得在 PR 描述里提一下。
3. ⚠️ _deepClone 对未知类型的静默降级
横向调研了 Cocos Creator、PlayCanvas、Unity、Babylon.js 的克隆系统对"不认识的类型"的处理：
| 引擎 | 内建类型 (Date/RegExp) | 函数字段 | 兜底策略 |
|---|---|---|---|
| Cocos | 共享引用（静默） | 共享引用 | return obj，无警告 |
| PlayCanvas | 无法声明为 attribute | 不克隆 | return value，无警告 |
| Unity | default(T) + Editor 警告 |
null | 跳过 + 控制台警告 |
| Babylon | 共享引用 / null | 显式 continue |
静默跳过 / Logger.Warn |
当前 _deepClone 通用分支做 new value.constructor() + 递归 @property 字段。对于 Date 这种"状态藏在 JS 内部 slot、无 copyFrom、无 @Property 字段"的类型，结果是一个空壳（new Date() = 当前时间）——静默给了一个错误值，比共享引用（值至少对）和给 null（明确不对）都差。
当前 codebase 不使用这些类型，实际风险为 0。但建议在 _deepClone 通用 Object 分支加一个防御性警告（与 Unity 策略对齐）：
// _deepClone 方法内，getProperties(...).forEach 之后、_cloneTo 之前
if (value.constructor !== Object &&
CloneManager.getProperties(value.constructor).size === 0 &&
!(<ICustomClone>value).copyFrom &&
!(<ICustomClone>value)._cloneTo) {
Logger.warn(`Clone: type ${value.constructor.name} has no @property fields and no copyFrom — the clone may be incomplete.`);
}这样未来开发者写了 @property date: Date 会立刻在控制台看到警告，而不是得到一个静默错误的值。不需要维护类型白名单。
4. ⚠️ CloneMode 枚举数值变更 — 下游编译破坏
枚举值变更（旧 Ignore=0/Assignment=1/Shallow=2/Deep=3 → 新 Assignment=0/Deep=1/Remap=2）：
runtime 仓库内：所有引用都是符号引用（CloneMode.Assignment 等），无数字字面量比较，无风险。
下游仓库：engine-lottie 有实际破坏：
// engine-lottie/src/LottieAnimation.ts:5
import { Script, Vector2, ignoreClone, Entity, Layer, Engine, Renderer } from "@galacean/engine";PR 删除了 ignoreClone 的导出，engine-lottie 编译会失败。需要同步升级（opt-in 模型下，去掉 @ignoreClone 即可——不标 @property = 不克隆）。
建议在 PR 描述加 BREAKING CHANGE 标签，标明：
ignoreClone/assignmentClone/deepClone/shallowClone装饰器函数删除CloneMode.Ignore/CloneMode.Shallow枚举成员删除CloneUtils类删除
5. ✅ 箭头函数实例字段 — 天然安全
_cloneValue 分支顺序（!(value instanceof Object) → typeof === "function" → 类型驱动分支）正确。但更关键的是：旧版带 @ignoreClone 的箭头函数字段在新版没有 @property，根本不在 getProperties() 集合中。new ctor() 时 class field 语法重新执行初始化器，箭头函数自动绑定到新 this。
总结
| 项目 | 状态 | 行动 |
|---|---|---|
| Camera replacement 字段 | 需修复 | 补 @property × 3 |
| _deepClone 未知类型兜底 | 建议增强 | 加 Logger.warn 防御 |
| BREAKING CHANGE 标记 | 需补充 | PR 描述 + engine-lottie 同步 |
| RenderTarget 分支 | ✅ | — |
| 箭头函数迁移 | ✅ | — |
| Animator/AudioSource refcount 修复 | ✅ | PR 描述里提一下 |
| Skin clone bug 顺手修复 | ✅ | PR 描述里提一下 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… MeshColliderShape shape) Same class as the earlier PhysicsMaterial fix — a cloned component's @Property fields are copied, but native/derived state that's normally built through setters is not, leaving it at constructor defaults. - ShaderData: add `_cloneTo` (delegates to the existing `cloneTo`) so a `@property` ShaderData field (Renderer/Camera) carries its macros + property values to the clone instead of being left at constructor defaults. Fixes set-once shader state lost on clone — e.g. Renderer receiveShadows, SpriteMask alphaCutoff, ParticleRenderer renderMode macro. - MeshColliderShape: add `_cloneTo` to cook a fresh native PhysX shape from the cloned mesh; the Collider re-attaches it via `_addNativeShape`. Without it the cloned shape had no collision geometry (objects fell through). - Tests: shaderData macros/values survive clone + receiveShadows macro state (CloneUtils); cloned MeshColliderShape rebuilds its native shape (MeshColliderShape); cloned collider shape material keeps native values (PhysicsMaterial — backfills the earlier R8 fix). Verified: relevant suites pass (CloneUtils / MeshColliderShape / PhysicsMaterial PhysX / TextRenderer / ui-Text / Animator / PrefabResource); type-check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ 79e1d2916（第十二轮）
自第十一轮（HEAD
f13b12814）以来作者新增 1 个 commit79e1d2916（rebuild native/derived state on clone）：给ShaderData与MeshColliderShape各加_cloneTo，并补了三类反向回归测试（含上一轮我提的 PhysicsMaterial native fix 测试 backfill）。本轮专审这个新 commit，并复核两个历史 open 项。作者本轮无 PR 评论回复。
已关闭问题清单（本轮新核验）
- [已解决] PhysicsMaterial native 同步无测试守护（十/十一轮 P2）— 本 commit 补了
cloned collider shape material keeps native values：clone 一个 bouncy DynamicCollider 给定速度撞 bouncy wall、跑 40 步仿真、断言linearVelocity.x < -1（弹回）。revert PhysicsMaterial_cloneTo→ clone native 停在 bounciness 0 → 不弹 → 速度仍为正 → 断言 fail。这是经 PhysX 仿真路径的真反向测试，正是我前两轮要的（读 native 行为而非 JS getter）。收口。 - [已解决/收敛] gate release-old + acquire-new、Animator 双重 bump、容器 dedup、CloneMode 公开化、TextRenderer 字体泄漏 — 本 commit 未触及，维持收口。
- [非问题] component 数组下标配对（CodeRabbit 反复 Critical）— 配对逻辑未变，多轮核验，不再提。
- [不重申] 历史 P3（
CloneUtils.test.ts文件名、defaultCloneMode缺@param mode、Entity.ts:456-458/ TextRenderer 测试注释句号、CustomDataModule._cloneTo丢 cloneMap）— 作者本轮未触及，按去重规则不重申。
核验 1：ShaderData _cloneTo 正确性（refcount 不双计）
落点正确、refcount 平衡。 _shaderData 是 @property 且 ShaderData 无 @defaultCloneMode → deep clone（object 分支）。ShaderData 零 @property 字段 → populate 为空集 no-op，全部由 _cloneTo → cloneTo 接管（与 round-5 的 CustomDataModule 同模式）。
逐链路核对 texture refcount：
- clone Renderer 构造期
_addResourceReferCount(this.shaderData, 1)→ clone shaderData_refCount=1；构造期只 set Vector4（非 refcounted），无预占 texture。 cloneTo里 texture 走referCount > 0 && property._addReferCount(referCount)，referCount = target._getReferCount() = 1→ 每个 source texture +1（仅此一次）；texture 存在_propertyValueMap（非@property），gate 从不直接碰，无第二次 bump。- clone destroy →
_addResourceReferCount(shaderData, -1)→ 级联 texture -1。净 0。无双计，JSDoc 的 "composes without double-counting" 属实。
receiveShadows 测试是好的反向证伪：receiveShadows=false 清掉 RENDERER_IS_RECEIVE_SHADOWS macro（构造期默认 enable），无 ShaderData clone 时 clone 的 shaderData macro 停在 ctor-enabled、而 _receiveShadows field clone 成 false → field/macro 分叉。断言两者都为 false，revert 即 fail。
核验 2：MeshColliderShape _cloneTo 正确性（refcount + native 重建 + 时序）
落点正确，是唯一不适配 _syncNative 模式的 shape 的针对性修复。 对照 Box/Sphere/Capsule/Plane：它们在构造期建 native shape，且各自 override _syncNative() 把 size/radius/height 推进 native（base _cloneTo → _syncNative 覆盖几何）。唯独 MeshColliderShape 是惰性建 native（依赖 extracted buffers），无法靠 _syncNative，故需专门 _cloneTo cook 一次。
- mesh refcount：
_mesh是@property且ModelMesh extends Mesh extends ReferResource（Assignment）→ populate 走 Assignment 分支+1（reuse=null 短路）；_cloneTo只读target._mesh、_extractMeshData（纯读 positions/indices）、_createNativeShape，不碰 mesh refcount；destroy-1平衡。无双计。 - 时序：deep clone object 分支先 populate 全部
@property（含_materialdeep clone + 其_cloneTo同步 native），再跑 parent_cloneTo，故_createNativeShape用的target._material._nativeMaterial已是正确值。 - collider 重附：
_cloneTo时target._collider尚未设置 →_createNativeShape的 attach 块跳过、只建几何；随后 Collider_cloneTo → _syncNative → _addNativeShape设回shape._collider并 attach。commit 描述属实。 - happy path 不丢 base sync：
_createNativeShape内部已调super._syncNative()（建几何后立即同步 position/rotation/material），故不走super._cloneTo也不丢；else 分支（mesh null/extract 失败）走super._cloneTo → _syncNative，此时_nativeShapenull → early-return no-op，正确。
MeshColliderShape rebuilds its native PhysX shape 测试：clone mesh ground、丢 sphere、断言落地（y > -1）。revert _cloneTo → clone 无 native shape → 穿透 → fail。真反向测试。
复核：其余 native-state holder 无同类遗漏
穷举物理侧 native-state holder：Box/Sphere/Capsule/Plane 经 _syncNative override 覆盖几何（构造期建 native）；Joint/CharacterController 经 component _onEnable 生命周期重建 native（非 clone-time），不属同类。本 commit 把"eager/lazy 建 native 且不靠 enable 生命周期"的三处（PhysicsMaterial/ShaderData/MeshColliderShape）补齐，扫描无其他遗漏。
问题
[P0] packages/core/src/Camera.ts:133/135/137 — _replacementShader / _replacementSubShaderTag / _replacementFailureStrategy 仍漏标 @property，clone 时静默丢失（上一轮 P0，本轮仍未修复，重申）
已对照 HEAD 79e1d2916 源码核实：@property 在 132 行装饰 _virtualCamera，下面三个 replacement 字段仍是裸 @internal：
@property
_virtualCamera: VirtualCamera = new VirtualCamera();
/** @internal */
_replacementShader: Shader = null; // ← 无 @property
/** @internal */
_replacementSubShaderTag: ShaderTagKey = null; // ← 无 @property
/** @internal */
_replacementFailureStrategy: ReplacementFailureStrategy = null; // ← 无 @property这三个字段由 public setReplacementShader() 写入（Camera.ts:725-728），被 _drawRenderPass 读取（Camera.ts:664-666）。opt-out 时它们会被枚举 clone；切到 opt-in 后未标 @property → 不再 clone。一个调用过 setReplacementShader() 的 Camera 被 clone 后，replacement 配置静默丢失（clone 走普通 shader，渲染错但不报错、无编译错误）。
本 commit 修了 ShaderData/MeshColliderShape 的同根问题（native/derived state 不随 populate 走），却没顺手修 Camera 这条更简单的"裸 @property 字段漏标"——而后者是 opt-in 迁移最危险的一类（默认 clone → 默认不 clone，纯静默丢数据）。修法：给三个字段补 @property（Shader/ShaderTagKey 是 Assignment 类型、enum 是 primitive，正好走对应 gate 分支）。同时建议作者最后扫一遍所有迁移文件里原本未装饰、现在也未标 @property 的带构造默认值/setter 写入的状态字段，逐个确认是有意丢弃还是漏标。也与 #2988 相关：#2988 给这三个字段加的是 legacy @assignmentClone，本 PR 下应统一改 @property，两 PR 注意合并顺序。
P2
packages/core/src/clone/CloneManager.ts:104-105— gate JSDoc 仍未反映f13b12814引入的 release-old（十一轮 P2，重申）。_cloneValue的 Assignment 分支现做两件事：reuse?._addReferCount?.(-1)（释放 clone 构造期预占的旧 slot）+value._addReferCount?.(1)，但 JSDoc 仍只写 "share the reference, and bump the ref count"，未提 release-old。本 commit 未触及 CloneManager，故仍 open。建议补一句反映 release-old + acquire-new 的对称。
P3
packages/core/src/physics/shape/MeshColliderShape.ts:106-108— 新增_cloneTo的 3 行//注释末尾带句号（"via `_addNativeShape`."）。本项目约定单行//（含换行//块）不加句号，与Entity.ts:456-458/ CloneManager deep-clone 注释同根。去掉即可。- 历史 P3（
CloneUtils.test.ts文件名、defaultCloneMode缺@param mode）随手清。
备注（latent，无触发，不要求改）
PostProcessEffectParameter._needLerp（裸字段、子类构造默认 true）：本轮逐链路复核确认靠 construct-reuse 结构性兜底——effect 的 param 永远是构造期初始化的 @property 字段、同类型、且不能动态新增（PostProcessEffect._getParameters 注释明示），故 clone 时 reuse 路径必命中、_needLerp 保留 ctor 值（与 source 同）。无实际触发点，不作 P 级问题，仅提示作者在补 Camera @property 时一并判断是否需要显式标注（与 Camera 同属"裸字段携带非默认构造态",但 Camera 有真触发、它没有）。
结论
本 commit 方向正确、质量高：把"native/derived state 不随 populate 携带"的同根 bug 在 ShaderData/MeshColliderShape 两处收口，refcount 逐链路平衡、native 重建时序正确、配了经仿真路径的真反向回归测试（含上一轮 PhysicsMaterial P2 的 backfill）。物理侧其余 native holder 经穷举无同类遗漏。
但上一轮的 P0（Camera 三个 replacement 字段漏标 @property → clone 静默丢失）本轮仍未修复——作者修了更隐蔽的 ShaderData/MeshColliderShape，却漏了更简单、危害同样实在的 Camera 裸字段漏标。补 3 个 @property 即可，建议同时扫一遍其余迁移文件的同类裸状态字段。修复 P0 后可合并；P2（gate JSDoc）+ P3（注释句号、文件名）随手清。
2 participants
What
Re-architects the clone system for 2.0 from opt-out to opt-in.
@propertyare cloned; an unmarked field is not cloned (the Unity[SerializeField]/ UEUPROPERTY/ Cocos@propertymodel — one neutral "managed property" marker).@defaultCloneMode— Entity/Component → remap to the clone, ReferResource / interned flyweights → share, otherwise deep clone. No per-field clone-mode decorators.Map<source, target>built at clone time handles both reference remap and cycle / shared-subgraph dedup (replaces the old path-based remap).new ctor()/ reuse) → Populate (copyFromfast path, or recurse@propertyfields) → Finalize (_cloneTopost-clone hook, ≈ UEPostDuplicate).Why
Opt-out (clone every enumerable field unless
@ignoreClone) mixed three mechanisms and silently cloned transient/runtime state. Opt-in collapses the per-field decision to a single marker and infers HOW from the type — matching how Unity/UE/Cocos/Godot drive duplication from the same property metadata as serialization.Scope
CloneManagerwalks a cached per-class@propertyset (getProperties) instead offor k in source; legacy@deepClone/@assignmentClone/@ignoreClonebridges andCloneMode.Shallow/Ignoreremoved.@property; add@propertyto fields that were previously cloned by default (Camera, Renderer, lighting, physics, particle, postprocess, ui, …).@property-field-type sweep): physics shape/joint subclasses,PhysicsMaterial,PostProcessEffect(+Parameter),Transition(_target/_interactive),PointLight, particle emitter shapes.Skin.namemade a@propertyfield + parameterless-constructible.Verification
986 browser tests pass — core 826 (physics / postprocess / materials / particle / all components), ui 64, loader 96 (incl. PrefabResource + SceneFormatV2).
Follow-ups (not in this PR)
@propertyto also drive serialization + inspector (one declaration, orthogonalserialize/clone/visibleflags) — removes the deserialize↔clone asymmetry at the root. Touches editor + asset pipeline + content migration; separate RFC.MeshColliderShapeclone doesn't rebuild its native shape / unbalanced_meshrefcount;ShaderDatadeep-clone copies nothing.🤖 Generated with Claude Code
Summary by CodeRabbit