fix(animation, physics, audio, gltf): cherry-pick fixes from fix/shaderlab#3014
fix(animation, physics, audio, gltf): cherry-pick fixes from fix/shaderlab#3014luzhuang wants to merge 6 commits into
Conversation
Loader: - componentRef 解析支持 clone entity 子树查找 - always create GLTF_ROOT container for consistent animation paths - normalize gltf wrapper and skin roots - include skinned mesh nodes in skin root LCA - compute skinned bounds from non-joint rootBone - add audio extension to AudioLoader supported types - recognize .m4a/.aac/.flac extensions for audioClip loader - apply scene physics settings - cover stripped entity child attachment Animation: - normalize single-root clip binding paths - add per-instance speed to AnimatorStatePlayData - use per-instance speed during cross fade - update animator - expose clip sampling and gate animation events Physics: - resolve clone-bypass and runtime native handle desync bugs - add DynamicCollider.applyForceAtPosition - remove redundant wakeUp in addForce/addTorque - repair 8 pre-existing test failures (Lite material + PhysX scene queries) - kinematic transform sync via setKinematicTarget - correct collision contact normal orientation - support kinematic transform teleport sync - teleport collider on re-enable instead of sweeping - support PhysX tolerance scale - raycast 和 sweep 跳过 initial overlap Clone: - prefab 克隆时自动 deep clone 同类型 Object 属性 Audio: - restore pending playback correctly - restore sources after background interruption - recover audio after foreground resume
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
WalkthroughThis PR refactors the animation system's state-play-data model to use per-instance speed/wrap modes with pooled event handlers, introduces physics collider transform synchronization modes and optional tolerances defaults, implements comprehensive audio suspend/resume with user-gesture and visibility detection, and updates GLTF skin parsing to use lowest-common-ancestor root bone resolution. ChangesAnimation State and Play-Data Refactoring
Physics Collider Synchronization and Defaults
Audio Suspend/Resume with User-Gesture Recovery
GLTF Loader Skin and Schema Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/physics/shape/MeshColliderShape.ts (1)
199-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear pending retry state for non-retryable mesh collider config.
When non-convex is used on a non-kinematic
DynamicCollider,_createNativeShape()returns early but_pendingNativeShapeCreationcan remaintrue(set inset mesh), so_onPhysicsUpdate()keeps retrying and logging every tick.💡 Proposed fix
private _createNativeShape(): void { // Non-convex MeshColliderShape is only supported on StaticCollider or kinematic DynamicCollider if (!this._isConvex && this._collider instanceof DynamicCollider && !this._collider.isKinematic) { console.error("MeshColliderShape: Non-convex mesh is not supported on non-kinematic DynamicCollider."); + this._pendingNativeShapeCreation = false; return; }Also applies to: 248-250
🤖 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 199 - 203, The method _createNativeShape() returns early when a non-convex mesh is used on a non-kinematic DynamicCollider but does not clear the retry flag _pendingNativeShapeCreation, causing _onPhysicsUpdate() to repeatedly retry; update _createNativeShape() to set this._pendingNativeShapeCreation = false before each early return (including the non-convex/non-kinematic DynamicCollider branch and the other early-return branches around the same area) so pending retry state is cleared when the mesh config is non-retryable.packages/loader/src/SceneLoader.ts (1)
1-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing
Materialimport inSceneLoadersky-material lookup
packages/loader/src/SceneLoader.tsusesMaterialinresourceManager.getResourceByRef<Material>(background.skyMaterial), butMaterialis not imported from@galacean/engine-core, which will fail TypeScript compilation.➕ Suggested fix
import { AssetPromise, AssetType, BackgroundMode, DiffuseMode, Loader, LoadItem, Logger, + Material, Mesh, resourceLoader, ResourceManager, Scene } from "`@galacean/engine-core`";🤖 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/loader/src/SceneLoader.ts` around lines 1 - 13, The import list in SceneLoader.ts is missing Material, causing the generic call resourceManager.getResourceByRef<Material>(background.skyMaterial) to fail; update the import from "`@galacean/engine-core`" in SceneLoader.ts to include Material alongside AssetPromise, AssetType, BackgroundMode, DiffuseMode, Loader, LoadItem, Logger, Mesh, resourceLoader, ResourceManager, and Scene so the Material type is available for the skyMaterial lookup and TypeScript compilation succeeds.packages/core/src/animation/Animator.ts (1)
400-415:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing layer-index bounds check can throw runtime errors.
At Line 414,
layers[layerIndex]is accessed without validating range. InvalidlayerIndexvalues should safely return no state instead of throwing.🤖 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/animation/Animator.ts` around lines 400 - 415, In _getAnimatorStateInfo, avoid indexing layers with an out-of-range layerIndex: before accessing layers[layerIndex].stateMachine (or calling findStateByName on a specific layer), validate that layerIndex is >= 0 and < layers.length; if the index is invalid, return the default _tempAnimatorStateInfo (no state) instead of proceeding. Ensure both the else branch that uses layers[layerIndex] and any post-loop access paths check bounds, and keep references to symbols: _getAnimatorStateInfo, _animatorController, layers, layerIndex, stateMachine.findStateByName, and _tempAnimatorStateInfo to locate and update the code.
🟠 Major comments (23)
.husky/_/husky.sh-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the accidental local absolute path with the real Husky helper.
Line 1 hardcodes a workstation-specific path, so hooks will fail on any other checkout, and this also leaks a local filesystem path into the repo. Please restore/regenerate the actual
.husky/_/husky.shscript instead of committing a local absolute path literal.🤖 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 @.husky/_/husky.sh at line 1, The committed .husky/_/husky.sh contains a workstation-specific absolute path instead of the real Husky helper script; remove the hardcoded path and restore the proper husky helper by regenerating or replacing the file with Husky's shipped script (the standard husky.sh helper used by Git hooks). To fix, delete the current .husky/_/husky.sh, then regenerate the helper by running the Husky install command (or copy the husky.sh helper from the Husky package) so the file contains the canonical husky helper content rather than an absolute local path; verify hooks invoke the restored husky.sh helper correctly.tests/src/core/particle/ParticleStopResume.test.ts-85-89 (1)
85-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTests are asserting known-bug behavior instead of expected behavior.
Line 87 and Line 140 currently validate the bug path, which hardens the defect into CI and blocks the real fix later. Please switch these to expected-correct assertions (or mark as
it.fails/it.todountil the fix lands).Also applies to: 137-140
🤖 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/particle/ParticleStopResume.test.ts` around lines 85 - 89, The test currently asserts the known-bug behavior instead of the correct behavior; update the failing assertions that reference emittedDuringIdle, emittedFirstFrameAfterResume and the time delta playTimeAfterIdle - playTimeAtStop to reflect the expected-correct outcomes (e.g., emittedFirstFrameAfterResume should assert a small/single catch-up like toBeLessThanOrEqual(1) and playTimeAfterIdle - playTimeAtStop should assert a small delta like toBeLessThanOrEqual(1) or toBeCloseTo) OR, if you cannot fix behavior now, mark the affected test cases (the Particle stop/resume test that uses those variables) as it.todo or it.fails so CI does not lock in the buggy expectations.packages/loader/src/gltf/parser/GLTFSkinParser.test.ts-5-7 (1)
5-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGlobal
windowis being clobbered and not restored.Line 5 replaces the entire
windowobject, which can pollute global state and cause cross-test flakiness. Stub only required fields and restore after each test.🔧 Safer isolation pattern
-import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; describe("GLTFSkinParser rootBone resolution", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + async function createParser(): Promise<any> { - (globalThis as any).window = { AudioContext: undefined, TextMetrics: undefined }; + vi.stubGlobal("window", { + ...(globalThis as any).window, + AudioContext: undefined, + TextMetrics: undefined + }); const { GLTFSkinParser } = await import("./GLTFSkinParser"); return new GLTFSkinParser(); }🤖 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/loader/src/gltf/parser/GLTFSkinParser.test.ts` around lines 5 - 7, The test currently replaces globalThis.window entirely which can pollute global state; instead, in GLTFSkinParser.test.ts stub only the needed properties (e.g., set globalThis.window = globalThis.window ?? {} then assign window.AudioContext and window.TextMetrics) or better save const _origWindow = globalThis.window and replace only the missing properties, and restore the original window in an afterEach/teardown block; ensure the test still imports GLTFSkinParser after stubbing and that the restoration happens even if the test errors so other tests are unaffected.packages/core/src/animation/AnimatorStateMachine.ts-22-23 (1)
22-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
defaultStatecan become dangling afterremoveStateLine 22 makes
defaultStatenon-null, but Line 66-73 does not rebind it when the current default is removed. This can leave auto-play pointing to a state no longer instates/_statesMap.🔧 Suggested fix
- defaultState: AnimatorState; + defaultState: AnimatorState | null = null; @@ removeState(state: AnimatorState): void { @@ if (index > -1) { this.states.splice(index, 1); } delete this._statesMap[name]; + if (this.defaultState === state) { + this.defaultState = this.states[0] ?? null; + } }Also applies to: 66-73
🤖 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/animation/AnimatorStateMachine.ts` around lines 22 - 23, The defaultState property can become dangling when removeState removes the currently assigned default; update the removeState method (and any code that mutates states/_statesMap or autoplay behavior) to check if the state being removed === this.defaultState and, if so, rebind this.defaultState to either null/undefined or to a safe fallback state (e.g., the first remaining state in states/_statesMap) and update autoplay handling accordingly so auto-play never references a removed state; also make defaultState nullable in the class signature if you choose to allow null to represent "no default".packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts-42-49 (1)
42-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset dependency-progress state in
clear().Lines 42-49 clear entity/component collections but leave
_tasks,_loaded, and_totalunchanged. If this context is reused, URL dedupe and progress reporting become stale.🔧 Proposed fix
clear() { this.entityMap.clear(); this.components.clear(); this.componentConfigMap.clear(); this.entityConfigMap.clear(); this.rootIds.length = 0; this.strippedIds.length = 0; + this._tasks.clear(); + this._loaded = 0; + this._total = 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts` around lines 42 - 49, The clear() method on ParserContext currently resets entity/component collections but fails to reset dependency/progress state (_tasks, _loaded, _total), causing stale URL dedupe and progress when the context is reused; update ParserContext.clear() to clear or reinitialize _tasks (e.g., call _tasks.clear() or assign a new empty map/set) and reset numeric counters _loaded and _total back to 0 so dependency tracking and progress reporting start fresh.packages/loader/src/ShaderLoader.ts-21-24 (1)
21-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
@builtinresolves to an unknown shader inShaderLoader
Shader.find(name)isreturn Shader._shaderMap[name], so an unknown name returns a missing entry at runtime.ShaderLoadercurrently returnsShader.find(builtinShader)directly, whileMaterialLoaderalready guardsif (shader) ...before using it—soShaderLoadershould reject/throw instead of resolving with an invalid value.🔧 Proposed fix
const builtinShader = this._getBuiltinShader(code); if (builtinShader) { - return Shader.find(builtinShader); + const shader = Shader.find(builtinShader); + if (!shader) { + throw new Error(`ShaderLoader: builtin shader "${builtinShader}" not found.`); + } + return shader; }🤖 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/loader/src/ShaderLoader.ts` around lines 21 - 24, The current ShaderLoader returns Shader.find(builtinShader) directly which can yield undefined for unknown builtins; update the logic in ShaderLoader (use the _getBuiltinShader method and the call to Shader.find) to check the result and throw or reject with a clear error if Shader.find(builtinShader) is falsy (include the builtin name in the message), instead of returning the invalid value so callers fail fast.packages/loader/src/MaterialLoader.ts-24-32 (1)
24-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard missing
renderStatebefore applying it.When the schema omits
renderState, this code currently assignsmaterial.renderState = undefined, which wipes the material’s default render-state object. That turns a missing optional field into a runtime regression for otherwise valid.matfiles.🛠️ Proposed fix
-function parseProperty(object: Object, key: string, value: any) { - if (typeof value === "object") { - for (let subKey in value) { - parseProperty(object[key], subKey, value[subKey]); - } - } else { - object[key] = value; - } +function parseProperty(object: any, key: string, value: any) { + if (value == null) { + return; + } + + if (typeof value === "object") { + const target = object[key]; + if (target == null) { + return; + } + + for (const subKey in value) { + parseProperty(target, subKey, value[subKey]); + } + } else { + object[key] = value; + } } @@ - parseProperty(material, "renderState", renderState); + renderState && parseProperty(material, "renderState", renderState);Also applies to: 125-126
🤖 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/loader/src/MaterialLoader.ts` around lines 24 - 32, The parseProperty function currently assigns nested keys blindly and ends up setting material.renderState = undefined when the schema omits renderState; update parseProperty (and the code paths that call it for material.renderState, e.g., where material.renderState is populated around the renderState handling at the second occurrence) to first check that object[key] (i.e., material.renderState) exists and, if not, create or skip applying subkeys as appropriate—specifically: when key === "renderState" ensure you do not overwrite an existing material.renderState with undefined (create a default object only if needed) or skip assignment when value is undefined so missing optional renderState does not wipe defaults.packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts-48-53 (1)
48-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad sky dependencies independently instead of requiring both refs.
At Line 48, dependency collection only runs when both
skyMeshandskyMaterialare present. If only one is configured, it won’t be collected.💡 Suggested fix
- const { skyMesh, skyMaterial } = background; - if (skyMesh && skyMaterial) { - // `@ts-ignore` - context._addDependentAsset(skyMesh.url, resourceManager.getResourceByRef(skyMesh)); - // `@ts-ignore` - context._addDependentAsset(skyMaterial.url, resourceManager.getResourceByRef(skyMaterial)); - } + const { skyMesh, skyMaterial } = background; + // `@ts-ignore` + skyMesh && context._addDependentAsset(skyMesh.url, resourceManager.getResourceByRef(skyMesh)); + // `@ts-ignore` + skyMaterial && context._addDependentAsset(skyMaterial.url, resourceManager.getResourceByRef(skyMaterial));🤖 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/loader/src/resource-deserialize/resources/scene/SceneParser.ts` around lines 48 - 53, The current logic only adds sky dependencies when both skyMesh and skyMaterial are truthy; change it to add each dependency independently: if skyMesh is present call context._addDependentAsset(skyMesh.url, resourceManager.getResourceByRef(skyMesh)), and if skyMaterial is present call context._addDependentAsset(skyMaterial.url, resourceManager.getResourceByRef(skyMaterial)) so each is collected even when the other is missing; update the checks around skyMesh and skyMaterial in SceneParser (the block using context._addDependentAsset and resourceManager.getResourceByRef) accordingly.packages/loader/src/Texture2DLoader.ts-49-52 (1)
49-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the original mipmap decision during content restore.
The initial image path only generates mipmaps when
supportGenerateMipmapsWithCorrection(...)says it is valid, butrestoreContent()always regenerates them. Textures loaded withmipmap: falseor unsupported dimensions/format will restore to different GPU state after context loss.🧩 Suggested fix
- loadImageFromBuffer(buffer).then((img) => { - const texture = this._createTexture(img, item, resourceManager); - resourceManager.addContentRestorer(new Texture2DContentRestorer(texture, url, requestConfig)); + loadImageFromBuffer(buffer).then((img) => { + const { texture, generateMipmap } = this._createTexture(img, item, resourceManager); + resourceManager.addContentRestorer( + new Texture2DContentRestorer(texture, url, requestConfig, generateMipmap) + ); resolve(texture); }, reject); @@ - private _createTexture(img: HTMLImageElement, item: LoadItem, resourceManager: ResourceManager): Texture2D { + private _createTexture( + img: HTMLImageElement, + item: LoadItem, + resourceManager: ResourceManager + ): { texture: Texture2D; generateMipmap: boolean } { @@ - return texture; + return { texture, generateMipmap }; } } class Texture2DContentRestorer extends ContentRestorer<Texture2D> { constructor( resource: Texture2D, public url: string, - public requestConfig: RequestConfig + public requestConfig: RequestConfig, + private readonly generateMipmap: boolean = false ) { super(resource); } @@ } else { return loadImageFromBuffer(buffer).then((img) => { texture.setImageSource(img); - texture.generateMipmaps(); + this.generateMipmap && texture.generateMipmaps(); return texture; }); }Also applies to: 60-96, 108-127
🤖 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/loader/src/Texture2DLoader.ts` around lines 49 - 52, The loader currently discards the original mipmap decision and always regenerates mipmaps during restore; update the flow so the mipmap boolean chosen at load time is stored and reused by the restorer. Concretely: when creating the texture in loadImageFromBuffer and other load paths (calls to _createTexture and instantiation of Texture2DContentRestorer), capture the computed mipmap flag (from supportGenerateMipmapsWithCorrection(...) or explicit mipmap option) and pass that flag into Texture2DContentRestorer. Then change Texture2DContentRestorer.restoreContent to use the stored mipmap flag instead of recomputing/generating mipmaps unconditionally. Ensure all places that add a Texture2DContentRestorer (the other load branches) are updated to include this stored flag.tests/src/core/audio/AudioSourcePendingPlayback.test.ts-90-109 (1)
90-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
window.AudioContextin teardown.
beforeEach()overwrites the global constructor, butafterEach()never puts it back. That leaks the mock into later suites and makes them order-dependent.🤖 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/audio/AudioSourcePendingPlayback.test.ts` around lines 90 - 109, The test setup overrides the global constructor by assigning MockAudioContext to window.AudioContext in beforeEach but never restores it; capture the original constructor (e.g., save window.AudioContext to a variable like originalAudioContext at top or at start of beforeEach) and restore it in afterEach by reassigning window.AudioContext = originalAudioContext (or delete window.AudioContext if it was undefined originally) so the mock does not leak into other suites; update the afterEach alongside the existing vi.restoreAllMocks()/useRealTimers() cleanup to perform this restore.packages/core/src/clone/CloneManager.ts-165-194 (1)
165-194:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDeep-cloning
Map/Setstill aliases nested mutable entries.These branches only allocate a new container. Non-
_remapkeys/values are copied by reference, soCloneMode.Deepstill shares nested objects insideMapandSet.🤖 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 165 - 194, The Map/Set branches only allocate new containers but still copy nested objects by reference when they don't implement _remap, so CloneMode.Deep doesn't actually deep-clone entries; modify the Map (targetPropertyM) and Set (targetPropertyS) loops to, for each entry where value or key is an object and does NOT have a _remap, invoke the same recursive cloning routine used elsewhere in CloneManager (the per-property deep-clone function used with srcRoot/targetRoot and CloneMode.Deep) to produce a cloned key/value before inserting into the new Map/Set, while keeping the existing _remap path (i.e., prefer (<ICustomClone>...)._remap(srcRoot, targetRoot) when present).packages/core/src/animation/internal/AnimatorLayerData.ts-15-17 (1)
15-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a null-prototype map for state names.
animatorStateDataMapis now keyed by arbitrary state names, but{}makes keys like__proto__andconstructorspecial. A user-authored controller can corrupt lookups here.🛡️ Suggested fix
- animatorStateDataMap: Record<string, AnimatorStateData> = {}; + animatorStateDataMap: Record<string, AnimatorStateData> = Object.create(null);🤖 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/animation/internal/AnimatorLayerData.ts` around lines 15 - 17, animatorStateDataMap is currently initialized with {} which allows prototype keys like "__proto__" to collide; update the AnimatorLayerData implementation so animatorStateDataMap uses a null-prototype object or an actual Map to avoid prototype pollution: replace the current Record<string, AnimatorStateData> = {} initialization with either Record<string, AnimatorStateData> = Object.create(null) (keeping the same type) or change the field to Map<string, AnimatorStateData> and initialize it with new Map() and update any lookup/assignment code that references animatorStateDataMap accordingly.packages/core/src/clone/CloneManager.ts-108-139 (1)
108-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep explicit clone decorators authoritative.
@ignoreCloneno longer wins for remappable refs because_remapruns first, and Lines 128-138 can upgrade@shallowCloneinto deep cloning. That breaks the documented contract of both decorators.✂️ Suggested fix
- // 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 + // 1. Explicit ignore must stay authoritative if (cloneMode === CloneMode.Ignore) return; - // 3. Primitives / null / undefined - direct assign + // 2. Remappable references (Entity/Component) + if (sourceProperty instanceof Object && (<ICustomClone>sourceProperty)._remap) { + target[k] = (<ICustomClone>sourceProperty)._remap(srcRoot, targetRoot); + return; + } + + // 3. Primitives / null / undefined - direct assign if (!(sourceProperty instanceof Object)) { target[k] = sourceProperty; return; } @@ - } 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; - } }🤖 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 108 - 139, The current logic upgrades a decorated shallow clone into deep because the upgrade branch checks effectiveCloneMode !== CloneMode.Assignment rather than whether the cloneMode was explicitly provided; to fix this, make explicit decorators authoritative by only performing the "upgrade to Deep when target already has independent same-type instance" logic when the incoming cloneMode was not explicitly provided (i.e., cloneMode === undefined / inferred). Concretely, in CloneManager (the block that sets effectiveCloneMode and then checks targetProperty to maybe set effectiveCloneMode = CloneMode.Deep), change the condition so the upgrade runs only when the original cloneMode parameter was undefined (use that original flag or a local isExplicit := cloneMode !== undefined) and keep Assignment behavior unchanged; reference symbols: cloneMode, effectiveCloneMode, CloneMode, CloneManager._inferCloneMode, sourceProperty, target[k], and _remap.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts-308-321 (1)
308-321:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the optional
componentsarray.
IBasicEntity.componentsis optional in the schema._addComponents()currently assumes it is always present and will throw oncomponents.lengthfor valid files that omit an empty component list.Suggested fix
private _addComponents(entity: Entity, components: IEntity["components"]): void { + if (!components?.length) { + return; + } + const context = this.context; const componentMap = context.components; const componentConfigMap = context.componentConfigMap;🤖 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/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 308 - 321, The _addComponents method assumes components is always present and will throw when IEntity["components"] is undefined; add a guard at the top of _addComponents(entity: Entity, components: IEntity["components"]) to return early if !components or components.length === 0. Locate the function named _addComponents and add the check before using componentMap, componentConfigMap, Loader.getClass(...) and calling this._addComponentPlugin so the rest of the loop only runs when a valid components array is provided.packages/loader/src/HDRDecoder.ts-182-189 (1)
182-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip
\rwhen reading HDR header lines.
_readStringLine()currently keeps carriage returns, soparseHeader()will reject valid CRLF-terminated HDR files:"FORMAT=32-bit_rle_rgbe\r"no longer matches, and the blank separator line is never seen as empty.Suggested fix
private static _readStringLine(uint8array: Uint8Array, startIndex: number): string { let line = ""; for (let i = startIndex, n = uint8array.length; i < n; i++) { const character = String.fromCharCode(uint8array[i]); if (character === "\n") break; - line += character; + if (character !== "\r") { + line += character; + } } return line; }🤖 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/loader/src/HDRDecoder.ts` around lines 182 - 189, _readStringLine currently preserves carriage returns which breaks parseHeader's detection of lines (e.g., "FORMAT=32-bit_rle_rgbe\r") and blank separator lines; update HDRDecoder._readStringLine to not include '\r' characters when accumulating the line (or trim a trailing '\r' before returning) so lines returned to parseHeader are normalized and CRLF-terminated files are accepted. Ensure the change affects the HDRDecoder._readStringLine method only and that parseHeader will see an empty string for a CRLF blank separator.packages/core/src/Entity.ts-572-590 (1)
572-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
_isActiveChangingin afinallyblock.
_setActiveInHierarchy()and_setInActiveInHierarchy()can run user code through component/script enable/disable hooks. If one of those throws, this flag staystrueforever and every later active-state change on the entity will fail immediately.Suggested fix
_processActive(activeChangeFlag: ActiveChangeFlag): void { if (this._isActiveChanging) { throw "Note: can't set the 'main inActive entity' active in hierarchy, if the operation is in main inActive entity or it's children script's onDisable Event."; } - this._isActiveChanging = true; - this._setActiveInHierarchy(activeChangeFlag); - this._isActiveChanging = false; + this._isActiveChanging = true; + try { + this._setActiveInHierarchy(activeChangeFlag); + } finally { + this._isActiveChanging = false; + } } @@ _processInActive(activeChangeFlag: ActiveChangeFlag): void { if (this._isActiveChanging) { throw "Note: can't set the 'main active entity' inActive in hierarchy, if the operation is in main active entity or it's children script's onEnable Event."; } - this._isActiveChanging = true; - this._setInActiveInHierarchy(activeChangeFlag); - this._isActiveChanging = false; + this._isActiveChanging = true; + try { + this._setInActiveInHierarchy(activeChangeFlag); + } finally { + this._isActiveChanging = false; + } }🤖 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 572 - 590, Both _processActive and _processInActive leave this._isActiveChanging set to true if _setActiveInHierarchy/_setInActiveInHierarchy throws; wrap the call and the flag reset in a try/finally so this._isActiveChanging is always set back to false (and rethrow the original error after finally). Modify the bodies of _processActive and _processInActive to set this._isActiveChanging = true before calling _setActiveInHierarchy/_setInActiveInHierarchy, call them inside a try, and reset this._isActiveChanging = false in finally to guarantee cleanup even on exceptions.packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts-21-38 (1)
21-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait transform mutations before resolving
parseEntity.
parsePropsAndMethods()is async, but this path drops its promise and returns theEntityimmediately. Any async transform prop/method resolution can still be running while callers continue wiring the hierarchy, so initialization order becomes nondeterministic.Suggested fix
- parseEntity(entityConfig: IEntity): Promise<Entity> { - return this._getEntityByConfig(entityConfig).then((entity) => { + parseEntity(entityConfig: IEntity): Promise<Entity> { + return this._getEntityByConfig(entityConfig).then(async (entity) => { entity.isActive = entityConfig.isActive ?? true; const transform = entity.transform; const transformConfig = entityConfig.transform; if (transformConfig) { - this.parsePropsAndMethods(transform, transformConfig); + await this.parsePropsAndMethods(transform, transformConfig); } else { const { position, rotation, scale } = entityConfig; if (position) transform.position.copyFrom(position); if (rotation) transform.rotation.copyFrom(rotation); if (scale) transform.scale.copyFrom(scale); } entity.layer = entityConfig.layer ?? entity.layer; // `@ts-ignore` this._context.type === ParserType.Prefab && entity._markAsTemplate(this._context.resource); return entity; }); }🤖 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/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 21 - 38, parseEntity returns before async transform mutations complete because parsePropsAndMethods is called but not awaited; update parseEntity (the method that calls this._getEntityByConfig(...).then(...)) to await or return the promise from parsePropsAndMethods when transformConfig exists so the Entity is only resolved after parsePropsAndMethods finishes. Ensure the change preserves setting entity.isActive, applying fallback position/rotation/scale, assigning entity.layer, and still calls entity._markAsTemplate(this._context.resource) when this._context.type === ParserType.Prefab.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts-65-79 (1)
65-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
Layer.Layer0explicitly instead of truthiness-checking it.
layeris numeric, soLayer0is0. The current check skips valid overrides back to layer 0, which breaks prefab/scene modifications when the existing entity is already on a non-zero layer.Suggested fix
- if (entityConfig.layer) entity.layer = entityConfig.layer; + if (entityConfig.layer !== undefined) entity.layer = entityConfig.layer;🤖 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/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 65 - 79, In _applyEntityData, the code uses truthiness to decide whether to set entity.layer (if (entityConfig.layer) ...), which skips valid numeric layer 0; change the guard to check for undefined (e.g. if (entityConfig.layer !== undefined) entity.layer = entityConfig.layer) so a config value of Layer.Layer0 (0) is applied to Entity.layer; update the condition in the _applyEntityData method referencing entityConfig.layer and Entity.layer accordingly.packages/loader/src/gltf/parser/GLTFSceneParser.ts-199-201 (1)
199-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude the skinned-mesh node transform in the fallback bounds conversion.
mesh.boundsare in the renderer entity's local space, not inrootBonespace. Using onlyinverse(rootBone.worldMatrix)drops the skinned-mesh node transform, solocalBoundsbecomes wrong wheneverrootBoneis outsidebonesand the renderer node is not identity relative to it.Suggested fix
} else { + const toRootBoneSpace = new Matrix(); const inverseRootBoneWorld = new Matrix(); Matrix.invert(rootBone.transform.worldMatrix, inverseRootBoneWorld); - BoundingBox.transform(mesh.bounds, inverseRootBoneWorld, skinnedMeshRenderer.localBounds); + Matrix.multiply(inverseRootBoneWorld, skinnedMeshRenderer.entity.transform.worldMatrix, toRootBoneSpace); + BoundingBox.transform(mesh.bounds, toRootBoneSpace, skinnedMeshRenderer.localBounds); }🤖 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/loader/src/gltf/parser/GLTFSceneParser.ts` around lines 199 - 201, The current fallback uses only inverseRootBoneWorld (Matrix.invert(rootBone.transform.worldMatrix)) which ignores the skinned-mesh node's world transform and yields wrong localBounds; compute a combined transform that maps the renderer's local space into the rootBone local space (i.e., first get the skinned-mesh node's world matrix from the renderer node, multiply it by inverseRootBoneWorld or multiply inverseRootBoneWorld by the node world depending on matrix convention), then pass that combined matrix to BoundingBox.transform(mesh.bounds, combinedMatrix, skinnedMeshRenderer.localBounds) so the renderer node transform is included when converting mesh.bounds.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts-251-275 (1)
251-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the full prefab ref through and register the nested dependency.
IRefEntitycarrieskeyandisClone, but this path only forwards{ url }, so nested prefab/GLTF subresource refs no longer resolve the same way asReflectionParser._getEntityByConfig(). This branch also never associates the loadedPrefabResource/GLTFResourcewith the owning prefab resource, so nested asset lifetime tracking is lost.Suggested fix
- .getResourceByRef<Entity>({ - url: assetUrl - }) + .getResourceByRef({ + url: assetUrl, + key: entityConfig.key, + isClone: entityConfig.isClone + }) .then((prefabResource: PrefabResource | GLTFResource) => { + if (this.context.type === ParserType.Prefab) { + // `@ts-ignore` + this.context.resource._addDependenceAsset(prefabResource); + } const entity = prefabResource instanceof PrefabResource ? prefabResource.instantiate()🤖 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/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts` around lines 251 - 275, The _parsePrefab path currently calls engine.resourceManager.getResourceByRef with only { url: assetUrl }, losing IRefEntity fields (key, isClone) and failing to register the loaded PrefabResource/GLTFResource as a nested dependency; update the call in _parsePrefab to pass the full IRefEntity (entityConfig) so nested refs resolve the same way as ReflectionParser._getEntityByConfig, and after the resource is loaded associate/link it with the owning prefab resource (using your resource manager's dependency/link API) so lifetime tracking is preserved and nested prefab/GLTF subresources are registered (keep the rest of the logic that instantiates, creates ParserContext, sets _prefabContextMap and resolves _prefabPromiseMap).packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts-48-73 (1)
48-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix image-backed Texture2D decode: reject on
Imagefailure, revoke object URLs, and cache in_objectPool.
- The image-backed path never attaches
img.onerror, so theAssetPromisecan hang forever on a broken base/mip image.URL.createObjectURL(...)is never revoked (leaks).- The successful decode is not registered into
engine.resourceManager._objectPool[url], unlike the pixel-buffer branch, so it can’t be found later by URL.Suggested fix
} else { - const blob = new window.Blob([imagesData[0]]); - const img = new Image(); - img.onload = () => { - texture2D.setImageSource(img); - let completedCount = 0; - const onComplete = () => { - completedCount++; - if (completedCount >= mipCount) { - resolve(texture2D); - } - }; - onComplete(); - if (mipmap) { - texture2D.generateMipmaps(); - for (let i = 1; i < mipCount; i++) { - const blob = new window.Blob([imagesData[i]]); - const img = new Image(); - img.onload = () => { - texture2D.setImageSource(img, i); - onComplete(); - }; - img.src = URL.createObjectURL(blob); - } - } - }; - img.src = URL.createObjectURL(blob); + const loadImage = (data: Uint8Array) => + new Promise<HTMLImageElement>((resolveImage, rejectImage) => { + const objectURL = URL.createObjectURL(new window.Blob([data])); + const img = new Image(); + img.onload = () => { + URL.revokeObjectURL(objectURL); + resolveImage(img); + }; + img.onerror = (e) => { + URL.revokeObjectURL(objectURL); + rejectImage(e); + }; + img.src = objectURL; + }); + + loadImage(imagesData[0]) + .then((baseImage) => { + texture2D.setImageSource(baseImage); + if (mipmap) { + texture2D.generateMipmaps(); + } + return Promise.all( + imagesData.slice(1).map((data, index) => + loadImage(data).then((mipImage) => texture2D.setImageSource(mipImage, index + 1)) + ) + ); + }) + .then(() => { + // `@ts-ignore` + engine.resourceManager._objectPool[url] = texture2D; + resolve(texture2D); + }, reject); }🤖 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/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts` around lines 48 - 73, The image-backed decode in TextureDecoder must handle image load errors, revoke created object URLs, and cache the finished Texture2D in engine.resourceManager._objectPool keyed by the source URL; update the img handling in the block that creates Image() (base and mip images) to attach img.onerror to call reject(err) on the surrounding AssetPromise (or pass an Error describing which mip failed), call URL.revokeObjectURL(img.src) in both onload and onerror, and after the final onComplete resolves, store the texture into engine.resourceManager._objectPool[url] (same key used by the pixel-buffer branch); ensure the mip loop also revokes each created blob URL on load/error and propagates failures to reject so the promise never hangs.packages/core/src/animation/Animator.ts-492-520 (1)
492-520:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClip-change listeners are never unsubscribed.
At Line 519, listeners are registered on each state data build, but there is no matching unsubscribe in reset/destroy paths (Line 336-Line 354, Line 367-Line 374). This can leak closures and trigger duplicate handler rebuilds over time.
Also applies to: 336-354, 367-374
🤖 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/animation/Animator.ts` around lines 492 - 520, The clipChangedListener created in _saveAnimatorEventHandlers is added to state._updateFlagManager but never removed, causing leaks and duplicate rebuilds; modify _saveAnimatorEventHandlers to return the created listener (or store it on AnimatorState/AnimatorStateData, e.g. attach to animatorStateData._clipChangedListener) and ensure reset/destroy paths (methods handling state teardown referenced around lines handling reset/destroy) call state._updateFlagManager.removeListener(listener) (or use that stored reference) when the state is cleared/destroyed so the listener is unsubscribed and closures are released.packages/core/src/animation/Animator.ts-232-242 (1)
232-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
findAnimatorStatecan return the wrong state play-data.At Line 240-Line 241, if the target state exists but is not current
srcPlayData/destPlayData, the method still returnssrcPlayData(which may belong to a different state). This can silently mutate/read the wrong state instance.💡 Suggested fix
findAnimatorState(stateName: string, layerIndex: number = -1): AnimatorStatePlayData { const { state, layerIndex: foundLayer } = this._getAnimatorStateInfo(stateName, layerIndex); if (!state || foundLayer < 0) return null; const layerData = this._animatorLayersData[foundLayer]; if (!layerData) return null; - // Check srcPlayData and destPlayData for the matching state + // Check srcPlayData and destPlayData for the matching state if (layerData.srcPlayData.state === state) return layerData.srcPlayData; if (layerData.destPlayData.state === state) return layerData.destPlayData; - // State exists in controller but not currently playing — return srcPlayData initialized with the state - return layerData.srcPlayData; + return null; }🤖 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/animation/Animator.ts` around lines 232 - 242, findAnimatorState currently returns layerData.srcPlayData when the requested state exists in the controller but isn’t equal to srcPlayData or destPlayData, which can return the wrong instance; update findAnimatorState to, after obtaining { state, layerIndex: foundLayer } from _getAnimatorStateInfo and validating layerData, if neither layerData.srcPlayData.state nor layerData.destPlayData.state matches state, create and return a new AnimatorStatePlayData (or a properly initialized play-data object) whose .state is the found state (do not return the existing srcPlayData instance), ensuring you use the AnimatorStatePlayData constructor/initialization used elsewhere and preserve null checks for state and layerData.
🟡 Minor comments (4)
packages/loader/src/SpriteLoader.ts-35-35 (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType inference for
getResourceByRefisn’t possible without explicit generics
getResourceByRef<T extends EngineObject>(ref: { url: string; ... }): AssetPromise<T>can’t inferTfrom therefargument becauseTonly appears in the return type. Inpackages/loader/src/SpriteLoader.ts(lines 35 and 47), the calls are also preceded by//@ts-ignore``, so the annotated.then((atlas: SpriteAtlas) => ...)/ `.then((texture: Texture2D) => ...)` isn’t enforced by the actual return type—reintroduce explicit generics or adjust the API typing to make the returned asset type safe.🤖 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/loader/src/SpriteLoader.ts` at line 35, Calls to getResourceByRef lack an explicit generic so TypeScript can’t infer the returned AssetPromise type; update the SpriteLoader usage to call getResourceByRef with an explicit type parameter (e.g., getResourceByRef<SpriteAtlas>(...) and getResourceByRef<Texture2D>(...)) and remove the surrounding // `@ts-ignore` so the .then callbacks (atlas: SpriteAtlas / texture: Texture2D) are type-checked, or alternatively adjust the getResourceByRef signature to surface the asset generic from the ref argument so callers don’t need to specify generics.tests/src/loader/SceneParser.test.ts-4-5 (1)
4-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkipped placeholder test provides no regression signal.
The new suite currently adds no active coverage. Please either implement this test now or convert it to
it.todo(...)and track it outside this PR.If you want, I can draft a concrete test case for the deleted-component reference path and propose assertions.
🤖 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/loader/SceneParser.test.ts` around lines 4 - 5, The skipped placeholder test "should load project successfully with component be deleted" in tests/src/loader/SceneParser.test.ts provides no coverage; either implement the test body for the deleted-component reference path inside the existing test (replace it.skip with it and add assertions that SceneParser loads the project and handles deleted components gracefully), or change it.skip to it.todo("should load project successfully with component be deleted") so CI records the pending test; look for the test identifier string and the use of it.skip to locate and update the test accordingly.packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts-93-95 (1)
93-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
booleantoIMaterialSchema.shaderData[*].valueto matchMaterialLoaderType.Boolean.
MaterialLoaderTypeincludesBoolean,MaterialLoader.tsconsumes it asvalue ? 1 : 0, butIMaterialSchema.shaderData[*].valueomitsboolean, rejecting valid boolean shader payloads.🔧 Proposed fix
shaderData: { [key: string]: { type: MaterialLoaderType; - value: IVector3 | IVector2 | IColor | number | IAssetRef; + value: IVector3 | IVector2 | IColor | number | boolean | IAssetRef; }; };🤖 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/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts` around lines 93 - 95, The IMaterialSchema currently defines shaderData[*].value without boolean, but MaterialLoaderType includes Boolean and MaterialLoader.ts expects booleans (it uses value ? 1 : 0); update the IMaterialSchema type for shaderData entries so that the value union adds boolean (e.g., include "boolean" alongside IVector3 | IVector2 | IColor | number | IAssetRef) so boolean shader payloads validate correctly; adjust any associated types or tests that rely on IMaterialSchema.shaderData to accept the new boolean variant.packages/loader/src/Texture2DLoader.ts-19-29 (1)
19-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRevoke the blob URL on failed image decodes.
Line 27 rejects without releasing the
createObjectURL()result, so repeated bad/aborted image loads leak blob URLs in the page.♻️ Suggested fix
function loadImageFromBuffer(buffer: ArrayBuffer): AssetPromise<HTMLImageElement> { return new AssetPromise((resolve, reject) => { const blob = new Blob([buffer]); const img = new Image(); + const objectURL = URL.createObjectURL(blob); img.onload = () => { - URL.revokeObjectURL(img.src); + URL.revokeObjectURL(objectURL); resolve(img); }; - img.onerror = reject; - img.src = URL.createObjectURL(blob); + img.onerror = (e) => { + URL.revokeObjectURL(objectURL); + reject(e); + }; + img.src = objectURL; }); }🤖 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/loader/src/Texture2DLoader.ts` around lines 19 - 29, The loadImageFromBuffer function leaks blob URLs on failed decodes because the img.onerror handler calls reject without revoking the URL; update loadImageFromBuffer so that both img.onload and img.onerror revoke URL.revokeObjectURL(img.src) before resolving/rejecting (and pass the error/event into reject), ensuring the blob created for img.src is always released; reference the img.onload, img.onerror handlers and URL.revokeObjectURL usage in loadImageFromBuffer to implement this change.
🧹 Nitpick comments (1)
tests/src/loader/StrippedEntity.test.ts (1)
67-128: ⚡ Quick winGuard test cleanup with
try/finallyto avoid cache leakage on assertion failures.At Line 127/204/275, cleanup runs only on success. Wrap each test body after
cachePrefab(...)intry/finallysoremoveCachedPrefab(...)androot.destroy()always execute.💡 Suggested pattern
- const nestedPrefab = await PrefabParser.parse(engine, "char.prefab", nestedPrefabData); - cachePrefab("char.prefab", nestedPrefab); - ... - const outerPrefab = await PrefabParser.parse(engine, "outer.prefab", outerPrefabData); - const root = outerPrefab.instantiate(); - ... - root.destroy(); - removeCachedPrefab("char.prefab"); + const nestedPrefab = await PrefabParser.parse(engine, "char.prefab", nestedPrefabData); + cachePrefab("char.prefab", nestedPrefab); + let root; + try { + ... + const outerPrefab = await PrefabParser.parse(engine, "outer.prefab", outerPrefabData); + root = outerPrefab.instantiate(); + ... + } finally { + root?.destroy(); + removeCachedPrefab("char.prefab"); + }Also applies to: 139-205, 216-276
🤖 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/loader/StrippedEntity.test.ts` around lines 67 - 128, The test can leak cached prefabs and not destroy instantiated roots on assertion failure; wrap the test body that begins after cachePrefab("char.prefab", ...) in a try/finally so that removeCachedPrefab("char.prefab") and root.destroy() always run; locate the block around PrefabParser.parse/outerPrefab.instantiate() and ensure the finally calls root.destroy() (guarding if root is defined) and removeCachedPrefab("char.prefab") so cleanup happens even when expectations fail; apply the same try/finally pattern to the other similar tests that call cachePrefab/removeCachedPrefab (the blocks referenced at the other ranges).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Remove dead parseProperty/renderState in MaterialLoader - Restore revokeObjectURL in GLTFUtils.loadImageBuffer - Add null guard in Animator._crossFade - Guard playSpeed=0 division in _updatePlayingState - Clean up clipChangedListener in Animator._reset
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Revert non-gltf loader changes and restore misdeleted files back to dev/2.0 state.
…ture AssetType enum has no Texture2D member; this was an incorrect change from fix/shaderlab.
luzhuang
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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/animation/Animator.ts (1)
238-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback returns play data for a different state than requested.
When the requested state exists in the controller but is not currently playing in
srcPlayDataordestPlayData, the fallback at line 241 returnssrcPlayDatawhich may contain a completely different state. The JSDoc promises "Per-instance play data for a state by name" but this can return play data for the wrong state.If caller requests state "Run" while "Walk" is playing,
srcPlayData.stateis "Walk" but we return it as the result for "Run".Consider returning
nullwhen the state is not currently active, consistent with the JSDoc's "or null if not found" contract:if (layerData.srcPlayData.state === state) return layerData.srcPlayData; if (layerData.destPlayData.state === state) return layerData.destPlayData; - // State exists in controller but not currently playing — return srcPlayData initialized with the state - return layerData.srcPlayData; + // State exists in controller but not currently playing + return null;🤖 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/animation/Animator.ts` around lines 238 - 242, The fallback in Animator.ts currently returns layerData.srcPlayData when neither layerData.srcPlayData.state nor layerData.destPlayData.state matches the requested state, which can return play data for the wrong state; change the method in the Animator class that queries per-instance play data (the code referencing layerData.srcPlayData, layerData.destPlayData, srcPlayData.state and destPlayData.state) to return null instead of returning srcPlayData when no active playData matches the requested state, and update the method's return handling/comments to reflect the "or null if not found" contract so callers can detect a missing active state.
🤖 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.
Outside diff comments:
In `@packages/core/src/animation/Animator.ts`:
- Around line 238-242: The fallback in Animator.ts currently returns
layerData.srcPlayData when neither layerData.srcPlayData.state nor
layerData.destPlayData.state matches the requested state, which can return play
data for the wrong state; change the method in the Animator class that queries
per-instance play data (the code referencing layerData.srcPlayData,
layerData.destPlayData, srcPlayData.state and destPlayData.state) to return null
instead of returning srcPlayData when no active playData matches the requested
state, and update the method's return handling/comments to reflect the "or null
if not found" contract so callers can detect a missing active state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03b05a77-20b0-4a05-a92d-3b96c56f8486
📒 Files selected for processing (6)
packages/core/src/animation/Animator.tspackages/core/src/animation/internal/AnimatorStateData.tstests/src/core/particle/ParticleTextureSheetAnimation.test.tstests/src/core/particle/RateOverTimeReplay.test.tstests/src/loader/PrefabResource.test.tstests/src/loader/RenderTargetLoader.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/src/loader/RenderTargetLoader.test.ts
- tests/src/core/particle/RateOverTimeReplay.test.ts
- tests/src/loader/PrefabResource.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
从 fix/shaderlab 摘出 animation/physics/audio/gltf 四个子系统的修复，收窄成独立 PR。physics（teleport-vs-sweep 拆 _syncEntityTransformToNative、Collision normal 方向、PhysX kinematic CCD）、audio（前后台生命周期 + pending playback）、gltf（skinned bounds 用 rootBone 逆世界矩阵、LCA 含 skinned-mesh 节点）这三块方向合理、基本 self-contained。阻塞项集中在 animation，外加两个非 animation 的 scope 问题。
问题
P0
physics-physx/package.json/physics-lite/package.json版本被降级2.0.0-alpha.33→0.0.0-experimental-2.0-game.14（已核对 HEAD，physx 第 3 行就是）。这是从 game 分支 cherry-pick 带进来的产物，绝不能合进 dev/2.0。改回 alpha 版本。
P1
-
Animator.ts:241—findAnimatorState的 fallback 返回错误 state 的 playDataif (layerData.srcPlayData.state === state) return layerData.srcPlayData; if (layerData.destPlayData.state === state) return layerData.destPlayData; // State exists in controller but not currently playing — return srcPlayData initialized with the state return layerData.srcPlayData; // ← bug
查询的 state 存在但未在播时，返回的是当前正在播的
srcPlayData，它的.state是另一个动画。注释说 "srcPlayData initialized with the state" 与代码不符——根本没初始化成查询的 state。调用方拿去改.speed/.wrapMode会静默作用到错误的动画上。应return null。 -
AnimatorStateMachine.removeState—delete this._statesMap[name]移到if (index > -1)块外，且不再 nulldefaultState拿一个 stale
state（index===-1）调用时，仍会把 map 里同名的新 state删掉。加上defaultState类型从AnimatorState | null改成非空但 removeState 不再清它 → 悬挂 default 隐患。 -
自/活跃-dest crossFade 的 no-op 守卫被删除，且无替代测试
老代码里
if srcPlayData/destPlayData.state === crossState return false的守卫连同 3 个测试一起删了，没有补充测试说明新行为。如果是有意对齐 Unity，请补一个行为测试或在描述里标 breaking change。 -
测试质量两条（链路 bug 必须用公开 API 测）：
Animator.test.ts"find animator state"：expect(animatorState).to.eq(currentAnimatorState)拿findAnimatorState（返回AnimatorStatePlayData）和getCurrentAnimatorState（返回AnimatorState）比，类型不同永远不等。应比.state。- crossfade-speed 测试仍 reach 进
animator["_animatorLayersData"][0]私有字段，而不是走它本该验证的findAnimatorState公开 API。把实现结构锁死进测试，且 revertfindAnimatorState修复后测试照样绿——没守住回归。
P2
-
Animator.ts:384-385新增的srcPlayData && .../destPlayData && ...空守卫是死代码 — 这俩现在 by construction 非空（AnimatorLayerData急切初始化成new AnimatorStatePlayData()）。是本 PR 新引入的+行冗余，删掉。 -
findAnimatorState上方有两个相邻的/** */JSDoc 块，只有第二个生效，删掉第一个。 -
PhysicsScene.test.ts静默删了约 290 行 raycast/sweep filter-stack 测试。如果这些测试对应的生产代码确实没被 cherry-pick 进来，删除是合理的；但这是一大块静默的覆盖移除，请确认这些测试的被测对象确实不在 dev/2.0，并在描述里说明。（注意同文件里两个既有测试的断言从not.toHaveBeenCalled翻成toHaveBeenCalled——那个是 kinematic-move→setKinematicTarget 修复带来的合理行为变更，已有注释，OK。）
P3
Collision.test.ts/DynamicCollider.test.ts里有一批探索式测试（HYPOTHESIS:/ultrathink probe/R0:-R8:/cocos-parity probe，还有个console.info打 ratio 的），像开发期 scratchpad 而非沉淀的回归用例。建议清理成明确断言的回归测试。
注意（不是问题，是确认对了）
physics 的 _syncEntityTransformToNative 拆分是非对称的：Collider 基类给 teleport 语义，DynamicCollider override 把 kinematic-Target 走 move()，CharacterController 只 override _teleportToEntityTransform（用 setWorldPosition、忽略 rotation）。每个子类确实不同——这是好模式，不是"对称改动"陷阱。唯一确认点：CharacterController 丢 rotation 是否有意。
自检
findAnimatorState:241 与 physics-physx/package.json:3 版本降级均已对照 HEAD 0375f073f 实际源码确认。我 03:34/09:56 两轮 review 针对的就是当前 HEAD，所列 animation 项均未在 HEAD 修复。作者未对任何 inline 回复，无已解释项需排除。_syncEntityTransformToNative 非对称确认为正确模式、不作为问题列出。
2 participants
Summary
从
fix/shaderlab分支抽离 Animation、Physics、Audio、GLTF Loader 相关修复，合入dev/2.0。Animation
AnimatorStatePlayData承载 per-instance 的 speed/wrapMode，替代AnimatorStateInstance代理模式AnimationClip.sampleAnimation()公开 API，支持外部采样动画Animator.fireEvents开关，控制是否触发 AnimationEvent 回调_saveAnimatorEventHandlers用 listener 模式监听 clip 变更，_reset()时正确移除 listener 防止泄漏_crossFade增加 null guard，防止_getAnimatorStateInfo返回空 state 时崩溃_updatePlayingState增加playSpeed === 0除零保护，防止 NaNClearableObjectPool池化复用Physics
Collider增加_teleportToEntityTransform，re-enable 时 teleport 而非 sweepDynamicCollider增加applyForceAtPosition、setKinematicTargetDynamicCollider移除addForce/addTorque中冗余的wakeUp调用Collision碰撞法线方向修正（shape0→shape1 约定）MeshColliderShape增加 cooking 自动重试PhysXPhysics支持 tolerance scale 配置PhysicsMaterial属性同步修复CharacterController增加_teleportToEntityTransformoverrideAudio
AudioManager重构前后台生命周期管理：hide/show 时自动 suspend/resume playing sourcesAudioSourcepending playback 注册到AudioManager，由 context 恢复时统一驱动_suspendPlaybackForInterruption/_resumeInterruptedPlayback机制_initSourceNode增加 offset 边界处理和非循环播放结束保护AudioLoader新增"audio"扩展名支持AudioSourcePendingPlayback测试覆盖 autoplay blocking、前后台恢复、手势解锁等场景GLTF Loader
GLTFSceneParser存储_sceneRoots，_computeSkinnedBounds简化为直接用 rootBone worldMatrix 逆矩阵GLTFSkinParser重构_findSkinRootBoneByLCA，skin root 解析时包含 skinned mesh 节点参与 LCA 计算GLTFExtensionSchema中IGalaceanMaterialRemap改为独立的 url/key 字段（去除 RefItem 依赖）GLTFUtils.loadImageBuffer恢复URL.revokeObjectURL防止 blob URL 泄漏GLTFSkinParser.test.ts测试 rootBone LCA 解析Test plan
npx vitest run tests/src/core/Animator.test.tsnpx vitest run tests/src/core/physics/npx vitest run tests/src/core/audio/npx vitest run tests/src/loader/GLTFLoader.test.tsSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests