Godot: Saving with instanced scenes is nondeterministic.

[DavidSichma]

Godot version:
Master, built from 0b69be91d56e8983877ec5a48c219cf28a741903

OS/device including version:
Windows

Issue description:
When saving, the order in which external scenes are loaded and thus their index can randomly change. Other external resources seem fine.

To be clear, I am talking about these lines in a .tscn-file:

[ext_resource path="res://path_to_scene.tscn" type="PackedScene" id=16]

The order of such lines can change and thus the index/id will differ.

While this bug does not cause the scene to break, it generates a lot of clutter for version control.

Steps to reproduce:

1. Have a scene use instances of many other scenes. Such a scene is provided.
2. Save the scene and observe the changes in your scene file.

Minimal reproduction project:
nondeterminism.zip
The main scene is Test.tscn. It consists of 10 MeshInstances using mesh1.tres through mesh10.tres. These external resources seem fine. It additionally contains instances of sub1.tscn through sub10.tscn. These resources get shuffled.

[akien-mga]

I can confirm this, it's a real pain (and I think it's a regression, it used to be more reliable in the past, though there were other issues with tscn determinism).

There's an existing issue about this but I'm on phone now, can't check for the number. Might make sense to keep this one open and focused on a potential regression in handling tscn instances, it's the first time I see it mentioned as being the potential culprit.

[creikey]

We could just alphabetically sort by path name on save?

[creikey]

Seems as if the consistent resource IDs were implemented in commit c1dcdf6109dbe29549517d683d85b81c0ade8611 , however given its current buggyness, I'd move to get rid of this caching code and sort each category of resources alphebetically on resource saving regardless of whether or not the editor tools are in the current binary.

[creikey]

After removing the changes made in c1dcdf6109dbe29549517d683d85b81c0ade8611 that have to do with path caching, resources are saved in the order that they are in the editor. Is this acceptable behavior?

[Anutrix]

After removing the changes made in c1dcdf6 that have to do with path caching, resources are saved in the order that they are in the editor. Is this acceptable behavior?

Storing in alphabetically would be more consistent in my opinion.
Imagine a situation the where the user changes order in the editor, the lines in the tscn would be reshuffled without a gain.
Unless there is a need/gain for them following editor order(I don't know any, just a beginner), I would vote for alphabetical order. Honestly, isn't it a choice between the time needed for sort and clutter caused by code changes? I think the former option is better.
Let's see what core devs and others here think.

[fire]

Note that when you say alphabetical, you mean lexigraphical (ascii / utf8 ordering) and not actual alphabetical order which requires per language lookup tables right?

[Anutrix]

Note that when you say alphabetical, you mean lexigraphical (ascii / utf8 ordering) and not actual alphabetical order which requires per language lookup tables right?

Whichever is simpler and faster is better, as long the order is reproducible with as less extra/unrelated/useless data as possible.
This will make sure changes are minimum.
Lexicographically is what I meant when I mentioned alphabetically. It is fast, universal and manually reproducible too.

[creikey]

Lexicographically, as the < operator is already overloaded on Godot's String class to do so I beleive.

[creikey]

I've implemented a lexicographical sorted external scene saving and opened a PR to have it merged. It makes the external resource listings stay the same no matter the order of the nodes in the scene tree, however, it's worth noting that rearranging the nodes still produces a different scene file as the [node listings are still dependent on their order in the editor.

[Anutrix]

I've implemented a lexicographical sorted external scene saving and opened a PR to have it merged. It makes the external resource listings stay the same no matter the order of the nodes in the scene tree, however, it's worth noting that rearranging the nodes still produces a different scene file as the [node listings are still dependent on their order in the editor.

That should be fine since rearranging nodes is a change that is relevant.
If this is not true, then [node listings might also be sorted in some way. But that seems to be out this issue's scope and is better to have as separate issue(if at all needed) with it's own PR(if at all needed).

[nhydock]

I'm not a fan of lexicographical sorting because it has the potential of creating large diffs at the introduction of a single new resource. By introducing a value that sorts earlier than others, you'll shift all the other indices and cause diffs on nodes that didn't actually change in any way as they need to update the references.

What's really needed is a consistent hash so then ordering doesn't matter.

[creikey]

I'm in the process of fixing the actual bug right now @nhydock . The IDs are cached in the resource, but because the resource is a reference, I believe that at the end of the ResourceInteractiveLoaderText poll because nothing has loaded it yet, the reference is freed, and so the cached ID is lost.

[creikey]

It seems as if resources are being freed, and so the path cache is being lost on scene startup and also the mesh-instances on closing the scene. I'm going to move the path cache to a singleton somewhere and that should fix the bug. on second thought this is not the best solution to the issue, I'm going to take more time to figure out why the entire SceneState's vector of packed scenes is being freed, as it should still be referenced by the editor node...

[rodolforg]

Not only the order, but sometimes a root node in a TSCN scene has an attribute index="0", but another this same scene removes this attribute when saved again.

[Zireael07]

Index=0 is a separate issue that should've been fixed already.

[Zylann]

I had this happen again in 3.2 alpha3:

As well as one of these:

We got rid of folds for the same reason, so I wonder why those are here.

[creikey]

@Zylann I can confirm that the issue is reappearing in a different form in 3.2 alpha 3, it seems to consistently happen with only two packed scenes on every save, however I am not seeing the meta tags reappear in this version. ( in the provided image, only those two packed scenes have been switching back and forth nondeterministically every save )