GDScript: Fix loading of GDScript resource imported from another file extension

[dalexeev]

• Fixes Can't load properly GDScript that is generated code saved by ResourceSaver.save in EditorImportPlugin #105299.

[dalexeev]

Please note that there may be a bug in the core. It's somewhat odd that ResourceFormatLoaderGDScript is called for the original path, which has an extension not supported by the loader (because the actual path is checked, not the original one). However, I didn't investigate this possibility due to the scale of the implications.

godot/core/io/resource_loader.cpp

Lines 306 to 318 in d5edd4a

	// Try all loaders and pick the first match for the type hint
	bool found = false;
	Ref<Resource> res;
	for (int i = 0; i < loader_count; i++) {
	if (!loader[i]->recognize_path(p_path, p_type_hint)) {
	continue;
	}
	found = true;
	res = loader[i]->load(p_path, original_path, r_error, p_use_sub_threads, r_progress, p_cache_mode);
	if (res.is_valid()) {
	break;
	}
	}

godot/modules/gdscript/gdscript.cpp

Lines 2918 to 2921 in d5edd4a

	void ResourceFormatLoaderGDScript::get_recognized_extensions(List<String> *p_extensions) const {
	p_extensions->push_back("gd");
	p_extensions->push_back("gdc");
	}

[HolonProduction]

Suggested change

	Ref<GDScript> scr = GDScriptCache::get_full_script(target_path, err, "", ignoring);
	Ref<GDScript> scr = GDScriptCache::get_full_script(ResourceLoader::import_remap(p_original_path), err, "", ignoring);

Wouldn't this basically be the same?

[dalexeev]

I'm not sure. GDScriptCache::get_full_script() already calls ResourceLoader::path_remap(), but that's a different method and doesn't help. Perhaps there's a difference between importers and format loaders.

godot/modules/gdscript/gdscript_cache.cpp

Line 365 in 780b689

const String remapped_path = ResourceLoader::path_remap(p_path);

[HolonProduction]

As far as I understand the system, there are two parts to how a path maps to something that can be loaded from the pck.

Path changes that happen at import time. And path changes which happen at export time.

I suspect path_remap only takes care of the later, while import_remap takes care of the former. For a full transformation they need to be chained. This seems to be supported by my limited testing:

p_path:  res://TestScene.gdc
p_original_path:  res://TestScene.gd
ResourceLoader::import_remap(p_original_path):  res://TestScene.gd
ResourceLoader::path_remap(p_original_path):  res://TestScene.gdc
ResourceLoader::path_remap(ResourceLoader::import_remap(p_original_path)):  res://TestScene.gdc

p_path:  res://.godot/imported/import_test_target.test-f75ad55ac69400bc5d60d5af43574001.gd
p_original_path:  res://import_test_target.test
ResourceLoader::import_remap(p_original_path):  res://.godot/imported/import_test_target.test-f75ad55ac69400bc5d60d5af43574001.gd
ResourceLoader::path_remap(p_original_path):  res://import_test_target.test
ResourceLoader::path_remap(ResourceLoader::import_remap(p_original_path)):  res://.godot/imported/import_test_target.test-f75ad55ac69400bc5d60d5af43574001.gd

[HolonProduction]

Also worth noting, that the export plugin which does binary tokenization does not seem to account for import remaps. Which is why using p_path has the same behaviour as import_remap(original_path). But in theory if there was a path that was import and export remapped they would not be equivalent from my understanding

[HolonProduction]

The main issue I have with this, is that it will result in a state were the GDScriptCache and ResourceCache reference the script with different paths. Desyncs between the both caches were responsible for script duplication issues in the editor. Without further investigation I'm not confident that replacing the 1:1 mapping of keys, with a gdscript_key = import_remap(resource_key) mapping is safe. And even if so, it would add even more mental overhead to the system.

IMO if we want to support this on a best effort basis we should use the .test path for the cache and use import_remap in addition to path_remap. If we are not comfortable with this (because of all the hardcoded extension checks that might not take remapping into account) we should just detect the case and throw an explicit error: "GDScript code generation should not be done in the context of the import system. GDScript needs a canonical .gd file in the project."

[dalexeev]

If we are not comfortable with this (because of all the hardcoded extension checks that might not take remapping into account) we should just detect the case and throw an explicit error

I had this idea, but I'm unsure about choosing between strict behavior and best effort support (without guarantees the absence of weird bugs). I submitted this PR as is because: 1. the issue is labeled as a regression, and 2. at first glance, this fix looks like it should work.

Given that you also support a stricter option, I'll probably update the PR as you suggested. This also makes sense in the long term (if we abandon the "script is a resource" approach in Godot 5).

However, we still face the question: should we make this a strict error or just a warning? For example, we could use the WARN_PRINT_ONCE macro to warn the user about an unsupported file extension and still attempt to gracefully handle this case, albeit without guarantees of correct cache system operation.

[HolonProduction]

I think the best middle ground right now would be:

• print a warning
• add a comment that explains that the code is on best effort basis (otherwise someone will try to fix or rework something and will carry this as tech debt into other parts of the code)
• use import_remap over p_path
  • p_path is also path_remaped, so it could in theory point to a .gdc file
  • so only import remapping p_original_path feels like the solution which is most faithful to our existing code to me