godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Provide a way to modify order of `_ready` calls #5332

Open and3rson opened 2 years ago

and3rson commented 2 years ago

Describe the project you are working on

A 2D platformer which uses a lot of shared nodes in game scene (HUD, Persistence (game saves), Inventory, Stat modifiers, Effects, etc)

Describe the problem or limitation you are having in your project

I'm attempting to make my node (bearer) interact with child of one of its parents (hud - let's call it a "shared" node). However, bearer is always ready earlier than hud because it's located deeper in the tree.

Consider the following tree structure:

image

Here's an excerpt from the hud node (which is used in many places across the project):

# hud:
extends Node

onready var top = find_node('top')
onready var bottom = find_node('bottom')
onready var icon = find_node('icon')

func _enter_tree():
    Globals.hud = self  # Store ourselves globally as early as possible since this node is accessed by other nodes

func set_text(line1, line2):
    top.text = line1
    bottom.text = line2

Now, let's say we're equipping some player item within bearer node in its _ready function (this node handles player's ability to "hold" items):

# bearer:
func _ready():
    var current_item = Globals.persistence.get('current_item')
    Globals.hud.set_text(current_item.title, current_item.ammo)

Once we run this, we are getting an error in hud node on line 11 (top.text = line1) because hud not ready yet, even though it appears higher in the tree.

At this point, there's no way to make this work out of the box since bearer will ALWAYS become ready earlier than hud, which is documented but is not as many would expect: hud is higher in the tree, it's "more common", "more important", etc.

I've found myself stumbling upon this many times and I really wish there was some sort of process_priority alternative, but for _ready.

TL;DR:

Current tree order for _ready calls is:

- a (7)
  +- b (5)
     +- c (1)
     +- d (2)
  +- e (6)
     +- f (3)
     +- g (4)

However I'm willing (and was expecting) to have it like this:

- a (7)
  +- b (3)
     +- c (1)
     +- d (2)
  +- e (6)
     +- f (4)
     +- g (5)

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

In other words, I feel like the order should be "a node is ready EXACTLY after all of its children are ready", not "a node is ready AN ARBITRARY NUMBER OF NODES after all of its children are ready".

In fact, the documentation says that my suggestion is the correct assumption of what the readiness order should be. Excerpt from the docs:

Called when the node is "ready", i.e. when both the node and its children have entered the scene tree. If the node has children, their _ready callbacks get triggered first, and the parent node will receive the ready notification afterwards.

However this is not the case. For example, in this scenario e._ready is called BEFORE b._ready, which will make it impossible t access b from e and also means the documentation is slightly counterintuitive:

- a (5)
  +- b (3)
     +- c (1)
  +- d (4)
     +- e (2)

One would clearly expect b._ready to be called directly after c._ready, since all of b's children are ready. Then the proper order would be as follows:

c -> b -> e -> d -> a

This would make b available for all nodes that follow it in the tree (including d and e).

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

I have several suggestions in mind:

  1. Change tree order for _ready so that a node becomes ready directly after all of its children are ready. Yeah, I understand this is a major change, but I was honestly expecting it to be the default behavior. Besides, I highly doubt that anyone working with Godot writes their scripts while keeping the number of nesting levels in mind: most of us simply think that "if it's closer to the top, it probably is handler earlier."
  2. Provide "instantiation_priority" - please correct me if there's a better way to name a process that makes the node "ready". But you get the idea.
  3. Add an ability to define some sort of "instantiation groups" to override the flow of node instantiation.

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

Possible workarounds:

  1. Use singletons for all those shared nodes (like hud): but what if we have different scenes (game, menu, map, etc)? What if we want to have hud ONLY on game scene? Singleton nodes would be global, which will keep them loaded all the times even when they are not needed and will introduce a lot of checks in them like "if is_game_scene: hud.show()", etc.
  2. Use yield(hud, 'ready'): this will introduce a huge amount of boilerplate code every time we want to access some shared node (like hud). It will be terribly easy to forget to add this check and will introduce huge amount of overhead. Developers will need to remember which nodes to yield(node,'ready') and which not.
  3. Don't use shared nodes at all - but nodes are great for composition and code structuring. Our entire -projects uses built-in scripts almost everywhere, nodes are extremely convenient for this. We have nodes like animated, kinematic, inventory, damageable, pickable, etc and we heavily use composition for building our game logic.

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

Since this is a built-in functionality, it cannot be overridden by a library of an asset.

YuriSizov commented 2 years ago

However, bearer is always ready earlier than hud because it's located deeper in the tree.

Not really, that order is undefined, because those nodes are not related. Your logic that involves both of them should be located in their common parent, which will be ready only when both of them are.

The logic still works as described in the docs, but neither the docs nor the engine give you a guarantee that unrelated nodes will be ready in some predictable or controllable order. This is simply not possible. Hence why the appropriate code organization, or a "workaround" if you will, is to have the shared logic in the shared ancestor. This is what you are supposed to do in Godot.

and3rson commented 2 years ago

Not really, that order is undefined, because those nodes are not related

Having to keep shared logic in the shared ancestor kills the composition since the "components" have their own logic that relies on some globally available nodes. I'm simply trying to find a proper way to ensure that an arbitrary node X somewhere deep in the tree is ready before some other arbitrary node Y somewhere else deep in the tree, but Godot provides no such guarantee.

Edit: singletons (autoload) do this exact thing (they are ready before everything else), but as I mentioned, using autoload forces the object to be available on all scenes, even where they are not used, thus providing overhead and extra complexity.

YuriSizov commented 2 years ago

I'm simply trying to find a proper way to ensure that an arbitrary node X somewhere deep in the tree is ready before some other arbitrary node Y somewhere else deep in the tree

You have that guarantee in the shared parent. It does nothing bad to the composition. If it's important to some general state of the game, you can obviously use singletons or event bus patterns as well.

singletons (autoload) do this exact thing (they are ready before everything else)

This is actually not guaranteed either, because they are simply nodes added directly to the tree root. If their ready status is delayed by something, they will not be ready before your main scene. And the same is true for the relationship between multiple autoloaded nodes.

and3rson commented 2 years ago

@YuriSizov what about my situation with the hud node? How do I make sure it (and all its children) are always ready before everything else?

YuriSizov commented 2 years ago

@YuriSizov what about my situation with the hud node? How do I make sure it (and all its children) are always ready before everything else?

You can't make sure they are ready before anything else. Ready is a status of the node in relation to the tree and the node's own descendants. If your logic needs X to initialize Y you code it separately from the ready status. Using singletons, event busses, or, most commonly, shared parents.

AttackButton commented 2 years ago

If you don't want to put the logic directly in the parent node, you can make the bearer _ready function to wait until the parent is ready. This guarantees that the hud node will be ready before the bearer node:

In godot 4:

in the bearer node:

func _ready() -> void:
    await get_parent().get_parent().ready
    var current_item = Globals.persistence.get('current_item')
    Globals.hud.set_text(current_item.title, current_item.ammo)
    ...

or

func _ready() -> void:
    await $"../..".ready
    var current_item = Globals.persistence.get('current_item')
    Globals.hud.set_text(current_item.title, current_item.ammo)
    ...
and3rson commented 2 years ago

@YuriSizov what about my situation with the hud node? How do I make sure it (and all its children) are always ready before everything else?

You can't make sure they are ready before anything else. Ready is a status of the node in relation to the tree and the node's own descendants. If your logic needs X to initialize Y you code it separately from the ready status. Using singletons, event busses, or, most commonly, shared parents.

This makes sense. I did not think about the fact that nodes are doing lots of complex stuff under the hood, so their readiness order will not always be deterministic.

I think I'll solve my problem by adding some flag to "Globals" instance which will indicate whether any requested shared object is ready.

Still, it would make great sense to extend Node's documentation with a statement that "parent's _ready method will not always be called directly after all of its children are ready, but might be delayed due to internal processing" (or similar).

Additionally, making it possible to force-wait for some specific node to be ready before instancing other nodes would still be great. Currently it can be achieved only by using nested scenes and "placeholder" flag.

Zireael07 commented 2 years ago

In a similar situation, I used a combo of a) doing stuff as far up the tree as possible (tree root or its immediate child) and b) an event bus singleton and signals because some things are spread over several frames by await - if you do not have the latter, just doing stuff from tree root should be enough

and3rson commented 2 years ago

In a similar situation, I used a combo of a) doing stuff as far up the tree as possible (tree root or its immediate child) and b) an event bus singleton and signals because some things are spread over several frames by await - if you do not have the latter, just doing stuff from tree root should be enough

I was also thinking about converting most of my scene into an embedded scene, marking it as placeholder (i. e. InstancePlaceholder) and then calling create_instance once all shared objects are ready. The obvious downside is I'll end up having a bunch of parts scattered around instead of one nice self-containing scene.

I wonder if there's something similar to InstancePlaceholder for ordinary nodes to artificially postpone instantiation of part of the scene, but without the need to create an actual separate scene file. Or just a way to tell Godot "hey, don't do ANYTHING with this node until that other node is ready." Like some sort of a local PackedScene.

AttackButton commented 2 years ago

Or just a way to tell Godot "hey, don't do ANYTHING with this node until that other node is ready."

Did you try using the ready signal with the await keyword as I mentioned? That worked perfectly for me in the past.

and3rson commented 2 years ago

Or just a way to tell Godot "hey, don't do ANYTHING with this node until that other node is ready."

Did you try using the ready signal with the await keyword as I mentioned? That worked perfectly for me in the past.

Yes, thanks for your suggestion, that would totally work, but it would require me to await every time I attempt to do anything with hud. And sometimes I don't directly access hud, but via intermediary functions, so these would need to await too and would become coroutines. And besides hud, there are ~8 other singletons, so all of those need to be awaited, too...

In the end, I'll have a lot of boilerplate awaits where a single explicit blocking dependency in the parent node would do the same.

AttackButton commented 2 years ago

but it would require me to await every time I attempt to do anything with hud

But this will not create a coroutine at all with the hud node, the await will just wait once for the ready signal from the common parent.

But ok. :)

and3rson commented 2 years ago

but it would require me to await every time I attempt to do anything with hud

But this will not create a coroutine at all with the hud node, the await will just wait one time for the ready signal from the common parent.

But ok. :)

I think that might be true for 4.x, but in 3.x it will still create a coroutine

KoBeWi commented 2 years ago

Not really, that order is undefined, because those nodes are not related.

Eh, all nodes in the tree are related, because they belong to the same tree. In the provided screenshot, the ready order will be

Not sure what you mean by "undefined". This is perfectly deterministic. This is of course assuming that the whole scene is loaded at once.

@and3rson The nodes should be initialized in the order you listed as "expected". Either you do something else than you described or you ran into some bug. Do you have some example scene where the order is different? Also what version are you using?

AttackButton commented 2 years ago

but it would require me to await every time I attempt to do anything with hud

But this will not create a coroutine at all with the hud node, the await will just wait one time for the ready signal from the common parent. But ok. :)

I think that might be true for 4.x, but in 3.x it will still create a coroutine

I did test this in 3.x and this works as well. I remember that you could also use the root node or the owner property (whether the node, instantiated or not has the owner defined).

func _ready():
    yield(get_tree().current_scene, "ready")
    print("bearer")

or

func _ready():
    yield(owner, "ready")
    print("bearer")
and3rson commented 2 years ago

yield(get_tree().current_scene, "ready")

@AttackButton This is actually awesome, I did not think about the possibility of waiting for an entire scene to become ready. Additionally I could group the "singletons" in some common node (like "shared") and then wait for it. This helps a lot, thanks for an idea! The only downside is they might be already "ready" under some circumstances, so an additional boilerplate like "get_tree().current_scene.is_inside_tree()" is required.

@KoBeWi In my case, $player/bearer._ready gets called before $hud._ready. I'm using 3.5-stable. EDIT: I'm still trying to consistently reproduce this, so it might take a while...

YuriSizov commented 2 years ago

Not sure what you mean by "undefined". This is perfectly deterministic. This is of course assuming that the whole scene is loaded at once.

👀 "It's perfectly deterministic, if we assume some things to be true" is not perfectly deterministic.

What I mean by "undefined" is that regarding the "ready" status there is no connection between those nodes, so there is no determined order between them. They simply don't affect each other, so you should not code as if any sort of order is expected. You will run into bugs.

hilfazer commented 2 years ago

Careful with yielding in _ready(). ready signal is emitted not only on function's return but also on first yield (in which case it won't be emitted on return). That can mess up nodes that rely on this node's ready signal being emitted when _ready() did all of its job.

KoBeWi commented 2 years ago

In my case, $player/bearer._ready gets called before $hud._ready. I'm using 3.5-stable.

I get the same result in 3.5 (i.e. the "correct" one). As I mentioned in my comment, this order is only true if you have this tree structure in a single scene and instance it all at once. If some nodes are added later then they obviously will initialize later (depending on when you add them).

What I mean by "undefined" is that regarding the "ready" status there is no connection between those nodes, so there is no determined order between them.

But technically this is deterministic. You said that

The logic still works as described in the docs, but neither the docs nor the engine give you a guarantee that unrelated nodes will be ready in some predictable or controllable order. This is simply not possible.

which is not really true. The order of initialization is very predictable, but you need to know how to use it. When you instance a scene and do an add_child(), the added node(s) will have their _ready() called always in the same order, as documented.

Although I agree that you shouldn't rely on initialization order of sibling nodes. You change scene order and everything breaks.

AttackButton commented 2 years ago

Careful with yielding in _ready(). ready signal is emitted not only on function's return but also on first yield (in which case it won't be emitted on return). That can mess up nodes that rely on this node's ready signal being emitted when _ready() did all of its job.

It is emitted just once, when the _ready function is called, which always guarantee such order. I don't think you can delay the ready signal emission. Probably, the only thing that could delay it is the size of the node tree, but that is expected and don't change the called order.

Edit: Reading this makes me think that _start function is a better name than _ready.

hilfazer commented 2 years ago

Yes, it's emitted only once but my point was it's not neccessarily after _ready() did all of its work. Consider:


var m = 0

func _ready():
    yield()
    m = 4
    return

ready will be emitted before m becomes 4. Objects connected to this signal expecting m to be four when it fires will be in for a nasty surprise.

AttackButton commented 2 years ago

You're right. Another issue is to remember that _process and _physics_process will be called independently of the _ready() block, needing to be disabled before the await/yield and enabled after it.

var m = 0

func _ready():
        _set_physics_process(false)
    yield(get_tree().current_scene, "ready")
        m = 4
        _set_physics_process(true)
    print("bearer")

func _physics_process(delta):
        print(m) #could print 0 before await and 4 after await