djeedai / bevy_tweening

Tweening animation plugin for the Bevy game engine.
Other
412 stars 65 forks source link

Improve capabilities of modifying Tweenables after creation #83

Open ivenw opened 1 year ago

ivenw commented 1 year ago

I have implemented a drag and drop system for Sprite and would like to build on this by having sprite ease back towards its original position after being dropped.

Here I came across some limitations of bevy_tweening.

For this I query Animator<Transform> in a system that checks for the Sprite position and whether it has been dropped. Today, there seems to be no way to update the TransformationPositionLens of the Tween of that Animator with a new start value, or change other fields of the Tween (as they are private).

The only way to get this working that I found is to use the set_tweenable method on the Animator to set a new Tween. I am not sure about performance impact, but it seems not an ideal way to do things. This also means that the Tween originally defined for this Animator is just unused boilerplate.

I would like to define an initial Tween and then change some of its parameters in a system call, without having to instantiate a new Tween.

I don’t know if this is in the scope of this crate or if there are technical reasons this is not possible, but this seems like an area where improved ergonomics would make bevy_tweening even more useful.

ivenw commented 1 year ago

It occurred to me that this could also enable faster prototyping with the InspectorPlugin as showcased in examples/transform_rotation.rs for the animation speed.

djeedai commented 1 year ago

Yes, set_tweenable() is the current flow to do this. I appreciate it's not the nicest, but there's a few issues doing what you suggest:

Note that as explained somewhere else, you can create your own Lens which instead of storing the end Vec3 instead stores a reference to it like Arc<Mutex<Vec3>> or whatnot, and keep a reference to modify the lens from your code at any point. I've not tried, but I believe that mostly works. So far personally I've found that recreating the lens/tween is not too clunky if you have a helper function. But maybe there's a case where that's too slow? Not sure.

I'm open to suggestions of how this can be improved, but remembering the complication of the trait object, which I think is the main blocker here.

ivenw commented 1 year ago

Btw, I should say thanks for putting together bevy_tweening. I found it so far very enjoyable to use :)

  • Practically, the Animator doesn't hold a Tween (struct) but a dyn Tweenable trait object, and I don't see how you can expose an interface to get from there to your Tween to modify it.

I was afraid that was the case but am not quite there with my understanding of Traits to have arrived at that conclusion myself.

  • Theoretically, I'm slightly concerned that allowing mutations further increases the surface of changes to take into account in the Animator, which would make it even more complex and with even more edge cases to handle. Maybe that's me worrying too much, and that would work, but that was one of the reason for the Animator taking ownership and "hiding" the Tweenable once it started.

I could see how that introduces unnecessary trouble. (What if the Tweenable parameters change while it is running? etc.)

I have yet to try your suggestion of using Arc<Mutex<T>> but arrived at an idea to make setting a new Tweenable as a way to react to e.g. user input more ergonomic by slightly increase the API surface.

The main thing that made me feel like doing something suboptimal/non intended, was to instantiate the Animator first with a Tween that was essentially unused. I would suggest to implement a Dummy<T> Tweenable that serves as a placeholder and easy one-line instantiation of a Animator component at time of Entity spawn.

That Dummy<T> wouldn't too anything but signal that the Animator is being added for later use. (Useful for avoiding frame delays that come from adding an Animator to an Entity during a later point. There are of course ways around that with using stages but those are more error prone and cumbersome.)

In practice this could look like this:

Animator::new(Dummy::<Transform>::new())

I have put together a hack implementation that makes Dummy::<T>::new() return a Delay::<T>::new(Duration.from_nanos(1)) but I am sure this can be done better. (Avoiding the anti-pattern of Dummy::<T>::new() not returning Self.)

I think this could work out quite well as it doesn't affect other systems but makes the API more capable of expressing this particular intent. This would be preferable over a convenience function too, as it sticks to the existing API patterns.

As most of the time we wouldn't want to replace a running Tweenable with a new one, I would suggest adding a is_completed() -> bool convenience method to Animator that just wraps around tweenable().progress() == 1.0.

What do you think?

ivenw commented 1 year ago

I also put together a small example that could be used to demonstrate this. (Although here I am not using the proposed is_completed())

https://github.com/ivenw/bevy_tweening/blob/main/examples/squash_stretch_jump.rs