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

Exported dictionaries have their keys sorted alphabetically #90142

Open
JHDev2006 opened this issue Apr 2, 2024 · 18 comments · May be fixed by #104164
Open

Exported dictionaries have their keys sorted alphabetically #90142

JHDev2006 opened this issue Apr 2, 2024 · 18 comments · May be fixed by #104164

Comments

@JHDev2006
Copy link

JHDev2006 commented Apr 2, 2024

Tested versions

  • Tested with Godot 4.1.2 and Godot 4.3 dev 5

System information

Godot v4.3.dev5.mono - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3070 Ti (NVIDIA; 31.0.15.4592) - AMD Ryzen 7 5800X 8-Core Processor (16 Threads)

Issue description

When an exported dictionary is created, they keys which are set in the editor, are sorted at runtime, which is incredibly frustrating for my use case.

Steps to reproduce

  • Create an exported dictionary (@export var dict := {})
  • Add some values into the dictionary through the editor. - ({"b": 0, "c": 1, "a": 2})
  • Print the dictionary, it will output: "{"a": 2, "b": 0, "c": 1}"

Minimal reproduction project (MRP)

N/A

@AThousandShips
Copy link
Member

AThousandShips commented Apr 2, 2024

This is by design, for stability, as far as I know

See also:

@JHDev2006
Copy link
Author

should it happen with exported dictionaries though? I get why it needs to happen for dictionaries written to files and such, but i dont see why it should need to be the same with exported dictionaries?

@AThousandShips
Copy link
Member

That's what exported dictionaries are though? They are stored and loaded

@JHDev2006
Copy link
Author

JHDev2006 commented Apr 2, 2024

but why should it be? Other than for stability as you mentioned, which seems only needed for file reading, i dont see any other reason why exported dictionaries should be sorted without the users knowledge. sort_custom should be used instead if the user or the engine REALLY has to have the dictionary sorted

@AThousandShips
Copy link
Member

But they are exported, so they're stored, so they're sorted

Are you printing it from the editor? Or from running project? If it's from the running project you are loading it, you have to load the scene?

@JHDev2006
Copy link
Author

im running it from the project, but why then can it print the dictionary if its a hard coded variable, even a constant dosent have this issue, and wouldnt that be stored too?

@timothyqiu
Copy link
Member

timothyqiu commented Apr 3, 2024

Keeping insertion order is a feature according to the documentation:

Dictionaries will preserve the insertion order when adding new entries.

I don't think it should be sorted during serialization.

Dictionary keeping insertion order is a result of 4.0 changing the implementation to use HashMap instead of OrderedHashMap. But the decision of sorting keys during serialization was made way before that. So I think it's a bug.

@ramdor
Copy link
Contributor

ramdor commented Jun 5, 2024

It also effects storing and recovering a dictionary via ConfigFile

@dog-on-moon
Copy link
Contributor

The cause of this issue is this line: https://github.com/godotengine/godot/blob/master/core/variant/variant_parser.cpp#L2248. I tried tracking down the original commit, but couldn't quite make it far back enough.

It definitely feels... load-bearing, but I removed it on my build a few weeks ago, and it has even fixed similar issues as well (one of my exported dictionaries kept re-sorting its keys on every resource save, which would have caused endless merge conflicts for my team).

I assume making this change would break existing test cases, but I agree that sorting dictionary keys at this stage of serialization should be considered a bug.

@increpare
Copy link

increpare commented Mar 13, 2025

Is there any workaround to this beyond recompiling? JSON-stringify I think loses a lot of information? I guess to use var_to_bytes, which is IIRC stable/in conformance with the documentation, but sacrificing human-readability/diffability? (I was working on an editor for my game, relying on the ordered behaviour of the Dictionaries for displaying properties in the object inspector, but the fields are shuffled about when I load them)

@YYF233333
Copy link
Contributor

I'm currently touching Dictionary code and want to add some points. This feature is likely to have a significant effect on performance, especially after #104047 is merged. Sorting means we need to copy the keys, sort them, and then perform hashmap lookups one by one. Without sorting, we can iterate directly over the internal HashMap with zero copies. Large Dictionarys (store in JSON or other serialization formats) are quite common in certain types of games (e.g. for storing dialogues or item data). I think it is a trade-off worth considering.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 14, 2025

I am reluctant to deem this a "bug" given it's intentionally coded this way, but I've read enough of the above to deem this more annoying than it's worth. It's the only Variant type that does (or can do) this and it breaks user expectations.

@dog-on-moon
Copy link
Contributor

I had patched this issue locally for around 3 months and noticed no issues -- of course this is anecdotal and only covered my use cases, but it's enough for me to suggest stripping it for the 4.5 cycle, just to see if anything comes up

@JayMikeMill
Copy link

no one has a easy way to keep dictionarys from sorting when saving a scene?

@Mickeon
Copy link
Contributor

Mickeon commented Mar 14, 2025

I had patched this issue locally for around 3 months and noticed no issues -- of course this is anecdotal and only covered my use cases, but it's enough for me to suggest stripping it for the 4.5 cycle, just to see if anything comes up

As far as I have seen, no one has opened a PR addressing this issue yet. You could be the one.

@dog-on-moon dog-on-moon linked a pull request Mar 15, 2025 that will close this issue
@dog-on-moon
Copy link
Contributor

I had patched this issue locally for around 3 months and noticed no issues -- of course this is anecdotal and only covered my use cases, but it's enough for me to suggest stripping it for the 4.5 cycle, just to see if anything comes up

As far as I have seen, no one has opened a PR addressing this issue yet. You could be the one.

Okie Dokie

@aaronfranke
Copy link
Member

aaronfranke commented Mar 15, 2025

As of Godot 4.4 with PR #77213, it is possible to manually sort when needed. In theory this means that it's not needed to do it automatically when serializing anymore, since if the user wants sorting, they can just call the function before serializing.

However, in the past, str_to_var(var_to_str(dict)) was suggested as a hack to sort Dictionaries. Making this change could break projects that expect this to work. Since sort() isn't in Godot 4.3, if PR #104164 was merged for 4.5, this would leave 4.4 as the only "transition period" version where both techniques work. I think ultimately this change is a good one, it's better to keep user data as-is when serializing, but I'm unsure about the timing of merging this.

@Creta5164

This comment has been minimized.

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

Successfully merging a pull request may close this issue.