added some simple functions to AnimationPlayer by asafigan · Pull Request #5912 · bevyengine/bevy

asafigan · 2022-09-08T03:02:47Z

Objective

Solution

Adding function to get animation_clip from AnimationPlayer while documenting that it could be the default handle.
Adding a function which tells if the animation player has finished playing the animation_clip.

The language used to explain what the default handle implies could be more clear. My problem is that the default handle could actually refer to an actual animation if an AnimationClip is set for the default handle, which leads me to ask, "Should animation_clip should be an Option?"

crates/bevy_animation/src/lib.rs

asafigan · 2022-09-08T05:20:23Z

I changed it to an option which was a really simple change.

crates/bevy_animation/src/lib.rs

afonsolage · 2022-09-08T07:00:35Z

crates/bevy_animation/src/lib.rs

            if elapsed < 0.0 {
                elapsed += animation_clip.duration;
+            } else if elapsed > animation_clip.duration {
+                player.finished = true;


Shouldn't this information (player.finished) be used to avoid calculating the elapsed time over and over again?

like add it together if paused check:

if player.finished || (player.paused && !player.is_changed()) { continue; }

Well I think:

if (player.paused || player.finished) && !player.is_changed() { continue; }

would be more correct with this implementation, because some field like repeat could have changed.

mockersf · 2022-09-08T07:21:58Z

I have a change stashed locally that does this and a bit more, but it's not finished yet because it's actually a bit more complicated than that.

I don't think this PR handles animations being played backward, repeating animations or setting the elapsed manually

asafigan · 2022-09-08T09:06:25Z

So for playing backward. It looks like the animation will repeat even if repeat is false. But I didn't change that. I could fix it in this PR.

In this PR, repeating animations never finish. The correctness of this is up to debate.

Setting elapsed manually has a one frame delay before finished is updated. This is unavoidable unless the AnimationPlayer keeps track of the AnimationClip's duration. It would take a frame to get this information whenever the animation player starts playing an animation. It could also be temporarily incorrect if the AnimationClip in Assets has been changed.

Which of these would you like me to work on?

mockersf · 2022-09-09T20:33:36Z

Which of these would you like me to work on?

I actually haven't found yet a fix that makes me happy, which is why I didn't open the PR. Try some things, let's see how it works out 😄

Another thing about repeat that I don't like if you have ideas on how to fix: setting repeat to false on a currently playing and repeating animation should let the current loop finish before stopping. This is currently true if done during the first repeat, not during others

asafigan · 2022-09-10T01:34:48Z

So for playing backward. It looks like the animation will repeat even if repeat is false. But I didn't change that. I could fix it in this PR.

In this PR, repeating animations never finish. The correctness of this is up to debate.

After a deeper look this is actually incorrect. I will set up so test cases and iterate over some solutions.

asafigan · 2022-09-10T01:50:16Z

What should the behavior of a non-repeating animation player with a negative speed at time 0?

Should it stay on the first frame or should it play the entire animation backwards. What should happen if speed is negative with any positive time? Should you have to set the time to the duration of the animation to play it once backwards? What if you want to play it forward and then backward?

With the current implementation, it seems like a non-repeating animation with negative speed set to the duration of the animation will play the animation backward twice.

Also I find it odd that the elapsed time goes up (or down if negative) forever. This is also weird because a non-repeating backward animation with an elapsed time greater than the duration of the animation will be on the last frame for a period of time before playing the animation backward (twice).

james7132

LGTM other than this one nit.

james7132 · 2022-12-17T00:43:13Z

crates/bevy_animation/src/lib.rs

 ) {
    for (entity, mut player) in &mut animation_players {
-        if let Some(animation_clip) = animations.get(&player.animation_clip) {
+        if let Some(animation_clip) = player.animation_clip().and_then(|x| animations.get(x)) {


I'd actually prefer for us to use let ... else with continue here instead of and_then inside the if expression.

james7132 · 2022-12-17T00:49:47Z

What should the behavior of a non-repeating animation player with a negative speed at time 0?
Should it stay on the first frame or should it play the entire animation backwards. What should happen if speed is negative with any positive time? Should you have to set the time to the duration of the animation to play it once backwards? What if you want to play it forward and then backward?

IMO, we should treat non-repeating behavior as a clamp on the time modulus the length of the clip. If it's not repeating and the speed is negative, we should play to the beginning then mark it finished. This is essentially playing the clip backwards, and allows users to ping-pong the clip if they so desire. It's the least surprising interaction between these two features.

alice-i-cecile · 2023-06-30T19:43:30Z

@asafigan can you resolve the merge conflicts? I'd like to get this moving again and they're more complex than I want to tackle in the web UI :)

alice-i-cecile · 2023-06-30T19:43:53Z

@DevinLeamy your review here would be welcome, since you're building on it.

DevinLeamy · 2023-06-30T23:18:02Z

@DevinLeamy your review here would be welcome, since you're building on it.

I'm on board with the changes here! IMO it probably would be easiest to patch them into #9002. @alice-i-cecile Any opposition to doing that?

alice-i-cecile · 2023-07-01T03:25:59Z

No objections; just credit the original author at the top of your PR description :)

DevinLeamy · 2023-07-01T13:56:20Z

No objections; just credit the original author at the top of your PR description :)

Of course! It's been updated.

alice-i-cecile · 2023-07-01T15:28:54Z

Updated and incorporated into #9002. Thanks a ton for your work, and if you get a chance your review there would be welcome!

@asafigan

# Objective Added `AnimationPlayer` API UX improvements. - Succestor to #5912 - Fixes #5848 _(Credits to @asafigan for filing #5848, creating the initial pull request, and the discussion in #5912)_ ## Solution - Created `RepeatAnimation` enum to describe an animation repetition behavior. - Added `is_finished()`, `set_repeat()`, and `is_playback_reversed()` methods to the animation player. - ~~Made the animation clip optional as per the comment from #5912~~ > ~~My problem is that the default handle [used the initialize a `PlayingAnimation`] could actually refer to an actual animation if an AnimationClip is set for the default handle, which leads me to ask, "Should animation_clip should be an Option?"~~ - Added an accessor for the animation clip `animation_clip()` to the animation player. To determine if an animation is finished, we use the number of times the animation has completed and the repetition behavior. If the animation is playing in reverse then `elapsed < 0.0` counts as a completion. Otherwise, `elapsed > animation.duration` counts as a completion. This is what I would expect, personally. If there's any ambiguity, perhaps we could add some `AnimationCompletionBehavior`, to specify that kind of completion behavior to use. Update: Previously `PlayingAnimation::elapsed` was being used as the seek time into the animation clip. This was misleading because if you increased the speed of the animation it would also increase (or decrease) the elapsed time. In other words, the elapsed time was not actually the elapsed time. To solve this, we introduce `PlayingAnimation::seek_time` to serve as the value we manipulate the move between keyframes. Consequently, `elapsed()` now returns the actual elapsed time, and is not effected by the animation speed. Because `set_elapsed` was being used to manipulate the displayed keyframe, we introduce `AnimationPlayer::seek_to` and `AnimationPlayer::replay` to provide this functionality. ## Migration Guide - Removed `set_elapsed`. - Removed `stop_repeating` in favour of `AnimationPlayer::set_repeat(RepeatAnimation::Never)`. - Introduced `seek_to` to seek to a given timestamp inside of the animation. - Introduced `seek_time` accessor for the `PlayingAnimation::seek_to`. - Introduced `AnimationPlayer::replay` to reset the `PlayingAnimation` to a state where no time has elapsed. --------- Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com> Co-authored-by: François <mockersf@gmail.com>

@asafigan

# Objective Added `AnimationPlayer` API UX improvements. - Succestor to bevyengine#5912 - Fixes bevyengine#5848 _(Credits to @asafigan for filing bevyengine#5848, creating the initial pull request, and the discussion in bevyengine#5912)_ ## Solution - Created `RepeatAnimation` enum to describe an animation repetition behavior. - Added `is_finished()`, `set_repeat()`, and `is_playback_reversed()` methods to the animation player. - ~~Made the animation clip optional as per the comment from bevyengine#5912~~ > ~~My problem is that the default handle [used the initialize a `PlayingAnimation`] could actually refer to an actual animation if an AnimationClip is set for the default handle, which leads me to ask, "Should animation_clip should be an Option?"~~ - Added an accessor for the animation clip `animation_clip()` to the animation player. To determine if an animation is finished, we use the number of times the animation has completed and the repetition behavior. If the animation is playing in reverse then `elapsed < 0.0` counts as a completion. Otherwise, `elapsed > animation.duration` counts as a completion. This is what I would expect, personally. If there's any ambiguity, perhaps we could add some `AnimationCompletionBehavior`, to specify that kind of completion behavior to use. Update: Previously `PlayingAnimation::elapsed` was being used as the seek time into the animation clip. This was misleading because if you increased the speed of the animation it would also increase (or decrease) the elapsed time. In other words, the elapsed time was not actually the elapsed time. To solve this, we introduce `PlayingAnimation::seek_time` to serve as the value we manipulate the move between keyframes. Consequently, `elapsed()` now returns the actual elapsed time, and is not effected by the animation speed. Because `set_elapsed` was being used to manipulate the displayed keyframe, we introduce `AnimationPlayer::seek_to` and `AnimationPlayer::replay` to provide this functionality. ## Migration Guide - Removed `set_elapsed`. - Removed `stop_repeating` in favour of `AnimationPlayer::set_repeat(RepeatAnimation::Never)`. - Introduced `seek_to` to seek to a given timestamp inside of the animation. - Introduced `seek_time` accessor for the `PlayingAnimation::seek_to`. - Introduced `AnimationPlayer::replay` to reset the `PlayingAnimation` to a state where no time has elapsed. --------- Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com> Co-authored-by: François <mockersf@gmail.com>

added some simple functions to AnimationPlayer

9d81bf3

alice-i-cecile requested a review from mockersf September 8, 2022 03:16

alice-i-cecile added A-Animation Make things move and change over time C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 8, 2022

alice-i-cecile reviewed Sep 8, 2022

View reviewed changes

crates/bevy_animation/src/lib.rs Outdated Show resolved Hide resolved

asafigan added 2 commits September 8, 2022 14:17

change animation_clip to an option

e038fd9

remove part of doc comment

43481b3

tguichaoua reviewed Sep 8, 2022

View reviewed changes

crates/bevy_animation/src/lib.rs Outdated Show resolved Hide resolved

afonsolage reviewed Sep 8, 2022

View reviewed changes

return an option instead of a ref to an option

5b867f2

james7132 approved these changes Dec 17, 2022

View reviewed changes

james7132 requested review from afonsolage, alice-i-cecile and tguichaoua and removed request for afonsolage and tguichaoua December 17, 2022 00:44

DevinLeamy mentioned this pull request Jun 30, 2023

API updates to the AnimationPlayer #9002

Merged

alice-i-cecile approved these changes Jun 30, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 30, 2023

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 30, 2023

alice-i-cecile closed this Jul 1, 2023

Uh oh!

Conversation

asafigan commented Sep 8, 2022

Objective

Solution

Uh oh!

Uh oh!

asafigan commented Sep 8, 2022

Uh oh!

Uh oh!

afonsolage Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asafigan Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf commented Sep 8, 2022

Uh oh!

asafigan commented Sep 8, 2022

Uh oh!

mockersf commented Sep 9, 2022

Uh oh!

asafigan commented Sep 10, 2022

Uh oh!

asafigan commented Sep 10, 2022

Uh oh!

james7132 left a comment

Choose a reason for hiding this comment

Uh oh!

james7132 Dec 17, 2022

Choose a reason for hiding this comment

Uh oh!

james7132 commented Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jun 30, 2023

Uh oh!

alice-i-cecile commented Jun 30, 2023

Uh oh!

DevinLeamy commented Jun 30, 2023

Uh oh!

alice-i-cecile commented Jul 1, 2023

Uh oh!

DevinLeamy commented Jul 1, 2023

Uh oh!

alice-i-cecile commented Jul 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

afonsolage Sep 8, 2022 •

edited

Loading

asafigan Sep 8, 2022 •

edited

Loading

james7132 commented Dec 17, 2022 •

edited

Loading