Adding a sound to a SampleLibrary causes the whole list to appear in diff

[Zylann]

Godot 2.1, Windows 10 64 bits

I added a sound to a SampleLibrary, but looking at my git diff I see the whole SampleLibrary list has been modified, and it's hard to see what I changed.

@@ -1,26 +1,27 @@
-[gd_scene load_steps=25 format=1]
+[gd_scene load_steps=26 format=1]

 [ext_resource path="res://scripts/avatar.gd" type="Script" id=1]
 [ext_resource path="res://scripts/avatar_sprite.gd" type="Script" id=2]
 [ext_resource path="res://textures/runner.png" type="Texture" id=3]
 [ext_resource path="res://scripts/magnetic_visual.gd" type="Script" id=4]
 [ext_resource path="res://textures/stick.png" type="Texture" id=5]
 [ext_resource path="res://scripts/avatar_camera.gd" type="Script" id=6]
 [ext_resource path="res://sounds/close.wav" type="Sample" id=7]
 [ext_resource path="res://sounds/death.wav" type="Sample" id=8]
-[ext_resource path="res://sounds/jump.wav" type="Sample" id=9]
-[ext_resource path="res://sounds/land.wav" type="Sample" id=10]
-[ext_resource path="res://sounds/roll.smp" type="Sample" id=11]
-[ext_resource path="res://sounds/sprint_begin.smp" type="Sample" id=12]
-[ext_resource path="res://sounds/sprint_end.smp" type="Sample" id=13]
-[ext_resource path="res://sounds/stick_jump.smp" type="Sample" id=14]
-[ext_resource path="res://sounds/stick_land.smp" type="Sample" id=15]
-[ext_resource path="res://scripts/avatar_sound.gd" type="Script" id=16]
-[ext_resource path="res://scripts/avatar_drift_away.gd" type="Script" id=17]
-[ext_resource path="res://scripts/avatar_input.gd" type="Script" id=18]
-[ext_resource path="res://scripts/avatar_achievements.gd" type="Script" id=19]
+[ext_resource path="res://sounds/death_fall.smp" type="Sample" id=9]
+[ext_resource path="res://sounds/jump.wav" type="Sample" id=10]
+[ext_resource path="res://sounds/land.wav" type="Sample" id=11]
+[ext_resource path="res://sounds/roll.smp" type="Sample" id=12]
+[ext_resource path="res://sounds/sprint_begin.smp" type="Sample" id=13]
+[ext_resource path="res://sounds/sprint_end.smp" type="Sample" id=14]
+[ext_resource path="res://sounds/stick_jump.smp" type="Sample" id=15]
+[ext_resource path="res://sounds/stick_land.smp" type="Sample" id=16]
+[ext_resource path="res://scripts/avatar_sound.gd" type="Script" id=17]
+[ext_resource path="res://scripts/avatar_drift_away.gd" type="Script" id=18]
+[ext_resource path="res://scripts/avatar_input.gd" type="Script" id=19]
+[ext_resource path="res://scripts/avatar_achievements.gd" type="Script" id=20]

 [sub_resource type="SampleLibrary" id=5]

 samples/close = { "db":0.0, "pitch":1.0, "sample":ExtResource( 7 ) }
 samples/death = { "db":0.0, "pitch":1.0, "sample":ExtResource( 8 ) }
-samples/jump = { "db":0.0, "pitch":1.0, "sample":ExtResource( 9 ) }
-samples/land = { "db":0.0, "pitch":1.0, "sample":ExtResource( 10 ) }
-samples/roll = { "db":0.0, "pitch":1.0, "sample":ExtResource( 11 ) }
-samples/sprint_begin = { "db":0.0, "pitch":1.0, "sample":ExtResource( 12 ) }
-samples/sprint_end = { "db":0.0, "pitch":1.0, "sample":ExtResource( 13 ) }
-samples/stick_jump = { "db":-3.0, "pitch":1.0, "sample":ExtResource( 14 ) }
-samples/stick_land = { "db":0.0, "pitch":1.0, "sample":ExtResource( 15 ) }
+samples/death_fall = { "db":-2.0, "pitch":1.0, "sample":ExtResource( 9 ) }
+samples/jump = { "db":0.0, "pitch":1.0, "sample":ExtResource( 10 ) }
+samples/land = { "db":0.0, "pitch":1.0, "sample":ExtResource( 11 ) }
+samples/roll = { "db":0.0, "pitch":1.0, "sample":ExtResource( 12 ) }
+samples/sprint_begin = { "db":0.0, "pitch":1.0, "sample":ExtResource( 13 ) }
+samples/sprint_end = { "db":0.0, "pitch":1.0, "sample":ExtResource( 14 ) }
+samples/stick_jump = { "db":-3.0, "pitch":1.0, "sample":ExtResource( 15 ) }
+samples/stick_land = { "db":0.0, "pitch":1.0, "sample":ExtResource( 16 ) }

(and more)

Note: this is only one part of the diff, there are tons of other places I can see modifications that don't actually do anything besides swapping IDs or lines.

[akien-mga]

That's not really a bug, but a feature. The samples are listed alphabetically, which allows to enforce some consistency. Previously, this ordering was not enforced and the resources would get assigned a new ID every now and then based on whatever dark magic the filesystem timestamps did with Godot.

[Zylann]

Well, they are alphabetically ordered in this diff, and it could have made only one line to change, which is correct.

However what's causing all the diffs is the fact every ID has changed. As far as I understand the goal of TSCN, I don't really care what the ID is as long as it's unique within the file, it should just not change and shift like that everytime one item is added.
This case also happens when you edit a tileset. Adding one tile litteraly makes the whole .tres to appear different, although we only added one tile.

Another practical example: sometimes I make changes to a scene or resource, but I only want to commit part of it and leave the rest for further commits. But I often cannot do that because the other changes caused lots of other places to change as well, even if they could be left unchanged by setting correct IDs.

Not to mention merging scenes :p

[akien-mga]

But that's the point: they are ordered alphabetically, and their ID depends on the order. So the ID must change for all of them.

Basically Godot reads the tscn, handles data internally, and then outputs a new tscn when you save. It doesn't (so far) read the previous file to try to keep the same arbitrary order as previously, it just creates a new file with the order it's designed to use.

[hubbyist]

Will (biggest_id + 1) to create new ids solve this problem?

[akien-mga]

Will (biggest_id + 1) to create new ids solve this problem?

As I mentioned right above, this would imply not saving scenes but merging the new scene state with the existing scene. So you'd get a different output when saving as a new scene and when overwriting an existing scene.

[Zylann]

Well, this is how Godot works, but it's not really how TSCN was advertised. It's meant to be human-readable and friendly to version control. However version control doesn't seem to be that true.

In the current case of unstable IDs like now, it makes it more difficult to guarantee diffs will represent what actually changed.

@hubbyist this would solve how you generate new IDs, but it won't guarantee to save objects with the same ID as before. Currently it's using the order in which items are serialized, which guarantees unicity, but not stability.
Stability could be achieved with a hash, memorizing the ID in the resource, or re-reading it to perform a merge rather than a re-write (but even that doesn't seems to work with tilemaps). I need to read more how it works now because those solutions are theory so far. But it's definitely possible, I already did that in an earlier project.

[hubbyist]

Will making ids a property of resource and keeping track of them and using (biggest_id + 1) just work? Or will this complicate things further? After all they seem to me working as a sorting order rather than ids in the current situation.

Maybe base64 encoded representation of file hash may work. But not sure about the performance impact.

[Zylann]

When reading a .tscn file, here is what I found:

load_steps is also increased to match how many [sections] there are in the file. But I don't understand why, can't the parser already detect that amount?

Next, I see IDs are the only way internal resources can be identified. However in the case of external resources, IDs are only indirections to avoid writing the path directly. Both can collision but it doesn't matters because they are differenciated with ExtResource or SubResource.

External resources:
Unity3D gets a point here because they have GUIDs to identify external resources, and they won't change whatever you do with them (move, modify, rename).
Godot identifies resources with paths, so we can hash the path to deduce the ID, which is better to make them order-independent and avoid cluttered diffs, however it will change if the path changes. But due to how Godot works you have to change the file anyways if the external resource moved, so it would actually be an improvement. But there is a better solution, see below.

Internal resources:
These are a bit trickier, because their ID is the only way to identify them.
In this case we could generate new IDs from a counter. We have it already if we check what is being saved, no need to cache it. Then we store the ID in the editor metadata of the resource, if that's not already the case (is there such a thing? like the tree-expand state for nodes).

The latter is actually better for external resources too, because it maintains IDs and it won't cause existing resources to be saved with lots of changes. It doesn't breaks compatibility, it relies on something Godot might already have (metadata) and as such, can be stripped in release builds because we use SCN to focus on performance instead of dev workflow.
As a bonus, IDs would keep look prettier than hashes :)

[bojidar-bg]

Would it help if we use the path's hash as id, instead of some sequential id?

[ghost]

No longer relevant in master as SampleLibrary is no more. But is it still reproducible in 2.1?

[Zylann]

It still happens in 2.1.5 rc1.

[akien-mga]

The 2.1 branch is no longer actively worked on by engine developers, unless critical fixes are needed for current games in production. As such we are now closing non-critical bug reports affecting only the 2.1 branch.

Please comment if this was incorrectly triaged and is still relevant in 3.x versions.

[Zylann]

Random diffs due to adding or removing resources to a scene might still happen in master because I don't recall that this logic was ever changed.

[akien-mga]

There were some changes done in 3.0, and though I believe not all cases might be fixed, it's probably better covered in a new issue about what happens in 3.1-alpha, if anything.