-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add Tween.tween_subtween
method for nesting tweens within each other
#98660
base: master
Are you sure you want to change the base?
Conversation
I was hoping to implement The way that var tw := create_tween()
tw.tween_property(self, "position:x", 100, 1.0)
tw.set_trans(Tween.TRANS_SINE) # no effect! The object would still move with linear motion, as if the
I might go and submit a separate PR to make this more clear. Anyways, because of this, the following doesn't work, at least in the way I would intuitively expect: Ref<SubtweenTweener> SubtweenTweener::set_trans(Tween::TransitionType p_trans) {
subtween->set_trans(p_trans);
return this;
} With this, if you start adding All this to say, while I think it would be nice to have support for methods like As such, for now I think I'll just go ahead with only having the |
I've created a small project with several testing scenes, so people can see how it behaves and what the corresponding GDScript code is like: tween-subtween.zip I'll also add this to the original post so it's easily accessible. With this complete, I think the PR is ready to be more throughly looked at, so I'll open it for review!
|
One alternative I can think of is allowing to use |
tweens.push_back(tween); | ||
return tween; | ||
} | ||
|
||
bool SceneTree::remove_tween(Ref<Tween> &p_tween) { | ||
_THREAD_SAFE_METHOD_ | ||
return tweens.erase(p_tween); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweens
is a linked List, so erasing can be costly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! That may be a good reason to allow users to manually create Tween objects, and then add them to the SceneTree (or not) as necessary, rather than only allowing them to be created via create_tween
.
doc/classes/Tween.xml
Outdated
@@ -207,6 +207,7 @@ | |||
<description> | |||
Pauses the tweening. The animation can be resumed by using [method play]. | |||
[b]Note:[/b] If a Tween is paused and not bound to any node, it will exist indefinitely until manually started or invalidated. If you lose a reference to such Tween, you can retrieve it using [method SceneTree.get_processed_tweens]. | |||
[b]Note:[/b] When a subtween (set via [method tween_subtween]) is paused, it will hold up execution of the parent [Tween] on that step. Make sure to either resume the subtween with [method play] or end it with [method kill] so that the rest of the parent [Tween] can finish executing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should "pollute" method description like this or just add some more notes to tween_subtween()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm against this, too. It makes the whole method look more complex, even though this "mechanic" only applies to one, unique type of tweener (and the hardest to wrap your head around by far).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I'll make a note to remove it shortly, thanks for the feedback :)
scene/animation/tween.h
Outdated
class SubtweenTweener : public Tweener { | ||
GDCLASS(SubtweenTweener, Tweener); | ||
|
||
friend class Tween; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because SubtweenTweener::subtween
is a private member, but the Tween::tween_subtween
method needs to be able to access it.
I see a few possibilities for how to handle this:
- Keep
friend class Tween
andsubtween
asprivate
- Remove
friend class Tween
, makesubtween
bepublic
- Remove
friend class Tween
, keepsubtween
asprivate
but add aSubtweenTweener::get_subtween
method so that the subtween can be retrieved from theSubtweenTweener
, but the variable itself cannot be modified from outside the class instance
Which of these do you think would be best?
The functionality works correctly. The implementation could be improved with the change I suggested. The subtween looks neat and allows for some nice sequences, but tbh I'd have to think hard to come up with some real example where they are useful, let alone necessary. You made a nice test project, but these are just abstract examples, not something you'd use in a game. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback and suggestions! 😄 I've committed the direct suggestions, and will have other changes up shortly.
doc/classes/Tween.xml
Outdated
@@ -207,6 +207,7 @@ | |||
<description> | |||
Pauses the tweening. The animation can be resumed by using [method play]. | |||
[b]Note:[/b] If a Tween is paused and not bound to any node, it will exist indefinitely until manually started or invalidated. If you lose a reference to such Tween, you can retrieve it using [method SceneTree.get_processed_tweens]. | |||
[b]Note:[/b] When a subtween (set via [method tween_subtween]) is paused, it will hold up execution of the parent [Tween] on that step. Make sure to either resume the subtween with [method play] or end it with [method kill] so that the rest of the parent [Tween] can finish executing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I'll make a note to remove it shortly, thanks for the feedback :)
scene/animation/tween.h
Outdated
class SubtweenTweener : public Tweener { | ||
GDCLASS(SubtweenTweener, Tweener); | ||
|
||
friend class Tween; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because SubtweenTweener::subtween
is a private member, but the Tween::tween_subtween
method needs to be able to access it.
I see a few possibilities for how to handle this:
- Keep
friend class Tween
andsubtween
asprivate
- Remove
friend class Tween
, makesubtween
bepublic
- Remove
friend class Tween
, keepsubtween
asprivate
but add aSubtweenTweener::get_subtween
method so that the subtween can be retrieved from theSubtweenTweener
, but the variable itself cannot be modified from outside the class instance
Which of these do you think would be best?
tweens.push_back(tween); | ||
return tween; | ||
} | ||
|
||
bool SceneTree::remove_tween(Ref<Tween> &p_tween) { | ||
_THREAD_SAFE_METHOD_ | ||
return tweens.erase(p_tween); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! That may be a good reason to allow users to manually create Tween objects, and then add them to the SceneTree (or not) as necessary, rather than only allowing them to be created via create_tween
.
Allowing tweens to be created without
I definitely get what you mean in terms of the test project - for the most part they were just intended to test functionality, making sure that specific tween settings weren't causing the feature to break. It's been quite a while, but I remember that while working in Unity, DOTween's nestability allowed me to compose complex UI animation sequences like these. For example, the room names that slide in on the left at the very beginning of the video could each have a method that builds just their slide-in animation, then I could join/append all of those together into a single sequence, and once that sequence is complete, then play the focus animation (becoming more opaque, growing) for the active element. In GDScript pseudocode, that could look something like this: func tween_in_rooms() -> Tween:
var tw = create_tween()
# Rooms 2 and 3 come in parallel but with slight delay, to get
# an "overlapping animation" effect instead of being directly
# sequential.
tw.tween_subtween(room1.tween_in())
tw.parallel().tween_subtween(room2.tween_in()).set_delay(0.2)
tw.parallel().tween_subtween(room3.tween_in()).set_delay(0.4)
# Once all rooms have come in, make room 1 focus.
tw.tween_subtween(room1.tween_focus())
# By returning this Tween from this method, we could make it
# part of an even bigger overall sequence if we wanted to.
return tw The results screen later on in the video is much more complex, but can be broken down into "play the tween defined for object 1, then play the tween defined for object 2, then play..." etc., and each of those animations is much easier to define in a sequence. They can also then be tested independently of each other, or reused in other locations if need be. |
That's not really needed.
The same can be achieved with multiple Tweens played with a delay and waiting for the last one to finish. func tween_in_rooms() -> Tween:
var last_room_tween: Tween
var delay := 0.0
for room in rooms:
last_room_tween = room.tween_in(delay) # Delay is applied via tween_interval() at the beginning.
delay += 0.2
await last_room_tween.finished
return rooms[0].tween_focus()) The method is async though, if you really need to return the Tween for some reason. |
My hope is that the tween objects could be produced by methods in such a way that the user could choose to make them a "top-level tween" (run directly by the scene tree, like the result of doing # Animate a single button tween standalone.
# The `in_tween` method creates its tween via `Tween.new` so that it
# isn't added to the tree by default, because it's intended to be usable
# as a subtween.
var button_in_tween: Tween = my_button.in_tween()
get_tree().queue_tween(button_in_tween)
# The same tween, but now it's just one step of a larger sequence.
var button_in_tween_2: Tween = my_button.in_tween()
some_larger_sequence.tween_subtween(button_in_tween_2) It could still be accomplished without adding a var button_in_tween: Tween = my_button.in_tween()
var s: Tween = create_tween()
s.tween_subtween(button_in_tween) but it strikes me as a bit superfluous to create another tween tied to the scene tree, to only serve as a "container" for the intended tween.
That's true, and I suppose for the results screen one could also probably string along a number of |
Eh, for now I'd make orphan Tweens return an error if they aren't started in the same frame and potentially improve it in the future. The main problem with creating Tweens outside Tree is that users will likely expect them to run normally, so some error/warning is a necessity.
It's actually impossible. The awaited call can be "canceled" only by getting rid of the calling object or making it wait forever. So if you can't use await, that would be when the current system falls short. The AwaitTweener doesn't have this problem, but it's not merged yet, so it's only speculative. |
I agree with this! What I meant to propose was that when you used a factory method to create a Tween, you had the option of either adding it to a SceneTree that frame, or making it part of a sequence with It does make me wonder, though: if a Tween is created without a SceneTree attached to it, how would it know when the end of the frame is?
Do you know how common it is for people to be working with multiple SceneTrees? I know I've only ever had one to deal with, so # Queue this Tween on the main SceneTree,
# accessible via `get_tree()`.
var some_tween := Tween.new()
get_tree().queue_tween(some_tween)
# Queue a Tween to run on another SceneTree
# we have a reference to.
var some_other_tween := Tween.new()
some_other_tree.queue_tween(other_tween) If the method is on Tween itself, then the Tween instance would have to know which SceneTree to add itself to, if the method takes no arguments: # Queue this Tween on a SceneTree.
# How does it know what SceneTree to use?
var some_tween := Tween.new()
some_tween.add_to_tree() If the method does take a SceneTree as an argument, the "which SceneTree" problem is solved of course, but the API is unusual in my opinion - it'd be like having # Queue this Tween on a SceneTree.
# It knows which SceneTree to use because we're
# passing it as an argument, but the order is unusual.
var some_other_tween := Tween.new()
some_other_tween.add_to_tree(get_tree())
Makes sense! What I was thinking of when I said it was just "more complicated" were ways I've found to hack around the coroutine sequence, by providing early-exit points. But they have to be made on a very case-by-case basis, and maybe sometimes they are indeed just straight-up impossible to get working. 😅 |
It doesn't need to be in scene tree to wait for the signal.
There can be only one SceneTree. |
Great! I won't be able to work on this for a few days, but when I'm back, I can experiment with having the Tween constructor get the scene tree instance and waiting for the end-of-frame signal. This did make me think, though, if we may be able to stick with the This way we wouldn't need to tell users about two different ways to create Tweens – from the user end, it would seem like the same as it always was. This would, however, make it so that the execution order of Tweens isn't guaranteed to match the creation order (as far as I understand, that is currently guaranteed). I'm not sure how much of a breaking change this would be for existing projects. |
The Tweens need to be in a structure that is iteration-friendly. Not sure if HashSet is like that. |
Looks like HashSet can be iterated over, and it appears to retain its order from the tests I've done so far. HashSet<int> nums;
for (int i = 0; i < 10; i++) {
int some_rand = rand() % 100;
print_line("inserting", some_rand);
nums.insert(some_rand);
}
print_line("Nums from the hash set:");
for (int num : nums) {
print_line(num);
}
print_line("----"); I ran this twice, and got the output:
Based on how HashSet works, maybe I just didn't add enough elements, or a wide enough range, to get collisions that could cause the iteration order to not match the insertion order perfectly? I'm currently thinking it might make sense to have a HashSet tween queue that gets emptied into the linked list at the end of each frame. That way, we can quickly remove tweens from the queue before the end of the frame, if we want to reassign a tween to be a subtween. Before I get started trying to implement that, though, it'd be good to get others' thoughts. |
AFAIK HashSet does not guarantee order. It's an implementation detail, so it can change at any point. |
That makes sense! How big of a compatibility break do you think it'd be to not guarantee the precise order of tween execution? This note is on the Tween documentation page:
Given that modifying the same property from multiple tweens is discouraged behavior, IMO it wouldn't be the worst thing to update this to say "one of the Tweens created will assign the final value", and if people are running into problems with it, then the solution is for them to stop the already-discouraged behavior. I could also envision some cases where var tw1 := create_tween()
tw1.tween_interval(1.0)
tw1.tween_callback(func(): print("Tween 1"))
var tw2 := create_tween()
tw2.tween_interval(1.0)
tw2.tween_callback(func(): print("Tween 2")) If using a HashSet instead of a linked list, these would have no guarantee of printing in the same order, which I could see causing more issues... While I'd definitely prefer to keep |
Eh, I think removing from the list is fine for now. It's costly, but it won't be noticeable until someone makes a subtween while lots of Tweens are running. It can always be improved later if it's really a problem. For now remove subtween mentions from unrelated methods ( |
b6b33b5
to
78e5cb4
Compare
703a4ae
to
1720db1
Compare
1720db1
to
ee7f77d
Compare
No actual functionality yet Actual subtween functionality implemented Added documentation for Tween.tween_subtween and SubtweenTweener Implemented some additional functions `set_ease`, `set_trans`, and `set_delay` Documentation only for `set_delay` so far, since I have tested it Removed set_ease and set_trans Upon further investigation, the way they are implemented for Tween doesn't appear to work here Fixed indentation in documentation Reset subtween when parent loops Fix return type of `SubtweenTweener.set_delay` Add notes to documentation Apply suggestions from code review Co-authored-by: Tomasz Chabora <[email protected]> Apply some suggested changes - Remove excessive documentation - Add Tween constructor that takes in SceneTree - Make `SubtweenTweener::subtween` public so that `Tween` doesn't have to be a friend class Remove unneeded friend class SceneTree Remove superfluous documentation describing subtween behavior Apply suggestions from code review Co-authored-by: Tomasz Chabora <[email protected]> Apply suggestions from code review Co-authored-by: Thaddeus Crews <[email protected]>
6487323
to
1f4110e
Compare
// Remove the tween from its parent tree, if it has one. | ||
// If the user created this tween without a parent tree attached, | ||
// then this step isn't necessary. | ||
if (tweener->subtween->parent_tree != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this crash if I call tween.tween_subtween(null)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question - I never thought to try tween.tween_subtween(null)
, admittedly, but I could see that situation coming up in some use cases where the user isn't sure if some other method is going to return a Tween or not.
For this specific line, I think it'd be sufficient just to check if tweener->subtween
is null
or not before the parent_tree
check, but there may also be other parts of the code where a null subtween needs to be handled, perhaps by treating it like a no-op step in the tween sequence.
This is an implementation of godotengine/godot-proposals#11022, which adds the
tween_subtween
method to theTween
class. Using this method,Tween
objects can be nested within each other and scheduled to play at specified points.It's not fully complete yet (see the to do list below), but I wanted to get a draft posted with the core functionality so that others could give their thoughts on it. I'm especially unsure about the method used to remove a
Tween
from theSceneTree
'stweens
list once it is made into a subtween.Example usage
Godot.Tween.Subtween.Video.1.mp4
Example project
tween-subtween.zip
Inside this project are several
*_test
folders; you can open them and run the scenes inside to see demos of the method in action.To do
Tween
s from theSceneTree
is okay, or improve it if necessaryTweener
subclassesInfeasible due to the way default transitions and tween evaluation is implementedset_trans
Infeasible due to the way default transitions and tween evaluation is implementedset_ease
set_delay
May be out of scope for now, and not necessary enough (possibly best to implement onset_custom_interpolator
Tween
itself?)Tween
features like loops, play, pause, etc doesn't break when in a parentTween
Tween.tween_subtween
Tween.play
andTween.pause
that a paused subtween will result in the parent tween never moving past that stepTween.set_loops
that an infinitely-looping subtween will result in the parent tween never moving past that stepTween.set_pause_mode
andTween.set_process_mode
that they will not have an effect when the tween is a subtweentween_subtween
method for nesting Tweens godot-proposals#11022