Add cyclic loading abstraction to ResourceLoader

[adamscott]

This creates an abstraction for ResourceLoader and ResourceFormatLoader to load cyclic resources, ie. ressources currently loading that normally returns a null Resource when queried.

Supersedes #71004
Fixes #70985
Fixes #71610

[akien-mga]

This seems wrong, it will be overridden below by

	if (r_error) {
		*r_error = OK;
	}

[akien-mga]

Semantically, it's also weird to set an error in an external variable before actually attempting to load the script which may not at all be an error. It's less descriptive for the reader, and could lead to bugs if there are race conditions?

[akien-mga]

I see pre-existing code does the same though with

	if (r_error) {
		*r_error = ERR_FILE_CANT_OPEN;
	}

It's a bit weird.
Normally we'd use a local Error err to keep track of the error, and only set r_error before returning (on failure, or at the end of the function on success).

[adamscott]

All loaders use a pointer for errors instead of editing the reference itself. It's because it's editing the value of the ResourceLoader::ThreadLoadTask struct error value.

See this example:

godot/core/io/image_loader.cpp

Lines 150 to 157 in 629796c

	Ref<Resource> ResourceFormatLoaderImage::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) {
	Ref<FileAccess> f = FileAccess::open(p_path, FileAccess::READ);
	if (f.is_null()) {
	if (r_error) {
	*r_error = ERR_CANT_OPEN;
	}
	return Ref<Resource>();
	}

[akien-mga]

Can't get_full_script() tell you that it can't get a full script due to cyclic dependency? Instead of having to make an educated guess.

[adamscott]

Loaders must make educated guesses because when loading, they don't have control anymore.

[adamscott]

@akien-mga @vnen The code now tests if the file can be loaded. If so, the error is changed to be a cyclic load error.

It will not affect other loaders and there's no presumption to be done about ERR_FILE_CANT_OPEN.

https://github.com/godotengine/godot/blob/b288a60e51b95dfc433cc705ba9ba519d14d2d9f/modules/gdscript/gdscript.cpp#L2651-L2670

[adamscott]

My PR fixes #71610 too.

[reduz]

I am still not convinced in the approach or whether this a problem, if you cyclic preloads() with scenes, then you will not be able to easily free them eventually. I think its correct that it fails and should not be fixed.

The error you receive should let you know that this is a problem and that you should use load() instead and take care of properly freeing this scene manually to break the cyclic link.

[adamscott]

@reduz If I would happen to create a global scope function to free cyclic references, like you wanted to implement, would this PR legitimacy be impacted positively?

The usage of the issue #70985 seems to be legitimate. a.gd is loaded, refers to b.gd that preloads a.tscn scene that a.gd happens to be the script. It causes that the a.tscn scene will lose it's script.

I'll suggest the creation of the Squirrel-like function tomorrow at our GDScript meeting.

[reduz]

@adamscott Circular reference for scripts should be valid to some extent, because you can have a reference to the parser code (at least how it was meant to be implemented when we discussed about this with @vnen) however cyclic reference for resources is not because its way too difficult to fix and extremely error prone. As such the use case described in the linked issue should always be considered a bug.

[reduz]

Creating code that checks scripts and see if they contain cyclical references and eventually break them is not very difficult, because all you have to do is eliminate the scripts from the objects that contain it and were marked as unused (hence the cyclical link breaks and the objects are freed).

Extending this to the whole resource system is extremely inefficient and would force users to have to do this check for cyclical inclusion errors constantly when instead you can just prevent them on load.

[adamscott]

Closing this PR. The GDScript team will have to discuss how to fix this issue otherwise, taking @reduz comments into account.