Prevent same-frame duplicate onUpdate in stateMachineScript of Animator #2805
Conversation
WalkthroughRefactors Animator end-of-frame event dispatch to explicitly handle Finished state and zero deltaTime. Ensures onExit fires once upon finishing, skips onUpdate when Finished or deltaTime is zero, and retains onEnter on first start. Adds tests covering zero-duration crossFade, no duplicate updates, and reverse playback update behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Time
participant Animator
participant PlayData
participant StateScript
Time->>Animator: update(deltaTime)
Animator->>PlayData: inspect playState, prevState
alt prevState == UnStarted
Animator->>StateScript: onStateEnter()
end
alt PlayState == Finished
note over Animator,StateScript: Finished handling
opt prevState != Finished
Animator->>StateScript: onStateExit()
end
Note over Animator: Return (no onUpdate)
else deltaTime == 0
Note over Animator: Skip onUpdate
else Normal advance
Animator->>StateScript: onStateUpdate(deltaTime)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…updates
Exit once when Finished; skip onUpdate in same frame
Skip onUpdate when deltaTime === 0;
Add tests in tests/src/core/Animator.test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2805 +/- ##
==========================================
+ Coverage 79.23% 79.26% +0.02%
==========================================
Files 846 846
Lines 90233 90238 +5
Branches 8951 8956 +5
==========================================
+ Hits 71500 71523 +23
+ Misses 18592 18574 -18
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/animation/Animator.ts (2)
1571-1574: Skip onUpdate for zero advance; consider tolerating sub-epsilon deltas.The zero-delta guard aligns script callbacks with evaluation (no time advanced => no onUpdate). To avoid spurious onUpdate from floating drift (e.g., tiny ±1e-15 residuals), consider a tolerance:
- if (deltaTime === 0) { + if (Math.abs(deltaTime) <= MathUtil.zeroTolerance) { return; }This still allows meaningful reverse playback (negative deltas) to reach onUpdate.
1548-1578: Clarify parameter semantics for maintainers (deltaTime here is “clip-time delta”, not engine time).deltaTime in this helper is the per-state “played” delta (may be 0 or negative), not engine Time.deltaTime. A short doc block will prevent future regressions.
TypeScript snippet to add above the method (no code movement required):
/** * Fire animation events and invoke state machine scripts for this slice. * Note: `deltaTime` is the state's clip-time delta used for evaluation in this call. * It is 0 when no time advanced (e.g., first frame after play/crossFade, zero speed, zero-duration transition), * and may be negative during reverse playback. */tests/src/core/Animator.test.ts (3)
711-755: Great coverage for “finish this frame” semantics; make cleanup robust.The assertions precisely capture the intended callback order. To avoid leaving scripts attached if an assertion throws, wrap spy assertions in try/finally and always remove scripts:
- const walkExitSpy = vi.spyOn(walkScript, "onStateExit"); + const walkExitSpy = vi.spyOn(walkScript, "onStateExit"); // Play source state, then in the same frame crossFade to dest with duration 0 animator.play("Walk"); animator.crossFade("Run", 0); animator.update(0.1); - expect(walkExitSpy).toHaveBeenCalledTimes(1); - expect(walkUpdateSpy).toHaveBeenCalledTimes(0); - expect(runEnterSpy).toHaveBeenCalledTimes(1); - expect(runUpdateSpy).toHaveBeenCalledTimes(0); - expect(runExitSpy).toHaveBeenCalledTimes(0); - - // cleanup: remove scripts - // @ts-ignore - walkState._removeStateMachineScript(walkScript); - // @ts-ignore - runState._removeStateMachineScript(runScript); + try { + expect(walkExitSpy).toHaveBeenCalledTimes(1); + expect(walkUpdateSpy).toHaveBeenCalledTimes(0); + expect(runEnterSpy).toHaveBeenCalledTimes(1); + expect(runUpdateSpy).toHaveBeenCalledTimes(0); + expect(runExitSpy).toHaveBeenCalledTimes(0); + } finally { + // cleanup: remove scripts + // @ts-ignore + walkState._removeStateMachineScript(walkScript); + // @ts-ignore + runState._removeStateMachineScript(runScript); + }
757-787: Remove unused variable to keep tests clean.walkState is declared but never used.
- const walkState = animator.findAnimatorState("Walk"); - const runState = animator.findAnimatorState("Run"); + const runState = animator.findAnimatorState("Run");
789-813: Reverse playback callback check looks good; optionally assert no extra calls.Current check ensures at least one onStateUpdate during reverse playback. To tighten it slightly without brittleness, assert exactly one call (mirrors your positive-delta tests).
- expect(runUpdateSpy).toHaveBeenCalledTimes(1); + expect(runUpdateSpy).toHaveBeenCalledTimes(1);If you later add multiple updates in this test, keep the exact count aligned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/core/src/animation/Animator.ts(1 hunks)tests/src/core/Animator.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/animation/Animator.ts (1)
packages/core/src/base/Time.ts (1)
deltaTime(40-42)
tests/src/core/Animator.test.ts (1)
packages/core/src/animation/index.ts (1)
StateMachineScript(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (4)
packages/core/src/animation/Animator.ts (1)
1562-1569: Correctly suppress onUpdate when state finishes in this slice; onExit fires once.The early-return on Finished prevents same-frame onUpdate after onExit and guards against duplicate exits across re-entrance. This matches the stated intent and the tests.
tests/src/core/Animator.test.ts (3)
326-327: Formatting-only change; OK.
872-874: Formatting-only change; OK.
1125-1125: Style-only change to closing brace; OK.
|
This PR is a partial fix. |
|
First, it's important to understand that this is a Callback to the state machine, so it's independent of the Clip's behavior. The state machine strives for state continuity. Therefore:
|
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
解决问题: