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

EXPERIMENTAL/ FOR DISCUSSION separated autoload initialization from normal node initialization #72266

Conversation

RedwanFox
Copy link
Contributor

@RedwanFox RedwanFox commented Jan 28, 2023

(continuation of #72248)

THIS CHANGE IS EXPERIMENTAL AND PROVIDED FOR DISCUSSION FOR #71695

Autoload singletons should be accessible from any normal node at any time. Right now singletons init as any other node and thus are not yet initialized during initialization of start scene.

Order is something like this

singleton1 _enter_tree
...
singletonN _enter_tree
mainSceneRootNode _enter_tree
   mainSceneChild1 _enter_tree
   ....
   mainSceneChildN _enter_tree
singleton1 _ready
...
singletonN _ready
   mainSceneChild1 _ready
   ....
   mainSceneChildN _ready
mainSceneRootNode _ready

As you can see during main scene _enter_tree phase singletons are not yet ready.

This change treats autoloaded singletons (and its descendant) as separate tree which becomes fully _ready before main scene tree.

Like this:

singleton1 _enter_tree
...
singletonN _enter_tree
singleton1 _ready
...
singletonN _ready
mainSceneRootNode _enter_tree
   mainSceneChild1 _enter_tree
   ....
   mainSceneChildN _enter_tree
   mainSceneChild1 _ready
   ....
   mainSceneChildN _ready
mainSceneRootNode _ready

From documentation https://docs.godotengine.org/en/latest/classes/class_node.html

This means that when adding a node to the scene tree, the following order will be used for the callbacks: _enter_tree of the parent, _enter_tree of the children, _ready of the children and finally _ready of the parent (recursively for the entire scene tree).

Thus it is slightly(?) breaking change. And it doesn't take into account possible dependencies between autoload singletons (aka singletons can't use each other from _ready in any order).

Some things to discuss:

  1. Do autoload nodes need to be registered as singletons in engine?
  2. Do autoload nodes need to be separated from normal nodes to be initialized before them. (thats what this change does)
  3. Does each autoload node (and its children) be treated as separate tree? This makes sure, that each initialized autoload node can safely access all autoload nodes initialized earlier.

@vnen
Copy link
Member

vnen commented Feb 7, 2023

  • Do autoload nodes need to be registered as singletons in engine?

I don't see why that has to be the case. They are accessible via SceneTree and by a script constant when they are marked as singleton (which is a misleading name, but that's another discussion), no need to also make an engine singleton. Autoload names may also conflict with existing singletons.

  • Do autoload nodes need to be separated from normal nodes to be initialized before them. (thats what this change does)

I don't think that should be the case. The only thing I see value is getting all of them ready before the rest of the tree starts being set up. Maybe SceneTree has to be aware of autoloads to perform this prioriority initialization. But after that initial step they can be treated as regular nodes.

  • Does each autoload node (and its children) be treated as separate tree? This makes sure, that each initialized autoload node can safely access all autoload nodes initialized earlier.

Same as above. It does have value initializing them fully before starting the next, but they should not really be treated specially after that.

@RedwanFox
Copy link
Contributor Author

RedwanFox commented Feb 7, 2023

I don't see why that has to be the case. They are accessible via SceneTree and by a script constant when they are marked as singleton (which is a misleading name, but that's another discussion), no need to also make an engine singleton. Autoload names may also conflict with existing singletons.

I didn't know about script constant. I'll redo this part. Oh, didn't read it right. Originally I've added autoloads to singleton list to know which nodes, are autoloads and which aren't. Is there a correct way to tell if node is autoload in engine code?

The only thing I see value is getting all of them ready before the rest of the tree starts being set up. Maybe SceneTree has to be aware of autoloads to perform this prioriority initialization. But after that initial step they can be treated as regular nodes.

Basicly my fix behaves exactly like that. We add singletons only on application start and order is changed only for _enter_tree and _ready events. Then tree traverse order is exactly the same as before. I think _deinitialization should be changed, that singletons are destroyed after all nodes.

It does have value initializing them fully before starting the next,

Ok. I'll split singleton init one by one

autoload singletons should be accessible from any normal node at any time. Right now singletons init as any other node and thus are not yet initialized during initialization of start scene.

Order is something like this

```
singleton1 _enter_tree
...
singletonN _enter_tree
mainSceneRootNode _enter_tree
   mainSceneChild1 _enter_tree
   ....
   mainSceneChildN _enter_tree
singleton1 _ready
...
singletonN _ready
   mainSceneChild1 _ready
   ....
   mainSceneChildN _ready
mainSceneRootNode _ready
```

As you can see during main scene _enter_tree phase singletons are not yet ready.

This change treats autoloaded singletons (and its descendant) as separate tree which becomes fully _ready before main scene tree.

Like this:

```
singleton1 _enter_tree
...
singletonN _enter_tree
singleton1 _ready
...
singletonN _ready
mainSceneRootNode _enter_tree
   mainSceneChild1 _enter_tree
   ....
   mainSceneChildN _enter_tree
   mainSceneChild1 _ready
   ....
   mainSceneChildN _ready
mainSceneRootNode _ready
```
Made it so each other autoload node in order could safely access previously initialized autoloads.

New order of scenetree init is like this
```
singleton1 _enter_tree
singleton1 _ready
....
singletonN _enter_tree
singletonN _ready

scene root node _enter_tree
...its children
scene root node _ready
scene root node _exit_tree

singletonN _exit_tree
...
singleton1 _exit_tree
```
@RedwanFox RedwanFox force-pushed the experimental_singletons_init_order_fix branch from ce84e49 to e363983 Compare February 9, 2023 09:36
@RedwanFox
Copy link
Contributor Author

Small update:

  1. Removed registering autoloads in classDB
  2. Split initialization of each singleton

@vnen
Copy link
Member

vnen commented Feb 23, 2023

Basicly my fix behaves exactly like that. We add singletons only on application start and order is changed only for _enter_tree and _ready events. Then tree traverse order is exactly the same as before. I think _deinitialization should be changed, that singletons are destroyed after all nodes.

What I meant is that it shouldn't happen every time a Node propagates _ready(). Only on the initial tree setup this is relevant. When the game is running there'll be many nodes entering the tree and with this code they all will be checked whether they are autoloads which will never be true. This seems quite wasteful to me, it does not need to be done every time a node enters the tree.

So the Node class itself should not care about this, only the SceneTree when doing the initialization (and finalization as you mentioned), by controlling when propagating the _ready() of the autoloads.

TBH I'm not sure if SceneTree is the one to handle this, maybe it's something for main, but it should definitely be handled in a step above Node.

@clayjohn clayjohn removed request for a team February 23, 2023 20:29
@RedwanFox
Copy link
Contributor Author

I'll think about another solution then. And this particular PR can be closed.

@RedwanFox RedwanFox closed this Feb 26, 2023
@aaronfranke aaronfranke removed this from the 4.x milestone Feb 28, 2023
@vnen
Copy link
Member

vnen commented Feb 28, 2023

BTW, you should open a proposal about this. I do see the merit of doing this but I'm not the only one you need to convince. It also helps reaching an agreement on how to implement before actually writing the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants