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

New Tweens should run parallel by default #3478

Closed
nathanfranke opened this issue Oct 29, 2021 · 12 comments
Closed

New Tweens should run parallel by default #3478

nathanfranke opened this issue Oct 29, 2021 · 12 comments

Comments

@nathanfranke
Copy link
Contributor

nathanfranke commented Oct 29, 2021

See godotengine/godot#41794 (PR) and #514 (Proposal)

Describe the project you are working on

None, waiting for Godot 4

Describe the problem or limitation you are having in your project

The new tween system is amazing, but I dislike how steps are sequential by default. It would be way easier and cleaner to use Godot's built-in co-routine system. Here are the problems I see with the new system:

Debugging and normal program execution:

When you place a break-point, you expect it to fire when that line is about to run. However, with sequential steps, all code is executed in the same frame, so break-points and prints are basically useless.

Normal usage of Tweens:

Think about how you usually use Tweens. My first thought is making transitions for canvas items. For example, making a UI menu fade in/out, making a character flash when hit, interpolating the camera to a different position, etc. All of these transitions are single operations and don't require extra steps. Usually, if someone wants to have complex animations, they would just use AnimationPlayer.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The sequencing should be done almost the same as the current (3.x) tween, with additional features for clean code.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Taking this example:
image

The code to represent this with my proposal would be:

var tween := create_tween()

tween.tween_property(...) # 1
await tween.all_completed
tween.tween_property(...) # 2
tween.tween_property(...) # 3
await tween.all_completed
tween.tween_property(...) # 4
await tween.tween_property(...) # 5
tween.tween_property(...) # 6

Note that the last await awaits the Tweener object returned from tween_*. I am not sure if this will be directly awaited or if you would need to do await tween.tween_property(...).completed

If this enhancement will not be used often, can it be worked around with a few lines of script?

This enhancement will be used often, and it will also clean up code. For example, you wouldn't need to put .parallel() before every tween operation.

Is there a reason why this should be core and not an add-on in the asset library?

Tweens are part of core

@Calinou Calinou added this to the 4.0 milestone Oct 29, 2021
@Calinou Calinou changed the title [4.0] Tween should run parallel by default New Tweens should run parallel by default Oct 29, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Oct 29, 2021

From my experience, chained usage is much more common. E.g. character flashing you mentioned is done with a sequence (white->normal->white->normal etc.).

Usually, if someone wants to animate multiple steps, they would just use AnimationPlayer.

lol
People didn't use Tweens for multiple steps, because the old system was extremely unusable. In fact, sequenced Tweens was the most requested enhancement to the system. Complex fire-and-forget animations without needing AnimationPlayer are super convenient thing.

For example, you wouldn't need to put .parallel() before every tween operation.

Just do create_tween().set_parallel()

I thought that maybe we could have global tween settings, i.e. you set them once and it applies to all Tweens created after. Could be used for default eases, parallel etc.

@nathanfranke
Copy link
Contributor Author

I don't think this is compelling enough. Why should each tween interpolation be delayed when they all execute at the same time?

From my experience, chained usage is much more common.

I heavily disagree, sure there are examples where you can chain but the majority of transitions are going to run in parallel. FYI when I said character flash I meant a single flash.

In fact, sequenced Tweens was the most requested enhancement to the system.

Assuming this is true, that still doesn't mean sequencing is a better default than parallel.

I thought that maybe we could have global tween settings

I really don't think this is a good idea. This would cause a lot of issues with helping people and finding inconsistencies.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 30, 2021

Here's a bunch of examples from my project (using a 3.x plugin that mimics the new API):

var seq := TweenSequence.new().set_loops()
if shifted:
	position.y += 180
	seq.append_advance(self, "position:y", -180, 0.5).set_trans(Tween.TRANS_SINE)
	seq.append_interval(0.5)
	seq.append_advance(self, "position:y", 180, 0.5).set_trans(Tween.TRANS_SINE)
	seq.append_interval(0.5)
else:
	seq.append_advance(self, "position:y", 180, 0.5).set_trans(Tween.TRANS_SINE)
	seq.append_interval(0.5)
	seq.append_advance(self, "position:y", -180, 0.5).set_trans(Tween.TRANS_SINE)
	seq.append_interval(0.5)
tut_seq = TweenSequence.new()
tut_seq.append(tut1, "modulate:a", 0, 1)
tut_seq.append(tut2, "modulate:a", 1, 1)
var seq := TweenSequence.new()
seq.append(Util.game, "darkness", 2, 1)
seq.append_callback(map, "add_child", [wall])
seq.append_callback(map, "add_child", [boss])
seq.append_callback(boss, "nozaryczno")
seq.append(Util.game, "darkness", 1, 1)
seq.append_callback(self, "do_boss")
tween = TweenSequence.new().set_loops()
tween.append(ambient, "modulate:a", 0, 1).set_trans(Tween.TRANS_SINE).set_ease(Tween.EASE_IN_OUT)
tween.append(ambient, "modulate:a", 1, 1).set_trans(Tween.TRANS_SINE).set_ease(Tween.EASE_IN_OUT)
var seq := TweenSequence.new()
seq.append(ambient, "modulate:a", 1, 3).set_trans(Tween.TRANS_SINE).set_ease(Tween.EASE_IN_OUT)
seq.append_callback(self, "end")
var seq := TweenSequence.new()
seq.append(crystal, "position", crystal_target.position, 2)
seq.parallel().append(platform, "position:x", platform.position.x + 60, 1)
seq.append_callback(boss, "start")
var seq := TweenSequence.new()
seq.append_callback(Util, "play_sample", ["res://Samples/Knock.wav"])
seq.append_interval(0.9)
seq.append_callback(Util, "play_sample", ["res://Samples/Knock.wav"])
seq.append_interval(0.3)
seq.append_callback(Util, "play_sample", ["res://Samples/Knock.wav"])
seq.append_interval(0.3)
seq.append_callback(Util, "play_sample", ["res://Samples/Knock.wav"])
seq.append_interval(0.5)
seq.append_callback(door, "set", ["locked", false])
seq.append_callback(door, "open")
seq.append_callback(player, "cancel_event")

Not a single parallel (almost). And in most cases it's not something that can be done with AnimationPlayer.

btw your example code is bogus, it won't even work. It should be like this:

var tween := create_tween()

tween.tween_property(...) # 1
tween.tween_property(...) # 2
tween.parallel().tween_property(...) # 3
tween.tween_property(...) # 4
tween.parallel().tween_property(...).finished.connect(func():  create_tween().tween_property(...)) # 5, 6

You don't need to use await anymore, because animations happen one after another. I'd suggest to learn API first before making suggestions 🙃
Also if Tweens were parallel by default, you'd need to use chain() before #2 and #4, so the code wouldn't even change.

FYI when I said character flash I meant a single flash.

Then you only need one interpolation, so parallel or not is irrelevant.

Think about how you usually use Tweens. My first thought is making transitions for canvas items.

Tweens are more than UI toys. They are advanced tools that can be used for complex animations and scripting game logic/events. And even for UI, there are lots of effects that need sequenced animations. Again, I think you are misunderstanding how the system works.

I really don't think this is a good idea. This would cause a lot of issues with helping people and finding inconsistencies.

You could say the same about any global, e.g. autoloads. I only suggested this, because global settings exist in DOTween (the library that inspired rework). They could be easily added, but so far there was no much demand.

@dalexeev
Copy link
Member

await is part of GDScript and Tween is part of the core where all functions are synchronous (signals are used for asynchronous things). Therefore, we cannot fully integrate Tween with the asynchronous capabilities of GDScript. And it's sad.

You can work around it like this:

Tween and Tweener both already have finished signal. You can:

var tween1 = create_tween()
var tween2 = create_tween()

tween1.tween_property(...) # 1

tween1.tween_property(...) # 2
tween1.parallel().tween_property(...) # 3

tween1.tween_property(...) # 4
await tween1.parallel().tween_property(...).finished # 5

tween2.tween_property(...) # 6

or

var tween1 = create_tween()
var tween2 = create_tween()

tween1.tween_property(...) # 1

tween1.tween_property(...) # 2
tween1.parallel().tween_property(...) # 3

await tween1.finished

tween1.tween_property(...) # 4

tween2.tween_property(...) # 5
tween2.tween_property(...) # 6

But having to use await may indicate a problem. @KoBeWi Is it possible to add support for a synchronous option like

var tween = create_tween()

tween.tween_property(...) # 1

tween.tween_property(...) # 2
tween.parallel().tween_property(...) # 3

tween.tween_property(...) # 4
tween.parallel().tween_property(...)\
        .chain().tween_property(...) # 5, 6

?

But it seems to me that such cases are rare, I'm not sure if it's worth solving them.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 30, 2021

But having to use await may indicate a problem.

Not really. Tweens can't be changed to what you suggest. Tween is basically a container for Tweeners. Or more like container of containers. Each animation step is one or more Tweeners that run in parallel and when each of them is finished another step starts. So having asynchronous steps would require multiple "sub-Tweens" running in parallel, which is the same as multiple Tweens right now.

But it seems to me that such cases are rare, I'm not sure if it's worth solving them.

It's not IMO. Right now you have at least 3 options for asynchronous steps.
Your await approach:

var tween1 = create_tween()

tween1.tween_property(...) # 1

tween1.tween_property(...) # 2
tween1.parallel().tween_property(...) # 3

tween1.tween_property(...) # 4
await tween1.parallel().tween_property(...).finished # 5

var tween2 = create_tween()
tween2.tween_property(...) # 6

Callable approach I gave example comment above:

var tween := create_tween()

tween.tween_property(...) # 1
tween.tween_property(...) # 2
tween.parallel().tween_property(...) # 3
tween.tween_property(...) # 4
tween.parallel().tween_property(...).finished.connect(func():  create_tween().tween_property(...)) # 5, 6

And a mix of both:

var tween1 = create_tween()
var tween2 = create_tween()
tween2.tween_property(...) # 6
tween2.stop()

tween1.tween_property(...) # 1

tween1.tween_property(...) # 2
tween1.parallel().tween_property(...) # 3

tween1.tween_property(...) # 4
tween1.parallel().tween_property(...).finished.connect(tween2.start) # 5 + 6

Each of them is perfectly fine. This

tween.parallel().tween_property(...)\
        .chain().tween_property(...) # 5, 6

is impossible without significant system rework, which would also make it more complex than needed. I see no reason to change it.

@nathanfranke
Copy link
Contributor Author

@dalexeev purely meta, but I wouldn't use images for quotes. I could barely read it on mobile, but more importantly it's not accessible to everyone.

@samdze
Copy link

samdze commented Nov 5, 2021

I don't know yet exactly how the new Tween system works, but I saw this example above:

var tween := create_tween()

tween.tween_property(...) # 1
# Here, 2 and 3 would run in parallel.
tween.tween_property(...) # 2
tween.parallel().tween_property(...) # 3

I'm not a fan of this way of declaring parallel tweeners.
The parallel tweeners are two, but here we start only one of them with parallel() to indicate that it should run together with the previous one.
To me, this is only an ostensibly simpler approach, and in reality it will mess with users wanting to create more complex tweens.

I think a similar solution has already been evaluated before and maybe discarded, but I would definitely go in this direction instead:

# By default, create_tween creates a sequential group of tweeners.
var tween := create_tween()

tween.tween_property(...) # 1
# A new parallel group of tweeners is added to the sequence.
var parallel1 = tween.parallel()
# Now each tweener added to parallel1 is executed in parallel.
parallel1.tween_property(...) # 2
parallel1.tween_property(...) # 3

# When parallel1 is finished, parallel2 runs.
var parallel2 = tween.parallel()
parallel2.tween_property(...) # 4
# Adding a sequential group of tweeners to the parallel group.
var inner_seq = parallel2.sequence()
inner_seq.tween_property(...) # 5
inner_seq.tween_property(...) # 6

# Chaining like it was proposed above would be ok.
# It's also more readable thanks to indentation.
# The chain() method could also be called end() or something similar to indicate the end of the Tweener configuration.
# I'm omitting the backslashes just to better display comments. (otherwise they get highlighted in a strange way)
var tween := create_tween()
tween.tween_property(...) # 1
tween.parallel()
    .tween_property(...).set_trans(...).chain() # 2 could also be end()
    .tween_property(...) # 3
tween.parallel()
    .tween_property(...).chain() # 4
    .sequence()
        .tween_property(...).chain() # 5
        .tween_property(...) # 6

# Simple PropertyTweeners, MethodTweeners, etc. can't directly add other Tweeners to themselves.
# So, they can automatically add them to their parent Tweener.
# This way, the chain() or end() method can be omitted on simple Tweeners, and it's now needed in group (sequence or parallel) Tweeners.
var tween := create_tween()
tween.tween_property(...) # 1
tween.parallel()
    .tween_property(...) # 2
    .tween_property(...) # 3
tween.parallel()
    .tween_property(...).set_trans(...) # 4
    .sequence() # When calling sequence(), the PropertyTweener above knows that it should be called on the parent Tweener, not on itself.
        .tween_property(...) # 5
        .tween_property(...) # 6
    .end() # a method is necessary to end groups of tweeners in this case.
    .tween_property(...) # Other tweeners...

It's similar to or almost the same as the chain approach and it may seem more complex but it's the most flexible, clear and consistent solution and lets the user create any kind of complex tweeners nesting.

This way, the order of execution of the tweeners is completely declarative, fixed and independant from the declaration of other tweeners.
Groups of tweeners could also be passed around to configure individual steps of the tween independently.

I see that this would require quite a system rework.
I just wanted to let you know that as an advanced user and creator of several tweening libraries, I always found this approach the best.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 5, 2021

The indentation syntax looks cool, but Godot doesn't allow calling methods this way.

As for your proposed system, indeed it would require another rework (not as drastic, but still). However it should be possible to write a system like this using GDScript, by wrapping the Tweens into a custom API.

@samdze
Copy link

samdze commented Nov 5, 2021

You can call methods like that using backslashes, I just omitted them because comments written after them get highlighted weirdly by github.

Having this proposed system natively and out of the box would be optimal in my opinion.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 5, 2021

I'd wait for more people to try the new system. If there is demand, we could still do some tweaks until beta. In the meantime you can open a new proposal.

@samdze
Copy link

samdze commented Nov 8, 2021

@nathanfranke
Copy link
Contributor Author

Closing due to lack of community support. I'm really happy with the syntax that #3515 proposes, though!

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

5 participants