Fix animator play backwards error and onStateExit not triggered when crossFade finished and actualDeltaTime error #2325
Conversation
luzhuang
added
bug
WalkthroughThe recent changes enhance animation capabilities with the Galacean engine, introducing a new script for 3D scene animation that allows for reverse playback. A configuration option has been added to accommodate this feature. Additionally, improvements to the Animator class streamline transition logic, boosting code readability and maintainability while retaining existing functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Scene
participant Animator
User->>Scene: Initialize scene with camera and lighting
Scene->>Animator: Load GLTF model and configure animator
Animator->>Animator: Play "walk" animation in reverse
User->>Scene: Interact with orbit controls
Scene->>User: Render updated scene
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e/fixtures/originImage/Animator_animator-play-backwards.jpgis excluded by!**/*.jpg
Files selected for processing (3)
- e2e/case/animator-play-backwards.ts (1 hunks)
- e2e/config.ts (1 hunks)
- packages/core/src/animation/Animator.ts (2 hunks)
Additional comments not posted (4)
e2e/case/animator-play-backwards.ts (1)
1-37: LGTM! Ensure test coverage.The code for the new test case is well-structured and follows good practices for setting up a scene and testing animation playback. Ensure that this test case is included in the automated test suite to verify backward animation functionality.
e2e/config.ts (1)
48-52: Configuration addition looks good.The new
playBackWardsconfiguration option is consistent with the existing structure and enhances the functionality by supporting the new playback scenario.packages/core/src/animation/Animator.ts (2)
551-569: Refactoring improves readability.The use of destructuring and streamlined conditional logic in the transition application process enhances code readability and maintainability while preserving functionality.
900-918: Refactoring improves readability.The refactoring of transition logic in the
_updateFinishedStatemethod improves clarity without altering the existing behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- e2e/case/animator-play-backwards.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/case/animator-play-backwards.ts
luzhuang
changed the title
luzhuang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
tests/src/core/Animator.test.ts (1)
646-711: Well-structured test case for state machine scripts.The new test case effectively verifies the interaction between animator states and scripts using spies. Consider adding comments or documentation to describe the purpose and expected behavior of the test for future reference.
+ // Test case for validating state machine script interactions with animator states.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/animation/Animator.ts (5 hunks)
- tests/src/core/Animator.test.ts (1 hunks)
Additional comments not posted (3)
packages/core/src/animation/Animator.ts (3)
551-569: Improved transition logic with destructuring and conditional checks.The use of destructuring and concise conditional checks enhances the readability and maintainability of the code. Ensure that the logic for applying transitions is thoroughly tested to avoid regressions.
812-812: Ensure consistent handling ofactualCostTime.The handling of
actualCostTimewhenplaySpeedis zero should be consistent across different methods to avoid discrepancies in playback behavior.Verification successful
Consistent handling of
actualCostTimeconfirmed.The handling of
actualCostTimewhenplaySpeedis zero is consistent across the relevant methods inAnimator.ts. The logic is applied uniformly, ensuring no discrepancies in playback behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure consistent handling of `actualCostTime` across methods. # Test: Search for all occurrences of `actualCostTime`. Expect: Consistent handling in relevant methods. rg --type ts 'actualCostTime'Length of output: 582
Line range hint
687-699:
Verify impact ofactualCostTimechanges on playback timing.The modification to use
deltaTimewhendstPlaySpeedis zero may affect the timing calculations during playback. Verify that this change does not introduce any unintended side effects.
…crossFade finished and actualDeltaTime error (galacean#2325) * fix: animator play backwards error * fix: onStateExit not called when crossFade finished but state not finished * fix: actualDelta time calculate error when speed is 0
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Bug Fixes
Tests