Skip to content

fix(animation): harden animator state instance data#3024

Open
luzhuang wants to merge 28 commits into
dev/2.0from
fix/animation-shaderlab-split
Open

fix(animation): harden animator state instance data#3024
luzhuang wants to merge 28 commits into
dev/2.0from
fix/animation-shaderlab-split

Conversation

@luzhuang

@luzhuang luzhuang commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keeps AnimatorStateInstance as the public per-Animator API and keeps AnimatorStatePlayData internal as the runtime playback slot.
  • Adds AnimationClip.sampleAnimation(entity, time) as a public curve-sampling API; direct sampling applies clip curves without dispatching AnimationEvents, with no separate internal sampling wrapper.
  • Adds Animator.fireEvents (default true) so AnimationEvent dispatch can be muted while event cursor bookkeeping stays in sync across loop wraps.
  • Fixes stable per-state instance lookup, null-safe current-state APIs, invalid or missing state/layer no-ops, controller-change/state-removal invalidation, replacement-safe layer state-machine invalidation, sparse layer reset, destroy reset cleanup, and default-state removal cleanup.
  • Uses a Map-backed state-data cache plus WeakMap-backed instance and curve-owner caches with layer-local reset traversal, rebuilds animation event handlers lazily from clip/script versions, preserves per-Animator speed/wrapMode overrides through AnimatorStateInstance, reduces repeated play-data getter access in the playback hot path, and avoids no-op crossFade transition setup.
  • Adds focused coverage for non-playing state lookup, crossFade slot reuse, same-state/destination crossFade no-op, invalid layers, script/clip event rebinding after play, default state removal, state-removal cache invalidation, replacement state-machine invalidation, sampleAnimation, fireEvents, fireEvents loop-wrap cursor sync, forward/backward loop and Once-clip AnimationEvents, deterministic current-state lookup, and the AnimatorHang load 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.ts
  • pnpm exec prettier --check packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorStateMachine.ts tests/src/core/Animator.test.ts
  • pnpm exec prettier --check packages/core/src/animation/Animator.ts tests/src/core/Animator.test.ts
  • pnpm exec prettier --check packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorControllerLayer.ts tests/src/core/Animator.test.ts
  • npx eslint --fix --no-eslintrc -c .eslintrc.js packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorStateMachine.ts tests/src/core/Animator.test.ts
  • npx eslint --fix --no-eslintrc -c .eslintrc.js packages/core/src/animation/Animator.ts tests/src/core/Animator.test.ts
  • npx eslint --fix --no-eslintrc -c .eslintrc.js packages/core/src/animation/AnimatorController.ts packages/core/src/animation/AnimatorControllerLayer.ts tests/src/core/Animator.test.ts
  • git diff --check
  • pnpm -F @galacean/engine-core run b:types
  • pnpm run b:module
  • pnpm 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_modules

Summary by CodeRabbit

  • New Features

    • Added AnimationClip.sampleAnimation(entity, time) for direct curve sampling.
    • Added Animator.fireEvents to control whether animation event handlers dispatch.
  • Bug Fixes

    • Improved animation event dispatch during loop wrap-around, including forward/backward refires and Once-clips behavior.
    • Refined animator playback/cross-fade handling so timing, speed, and wrap-mode overrides behave consistently (including shared controllers).
  • Tests

    • Updated animator tests to rely on public state/play APIs.
    • Added a regression test to ensure loading an Animator does not hang.

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The animation runtime is refactored to access playback state through AnimatorStatePlayData proxy properties (state, speed, wrapMode) instead of AnimatorStateInstance internals. WeakMap caching replaces numeric instanceId-keyed records across AnimatorLayerData, AnimationClipCurveBinding, and Animator. A public sampleAnimation method is added to AnimationClip, and a fireEvents flag is added to Animator with loop-aware wrap-around event dispatch logic. Tests are updated to use public API handles throughout.

Changes

Animator State/Play-Data Boundary Refactor

Layer / File(s) Summary
Internal data structures: WeakMap caches and AnimatorStatePlayData proxies
packages/core/src/animation/internal/AnimatorStatePlayData.ts, packages/core/src/animation/internal/AnimatorLayerData.ts, packages/core/src/animation/AnimationClipCurveBinding.ts
AnimatorStatePlayData adds public proxy getters/setters for state, speed, and wrapMode. AnimatorLayerData replaces its numeric curveOwnerPool with a WeakMap<Component, ...>, adds stateDataList, and removes getOrCreateInstance/completeCrossFade/clearCrossFadeSlot. AnimationClipCurveBinding switches _tempCurveOwner from an instanceId-keyed object to a WeakMap<Entity, ...>.
Public API: AnimationClip.sampleAnimation and Animator.fireEvents
packages/core/src/animation/AnimationClip.ts, packages/core/src/animation/Animator.ts
AnimationClip exposes a new public sampleAnimation(entity, time) that wraps _sampleAnimation. Animator gains a fireEvents: boolean field, and imports WrapMode to support loop-aware event gating.
Animator lifecycle: reset, owner pool, and state-data wiring
packages/core/src/animation/Animator.ts
_reset() iterates stateDataList/curveLayerOwner to revert curve default values and recreates _curveOwnerPool as a new WeakMap. _onDestroy() now calls _reset(). _saveAnimatorStateData reads and writes the WeakMap by Component identity. New AnimatorStateData entries are pushed onto stateDataList.
Playback and cross-fade evaluation using playData.state proxies
packages/core/src/animation/Animator.ts
All update/evaluate paths—playing, cross-fading, pose cross-fading, and finished—now read state/clip from playData.state/playData.state.clip and compute speed from playData.speed. _preparePlayOwner, _applyStateTransitions, and cross-fade no-op guards compare AnimatorState identity through playData.state.
Loop-aware animation event dispatch and fireEvents gating
packages/core/src/animation/Animator.ts
_fireAnimationEvents adds WrapMode.Loop wrap-around logic, firing events in two segments when the time window crosses clip boundaries. _fireAnimationEventsAndCallScripts gates all dispatch on this.fireEvents and the presence of built handlers.
Animator test migration, new coverage, and hang regression test
tests/src/core/Animator.test.ts, tests/src/core/AnimatorHang.test.ts
Tests are rewritten to use public AnimatorState handles and playData.state.name assertions. New coverage validates per-instance speed/wrapMode isolation, controller-sharing non-leak, fireEvents gating, sampleAnimation without events, and removing a default state preventing auto-play. A new regression test verifies an Animator loads without hanging from a GLTF resource.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • galacean/engine#2999: Overlapping refactors to Animator.ts cross-fade, state/playback handling, and state-machine evaluation paths directly precede and relate to this PR's changes.

Suggested labels

Animation

Suggested reviewers

  • GuoLei1990

Poem

🐰 Hop hop, the WeakMap grows,
No more instanceId woes!
playData.state leads the way,
fireEvents gates the fray,
Loop wraps and samples — hooray! 🎞️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(animation): harden animator state instance data' directly summarizes the main architectural change: improving the robustness and reliability of animator state instance data management, which aligns with the core objective of hardening the animator state instance data API and internal mechanics across multiple files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/animation-shaderlab-split

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (1)
tests/src/core/AnimatorHang.test.ts (1)

10-10: ⚡ Quick win

Improve 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

📥 Commits

Reviewing files that changed from the base of the PR and between de75496 and abb5c3a.

📒 Files selected for processing (12)
  • packages/core/src/animation/AnimationClip.ts
  • packages/core/src/animation/AnimationClipCurveBinding.ts
  • 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.ts
💤 Files with no reviewable changes (1)
  • packages/core/src/animation/AnimatorStateInstance.ts

Comment thread packages/core/src/animation/Animator.ts
Comment thread packages/core/src/animation/Animator.ts Outdated
Comment thread packages/core/src/animation/AnimatorStateMachine.ts
Comment thread packages/core/src/animation/internal/AnimationEventHandler.ts Outdated
Comment thread packages/core/src/animation/internal/AnimatorStatePlayData.ts Outdated
Comment thread tests/src/core/AnimatorHang.test.ts Outdated
GuoLei1990

This comment was marked as outdated.

@GuoLei1990 GuoLei1990 mentioned this pull request Jun 14, 2026
3 tasks
GuoLei1990

This comment was marked as outdated.

@GuoLei1990 GuoLei1990 marked this pull request as draft June 15, 2026 02:49
@luzhuang luzhuang changed the title fix(animation): split shaderlab animation fixes fix(animation): harden animator state instance data Jun 15, 2026
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.32164% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.42%. Comparing base (de75496) to head (a1eed4f).
⚠️ Report is 18 commits behind head on dev/2.0.

Files with missing lines Patch % Lines
packages/core/src/animation/AnimatorController.ts 50.00% 6 Missing ⚠️
...ages/core/src/animation/AnimatorControllerLayer.ts 94.28% 2 Missing ⚠️
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     
Flag Coverage Δ
unittests 79.42% <95.32%> (+1.94%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@luzhuang luzhuang marked this pull request as ready for review June 16, 2026 07:20
@luzhuang

Copy link
Copy Markdown
Contributor Author

Ready for re-review.

The earlier review points are addressed on the latest head:

  • AnimatorStateInstance remains the public per-Animator facade; AnimatorStatePlayData stays internal.
  • getCurrentAnimatorState() / findAnimatorState() keep nullable, stable instance semantics and no longer return unrelated play data.
  • Removed default states are cleared from defaultState.
  • The AnimatorHang suite now uses beforeAll instead of async describe setup.
  • Event handlers rebuild lazily from clip/script versions; no pooled handler state remains.
  • Added focused coverage for non-playing state lookup, crossFade slot reuse, invalid layers, script/clip event rebinding after play, default state removal, and forward/backward loop events.

Current head bb323cac20a1d22a7e56f6a12fd45ebcbdfec92d has green CI. I also checked a local merge onto current origin/dev/2.0: b:module, focused Animator vitest (49 passed), and core b:types pass after generating @galacean/engine-design types.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
tests/src/core/Animator.test.ts (2)

1579-1623: 💤 Low value

Minor assertion style inconsistency.

Line 1616 uses .to.be.null while 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 value

Good 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 _state property:

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

📥 Commits

Reviewing files that changed from the base of the PR and between abb5c3a and bb323ca.

📒 Files selected for processing (6)
  • notes/animation/2026-06-15-animator-state-instance-boundary.md
  • packages/core/src/animation/Animator.ts
  • packages/core/src/animation/internal/AnimatorLayerData.ts
  • packages/core/src/animation/internal/AnimatorStatePlayData.ts
  • tests/src/core/Animator.test.ts
  • tests/src/core/AnimatorHang.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/src/core/AnimatorHang.test.ts

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

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

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

Expression-body arrow for _onStateMachineChanged, hoist layer local in
_setEngine, and use the destructured alias in clearLayers.
GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

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 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

总结(复审 03f9f07f2,删除我历轮 [P2] 的冗余 crossFade entry guard,行为等价,--approve 重新在当前 tip 建立通过)

自上轮 --approveacf7648cf,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 重新建立通过。

逐路径核对(✅ 删除等价 + 单一权威守卫收口)

删掉 _crossFadeAnimator.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 污染。回到 _crossFadeif (_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 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

总结(复审 a1eed4f79,删除一处 structure-restating 注释,docs-only 零产物逻辑,--approve 重新在当前 tip 建立通过)

自上轮 --approve03f9f07f2,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 重新建立通过。

逐处核对(✅ 删除合理 + 保留了真正解释「为什么」的注释)

删掉 _saveAnimatorStateDataAnimator.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)!xObject.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 未触碰的既有行,不在范围。删除注释无法引入注释违规。

这恰好收敛了我历轮记录的一个 P3Animator.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。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants