Support running an animation N times

[SUPERCILEX]

Fixes #17

This supports an animation like the following:

pub fn error_shake(current: &Transform) -> Sequence<Transform> {
    let wiggle = Quat::from_rotation_z(PI / 16.);
    Tween::new(
        EaseMethod::Linear,
        TweeningType::Once,
        Duration::from_millis(25),
        TransformRotationLens {
            start: current.rotation,
            end: current.rotation * wiggle.inverse(),
        },
    )
    .then(Tween::new(
        EaseMethod::Linear,
        TweeningType::PingPongTimes(3),
        Duration::from_millis(50),
        TransformRotationLens {
            start: current.rotation * wiggle.inverse(),
            end: current.rotation * wiggle,
        },
    ))
    .then(Tween::new(
        EaseMethod::Linear,
        TweeningType::Once,
        Duration::from_millis(25),
        TransformRotationLens {
            start: current.rotation * wiggle,
            end: current.rotation,
        },
    ))
}

[SUPERCILEX]

@djeedai Thoughts on getting rid of is_looping as per above?

[SUPERCILEX]

This PR depends on #29 so that I don't have to manually adjust line breaks while editing the docs.

[SUPERCILEX]

Mmmm, the diff is garbage so I guess #29 needs to go through and then I'll rebase.

[SUPERCILEX]

Ok, the diff is actually usable now.

[SUPERCILEX]

@djeedai bump

[djeedai]

Thanks for the work! Sorry I missed your earlier message, I didn't realize this was ready for review. I think it looks mostly good, aside from a terminology question on the repeat strategy. However I'm concerned about the additional reset() in the examples, it really shouldn't be needed.

[djeedai]

I don't know if "teleport" really convey the meaning here (teleport to where?). What about using the terminology of texture mapping and call that"repeat", and the bounce one "mirrored repeat"? Although they're not great terms, they do have the advantage of being familiar.

Alternatively, what about "wrap"?

[SUPERCILEX]

Repeat and mirrored repeat works for me.

[djeedai]

Why is that needed? It shouldn't, set_progress() should take care of updating any state as needed such that the Tracks tweenable is in a state where the next tick() will start from 90% progress.

[SUPERCILEX]

Maybe I misunderstood our previous discussion, but I thought the plan was to make progress a local concept. That is, progress only applies within one repetition. The rewinds are there because setting the progress isn't changing the number of times the animation has repeated. Right now, there's no API to do that.

[djeedai]

The problem of doing so is to implement Tracks. I originally added set_progress() mainly for that purpose, and exposed it publicly because why not.

Consider a Track with 2 Tweens:

• Tween A repeats every 1 second
• Tween B lasts 4 seconds

The Track duration is computed as 4 seconds. Now, if you call set_progress(0.75) on the track itself, you need somehow to set the times_completed of A to 3. The idea was that A's own set_progress() internal call should take care of that.

Now, we can also add a separate API to set the times_completed, and maybe that's valuable for the case you just want to add a few loops without changing the current state (why, I'm not sure, but let's assume there's a use case). But now for the use case of "I have a duration obtained from my game/editor and want to set the animation progress" it would much nicer if I don't have to do some modulo arithmetic as a user and can just call set_progress(23.58) and it sets times_completed to 23 and progress to 0.58 by itself, no?

[SUPERCILEX]

Interesting, I only considered sequences where the model works fine since the progress applies within one repetition of the sequence.

I'm not sure your model works because what if Tween A repeats infinitely? The tracks still need to handle setting times completed manually to be correct (since progress * infinity would be infinite repetitions). The model breaks down when you have an infinite tween, hence keeping progress local only.

Then again, I see your point about making it easy to scrub through a tween, but I still really don't like the idea of having to share your internal progress precision with your times completed. I want to get the bits right, so that leaves us with several things to consider if we go the progress does everything route:

• Currently times_completed is 32 bits and everything we do is saturating.
  • Everything should maybe wrap instead? The transition might be glitchy, but at least the animation won't get stuck in infinite mode. A problem with this is that if your repetition count is right on the border of wrapping, the timing might work out such that the tick is too big and causes two completions thus wrapping and causing us to not realize we were supposed to stop. I think this is acceptable given enough bits because no one is going to be repeating exactly some number of times greater than 1000.
  • We can either make times_completed 16 bits and keep f32s which gives 7 decimal places to work with at saturation or use f64 with 32 bits of time completion which gives 11 decimal places of precision at saturation.
• If the way to set times_completed is through progress, then the times_completed API needs to be removed.

What do you think about this plan: use 16 bits of times_completed, keep f32 progress, make progress always use fract instead of clamping (aka be times_completed.progress), get rid of the times_completed API.

PS: In terms of PRs, this one is getting too big for me so I'd like to get agreement on future plans, merge it, and then fix whatever the plan is + sequences + tracks in separate PRs which means things might be in a slightly broken state in the interim.

[SUPERCILEX]

BTW, I think this solves #8. The answer to what happens with PingPong asymmetric tracks is "compute the times_completed.progress for each track given that 1.0 is defined as the length of the longest track." This works fine with infinitely repeating tweens too since we only care about how long a single repetition is.

[djeedai]

I only considered sequences where the model works fine since the progress applies within one repetition of the sequence.

Yes Sequence is generally easier, Tracks is where most issues occur.

I'm not sure your model works because what if Tween A repeats infinitely?

This is explicitly disallowed. I'm pretty sure I documented that somewhere. That's one limitation of Tracks.

I still really don't like the idea of having to share your internal progress precision with your times completed.

That's a fair point. Setting progress as f32 will have a limited precision. Although I doubt in practice this matters too much. Note however that I didn't say we should internally merge the [0:1] progress with the times_completed; that would be indeed bad. Internally they stay separate, I just though to make the API easier to use. But I'm starting to think you may be right that it would be better to make set_progress() consistent with the internal split. Also, maybe set_progress() is a bad interface, and we should instead have set_elapsed() which takes a Duration and has enough precision with 128 bits.

Everything should maybe wrap instead? The transition might be glitchy, but at least the animation won't get stuck in infinite mode.
[...]
I think this is acceptable given enough bits because no one is going to be repeating exactly some number of times greater than 1000.

Yes that was my reasoning too, I doubt any repeat count will go over, say 1 million to be conservative. Think of a UI animation repeating every 250ms on screen, forever while the game plays, over 1 year of gameplay (!) that's "only" 126 millions, so fits in 32 bits. Unlikely we ever hit the wrapping. Saturating or wrapping, probably doesn't matter.

What do you think about this plan: use 16 bits of times_completed, keep f32 progress, make progress always use fract instead of clamping (aka be times_completed.progress), get rid of the times_completed API.

Sounds good, but keeping 32-bit times_completed, at least for now. With 16 bits only, my example above reaches capacity at 4.5 hours of game running, which is quite common to hit. If we really think we need to save 2 extra bytes we can review and nail down a precise wrap around behavior in a future change; in the meantime, I propose we just use 32 bits and push the problem away enough that we don't need to care.

In terms of PRs, this one is getting too big for me so I'd like to get agreement on future plans, merge it, and then fix whatever the plan is + sequences + tracks in separate PRs which means things might be in a slightly broken state in the interim.

Sounds good.

BTW, I think this solves #8. The answer to what happens with PingPong asymmetric tracks is "compute the times_completed.progress for each track given that 1.0 is defined as the length of the longest track." This works fine with infinitely repeating tweens too since we only care about how long a single repetition is.

I don't think it fully does. What happens with the previous example of tweens A and B if tween B is 5 seconds long, and tween A is 1 second long and repeat 4 times (so total 4 seconds). What happens between t=4s and t=5s for tween A? And what happens at t=5s when reversing direction? I don't think there's a unique answer, but it's still unclear to me what's the least surprising behavior for users here.

[djeedai]

I'm not sure your model works because what if Tween A repeats infinitely?

This is explicitly disallowed. I'm pretty sure I documented that somewhere. That's one limitation of Tracks.

That being said, we could make that work by saying that the Tracks itself becomes of infinite duration/repeat, and that any child tweenable that doesn't repeat infinitely is some kind of "intro" that will only ever be played once at start. Not sure it's worth the hassle though.

[SUPERCILEX]

That's it! set_elapsed is the missing piece. It's objectively the better API because it's more fundamental — progress and times_completed can be derived from the combination of duration and elapsed.

This also leads me to believe number of completions for the repetition count is the wrong way to go. It should instead be a max duration. That's strictly more flexible since you can end an animation in a partially completed state. Again, it's also a more fundamental API because you can derive a max duration from a desired number of completions.

---

I don't think it fully does. What happens with the previous example of tweens A and B if tween B is 5 seconds long, and tween A is 1 second long and repeat 4 times (so total 4 seconds). What happens between t=4s and t=5s for tween A? And what happens at t=5s when reversing direction? I don't think there's a unique answer, but it's still unclear to me what's the least surprising behavior for users here.

I agree that there isn't a unique answer, but I think if we get the fundamentals right, we'll be able to find solutions that maximize consistency. set_elapsed for example makes updating the progress of tracks trivially simple: call set_elapsed on each sub tween. Infinite tweens don't cause problems because they'll update the same as everyone else with the only difference being that they'll never finish.

On that note, I think we need to figure out infinite tweens once and for all. The core conflict lies in the fact that we want non-infinite tweens to run through their repetitions before we move on whereas infinite tweens doing the same makes no sense. I spent a few hours thinking about this and didn't really get anywhere: you end up with a bunch of contradictions or limitations if you do weird stuff with the infinities. Thus, the most consistent answer is likely the simplest: infinity means infinity. If you put something infinite in a sequence or track, then any non-zero number of repetitions is the same because you'll never actually repeat.

As for your example, I believe the most flexible answer is to say that tween A will stop and reversing directions will simply call the tween in the same order but with the direction reversed. If it turns out you wanted symmetry, you can create a sequence with Tween A and a delay of 1s afterwards. On the other hand, if we enforce symmetry directly in the tracks, then there's no way for people to get A and B starting at the same time but reversed which is why we have to let the user build symmetry for themselves if so desired. Perhaps if people want it, we could add a constructor for tracks that automatically makes every tracks symmetric by wrapping it in a sequence + delay, but the point is that the API is flexible enough for us to be able to do that in the future.

Ok, so here's the updated plan:

• Get rid of all notion of progress.
  • This (and times_completed) could be added back via an extension trait that only uses the public API of Tweenable if people want it.
• Get rid of the times_completed API.
• Get rid of the set_repeat_* methods because changing the repeat count of a live tween in a sequence for example wouldn't make any sense (our duration would be wrong).
• Add RepeatCount::Until(Duration). Under the hood, Finite(u32) will just convert itself to Until once we know the tween duration.
• Unrelated but I just noticed this: get rid of enabled in *_completed_event methods since you basically always pass in true.
  • Add a clear_completed_event method to be consistent with the callback APIs.
• Add set_elapsed and elapsed methods.
• Make sequences and tracks simply wait for their tweens to complete before repeating (which means infinite tweens block their completion).
• Tracks always play their tweens in the same order when repeating (meaning creating symmetry is left to the user through a combination of sequence + delay).

[djeedai]

This also leads me to believe number of completions for the repetition count is the wrong way to go. It should instead be a max duration. That's strictly more flexible since you can end an animation in a partially completed state. Again, it's also a more fundamental API because you can derive a max duration from a desired number of completions.

That looks like a very low level API not very practical for common use cases. Ending an animation in a partially completed state is probably not a very common use case, is it? Not objecting to having that low-level API, but probably need to retain the higher-level one too based on repeat count.

I agree that there isn't a unique answer, but I think if we get the fundamentals right, we'll be able to find solutions that maximize consistency. set_elapsed for example makes updating the progress of tracks trivially simple: call set_elapsed on each sub tween. Infinite tweens don't cause problems because they'll update the same as everyone else with the only difference being that they'll never finish.

Very good point. That will cause issue to implement Tweenable::duration() though if we allow infinite tweenables inside a Tracks. Or maybe we need to return Option<Duration> there or an enum Finite(Duration) vs. Infinite?

If you put something infinite in a sequence or track, then any non-zero number of repetitions is the same because you'll never actually repeat.

Yes, we can't repeat a sequence or tracks with an infinite child. We could maybe throw a warning in that case on creation, or return a Result<> somewhere to catch those misuses.

As for your example, I believe the most flexible answer is to say that tween A will stop and reversing directions will simply call the tween in the same order but with the direction reversed. If it turns out you wanted symmetry, you can create a sequence with Tween A and a delay of 1s afterwards. On the other hand, if we enforce symmetry directly in the tracks, then there's no way for people to get A and B starting at the same time but reversed which is why we have to let the user build symmetry for themselves if so desired. Perhaps if people want it, we could add a constructor for tracks that automatically makes every tracks symmetric by wrapping it in a sequence + delay, but the point is that the API is flexible enough for us to be able to do that in the future.

All agreed.

• Get rid of all notion of progress.
  • This (and times_completed) could be added back via an extension trait that only uses the public API of Tweenable if people want it.
• Get rid of the times_completed API.

Not sure I'd be as aggressive as to totally remove them, but yes at least implement them in terms of the more fundamental set_elapsed(), and avoid referring to those concepts internally in the implementation.

• Get rid of the set_repeat_* methods because changing the repeat count of a live tween in a sequence for example wouldn't make any sense (our duration would be wrong).

I guess this was already not well defined, so removing probably makes sense. We can re-add later if we have a well-defined behavior for this.

• Add RepeatCount::Until(Duration). Under the hood, Finite(u32) will just convert itself to Until once we know the tween duration.

Sounds good.

• Unrelated but I just noticed this: get rid of enabled in *_completed_event methods since you basically always pass in true.
  • Add a clear_completed_event method to be consistent with the callback APIs.

See other PR; this is just a different API proposal for the same thing. Not necessarily opposed to it, if there's a rationale to justify this one is better than the existing one. Not sure personally which one is better.

• Make sequences and tracks simply wait for their tweens to complete before repeating (which means infinite tweens block their completion).

This is already the case for Sequence, but yes agreed for Tracks.

• Tracks always play their tweens in the same order when repeating (meaning creating symmetry is left to the user through a combination of sequence + delay).

Yes.

[SUPERCILEX]

need to retain the higher-level one too based on repeat count.

Agreed.

Or maybe we need to return Option there or an enum Finite(Duration) vs. Infinite?

I was thinking we could just return Duration::MAX, but I think I like the enum better.

Yes, we can't repeat a sequence or tracks with an infinite child. We could maybe throw a warning in that case on creation, or return a Result<> somewhere to catch those misuses.

I'm not sure that's worth it because it does make sense to use infinite tweens. For example you could make a track for a tutorial that moves an object to where the player is supposed to focus and also wiggles the object. You could want the wiggle animation to go on forever while the movement animation only completes N times.

I guess we could crash if you use a repeat count greater than 1, but eh, it's not an invalid state.

Not sure I'd be as aggressive as to totally remove them, but yes at least implement them in terms of the more fundamental set_elapsed(), and avoid referring to those concepts internally in the implementation.

In that case I'd like to add them back via an extension trait so as to not pollute the core API.

I guess this was already not well defined, so removing probably makes sense. We can re-add later if we have a well-defined behavior for this.

👍

---

Woohoo, looks like we have a path forward! I'll try to get this PR fixed up today or sometime this week.

[SUPERCILEX]

@djeedai Ok, I think this PR is ready to be merged! Stuff is still a bit half-baked, but I'll clean it up in the next PR.

[djeedai]

A few minor things but should be good after that!

[SUPERCILEX]

The CI error is fake: note: D:\a\bevy_tweening\bevy_tweening\target\debug\examples\transform_translation.exe : fatal error LNK1180: insufficient disk space to complete link

[SUPERCILEX]

@djeedai oh wait, are you going to ship these changes in the 0.8 compatible version? I'll have to speed up a bit then so we end up in a nice place. :)

[djeedai]

@djeedai oh wait, are you going to ship these changes in the 0.8 compatible version? I'll have to speed up a bit then so we end up in a nice place. :)

Yes I am, this is a very good feature I think, which if I remember has been asked on Discord. If we can get the progress update (#31) that would be nice too, but I'd like to get a new release supporting 0.8 as soon as possible, as several people asked for it and are waiting, and there's an upcoming Bevy jam too. Let me know if you think you can make it, or we can cut today-ish and we can make another release 0.5.1 later with some added features etc.

[SUPERCILEX]

Sounds good, I'll see what I'm able to get done. I just want to make sure we aren't rushing.