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

Saving with instanced scenes is nondeterministic. #30538

Closed
DavidSichma opened this issue Jul 12, 2019 · 20 comments · Fixed by #30622
Closed

Saving with instanced scenes is nondeterministic. #30538

DavidSichma opened this issue Jul 12, 2019 · 20 comments · Fixed by #30622

Comments

@DavidSichma
Copy link
Contributor

Godot version:
Master, built from 0b69be9

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
Copy link
Member

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
Copy link
Contributor

creikey commented Jul 13, 2019

We could just alphabetically sort by path name on save?

@creikey
Copy link
Contributor

creikey commented Jul 13, 2019

Seems as if the consistent resource IDs were implemented in commit c1dcdf6 , 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
Copy link
Contributor

creikey commented Jul 14, 2019

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?

@Anutrix
Copy link
Contributor

Anutrix commented Jul 14, 2019

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
Copy link
Member

fire commented Jul 14, 2019

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
Copy link
Contributor

Anutrix commented Jul 14, 2019

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
Copy link
Contributor

creikey commented Jul 15, 2019

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

@creikey
Copy link
Contributor

creikey commented Jul 15, 2019

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
Copy link
Contributor

Anutrix commented Jul 15, 2019

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).

@erodozer
Copy link

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
Copy link
Contributor

creikey commented Jul 16, 2019

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
Copy link
Contributor

creikey commented Jul 16, 2019

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...

creikey added a commit to creikey/godot that referenced this issue Jul 26, 2019
 - PackedScenes freed before saved, so their path cache is lost
 - Solution is to move data to persistent static map
 - This patch will fix godotengine#30538
@rodolforg
Copy link
Contributor

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
Copy link
Contributor

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

@Zylann
Copy link
Contributor

Zylann commented Oct 30, 2019

I had this happen again in 3.2 alpha3:

image

As well as one of these:

image

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

@creikey
Copy link
Contributor

creikey commented Oct 30, 2019

@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 )
example

@jitspoe
Copy link
Contributor

jitspoe commented Sep 19, 2022

This seems to have gotten REALLY bad recently. I've been doing custom builds, so I'm not sure exactly which version has the issue, but my previous 3.x sync was before the beginning of the year and I just recently synced a few weeks ago, so probably around 3.5, and now, if I so much as LOOK at a scene, 90+% of my id's get randomly shuffled:

image

Makes it extremely difficult to tell what, if anything, actually changed.

I thought maybe something just changed with the sorting order and once files had been updated to the new version, they'd be consistent, but every time I go to check in, files are changed. Basically any scene I have open, even if I don't touch it, gets randomly shuffled.

@realkotob
Copy link
Contributor

If this still happens why not re-open it? Or should I open a separate issue?

@akien-mga
Copy link
Member

akien-mga commented Jan 17, 2023

You should open a new issue. Similar general description of symptoms 3 years later != same bug.

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