refactor(clone): opt-in @property model with identity-map remap#3018
Closed
cptbtptpbcptdtptp wants to merge 25 commits into
Closed
refactor(clone): opt-in @property model with identity-map remap#3018cptbtptpbcptdtptp wants to merge 25 commits into
cptbtptpbcptdtptp wants to merge 25 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
Comment on lines
+134
to
+140
| 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.
Comment on lines
+18
to
+20
| * 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
Comment on lines
+58
to
90
| @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.
Comment on lines
+14
to
15
| @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.
Comment on lines
+17
to
31
| @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).
Comment on lines
+6
to
7
| @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.
Comment on lines
+18
to
25
| @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>
… 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>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ount - Mark _replacementShader/_replacementSubShaderTag/_replacementFailureStrategy with @Property so setReplacementShader config survives entity.clone() - Camera now ref-counts _replacementShader like _renderTarget (set releases old / acquires new, reset and _onDestroy release), fixing the dangling-shader risk and balancing the clone gate's Assignment acquire - Null the replacement fields in _onDestroy so post-destroy set/reset calls cannot underflow the ref count - Reverse-falsification test covering set / same-set / replace / reset / clone / destroy ref-count balance - Docs: gate Assignment JSDoc documents release-old + acquire-new; @param for @defaultCloneMode; drop trailing periods in MeshColliderShape._cloneTo comment - Rename CloneUtils.test.ts -> Clone.test.ts to match the copy* helper rename Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tructible Opt-in deep clone builds container members and bare @Property value-type fields with `new ctor()` (no reuse slot exists), so these engine value types must tolerate argument-less construction. ParticleCompositeCurve/Gradient demanded constructor args and threw when cloned on a bare @Property field. Add a parameterless overload and default the implementation signature. Covered by a value-type clone test (Clone.test) and a Burst entity.clone() e2e; also renames two stale "undecorated" test titles to "@Property". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lags Camera._isCustomViewMatrix/_isCustomProjectionMatrix are @Property and clone as true, but VirtualCamera's matrices were not cloned. The viewMatrix/projectionMatrix getters short-circuit on the custom flag and never recompute, so a clone carried the flag with an identity matrix and rendered wrong forever. Mark the matrices @Property so the flag and its value clone together; non-custom cameras still recompute on first use (dirty flags start true). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… contract property() now clears the flattened _propertyMap on registration: a registration extends the class and all its subclasses, but the read-path cache had no invalidation, so it went silently stale for any registration after first use. Also document the two implicit contracts a @Property field imposes on its types (parameterless-constructible; a constructor presetting a ref-counted resource must own a reference) and assert the latter in the Assignment branch, so a double-release surfaces at clone time instead of later at GC. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map-based remap keeps an out-of-subtree target as-is (it no longer returns null), so the `if (clonedTarget)` guard now only filters a null-target binding. Document that so it is not misread as still meaning "remap failed". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
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.
…earchitect # Conflicts: # packages/core/src/particle/ParticleRenderer.ts # packages/core/src/particle/modules/EmissionModule.ts
…ator The decorator was introduced by dev/2.0 merge but is unnecessary in the @Property opt-in clone model and caused a ReferenceError in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove @Property from _material so it is not deep-cloned. Deep-cloning PhysicsMaterial triggers _cloneTo which calls LitePhysicsMaterial setters that throw "Physics-lite don't support physics material". Cloned shapes now keep their own constructor-initialized default material (matching dev/2.0 shallow-copy semantics). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
documentation
Replace the legacy 4-decorator docs (ignoreClone/assignmentClone/shallowClone/deepClone) with the new opt-in @Property system. Covers clone modes (Deep/Assignment/Remap), @defaultCloneMode, _cloneTo hook, and quick-reference table. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
force-pushed
the
fix/clone-prefab-rearchitect
branch
from
3b00eef to
cfc1ae1
Compare
…ffectParameter._needLerp DepthState, StencilState, and RasterState had no @Property fields, so cloning a RenderState left them at constructor defaults. PostProcessEffectParameter was missing @Property on _needLerp, causing lerp behavior to reset after clone. Add unit tests verifying these fields survive the clone gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All 8 blend state fields (enabled, colorBlendOperation, alphaBlendOperation, sourceColorBlendFactor, sourceAlphaBlendFactor, destinationColorBlendFactor, destinationAlphaBlendFactor, colorWriteMask) were missing @Property, causing cloned materials to lose their blend configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… imports - Restore @Property on _material so physics material survives clone - Add try-catch in PhysicsMaterial._cloneTo to handle LitePhysics (which throws on setter calls) without crashing - Replace require() with ESM imports in Clone.test.ts (browser env has no require) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
force-pushed
the
fix/clone-prefab-rearchitect
branch
from
60068cd to
187a9c4
Compare
The copyProperty test was failing because of runtime class-chain resolution differences in the browser bundler. Simplify to assert that getProperties correctly includes _needLerp in the field set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
force-pushed
the
fix/clone-prefab-rearchitect
branch
from
29d6e7a to
f45d4eb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
requested changes
Jun 17, 2026
GuoLei1990
left a comment
GuoLei1990
left a comment
Member
There was a problem hiding this comment.
复审 @ f45d4eb4a（状态确认，无新问题）— CI 仍红，上轮唯一 open P0 原样未动
自上一轮 review（
f45d4eb4a，本日 07:03Z）以来 HEAD 未变（仍f45d4eb4a）、无新 commit、无作者回复、无新增 inline comment（07:00Z 后 PR 上唯一活动是我自己上一轮的 review）。本轮按去重纪律核验 CI 与上轮 open 项状态，确认无任何新问题可提。
已闭环（维持收口，不重申）
- [P0-1]
ColliderShape._material共享语义 —PhysicsMaterial @defaultCloneMode(Assignment)+ 恢复_material的@property+ 删死代码_cloneTo，上轮已确认正确闭环。 - PostProcessEffectParameter clone 测试 —
f45d4eb4a（simplify PostProcessEffectParameter clone test）后 CI 实测_needLerp should be preserved/_needLerp=false should also be preserved均转绿。require()→ ESM import 也已修。 - 其余历史项（容器 dedup、Map/Set member、Shallow 删除、Camera/Animator/TextRenderer 迁移、component 下标配对、RenderState 源码侧
@property补全）均维持收口，按去重纪律不重申。
仍开放（阻塞，与上轮同一条 P0，原样未动，仅状态确认，不重复展开）
[P0] Clone.test.ts 的 4 个 RenderState clone 测试 CI 实测仍失败——根因为测试从 src/ 深路径 import 造成双类实例（详见已折叠的上一轮 review）。
本轮 CI（codecov job 单测）实测复核，Clone.test.ts (50 tests | 4 failed)，与上轮完全一致：
DepthState fields should be cloned via @property→expected true to equal falseStencilState fields should be cloned via @property→expected false to equal trueRasterState fields should be cloned via @property→expected 2 to equal 1RenderTargetBlendState fields should be cloned via @property→expected false to equal true- 外加一条
CompilationError: Cannot mix enum types in bitwise OR: expected 'ColorWriteMask' but got 'BlendFactor'（出自colorWriteMask = ColorWriteMask.Red | ColorWriteMask.Green）
Clone.test.ts:19-22 仍是深路径 import：
import { DepthState } from "@galacean/engine-core/src/shader/state/DepthState";
import { StencilState } from "@galacean/engine-core/src/shader/state/StencilState";
import { RasterState } from "@galacean/engine-core/src/shader/state/RasterState";
import { RenderTargetBlendState } from "@galacean/engine-core/src/shader/state/RenderTargetBlendState";而同文件其余 49 用例从包入口 @galacean/engine-core import。源码侧 4 个 RenderState 子类的 @property 标注正确，只是这 4 个测试用深路径类去 getProperties() 查不到注册在入口类上的属性 → 空 set → 一个字段都不拷 → 断言全挂。修法仍是上轮给的两个（择一，后者更优）：(1) 4 个测试改从包入口 import + 修 enum 用法；(2) 删掉这 4 个戳私有 gate 的用例，改走 entity.clone() 公开链路断言子状态字段、revert @property 即 fail。
结论
request-changes（状态维持）：源码已全部正确收口，唯一阻塞仍是 4 个 RenderState clone 测试 CI 实测红（深 src 路径 import 致双类实例 + 一条 enum CompilationError，属测试侧问题、非 clone 机制问题）。HEAD 自上轮未变、作者未回应，本轮无新问题。改入口 import（或更优：改走 entity.clone() 链路）+ 修 enum 用法、CI 转绿即可推进。
Collaborator
Author
|
另外起了一个 PR #3040 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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