Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TSCN/TRES subresource random reordering of CollisionPolygon2Ds #5889

Closed
akien-mga opened this issue Jul 24, 2016 · 14 comments
Closed

TSCN/TRES subresource random reordering of CollisionPolygon2Ds #5889

akien-mga opened this issue Jul 24, 2016 · 14 comments
Milestone

Comments

@akien-mga
Copy link
Member

Operating system or device - Godot version:
Mageia 6 x64, git master HEAD

Issue description (what happened, and what was expected):
Loading a tileset source TSCN and saving it, and/or exporting/saving again an existing TRES tileset resource generates random redistribution of the subresources' IDs, leading to unnecessary diffs.

Steps to reproduce:

  1. Download the attached minimal example, which is a zipped git repo (so that changes can be seen with git diff)
  2. Load mountain_tileset.tscn in the editor, export the TileSet to overwrite mountain_tileset.tres (make sure to untick "Merge with existing").
  3. Check the git diff: it might show some random reordering of the subresources in mountain_tileset.tres. If it doesn't, close the editor, and repeat step 2 and 3. It might take several attempts to happen.
  4. Once you have a diff, git add . the changes to take it as the new "reference" (in case the previous state was actually bad).
  5. Repeat steps 2 and 3; depending on your luck, you might see the previous changes being reverted again. Saving TileMap.tscn might also exhibit the issue, as it resaves the TRES resource.

The behaviour is a bit non-deterministic, it kind of looks like the order of the subresources depends on the time, or the way the resources were loaded into memory in the editor, etc.

At any rate, some way should be found to ensure that such resources won't get randomly reordered, and ideally they should be sorted by ascending IDs.

Example diff:

diff --git a/mountain_tileset.tres b/mountain_tileset.tres
index 0a6adab..7ded8df 100644
--- a/mountain_tileset.tres
+++ b/mountain_tileset.tres
@@ -7,82 +7,82 @@
 custom_solver_bias = 0.0
 points = Vector2Array( 0, 24, 64, 24, 64, 128, 0, 128 )

-[sub_resource type="ConvexPolygonShape2D" id=2]
+[sub_resource type="ConvexPolygonShape2D" id=10]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 128, 0, 128, 32, 96, 64, 0, 64 )

-[sub_resource type="ConvexPolygonShape2D" id=3]
+[sub_resource type="ConvexPolygonShape2D" id=11]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 80, 0, 128, 24, 128, 128, 0, 128 )

-[sub_resource type="ConvexPolygonShape2D" id=4]
+[sub_resource type="ConvexPolygonShape2D" id=12]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 96, 0, 128, 16, 128, 40, 96, 128, 0, 128 )

-[sub_resource type="ConvexPolygonShape2D" id=5]
+[sub_resource type="ConvexPolygonShape2D" id=13]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 16, 16, 16, 0, 40 )

-[sub_resource type="ConvexPolygonShape2D" id=6]
+[sub_resource type="ConvexPolygonShape2D" id=14]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 16, 128, 16, 128, 40, 0, 40 )

-[sub_resource type="ConvexPolygonShape2D" id=7]
+[sub_resource type="ConvexPolygonShape2D" id=15]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 24, 48, 24, 64, 56, 64, 192, 0, 192 )

-[sub_resource type="ConvexPolygonShape2D" id=8]
+[sub_resource type="ConvexPolygonShape2D" id=16]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 56, 128, 56, 128, 192, 0, 192 )

-[sub_resource type="ConvexPolygonShape2D" id=9]
+[sub_resource type="ConvexPolygonShape2D" id=17]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 56, 64, 56, 64, 192, 0, 192 )

-[sub_resource type="ConvexPolygonShape2D" id=10]
+[sub_resource type="ConvexPolygonShape2D" id=2]

 custom_solver_bias = 0.0
 points = Vector2Array( 128, 192, 0, 192, 0, 24, 64, 48, 128, 88 )

-[sub_resource type="ConvexPolygonShape2D" id=11]
+[sub_resource type="ConvexPolygonShape2D" id=3]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 24, 64, 88, 64, 192, 0, 192 )

-[sub_resource type="ConcavePolygonShape2D" id=12]
+[sub_resource type="ConcavePolygonShape2D" id=4]

 custom_solver_bias = 0.0
 segments = Vector2Array( 0, 24, 64, 72, 64, 72, 128, 88, 128, 88, 128, 192, 128, 192, 0, 192, 0, 192, 0, 24 )

-[sub_resource type="ConvexPolygonShape2D" id=13]
+[sub_resource type="ConvexPolygonShape2D" id=5]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 24, 128, 24, 96, 128, 0, 128 )

-[sub_resource type="ConvexPolygonShape2D" id=14]
+[sub_resource type="ConvexPolygonShape2D" id=6]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 96, 0, 96, 128, 0, 128 )

-[sub_resource type="ConvexPolygonShape2D" id=15]
+[sub_resource type="ConvexPolygonShape2D" id=7]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 96, 0, 72, 32, 0, 32 )

-[sub_resource type="ConvexPolygonShape2D" id=16]
+[sub_resource type="ConvexPolygonShape2D" id=8]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 128, 0, 128, 32, 0, 32 )

-[sub_resource type="ConvexPolygonShape2D" id=17]
+[sub_resource type="ConvexPolygonShape2D" id=9]

 custom_solver_bias = 0.0
 points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
@@ -112,7 +112,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 2/occluder_offset = Vector2( 0, 0 )
 2/navigation_offset = Vector2( 0, 0 )
 2/shape_offset = Vector2( 0, 0 )
-2/shapes = [ SubResource( 10 ) ]
+2/shapes = [ SubResource( 2 ) ]
 3/name = "Ramp"
 3/texture = ExtResource( 1 )
 3/tex_offset = Vector2( 0, 0 )
@@ -120,7 +120,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 3/occluder_offset = Vector2( 0, 0 )
 3/navigation_offset = Vector2( 0, 0 )
 3/shape_offset = Vector2( 0, 0 )
-3/shapes = [ SubResource( 11 ) ]
+3/shapes = [ SubResource( 3 ) ]
 4/name = "Ramp End"
 4/texture = ExtResource( 1 )
 4/tex_offset = Vector2( 0, 0 )
@@ -128,7 +128,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 4/occluder_offset = Vector2( 0, 0 )
 4/navigation_offset = Vector2( 0, 0 )
 4/shape_offset = Vector2( 0, 0 )
-4/shapes = [ SubResource( 12 ) ]
+4/shapes = [ SubResource( 4 ) ]
 5/name = "Edge"
 5/texture = ExtResource( 1 )
 5/tex_offset = Vector2( 0, 0 )
@@ -136,7 +136,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 5/occluder_offset = Vector2( 0, 0 )
 5/navigation_offset = Vector2( 0, 0 )
 5/shape_offset = Vector2( 0, 0 )
-5/shapes = [ SubResource( 13 ) ]
+5/shapes = [ SubResource( 5 ) ]
 6/name = "Wall"
 6/texture = ExtResource( 1 )
 6/tex_offset = Vector2( 0, 0 )
@@ -144,7 +144,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 6/occluder_offset = Vector2( 0, 0 )
 6/navigation_offset = Vector2( 0, 0 )
 6/shape_offset = Vector2( 0, 0 )
-6/shapes = [ SubResource( 14 ) ]
+6/shapes = [ SubResource( 6 ) ]
 7/name = "Ceiling2Wall"
 7/texture = ExtResource( 1 )
 7/tex_offset = Vector2( 0, 0 )
@@ -152,7 +152,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 7/occluder_offset = Vector2( 0, 0 )
 7/navigation_offset = Vector2( 0, 0 )
 7/shape_offset = Vector2( 0, 0 )
-7/shapes = [ SubResource( 15 ) ]
+7/shapes = [ SubResource( 7 ) ]
 8/name = "Ceiling"
 8/texture = ExtResource( 1 )
 8/tex_offset = Vector2( 0, 0 )
@@ -160,7 +160,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 8/occluder_offset = Vector2( 0, 0 )
 8/navigation_offset = Vector2( 0, 0 )
 8/shape_offset = Vector2( 0, 0 )
-8/shapes = [ SubResource( 16 ) ]
+8/shapes = [ SubResource( 8 ) ]
 9/name = "CeilingRamp"
 9/texture = ExtResource( 1 )
 9/tex_offset = Vector2( 0, 0 )
@@ -168,7 +168,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 9/occluder_offset = Vector2( 0, 0 )
 9/navigation_offset = Vector2( 0, 0 )
 9/shape_offset = Vector2( 0, 0 )
-9/shapes = [ SubResource( 17 ) ]
+9/shapes = [ SubResource( 9 ) ]
 10/name = "Wall2Ceiling"
 10/texture = ExtResource( 1 )
 10/tex_offset = Vector2( 0, 0 )
@@ -176,7 +176,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 10/occluder_offset = Vector2( 0, 0 )
 10/navigation_offset = Vector2( 0, 0 )
 10/shape_offset = Vector2( 0, 0 )
-10/shapes = [ SubResource( 2 ) ]
+10/shapes = [ SubResource( 10 ) ]
 11/name = "Ground2Wall"
 11/texture = ExtResource( 1 )
 11/tex_offset = Vector2( 0, 0 )
@@ -184,7 +184,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 11/occluder_offset = Vector2( 0, 0 )
 11/navigation_offset = Vector2( 0, 0 )
 11/shape_offset = Vector2( 0, 0 )
-11/shapes = [ SubResource( 3 ) ]
+11/shapes = [ SubResource( 11 ) ]
 12/name = "Wall2Bridge"
 12/texture = ExtResource( 1 )
 12/tex_offset = Vector2( 0, 0 )
@@ -192,7 +192,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 12/occluder_offset = Vector2( 0, 0 )
 12/navigation_offset = Vector2( 0, 0 )
 12/shape_offset = Vector2( 0, 0 )
-12/shapes = [ SubResource( 4 ) ]
+12/shapes = [ SubResource( 12 ) ]
 13/name = "Peak"
 13/texture = ExtResource( 1 )
 13/tex_offset = Vector2( 0, 0 )
@@ -200,7 +200,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 13/occluder_offset = Vector2( 0, 0 )
 13/navigation_offset = Vector2( 0, 0 )
 13/shape_offset = Vector2( 0, 0 )
-13/shapes = [ SubResource( 5 ) ]
+13/shapes = [ SubResource( 13 ) ]
 14/name = "Bridge"
 14/texture = ExtResource( 1 )
 14/tex_offset = Vector2( 0, 0 )
@@ -208,7 +208,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 14/occluder_offset = Vector2( 0, 0 )
 14/navigation_offset = Vector2( 0, 0 )
 14/shape_offset = Vector2( 0, 0 )
-14/shapes = [ SubResource( 6 ) ]
+14/shapes = [ SubResource( 14 ) ]
 15/name = "StepUp"
 15/texture = ExtResource( 1 )
 15/tex_offset = Vector2( 0, 0 )
@@ -216,7 +216,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 15/occluder_offset = Vector2( 0, 0 )
 15/navigation_offset = Vector2( 0, 0 )
 15/shape_offset = Vector2( 0, 0 )
-15/shapes = [ SubResource( 7 ) ]
+15/shapes = [ SubResource( 15 ) ]
 16/name = "Step"
 16/texture = ExtResource( 1 )
 16/tex_offset = Vector2( 0, 0 )
@@ -224,7 +224,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 16/occluder_offset = Vector2( 0, 0 )
 16/navigation_offset = Vector2( 0, 0 )
 16/shape_offset = Vector2( 0, 0 )
-16/shapes = [ SubResource( 8 ) ]
+16/shapes = [ SubResource( 16 ) ]
 17/name = "StepDown"
 17/texture = ExtResource( 1 )
 17/tex_offset = Vector2( 0, 0 )
@@ -232,7 +232,7 @@ points = Vector2Array( 0, 0, 64, 0, 64, 96, 0, 32 )
 17/occluder_offset = Vector2( 0, 0 )
 17/navigation_offset = Vector2( 0, 0 )
 17/shape_offset = Vector2( 0, 0 )
-17/shapes = [ SubResource( 9 ) ]
+17/shapes = [ SubResource( 17 ) ]
 18/name = "DarkDetail 1"
 18/texture = ExtResource( 1 )
 18/tex_offset = Vector2( 0, 0 )

Link to minimal example project (optional but very welcome):
tres_subresource_reordering.zip

@akien-mga akien-mga added this to the 2.1 milestone Jul 24, 2016
@Zylann
Copy link
Contributor

Zylann commented Jul 24, 2016

I had this problem in this issue #5289
But it still happens to my project, I have diffs like that as well:

 [node name="About" type="TextureButton" parent="CanvasLayer/Sprite"]

-margin/left = 87.0
-margin/top = -6.0
-margin/right = 111.0
-margin/bottom = 10.0
 focus/ignore_mouse = false
 focus/stop_mouse = true
 size_flags/horizontal = 2
 size_flags/vertical = 2
+margin/left = 87.0
+margin/top = -6.0
+margin/right = 111.0
+margin/bottom = 10.0
 toggle_mode = false
 textures/normal = ExtResource( 4 )
 textures/pressed = ExtResource( 4 )
 textures/hover = ExtResource( 5 )
-[sub_resource type="ConcavePolygonShape2D" id=13]
+[sub_resource type="ConcavePolygonShape2D" id=3]

 custom_solver_bias = 0.0
 segments = Vector2Array( -16, -13, -12, -8, -12, -8, -10, -3, -10, -3, -10, 3, -10, 3, -12, 8, -12, 8, -16, 13, -16, 13, -16, -13 )

-[sub_resource type="ConcavePolygonShape2D" id=14]
+[sub_resource type="ConcavePolygonShape2D" id=4]

 custom_solver_bias = 0.0
 segments = Vector2Array( -16, -1, 16, -1, 16, -1, 16, 11, 16, 11, -10, 11, -10, 11, -16, -1 )

-[sub_resource type="ConvexPolygonShape2D" id=16]
+[sub_resource type="ConvexPolygonShape2D" id=5]

 custom_solver_bias = 0.0
 points = Vector2Array( -16, -1, 16, -1, 16, 11, -16, 11 )

-[sub_resource type="ConvexPolygonShape2D" id=17]
+[sub_resource type="ConvexPolygonShape2D" id=6]

etc

@reduz
Copy link
Member

reduz commented Jul 24, 2016

This is not a bug and It should not be fixed. The resources have effectively changed by re-building the tileset, so Godot will not try to overwrite their internal IDs.

Remember that the internal IDs are persistent, mean to always represent the same resource inside the file. In this case, the resources are replaced so Godot will assign different IDs to them.

@reduz reduz closed this as completed Jul 24, 2016
@reduz
Copy link
Member

reduz commented Jul 24, 2016

What we could try to do instead is to somehow steal the IDs of the previews tile item when converting/merging to tileset

@reduz reduz reopened this Jul 24, 2016
@akien-mga
Copy link
Member Author

Have you tried my steps to reproduce? This happens even when not re-building the tileset. And effectively, the IDs do not represent the same resources inside the file, since their IDs change.

@reduz
Copy link
Member

reduz commented Jul 24, 2016

let me check if i can do this

@reduz
Copy link
Member

reduz commented Jul 24, 2016

just checking this and I don't really think it could be done.. maybe with a lot of hacking

@akien-mga
Copy link
Member Author

In the example I gave I suggested to recreate the tileset without merging as it makes the issue easier to reproduce, but I also get such diffs on tileset resources that I haven't modified in weeks. Just saving the scene that uses it sometimes changes the ordering of subresources.

@akien-mga
Copy link
Member Author

For example clone https://github.com/KOBUGE-Games/jetpaca, open intro.tscn and save it, it shows the same diff as above for the tileset resource.

@akien-mga
Copy link
Member Author

akien-mga commented Jul 24, 2016

Some more discussion from today on IRC:

<Akien> reduz: Actually I have a good reproducer that shows that it doesn't only related to tilesets being merged
<Akien> reduz: In jetpaca, open "tileset/mountain_tileset.tscn". It's the source scene of the tileset, so if you save it without modification, the subresources shouldn't change either.
<Akien> reduz: You should see however that it generates a similar diff.
<Akien> (Save the scene I mean, not export the tileset)
<Akien> The scene was last edited in Godot directly, so this reordering should not happen as the contents are valid and don't have to be reprocessed.
<Akien> All current *_tileset.tscn files seem to be good reproducers of the bug in jetpaca
<reduz> Akien, ah yes, the file tilesets/mountain_tileset.tscn is different
<reduz> Akien, but this is because of how CollisionPolygon2D works, until we rewrite it for 3.0 there is no way to fix it
<reduz> Akien, every time it opens the scene, it re-generates the list of convex polygons
<Akien> reduz: Yeah, and somehow such diffs come and go. From time to time I resave all scenes and commit the diff, but some weeks later the diffs reappear to basically undo what had been committed.
<reduz> Akien, for 3.0 we need to change it so collision shapes are actually kept in the node, so this re-generating does not need to happen
<Akien> Right, I thought it might be specific to the polygons yes.
<Akien> I guess that's something tscn users can live with until 3.0 :)
<Akien> Is it expected though that resources get resaved when saving a scene where they are included?
<Akien> For example if you open 'stages/world_1/intro.tscn" and save it, you should get a diff in the tileset resource
<Akien> Because the resource is resaved, and the same issue with ConvexPolygon2D likely happens
<reduz> yes i don't think it can be fixed for now

So this will likely have to wait for 3.0 to be fixed properly. I promise to ensure (by harassing @reduz) that such diffs are eradicated in 3.0 ;)

@akien-mga akien-mga modified the milestones: 3.0, 2.1 Jul 24, 2016
@akien-mga akien-mga changed the title TSCN/TRES subresource random reordering TSCN/TRES subresource random reordering of CollisionPolygon2Ds Jul 24, 2016
@akien-mga
Copy link
Member Author

@Zylann Regarding your diff on margin/*, if you manage to reproduce this consistently, it might be worth it to open a separate issue for those; the conclusion here regarding CollisionPolygon2Ds is likely not valid for margin/* properties of Node2D.

@reduz
Copy link
Member

reduz commented Aug 5, 2017

I remember i fixed dictionaries at some point, which got rid of many of these issues. Is this still a problem?

@reduz
Copy link
Member

reduz commented Aug 8, 2017

but this is because of how CollisionPolygon2D works, until we rewrite it for 3.0 there is no way to fix it

so I guess this can be closed now, @akien-mga ? would be the sixteenth version of the issue present in the 3.0 milestone :P

@HummusSamurai
Copy link
Contributor

IIRC a lot of dictionaries now use OrderedHashMap so this could have been fixed.

@akien-mga
Copy link
Member Author

I haven't checked in depth, but assuming fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants