Don't abort loading when ext_resource is missing

[KoBeWi]

Fixes #85152
Bugsquad edit: Fixes #86154
Resource loaders have inconsistent error values in case of missing files. If a loader returns error that indicates missing file, it's safe to ignore it; the resource will be replaced by null when you Open Anyway.

There are more errors related to files, not sure if ignore them all (the premise is technically the same).

[RandomShaper]

I think that Open anyway should set (and later unset) ResourceLoader::set_abort_on_missing_resources(). To me, that's already existing functionality to be able to load despite external resources missing. However, the branch when that's disabled, which calls ResourceLoader::notify_dependency_error() should maybe set err (the one that will be returned) to OK.

[KoBeWi]

Changed to use set_abort_on_missing_resources(). The name feels backwards, unless I'm misunderstanding it.

[RandomShaper]

Quoting current usages:

godot/scene/resources/resource_format_text.cpp

Lines 160 to 167 in c2f8fb3

	if (ResourceLoader::get_abort_on_missing_resources()) {
	error = ERR_FILE_MISSING_DEPENDENCIES;
	error_text = "[ext_resource] referenced non-existent resource at: " + path;
	_printerr();
	err = error;
	} else {
	ResourceLoader::notify_dependency_error(local_path, path, type);
	}

godot/scene/resources/resource_format_text.cpp

Lines 469 to 476 in c2f8fb3

	if (ResourceLoader::get_abort_on_missing_resources()) {
	error = ERR_FILE_CORRUPT;
	error_text = "[ext_resource] referenced non-existent resource at: " + path;
	_printerr();
	return error;
	} else {
	ResourceLoader::notify_dependency_error(local_path, path, type);
	}

godot/core/io/resource_format_binary.cpp

Lines 455 to 460 in c2f8fb3

	if (!ResourceLoader::get_abort_on_missing_resources()) {
	ResourceLoader::notify_dependency_error(local_path, external_resources[erindex].path, external_resources[erindex].type);
	} else {
	error = ERR_FILE_MISSING_DEPENDENCIES;
	ERR_FAIL_V_MSG(error, "Can't load dependency: " + external_resources[erindex].path + ".");
	}

godot/core/io/resource_format_binary.cpp

Lines 688 to 693 in c2f8fb3

	if (!ResourceLoader::get_abort_on_missing_resources()) {
	ResourceLoader::notify_dependency_error(local_path, path, external_resources[i].type);
	} else {
	error = ERR_FILE_MISSING_DEPENDENCIES;
	ERR_FAIL_V_MSG(error, "Can't load dependency: " + path + ".");
	}

So, abort on missing resources, if true, means reporting an error to the caller. If false, it means send the broken dep error and otherwise ignore.

---

Now, EditorNode::load_resource and EditorNode::load_scene already provide the ignore-broken-deps functionality (bool p_ignore_broken_deps). I'd say that it's the responsibility of those two functions to use ResourceLoader::set_abort_on_missing_resources() depending on the value of that parameter:

if (p_ignore_broken_deps) {
    ResourceLoader::set_abort_on_missing_resources(false);
}

// Load, etc.

if (!p_ignore_broken_deps && dependency_errors.has(p_resource)) {
    // Show dependency dialog. There will be dep errors, but we won't enter this branch.
}

if (p_ignore_broken_deps) {
    ResourceLoader::set_abort_on_missing_resources(true); // Restore.
}

What do you think?

[KoBeWi]

Makes sense, changed.

[KoBeWi]

Reverted to my original fix. The only call for set_abort_on_missing_resources() (not counting some web code) is this:

godot/editor/editor_node.cpp

Line 6919 in 1faf2f5

ResourceLoader::set_abort_on_missing_resources(false);

The the abort is already disabled in the editor, and according to your comments it is working as intended, so the problem is text format code not handling it properly. Also the new fix was breaking binary format, so it was bad anyway.

[RandomShaper]

ResourceLoaderBinary will error with ERR_FILE_MISSING_DEPENDENCIES in case an external resource can't be loaded, regardless the reason. It may make sense that loading of text resources was more permissive because of certain specific needs in the editor. However, for consistency I'd still model that with some kind of global setting that both can obey. If abort-on-missing-resources is too limiting by being a boolean, could it maybe be a three-item enum, with a new value that would behave like the current false, only that it won't either abort nor report broken deps in cases like a missing file?

In any case, I'm willing to attempt to add such a refactor myself in another PR. If this PR is already enough to fix a known issue, I won't oppose to merging it and we can keep the discussion over actual code showing what I'm trying to tell.

PS: I've realized why abort-on-missing-resources looks reversed. It depends on what is aborted. The editor sets to false so the resource loader doesn't abort, but the editor itself will by showing the broken deps dialog instead of the scene.

[akien-mga]

Thanks!

[tlobig]

fixes #86154

[neruthes]

I had a similar problem where no preload is involved. My guess is that strong-type variables are causing cyclic GDScript parsing. Consider some code like this:

# hive.gd
extends Node2D
class_name MyHive
var bee : MyBee = null

# bee.gd
extends Node2D
class_name MyBee
var hive : MyHive = null

After removing a type notation in the attached GDScript, my scene is working again.

I wish that this PR also fixes any problem that may arise in this hypothetical situation. I will report back if the problem persists in 4.3.

[KoBeWi]

GDScript does support cyclic dependencies since 4.0. Also it shouldn't be affecting scenes...

[neruthes]

GDScript does support cyclic dependencies since 4.0. Also it shouldn't be affecting scenes...

That is a surprise to me. I will try making a minimal working example.

[KoBeWi]

Though by cyclic dependencies I mean scripts only. Preloading other resources, especially scenes, in a cyclic way won't work and will make the scenes non-openable until you remove the preloads.

[neruthes]

Though by cyclic dependencies I mean scripts only. Preloading other resources, especially scenes, in a cyclic way won't work and will make the scenes non-openable until you remove the preloads.

Thanks for the clarification. I had another look into my scripts and figured out that a script contained a preload which I failed to notice.

I found the correct way to reproduce the problem and you may have a look.

In summary:

• Hive.gd (class_name Hive) contains preload Bee.tscn (var bee2: Bee = preload("res://main/Bee.tscn").instantiate()).
• Bee.tscn uses Bee.gd (class_name Bee).
• Bee.gd has a varaible typed Hive (var master : Hive = null).
• Boom! Cannot open Bee.tscn in editor, but running game with F5 is ok.

[KoBeWi]

Thanks, there's already an existing issue about this bug, I attached your project there: #79545 (comment)

[neruthes]

Thanks, there's already an existing issue about this bug, I attached your project there: #79545 (comment)

Thank you. You have done great help in this PR.

[akien-mga]

Cherry-picked for 4.2.2.