-
-
Notifications
You must be signed in to change notification settings - Fork 399
fix(clone): preserve target instance + Map/Set deep clone + cycle detection #2992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev/2.0
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,21 +105,46 @@ export class CloneManager { | |
| ): void { | ||
| const sourceProperty = source[k]; | ||
|
|
||
| // Remappable references (Entity/Component) are always remapped, regardless of clone decorator | ||
| // 1. Remappable references (Entity/Component) are always remapped, highest priority | ||
| if (sourceProperty instanceof Object && (<ICustomClone>sourceProperty)._remap) { | ||
| target[k] = (<ICustomClone>sourceProperty)._remap(srcRoot, targetRoot); | ||
| return; | ||
| } | ||
|
|
||
| // 2. Explicit ignore | ||
| if (cloneMode === CloneMode.Ignore) return; | ||
|
|
||
| // Primitives, undecorated, or @assignmentClone: direct assign | ||
| if (!(sourceProperty instanceof Object) || cloneMode === undefined || cloneMode === CloneMode.Assignment) { | ||
| // 3. Primitives / null / undefined - direct assign | ||
| if (!(sourceProperty instanceof Object)) { | ||
| target[k] = sourceProperty; | ||
| return; | ||
| } | ||
|
|
||
| // @shallowClone / @deepClone: deep copy complex objects | ||
| // 4. Determine effective clone mode | ||
| let effectiveCloneMode: CloneMode = cloneMode; | ||
| if (effectiveCloneMode === undefined) { | ||
| // Undecorated: infer from runtime type | ||
| effectiveCloneMode = CloneManager._inferCloneMode(sourceProperty, target[k]); | ||
| } else if (effectiveCloneMode !== CloneMode.Assignment) { | ||
| // Decorated Shallow/Deep: upgrade to Deep if target already has independent same-type instance. | ||
| // Assignment is never upgraded — it means the user explicitly wants a reference copy. | ||
| const targetProperty = target[k]; | ||
| if ( | ||
| targetProperty && | ||
| targetProperty !== sourceProperty && | ||
| targetProperty.constructor === sourceProperty.constructor | ||
| ) { | ||
| effectiveCloneMode = CloneMode.Deep; | ||
| } | ||
| } | ||
|
|
||
| // 5. Assignment - direct reference copy | ||
| if (effectiveCloneMode === CloneMode.Assignment) { | ||
| target[k] = sourceProperty; | ||
| return; | ||
| } | ||
|
|
||
| // 6. Shallow/Deep clone for complex types | ||
| const type = sourceProperty.constructor; | ||
| switch (type) { | ||
| case Uint8Array: | ||
|
|
@@ -137,6 +162,37 @@ export class CloneManager { | |
| targetPropertyT.set(<TypedArray>sourceProperty); | ||
| } | ||
| break; | ||
| case Map: | ||
| let targetPropertyM = <Map<any, any>>target[k]; | ||
| if (targetPropertyM == null) { | ||
| target[k] = targetPropertyM = new Map<any, any>(); | ||
| } else { | ||
| targetPropertyM.clear(); | ||
|
Comment on lines
+166
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only preserve a preinitialized target when its type matches the source. These paths currently reuse any truthy Suggested fix- let targetPropertyM = <Map<any, any>>target[k];
+ let targetPropertyM = target[k] instanceof Map ? <Map<any, any>>target[k] : null;
if (targetPropertyM == null) {
target[k] = targetPropertyM = new Map<any, any>();
} else {
targetPropertyM.clear();
}
- let targetPropertyS = <Set<any>>target[k];
+ let targetPropertyS = target[k] instanceof Set ? <Set<any>>target[k] : null;
if (targetPropertyS == null) {
target[k] = targetPropertyS = new Set<any>();
} else {
targetPropertyS.clear();
}
let targetPropertyD = <Object>target[k];
- if (!targetPropertyD) {
+ if (!targetPropertyD || targetPropertyD.constructor !== sourceProperty.constructor) {
targetPropertyD = new sourceProperty.constructor();
target[k] = targetPropertyD;
}Also applies to: 183-187, 223-228 🧰 Tools🪛 Biome (2.4.14)[error] 166-166: Other switch clauses can erroneously access this declaration. (lint/correctness/noSwitchDeclarations) 🤖 Prompt for AI Agents |
||
| } | ||
| (<Map<any, any>>sourceProperty).forEach((value, key) => { | ||
| if (key instanceof Object && (<ICustomClone>key)._remap) { | ||
| key = (<ICustomClone>key)._remap(srcRoot, targetRoot); | ||
| } | ||
| if (value instanceof Object && (<ICustomClone>value)._remap) { | ||
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | ||
| } | ||
| targetPropertyM.set(key, value); | ||
| }); | ||
|
Comment on lines
+165
to
+180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Map/Set “deep clone” still aliases nested entries. These branches only remap Also applies to: 182-194 🧰 Tools🪛 Biome (2.4.14)[error] 166-166: Other switch clauses can erroneously access this declaration. (lint/correctness/noSwitchDeclarations) 🤖 Prompt for AI Agents |
||
| break; | ||
| case Set: | ||
| let targetPropertyS = <Set<any>>target[k]; | ||
| if (targetPropertyS == null) { | ||
| target[k] = targetPropertyS = new Set<any>(); | ||
| } else { | ||
| targetPropertyS.clear(); | ||
| } | ||
| (<Set<any>>sourceProperty).forEach((value) => { | ||
| if (value instanceof Object && (<ICustomClone>value)._remap) { | ||
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | ||
| } | ||
| targetPropertyS.add(value); | ||
| }); | ||
| break; | ||
| case Array: | ||
| let targetPropertyA = <Array<any>>target[k]; | ||
| const length = (<Array<any>>sourceProperty).length; | ||
|
|
@@ -150,46 +206,82 @@ export class CloneManager { | |
| <Array<any>>sourceProperty, | ||
| targetPropertyA, | ||
| i, | ||
| cloneMode, | ||
| cloneMode, // Pass original mode: decorated → children inherit, undecorated → children infer independently | ||
| srcRoot, | ||
| targetRoot, | ||
| deepInstanceMap | ||
| ); | ||
| } | ||
| break; | ||
| default: | ||
| let targetProperty = <Object>target[k]; | ||
| // If the target property is undefined, create new instance and keep reference sharing like the source | ||
| if (!targetProperty) { | ||
| targetProperty = deepInstanceMap.get(sourceProperty); | ||
| if (!targetProperty) { | ||
| targetProperty = new sourceProperty.constructor(); | ||
| deepInstanceMap.set(sourceProperty, targetProperty); | ||
| } | ||
| target[k] = targetProperty; | ||
| // Check if we've already visited this source object (cycle detection) | ||
| if (deepInstanceMap.has(sourceProperty)) { | ||
| target[k] = deepInstanceMap.get(sourceProperty); | ||
| return; | ||
| } | ||
|
|
||
| let targetPropertyD = <Object>target[k]; | ||
| if (!targetPropertyD) { | ||
| targetPropertyD = new sourceProperty.constructor(); | ||
| target[k] = targetPropertyD; | ||
| } | ||
| deepInstanceMap.set(sourceProperty, targetPropertyD); | ||
|
|
||
| if ((<ICustomClone>sourceProperty).copyFrom) { | ||
| (<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty); | ||
| (<ICustomClone>targetPropertyD).copyFrom(<ICustomClone>sourceProperty); | ||
| } else { | ||
| const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor); | ||
| for (let k in sourceProperty) { | ||
| CloneManager.cloneProperty( | ||
| <Object>sourceProperty, | ||
| targetProperty, | ||
| targetPropertyD, | ||
| k, | ||
| cloneModes[k], | ||
| srcRoot, | ||
| targetRoot, | ||
| deepInstanceMap | ||
| ); | ||
| } | ||
| (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetProperty, srcRoot, targetRoot); | ||
| (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetPropertyD, srcRoot, targetRoot); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Infer the appropriate clone mode for an undecorated property based on its runtime type. | ||
| * This enables user custom scripts to get correct clone behavior without decorators. | ||
| */ | ||
| private static _inferCloneMode(sourceProperty: Object, targetProperty: any): CloneMode { | ||
| // If target already has an independent instance of the same type, | ||
| // deep clone to preserve isolation (e.g., constructor-created objects) | ||
| if ( | ||
| targetProperty && | ||
| targetProperty !== sourceProperty && | ||
| targetProperty.constructor === sourceProperty.constructor | ||
| ) { | ||
| return CloneMode.Deep; | ||
| } | ||
|
|
||
| // Arrays need recursive processing (may contain Entity/Component refs) | ||
| if (Array.isArray(sourceProperty)) return CloneMode.Deep; | ||
|
|
||
| // TypedArrays - copy data | ||
| if (ArrayBuffer.isView(sourceProperty)) return CloneMode.Deep; | ||
|
Comment on lines
+269
to
+270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Yes. ArrayBuffer.isView(value) returns true for each of these, because it returns true when the argument has the internal slot [[ViewedArrayBuffer]], which is used by ArrayBuffer views such as typed array objects and DataView [1][2]. Specifically: - DataView: true [2] - Uint8ClampedArray: true (it’s a typed array view) [2][3] - BigInt64Array: true (it’s a typed array view) [2][3] - BigUint64Array: true (it’s a typed array view) [2][3] If you want to validate quickly in code: ArrayBuffer.isView(new DataView(new ArrayBuffer(1))) // true ArrayBuffer.isView(new Uint8ClampedArray(1)) // true ArrayBuffer.isView(new BigInt64Array(1)) // true ArrayBuffer.isView(new BigUint64Array(1)) // true Citations:
🏁 Script executed: find . -name "CloneManager.ts" -type fRepository: galacean/engine Length of output: 101 🏁 Script executed: cat -n ./packages/core/src/clone/CloneManager.ts | head -350 | tail -100Repository: galacean/engine Length of output: 4429 🏁 Script executed: cat -n ./packages/core/src/clone/CloneManager.ts | head -250 | tail -100Repository: galacean/engine Length of output: 4288 🏁 Script executed: cat -n ./packages/core/src/clone/CloneManager.ts | sed -n '100,160p'Repository: galacean/engine Length of output: 2635 Handle all ArrayBuffer views explicitly in the switch statement, or narrow the The 🤖 Prompt for AI Agents |
||
|
|
||
| // Maps and Sets - create independent copies | ||
| if (sourceProperty instanceof Map || sourceProperty instanceof Set) return CloneMode.Deep; | ||
|
|
||
| // Value types with copyFrom (math types like Vector3, Color, etc.) | ||
| if ((<ICustomClone>sourceProperty).copyFrom) return CloneMode.Deep; | ||
|
|
||
| // Plain objects - deep clone (may contain Entity/Component refs) | ||
| if (sourceProperty.constructor === Object) return CloneMode.Deep; | ||
|
|
||
| // Other class instances (engine resources like Material, Texture) - shared reference | ||
| return CloneMode.Assignment; | ||
| } | ||
|
|
||
| static deepCloneObject(source: Object, target: Object, deepInstanceMap: Map<Object, Object>): void { | ||
| for (let k in source) { | ||
| CloneManager.cloneProperty(source, target, k, CloneMode.Deep, null, null, deepInstanceMap); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: galacean/engine
Length of output: 3134
🏁 Script executed:
Repository: galacean/engine
Length of output: 72
🏁 Script executed:
# Let's also check the complete file size to understand what we're dealing with wc -l packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 103
🏁 Script executed:
# Read the Biome configuration cat biome.jsonRepository: galacean/engine
Length of output: 1318
🏁 Script executed:
Repository: galacean/engine
Length of output: 5233
Wrap all
casedeclarations with block scope.The
letbindings in multiple switch cases live in the switch's shared lexical scope, which is why Biome flagsnoSwitchDeclarations. Bracing eachcasecreates isolated scopes and fixes the lint error. This applies to the TypedArray cases (lines 150–164), Map case (line 165), Set case (line 182), Array case (line 196), and default case (line 216).Suggested fix
case Uint8Array: case Uint16Array: case Uint32Array: case Int8Array: case Int16Array: case Int32Array: case Float32Array: case Float64Array: { let targetPropertyT = <TypedArray>target[k]; if (targetPropertyT == null || targetPropertyT.length !== (<TypedArray>sourceProperty).length) { target[k] = (<TypedArray>sourceProperty).slice(); } else { targetPropertyT.set(<TypedArray>sourceProperty); } break; + } case Map: { let targetPropertyM = <Map<any, any>>target[k]; if (targetPropertyM == null) { target[k] = targetPropertyM = new Map<any, any>(); } else { targetPropertyM.clear(); } (<Map<any, any>>sourceProperty).forEach((value, key) => { if (key instanceof Object && (<ICustomClone>key)._remap) { key = (<ICustomClone>key)._remap(srcRoot, targetRoot); } if (value instanceof Object && (<ICustomClone>value)._remap) { value = (<ICustomClone>value)._remap(srcRoot, targetRoot); } targetPropertyM.set(key, value); }); break; } case Set: { let targetPropertyS = <Set<any>>target[k]; if (targetPropertyS == null) { target[k] = targetPropertyS = new Set<any>(); } else { targetPropertyS.clear(); } (<Set<any>>sourceProperty).forEach((value) => { if (value instanceof Object && (<ICustomClone>value)._remap) { value = (<ICustomClone>value)._remap(srcRoot, targetRoot); } targetPropertyS.add(value); }); break; } case Array: { let targetPropertyA = <Array<any>>target[k]; const length = (<Array<any>>sourceProperty).length; if (targetPropertyA == null) { target[k] = targetPropertyA = new Array<any>(length); } else { targetPropertyA.length = length; } for (let i = 0; i < length; i++) { CloneManager.cloneProperty( <Array<any>>sourceProperty, targetPropertyA, i, cloneMode, srcRoot, targetRoot, deepInstanceMap ); } break; } default: { // Check if we've already visited this source object (cycle detection) if (deepInstanceMap.has(sourceProperty)) { target[k] = deepInstanceMap.get(sourceProperty); return; } let targetPropertyD = <Object>target[k]; if (!targetPropertyD) { targetPropertyD = new sourceProperty.constructor(); target[k] = targetPropertyD; } deepInstanceMap.set(sourceProperty, targetPropertyD); if ((<ICustomClone>sourceProperty).copyFrom) { (<ICustomClone>targetPropertyD).copyFrom(<ICustomClone>sourceProperty); } else { const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor); for (let k in sourceProperty) { CloneManager.cloneProperty( <Object>sourceProperty, targetPropertyD, k, cloneModes[k], srcRoot, targetRoot, deepInstanceMap ); } (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetPropertyD, srcRoot, targetRoot); } break; }📝 Committable suggestion
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents