godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.16k stars 21.19k forks source link

Node 'is_node_ready()' function and 'ready' signal do not function as expected with coroutines #95087

Open dev-mico opened 3 months ago

dev-mico commented 3 months ago

Tested versions

v4.2.2.stable.official [15073afe3]

System information

Godot v4.2.2.stable - Windows 10.0.22631 - GLES3 (Compatibility) - NVIDIA GeForce RTX 4070 Laptop GPU (NVIDIA; 31.0.15.3210) - 13th Gen Intel(R) Core(TM) i7-13700HX (24 Threads)

Issue description

When waiting for a node to be ready, the 'ready' signal will fire before the _ready() function finishing, if it is a coroutine. Same issue is present when we check if a node is ready via is_node_ready.

In my opinion (and what I would expect would be the case), nodes should not emit 'ready' until the whole _ready() function runs - even with coroutines - and a node's "is_node_ready" call should return false until the ready call finishes (even with the coroutines). According to the docs, this seems to be the intended behavior (see 'ready' signal description).

Not 100% sure if this is intended behavior, but if it isn't, it would be nice to get a fix, and if it is, then I think the docs for the 'ready' signal and 'is_node_ready' function should clarify that.

Steps to reproduce

Here is some simple sample code you can throw in any file and run:

extends Node

class CustomNode:
    extends Node

    func _ready():
        print("CustomNode ready start")
        await get_tree().create_timer(1).timeout
        print("CustomNode ready end")

    func _init():
        ready.connect(_on_ready)

    func _on_ready():
        print("CustomNode onready")

func _ready():
    var custom_node = CustomNode.new()
    add_child(custom_node)

Notice that "CustomNode ready end!" prints after "CustomNode onready."

Minimal reproduction project (MRP)

see sample code - not necessary to attach an entire project, just create a new one and throw the above in an autoload script

buffalocat commented 3 months ago

I think this is intended and reasonable; Node.is_node_ready() says a node is ready when "it's inside the scene tree and all its children are initialized". A node "becoming ready" has few consequences - causing is_node_ready() to return true, calling _ready(), and emitting the ready signal - and all of these happen as close together as possible. They should happen as if they were connected to a signal, in that order.

If these didn't all happen "at the same time" the semantics would be more confusing - is a parent node allowed to consider itself ready yet, or do you halt the entire initialization of the scene until this call to _ready() returns?

I might agree that the documentation could be more explicit because it's a subtle issue, but I don't think anything I saw is wrong. I think the key thing is that _ready() is described as being called "when the node is ready" - it's a consequence of the node becoming ready, not the definition of when it becomes ready!

KoBeWi commented 3 months ago

Well this is the order of actions: https://github.com/godotengine/godot/blob/3978628c6cc1227250fc6ed45c8d854d24c30c30/scene/main/node.cpp#L270-L274 NOTIFICATION_READY calls the _ready() callback and it does happen before the signal. The signal should be emitting after the function has finished.

ready_first is what affects is_node_ready(). The order is irrelevant here, because inside _ready() callback the node is assumed to be ready. The typical usage is:

if not is_node_ready():
    await ready
dev-mico commented 3 months ago

@buffalocat if this is intended, I agree - no problem here! A workaround is simple to implement. Just wanted to at least recommend an adjustment to the docs, to make it clear that the signal will not wait for coroutines.

dev-mico commented 3 months ago

can someone make an adjustment to the docs so i can close this? thank you!

KoBeWi commented 3 months ago

I took a closer look at your code now and it's not even a documentation issue. _ready() is just a script callback, halting it with await will not prevent ready signal from being emitted, because it happens internally (lower than at script level). It's a detail that's not really related to ready, but more to how scripting works in general.

dalexeev commented 3 months ago

Once execution reaches an await whose argument is a signal or a coroutine, the current call frame is stopped (even though the function has not fully completed) and control returns to the previous frame in the call stack.

Once the awaited signal is emitted or the awaited coroutine is fully completed, the rest of the function after the await will continue independent of the parent call stack.

Here is an image for 3.x, with yield instead of await:

So there is no bug, but we could describe this in more detail in the await documentation. This is already mentioned, but rather in an inverse form:

If you use await with an expression that isn't a signal nor a coroutine, the value will be returned immediately and the function won't give the control back to the caller

dev-mico commented 3 months ago

Understood. Is this the case as well with #82072? Made that report a while ago, and if it's intended behavior, then maybe I can add a comment there.