godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.95k stars 21.15k forks source link

Packed child scenes with same UID will not work in a confusing way (one wins out, race condition as to which) #91097

Open lowagner opened 6 months ago

lowagner commented 6 months ago

Tested versions

i've reproduced in 4.2.2 and 4.2.1 and 4.2 stable

System information

Godot v4.2.2.stable - Pop!_OS 22.04 LTS - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3050 Laptop GPU () - 13th Gen Intel(R) Core(TM) i9-13900H (20 Threads)

Issue description

i've created a PackedScene with different PackedScenes inside, but the packed scenes don't retain their packed scene type when running the game. they will convert into one or the other when running the game.

Steps to reproduce

starting with https://github.com/lowagner/triangle/commit/5ee6351fbadedfec994f52531356dd6ba6a576e1 and loading the project in the godot4 subdirectory. opening the project is fine, if you go into the Room packed scene, it's an instance of TestRoom. inside the room are a bunch of boxes plus a "MagicBox" at the end.

according to the editor, they are all the same type of PackedScene, a terrain/static-box.tscn scene (TerrainStaticBox class). however, i have created this test scene by creating a bunch of the TerrainStaticBox packed scenes, plus adding a MagicBox scene (from characters/magic-box/magic-box.tscn). it's saved at this point in the git versioning. you can see this if you look at the levels/test-room.tscn file, which has both MagicBox and TerrainStaticBox scenes loaded (e.g., inspect the commit above).

now's where it gets interesting/annoying. after opening up TestRoom in the editor, start the game. all the static boxes in the test room should remain static; the magic box should be a dynamic rigid body. HOWEVER, what actually happens is that everyone becomes either the MagicBox scene or the TerrainStaticBox scene. (if the former, since the magic box has rigidbody physics, everything will start dropping under the influence of gravity. if the latter, everything is static, including the magic box.) even without saving, you'll notice a git diff -- it removed the MagicBox external resource type and made everything a StaticBox type (even if everyone was acting as the MagicBox scene). if i clear my godot cache (rm -r .godot) and reload the project, then everything is static of course.

you can also try to add another MagicBox to the TestRoom scene (Ctrl+Shift+A -> then add magic-box.tscn). it will look correct until you try running the game, at which point it will convert into a TerrainStaticBox.

it's clear that i don't want this behavior; i want the editor to know that the MagicBox is a different scene and shouldn't affect the other packed scenes, and similarly for TerrainStaticBox. here is an example git diff after running the game, even without changing anything in the editor. you need to open TestRoom in the editor before it will show this diff:

diff --git a/godot4/levels/test-room.tscn b/godot4/levels/test-room.tscn
index 97fc818..6adb555 100644
--- a/godot4/levels/test-room.tscn
+++ b/godot4/levels/test-room.tscn
@@ -1,8 +1,7 @@
-[gd_scene load_steps=4 format=3 uid="uid://b0fpxqceogy4a"]
+[gd_scene load_steps=3 format=3 uid="uid://b0fpxqceogy4a"]

 [ext_resource type="Script" path="res://levels/test-room.gd" id="1_0n2oq"]
-[ext_resource type="PackedScene" uid="uid://7dqtlcu57oiv" path="res://characters/magic-box/magic-box.tscn" id="3_8rxya"]
-[ext_resource type="PackedScene" uid="uid://7dqtlcu57oiv" path="res://terrain/static-box.tscn" id="3_yhpnb"]
+[ext_resource type="PackedScene" uid="uid://7dqtlcu57oiv" path="res://terrain/static-box.tscn" id="3_8rxya"]

 [node name="TestRoom" type="Node2D"]
 script = ExtResource("1_0n2oq")
@@ -14,22 +13,22 @@ z_index = -100
 color = Color(0.419608, 0.784314, 0.643137, 1)
 polygon = PackedVector2Array(-2048, 2048, 2048, 2048, 2048, -2048, -2048, -2048)

-[node name="StartBox" parent="." instance=ExtResource("3_yhpnb")]
+[node name="StartBox" parent="." instance=ExtResource("3_8rxya")]
 position = Vector2(150, 350)
 size = Vector2(256, 128)

-[node name="CrookedBox" parent="." instance=ExtResource("3_yhpnb")]
+[node name="CrookedBox" parent="." instance=ExtResource("3_8rxya")]
 position = Vector2(284, 604)
 rotation = 0.785398
 size = Vector2(50, 128)
 color = Color(0.858824, 0.188235, 0.466667, 1)

-[node name="ThinBox" parent="." instance=ExtResource("3_yhpnb")]
+[node name="ThinBox" parent="." instance=ExtResource("3_8rxya")]
 position = Vector2(518, 434)
 size = Vector2(64, 256)
 color = Color(0.882353, 0.882353, 0.882353, 1)

-[node name="GroundBox" parent="." instance=ExtResource("3_yhpnb")]
+[node name="GroundBox" parent="." instance=ExtResource("3_8rxya")]
 position = Vector2(451, 806)
 size = Vector2(2000, 96)
 color = Color(0.737255, 0.396078, 0.211765, 1)

Minimal reproduction project (MRP)

N/A yet. i'll work on seeing if i can simplify https://github.com/lowagner/triangle/commit/5ee6351fbadedfec994f52531356dd6ba6a576e1 if this issue isn't already known.

huwpascoe commented 6 months ago

It should be impossible (maybe the files were just duplicated outside the editor?) but somehow your static box and magic box have ended up with the same UID.

Easiest way to fix this is to create a new scene, copy paste the contents of static box into it, and then replace all references to the original throughout the project.

Depending on... stuff, sometimes the godot uses the res path, sometimes the UID. This is why the error is so inconsistent. This is a bug though, I think. There should be some sort of check to make sure UIDs are actually unique when processing resources.

AThousandShips commented 6 months ago

Indeed, this looks like you probably copied the file manually at some point, which isn't safe, you need to fix this by removing the uid from the invalid source file

This shouldn't happen when you copy or duplicate resources in the editor, if it does happen that part is a bug, but handling users manually adding files with already used UIDs is complicated and will naturally break things

lowagner commented 6 months ago

nope, never edited the tscn manually (except to revert the automatic changes which deleted the MagicBox import and converted everyone to StaticBox import via git reset --hard HEAD && rm -r .godot). this is probably a bug in the loading logic. is there a way to debug how the tscns are loaded?

AThousandShips commented 6 months ago

Didn't mean you edited it manually, did you create the two different resources by copying one manually? Or did you duplicate them using the editor?

lowagner commented 6 months ago

some of the StaticBoxes are duplicates of each other, but i'm pretty sure i added the MagicBox via Ctrl+Shift+A. either way, i can consistently repro the issue on my side if i try to add another MagicBox to the TestScene. are you able to repro at all?

AThousandShips commented 6 months ago

Can yo repo it by duplicating a scene in the editor?

I don't have an MRP to repo from so I haven't tested, your project is not minimal, so I'd suggest trying to reproduce this in a new project and uploading it here

lowagner commented 6 months ago

i'd prefer to make sure it's not something in my configuration before i invest in making a MRP. the project isn't that big right now so it'd be a great help to know it's not just my machine if you'd be willing to try.

i may have duplicated the scene via the editor (e.g., in the FileSystem inspector). but why would that break things when it is distinct now?

AThousandShips commented 6 months ago

If you make an MRP and it works then that's a sign it is in your configuration, but without doing that you're essentially asking people to debug your project, rather than testing engine issues, so it'd be very appreciated if you put in the work to test it as you have the material to do so

Duplicating from the editor should work without causing these problems, it's been fixed in the past, that's why it sounds like you copy-pasted it outside the editor, therefore causing this, so just checking a simple project would be simple here, create a scene, and duplicate it, and look at the UID of both files, if they match that's a bug in the engine

lowagner commented 6 months ago

ah i think see the problem. duplicating the scene gave the same UID to both packed scenes. i'll go in and edit them to be different. they both had the same internal structure, however, which is maybe why they hashed to the same value.

i don't think i copy-pasted from outside the editor but can test.

AThousandShips commented 6 months ago

ah i think see the problem. duplicating the scene gave the same UID to both packed scenes.

Indeed, but that was fixed before, so I unless it's popped back up it probably isn't the cause, it will however happen if you copy-paste the resources, so if you can confirm it happens specifically when doing copying or duplicating in the editor then that'd be a good hint, but that was supposed to be fixed before 4.2 AFAIK

lowagner commented 6 months ago

ok that probably was the cause (copy-pasting the file outside of Godot). i deleted the magic-box.tscn file and duplicated it via the editor instead of by hand, and now it's working fine.

do we have any proposals to notice when a newly loaded file has the same UID as another one? this would have prevented some headache/confusion here.

AThousandShips commented 6 months ago

Thank you for confirming! This might be tracked in a different issue report, but this one will do for now if there isn't another one already, it's not so much a new proposed feature as it's a more robust system to handle cases like this, so it belongs here on the issue tracker

The problem is that it's not entirely clear how to handle it generally

Now if you do copy a file there's an original around and you can pick what file to connect the UID to, but what if you copy it and rename the original? Which should be used now? Etc., so the specific solution is a bit complicated

lowagner commented 6 months ago

i think it'd be nice to just notice that the UID is the same, warn at this point (e.g., "Warning: file magic-box.tscn has the same UID as static-box.tscn"). for people who are manually changing files that's probably enough to cue them on to the problem and that they need to update the UID in one and then update its references everywhere.

we don't necessarily need a full solution if this can't happen via the editor alone.