Fix freeze on non-thread-safe custom importers

[hunterloftis]

Godot currently assumes that all importers are thread-safe, leading to non-deterministic, OS and hardware-dependent deadlocks. Several of Godot's built-in importers already override this false assumption in order to avoid freezing the editor:

• godot/editor/import/3d/resource_importer_obj.h

  Lines 66 to 67 in 92e51fc

  	// Threaded import can currently cause deadlocks, see GH-48265.
  	virtual bool can_import_threaded() const override { return false; }

• godot/editor/import/3d/resource_importer_scene.h

  Line 307 in 92e51fc

  virtual bool can_import_threaded() const override { return false; }

This updates the EditorImportPlugin implementation and documentation to make custom importers safe to use by default, while still allowing them to opt into threaded concurrency optimizations if they are actually thread-safe.

Fixes #85237

[hunterloftis]

If this general approach is 👍 from the core team, I'll go through and enable _can_import_threaded for the built-in importers that are currently defaulting to true.

[fire]

I think changing the meaning of methods in a stable release might not make sense. I have no idea when godot 5 will be released.

Also importers can override this method so its a uiux bug not a blocker

Edited: doing the backwards compatibility of deprecrating the old method and renaming may work but not sure.

[hunterloftis]

I think changing the meaning of methods in a stable release might not make sense. I have no idea when godot 5 will be released.

Could you clarify this? This PR doesn't change the meaning of any methods.

Also importers can override this method so its a uiux bug not a blocker

Assuming they know about this issue, which deadlocks the editor as soon as you try to open the project. The reason so many issues exist about this is because many users don't know that their importer is being threaded:

• Freeze on ResourceSaver.save in custom importer when using multiple threads #85237
• Deadlock in multithreaded importer for .obj files, or "Creating buffers with data is forbidden during creation of a draw list" error #48265
• Multithreaded import deadlocks when importer generates lightmap UV's. #85773
• Godot 4.3 Crashes During Import of Large Asset Directory #96242
• Editor freezes when reimporting glb if mesh has a shader material that contains an #include #94586

Further, since the deadlock is host-hardware and OS dependent and random, even testing by multiple users may not reveal an underlying issue. I discovered it for example after integrating godot-ink when our first Windows-based developer tried to run the project.

Edited: doing the backwards compatibility of deprecrating the old method and renaming may work but not sure.

Again, could you clarify? There is no "old method" or method renaming here.

Currently, all custom importers for Godot fall into one of three categories:

1. Works, with concurrency optimizations
2. Deadlocks
3. Author overrides _can_import_threaded so it works, without concurrency optimizations

This change removes the deadlock category, so:

1. Works, without concurrency optimizations
2. Author overrides _can_import_threaded so it continues to work, with concurrency optimizations.

[fire]

@dsnopek Is there any problems with changing this definition in gdextension? I can see the merits but curious

[hunterloftis]

@AThousandShips I don't know the metrics used for "breaks compat" in this project, but in terms of functionality this change is fully backwards-compatible for both internal and custom importers.

[AThousandShips]

Changing the default return of a virtual method breaks compatibility in my book, it makes it so that anyone who didn't override this method because they were just going to return true now has broken code, how is it backwards compatible at all? Granted the loss of threaded loading might not be critical but still

But let's see what dsnopek says

[hunterloftis]

Like I said, the project can of course use whatever metric it likes for "breaks compat." I'm clarifying for accuracy, since this is not true:

it makes it so that anyone who didn't override this method because they were just going to return true now has broken code

Their code will not be broken, any more than their code would be broken running on a single-core CPU (as the behavior of their code will be identical to how it would run today on such a CPU - which, in all cases, is more likely to be stable).

[clayjohn]

I don't think this breaks compatibility. Yes it changes default behaviour, but that doesn't mean compatibility is broken.

IMO "breaks compat" should be reserved for cases where we actually change or remove a feature in a way that breaks things.

In this case, the change in behaviour won't break anything, at most it will make your custom resource import slower.

[AThousandShips]

I'd say that the public API is generally considered part of compatibility, especially behavior changes, but I'll defer to dsnopek on this

Either way I'd say this should be documented in the upgrading from 4.3 to 4.4 page when that is made

[dsnopek]

This is technically a compat breakage, because there may be custom resource importers which are thread-safe and relying on the existing default of true. However, I suspect defaulting to false is the conservative option, and resource importers that are thread-safe will still work even with false, just less efficiently? So, assuming that's right, and the asset/import team is OK with this change, then it's probably fine.

[hunterloftis]

I've updated the PR to enable threaded imports on engine importers that are thread-safe, leaving the non-thread-safe importers with the false default.

[fire]

I approve this because we switch from an optimistic default assumption that custom importers are thread-safe to a conservative default assumption that they are not thread-safe.

@dsnopek mentions this is an conservative default that changes the meaning but is safe in the sense it doesn't destroy code functionality, but only run slower.

Custom importers that are thread-safe can be declared thread-safe by their code maintainers.

This updates the EditorImportPlugin implementation and documentation to make custom importers safe to use by default while allowing them to opt into threaded concurrency optimizations if they are thread-safe.

We could ensure the status quo is kept for existing importers in our tree.

@lyuma @TokageItLab @SaracenOne might have opinions, but I'm ok with making this quality-of-life change where it's easier to implement custom importers without crashing because of a lack of knowledge of the threaded requirement - I assume this was because previous godot engine versions didn't have gdextension class extension and was an optimistic way to encourage godot engine contributors to test threaded-ness in their importer code by default when it was implemented by @reduz for Godot Engine 4.0.

[hunterloftis]

Is there anything I need to do to have this ready for 4.4 or is it good-to-go, just waiting?

[adamscott]

Seems ok to me. I was worried it could impact single-threaded builds, but it doesn't seem so, especially since the default for can_import_threaded() was true.

[Repiteo]

Thanks! Congratulations on your first contribution! 🎉