fix(animation): harden animator state instance data#3024
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe animation runtime is refactored to access playback state through ChangesAnimator State/Play-Data Boundary Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/src/core/AnimatorHang.test.ts (1)
10-10: ⚡ Quick winImprove test description to clarify regression being tested.
The test title "Canvas 1024 test" does not clearly describe what hang or regression is being prevented. The PR description mentions "Adds an Animator hang regression test" but the test name is vague.
📝 Suggested improvement
-describe("Canvas 1024 test", function () { +describe("Animator GLTF load hang regression", function () {Or add a comment explaining what hang this prevents:
+// Regression test for animator hang when loading GLTF with 1024x1024 canvas describe("Canvas 1024 test", function () {🤖 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/AnimatorHang.test.ts` at line 10, Update the test title string in the describe(...) call so it clearly documents the regression being prevented (e.g., "Animator hang regression: ensure canvas draw loop does not hang with 1024px width/height") or add a short comment immediately above the describe explaining which hang/bug this test prevents; target the describe invocation in AnimatorHang.test.ts to make the purpose explicit for future readers.
🤖 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 225-241: The method findAnimatorState currently returns the live
layerData.srcPlayData as a fallback even when neither srcPlayData.state nor
destPlayData.state match the requested state, which returns play-data for the
wrong state and allows erroneous reads/mutations; change the fallback to return
null (keeping the AnimatorStatePlayData nullable return) so only an exact match
from _getAnimatorStateInfo / _animatorLayersData (checking srcPlayData.state and
destPlayData.state) is returned, or if you need to support per-state mutable
play data create and consult a dedicated per-state cache keyed by state name
instead of reusing srcPlayData/destPlayData.
- Around line 212-218: The public method getCurrentAnimatorState currently can
return undefined when the layer index is out of range or the layer has never
played (it reads this._animatorLayersData[layerIndex]?.srcPlayData?.state) but
its signature promises AnimatorState; update the contract by either changing the
return type to AnimatorState | null or ensuring the method normalizes
falsy/undefined results to null before returning (e.g., return null when
srcPlayData or state is missing) so callers never receive undefined.
In `@packages/core/src/animation/AnimatorStateMachine.ts`:
- Around line 18-23: Make defaultState nullable again and ensure removeState()
clears or reparents it: change the type of Animator.defaultState back to
AnimatorState | null, update any initialization logic so it starts as null, and
in removeState(state) check if state === this.defaultState and either set
this.defaultState = null or reassign it to a valid remaining state; also review
Animator._checkAnyAndEntryState() (and related logic around default-state usage
at the entry path) to handle a null defaultState safely so removed states cannot
remain live targets.
In `@packages/core/src/animation/internal/AnimationEventHandler.ts`:
- Around line 6-10: The dispose() method must clear pooled state: reset the
handlers array and release the event reference to avoid leaking callbacks and
stale state. Update AnimationEventHandler.dispose() to clear handlers (e.g.
this.handlers.length = 0 or replace with a new empty array) and nullify the
event (e.g. set this.event = undefined/null) and if necessary adjust the event
property type on the class (AnimationEvent -> AnimationEvent | undefined) so
clearing the reference compiles.
In `@packages/core/src/animation/internal/AnimatorStatePlayData.ts`:
- Around line 15-18: AnimatorStatePlayData currently exposes runtime-only
internals (stateData) and mutator methods (reset, updateOrientation, update);
refactor by introducing a public read-only interface (e.g.,
AnimatorStatePlayInfo or a readonly version of AnimatorStatePlayData) that only
exposes immutable fields needed by consumers (readonly state: AnimatorState and
any other read-only view fields) and remove AnimatorStateData and lifecycle
methods from that public shape; keep the current class as an internal
implementation (rename or mark non-exported) that still contains stateData and
the mutators (reset(), updateOrientation(), update()) and update
animation/index.ts to export the new readonly interface instead of the mutable
class so consumers cannot access or call the internal mutators.
In `@tests/src/core/AnimatorHang.test.ts`:
- Around line 10-20: The describe callback is async which causes a race because
Vitest doesn't await it; remove the async from the describe wrapper and move the
asynchronous setup into a beforeAll (or inside the it) so the test waits for
initialization. Specifically, perform WebGLEngine.create(...) and
resourceManager.load<GLTFResource>(...) inside a beforeAll that assigns engine,
scene, rootEntity, defaultSceneRoot, and animator (or await those calls inside
the it) and keep the it("loaded", ...) synchronous/asserting only after animator
is set. Ensure references to WebGLEngine.create, engine, resourceManager.load,
defaultSceneRoot.getComponent(Animator), and animator are updated to use the
variables populated by beforeAll or the test body.
---
Nitpick comments:
In `@tests/src/core/AnimatorHang.test.ts`:
- Line 10: Update the test title string in the describe(...) call so it clearly
documents the regression being prevented (e.g., "Animator hang regression:
ensure canvas draw loop does not hang with 1024px width/height") or add a short
comment immediately above the describe explaining which hang/bug this test
prevents; target the describe invocation in AnimatorHang.test.ts to make the
purpose explicit for future readers.
🪄 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: 2a682d95-69cf-47c9-adae-c9b9908285e9
📒 Files selected for processing (12)
packages/core/src/animation/AnimationClip.tspackages/core/src/animation/AnimationClipCurveBinding.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/AnimationEventHandler.tspackages/core/src/animation/internal/AnimatorLayerData.tspackages/core/src/animation/internal/AnimatorStateData.tspackages/core/src/animation/internal/AnimatorStatePlayData.tstests/src/core/Animator.test.tstests/src/core/AnimatorHang.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/animation/AnimatorStateInstance.ts
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.
luzhuang
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3024 +/- ##
===========================================
+ Coverage 77.48% 79.20% +1.72%
===========================================
Files 914 914
Lines 101783 101744 -39
Branches 10430 11230 +800
===========================================
+ Hits 78862 80587 +1725
+ Misses 22738 20970 -1768
- Partials 183 187 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Ready for re-review. The earlier review points are addressed on the latest head:
Current head |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/src/core/Animator.test.ts (2)
1579-1623: 💤 Low valueMinor assertion style inconsistency.
Line 1616 uses
.to.be.nullwhile the rest of the file consistently uses.to.eq(null). While both work with vitest's chai compatibility, prefer the consistent style for maintainability.♻️ Proposed fix for consistency
- expect(layerData.destPlayData).to.be.null; + expect(layerData.destPlayData).to.eq(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 `@tests/src/core/Animator.test.ts` around lines 1579 - 1623, Change the assertion on line 1616 in the noExitTime transition scan test from `.to.be.null` to `.to.eq(null)` to match the assertion style consistently used throughout the rest of the test file. The assertion `expect(layerData.destPlayData).to.be.null` should be updated to use `.to.eq(null)` for consistency with the file's convention.
247-296: 💤 Low valueGood coverage for state lookup edge cases.
These three new tests provide solid coverage for:
- Stable per-state instances for non-playing states
- Instance stability across crossFade slot reuse
- Graceful handling of invalid layer indices
However, line 283 accesses the private
_stateproperty:expect(animator.findAnimatorState("Walk", 0)._state).to.eq(walkState);While acceptable for verification in tests, this breaks the public API boundary. Consider whether this internal verification is necessary or if public properties suffice.
🤖 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 247 - 296, The test "findAnimatorState remains stable after crossFade play slots are reused" accesses the private _state property on the result of animator.findAnimatorState(), which breaks the public API boundary. Remove the expectation that checks animator.findAnimatorState("Walk", 0)._state or replace it with a verification using only public properties. The other assertions already verify that the instance remains stable across the crossFade operation, so this private property check may be unnecessary.
🤖 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.
Nitpick comments:
In `@tests/src/core/Animator.test.ts`:
- Around line 1579-1623: Change the assertion on line 1616 in the noExitTime
transition scan test from `.to.be.null` to `.to.eq(null)` to match the assertion
style consistently used throughout the rest of the test file. The assertion
`expect(layerData.destPlayData).to.be.null` should be updated to use
`.to.eq(null)` for consistency with the file's convention.
- Around line 247-296: The test "findAnimatorState remains stable after
crossFade play slots are reused" accesses the private _state property on the
result of animator.findAnimatorState(), which breaks the public API boundary.
Remove the expectation that checks animator.findAnimatorState("Walk", 0)._state
or replace it with a verification using only public properties. The other
assertions already verify that the instance remains stable across the crossFade
operation, so this private property check may be unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67ff5265-2b68-4bcc-9962-4814431c8219
📒 Files selected for processing (6)
notes/animation/2026-06-15-animator-state-instance-boundary.mdpackages/core/src/animation/Animator.tspackages/core/src/animation/internal/AnimatorLayerData.tspackages/core/src/animation/internal/AnimatorStatePlayData.tstests/src/core/Animator.test.tstests/src/core/AnimatorHang.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/core/AnimatorHang.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
审查 @ 08ec9a1eb（本轮 request-changes，HEAD 未变）— fix(animation): harden animator state instance data
总结
逐项复核了整个 refactor。_reset/_onDestroy 的 revert 等价、canWrap 修 Once 重放、_curveOwnerPool 改 WeakMap<Component>、animatorStateDataMap 改 Map（identity 索引、非 GC fix）、state-removal 失效机制（_setChangeCallback 链路）、crossFade 自/活跃 dest no-op 守卫、PlayData getter 委托——这些都核为正确，方向是对的。唯一阻塞项是我此前指出、本 HEAD 仍未修的 fireEvents 跨 loop wrap 游标失同步 P2，本轮再次真机复现确认在位。
问题
[P2] Animator.ts:1617-1619 — fireEvents=false 跨 loop wrap 后再开，currentEventIndex 卡高位 → 静默丢下一轮事件
this.fireEvents &&
eventHandlers.length &&
this._fireAnimationEvents(playData, eventHandlers, lastClipTime, deltaTime);currentEventIndex 是播放游标。它只在 _fireSubAnimationEvents/_fireBackwardSubAnimationEvents 内推进（1546/1571），并且 loop wrap 的游标重置（currentEventIndex = 0 / length-1，1507/1517）只住在 _fireAnimationEvents 内。这三处全在 this.fireEvents 门控之下。所以当 fireEvents=false 跨越一次 loop wrap：update() 的 clipTime 取模 wrap 照常发生（与门控无关、每帧都跑），但 wrap 的游标重置被连同派发一起短路掉。fireEvents 重开后，游标卡在 wrap 前的高位 → _fireSubAnimationEvents 从该 stale 高位起遍历 → 新一轮 loop 里低 time 的早期事件立刻 time > curClipTime break → 永久丢弃，直到下次 wrap 才自愈。
base 无此门控故无此 bug，是 fireEvents 新开关引入的回归。
复现（loop clip，事件 e0@0.1、e1@0.75）：
- F1
fireEvents=true，clipTime 0→0.8：e0、e1 fire，游标→1。 - F2
fireEvents=false，wrap 0.8→0.05：_fireAnimationEvents整个被短路，游标停在 1（wrap-reset 在门控内被跳过）。 - F3
fireEvents=true，0.05→0.5（窗口含 e0@0.1）：从游标=1 起，见 e1@0.75 > 0.5 → break。e0 从未被重新检查 → e0 共 fire 1 次（应 2 次）。
这是修复落点错的典型：把「是否派发」的开关放在「既派发又推进/重置游标」的函数外层，会连游标记账一起跳过。正确落点是把门控下沉到最内层「真正调 handler」那一处，让游标记账无条件执行：
- 1617-1619 去掉
this.fireEvents &&（保留eventHandlers.length &&）； _fireSubAnimationEvents(1543-1547) /_fireBackwardSubAnimationEvents(1567-1571) 里只把handlers[j](parameter)派发循环裹进if (this.fireEvents)，playState.currentEventIndex = …赋值始终执行。
测试缺口：新测试 fireEvents gates AnimationEvent dispatch without consuming the event 用 update(0) + update(0.1)、无 loop wrap，抓不到这个 bug；两个 loop-wrap 测试又从不切换 fireEvents。守这条回归的测试必须同时满足「fireEvents=false + 跨一次 loop wrap + 再开」，断言重开后那个低 index 事件仍 fire（卡在自愈前的精确窗口）。
其余核对（正确，不阻塞，记录结论避免重复审）
- (D)
_reset新遍历覆盖完整 —_createCurveOwner唯一调用点在_saveAnimatorStateData:479，每个 owner 同时被包进curveLayerOwner[i]（495），其stateData入animatorStateDataMap（436）。新的layerData → animatorStateDataMap → curveLayerOwner遍历覆盖到旧扁平池遍历的所有 owner，无遗漏。_curveOwnerPool仅剩 save 期去重职责。 - (E)
animatorStateDataMapWeakMap→Map — 是新_reset必须可迭代（WeakMap 不可迭代）所致，且无泄漏：removeState经_onChanged → _updateFlagManager.dispatch()置 dirty，下次_resetIfControllerUpdated整体清掉_animatorLayersData（连同 Map），removed-state entry 随整体 GC。 - (F)
_onDestroy→_reset— 不崩（Component._onDestroy不 null 化_entity，owner 持 assembler 直引用，revert 写入成功）。 - (B) Once clip 的
canWrap— 正确跳过 wrap 重置 + 二段重放；playData.reset()置currentEventIndex=0，Once 重放干净。 - (G) crossFade no-op 守卫 —
play()（_preparePlay）无守卫，replay-from-offset 仍正常重启；两处 crossFade 守卫（387 预短路 / 1456 承重）一致，自/活跃 dest no-op 是「每 state 每 layer 单实例」的刻意设计，已有测试。 - (C) PlayData getter 委托 — 与旧
playData.instance.x同级间接，V8 可内联，热点循环已 destructure，不构成问题。 sampleAnimation去@internal、JSDoc 完整、无 stale_sampleAnimation引用。
不阻塞的小项
- [P3]
_onDestroy现在会 revert 动画值 — 旧_onDestroy只减引用计数、不调_reset；现在销毁 animator 会把动画目标 snap 回默认。整子树一起销毁时无害（常见），但若 animator 经removeComponent单独移除、或动画的是存活更久的兄弟/子实体，这些属性现在会在 destroy 时重置——新副作用。大概率是期望行为，确认意图 + 补一个测试更稳。 - [P3]
AnimatorControllerLayer.stateMachine公开字段在addLayer之后重赋会丢_onChanged回调 → 对新 stateMachine 的removeState不触发失效。当前测试都在 addLayer 前重赋故未触发，pre-existing footgun，本 PR 的失效机制让它略相关。考虑加 setter 重连或文档化 ordering 要求。 - [P3] 注释句号 —
Animator.ts:471、Animator.ts:483、AnimatorStatePlayData.ts:100的单行//注释带句号，违反「单行注释不加句号」，去掉。
简化建议
refactor 本身已足够干净，无进一步精简空间。fireEvents 门控下沉是唯一需要的结构修正。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
复审 HEAD a92f56bb3（自上轮 08ec9a1eb 起 1 个新 commit：a92f56bb3 fix(animation): keep event cursor synced when events are muted）。相对 merge-base（de75496876）真实 diff 仍为 7 src + test。本轮这个 commit 正是修我历轮（03:58 / 04:30 / 06:50）唯一开放阻塞项——fireEvents 跨 loop wrap 游标失同步 P2——的修复，落点与我此前给出的处方逐字一致，并补上了我要求的回归测试。逐行核对 + 真机反向证伪确认正确。该 P2 闭环，无 P0/P1/P2，可合并。
已闭环（本轮新修，逐项核对）
- [P2→已修]
fireEvents跨 loop wrap 游标失同步（a92f56bb3）—— 修法与我历轮处方完全一致：_fireAnimationEventsAndCallScripts(1621) 去掉this.fireEvents &&，只保留eventHandlers.length && this._fireAnimationEvents(...)→_fireAnimationEvents每帧无条件跑，wrap 段游标重置（1507currentEventIndex=0/ 1517=length-1）不再被门控短路。_fireSubAnimationEvents(1543-1547) /_fireBackwardSubAnimationEvents(1570-1574) 里只把handlers[j](parameter)派发循环裹进if (this.fireEvents)，playState.currentEventIndex = …(1548/1575) 始终执行 → 游标记账无条件、只静默派发。
- 前后向对称：backward 路径同样只 gate 派发循环、
currentEventIndex = Math.max(eventIndex-1,0)与 wrap-reset 1517 均无条件跑，结构与 forward 镜像一致。 - 真机反向证伪（脱机精确复刻
_fireAnimationEvents+_fireSubAnimationEventsforward+Loop wrap 算法，clip len=1，e0@0.1/e1@0.75，跑测试三帧场景）：- FIXED：F1
{e0:1,e1:1}→ F2 muted{e0:0,e1:0}（wrap 期间游标被重置回 0）→ F3{e0:1,e1:0}（重开后 e0 重新 fire）。 - OLD（整
_fireAnimationEvents被!fireEvents短路）：F1/F2 同，F3{e0:0,e1:0}（游标卡在 wrap 前高位 → e0 永久丢弃）。 - 即 fix 前 fail（
expect(0).toBe(1)）、fix 后 pass，是真回归测试而非恒过桩。
- FIXED：F1
- 测试缺口已补：新增
keeps AnimationEvent cursor in sync when fireEvents is disabled across forward loop wrap（Animator.test.ts:673）恰好命中我此前指出的"fireEvents=false+ 跨一次 loop wrap + 再开"精确窗口，断言重开后低 index 事件 e0 仍 fire 恰一次（卡在自愈前的关键帧），与上面反向证伪逐帧一一对应。✅
已闭环 / 不在范围（逐条核对，不重提）
- 两个 P1（
findAnimatorState非播放态返错 /removeState清defaultState）+ 历史全部 P2（getCurrentAnimatorStatenullable /_getAnimatorStateInfo越界守卫 / event handler 无界增长 /AnimatorHangbeforeAll/ 双 JSDoc / 死&&守卫 /_crossFade重复守卫收窄 / 标题去 "shaderlab" / 测试去随机化）→ 历轮落定。 - 重构简化（
sampleAnimation透传合并、completeCrossFade/clearCrossFadeSlot内联、WeakMap+stateDataList双源→单Map、update()热路径内联）、canWrap修 Once 重放、WeakMap 化 curveOwner pool、state-removal 缓存失效（removeState→_onChanged→dispatch→_reset，按对象 identity 索引、addState非对称正确）→ 历轮核对正确。 - 本轮源码除
a92f56bb3三处派生外，其余 6 src 文件逐字节未变，结论沿用，不重提。
备注（P3，非阻塞，仅记录）
Animator.ts:473（// Key owner lookup by Component identity instead of instanceId.）、Animator.ts:483（// Keep layer owner lookup on the same Component identity path.）、AnimatorStatePlayData.ts:100（// Reverse playback at clipTime=0 would step into negatives; jump to clipEnd.）三处单行//注释仍带句尾句号，违反「单行注释不加句号」。上轮已提一次，纯 cosmetic、不阻塞、不再展开。_onDestroy现在会 revert 动画值（新副作用）/AnimatorControllerLayer.stateMachineaddLayer 后重赋丢_onChanged回调（pre-existing footgun）→ 上轮已记 P3，本轮无变化，不重提。
自检
- 重复检查：本轮无新 finding。我历轮唯一开放的 P2 由
a92f56bb3修复并核对，归入「已闭环」，不再以任何形式重提。其余结论落在「已修复 / 已闭环 / 源码逐字节未变」。 - 代码验证：fix 三处落点（1543/1570 派发 gate、1507/1517 wrap-reset、1621 外层 gate 移除）、前后向对称、新测试逐帧追踪，均经实读 PR head 源码 + 脱机反向证伪（FIXED F3 fire e0 / OLD F3 丢 e0）确认。真实 diff 7 文件口径、merge-base
de75496876经实读对照。
CI：lint / labeler 已绿，build×3 / codecov / e2e×4 进行中（与历轮一致的绿色基线）。本 PR 历经十余轮迭代，两个 P1 + 全部 P2（含本轮 fireEvents 跨 wrap 游标失同步这条静默丢事件回归）全部闭环，建议合并。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
复审 @ f2d788bd（新 commit 闭环上轮 P3 footgun，--comment 可合并）
自上轮 --comment（a92f56bb3，全部 P1+P2 闭环）以来推进一个 commit f2d788bd0 fix(animation): keep replaced state machines wired。这正是我上轮记录的 [P3] pre-existing footgun：AnimatorControllerLayer.stateMachine 是公开字段，在 addLayer 之后重新赋值会丢掉 _onChanged 回调 → 新 state machine 的 removeState 不再触发缓存失效。本 commit 把它修成根上正确。
修复正确（落点在输入端，funnel 收口）
stateMachine 从公开字段改成 getter/setter（AnimatorControllerLayer.ts），setter 在每次换 state machine 时完成四件事：
lastStateMachine._setChangeCallback(null)—— 摘掉旧 SM 的回调（避免旧 SM 仍持 layer 的失效信号）；value._setChangeCallback(this._onStateMachineChanged)—— 把 layer 缓存的回调挂到新 SM；this._engine && value._setEngine(this._engine)—— 补齐引擎绑定；this._onStateMachineChanged?.()—— 立即 dispatch 一次，让换 SM 这个动作本身触发各 Animator 的缓存重建。
配套把 AnimatorController 的 addLayer/removeLayer/clearLayers/_setEngine 从直接戳 layer.stateMachine._setChangeCallback(...) 改为走 layer 新增的 _setStateMachineChangeCallback(onChanged)（同时记到 layer 的 _onStateMachineChanged 字段 + 转发给当前 SM）。这把"回调归属"从 controller 直接 reach-through 到 SM，收口成了 layer 自己持有 + 在 SM swap 时转发的 funnel —— 消除了 Demeter 违规，也让 setter 能在 swap 时正确重挂。架构方向对。
回归测试守得住（反向证伪成立）
新增 replacing a layer state machine keeps state removal invalidation（Animator.test.ts:1584）从公开 API 驱动：addLayer → layer.stateMachine = newSM（换 SM）→ play("Temp") 建缓存 → removeState(oldState) + addState/换 clip → 再 play → 断言 animatorStateDataMap.has(oldState)===false 且 has(newState)===true。
- revert fix（恢复成公开字段）→ 新 SM 拿不到
_onChanged→removeState不 dispatch → 缓存仍含oldState→has(oldState)留 true → 测试 fail，是守得住换-SM 这条链的真回归测试，非戳私有的恒过桩。链路粒度正确（公开入口进、公开可观测态出，仅_animatorLayersData一处内部读用于断言缓存，可接受）。
边界核对（无 NPE / 无顺序坑）
- setter 里
lastStateMachine恒非 null（构造器初始化_stateMachine = new AnimatorStateMachine()），lastStateMachine._setChangeCallback(null)安全。 lastStateMachine === value提前 return，自赋值无副作用。_onStateMachineChanged初始为 null，未加入 controller 前换 SM 时value._setChangeCallback(null)+this._onStateMachineChanged?.()均为 no-op，正确。_setEngine改成存this._engine = engine再转发给 SM，使 setter 的引擎补绑有据可依，顺序自洽。
结论
上轮 P3 footgun 已根上闭环，无新 P0/P1/P2，--comment，建议合并。 历史全部已闭环项（fireEvents 跨 wrap 游标同步、state-removal 缓存失效、_reset/_onDestroy revert 等价、WeakMap→Map identity）不再复述。剩余非阻塞 P3（若干单行注释带句号，历轮已提）维持，不阻塞。
2 participants
Summary
AnimatorStateInstanceas the public per-Animator API and keepsAnimatorStatePlayDatainternal as the runtime playback slot.AnimationClip.sampleAnimation(entity, time)as a public curve-sampling API; direct sampling applies clip curves without dispatching AnimationEvents, with no separate internal sampling wrapper.Animator.fireEvents(defaulttrue) so AnimationEvent dispatch can be muted while event cursor bookkeeping stays in sync across loop wraps.AnimatorStateInstance, reduces repeated play-data getter access in the playback hot path, and avoids no-op crossFade transition setup.sampleAnimation,fireEvents, fireEvents loop-wrap cursor sync, forward/backward loop and Once-clip AnimationEvents, deterministic current-state lookup, and theAnimatorHangload regression.Verification
pnpm exec prettier --check packages/core/src/animation/Animator.ts packages/core/src/animation/AnimatorStateInstance.ts packages/core/src/animation/AnimatorStateMachine.ts packages/core/src/animation/index.ts packages/core/src/animation/internal/AnimationEventHandler.ts packages/core/src/animation/internal/AnimatorLayerData.ts packages/core/src/animation/internal/AnimatorStateData.ts packages/core/src/animation/internal/AnimatorStatePlayData.ts tests/src/core/Animator.test.ts tests/src/core/AnimatorHang.test.tspnpm exec prettier --check packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorStateMachine.ts tests/src/core/Animator.test.tspnpm exec prettier --check packages/core/src/animation/Animator.ts tests/src/core/Animator.test.tspnpm exec prettier --check packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorControllerLayer.ts tests/src/core/Animator.test.tsnpx eslint --fix --no-eslintrc -c .eslintrc.js packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorStateMachine.ts tests/src/core/Animator.test.tsnpx eslint --fix --no-eslintrc -c .eslintrc.js packages/core/src/animation/Animator.ts tests/src/core/Animator.test.tsnpx eslint --fix --no-eslintrc -c .eslintrc.js packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorControllerLayer.ts tests/src/core/Animator.test.tsgit diff --checkpnpm -F @galacean/engine-core run b:typespnpm run b:modulepnpm vitest tests/src/core/Animator.test.ts --run -t "keeps AnimationEvent cursor in sync"(1 passed)pnpm vitest tests/src/core/Animator.test.ts --run -t "replacing a layer state machine"(1 passed)pnpm vitest tests/src/core/Animator.test.ts tests/src/core/AnimatorHang.test.ts --run(54 passed)CI=true pnpm run coverage(111 passed,1425 passed)pnpm exec eslint packages/core/src/animation/Animator.ts --ext .ts --resolve-plugins-relative-to ./node_modulesSummary by CodeRabbit
New Features
AnimationClip.sampleAnimation(entity, time)for direct curve sampling.Animator.fireEventsto control whether animation event handlers dispatch.Bug Fixes
Tests
Animatordoes not hang.