Fix loading of cyclic scenes from GDScript

[adamscott]

Makes it possible for ClassA of SceneA to refer to ClassB of SceneB that refers to ClassA back.

This PR creates Ref<Resource> ResourceLoader::get_loading_resource(const String &p_path, Error *r_error). This function returns a (shallow) resource that is currently being loaded. If not, it returns nullptr and the FAILED error. This is important, because most of the time, the returned reference will be not valid, but will still point later to the loaded resource.

This allows GDScriptAnalyzer to "load" a PackedScene when analyzing a script file even if the scene is currently loading, preventing a cyclic load error.

Fixes #70985
Fixes #90362
Fixes #90954

[adamscott]

Please note: I changed the name of the function from get_shallow_resource to get_loading_resource as it seems that the name better explains what the function does: the function will fail if the resource has been successfully loaded.

[AThousandShips]

If passing is required I'd pass a reference instead to make the requirement explicit

[RandomShaper]

Given there's no nuance to report about the error, I think returning a null Ref is enough, so r_error can go away.

[RandomShaper]

While we're at it, I'm not very sure about the "shallow" wording. Since this is async, by the time the caller deals with the resource, the completeness in its properties having been set and references to dependencies being filled ranges from none (we could call that "shallow") to full. Also, this wouldn't be thread safe, in the sense that there's no sync point with the potentially arbitrary loading thread, which can lead to the caller not only seeing an incomplete resource, but running into data races or race conditions.

Do you think it would be an option to wait for the full resource to be loaded? That would guarantee safety.

If the caller can be happy with the resource in a shallow state (that is, own properties set) the loader needs to be enhanced to support it properly. However, bear in mind that the caller can't safely access anything that can still change (references to external resources, properties already set but that will change due to one of those "queued update" callbacks running later, etc.).

[dalexeev]

For GDScript this doesn't matter in most cases. Only if the user preload() a currently loading resource and do something with the resource at the point. This is probably only possible in a static initializer (explicit or implicit).

If we choose this approach, then we should add a comment that ResourceLoader::get_loading_resource() is intended for GDScript (due to how it works) and should not be used elsewhere in the engine.

Alternatively, we could make such a "shallow" resource null until the end of loading. In both cases it will look unexpected to the user. Not sure which option is better, but probably null option is more explicit.

If the required non-GDScript resource is currently being loaded, then we should not call ResourceLoader::load(). But we need get it sometime, and its loading will complete later than the script. What could be bad if the script executes a code before that, for instance the static initializer.

Here we have two options:

1. Add a hook to ResourceLoader that allow us to get the resource when it is fully loaded. Until than it is null in the script.
2. Add a posibility to ResourceLoader to get a shallow resource, i.e. a currently loading resource, that is not fully initialized yet.

[RandomShaper]

Adding a thought here: I think this only needs to be aware of other loads happening on the same thread (in other words, in the same stack). In that case, ResourceLoader could have a different function for that case explicitly that wouldn't involve any interaction with other threads. I could implement it if you think that'd do the trick.

[adamscott]

Given there's no nuance to report about the error, I think returning a null Ref is enough, so r_error can go away.

It's impossible. 99% of the time, the returned reference will be null. Hence why I force the error return value. We can't differentiate a successful and a failing request only by the returned reference, as both will be null.

[adamscott]

If we choose this approach, then we should add a comment that ResourceLoader::get_loading_resource() is intended for GDScript (due to how it works) and should not be used elsewhere in the engine.

I don't think it's a good idea. Like, I think we could push this technique to support cyclic loading elsewhere too (cyclic node scenes references?). But I agree that this function should have a warning or something like that.

[RandomShaper]

Adding a thought here: I think this only needs to be aware of other loads happening on the same thread (in other words, in the same stack). In that case, ResourceLoader could have a different function for that case explicitly that wouldn't involve any interaction with other threads. I could implement it if you think that'd do the trick.

Elaborating on that:

Since this PR is about a script that is cyclically referenced, ResourceLoader already has a data member that fits the bill: load_paths_stack. That's unique per thread, so checking there is good as it keeps the concern within the dependency chain of the current load chain, deterministically as no threads are involved. However, that vector would need to be modified to it contains the resource references as well.

[adamscott]

Since this PR is about a script that is cyclically referenced

This PR is more about anything that is cyclically referenced in the context of script loading.

[akien-mga]

CC @rune-scape as you've been poking at loosely related issues I think, and we could use more eyes on this important fix for release blocking issues.

[adamscott]

@dalexeev @RandomShaper I removed the cache check. ResourceLoader::get_loading_resource will fail if the resource has been loaded.

[akien-mga]

Let's YOLO merge and see what happens, this fixes a number of critical regressions.

We can iterate on the design of the core API later, or flag it as something that's a hack only for GDScript and shouldn't be used in other parts of the engine.

[akien-mga]

Actually I tested again with the Planet Crasher MRP from #90362 (the ZIP archive, not the Git repo which has since changed and no longer seems to exhibit the bug).

This PR solves this error:

SCRIPT ERROR: Parse Error: Could not preload resource file "res://items/weapon_drop.tscn".
          at: GDScript::reload (res://characters/character.gd:420)
SCRIPT ERROR: Compile Error: 
          at: GDScript::reload (res://items/weapon_drop.gd:-1)
ERROR: Failed to load script "res://items/weapon_drop.gd" with error "Parse error".
   at: load (./modules/gdscript/gdscript.cpp:2951)

But introduces these errors instead:

ERROR: Parameter "method" is null.
   at: is_builtin_method_vararg (./core/variant/variant_call.cpp:1398)
ERROR: Parameter "method" is null.
   at: get_builtin_method_argument_count (./core/variant/variant_call.cpp:1324)
ERROR: Parameter "method" is null.
   at: get_builtin_method_return_type (./core/variant/variant_call.cpp:1377)
ERROR: Parameter "method" is null.
   at: get_validated_builtin_method (./core/variant/variant_call.cpp:1303)
ERROR: Parameter "method" is null.
   at: get_validated_builtin_method (./core/variant/variant_call.cpp:1303)

Introduces new errors.

[akien-mga]

Superseded by #93346. Thanks for the great work!