PackedScene loading and instancing in separate threads respectively triggers thread guards

[felbecker]

Godot version

4.1

System information

Windows 11 64 Bit

Issue description

I have a 2D world streaming system with 2 background threads that queue the following kind of jobs:

• The first thread just loads PackedScenes.
• The second thread calls instantiate(), decomposes the scene into smaller pieces following specific rules and adds the pieces savely (call_deferred) to the main scene with some delay between them.

This system stopped working (error spam of this kind: get_use_parent_material: This function in this node (TileMap) can only be accessed from either the main thread or a thread group. Use call_deferred() instead.) when migrating from 4.0 to 4.1 because of the new thread guards. It worked fine in 4.0 and also works fine in 4.1 with set_thread_safety_checks_enabled(false).

Still, I was trying to make it work with active checks to be absolutely sure about thread safety. Doing that, I stumbled over this issue:

Apparently the thread guard errors I observe stem from the specific situation when loading a scene in one thread and then instantiate it in another.

In principle (according to Thread-safe APIs and my own experience) instantiating a PackedScene in a thread other than the main thread is safe (can someone confirm this for 4.1 in all cases (rendering, collision ect) ?). The thread guard errors disappear if I move the instancing to the loading thread. Otherwise there are 2 kinds of settings:

1. Someone can confirm that the guards are correct: Scenes should be instantiated in the thread that loaded them. In this case, I would refactor my system and move the instancing to the loading thread and the issue is fixed.
2. The guards should not trigger. Instancing is meant to be safe no matter the thread. In this case, this is a bug report.

Steps to reproduce

This script reproduces the error. In my case complex.tscn is a large 2D scene with particle systems, tilemaps, sprites, collision ect, which I can not provide. If required, I can try to put a test scene together that triggers the issue.

extends Node2D

var thread_load
var thread_instantiate

func _ready():
    thread_load = Thread.new()
    thread_instantiate = Thread.new()
    thread_load.start(_load.bind(self))

func _load(parent):
    var scene = load("res://complex.tscn")
    thread_instantiate.start(_instantiate_add.bind(scene, parent))

func _instantiate_add(scene, parent):
    var instance = scene.instantiate()
    parent.call_deferred("add_child", instance)

Minimal reproduction project

-

[AThousandShips]

It's unclear exactly how this should work, the way the check for testing thread safety for reading differs from how the access one works, the later checks if it is either outside the tree or the thread is safe, whereas the former does only the check for the thread

I'm unsure if this is an oversight or intended, if intended it should be documented if not already

[wagnerfs]

I was checking to see if someone else was having the same issue as I was, in my case, I'm getting errors by merely instantiating nodes in a separate thread. I literaly erased the rest of the code and only doing instantiating and this happens. The solution I had going was moving the collisionshape from the body to the scene and then on _ready(), I'd move it back in and the messages are gone, but it's so unreliable I'm glad I can just set_thread_safety_checks_enabled(false)

[felbecker]

I am not sure if you should be glad to just disable the guards or if it's actually unsafe. I think it could be this (from Thread safe APIs):

one must be careful not to load the same resource from multiple threads at once

Because instantiating would cause loading sub-resources. The Docs recommend a single thread for resource loading. Therefore, loading and instancing should be on the same thread. I'd like to have an official opinion on that.

Also, I'd like to know if this includes the main thread. Because it would mean that if you want thread safe async loading of resources at all, you can never 100% safely load or instantiate anything in the main thread that might be loaded by the loading thread.

[wagnerfs]

@felbecker honestly all I do is node.instantiate() and parent_node.call_deferred("add_child", node), I wouldn't think these would cause any trouble since I actually already did that whenever new scenes were needed, I ultimately added one more thread for it and this new one had this weird problem with "get_transform" when instancing, so I really don't think it's an issue with my threads or instantiating or the issue would've popped on all other threads day one when I started implementing them. The problem is certainly within godot itself because all I do in a thread is instantiating, being single or multiple didn't seem to be a problem.

To make sure I added some mutexes to make sure instantiating only happens once at time and even wrote a single thread for that, still giving me an issue so I'm almost certain it's something to do with how godot thinks something may or may not be unsafe.

[TLA-Games]

I'm having a similar problem. I made a separate project just to narrow down the issue: TestMultithreadingScenes.zip

For me, it handles things fine as long as there are no control nodes. Adding control nodes causes errors. (I saw 19 errors for one simple control node.) Adding things like buttons causes crashes. See the my test project for details.

It is possible that this is caused by loading some communal resource like @felbecker suggested, maybe a default theme?

The documentation doesn't list this as not thread safe, so I would classify this as a bug. Either the documentation needs to be more specific, so we know which kinds of scenes are safe to load from separate threads (Because some clearly are), or this needs to be marked as a bug and escalated so a fix can be made in the engine.

Either of those options would be fine, but I am not comfortable shipping a product with the thread safety checks disabled. In some of my own testing, the game still crashes while setting safety checks to false. I think they are there for a reason.

[dqdthanhthanh]

I just tested on 4.1.3 and 4.2 rc1 and it crashed using the thread. But works normally when convert scene from tscn to scn.

[geeky-devil]

I am completely new to this and was looking for solution for a while before finding this issue here. i checked the scenes provided by @TLA-Games and all worked fine without crashing. Also i tried your code for myself and it indeed help me fix the issue of loading a packedscene in a sub thread group node. however i still get similar errors mentioned above.

version 4.2.2

i am not able change the properties of any physics related object of the packedscene. would like to know if anyone found a solution, there aren't many sources to check or get help.

[pnico]

Is this still an issue on 4.3? I've tried the test project here and the one linked to issue #86049 and I can't trigger the crash on either one. Is it only happening on some devices maybe? I tested using 4.3 in the editor and on a fairly-decent ipad; it's possible those are both fast enough to prevent this happening.

I'm brand-new to Godot, but a longtime mobile game developer accustomed to being asked to reduce jank during scene transitions ;) I think a best-practices doc about how best to load scenes asynchronously without fear would be worth the time, it's a fairly common need.

Finally some clarity on whether this is/was indeed a bug or not would be great, to prevent people writing a bunch of unnecessary code to try to prevent something that doesn't happen any more, and clear a possible red flag to using Godot for mobile games.

I'm unsure if this is an oversight or intended, if intended it should be documented if not already

^ @AThousandShips would it be possible to find out? 😇

[AThousandShips]

I don't know in this case, can do some digging when I have the time