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 MainLoop and its custom script is set right after it's resolved #70771

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Dec 31, 2022

For a SceneTree-derived main_loop the OS::get_singleton()->set_main_loop(main_loop); was called after adding AutoLoads and the main scene to the tree. This PR makes it be set right after the main_loop is resolved.

Also previously setting custom script of the main_loop was postponed (via MainLoop::set_initialize_script) to be done in MainLoop::initialize. This PR removes MainLoop::set_initialize_script and makes the script of the main_loop be set directly right after it's instantiated.

Fixes #70764.

@kleonc kleonc added bug topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Dec 31, 2022
@kleonc kleonc added this to the 4.0 milestone Dec 31, 2022
@kleonc kleonc requested review from a team December 31, 2022 14:06
@kleonc kleonc requested a review from a team as a code owner December 31, 2022 14:06
@kleonc kleonc changed the title Ensure MainLoop is set right after it's resolved Ensure MainLoop is set right after it's resolved Dec 31, 2022
@kleonc kleonc marked this pull request as draft December 31, 2022 14:18
@kleonc kleonc force-pushed the main-loop-set-after-resolved branch from 6de1e07 to 7105cd2 Compare December 31, 2022 16:59
@kleonc kleonc marked this pull request as ready for review December 31, 2022 17:18
@kleonc kleonc changed the title Ensure MainLoop is set right after it's resolved Ensure MainLoop and its custom script is set right after it's resolved Dec 31, 2022
@kleonc kleonc force-pushed the main-loop-set-after-resolved branch from 7105cd2 to db48df3 Compare December 31, 2022 17:23
@akien-mga akien-mga removed request for a team January 9, 2023 09:57
@@ -2705,6 +2705,8 @@ bool Main::start() {
}
}

OS::get_singleton()->set_main_loop(main_loop);
Copy link
Member

Choose a reason for hiding this comment

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

Why moving this here?

Copy link
Member Author

@kleonc kleonc Jan 19, 2023

Choose a reason for hiding this comment

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

The if below handling SceneTree-derived main_loop adds nodes to the tree (autoloads, main scene), meaning it can trigger some user code. If they'd use Engine.get_main_loop() in there it would return null at this point because it was not set yet (pre moving it in here). This fixes e.g. snippet from #70764 (comment).

Copy link
Contributor

@RedwanFox RedwanFox Jan 19, 2023

Choose a reason for hiding this comment

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

Isn't it strange that main_loop (base or overriden) becomes initialized after first scene is initialized?
UPD Did some research #71695

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.

Hard to assess all the implications, but GTG IMO post-4.1. If this were not completely right, things would start exploding noticeably.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 23, 2023
@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Jun 23, 2023
@YuriSizov YuriSizov merged commit 95da8e1 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
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The custom Mainloop inherited from SceneTree does not work properly
6 participants