Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making Tweens more straightforward and useful #26529

Closed
KoBeWi opened this issue Mar 3, 2019 · 18 comments
Closed

Making Tweens more straightforward and useful #26529

KoBeWi opened this issue Mar 3, 2019 · 18 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Mar 3, 2019

Godot version:
3.1 beta10

Issue description:
Some time ago I used an Unity extension called DOTween. It was one of the best things I had come into. Compared to that, Godot's Tweens are terrible. Well, this might be also because Godot has AnimationPlayer, which Unity doesn't, but whatever. The only use for Tweens I see right now is animating properties that might have different start/end values, which AnimationPlayer doesn't support. However I never actually used Tweens and when needed I rather went with simple lerp.

To fix that, I'd like to suggest a complete overhaul of Tween workflow, to make it more like DOTween. Right now to tween something you need to do:

var tween = Tween.new()
tween.interpolate_property(sprite, 'transform/scale', sprite.get_scale(), Vector2(2.0, 2.0), 0.3, Tween.TRANS_QUAD, Tween.EASE_OUT)
add_child(tween)
tween.start()

The DOTween equivalent would be this, somewhat translated to Godot-ish:

Tween.To(sprite, "get_scale", "set_scale", Vector2(2.0, 2.0), 0.3).SetEase(Tween.QUAD_OUT)

Or even simpler:

sprite.DoScale(Vector2(2.0, 2.0), 0.3).SetEase(Tween.QUAD_OUT)

As for the second way, it works by somehow extending the target object to add tweening methods. I guess it's unfeasible unless there's a way to enable it per script, because otherwise every object would have tweening stuff not everyone uses.

As for the first one, that's how it should work IMO. It tweens the property from current value to target one in given time. "get_scale" and "set_scale" are getters/setters for the property. DOTween uses lambdas for that, but since we can just provide property path like in current interpolate_property, then maybe it's not needed.

Anyways, calling this method would automatically add the Tween to scene and start it. So it's a "fire and forget" approach and that's infinitely more useful than it's right now. Also, sequences. This is code from an issue about Tween sequencing (#17543, with one change):

var tween = Tween.new()
tween.interpolate_property(node, property, start_value, end_value, time, trans_type, ease_type)
tween.start()
yield(tween, "tween_completed")
tween.interpolate_property(node, property2, start_value2, end_value2, time2, trans_type, ease_type, delay)
tween.start()
yield(tween, "tween_completed")

DOTween's way would be something like (with the getter/setter removed as I mentioned):

Tween.Sequence()
.Append(Tween.To(node, property, end_value, time).SetEase(ease_type))
.AppendInterval(delay)
.Append(Tween.To(node, property2, end_value2, time2).SetEase(ease_type))

or

Tween.Sequence()
.Append(Tween.To(node, property, end_value, time).SetEase(ease_type))
.Append(Tween.To(node, property2, end_value2, time2).SetEase(ease_type).SetDelay(delay))

Much simpler, much more clear. It's just about adding static methods that make working with Tweens as straightforward as possible, by doing much of the work for you. It also simplifies methods by removing arguments and delegating them to other methods (it's simpler when you e.g. set a default ease and don't need to change it, so you don't repeat it all the time). It works because To() method returns Tweener and you can change its properties or append to sequence. Since managing this is done automatically, it's much more productive.

So that's it. I've seen another issue (#22513) that there's some Tween overhaul coming, so this might come as idea on how could they eventually work.

@Zylann
Copy link
Contributor

Zylann commented Mar 3, 2019

One aspect of this is that the scene tree is not a singleton AFAIK, so contrary to DOTween you'd need to pass the tree somehow as argument of the tween statement (while in Unity, new objects are directly created in the scene), as long as tweens are nodes.

Then DOTween can build on top of this and expose its API as static functions, also performing pooling so allocations are minimized if you run hundreds of tweens (yes we keep saying Godot doesnt need pooling, but I still don't believe that constructing nodes has no cost).
On C# side it also uses extension methods to further shorten the calls in various ways but I never really used that.

Note: Unity does have equivalents of AnimationPlayer. Also, different start/end values is possible by changing keyframes at runtime, I did that in my game. It's just a bit unfriendly.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 3, 2019

One aspect of this is that the scene tree is not a singleton AFAIK, so contrary to DOTween you'd need to pass the tree somehow as argument of the tween statement

There must be a reference to tree root somewhere. And if there isn't, well, you are supposed to pass the object as one of arguments. Calling get_tree() on that object should do (and would of course break if it's not in tree, in which case you don't need to tween it anyways).

Unity does have equivalents of AnimationPlayer.

If you mean the timeline thing, it's nowhere as good as Godot's.

Also, different start/end values is possible by changing keyframes at runtime, I did that in my game. It's just a bit unfriendly.

Yeah, my proposal is supposed to eliminate the unfriendly part.

@Zylann
Copy link
Contributor

Zylann commented Mar 3, 2019

If you mean the timeline thing, it's nowhere as good as Godot's.

I'm talking about Animation. It can animate properties and call functions just like Godot (and doesn't leave them modified if you forget to reset to beginning). Mecanim and Timeline are other features building on top of it, yet they have the same issues as Godot when it comes to procedural animation like Tween does.

I wonder if there is a way to sketch an API like this using static GDScript wrapper functions? Although the concept of sequences would have to be introduced.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 3, 2019

I wonder if there is a way to sketch an API like this using static GDScript wrapper functions? Although the concept of sequences would have to be introduced.

I'm thinking about doing this and I even have idea how, including sequences. Also, DOTween is open-source, so we could look how they did it too.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 3, 2019

Wow, that was easy:
TweeningUtils.zip

Here's the effect:
https://gfycat.com/EnviousSlimyBlackfootedferret

And here is the code:

Tweening.sequence(self
).append(Tweening.tween_to($icon, "position", Vector2(400, 400), 2).set_transition(Tween.TRANS_CUBIC)
).append(Tweening.tween_to($icon, "position", Vector2(800, 200), 2)
).append(Tweening.tween_to($icon, "modulate", Color.red, 1)
).append(Tweening.tween_to($icon, "rotation_degrees", 360, 1.5).set_delay(1)
)

Tweens are created and destroyed automatically, there's no need for any singleton. Everything is handled by single class in one script. I used get/set_meta to store Tweener objects, so they are not freed. The only problems are that you need to pass SceneTree to sequence somehow (that's why Tweening.senquence requires a Node argument) and that Godot supports method chaining only with awkward brackets at the beginning of the line. But well, still better than this:

var tween = Tween.new()
tween.interpolate_property($icon, "position", $icon.position, Vector2(400, 400), 2, Tween.TRANS_CUBIC, Tween.EASE_IN_OUT)
tween.start()
yield(tween, "tween_completed")
tween.interpolate_property($icon, "position", Vector2(400, 400), Vector2(800, 200), 2, Tween.TRANS_LINEAR, Tween.EASE_IN_OUT)
tween.start()
yield(tween, "tween_completed")
tween.interpolate_property($icon, "modulate", Color.white, Color.red, 1, Tween.TRANS_LINEAR, Tween.EASE_IN_OUT)
tween.start()
yield(tween, "tween_completed")
tween.interpolate_property($icon, "rotation_degrees", 0, 360, 1.5, Tween.TRANS_LINEAR, Tween.EASE_IN_OUT, 1)
get_tree().get_root().add_child(tween)
tween.start()
yield(tween, "tween_completed")

which I believe would be the current equivalent.

It's easy to extend it to support intervals in sequence (alternative to delays), looping, callbacks etc. The point of this issue is that this functionality is how Tweens should work, as they are close to useless without such wrappers due to unwieldiness. On the other hand, the 5-line code I used in the example could be arguably easier to write than using AnimationPlayer, so it's nice for quick animations.

@Valeryn4
Copy link
Contributor

Valeryn4 commented Mar 4, 2019

One aspect of this is that the scene tree is not a singleton AFAIK, so contrary to DOTween you'd need to pass the tree somehow as argument of the tween statement (while in Unity, new objects are directly created in the scene), as long as tweens are nodes.

in the "cocos2d-x" is also a tree. But there the tweens (Actions) work fine. MoveTo, MoveBy, ... (Interval Action)
Why use the twin for complex behavior, for this there is animation!?

@rxlecky
Copy link
Contributor

rxlecky commented Apr 13, 2019

I agree that Tween is currently quite user unfriendly and I personally often opt for AnimationPlayer even for simple animations due to this. A couple of thoughts on your proposal:

  1. I like the idea of supporting tween sequences that simplify declaring chain of tween actions.

  2. The syntax you proposed is unlike anything else in GDScript and I think for consistency sake, it should be reworked. I like the fact that it's brief, but I can see this being confusing for new users. I'd honestly prefer more verbose syntax to confusing syntax. My objection is also that this just feels like abusing the language constructs for purpose they was never designed for - in this case, the dot operator for chaining function calls to simulate declarative code.

My proposal to address this issue:

Add a TweenAction class to represent a single action animated by tween. TweenAction will have overloaded constructor with the following signatures:

  • TweenAction(Object object, String target, Variant initial_val, Variant final_val, float duration, TweenAction.TargetType target_type = PROPERTY))
  • TweenAction( Object object, String target, Object other, String other_target, Variant other_val, float duration, TweenAction.TargetType target_type = PROPERTY, TweenAction.ReferenceType reference_type = FINAL)
  • TweenAction( Object object, float duration, String callback, Variant arg1=null, Variant arg2=null, Variant arg3=null, Variant arg4=null, Variant arg5=null, TweenAction.CallbackType callback_type = CALL)

Enums used in the constructors are defined as follows:

enum TargetType {
    PROPERTY
    METHOD
}

enum ReferenceType {
    INITIAL,
    FINAL
}

enum CallbackType {
    CALL,
    DEFERRED
}

Constructors correspond to the methods that are currently present in Tween. First constructor corresponds to interpolate_property and interpolate_method, second one to targeting_property, targeting_method, follow_property and follow_method and third one to interpolate_callback and interpolate_deferred_callback. The idea is that using a constructor, one can make the most common type of action using as little characters as possible. In the same spirit, there will be following shortcuts:

  • TransitionType defaults to TRANS_LINEAR
  • EaseType defaults to EASE_IN_OUT
  • Passing null in the constructor in place of either initial_val, final_var or other_val would be shortcut for the current value of the target if TargetType is PROPERTY

In addition to these constructors, there will be functions for setting the additional tweening parameters. These will be:

  • set_transition(TransitionType trans_type)
  • set_ease(EaseType ease_type)
  • set_delay(float delay)
  • set_sequence_timing(SequenceTiming timing)
enum SequenceTiming {
    SERIES,
    PARALLEL
}

Sequence timing is a new proposed feature. It will be used for controlling behaviour of action sequences.
If set to SERIES (default), the TweenAction will be played after the previous action finished. If set to PARALLEL, it will start playing together with previous action. This will also work with delay parameter which can be used to delay how long after the previous action started, the following one shall start playing.

As for the Tween class, all the follow_*, interpolate_* and targeting_* methods would be removed since all the functionality for defining a type of action would be offloaded to TweenAction. Tween itself would only contain logic for playing, stopping and other auxiliary functions that it currently has. The play method would be overloaded with following signatures:

  • play(TweenAction action)
  • `play(Array actions)

The second overload will be used for already mentioned action sequences. It will simply accept array of TweenActions that will be played, respecting the delay and sequence_timing parameters of each element.

Translating your example to proposed API:

var tween = Tween.new()
var a1 = TweenAction.new(self, $icon, "position", null, Vector2(400, 400), 2)
a1.set_transition(Tween.TRANS_CUBIC)
var a2 = TweenAction.new(self, $icon, "position", null, Vector2(800, 200), 2)
var a3 = TweenAction.new(self, $icon, "modulate", null, Color.red, 1)
var a4 = TweenAction.new(self, $icon, "rotation_degrees", null, 360, 1.5)
a4.set_delay(1)

get_tree().get_root().add_child(tween)
tween.start([a1, a2, a3, a4])

In my opinion, this is cleaner, more typical GDScript syntax. Let me know what you think of this.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 14, 2019

I don't like how you create a variable for each TweenAction and then pass them in an array. They are meant to be played in sequence. What if you have a sequence of 30 actions? Create 30 variables?

I guess you could just make an array at the beginning and append each action as you create them. But it's just... weird to do. And if you decide to make e.g. TweenActions store previous action somewhere and then start only the first one, you either end up with double amount of code or necessity to create a variable for action (again, unfeasible for long chains).

Yes, my code is taken straight from Unity and only translated to work with GDScript. But the idea is to make the process as simple as possible, with the Tween system doing automatically as much as possible. I'm just against any unnecessary verbosity of the code, even if it's against some conventions >.>

EDIT:
Back to my example, I realized that if my functionality for Tweens was implemented, instead of using static methods to create them, we could utilize SceneTree, like Timers do. So the code would become
get_tree().create_tween_sequence().append(etc.)

@rxlecky
Copy link
Contributor

rxlecky commented Apr 14, 2019

If you dislike the creating variables part then another option would be to create something like TweenSequence class which would have functions that directly create and append TweenAction at the end of the sequence and Tween::play(TweenSequence sequence) overload would be added. However, I think this would merely be a wrapper around array of TweenActions with a few utility functions; not really worth creating and maintaining.

Yes, I get that you just tried to get the idea across. And I agree that unnecessary verbosity is just that... Unnecessary. However, I disagree that shorter syntax should be preferred at the expense of readability or maintainability.

Either way, I don't think there's any point in arguing over syntax if we don't know whether reworking Tween would be desired in the first place. Maybe @reduz could share his thought?

@LikeLakers2
Copy link
Contributor

LikeLakers2 commented Apr 14, 2019

I personally disagree, I don't think the Tween workflow is really something that needs changing. That said, I can sort of(?) see an argument for making them more straightforward. As a suggestion, perhaps we can take these four lines:

var tween = Tween.new()
tween.interpolate_property(node, property, initial_value, final_value, duration, transition_type, ease_type, delay)
add_child(tween)
tween.start()

and simplify them down to something like this:

tween(node, property, initial_value, final_value, duration, transition_type, ease_type, delay)
# Side note: transition_type and ease_type should really have defaults!

This would, in theory, perform all the same steps as the four lines of code above, but with a single function call. This function would probably live either in @GDScript or Node.

@rxlecky
Copy link
Contributor

rxlecky commented Apr 14, 2019

This only seems to take care of interpolating property, though. Tween can, however, also interpolate methods, target methods and properties, follow methods and properties and trigger deferred and non-deferred callbacks.

So, my suggestion seeks to address two points:

  1. Make creating tweens more intuitive - I believe the current way of creating tweens is not as intuitive as it could be. Opening documentation for Tween, you find 8 functions that create different actions, all of them accepting either 8 or 9 parameters. This can be really overwhelming when you encounter it at first, namely if you are not an experienced programmer. In addition to this, behaviour of targeting_* and follow_* functions is not very clearly documented, resulting in even more confusion.

    Since one of the Godot's highlighted features is animating/tweening (with a tagline "Animate Everything"), I really think the animation workflow should be as straightforward as possible. There is indeed a reason why there are so many parameters; animating things can be done in so many ways and users should have the power to make the animation just as they need it. However, there should be a way to create the most common type of tween actions in as little code as possible, allowing further customizations if necessary with a bit more verbose code.

  2. Make creating tween sequences easier - This is where I agree with @KoBeWi. I really think that to create a chain of animations you have to write an unnecessarily verbose code, sprinkled with yield and tween.start() statements after every action. This is what my proposition tries to eliminate too. One could argue that if a complex sequence of animation needs to be played, one should opt for AnimationPlayer instead. I don't have a strong opinon on that, but I still don't think that if a user chooses to take the path of code, they should be facing an excessively verbose interface.

That said, I understand that reworking Tween interface - no matter whether the solution I proposed would be used or anything else that might come up - would be a risky path to take. It would definitely have to be made so the backwards compatibility is preserved, at least for one full major version. However, I don't dare to estimate whether the gains would outweight the costs. Still, I believe that the current Tween workflow should be improved and that there's some low-hanging fruit that would already make working with the current Tween interface easier and require only little changes in code. The changes would be:

  • Improve the docs - Make the Tween class documentation more thorough, explaining differences between the various tween actions. Possibly create a tutorial, showing all of them in action (I wouldn't mind working on this).
  • Add defaults to trans_type and ease_type - the arguments should default to the most commonly used types. This would be, in my opinion, TRANS_LINEAR and EASE_IN_OUT, but this would be up for discussion.
  • Make null a shortcut for "current value" - this one is the most debatable out of the three. However, animating from the current property value to some final value is arguably the most common type of tween actions, so I believe there should be an easy way to specify that I want to simply use the current value. Passing null seems to be the most reasonable way to achieve this, since aniamting from or to null is not valid in any case, if talking about animating properties.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 25, 2019

I randomly thought about Tweens and looking at the example code again, do we even need Tweens to be nodes? There's no editor plugin related to Tweens and the few inspector properties could be as well controlled by code. Also, all the behavior is controlled by code anyways. Maybe we could rewrite the Tweens into independent objects similar to SceneTreeTimers?

@volzhs
Copy link
Contributor

volzhs commented Aug 25, 2019

it can be paused when scene is paused because it is a node inside tree.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 11, 2019

Side note: transition_type and ease_type should really have defaults!

Setting the defaults for TransitionType (TRANS_LINEAR) and EaseType (EASE_IN_OUT perhaps) would definitely be a step forward imo.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 12, 2019

@LikeLakers2 I've kinda managed to implement this as a GDScript method in https://github.com/Xrayez/godot/commit/341787415057bed912c85dc26becec685dbc4bf3 but I've stumbled upon node workarounds there having to make deferred calls.

@Zylann One aspect of this is that the scene tree is not a singleton AFAIK

You can access the scene tree via C++ as SceneTree::get_singleton(), but not in GDScript. I've taken advantage of this by adding a tween like this:

Tween *tween = memnew(Tween);
SceneTree::get_singleton()->get_root()->call_deferred("add_child", tween);
tween->interpolate_property(obj, property, initial_val, final_val, duration, trans_type, ease_type, delay);
tween->call_deferred("start");

It looks like this when used via GDScript:

# Fade away Godot sprite, free upon fading completely
animate($Godot, "modulate:a", null, 0.0, 5.0).connect("tween_all_completed", $Godot, "queue_free")

@rxlecky Make null a shortcut for "current value"

It seems like it's the current functionality, not sure if this was added recently.

So these "hacks" suggest that it could rather be made closer to the scene tree indeed. I gravitate towards similar to SceneTreeTimer implementation as suggested by @KoBeWi (SceneTreeTween)?

@volzhs it can be paused when scene is paused because it is a node inside tree.

According to API one can achieve similar pausing functionality:

godot-scene-tree-timer

So similar could be achieved for a tween I believe. The usage would look like so:

var tween = get_tree().create_tween($Godot, "modulate:a", null, 0.0, 5.0)
tween.connect("finished", $Godot, "queue_free")

It would also ensure that the tween is started automatically and freed upon finishing.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 12, 2019

I'm thinking about making a proposal for overhauling Tweens into something like I did here: godot-extended-libraries/godot-next#50

Methods from my TweenSequence could become Tween methods and instead of relying on underlying Tween, the Tween would animate on itself and you'd lose access to current methods and only use them via the new interface. Every problem from Tween being changed to Reference can be resolved.

@TicklesTheBrain
Copy link

TicklesTheBrain commented Jan 8, 2020

I'm thinking about making a proposal for overhauling Tweens into something like I did here: godot-extended-libraries/godot-next#50

Methods from my TweenSequence could become Tween methods and instead of relying on underlying Tween, the Tween would animate on itself and you'd lose access to current methods and only use them via the new interface. Every problem from Tween being changed to Reference can be resolved.

This sounds like a good idea, and I like your solution better, but there already is a Tween Sequencing rework Pull Request that has been put off to the next version twice already #20934 I guess a decision is needed from the leadershop on the direction of these Tween reworks.

Just one suggestion - there currently isn't any way to get a list of Tweens from a Tween class. I think it would be useful to have a way to get an array like that. Especially, if we will be able to sequence them.

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 23, 2020

Closing this in favor of godotengine/godot-proposals#514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants