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

Ensure that SceneTree is initialized and finalized at correct time #72248

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

RedwanFox
Copy link
Contributor

SceneTree should be fully initialized before any tree operation with any node and finalized only after all nodes exited tree.

Accounts only for in-tree operations, not constructors. After fix init-deinit order will be:

  1. constructors (_init) in unspecified order
  2. SceneTree native _initialize
  3. SceneTree script _initialize
  4. All nodes in-tree lifecycle (_enter_tree ->_ready -> ...-> _exit_tree)
  5. SceneTree script _finalize
  6. SceneTree native _finalize

Partially fixes #71695

@RedwanFox RedwanFox requested a review from a team as a code owner January 28, 2023 12:25
@akien-mga akien-mga added this to the 4.0 milestone Jan 28, 2023
@akien-mga
Copy link
Member

Possibly related to #70771 ? CC @kleonc

@RedwanFox
Copy link
Contributor Author

Added one more pull request as follow up to this #72266
It's experimental and is meant for discussion.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the changes, it's logical to:

  • in SceneTree::initialize firstly initialize the base class (call MainLoop::initialize), then do SceneTree-specific initialization (set tree for the root),
  • in SceneTree::finalize firstly finalize the SceneTree-related stuff, then finalize the base class (call MainLoop::finalize).

It's the same ordering as for constructors/destructors.

scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
@kleonc
Copy link
Member

kleonc commented Jan 28, 2023

Possibly related to #70771 ? CC @kleonc

@akien-mga Kinda related as both are fixing some aspects of likely not-so-logical initialization order for MainLoop/SceneTree. But I do think both PRs are fine on their own / independent.

@RedwanFox RedwanFox force-pushed the mainloop_init_order_fix branch from cde35da to 8d1dcd3 Compare January 28, 2023 19:00
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@akien-mga akien-mga requested a review from RandomShaper June 19, 2023 13:52
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for early testing in 4.2.

@YuriSizov
Copy link
Contributor

Needs a rebase though :)

SceneTree should be fully initialized before any tree operation with any node and finalized only after all nodes exited tree.
@RedwanFox RedwanFox force-pushed the mainloop_init_order_fix branch from 8d1dcd3 to 83f065c Compare June 20, 2023 21:25
@RedwanFox
Copy link
Contributor Author

@RandomShaper Could you also look into this #70771 ? It would be nice to have both fixes in upstream
@YuriSizov done

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As already mentioned in #72248 (review) this initialization/finalization order makes more sense to me.

Can't think of anything this change could break besides the cases where user implements custom MainLoop extending SceneTree, and somehow relies on the previous ordering of these callbacks. I find this unlikely though.

Looks good for early testing in 4.2.

☝️

@YuriSizov
Copy link
Contributor

Thanks! Yeah, we're aiming to merge this for 4.2 dev1.

@YuriSizov YuriSizov merged commit 7ff2a93 into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

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.

Mainloop is being created and initialized after main scene and singletons
5 participants