PR #84167 seems to cause corruption in scene files with exported references to PackedScenes

[emklasson]

Tested versions

Reproducible on custom build from main 4.3.0-dev f8a2a91.
Not reproducible if 2e94be2 is reverted.

System information

Godot v4.3.dev (30b60b6f9) - macOS 13.6.1 - Vulkan (Mobile) - integrated Apple M1 - Apple M1 (8 Threads)

Issue description

Running a scene A with an exported reference to a PackedScene B causes corruption in A's scene file. It seems to embed the B scene as a SubResource instead. In an actual project I've also seen it randomly regenerate ids in scene files.

This seems to only happen the first time the project is run after starting the editor.

Steps to reproduce

1. Build Godot with "scons platform=macos arch=arm64".
2. Start the editor.
3. Open the attached MRP.
4. Run the project.
5. Observe corrupted a.tscn file:

Minimal reproduction project (MRP)

PR84167bug_MRP_20231215_1406.zip

[YuriSizov]

cc @SaracenOne @KoBeWi

[KoBeWi]

I managed to reproduce it only once. Seems to be random, making it difficult to test.

[emklasson]

I managed to reproduce it only once. Seems to be random, making it difficult to test.

Just to make sure, did you restart the editor for each test? It only ever happens on the first run for me, but has never failed to happen on a first run in 10+ tests.

[KoBeWi]

Yes, I did restart the editor multiple times.

[LunaticInAHat]

It does not occur consistently upon opening the project for me, either, however there is a series of steps that seems to cause it to reproduce consistently (on my machine):

1. Unzip the MRP, open it in Godot
2. Select the A root node of a.tscn
3. Click on its B property in the Inspector
4. Click on "Open Scene"
5. Switch back to the a.tscn tab
6. Save the scene
7. Observe corruption of a.tscn (also note that the "Open Scene" button becomes permanently grayed out)

[emklasson]

It does not occur consistently upon opening the project for me, either, however there is a series of steps that seems to cause it to reproduce consistently (on my machine):

Good point. I realise now my particular configuration of open tabs is not preserved in the zip. Sorry, I didn't think about that.

The bug seems to only trigger when both a.tscn and b.tscn are open in tabs (with A in the first tab and active, if that matters too).

Only having the a.tscn tab open does not trigger the bug. It seems the act of opening/loading the b.tscn tab corrupts a.tscn's reference to b.tscn.

It's also repeatable in that you don't have to quit the editor:

1. Open a.tscn in a new tab
2. Open b.tscn in a new tab
3. Switch back to the a.tscn tab
4. Save the a.tscn tab
5. The a.tscn file is now corrupted
6. Close the b.tscn tab
7. Revert changes to a.tscn
8. Reload a.tscn when prompted in editor
9. Goto 2

[LunaticInAHat]

The bug can occur under other circumstances, but I have not been able to identify them with any certainty. In my own project, I have a Resource-derived class that exports references to PackedScenes, and those .tres files run into the same kind of corruption with some regularity. My point being: It may not be as simple as "have both scenes open in tabs", because it can afflict non-scene resources as well. It probably happens when I have the .tres open in the Inspector and then open one of the referenced scenes, but I can't confirm that right now.

[emklasson]

Fwiw, forcing
p_cache_mode = ResourceFormatLoader::CACHE_MODE_REUSE;
first thing in ResourceLoader::_load_start seems to prevent the bug from triggering.

As best I can tell it's the

godot/core/io/resource_loader.cpp

Line 343 in bf8dd73

load_task.resource->set_path(load_task.local_path, load_task.cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE);

call that's breaking a.tscn during the loading of b.tscn.

Digging deeper, it seems to me like resetting the path_cache of the existing b.tscn resource (referenced in a.tscn) might be causing serious issues, but I don't really know the code base at all:

godot/core/io/resource.cpp

Lines 69 to 77 in bf8dd73

	if (existing.is_valid()) {
	if (p_take_over) {
	existing->path_cache = String();
	ResourceCache::resources.erase(p_path);
	} else {
	ResourceCache::lock.unlock();
	ERR_FAIL_MSG("Another resource is loaded from path '" + p_path + "' (possible cyclic resource inclusion).");
	}
	}

This empty path_cache will then cause Resource::get_path to return an empty string and Resource::is_built_in to return true, both of which seem problematic?

What's supposed to happen to the existing resource when it's evicted from the ResourceCache? Should it somehow get consolidated with the new one or keep hanging around?

Simply commenting out the line resetting the existing->path_cache seems to stop the bug from triggering in my MRP but I'm not sure what consequences that has.

[KoBeWi]

Ok so there is a problem with CACHE_MODE_REPLACE. It causes the file to get loaded from disk and take over a path. The problem with this is that if any other instance of this resource exists, it will be disconnected from the original. This behavior might be fine at runtime, but in editor it leads to bugs like this one.

When resource is reloaded, it should instead update the existing instances, not replace the cache as if they didn't exist. There is copy_from() method that can be used.

[GameDevLlama]

This issue could be a current show-stopper for many who are trying 4.3 at the moment. I managed to destroy a lot of PackedScenes. Good I'm using VCS, but sometimes the error happens without taking notice and can cause weird bugs:

For example intentionally removing a node from a scene that is being used as PackedScene somewhere else, but it will still spawn when playing that scene. (if it has been embedded already)

[akien-mga]

See #86918 for another MRP.

[donn-xx]

Fwiw, forcing p_cache_mode = ResourceFormatLoader::CACHE_MODE_REUSE; first thing in ResourceLoader::_load_start seems to prevent the bug from triggering.

It stops the bug, but something's not right under the hood. You get this error when you open the scene (the one that is being loaded elsewhere as a PackedScene) in the editor:

ERROR: Another resource is loaded from path 'res://sphere_scene.tscn' (possible cyclic resource inclusion).