Skip to content

Add cyclic dependencies support for scenes's scripts#71004

Closed
adamscott wants to merge 1 commit intogodotengine:masterfrom
adamscott:fix-scene-cyclic-dependency
Closed

Add cyclic dependencies support for scenes's scripts#71004
adamscott wants to merge 1 commit intogodotengine:masterfrom
adamscott:fix-scene-cyclic-dependency

Conversation

@adamscott
Copy link
Copy Markdown
Member

@adamscott adamscott commented Jan 6, 2023

Enables loading scenes scripts with cyclic dependencies.

Fixes #70985


#ifdef MODULE_GDSCRIPT_ENABLED
// GDScript has a way to load cyclic dependencies
if (res.is_null() && ResourceLoader::get_resource_type(path) == "GDScript") {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I know @reduz won't be pleased with this, but I wonder if there's another way to do this? GDScript is pretty much the only resource (with the workaround found for packed scenes in GDScriptCache) that supports cyclic resources.

ResourceLoader::load(path, type) returns a null Variant when it's already loading that resource, instead of returning the resource currently loaded. But it cannot do that because that resource doesn't exist, hence that GDScript check.

@vnen
Copy link
Copy Markdown
Member

vnen commented Jan 12, 2023

This really needs an input from @reduz. Including module code in scene can be quite problematic.

@adamscott
Copy link
Copy Markdown
Member Author

@akien-mga told me that this core hack is a no-go. Closing.

@akien-mga
Copy link
Copy Markdown
Member

akien-mga commented Jan 12, 2023

To be clear, I said:

And yeah for #71004 the core hack is a no-go, we'll have to find a different solution. Can't this be handled in the GDScript resource loader, so it can fallback to the shallow stuff by itself?
Otherwise, if it's only needed for ResourceFormatLoader, then it will need more abstraction so that this can be configured from modules/gdscript. GDScript shouldn't escape its containment room 😈

I do believe that it should be possible to move this to GDScript code somehow.

Can't it be done here?

Ref<Resource> ResourceFormatLoaderGDScript::load(const String &p_path, const String &p_original_path, Error *r_error, bool p_use_sub_threads, float *r_progress, CacheMode p_cache_mode) {
        if (r_error) {
                *r_error = ERR_FILE_CANT_OPEN;
        }

        Error err;
        Ref<GDScript> scr = GDScriptCache::get_full_script(p_path, err, "", p_cache_mode == CACHE_MODE_IGNORE);

        // TODO: Reintroduce binary and encrypted scripts.

        if (scr.is_null()) {
                // Don't fail loading because of parsing error.
                scr.instantiate();
        }

        if (r_error) {
                *r_error = OK;
        }

        return scr;
}

Might require more work to figure out if it's failing loading due to parsing errors or due to cyclic dependencies (maybe this can be passed via err in get_full_script().

@adamscott
Copy link
Copy Markdown
Member Author

adamscott commented Jan 12, 2023

Can't it be done here?

No. :(

Considering that a.gd is the script of a.tscn:

  1. a.gd loads by ResourceLoader::load()
  2. ResourceLoader::load_threaded_get() checks if thread_load_tasks has a.gd. It has not.
  3. ResourceFormatLoaderGDScript::load() runs on a.gd, calls GDScriptCache::get_full_script()
  4. a.gd refers to b.gd
  5. b.gd preloads a.tscn
  6. a.tscn loads by ResourceLoader::load()
  7. ResourceLoader::load() tries to load a.gd (again)
  8. ResourceLoader::load_threaded_get() checks if thread_load_tasks has a.gd. It has.
  9. ResourceLoader::load() returns a null res

ResourceFormatLoaderGDScript::load() is not called if thread_load_tasks has already a file.

Otherwise, if it was called, GDScriptCache::get_full_script() supports cyclic references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Circular dependencies of scenes (by preload) cause a script can not be loaded

4 participants