godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Improve scene initialization with threading #3026

Open Avantir-Chaosfire opened 3 years ago

Avantir-Chaosfire commented 3 years ago

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 commented 3 years ago

This is already implemented in master where ResourceInteractiveLoader was replaced with a fully threaded solution: https://github.com/godotengine/godot/pull/36640

Avantir-Chaosfire commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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.

Avantir-Chaosfire commented 3 years ago

@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 commented 2 years ago

Related to https://github.com/godotengine/godot-proposals/issues/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 commented 2 years ago

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?