Skip to content

[Design experiment, based on #2999] Split AnimatorState asset and AnimatorStateInstance view#3006

Closed
GuoLei1990 wants to merge 82 commits into
galacean:dev/2.0from
GuoLei1990:redesign-animator-state-view
Closed

[Design experiment, based on #2999] Split AnimatorState asset and AnimatorStateInstance view#3006
GuoLei1990 wants to merge 82 commits into
galacean:dev/2.0from
GuoLei1990:redesign-animator-state-view

Conversation

@GuoLei1990
Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 commented May 15, 2026

⚠️ This is a design-experiment PR, not intended for direct merge.

This PR is opened against `dev/2.0` for design discussion only. The
diff includes the full content of #2999 (luzhuang's per-state PlayData
work) because this design builds on top of that PR. Once this design
stabilizes here, the final form will be ported back to #2999 (e.g. as
a follow-up PR targeting `luzhuang:fix/animation-loader`).

Background

#2999 promotes the per-state `AnimatorStatePlayData` handle to a public
type so users can do `findAnimatorState("Walk").speed = 0.5` without
polluting the shared `AnimatorController` asset. The direction is
right, but the public surface leaves two issues open:

  1. Two public types (`AnimatorState` + `AnimatorStatePlayData`):
    users have to learn which one they get from where and what
    `playData.state.xxx` does semantically.

  2. Dual mutation paths on the same handle:

    • `playData.speed = 0.5` writes per-instance
    • `playData.state.speed = 0.5` writes the shared asset (broadcasts
      to every Animator using the same controller)

    Code review can't tell which is intended without context.

The Design

Split `AnimatorStatePlayData` into three concerns that were previously
tangled in one class:

Type Visibility Role
`AnimatorState` (new public class) public Per-Animator runtime view. Users get this from `findAnimatorState`. Surface: `speed` (per-instance), `name`/`clip`/`wrapMode`/`clipStartTime`/`clipEndTime` (readonly forwards from the shared asset).
`AnimatorStateDef` (former `AnimatorState` renamed) public (editor/asset construction) Shared asset on the controller. Returned by `AnimatorStateMachine.findStateByName` / `addState`; carried on `AnimatorStateTransition.destinationState`; consumed by `StateMachineScript` callbacks.
`AnimatorStateRuntime` (internal) not exported Engine-owned runtime bookkeeping (former `_playedTime` / `_clipTime` / `_playState` etc. underscore-prefixed fields on `AnimatorStatePlayData`).

User surface

```ts
const state = animator.findAnimatorState("Walk"); // AnimatorState | null

state.speed = 0.5; // ✅ per-instance, isolated
state.clip.length; // ✅ readonly view of shared asset
state.wrapMode; // ✅ readonly view
// state has no .def — no short-circuit to mutate the shared asset
```

To mutate the shared asset (broadcast — typical for editor / asset
construction), take the controller path. The longer path is a
deliberate visual reminder that the change is global:

```ts
animator.animatorController
.layers[0].stateMachine
.findStateByName("Walk")
.wrapMode = WrapMode.Once;
```

Why this is the right end state

  • One public type for users (`AnimatorState`). No
    `AnimatorStatePlayData` / `AnimatorStateInstance` / `Handle`
    intermediate concept.
  • `Animator` itself is already a per-component instance, so the
    object returned from `animator.xxx` is naturally per-Animator —
    no `Instance` suffix needed on the function or the type.
  • Single mutation path on the view: `state.speed = ...` is the
    only write entry, and it is unambiguously per-instance.
  • Asset mutation deliberately requires the long controller path,
    which is itself the visual warning that the change is broadcast.
  • Industry-aligned: same shape as Unreal's
    `UAnimBlueprint` (shared asset) ↔ `UAnimInstance` (per-component
    view); same lazy-create pattern as Galacean's own
    `Renderer.getInstanceMaterial`.

Diff Composition

Three commits on top of #2999:

  1. `fe78f5ecb` — Split AnimatorState into Def + view; introduce internal Runtime; drop `AnimatorStatePlayData`.
  2. `e94f0366d` — Adapt tests / e2e / docs to the new view API.
  3. `47fc4ba96` — Demote `AnimatorState.def` to `@internal _def` (the controller path is the only public way to reach the shared asset).

(The 51+ commits below those are #2999's own history; this PR only adds
those three on top.)

Status

  • ✅ `npm run b:types` passes for all 13 packages
  • ✅ `npm run b:module` builds clean
  • ✅ Tests / e2e / docs type-check
  • ⚠️ `tests/src/core/Animator.test.ts` reached 47/53 passing in
    one full run before local vitest+playwright went into a navigation
    timeout loop (test-infra issue, not the code). The remaining 6 were
    all stale `_playedTime` references already fixed in commit 2.

Outcome

If the design here lands as good, the final form will be ported back
to #2999 (`luzhuang:fix/animation-loader`) — either by cherry-pick or
by an explicit follow-up PR there, depending on what works best for
the author.

Summary by CodeRabbit

  • New Features

    • Per-Animator state instances allowing animator-specific speed overrides.
  • Bug Fixes

    • State query APIs now return null when missing; callers guard/throw accordingly.
    • More robust playback/crossfade handling, safer reverse/resume timing, and listener cleanup to prevent leaks.
  • Documentation

    • Clarified pause/resume and per-instance vs shared state guidance with guarded examples.
  • Tests

    • Expanded e2e and unit coverage for animator runtime, state-machine, skin/root handling, and edge cases.

Review Change Stack

luzhuang added 30 commits May 12, 2026 20:08
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.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
packages/core/src/animation/Animator.ts (1)

214-215: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Refresh controller-updated caches before returning the current state.

getCurrentAnimatorState() still bypasses _resetIfControllerUpdated(), so it can return a stale per-layer instance immediately after controller mutations.

🤖 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 214 - 215,
getCurrentAnimatorState currently returns the per-layer instance without
refreshing controller-updated caches; call this._resetIfControllerUpdated() at
the start of getCurrentAnimatorState (before reading this._animatorLayersData)
so any controller mutations update internal caches first, then proceed to return
this._animatorLayersData[layerIndex]?.srcRuntime?.instance ?? null while
preserving the existing bounds/null behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/src/animation/Animator.ts`:
- Around line 831-838: The cross-fade progress currently uses
destRuntime.playedTime so if dstPlaySpeed === 0 the fade never completes;
instead introduce and advance an independent transition elapsed time (e.g.,
elapsedCrossTime) using the computed actualCostTime (or deltaTime) and derive
crossWeight = elapsedCrossTime / transitionDuration (clamped to 1.0), replacing
the use of Math.abs(destRuntime.playedTime) in the crossWeight calculation;
apply the same change to both places mentioned (the block around
actualCostTime/srcRuntime.update/destRuntime.update and the similar logic at
lines ~958-963) so fades complete even when destination playback is paused.

---

Duplicate comments:
In `@packages/core/src/animation/Animator.ts`:
- Around line 214-215: getCurrentAnimatorState currently returns the per-layer
instance without refreshing controller-updated caches; call
this._resetIfControllerUpdated() at the start of getCurrentAnimatorState (before
reading this._animatorLayersData) so any controller mutations update internal
caches first, then proceed to return
this._animatorLayersData[layerIndex]?.srcRuntime?.instance ?? null while
preserving the existing bounds/null behavior.
🪄 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: 073aaf9b-53cb-4ab8-9e39-a7bcd735620d

📥 Commits

Reviewing files that changed from the base of the PR and between 47fc4ba and a4a6974.

📒 Files selected for processing (11)
  • docs/en/animation/animator.mdx
  • docs/zh/animation/animator.mdx
  • e2e/case/animator-stateMachine.ts
  • e2e/case/animator-stateMachineScript.ts
  • packages/core/src/animation/Animator.ts
  • packages/core/src/animation/AnimatorStateInstance.ts
  • packages/core/src/animation/index.ts
  • packages/core/src/animation/internal/AnimatorLayerData.ts
  • packages/core/src/animation/internal/AnimatorStateData.ts
  • packages/core/src/animation/internal/AnimatorStateRuntime.ts
  • tests/src/core/Animator.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • packages/core/src/animation/internal/AnimatorStateData.ts
  • e2e/case/animator-stateMachine.ts
  • docs/en/animation/animator.mdx
  • docs/zh/animation/animator.mdx
  • tests/src/core/Animator.test.ts

Comment thread packages/core/src/animation/Animator.ts
GuoLei1990

This comment was marked as outdated.

…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.
GuoLei1990

This comment was marked as outdated.

- 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.
GuoLei1990

This comment was marked as outdated.

…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.
GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

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".
GuoLei1990

This comment was marked as outdated.

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.
The field is internal (class is @internal, field is @internal, _ prefix);
the prose comment above it sat as a second adjacent JSDoc block, which
doesn't get attached to the field by TS tooling and duplicated info that
is now obvious from the single call-site in _ensureEventHandlers.
…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.
Copy link
Copy Markdown
Member Author

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已关闭问题清单

问题 关闭原因
docs wrapMode 显示为可写 已修复
test 混淆 view vs def 断言 (._def) 已修复
srcRuntime.state 在 23 处引用为 undefined 已修复

总结

本轮引入了完整、成熟的三层架构(AnimatorStateInstance 公开视图 / AnimatorState 共享资产 / AnimatorStatePlayData 内部运行时),新增了大量高质量测试(clone 隔离、crossFade 别名守卫、self-transition no-op、out-of-range 边界、事件处理器懒刷新、GLTF skin LCA),架构方向和整体代码质量均已显著提升。

但以下问题连续五轮报告,仍未修复:2 个 P0(测试必然 fail)、2 个 P1(其中 1 个是五轮前标为"已修复"后被回归的生产 bug)。


问题

[P0] tests/src/core/Animator.test.ts:1365srcPlayData.speed 属性不存在(五轮持续未修复)

AnimatorStatePlayData 只有 instance 属性,没有 speed 属性。"per-instance speed survives crossFade out and back" 测试末尾:

expect(srcPlayData.speed).to.eq(0.5);  // 永远 undefined,断言必然 fail

修复:

expect(srcPlayData.instance.speed).to.eq(0.5);

[P0] tests/src/core/Animator.test.ts:1661, 1752destPlayData?.state.name 属性不存在(五轮持续未修复)

AnimatorStatePlayData 只有 instance 属性,没有 state 属性。?.stateundefinedexpect(undefined).to.eq("Run") 必然 fail:

// 行 1661 — "play during crossFade clears stale destPlayData"
expect(layerData.destPlayData?.state.name).to.eq("Run");

// 行 1752 — "no-exit transition out of speed=0 source..."
expect(layerData.destPlayData?.state.name).to.eq("Walk");

修复:

expect(layerData.destPlayData?.instance.name).to.eq("Run");
expect(layerData.destPlayData?.instance.name).to.eq("Walk");

[P1] Animator.ts:207getCurrentAnimatorState 仍然缺少 _resetIfControllerUpdated(回归)

之前某轮标记为"已修复",但当前代码里没有:

getCurrentAnimatorState(layerIndex: number): AnimatorStateInstance | null {
  // 缺少 this._resetIfControllerUpdated();
  return this._animatorLayersData[layerIndex]?.srcPlayData?.instance ?? null;
}

findAnimatorState(行 219)已调用 _resetIfControllerUpdated,两者语义对称。测试 "findAnimatorState resets stale layer data after controller mutation" 也验证了 findAnimatorState 在 controller 变更后 reset 并返回新 handle,但 getCurrentAnimatorState 相同路径没有覆盖。

修复:

getCurrentAnimatorState(layerIndex: number): AnimatorStateInstance | null {
+   this._resetIfControllerUpdated();
  return this._animatorLayersData[layerIndex]?.srcPlayData?.instance ?? null;
}

同一测试文件行 205 区域的断言 (animatorState as any)._state vs currentAnimatorState 类型不匹配:_stateAnimatorState(共享资产),currentAnimatorStateAnimatorStateInstance | null,永远不等。正确写法:

expect(animatorState).to.eq(currentAnimatorState);
// 行 209:
expect(animatorState).not.to.eq(currentAnimatorState);
expect(animatorState.name).to.eq(expectedStateName);

[P1] Animator.ts:799, 924 — dest state speed=0 时 cross-fade 永远无法完成(五轮持续未修复)

_updateCrossFadeState(行 799)和 _updateFixedCrossFadeState(行 924):

let crossWeight = Math.abs(destPlayData.playedTime) / transitionDuration;

dstPlaySpeed === 0 时,destPlayData.update(0) 使 playedTime 永不增加,crossWeight 永远为 0,过渡卡死。actualCostTime 已正确计算出挂钟时间(dstPlaySpeed === 0 ? deltaTime : ...),但从未用来驱动 crossWeight

新测试 "transition out of a state with per-instance speed 0..." 覆盖的是 src speed=0 的路径,不是 dest speed=0 的路径,且断言只检查 playedTime > 0,没有验证 crossWeight 能否达到 1.0(过渡是否能完成)。

根本修复:在 AnimatorLayerData 引入独立的 transitionElapsed 字段,每帧累加 actualCostTime,替换 destPlayData.playedTime 驱动 crossWeight

// AnimatorLayerData.ts
transitionElapsed: number = 0;

// completeCrossFade / clearCrossFadeSlot 中将其清零:
completeCrossFade(): void {
  this.transitionElapsed = 0;
  // ...
}
clearCrossFadeSlot(): void {
  this.transitionElapsed = 0;
  // ...
}

// Animator.ts — _updateCrossFadeState & _updateFixedCrossFadeState
layerData.transitionElapsed += actualCostTime;
let crossWeight = Math.abs(layerData.transitionElapsed) / transitionDuration;

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.
@GuoLei1990
Copy link
Copy Markdown
Member Author

已将本 PR 的所有 commits 直接 push 到 PR #2999 的分支 (luzhuang/engine:fix/animation-loader),保留 @luzhuang 作为 PR #2999 的原作者署名。本 PR 工作完整合并到 #2999,关闭。

@GuoLei1990 GuoLei1990 closed this May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants