godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

Add a `Node.create_timer` method #4908

Open mieldepoche opened 2 years ago

mieldepoche commented 2 years ago

Describe the project you are working on

prototyping with 4.0

Describe the problem or limitation you are having in your project

Whenever I need to use a SceneTreeTimer I need to type get_tree().create_timer(). It's a bit long to type (see https://github.com/godotengine/godot-proposals/issues/4886).

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

Similar to how tweens can be created with Node.create_tween, allow creating SceneTreeTimers with a Node.create_timer method.

There is currently no functionnality linked to the creation of a SceneTreeTimer in a node like there is for Tweens, but this could be a handy and similar convenience nonetheless.

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

func create_timer(time_sec: float, process_always: bool) -> SceneTreeTimer:
    if is_inside_tree():
        return get_tree().create_timer(time_sec, process_always)
    return null

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

It can be done locally but the point here is shaving get_tree(). from several scripts.

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

It can't be done in an addon.

nathanfranke commented 2 years ago

This has a lot of potential, because it could also fix some tedious errors, for example:

func _ready() -> void:
    await create_timer(1.0).timeout
    # Previously, if the node was removed by now, there would be an error printed.
    # Since the timer (should) be a child of the node, if the node is removed the timer is also removed.

Honestly, I don't even think it's possible to fix this with the current implementation of SceneTreeTimer, you'd have to add a timer node manually.

(See below) (Also, the timer probably wouldn't actually be a child node; I think that goes against the tween implementation, but it should be invalidated if the node stops existing without throwing an error.)

KoBeWi commented 2 years ago

create_tween() exists in Node because, aside from creating the Tween, it binds the Tween to a node (so it's freed/paused when node is freed/paused). What would be purpose of create_timer(), aside from being a shorthand?

Although, create_timer() could be a shorthand for SceneTree::get_singleton()->create_timer() and allow to easily create timers outside of tree. Maybe that way it could make sense 🤔

dalexeev commented 2 years ago

Node.create_tween creates a Tween bound to the node. Node.create_timer makes sense if the timer will be bound to the node, e.g. stop on pause, depending on the process_mode.

Mickeon commented 2 years ago

create_tween() exists in Node because, aside from creating the Tween, it binds the Tween to a node (so it's freed/paused when node is freed/paused). What would be purpose of create_timer(), aside from being a shorthand?

That exact purpose, same as Tweens. If SceneTreeTimer was allowed to optionally be bound to a Node, we could have both SceneTree.create_timer() and Node.create_timer().

KoBeWi commented 2 years ago

If SceneTreeTimer was allowed to optionally be bound to a Node

But this doesn't really make sense in a timer. SceneTreeTimers are supposed to just count down the time, no need to add bloat to them, when you can use Tween instead:

create_tween().tween_interval(1.0).finished.connect(something)

With Tweens you can even customize speed scale or process mode (idle/physics) easily:

create_tween().set_speed_scale(0.5).set_process_mode(Tween.TWEEN_PROCESS_PHYSICS).tween_interval(1.0).finished.connect(something)
kleonc commented 2 years ago

Although, create_timer() could be a shorthand for SceneTree::get_singleton()->create_timer() and allow to easily create timers outside of tree. Maybe that way it could make sense thinking

Note that in case you'd have own implementation of the MainLoop there would be no SceneTree singleton available. Of course check for that could be added in there but my concern is about adding a global method which works only in some specific circumstances. I'd say that's the first step to start bloating the global namespace with not-really-global methods. And this doesn't seem like a good idea in the long run.

kleonc commented 2 years ago

Because the main "issue" here is the get_tree().create_timer() being not so easy to type I wonder whether adding a readonly (getter-only) scene_tree property to the Node could be a solution (tree feels too vague, especially considering there's Tree node). Typing scene_tree.create_timer() seems way simpler (even though the character count is the same) because the proper code completion hint would probably be shown after typing at most "sc", contrary to typing "get_t" currently. And this solution wouldn't hide what's really happening in any way (timer is indeed created in the SceneTree).

dalexeev commented 2 years ago

I wonder whether adding a readonly (getter-only) scene_tree property to the Node could be a solution

Note that in case you'd have own implementation of the MainLoop there would be no SceneTree singleton available.

By the way, I wanted to ask for a long time why we cannot add a SceneTree singleton (like Input, Engine, etc.). This would allow accessing the scene tree from detached nodes and from classes not inherited from Node.

I understand that too much global access is bad and I know that custom implementations of MainLoop are possible (however in this case you can use MySceneTree instead of SceneTree I guess). And if the user wants to access the SceneTree from a resource, they can still do AnyAutoloadSingleton.get_tree(). Therefore, I think it makes no sense to restrict globality in this way.

So why is there no SceneTree singleton?

KoBeWi commented 2 years ago

Actually there is a way to access SceneTree globally - Engine.get_main_loop().

MewPurPur commented 2 years ago

It's not just counting down time, that's just the default setup. There's an extra argument for the pausing functionality.

create_tween().tween_interval(1.0).finished   # A bit more cucumbersome and unintuitive than...

get_tree().create_timer(1.0, true).timeout

Which would - kind of - just become create_timer(1.0).timeout and not have to keep track of pausability. I support the idea and would personally almost always prefer to bind the timer to a node, so as to not have stuff happen while my game is paused.

edit: Sorry for ninja edits

kleonc commented 2 years ago

By the way, I wanted to ask for a long time why we cannot add a SceneTree singleton (like Input, Engine, etc.). This would allow accessing the scene tree from detached nodes and from classes not inherited from Node.

I understand that too much global access is bad and I know that custom implementations of MainLoop are possible (however in this case you can use MySceneTree instead of SceneTree I guess). And if the user wants to access the SceneTree from a resource, they can still do AnyAutoloadSingleton.get_tree(). Therefore, I think it makes no sense to restrict globality in this way.

So why is there no SceneTree singleton?

@dalexeev I can be only guessing what's the exact reasoning but here's my take: as I've already mentioned, SceneTree isn't guaranteed to exist (in case custom MainLoop is provided) so that's why SceneTree shouldn't be a singleton (as it seems that each exposed singleton should be available (an instance should actually be created)).

But then you could ask: "Why there is no MainLoop singleton?" as a MainLoop instance should always be available. I'd guess the reason is that singletons are meant only to be used by the user, not actually defined by them (not sure about that part but it seems to be true for all currently exposed singletons). In case of the MainLoop you're actually supposed to extend it and provide such specification. (Also maybe extends MainLoop when MainLoop would be a singleton could be problematic or something like that.) However, I think technically exposing the MainLoop as a singleton (instead of via Engine->get_main_loop()) would be feasible. I mean it seems doable so it's probably a design decision why it's not like that (or maybe it wasn't even considered at all). Again, all of these are just my guesses.

Anyway, your comment and answers to it (including this one) are off topic to the proposal and thus I'll mark them as such. Edit: nevermind, I can't hide @KoBeWi's comment. I guess members are not off topic unless they say so. :smile:

passivestar commented 1 year ago

I always use create_tween() instead of get_tree().create_tween() even when node's lifecycle doesn't matter, just because it's shorter. This would be a big QOL because timers are super common. If it also binds to nodes and solves potential problems down the line that's even better 🔥

tokengamedev commented 1 year ago

adding create_timer() is as intuitive as create_tween(). I don't see any cons to this. It just improves QOL.

Gnumaru commented 1 year ago

It would be better if awaiting for numbers implicitly converted it to awaiting for a newly created scene tree timer.

We can already 'iterate numbers'

for i in 10: print(i)

wich is just syntatic sugar for

for i in range(10): print(i)

It would be really nice to 'await numbers'

await 1.5

which would be syntatic sugar for

await get_tree().create_timer(1.5).timeout
Mickeon commented 1 year ago

I understand where you come from, but "awaiting numbers" is a whole lot more vague than seeing a for loop with the number 10. GDScript aims to be easy to read, and this is not it.

MewPurPur commented 1 year ago

I don't think it's more vague, I think for i in 10 looks more stupid and unclear than await 10 - IMO the problem is that seconds aren't commonly used as a time unit for waiting. Anyway that's not what the proposal is for

dalexeev commented 1 year ago

We can already 'iterate numbers'

for i in 10: print(i)

wich is just syntatic sugar for

for i in range(10): print(i)

This is not actually syntactic sugar, but a behavior of for loop. You can iterate not only numeric literals, but also any numeric expressions (variables, function calls, etc.).

It would be really nice to 'await numbers'

await 1.5

If await operand is not a signal or coroutine, then the value is returned as is. This is so you don't have to worry about whether the operand is a coroutine or not, it solved a big problem we have with yield in 3.x. What you're proposing would add unpredictability to await x behavior, or inconsistency if you only propose it for numeric literals.

Acceptable syntactic sugar would be something like:

await 1.5 sec
await 1500 msec
await x sec

But I’m still skeptical about this; in my opinion, we should avoid implementing in the parser anything that can be implemented using standard means.

Another option is to add a global function to @GDScript (not to @GlobalScope, since no method in the core API returns GDScript-specific coroutines). However, you can implement such an async function yourself. The only limitation is that you will need to use a singleton/static class prefix (await Util.wait(1.5) instead of await wait(1.5)). See also #4740.

API-Beast commented 1 year ago

In Gameplay code you generally want to have timers be bound to the object, if the object is paused (whether because the whole scene is paused or because the particular node had it's process_mode changes) you don't want it to tick. Not having it bound is the reason why I don't use timers anywhere, making them basically useless. So the automatic self-binding from Tweens should definitely be used for Timers as well.

Use get_scene().create_timer() for global timers and Node.create_timer() for local ones.

hamoom commented 11 months ago

I haven't gotten into Godot 4 yet, but at least in version 3, I've encountered issues when yielding on a timer created on the scene tree after the node with the yielded code is freed. I have a custom solution where I'm creating a timer from a singleton and attaching that timer to a node. If the node is freed, there's no problem with the yield since the timer becomes a child of the node, as opposed to being attached to the root tree. I call my singleton like this:

yield(Timers.make_timer(self, 0.1), “timeout”)

Notice how I have to include self as an argument so I can attach the timer to the caller.

The timer also self deletes from the singleton after the timeout.

If you could directly attach a timer to a node, it would significantly improve the safety of yielding on the timer compared to yielding on a timer attached to the scene tree.

KoBeWi commented 11 months ago

In Godot 4 freeing awaiting object is safe.