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.

Sign up for GitHub

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

Fix loading of cyclic scenes from GDScript #92326

Closed
wants to merge 1 commit into from
Closed

Fix loading of cyclic scenes from GDScript #92326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions core/io/resource_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,25 @@ Ref<Resource> ResourceLoader::load(const String &p_path, const String &p_type_hi
return res;
}

// Returns a shallow reference of the loading resource.
// Will fail if the resource is not loading.
Ref<Resource> ResourceLoader::get_loading_resource(const String &p_path, Error *r_error) {
ERR_FAIL_COND_V(r_error == nullptr, nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@adamscott adamscott Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@adamscott adamscott Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*r_error = OK;

String local_path = _validate_local_path(p_path);
{
MutexLock thread_load_lock(thread_load_mutex);

if (!thread_load_tasks.has(local_path)) {
*r_error = FAILED;
adamscott marked this conversation as resolved.
Show resolved Hide resolved
return nullptr;
}

return thread_load_tasks[local_path].resource;
}
}

Ref<ResourceLoader::LoadToken> ResourceLoader::_load_start(const String &p_path, const String &p_type_hint, LoadThreadMode p_thread_mode, ResourceFormatLoader::CacheMode p_cache_mode) {
String local_path = _validate_local_path(p_path);

Expand Down
1 change: 1 addition & 0 deletions core/io/resource_loader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ class ResourceLoader {
static bool is_within_load() { return load_nesting > 0; };

static Ref<Resource> load(const String &p_path, const String &p_type_hint = "", ResourceFormatLoader::CacheMode p_cache_mode = ResourceFormatLoader::CACHE_MODE_REUSE, Error *r_error = nullptr);
static Ref<Resource> get_loading_resource(const String &p_path, Error *r_error);
static bool exists(const String &p_path, const String &p_type_hint = "");

static void get_recognized_extensions_for_type(const String &p_type, List<String> *p_extensions);
Expand Down
16 changes: 12 additions & 4 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4296,20 +4296,28 @@ void GDScriptAnalyzer::reduce_preload(GDScriptParser::PreloadNode *p_preload) {
}
} else {
// TODO: Don't load if validating: use completion cache.
String resource_type = ResourceLoader::get_resource_type(p_preload->resolved_path);

// Must load GDScript separately to permit cyclic references
// as ResourceLoader::load() detects and rejects those.
if (ResourceLoader::get_resource_type(p_preload->resolved_path) == "GDScript") {
if (resource_type == "GDScript") {
Error err = OK;
Ref<GDScript> res = GDScriptCache::get_shallow_script(p_preload->resolved_path, err, parser->script_path);
p_preload->resource = res;
if (err != OK) {
push_error(vformat(R"(Could not preload resource script "%s".)", p_preload->resolved_path), p_preload->path);
}
} else {
p_preload->resource = ResourceLoader::load(p_preload->resolved_path);
if (p_preload->resource.is_null()) {
push_error(vformat(R"(Could not preload resource file "%s".)", p_preload->resolved_path), p_preload->path);
// Permit cyclic loading if `ResourceLoader` is currently loading that resource.
Error err = OK;
Ref<PackedScene> res = ResourceLoader::get_loading_resource(p_preload->resolved_path, &err);
if (err == OK) {
p_preload->resource = res;
} else {
p_preload->resource = ResourceLoader::load(p_preload->resolved_path);
if (p_preload->resource.is_null()) {
push_error(vformat(R"(Could not preload resource file "%s".)", p_preload->resolved_path), p_preload->path);
}
}
}
}
Expand Down
Loading