fix(animation, loader): 从 #2983 抽离动画与 GLTF 加载器修复#2999
Open
luzhuang wants to merge 84 commits into
Open
Conversation
AnimatorState.speed is part of the shared AnimatorController asset. Modifying it at runtime pollutes all Animator instances sharing the same controller, causing animation speed corruption after cloning. - Add speed field to AnimatorStatePlayData, initialized from AnimatorState.speed on reset - Add proxy properties (name/clip/wrapMode/transitions/addStateMachineScript) - Change speed calculation to playData.speed * animator.speed - findAnimatorState now returns per-instance AnimatorStatePlayData - Export AnimatorStatePlayData for consumer code
Promote AnimatorStatePlayData from a play-slot object to a per-Animator per-state persistent handle. Each AnimatorLayerData holds a state→PlayData map; srcPlayData/destPlayData become nullable references into the map. API: - findAnimatorState(name, layerIdx?) returns AnimatorStatePlayData|null, lazy-creating the handle on first access (works even when the state has never played) - playData.speed is a getter/setter backed by _speedOverride; reads fall back to state.speed (live binding); clearSpeedOverride() resumes tracking the shared default - playData.state.xxx for shared asset access (no proxy properties) - resetForPlay() resets runtime fields only; user overrides survive transitions Bugs fixed: - _updateCrossFadeState now multiplies by playData.speed (was state.speed), so per-instance speed applies during cross-fade - findAnimatorState no longer returns the wrong state's playData when the queried state isn't currently playing (was: fell back to srcPlayData) Lifecycle changes: - AnimatorLayerData.statePlayDataMap caches per-state handles - switchPlayData() replaced by promoteDest() (src ← dest, dest = null) - _preparePlay/_prepareCrossFade get-or-create from the map and assign references rather than reset slot objects Cleanup: - Remove AnimatorStatePlayData proxy properties (name/clip/wrapMode/ transitions/addStateMachineScript) — use playData.state.xxx instead - Drop @todo on findLayerByName and duplicate JSDoc on findAnimatorState
…ernal/ Address code quality review on 57da59a: - AnimatorStatePlayData constructor no longer reads state.clip; clipTime defers to resetForPlay so findAnimatorState doesn't crash for states with no clip yet - Move AnimatorStatePlayData from internal/ to animation/ root since it is now public API returned by findAnimatorState; update imports - Annotate findAnimatorState and getCurrentAnimatorState return types as | null to match runtime behavior - Remove dead && guards in _updateCrossFadeState (layerState guarantees non-null entry) - Tighten AnimatorLayerData field comments
Add 6 regression tests covering the new findAnimatorState handle: - lazy create on first access (state never played) - speed override set before play applies on first play - override survives crossFade out and back - override is per-Animator (clone isolation, shared asset unmutated) - crossFade phase uses playData.speed (was state.speed before fix) - clearSpeedOverride resumes live tracking of state.speed Fix existing call sites broken by proxy removal: tests that accessed state.clip / state.clearTransitions / state.clipStartTime etc. now go through state.state.xxx (the shared AnimatorState). state.speed reads and writes remain on the per-instance handle.
Address code quality review: - Test #1 now uses a cloned animator (no afterEach pre-population) so it actually verifies lazy PlayData creation; rename to match intent - Test #2 drops @ts-ignore on _animatorLayersData by reading the override through the same handle returned by findAnimatorState - Test #5 tightens >0.1 threshold to closeTo(0.2, 0.05) so a regression reducing the multiplier wouldn't slip past - Align .eq/.greaterThan calls with the file's .to.eq/.to.be convention
Previously walk-up went all the way to GLTF_ROOT (the wrapper, no parent), but sceneRootChildren contains GLTF_ROOT's direct children — never GLTF_ROOT itself. Result: function always returned null, making multi-root skin wrapper detection a no-op. Stop the walk as soon as the entity is a direct child of the scene root. The final check then succeeds for joints under any sceneNode, returning the wrapper sceneRoot as rootBone. Verified via standalone reproduction matching the test fixture.
When entity X had a child also named X, findByPath("X") short-circuited to
return self due to the GLTF self-name prefix branch — making the same-name
child unreachable.
Try direct child lookup first; fall back to the self-name prefix only when
the child path doesn't match. Both the GLTF normalized-prefix case and the
same-name child case work correctly.
PR galacean#2984 changed Animator.findAnimatorState() to return AnimatorStatePlayData instead of AnimatorState. Unit tests were already updated to access shared-asset members via `.state.xxx`; e2e cases were missed and would TypeError at runtime when playwright loaded them. Convert each shared-asset access on findAnimatorState() results: - .clip -> .state.clip (animator-event, animator-additive) - .addTransition / .addExitTransition / ._getDuration -> .state.xxx (animator-stateMachine) - .addStateMachineScript -> .state.addStateMachineScript (animator-stateMachineScript) .speed reads/writes are intentionally preserved on the per-instance handle (the whole point of the API change).
…n flag - _prepareCrossFadeByTransition guards against crossFade to current src or dest state, since statePlayDataMap holds a single PlayData per AnimatorState; without the guard, dest aliases to src, resetForPlay clobbers the active runtime, and _updateCrossFadeState updates the same object twice - AnimatorStatePlayData.resetForPlay also resets _changedOrientation so re-entering a state doesn't carry the previous track's orientation flag into the new playback window True self-crossfade support requires splitting persistent override fields from transient src/dest runtime tracks; out of scope for this PR.
When the entity has a child with the same name as splits[0], findByPath must not fallback to the self-prefix interpretation: the user clearly intends to descend into the child, and a deeper-path miss should return null rather than silently re-resolve the path against the entity itself.
PR galacean#2984 changed findAnimatorState to return AnimatorStatePlayData | null. Update both EN and ZH docs to reflect: - Per-instance speed override (playData.speed) - Shared asset access (playData.state.xxx) - Nullable return guard - clearSpeedOverride() to resume live binding to state.speed
findAnimatorState now returns AnimatorStatePlayData | null. e2e cases were dereferencing without a guard, which would surface as "Cannot read properties of null" if a state name doesn't match the asset. Add fail-fast guards naming the missing state for actionable errors.
… multiple roots Previously: if all joints were under any sceneNodes' subtrees, _findSceneRootBone returned GLTF_ROOT, even when joints converged to a single top-level child. That over-promoted the rootBone to include unrelated sibling nodes (lights, cameras, props), affecting bounds. Now: track which top-level child each joint resolves to. Only return sceneRoot when joints span >1 different top-level children. Otherwise fall through to _findSkeletonRootBone for the LCA.
When play() interrupts a cross-fade, destPlayData and crossFadeTransition were left dangling. With persistent statePlayDataMap, this caused the self-crossFade alias guard to wrongly no-op subsequent crossFade calls to the previously-fading state. Clear destPlayData and crossFadeTransition on play() entry so the layer state matches reality.
If the requested state name doesn't match any layer, _getAnimatorLayerData was being called with playLayerIndex = -1, which would write a junk AnimatorLayerData entry at array index -1 (JS array negative indexing creates a property). Guard the lookup at the entry point.
Bring the JSDoc tag in line with the other engine-managed runtime fields on AnimatorStatePlayData (playedTime/clipTime/etc.) so docs/IDE filtering treats them uniformly.
Self-prefix fallback called _findChildByName with pathIndex=1, whose not-found backtrack path recursed into entity.parent — for detached or root entities, that's null and crashes on null._children. Use splits.slice(1) with pathIndex=0 so the recursion stays within the entity's subtree and returns null cleanly when the deeper path misses. Also retitle the fallback comment to a generic path-semantics description, since core/Entity should not carry GLTF-specific framing.
When called with an out-of-range layerIndex, _getAnimatorStateInfo accessed layers[idx].stateMachine and threw. This propagated to findAnimatorState (which is supposed to return null) and to play / crossFade entry points. Bound-check the index and return a stateInfo with layerIndex = -1 / state = null so all three callers see safe behavior.
When per-instance state speed is 0 (paused) and a transition fires, playCostTime / playSpeed produced NaN, which made remainDeltaTime > 0 evaluate false and the destination state silently dropped the remaining delta on that frame. Treat speed=0 as "no time consumed by this state" and pass deltaTime through to the destination instead.
GLTFSkinParser._findSceneRootBone reads glTFResource._sceneRoots which GLTFSceneParser populates synchronously. The current AssetPromise.all ordering preserves this; document the invariant so a future array reorder doesn't silently break skin root resolution.
…hine Bring local AnimatorStateTransition declarations into line with the project's camelCase convention.
…te(null) idiom AnimatorLayerData already used Record-style maps for animatorStateDataMap and curveOwnerPool; statePlayDataMap was the only Map in the animation module. Layer-internal stateName is canonical (AnimatorStateMachine deduplicates by name). Switch to the project's standard pattern for intra-class consistency and v8 hidden-class friendliness on small caches. Also normalize animatorStateDataMap initialization to Object.create(null) for the same null-prototype safety as curveOwnerPool.
The example showed `playData.speed = 0` immediately followed by `playData.clearSpeedOverride()`, which silently cancels the override. Comment out the resume call and label it as a later-stage operation so copy-pasting actually pauses the state.
The previous comment phrased the guard as a temporary workaround. The behavior is in fact deliberate: per-state persistent PlayData makes self-cross-fade structurally inexpressible without a separate transient track. Phrase the comment so future readers understand it as policy.
Replace per-scene Set<Entity> creation with parent-walk identity checks. Tracks first-encountered top-level joint root and compares subsequent joints by reference, returning sceneRoot the moment a divergent root is found.
Replace splits.slice(1) + _findChildByName(pathIndex=1) with a dedicated subtree-only path-search helper. Two improvements: no array allocation on every fallback, and the "fallback never backtracks to siblings" semantic is now expressed in the helper's contract instead of relying on the caller to neutralize backtracking via slicing.
- Restore AnimatorState as the shared asset (original name, unchanged semantics). Editor/asset code reaches it through the controller path. - Add AnimatorStateInstance: the per-Animator view returned by findAnimatorState / getCurrentAnimatorState. Writes only affect this Animator; reads of unwritten fields forward to the shared asset. - Rename internal AnimatorStatePlayData (deleted in upstream PR) to AnimatorStateRuntime; it owns the playback runtime and a 1:1 AnimatorStateInstance pair. AnimatorLayerData caches one instance per (layer, state-name). - Update Animator API return types to AnimatorStateInstance | null. - Tests/e2e cases: state-machine wiring uses the controller path (shared asset). Per-Animator playback tweaks use the view. - Docs (en/zh): explain the asset vs instance split and the Renderer.getInstanceMaterial pattern parallel.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…tateInstance Aligns wrapMode with speed: both are playback behavior parameters that can differ per Animator, while structural fields (clip, clipStartTime/EndTime, transitions, scripts) remain asset-only. Also fixes test references to runtime.state that should be runtime.instance after the earlier rename, and simplifies wrapMode writes in tests that no longer need the asset-path workaround.
The earlier rename to AnimatorStateRuntime was unmotivated — "PlayData" already accurately describes the internal class (playedTime, clipTime, playState, ...). Reverts class name, field names (srcPlayData/destPlayData, instance._playData), method getOrCreatePlayData, and local variable names back to the original PR galacean#2999 / dev/2.0 naming.
- Instance owns PlayData: created in Instance.constructor, no more reverse side-effect (PlayData no longer mutates the passed-in instance). - _state and _playData on Instance are now readonly. - AnimatorLayerData.getOrCreatePlayData renamed to getOrCreateInstance so the return type matches the method name. Call sites take ._playData themselves when they need the runtime data.
…uard When _getAnimatorStateInfo returns state!=null, layerIndex is always >= 0, so the second branch never fires on its own. Aligns findAnimatorState and _crossFade with the sibling play() function's single !state check.
… mutations stateMachine.addState/removeState now flow up to the controller's update flag via a parallel _setController injection chain (mirroring the existing _setEngine pattern). This fixes the silent staleness when users do removeState + addState same-name at runtime — the controller update flag now triggers a full _reset on the next Animator entry, which transparently clears all caches (stateDataMap, instanceMap, ...). Drops two now-redundant identity checks: - Animator._getAnimatorStateData: stateData.state !== animatorState - AnimatorLayerData.getOrCreateInstance: instance._state !== state Treats the root cause instead of patching each cache's lookup path, and aligns stateMachine mutations with the existing addLayer/removeLayer dispatch pattern on AnimatorController.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace listener-based clip change notification with lazy version pull, and switch AnimatorState-keyed maps from Record<string,T> to WeakMap. - UpdateFlagManager: add monotonic `_version` counter, bumped on dispatch - AnimatorStateData: drop clipChangedListener field and dispose method; add `eventsBuiltVersion` snapshot for lazy invalidation - Animator: replace `_saveAnimatorEventHandlers` (listener-based) with `_ensureEventHandlersUpToDate` (pull-based version check); drop dispose loop from `_reset` and `_reset` call from `_onDestroy` - AnimatorLayerData: switch animatorStateDataMap and instanceMap to WeakMap<AnimatorState, T> — entries auto-clear when state is GC'd - AnimatorStateMachine / AnimatorControllerLayer / AnimatorController: drop the stateMachine -> controller dispatch chain that was only needed to invalidate stale stateData on `removeState + addState`; WeakMap keying by identity makes that path unnecessary - tests: drop two listener-detach tests; add lazy invalidation test; update WeakMap access patterns Net -85 lines, eliminates listener leak path entirely.
…eEventHandlers The ClearableObjectPool for AnimationEventHandler couldn't actually reuse objects under our usage: pool is shared across stateData yet each stateData rebuilds independently — pool.clear() would alias live handlers across stateData, so we never called it; usedCount grew monotonically until _reset. Net effect: every rebuild allocated new objects and stranded the old ones in the pool. Inline `new` is simpler and equivalent in allocation count. Also rename `_ensureEventHandlersUpToDate` to `_ensureEventHandlers` — `ensure` already implies idempotent "make it correct". - drop _animationEventHandlerPool field and its clear() in _reset - drop ClearableObjectPool import - inline `new AnimationEventHandler()` in the rebuild loop - drop redundant `handlers.length = 0` (fresh instance starts empty)
Leftover from an earlier underscore-prefix → no-prefix field rename: the `_clipTime: clipTime` rename collapsed into `clipTime: clipTime`, which is just noise. Strip the redundant aliases.
…rData The two cross-fade slot fields (destPlayData, crossFadeTransition) were managed by ad-hoc field assignments scattered across Animator. Pull both reset patterns onto AnimatorLayerData where the fields live: - completeCrossFade(): dest promoted to src, slot cleared (replaces promoteDest() + manual `crossFadeTransition = null`) - clearCrossFadeSlot(): slot discarded without promoting dest (replaces _preparePlay's two-line inline reset after a play() interrupts a fade) Also tighten the comment in _preparePlay — the guard that was being defeated is the active-dest check, not "self-target alias".
Spell out the shared-asset + per-instance-override model on the class doc, and add per-getter doc for name/clip/clipStartTime/clipEndTime that were previously undocumented. Speed/wrapMode getters now state the read-through + isolated-write semantics explicitly.
Previously `removeState` deleted the state from both `states` and
`_statesMap` but left `defaultState` pointing at the removed state.
The next implicit default-state play would then dispatch to a state
the user already removed.
`defaultState` is the user's explicit choice ("which state to play
automatically"), not a fallback — auto-reselecting `states[0]` (Unity's
editor-side behavior) would fabricate intent the user didn't express.
Cleared to `null` instead; consumers in Animator already null-check
the field, so the contract is now honored consistently.
Also annotate the field type as `AnimatorState | null` (initialized to
null) to make the nullability explicit to consumers.
…Version Internal class + self-describing field name; sibling fields are all plain-declared. The prose just restated what the call site in _ensureEventHandlers already makes obvious.
The earlier rename and accompanying note ("Per-instance overrides are
preserved") suggested this method preserves overrides as a feature.
But overrides live on AnimatorStateInstance, not PlayData — this
method physically can't touch them. The "ForPlay" qualifier added no
disambiguation either: both call sites are obvious play-entry paths.
Restoring the conventional `reset` name and dropping the misleading
note.
Internal class, methods are 3-4 lines, names self-describe (getOrCreateInstance/completeCrossFade/clearCrossFadeSlot). The comments only restated what name + body already convey. Contextual "why is this called here" notes belong at the call sites, not on the methods themselves.
Member
|
追加 24 个 commits 到本 PR(自 efa4795 起),来自 #3006 的工作,主要在 主要改动主题架构 / API
真 Bug 修复
简化(总计 -85 行架构基础设施)
文档 / 注释
数量
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/src/core/Animator.test.ts (2)
197-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the actual layer index and compare handles consistently.
layerIndexis captured beforeplay(), so after the suite reset it is still the sentinel value instead of layer0. On top of that, Line 205 comparesview._stateto the handle returned bygetCurrentAnimatorState(), so the assertion is wrong for the new API even when the lookup succeeds.Suggested fix
it("find animator state", () => { const stateName = "Survey"; const expectedStateName = "Run"; - const layerIndex = animator["_tempAnimatorStateInfo"].layerIndex; + const layerIndex = 0; animator.play(stateName); const currentAnimatorState = animator.getCurrentAnimatorState(layerIndex); let animatorState = animator.findAnimatorState(stateName, layerIndex); - expect((animatorState as any)._state).to.eq(currentAnimatorState); + expect(animatorState).to.eq(currentAnimatorState); animator.play(expectedStateName); animatorState = animator.findAnimatorState(expectedStateName, layerIndex); - expect((animatorState as any)._state).not.to.eq(currentAnimatorState); - expect((animatorState as any)._state.name).to.eq(expectedStateName); + expect(animatorState).not.to.eq(currentAnimatorState); + expect(animatorState.name).to.eq(expectedStateName); });🤖 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/Animator.test.ts` around lines 197 - 210, The test captures layerIndex from animator["_tempAnimatorStateInfo"] before calling play so it remains a sentinel; update the test to use the real layer (e.g. 0) or re-read layerIndex after calling animator.play(stateName) (symbols: animator, animator["_tempAnimatorStateInfo"], play). Also update the assertions to compare the same kind of value from findAnimatorState and getCurrentAnimatorState: compare the state's handle (or whatever handle property is returned by getCurrentAnimatorState) rather than comparing the full _state object (symbols: findAnimatorState, getCurrentAnimatorState, (animatorState as any)._state, currentAnimatorState) so both expectations use the handle for equality/inequality checks and the final name check uses (animatorState as any)._state.name.
357-367:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the shared clip event and script in this test.
This test mutates the shared
Walkclip and the sharedanimator.entity, but the file-levelafterEachdoes not remove the added event or destroytestScript. Later tests reuse the same animator/controller, so this leaves order-dependent state behind.🤖 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/Animator.test.ts` around lines 357 - 367, The test mutates shared state by adding an AnimationEvent to the shared "Walk" clip and by adding a TestScript component to animator.entity; to avoid leaking state into later tests, store the added event and the created component (testScript) and remove them in this test's cleanup: remove the event from state.clip's event list (or call the clip's remove/unregister event API) and remove/destroy the TestScript component from animator.entity (or call the entity's removeComponent/destroy on testScript) after the assertion so the shared clip and entity are restored for subsequent tests.
♻️ Duplicate comments (1)
packages/core/src/animation/Animator.ts (1)
207-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset controller-change cache before returning current state.
getCurrentAnimatorState()can return stale state data after controller mutations because it reads_animatorLayersDatawithout running_resetIfControllerUpdated()(unlikeplay,update,crossFade, andfindAnimatorStateat lines 116, 191, 219, 366). This is observable if callers query current state before the next update tick.🔧 Proposed fix
getCurrentAnimatorState(layerIndex: number): AnimatorStateInstance | null { + this._resetIfControllerUpdated(); return this._animatorLayersData[layerIndex]?.srcPlayData?.instance ?? 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 207 - 209, getCurrentAnimatorState can return stale data because it doesn't run the controller-change reset logic; update the getCurrentAnimatorState method in Animator (function getCurrentAnimatorState) to call _resetIfControllerUpdated() before reading _animatorLayersData so the controller-change cache is cleared and the returned AnimatorStateInstance (this._animatorLayersData[layerIndex]?.srcPlayData?.instance) is always up-to-date.
🧹 Nitpick comments (1)
packages/core/src/animation/AnimatorStateInstance.ts (1)
61-63: 💤 Low valueConsider adding a method to clear per-instance overrides.
Once
speedorwrapModeis set, there's no way to revert to reading from the underlying_state. Users cannot "unset" the override to restore the fallback behavior. Consider adding a reset method:/** * Clear the per-instance speed override, reverting to the underlying state's speed. */ clearSpeed(): void { this._speed = undefined; } /** * Clear the per-instance wrapMode override, reverting to the underlying state's wrapMode. */ clearWrapMode(): void { this._wrapMode = undefined; }Also applies to: 75-77
🤖 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/AnimatorStateInstance.ts` around lines 61 - 63, The instance currently stores overrides via the setters (speed and wrapMode) in AnimatorStateInstance but offers no way to revert to the underlying _state values; add methods clearSpeed() and clearWrapMode() that set this._speed = undefined and this._wrapMode = undefined respectively, and ensure the existing getters for speed and wrapMode already check for undefined and fall back to this._state.speed / this._state.wrapMode; apply the same pattern referenced around the wrapMode setter (lines ~75-77) so callers can unset per-instance overrides.
🤖 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 `@e2e/case/animator-stateMachineScript.ts`:
- Around line 58-61: The code directly accesses
animator.animatorController.layers[0] before verifying the parent objects exist,
so guard against null/undefined to produce clearer errors: add checks for
animator, animator.animatorController, and
animator.animatorController.layers.length > 0 (or layers[0] truthiness) before
calling findStateByName; if any check fails, throw distinct errors like
"Animator missing", "AnimatorController missing", or "Animator has no layers" so
the failure is actionable, then proceed to call
animator.animatorController.layers[0].findStateByName("walk") and keep the
existing walk-state not found error.
In `@packages/core/src/animation/internal/AnimatorLayerData.ts`:
- Around line 24-45: The field crossFadeTransition is declared non-nullable but
is set to null in completeCrossFade() and clearCrossFadeSlot(); change its type
to allow null (e.g., AnimatorStateTransition | null) and initialize it to null
so the type system matches runtime behavior. Update the declaration in
AnimatorLayerData to be nullable and ensure any usages in _updateCrossFadeState,
_updateCrossFadeFromPoseState, completeCrossFade, and clearCrossFadeSlot handle
the null case (guards remain valid). This prevents potential null-dereference if
future code paths access crossFadeTransition.
---
Outside diff comments:
In `@tests/src/core/Animator.test.ts`:
- Around line 197-210: The test captures layerIndex from
animator["_tempAnimatorStateInfo"] before calling play so it remains a sentinel;
update the test to use the real layer (e.g. 0) or re-read layerIndex after
calling animator.play(stateName) (symbols: animator,
animator["_tempAnimatorStateInfo"], play). Also update the assertions to compare
the same kind of value from findAnimatorState and getCurrentAnimatorState:
compare the state's handle (or whatever handle property is returned by
getCurrentAnimatorState) rather than comparing the full _state object (symbols:
findAnimatorState, getCurrentAnimatorState, (animatorState as any)._state,
currentAnimatorState) so both expectations use the handle for
equality/inequality checks and the final name check uses (animatorState as
any)._state.name.
- Around line 357-367: The test mutates shared state by adding an AnimationEvent
to the shared "Walk" clip and by adding a TestScript component to
animator.entity; to avoid leaking state into later tests, store the added event
and the created component (testScript) and remove them in this test's cleanup:
remove the event from state.clip's event list (or call the clip's
remove/unregister event API) and remove/destroy the TestScript component from
animator.entity (or call the entity's removeComponent/destroy on testScript)
after the assertion so the shared clip and entity are restored for subsequent
tests.
---
Duplicate comments:
In `@packages/core/src/animation/Animator.ts`:
- Around line 207-209: getCurrentAnimatorState can return stale data because it
doesn't run the controller-change reset logic; update the
getCurrentAnimatorState method in Animator (function getCurrentAnimatorState) to
call _resetIfControllerUpdated() before reading _animatorLayersData so the
controller-change cache is cleared and the returned AnimatorStateInstance
(this._animatorLayersData[layerIndex]?.srcPlayData?.instance) is always
up-to-date.
---
Nitpick comments:
In `@packages/core/src/animation/AnimatorStateInstance.ts`:
- Around line 61-63: The instance currently stores overrides via the setters
(speed and wrapMode) in AnimatorStateInstance but offers no way to revert to the
underlying _state values; add methods clearSpeed() and clearWrapMode() that set
this._speed = undefined and this._wrapMode = undefined respectively, and ensure
the existing getters for speed and wrapMode already check for undefined and fall
back to this._state.speed / this._state.wrapMode; apply the same pattern
referenced around the wrapMode setter (lines ~75-77) so callers can unset
per-instance overrides.
🪄 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: 6d511f5b-98b9-43ce-8a8c-452706c4af71
📒 Files selected for processing (15)
docs/en/animation/animator.mdxdocs/zh/animation/animator.mdxe2e/case/animator-additive.tse2e/case/animator-event.tse2e/case/animator-stateMachine.tse2e/case/animator-stateMachineScript.tspackages/core/src/UpdateFlagManager.tspackages/core/src/animation/Animator.tspackages/core/src/animation/AnimatorStateInstance.tspackages/core/src/animation/AnimatorStateMachine.tspackages/core/src/animation/index.tspackages/core/src/animation/internal/AnimatorLayerData.tspackages/core/src/animation/internal/AnimatorStateData.tspackages/core/src/animation/internal/AnimatorStatePlayData.tstests/src/core/Animator.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/zh/animation/animator.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/case/animator-additive.ts
Comment on lines
+58
to
+61
| const walkState = animator.animatorController.layers[0].stateMachine.findStateByName("walk"); | ||
| if (!walkState) { | ||
| throw new Error("Animator state not found: walk"); | ||
| } |
There was a problem hiding this comment.
Guard animator and layer before walk-state lookup.
animator and layers[0] can fail before your explicit "walk" error path, giving a less actionable crash. Add explicit checks first.
Suggested patch
- const animator = defaultSceneRoot.getComponent(Animator);
- const walkState = animator.animatorController.layers[0].stateMachine.findStateByName("walk");
+ const animator = defaultSceneRoot.getComponent(Animator);
+ if (!animator) {
+ throw new Error("Animator component not found on scene root");
+ }
+ const layer = animator.animatorController.layers[0];
+ if (!layer) {
+ throw new Error("Animator layer not found: index 0");
+ }
+ const walkState = layer.stateMachine.findStateByName("walk");
if (!walkState) {
throw new Error("Animator state not found: walk");
}🤖 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 `@e2e/case/animator-stateMachineScript.ts` around lines 58 - 61, The code
directly accesses animator.animatorController.layers[0] before verifying the
parent objects exist, so guard against null/undefined to produce clearer errors:
add checks for animator, animator.animatorController, and
animator.animatorController.layers.length > 0 (or layers[0] truthiness) before
calling findStateByName; if any check fails, throw distinct errors like
"Animator missing", "AnimatorController missing", or "Animator has no layers" so
the failure is actionable, then proceed to call
animator.animatorController.layers[0].findStateByName("walk") and keep the
existing walk-state not found error.
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.
…t + skin index guard - tests: "find animator state" was comparing `(instance as any)._state` (AnimatorState) against `getCurrentAnimatorState` (AnimatorStateInstance), which can never be equal. Also fix the layerIndex source — it was read from `_tempAnimatorStateInfo` *before* play() populated it, so it was -1 instead of 0. - Animator.getCurrentAnimatorState now calls _resetIfControllerUpdated() so it doesn't return a stale instance after a controller mutation, consistent with play/crossFade/findAnimatorState/update. - Add @remarks on both getCurrentAnimatorState and findAnimatorState spelling out that the returned instance is invalidated by controller structure changes (layers added/removed) and must be re-fetched. - GLTFSkinParser: when `skin.skeleton` is an out-of-range index, throw a precise error instead of silently assigning `undefined` to `skin.rootBone` (which surfaces as a confusing error later).
…field refs
Two issues surfaced when running the full Animator suite:
1. animation event test fails — the lazy `_ensureEventHandlers` was only
invoked at play()/crossFade() entry, so a clip.addEvent() after
play() didn't rebuild handlers on the next update. Move the ensure
call into `_fireAnimationEventsAndCallScripts` so update-path
re-checks the clip version too.
2. Three tests still reference fields that moved during the refactor:
- `srcPlayData.speed` — speed is now on the instance, change to
`srcPlayData.instance.speed`
- `layerData.destPlayData?.state.name` (two sites) — PlayData no
longer has `state`, change to `.instance.name`
All 52 Animator tests pass after this commit.
GuoLei1990
reviewed
May 15, 2026
Member
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（2026-05-15）
已关闭问题清单
getCurrentAnimatorState未调用_resetIfControllerUpdated()：已修复 —Animator.ts:209已补上调用findAnimatorStateJSDoc 未说明 controller mutation 后 instance 失效：已修复 —@remarks已补充完整说明GLTFSkinParserentities[skeleton]无越界 guard：已修复 —GLTFSkinParser.ts:41-43已加 bounds check 并抛出有意义错误- 上轮 P1：测试断言
(animatorState as any)._state类型不匹配：已修复 —Animator.test.ts:205改为expect(animatorState).to.eq(currentAnimatorState)，类型正确
总结
所有历史跟踪问题（P1 测试 assertion 类型不匹配、P2 getCurrentAnimatorState 漏调 _resetIfControllerUpdated、P2 findAnimatorState JSDoc 缺失、P2 GLTFSkinParser 无越界 guard）本轮全部修复。
本轮代码整体干净，无新问题。整体 LGTM，可合入。
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.
Summary
从 #2983 抽离动画 + GLTF 加载器修复，独立 PR 便于 review 与合入
dev/2.0。物理 raycast/sweep 修复同步抽离至 #2998。
动画 — Shared asset + per-Animator view
把
AnimatorState一分为二：共享资产（AnimatorState，继续是 controller 持有的资源）和 per-Animator 视图（AnimatorStateInstance，新增类型）。视图按(Animator, state)对惰性创建，放在AnimatorLayerData的WeakMap里；视图上的 override 只影响所属 Animator，其它共享同一 controller 的 Animator 不受影响。公开 API
findAnimatorState(name, layerIdx?): AnimatorStateInstance | null— 返回该 Animator 上的视图（lazy create on first access）；controller mutation 后失效，需要重新调用获取新视图getCurrentAnimatorState(layerIndex): AnimatorStateInstance | null— 返回当前正在播的视图；同样在 controller mutation 后失效AnimatorStateInstance.speed/wrapMode— per-instance overrideable；未覆盖时 fall-through 到底层AnimatorState资产（编辑器调 asset 仍能传递到未声明 override 的 instance），覆盖后该 instance 独立持有自己的值AnimatorStateInstance.name/clip/clipStartTime/clipEndTime— 透传访问底层资产字段真 Bug 修复
removeState + addState同名场景下，旧设计用 string-key Map 缓存 stateData，导致用旧 state 的 curveLayerOwner 跑新 state 的播放。改为WeakMap<AnimatorState, T>，key 用对象 identity，新 state 必然 miss → 重建defaultState悬挂引用 —removeState删除当前 default state 时，defaultState仍指向被删 state。修复：置 null；字段类型显式标注AnimatorState | null_preparePlay调用新增的clearCrossFadeSlot()显式清理_updateCrossFadeState使用instance.speed— 修复原来用state.speed导致 cross-fade 阶段忽略 per-instance speed override 的 buginstance.speed = 0时playCostTime / playSpeed不再产生 NaN_getAnimatorStateInfo加 bounds checkfindAnimatorState不再读state.clip.length，state 无 clip 时不崩getCurrentAnimatorState一致性 — 加_resetIfControllerUpdated()，与 play/crossFade/findAnimatorState 对仗，避免 controller mutation 后返回 stale instanceclip.addEvent()/addComponent(Script)发生在 play() 之后时，原 lazy 设计只在入口 ensure，update 时不再 check version → 事件不触发。修复：把 ensure 调用扩展到_fireAnimationEventsAndCallScriptsskin.skeleton越界时 throw 明确错误，不再静默undefined（运行时 rootBone 消费才崩，错信息离根因很远）架构简化
UpdateFlagManager加 monotonic_version计数器；AnimatorStateData.eventsBuiltVersion快照式记录上次构建版本。clip events 变化触发_version++，Animator 在 update/play 路径上 pull-check version 决定是否重建。消除了原来 listener push 路径上的反向引用 leak 风险，以及clipChangedListener注册/注销/dispose 一整套基础设施AnimatorLayerData上animatorStateDataMap/instanceMap都用WeakMap<AnimatorState, T>；state 被 GC 时 entry 自动消失，无需手动清理ClearableObjectPool<AnimationEventHandler>— 在我们的多 stateData 共享 + 各自独立失效语义下，pool 永远单调增长，从未起到复用作用；直接new更简单且无差异stateMachine → controllerdispatch 转发链 — WeakMap 改造后变得不必要，一并删除AnimatorLayerData.completeCrossFade()/clearCrossFadeSlot()把散落在 Animator 的 slot 字段管理收回 LayerData 内部Lifecycle 不变量
addLayer/removeLayer/clearLayers触发 controller updateFlag → Animator 下次访问入口走_reset()，丢弃所有_animatorLayersData（包含 instanceMap）。Public API JSDoc 明示这条契约，用户必须重新调用findAnimatorState/getCurrentAnimatorState获取新视图speed/wrapModeoverride 不会被PlayData.reset()清掉（它们物理上在AnimatorStateInstance上，PlayData.reset只重置运行时游标）GLTF 加载器
fix(loader): resolve skin rootBone by joint LCA—GLTFSkinParser._findSkeletonRootBone重写为_findSkeletonRootBoneByLCA：没有显式skin.skeleton时，rootBone 一律通过 joints 的最近公共祖先算出来。删除之前"无 skeleton 时 fallback 抛错"分支。GLTF_ROOT wrapping 由 dev/2.0 已合入的 GLTFSceneParser: Always create container root node for consistent animation bone paths #2942/fix(loader): always create GLTF_ROOT container for consistent animation paths #2943 保证，multi-root spanning joints 自然解析为 wrapper，converged joints 解析为真实 skeleton root（如Character_Root）GLTFSceneParser同步写_sceneRoots[i]— 与_defaultSceneRoot在同 tick 可见，避免_handleSubAsset异步路径下两个 wrapper-index 字段出现"一个 set 一个 undefined"的窗口GLTFParserContextScene-before-Skin parse order — 把Scene从并行 parse 列表的尾部前置到Skin之前；显式注释 LCA 依赖 wrapper 已挂 parent chain 的不变量，并警告 Skin parser 不能 await full Scene（避免_createRenderer反向请求 Skin 造成循环依赖）GLTFSkinParserskin.skeleton 越界 guard — 显式 throwSkin skeleton index N is out of range.，不再静默undefined用户文档 + 示例
docs/{en,zh}/animation/animator.mdx— 同步新 API：findAnimatorState返回AnimatorStateInstance | null加 null guard、getCurrentAnimatorState返回AnimatorStateInstance | null加 null guard、instance.speed暂停/恢复示例e2e/case/animator-*.ts（5 个文件） — 适配findAnimatorState新返回类型，调用点加 null check；视图字段命名idleState/walkState/runState（变量名指代视图本身，而不是 def 资产）tests/src/core/Animator.test.ts，+775/-256 行） — 新增 51 个测试覆盖 per-instance override / fall-through / crossFade 保留语义 / lazy version 重建 / WeakMap 等行为抽离说明
dev/2.0的 commit（GLTF_ROOT 容器 GLTFSceneParser: Always create container root node for consistent animation bone paths #2942/fix(loader): always create GLTF_ROOT container for consistent animation paths #2943、blendShape e2e 适配）Breaking changes (2.0)
AnimatorStateInstance类型，从@galacean/engine-core公开导出Animator.findAnimatorState()返回AnimatorStateInstance | null（旧返回AnimatorState）Animator.getCurrentAnimatorState()返回AnimatorStateInstance | null（旧返回AnimatorState，且 out-of-range layer 会抛错）AnimatorStateInstance上的 override：speed/wrapMode是 per-Animator 字段，fall-through 到底层AnimatorState；视图上的 override 不影响其它 AnimatorAnimatorStateInstance在 controller 结构变化（addLayer/removeLayer/clearLayers）后失效；用户持有的旧引用变成 orphan，必须重新调用findAnimatorState/getCurrentAnimatorState获取AnimatorStateMachine.defaultState类型从AnimatorState改为AnimatorState | null；removeState删除当前 default state 时自动置 nullAnimatorStateMachine.removeState行为更精确：state 不在 array 中时整体跳过（包括_statesMap清理），避免误删同名 entryAnimator.findAnimatorState/play/crossFade对 out-of-range layerIndex 改为 safe no-op（旧实现会抛错）量化
b:types通过