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 游标失同步这条静默丢事件回归）全部闭环，建议合并。
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.
总结（ultracode 深度对抗复审 + APPROVE 解除门控）
对当前 HEAD f2d788bd0 跑了一轮多 agent 深度对抗审查（6 维度 finder → 每 finding 多 lens 默认证伪验证 → 完整性 critic），相对 merge-base de75496876 真实 diff（10 文件）。零 surviving finding，全部维度核为正确。历轮 fireEvents 跨 wrap 游标 P2 已在 a92f56bb3 闭环、f2d788bd0 又收口了相关 footgun。本次 --approve 解除我早前在 08ec9a1eb 留下的 CHANGES_REQUESTED 门控（--comment 清不掉，须 approve）。
本轮逐维度核对结论（全 ✅）
- fireEvents 游标 / canWrap：gate 已下沉到
_fireSubAnimationEvents/_fireBackwardSubAnimationEvents最内层 handler 派发（1543/1570），游标推进（1548/1575）在 gate 外、_fireAnimationEvents每帧无条件跑、wrap reset（1507/1517）始终执行。手 trace muted-across-wrap 三帧 + 前/后向 Loop wrap + Once 非重放，无双发/无漏发/无失同步。 - stateMachine field→getter/setter（
f2d788bd0）：setter 四步（self-assign 提前 return / detach 旧 SM / attach 新 SM / 条件 engine 绑定 / 立即 dispatch）正确有序；_setChangeCallback为幂等赋值无重复注册；removeLayer/clearLayers 均先 detach；GLTF loader 真实流程（addLayer 后换 SM）核过。失效正确且完整。 - 缓存失效 + Map：
_reset遍历Map.values()与旧_curveOwnerPool覆盖等价、revert 幂等；Map 为 stateData 单一真相源；removeState 经_onChanged→controller dispatch→_reset整体丢弃；addState 非对称正确（新对象 identity 必 miss）；无 stale data 被任何播放路径消费（所有入口先_resetIfControllerUpdated）。 - WeakMap 生命周期：
_curveOwnerPool/_tempCurveOwner只 get/set 不遍历（WeakMap 安全）；animatorStateDataMap改 plain Map 正因_reset要遍历它；_onDestroy→_resetrevert 默认姿态，Entity.destroy 先销毁自身组件后销毁子节点，父 Animator revert 子 transform 落在仍有效目标上、无 use-after-free。 - crossFade / playData：
_getDuration内联、分支扁平化（Math.abs(time)>=duration⟺time>=duration||time<=-duration，duration≥0）、completeCrossFade/clearCrossFadeSlot 内联、speed/wrapMode/state 代理 getter——逐项 bit-identical；destPlayData 完成即置 null，无 stale 槽位查表窗口。 - API 表面 + 测试：
sampleAnimation公开提升 +fireEvents新字段文档合规；所有 headline fix 都配能反向证伪的回归测试（revert 即红）；1473 行 churn 为机械重命名 + 删冗余旧测试，非锁死坏抽象。 - 完整性 critic：额外核了共享 controller 双 Animator 接线幂等、clearLayers dispatch 保留、新 import 无循环、fireEvents 不 gate StateMachineScript 生命周期（正确高度）——跨文件三方接线 coherent。
唯一观察（不作为问题）
AnimatorControllerLayer.stateMachine 从公开字段改 getter/setter 后，对其赋 null 会立即抛 TypeError（旧字段静默存 null）。判定非缺陷、不报：stateMachine: AnimatorStateMachine 是带类型的 setter，传 null 是 TS 类型违规/用户错误；按 runtime-trust 约定引擎不做防御性校验（编辑器职责）；全仓唯一外部写点 GLTFAnimatorControllerParser 永远传非空实例；fail-fast 优于旧的静默存 null。
结论
两个 P1 + 多个 P2（含我连提 4 轮的 fireEvents 游标）全部闭环，本轮深审无新问题。LGTM，approve。
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.