Godot: Removing a resource from a scene makes all nodes using resources to appear in the TSCN diff

[Zylann]

Godot 3.1 alpha 1

Following #6527, I tested adding and swapping resources in a scene, and the resulting TSCN only shows a difference on the resource that actually changed, which is better than before.

However, removing a resource from a scene generates new IDs for almost every other resources referenced by that scene (depends on which one was removed), making every property holding a resource to appear in the diff, even though they had nothing to do with the removal:

As I stated in the older issue, this is in conflict with the idea that TSCN files are VCS-friendly.

[akien-mga]

As I stated in the older issue, this is in conflict with the idea that TSCN files are VCS-friendly.

I don't agree. Most other issues were about changes happening for no apparent reason, and especially diffs appearing in a non-deterministic way, but this one is well defined and IMO the expected behaviour. If you have N resource and remove a resource M < N, all resources from [M+1;N] need to be changed to fit in the new N-1 size.

The alternative would be to keep empty IDs after a removal and have non-contiguous identifiers for external resources, which could quickly become messy. It's probably doable (after all I'm not sure anything requires IDs to be contiguous in the parsing code), but I'm not sure this would be a sensible change.

[Zylann]

@akien-mga what I mean is that if you assign IDs to resources within a saved text file, it would indeed allow to make VCS diffs better. If you consider the behaviour is well defined then the previous issue was also well defined, but to me it's not, because you can see it's the same kind of mess that was formerly created by an addition too. Even if you would consider it expected, it doesn't really help (try merging scenes where people added/removed a few textures in).

The alternative would be to keep empty IDs after a removal and have non-contiguous identifiers for external resources, which could quickly become messy

This is not messy because theoretically, IDs don't have to be contiguous. They currently appear to be indexes though, but do they have to be? Unless there is a strong technical issue preventing any QoL improvement to it?

[ibrahn]

Would it make sense to give resources GUIDs instead of incrementing IDs here?
That'd fix this and also make two separate resource additions to a scene mergable

[vnen]

I'm currently having this problem with a team. This combined with #22324 makes it impossible to merge scenes that changed on both branches. No diff tool helps you understanding what was really changed, and fixing this requires knowledge of the tscn format and some time to spare on the reading.

One of the ideas we had is, instead of sequential IDs, to use paths for the external resources. Or maybe a hash or GUID.

We are willing to implement this to help with the team workflow, but ideally we would like to do something that could get integrated into Godot itself, hence why I'm bumping this discussion.

[Zylann]

@vnen I mentionned this issue in https://github.com/godotengine/godot/issues/15673
As a Unity user working on a large team project, GUIDs help merging in cases like this, and also helps diff tools to understand what happened.
Paths would maybe help in that specific case, but on the other hand it produces a lot more diff when moving or renaming.
Another way is to never generate IDs that were already used for storage in the file, which means "holes" would be allowed.

[reduz]

There are two main reasons why [ext_resource] tags exists:

• So you can quickly verify dependencies
• So, when you use resource interactive loader, you see them load (more fine grained progress)

Honestly the fact they are indexed, in reality, does not add much. The simplest solution, in this case is to
remove the id="" field and repeat the reference (path) down the file. This is clearly compat breaking, so we can note it for 4.0 (while still keeping backwards compatibility). We could also sort the [ext] dependencies alphabetically to make them more VCS friendly.

Are there other changes you would like to make to the tscn file format so we note them down all together?

[reduz]

Ah, now that I think of it, repeating the text path is a bad idea, as well as using a hash because this makes replacing dependencies harder (need to go all the way down the file and change it everywhere). This is useful when you rename files in the editor because replacing dependencies everywhere they are used can take a while.

Definitely then, the simplest way is to just remove the ID, since they are sequential anyway.

[akien-mga]

Are there other changes you would like to make to the tscn file format so we note them down all together?

There are some in #20994.

Definitely then, the simplest way is to just remove the ID, since they are sequential anyway.

Won't they still get reordered then? Or would they be sorted alphabetically based on their ext_resource path?

[reduz]

The order you see now is "as they are found on the scene". If stored alphabetically, if you rename a file, the next time you save the scene they will get reordered. I think it's probably better to keep the current order.

[vnen]

Ah, now that I think of it, repeating the text path is a bad idea, as well as using a hash because this makes replacing dependencies harder (need to go all the way down the file and change it everywhere)

But adding/removing resources already does it to change the IDs, why would it be so bad when replacing?

Definitely then, the simplest way is to just remove the ID, since they are sequential anyway.

That doesn't really solve the issue as you still need to reference the ID in the nodes' properties, so removing a resource reference will still need to change the IDs of all the other resources. Unless there'll be another way to reference to those resources.

[reduz]

In don't really think there is a way out of this. Definitely you could remove the ext_resource tags and make the format cleaner for vcs, but then scanning files, renaming them or loading would be more inefficient.

Maybe an alternative could be to make the IDs persistent somehow.. could be kept while loading the scene and given as a hint when saving.

[Zylann]

I think IDs need to stay because if they are removed it would make merging difficult, isn't it?

[veryprofessionaldodo]

I was wondering how hard it would be to change the IDs to hashes. Wouldn't that solve the problem? By using hashes in the VariantParser, the ID that would be used for the ext_resources would be the same throughout all the project, right?

I was looking at the code, and the scene file is written in the "scene_format_text.cpp" in the scene/resources folder. The index value that is being stored come from the variable next_tag.fields["id"]. I traced back to where this is being written, and it seems it's in the "variant_parser.cpp" file. Would altering this part fix the dependencies problem you pointed out, @reduz ?

[Zylann]

@veryprofessionaldodo if it's a hash, it would change again if there are changes to the resource, so I guess you are actually talking about GUIDs?

[veryprofessionaldodo]

Yes, I didn't know about the meaning of GUIDs, but yes, I suppose they are GUIDs. I didn't know the ID's were globally unique, but it makes sense that they are. And does the ID change when the resource is altered? I didn't know about that behaviour...

[vnen]

Definitely you could remove the ext_resource tags and make the format cleaner for vcs, but then scanning files, renaming them or loading would be more inefficient.

I don't think the ext_resource tags are a problem. As long as they don't change ID when one is added/removed, the VCS will only show one changed line (which is fine since you actually did the change). The problem is changing the ID of the other resources.

Making the IDs persistent would be good in terms of VCS (and also for manual merging), though I see it will need something to keep track of them.

[ignaloidas]

It appears that this exact issue is no longer a problem, as when an external resource is removed, other resource ID's stay the same.

[sulai]

This issue is a show-stopper when trying to collaborate with others remotely using a version control system. It's impossible to merge .tscn files that got its ID's mixed up. You end up overriding one or the other and redo and miss out on things that have been there before.

I agree with others like @vnen, that a stable ID like GUID/UUID sounds like the perfect fit for this problem, without being an expert about how godot uses these IDs.

[dalexeev]

Why not use the same principle as in databases?

1     Added 1
1 2   Added 2
1     Removed 2
1 3   Added 3

[gd_scene load_steps=3 format=2]

[auto_increment ext_resource=4 sub_resource=1]

[ext_resource path="res://script.gd" type="Script" id=1]
[ext_resource path="res://scene.tscn" type="PackedScene" id=3]

[ignaloidas]

Why not use the same principle as in databases?

1     Added 1
1 2   Added 2
1     Removed 2
1 3   Added 3

This solves this exact problem but creates merge conflicts when 2 developers add a resource at the same time. To be fair, this problem already exists but with this solution it would be additionally reinforced.

[dalexeev]

but creates merge conflicts when 2 developers add a resource at the same time

Scenes are generally quite difficult to merge manually. GUIDs help avoid collisions, but they are less readable.
There is a proposal to create a merge tool: godotengine/godot-proposals#1281.