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 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 |
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3024 +/- ##
===========================================
+ Coverage 77.48% 79.42% +1.94%
===========================================
Files 914 903 -11
Lines 101783 100713 -1070
Branches 10430 11282 +852
===========================================
+ Hits 78862 79993 +1131
+ Misses 22738 20536 -2202
- Partials 183 184 +1
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
AnimatorStatePlayData set speed / set wrapMode had zero production callers; tests now write through the public AnimatorStateInstance via playData.instance directly.
clip and wrapMode locals in update() and clipLength in _correctTime() were each read once, so hoisting bought no getter dedup; _correctTime additionally paid the clip.length getters even when the guard missed.
Expression-body arrow for _onStateMachineChanged, hoist layer local in _setEngine, and use the destructured alias in clearLayers.
The _crossFade entry no-op guard was a strict subset of the guard inside _prepareCrossFadeByTransition, which also covers the automatic transition path (_applyTransition). Removing it leaves a single authoritative guard with one predicate to maintain; behavior is unchanged (both no-op regression tests still pass).
The layer owner pool block visibly mirrors the owner pool block above it; the comment only restated that structure.
GuoLei1990
left a comment
There was a problem hiding this comment.
总结(复审 03f9f07f2,删除我历轮 [P2] 的冗余 crossFade entry guard,行为等价,--approve 重新在当前 tip 建立通过)
自上轮 --approve(acf7648cf,2026-07-06T09:26)以来线性推进一个 commit(compare 确认 acf7648cf 是 tip 祖先、ahead_by=1/behind_by=0、无 force-push,03f9f07f2 的 parent 就是 acf7648cf):03f9f07f2 refactor(animation): drop redundant crossFade entry guard。仅动 Animator.ts(+0/-5)。这正是作者采纳我历轮(06-16 12:06 / 12:17)连提的 [P2] ——_crossFade 入口 active-state 守卫与 _prepareCrossFadeByTransition 深守卫重复,本轮把它删掉,逐路径核对为行为等价,无 P0/P1/P2/P3。CI 无失败项(build×3 / codecov / e2e 3/4 / lint / labeler 已绿,e2e 1/2/4 pending,与历轮一致的绿色基线)。新 commit 使上轮 approval 被 GitHub 判为 stale,本轮 --approve 重新建立通过。
逐路径核对(✅ 删除等价 + 单一权威守卫收口)
删掉 _crossFade(Animator.ts:383-388)里的两行:
const { srcPlayData, destPlayData } = animatorLayerData;
if ((!destPlayData && srcPlayData?.state === state) || destPlayData?.state === state) {
return;
}三点核对确认与 _prepareCrossFadeByTransition 的深守卫叠加后可观察行为完全等价:
- 深守卫是被删入口守卫的严格超集(commit message 论断属实):深守卫(
Animator.ts:1451-1453)为srcPlayData?.state === crossState || destPlayData?.state === crossState,无条件覆盖 src+dest;被删入口守卫是它的收窄子集(src 分支还多带!destPlayData条件,是我上轮核过的 narrow 版)。删掉入口守卫只会让更多 no-op 情形落到深守卫,而深守卫对它们的处理逐格相同。 - 深守卫落点在所有 slot 写之前:
return false发生在写destPlayData(1458)、layerState(1461+)、crossFadeTransition(1483)之前 → 仍 no-op,无 slot 污染。回到_crossFade后if (_prepareCrossFadeByTransition(...))为 false →_playFrameCount不更新,与旧入口守卫return的可观察结果逐字节相同。 - 无 scratch 泄漏、无孤儿引用:no-op 路径上
manuallyTransition的 4 个标量赋值(386-391)写了但深守卫 return 前从不被读(crossFadeTransition仅成功路径 1483 才指向它),下次 crossFade 先覆盖再用;被删的srcPlayData/destPlayData解构在_crossFade其余部分零引用(仅这条守卫用),删除无遗留。
两个 no-op 回归测试仍由深守卫守住(反向证伪成立):Animator.test.ts:286 crossFade to the active state is a no-op(play("Walk")→crossFade("Walk"),深守卫命中 srcPlayData?.state===crossState → 断言 destPlayData/crossFadeTransition===null)+ :302 crossFade to the active destination is a no-op(crossFade("Run")×2,深守卫命中 destPlayData?.state===crossState → 断言 dest/transition/playedTime 不变)。二者驱动公开 crossFade() API、断言公开可观测态,删入口守卫后仍全绿;若移除深守卫则两测均 fail(重建 destPlayData)→ 是守深守卫的真回归测试,非守被删入口守卫的恒过桩。
Git 历史透镜(无矛盾)
被删守卫的来源 commit 是 9483380c perf(animation): tighten play hot paths(引入)+ 39bf09df3 perf(animation): narrow crossfade no-op guard(收窄)——均为 perf: 非 fix:,即这条守卫从来是我历轮判为「与深守卫重复」的 perf 微优化,不是修正确性 bug 的 fix。本 commit 删它是采纳我的 [P2],非静默还原一个 fix:,无历史矛盾。
结论
历史两个 P1 + 全部 P2/P3 均已闭环(fireEvents 跨 wrap 游标、stateMachine swap footgun、缓存失效、WeakMap→Map identity、test-only setter 删除、hot-path 局部量内联/下移、transition clip bounds defer、controller tidy、curve owner alias、state invalidation 重命名等),本轮删除的正是我历轮连提的冗余 crossFade entry guard——收口成 _prepareCrossFadeByTransition 单一权威守卫、行为逐字节等价、两 no-op 回归测试仍守深守卫、CI 无失败项。LGTM,approve。
GuoLei1990
left a comment
There was a problem hiding this comment.
总结(复审 a1eed4f79,删除一处 structure-restating 注释,docs-only 零产物逻辑,--approve 重新在当前 tip 建立通过)
自上轮 --approve(03f9f07f2,2026-07-06T09:36)以来线性推进一个 commit(compare 确认 03f9f07f2 是 tip 祖先、ahead_by=1/behind_by=0、无 force-push,a1eed4f79 的 parent 就是 03f9f07f2):a1eed4f79 refactor(animation): drop code-restating comment。仅动 Animator.ts(+0/-1),纯注释删除、零产物逻辑,无 P0/P1/P2/P3,CI 全绿(build×3 / codecov+patch+project / e2e×4 / lint / labeler)。新 commit 使上轮 approval 被 GitHub 判为 stale,本轮 --approve 重新建立通过。
逐处核对(✅ 删除合理 + 保留了真正解释「为什么」的注释)
删掉 _saveAnimatorStateData(Animator.ts:478)里 layerCurveOwnerPool.get(component) 块上方的一行:
// Keep layer owner lookup on the same Component identity path.
let layerPropertyOwners = layerCurveOwnerPool.get(component);三点核对确认这是合规的注释卫生,非误删有价值信息:
- commit body 论断属实——该注释只在复述结构:layer-owner-pool 块(478-483,
get(component)→!x时Object.create(null)+set(component, x)→ 按property查表)与紧邻其上的 owner-pool 块(465-470,同一套curveOwnerPool.get(component)→ 惰性建 → 按property查表)结构逐字对称。被删注释「Keep layer owner lookup on the same Component identity path」只是重述「第二块也按component(Component identity)索引」——而代码layerCurveOwnerPool.get(component)本身已自证这点,注释零增量。 - 真正解释「为什么」的注释被正确保留:上方 owner-pool 块的
// Key owner lookup by Component identity instead of instanceId.(466)未被触碰——它解释的是 identity-vs-instanceId 这个 WeakMap 重构引入的非平凡设计决策(这层「为什么」不能从代码字面复原),故保留正确。本 commit 精准区分了「复述结构的冗余注释(删)」和「解释决策的 why 注释(留)」。 - 无孤儿引用、无违规:删注释不改任何控制流/数据流;保留的 466 行是单行
//无句尾句号、合规,且是本 commit 未触碰的既有行,不在范围。删除注释无法引入注释违规。
这恰好收敛了我历轮记录的一个 P3:Animator.ts:483(旧行号)的 // Keep layer owner lookup on the same Component identity path. 曾在 06-17 被我记为 change-narrating/句尾句号 family 的非阻塞项——本 commit 直接删掉它,比「去句号」更彻底的合规处理,非静默回退(原注释非 fix: 引入、无历史矛盾)。
结论
历史两个 P1 + 全部 P2/P3 均已闭环(fireEvents 跨 wrap 游标、stateMachine swap footgun、缓存失效、WeakMap→Map identity、test-only setter 删除、hot-path 局部量内联/下移、transition clip bounds defer、controller tidy、curve owner alias、state invalidation 重命名、冗余 crossFade entry guard 删除等),本轮删除的正是一处只复述结构的冗余注释——保留了解释 identity 决策的 why 注释、docs-only 零产物逻辑、CI 全绿。LGTM,approve。
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.