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

Improve scene initialization with threading #3026

Open
Avantir-Chaosfire opened this issue Jul 21, 2021 · 8 comments
Open

Improve scene initialization with threading #3026

Avantir-Chaosfire opened this issue Jul 21, 2021 · 8 comments

Comments

@Avantir-Chaosfire
Copy link

Avantir-Chaosfire commented Jul 21, 2021

Describe the project you are working on

A 2D game with multiple levels. Specifics are not important.

Describe the problem or limitation you are having in your project

I am trying to add an animated loading screen for all level transitions. Because animations run on Godot's main thread, I have to load the level on a background thread to keep the animations going. Loading a level has many steps, but there are four that take noticeable amounts of time:

  1. Loading the level's resources
  2. Instancing the level's scenes
  3. Adding the level root node to the active scene tree
  4. Initializing the level (often part of #⁠3 through calls to _EnterTree() and _Ready(), but can be separated for the purposes of dependency injection)

#⁠1 seems to take a bit more than half of the loading time, but they all take substantial amounts (with a sufficiently complex scene, 1 second or more), so I need to do all of them on a background thread to have a proper animated loading screen. Herein lies the problem: Running them on a background thread.

#⁠1 is officially supported and works.
#⁠2 is officially supported and doesn't work. (See this bug - this can happen while the scene is being instanced) In addition to the bug, it seems that #⁠2 will freeze the main thread while its running. This is very obvious in the minimal reproduction project for the aforementioned bug, where the main thread attempts to call GD.Print() while the background thread is loading. Here the resource loading is very fast (on scene reload, of course), but the scene instancing takes awhile, as it has many pieces.
#⁠3 is not officially supported and doesn't work.
#⁠4 depends on #⁠3.

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

  1. Multi-Threaded and Single-Unsafe 2D physics should be fully supported and should not risk crashing your game under any circumstances. (i.e. fix this bug, among other things)
  2. Calling PackedScene.Instance() on a background thread should not block the main thread.
  3. Interacting with the active scene tree should officially be made thread safe (see Thread-safe APIs Documentation)

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

I won't comment on how to implement thread safety in the existing areas that need it, as that's an implementation detail.

The minimal reproduction project of this bug demonstrates the basic concept of how I implemented my animated loading screen (using only #⁠1 and #⁠2 mentioned above, since adding #⁠3 is officially not supported at present). It doesn't look very good there, due to the call to PackedScene.Instance() freezing the calls to _PhysicsProcess() (see #⁠2 above). This gif shows it much better, as its loading a much more complicated level for a real game (still in development).

LoadingScreen

The freeze at the end of the loading screen is due to #⁠2, #⁠3 and #⁠4.

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

There is no workaround I am aware of. Animated loading screens simply don't work for the full duration of the loading.

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

Not possible to add in the asset library.

@YuriSizov
Copy link
Contributor

This is already implemented in master where ResourceInteractiveLoader was replaced with a fully threaded solution: godotengine/godot#36640

@Avantir-Chaosfire
Copy link
Author

@pycbouh Please reopen. If you read the proposal, you'll see it has nothing to do with ResourceLoader - the problem is scene instancing and add nodes to the active scene tree. ResourceLoader works fine.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 21, 2021

@Avantir-Chaosfire If it has nothing to do with background loading, then can you rephrase what your proposal is about, for a better title? I see 3 points in the "describe the feature" section, one being "fix the bug", so I'm not entirely sure. If those point refer to different (even though related) aspects, it may be better to create individual proposal that go into detail on each of them (there is no need to open a proposal for fixing a bug though).

@Avantir-Chaosfire
Copy link
Author

@pycbouh The proposal is about background loading, but not resource loading. As I said in the proposal, loading a level has 4 steps, each of which takes a substantial amount of time. When you say "background loading" you're thinking about resource loading, which is step 1, but I'm talking about all 4 steps. The gif I posted shows a pause at the end of the loading screen - that's because Godot only properly supports resource loading, not the other elements of background loading. This proposal is to address the other 3 steps - in particular the middle two, one of which is broken (with a bug (describe the feature #⁠1), and a non-bug (describe the feature #⁠2)) and the other of which is officially unsupported (describe the feature #3). It doesn't make sense to split these into separate proposals, as background loading needs all three to work properly.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 21, 2021

Instancing scenes and adding them to the tree has nothing to do with loading. A hiccup at the end may be (and very likely is) due to shaders initializing at the last moment, which is a know limitation of the current version of Godot.

If you experience crashes and other problems specifically with threading then that's what that should be about, but it's likely not a proposal worthy subject and just a set of bugs that need to be report (if not yet). That said, the tree still needs to be updated and the signals that pass through it still needs to handle, so some hangs will likely always exist, like they exist in any other game engine at the end of the loading stage.

But be it your way, I'll reopen it until we get a second opinion. I have to rename it, though, so it's not misleading.

@YuriSizov YuriSizov reopened this Jul 21, 2021
@YuriSizov YuriSizov changed the title Add Full Support for Background Loading (e.g. Animated Loading Screens) Improve scene initialization with threading Jul 21, 2021
@Avantir-Chaosfire
Copy link
Author

@pycbouh

Instancing scenes and adding them to the tree has nothing to do with loading.

Perhaps I am using the wrong term.

A hiccup at the end may be (and very likely is) due to shaders initializing at the last moment, which is a know limitation of the current version of Godot.

That could definitely be part of it - there are shaders being loaded in the gif I put up. But both the instancing the scene, adding to the active scene tree and initializing the scene take substantial time - I measured all three. And instancing the scene can take a long time even without shaders like in the minimal reproduction project of the bug I linked. In any case, the proposal is that all of these should be possible to be covered by an animated loading screen (otherwise the level freezes at startup, like in the gif).

so some hangs will likely always exist

Yes, there's always some things that initialize/hook up/whatever at the end before the level can start running, but there's no reason that can't be a part of the loading screen. Presently, Godot does not allow that to be part of an animated loading screen, and so forces the freeze. Compare that to a still JPG of a loading screen, which can still cover that initialization/hook up/whatever but doesn't look different at that moment.

If we were talking about a couple frames I wouldn't care, but this is in the order of seconds, and so I take issue with it.

@Calinou
Copy link
Member

Calinou commented May 15, 2022

Related to #1801.

  1. Interacting with the active scene tree should officially be made thread safe

I'd be surprised if SceneTree was ever made thread-safe. This is still not the case in master, even after all the threaded loading changes. There's probably a technical reason for SceneTree not being thread-safe (although I don't know about it personally).

@kayomn
Copy link

kayomn commented Jul 22, 2022

I suppose the question is, is the overhead coming from instancing the nodes or adding them to the tree.

Without breaking out a profiler, I suspect the many small allocations is what is causing the stutter. In this case, making the instancing of nodes in a packed scene happen on a separate thread may significantly mitigate the problem without needing to make the scene tree thread-safe, no?

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

No branches or pull requests

4 participants