[Core] Fix sorting of `Dictionary` keys #97542

AThousandShips · 2024-09-27T13:58:37Z

StringName keys were sorted as StringName which is unstable.

Fixes: Animation's data in *.tscn file always change keys order in Dictionary #97522

AThousandShips · 2024-09-27T14:00:10Z

Considering an alternative solution which allows fetching keys with StringName converted to String to improve behavior, will push as a separate commit soon when tested

Added a few cases I could find of this, will remove any that aren't relevant (the code in PackedDataContainer isn't affected as it sorts differently)

core/variant/dictionary.h

editor/plugins/visual_shader_editor_plugin.cpp

servers/rendering_server.cpp

dalexeev · 2024-09-27T14:40:34Z

core/variant/variant.h

@@ -836,6 +836,19 @@ struct StringLikeVariantComparator {
 	static bool compare(const Variant &p_lhs, const Variant &p_rhs);
 };

+struct StringLikeVariantOrder {


Given that there is already StringLikeVariantComparator, I have a minor question. As far as I know, there are three main types of comparators:

Equality (a == b). Not suitable for sorting.

Strict or non-strict comparison (a < b or a <= b).

Three-way comparison (a <=> b in C++20). Not sure if we use something like this.

It would be nice to make sure that we use consistent naming for these types. But that's not directly relevant to the issue, just a note.

struct \w+(Compare|Comparator|Sort|Sorter|Order)\b matches

core/config/project_settings.cpp: 1 381: 1: struct _VCSort { core/debugger/local_debugger.cpp: 1 37: 2: struct ProfileInfoSort { core/math/convex_hull.cpp: 1 1573: 1: struct PointComparator { core/string/string_name.h: 1 167: 2: struct AlphCompare { core/string/ustring.h: 3 515: 1: struct NoCaseComparator { 521: 1: struct NaturalNoCaseComparator { 527: 1: struct FileNoCaseComparator { core/templates/list.h: 1 705: 2: struct AuxiliaryComparator { core/templates/pair.h: 2 63: 1: struct PairSort { 108: 1: struct KeyValueSort { core/templates/sort_array.h: 1 44: 1: struct _DefaultComparator { core/variant/array.cpp: 1 681: 1: struct _ArrayVariantSort { core/variant/callable.h: 1 203: 1: struct CallableComparator { core/variant/variant.h: 2 831: 1: struct VariantComparator { 835: 1: struct StringLikeVariantComparator { drivers/gles3/rasterizer_scene_gles3.h: 1 155: 2: struct InstanceSort { drivers/gles3/storage/particles_storage.h: 1 71: 2: struct ParticlesViewSort { editor/animation_bezier_editor.h: 1 172: 2: struct PairCompare { editor/connections_dialog.cpp: 1 919: 1: struct _ConnectionsDockMethodInfoSort { editor/debugger/script_editor_debugger.h: 1 173: 2: struct ThreadSort { editor/doc_tools.cpp: 3 80: 1: struct ConstructorCompare { 98: 1: struct OperatorCompare { 114: 1: struct MethodCompare { editor/editor_command_palette.h: 2 62: 2: struct CommandEntryComparator { 68: 2: struct CommandHistoryComparator { editor/editor_file_system.h: 1 231: 2: struct DirectoryComparator { editor/editor_help.h: 1 204: 2: struct PropertyCompare { editor/editor_quick_open.h: 1 58: 2: struct EntryComparator { editor/editor_settings.cpp: 1 238: 1: struct _EVCSort { editor/file_info.h: 2 60: 1: struct FileInfoTypeComparator { 66: 1: struct FileInfoModifiedTimeComparator { editor/groups_editor.cpp: 1 66: 1: struct _GroupInfoComparator { editor/plugins/mesh_instance_3d_editor_plugin.cpp: 1 359: 1: struct MeshInstance3DEditorEdgeSort { editor/plugins/node_3d_editor_plugin.cpp: 2 9291: 1: struct _GizmoPluginPriorityComparator { 9300: 1: struct _GizmoPluginNameComparator { editor/plugins/node_3d_editor_plugin.h: 2 74: 2: struct Axis2DCompare { 420: 2: struct ShortcutCheckSetComparator { editor/plugins/text_shader_editor.h: 1 61: 2: struct WarningsComparator { editor/plugins/tiles/tiles_editor_plugin.h: 1 64: 2: struct SourceNameComparator { editor/plugins/visual_shader_editor_plugin.h: 1 403: 2: struct _OptionComparator { editor/project_manager/project_list.cpp: 1 322: 1: struct ProjectListComparator { modules/gdscript/gdscript.cpp: 2 280: 1: struct _GDScriptMemberSort { 2496: 1: struct GDScriptDepSort { modules/gridmap/editor/grid_map_editor_plugin.cpp: 1 791: 1: struct _CGMEItemSort { modules/lightmapper_rd/lightmapper_rd.h: 1 206: 2: struct TriangleSort { modules/mono/csharp_script.cpp: 1 548: 1: struct CSharpScriptDepSort { modules/text_server_adv/text_server_adv.h: 1 694: 2: struct GlyphCompare { // For line breaking reordering. scene/2d/tile_map_layer.h: 2 164: 1: struct CellDataYSortedXReversedComparator { 195: 2: struct CoordsWorldComparator { scene/3d/gpu_particles_collision_3d.h: 1 142: 2: struct FaceSort { scene/3d/voxelizer.h: 1 64: 2: struct CellSort { scene/gui/code_edit.h: 1 515: 1: struct CodeCompletionOptionCompare { scene/gui/control.h: 1 156: 2: struct CComparator { scene/gui/text_edit.cpp: 1 4847: 1: struct _CaretSortComparator { scene/main/scene_tree.h: 1 106: 2: struct ProcessGroupSort { scene/resources/3d/importer_mesh.h: 1 66: 3: struct LODComparator { scene/resources/resource_format_text.h: 1 188: 2: struct ResourceSort { scene/resources/surface_tool.h: 1 128: 2: struct WeightSort { servers/debugger/servers_debugger.cpp: 1 197: 2: struct ProfileInfoSort { servers/rendering/renderer_canvas_cull.h: 2 93: 2: struct ItemIndexSort { 99: 2: struct ItemPtrSort { servers/rendering/renderer_rd/storage_rd/light_storage.h: 2 165: 2: struct LightInstanceDepthSort { 312: 2: struct ReflectionProbeInstanceSort { servers/rendering/renderer_rd/storage_rd/texture_storage.h: 1 312: 2: struct DecalInstanceSort { servers/rendering/rendering_device_graph.h: 1 232: 2: struct RecordedCommandSort { servers/rendering/shader_language.h: 1 748: 2: struct UniformOrderComparator {

Agreed, would have gone with Compare if it wasn't already taken, can go for Sort as it is more common it seems, will change when final decision on the solution is taken

If we were to standardize this I'd change the ones named Compare but does equality checks to Eq or similar and the ones comparing order to Compare (as it seems the most common for those)

AThousandShips · 2024-09-27T15:45:43Z

Added a unit test for the Dictionary case, there are no stringify tests currently for JSON so left that for now, but can add if desired

AThousandShips · 2024-09-27T15:51:16Z

core/variant/variant.h

@@ -836,6 +836,19 @@ struct StringLikeVariantComparator {
 	static bool compare(const Variant &p_lhs, const Variant &p_rhs);
 };

+struct StringLikeVariantOrder {
+	static _ALWAYS_INLINE_ bool compare(const Variant &p_lhs, const Variant &p_rhs) {


This static method isn't technically needed with the alternative solution (or when using List<Variant> in the visual shader code) but I think it's useful to keep so we can use it when sorting Array as it takes a Callable and we can use callable_mp_static

I will look at adding such static methods to other comparators to aid in using them for such cases

core/io/json.cpp

Macksaur · 2024-10-04T14:51:44Z

core/variant/variant.h

+struct StringLikeVariantOrder {
+	static _ALWAYS_INLINE_ bool compare(const Variant &p_lhs, const Variant &p_rhs) {
+		if (p_lhs.is_string() && p_rhs.is_string()) {
+			return p_lhs.operator String() < p_rhs.operator String();


Allocating memory, filling it, then doing a comparison on the copy before dropping it immediately on the floor feels very slapdash. The memory should already be meaningful and available to use for such a comparison.

Perhaps this can be reused?:

godot/core/string/ustring.h

Line 534 in f032af7

_FORCE_INLINE_ bool is_str_less(const L *l_ptr, const R *r_ptr) {

edit: I don't trust this line 😆 perhaps I can be assuaged that there are no allocations in the general cases?

It doesn't allocate memory except when the StringName is created via a c-strinf, which is rare comparatively

I'll take it, thank you!

But will keep in mind for a future helper allowing comparison with both String and StringName generally using this correctly!

`StringName` keys were sorted as `StringName` which is unstable.

AThousandShips · 2024-10-18T07:55:02Z

Removed the alternative fix and will look at it for the future, to get this out there soon

Was looking at the new sort method of Dictionary which I had forgotten to look at after it was added for this purpose, but it doesn't suite this without some changes as it suffers from the same ordering limitations, namely preserving compatibility for Variant ordering in general

So for context, this solves that OP_LESS of Variant sorts by type, so all String keys are sorted separately and StringName keys are sorted separately, further StringName keys are sorted by pointer for performance, but that's unstable

This was introduced by changes that removed the conversion from StringName to String on all keys in:

StringName Dictionary keys #70096

Tl;Dr;
This aims to solve sorting of Dictionary keys in serialisation in a compatibility preserving way while not impacting performance in general

fire · 2024-10-18T08:34:46Z

Does this fix the animation tscn sorting issue?

AThousandShips · 2024-10-18T08:37:59Z

Yes it should, will confirm fully today if no one else has, the relevant change is in variant_parser.cpp

Edit: Confirmed it does indeed make the sorting stable

Repiteo · 2024-10-21T21:50:30Z

Thanks!

AThousandShips · 2024-10-22T07:23:38Z

Thank you!

AThousandShips added bug topic:core regression labels Sep 27, 2024

AThousandShips added this to the 4.4 milestone Sep 27, 2024

AThousandShips requested review from a team as code owners September 27, 2024 13:58

AThousandShips added the topic:animation label Sep 27, 2024

AThousandShips commented Sep 27, 2024

View reviewed changes

core/variant/dictionary.h Outdated Show resolved Hide resolved

AThousandShips commented Sep 27, 2024

View reviewed changes

editor/plugins/visual_shader_editor_plugin.cpp Outdated Show resolved Hide resolved

AThousandShips commented Sep 27, 2024

View reviewed changes

servers/rendering_server.cpp Outdated Show resolved Hide resolved

dalexeev reviewed Sep 27, 2024

View reviewed changes

AThousandShips force-pushed the dict_sort_fix branch from f34cec9 to 0df70fd Compare September 27, 2024 15:07

AThousandShips removed the request for review from a team September 27, 2024 15:07

AThousandShips force-pushed the dict_sort_fix branch from 0df70fd to 795318c Compare September 27, 2024 15:45

AThousandShips requested a review from a team as a code owner September 27, 2024 15:45

AThousandShips commented Sep 27, 2024

View reviewed changes

rune-scape reviewed Sep 29, 2024

View reviewed changes

core/io/json.cpp Outdated Show resolved Hide resolved

Macksaur reviewed Oct 4, 2024

View reviewed changes

[Core] Fix sorting of Dictionary keys

79f654c

`StringName` keys were sorted as `StringName` which is unstable.

AThousandShips force-pushed the dict_sort_fix branch from 795318c to e839a21 Compare October 18, 2024 06:47

AThousandShips requested a review from a team as a code owner October 18, 2024 06:47

Add test

610635e

AThousandShips force-pushed the dict_sort_fix branch from e839a21 to 610635e Compare October 18, 2024 06:47

KoBeWi approved these changes Oct 18, 2024

View reviewed changes

Repiteo merged commit 5fb2232 into godotengine:master Oct 21, 2024
20 checks passed

rune-scape mentioned this pull request Oct 22, 2024

StringLikeVariantOrder: Compare in-place #98408

Open

AThousandShips deleted the dict_sort_fix branch October 22, 2024 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Fix sorting of `Dictionary` keys #97542

[Core] Fix sorting of `Dictionary` keys #97542

AThousandShips commented Sep 27, 2024

AThousandShips commented Sep 27, 2024

dalexeev Sep 27, 2024

AThousandShips Sep 27, 2024 •

edited

Loading

AThousandShips Sep 27, 2024

AThousandShips commented Sep 27, 2024

AThousandShips Sep 27, 2024

Macksaur Oct 4, 2024 •

edited

Loading

AThousandShips Oct 4, 2024

Macksaur Oct 4, 2024

AThousandShips Oct 4, 2024

AThousandShips commented Oct 18, 2024

fire commented Oct 18, 2024

AThousandShips commented Oct 18, 2024 •

edited

Loading

Repiteo commented Oct 21, 2024

AThousandShips commented Oct 22, 2024

[Core] Fix sorting of Dictionary keys #97542

[Core] Fix sorting of Dictionary keys #97542

Conversation

AThousandShips commented Sep 27, 2024

AThousandShips commented Sep 27, 2024

dalexeev Sep 27, 2024

Choose a reason for hiding this comment

AThousandShips Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

AThousandShips Sep 27, 2024

Choose a reason for hiding this comment

AThousandShips commented Sep 27, 2024

AThousandShips Sep 27, 2024

Choose a reason for hiding this comment

Macksaur Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

AThousandShips Oct 4, 2024

Choose a reason for hiding this comment

Macksaur Oct 4, 2024

Choose a reason for hiding this comment

AThousandShips Oct 4, 2024

Choose a reason for hiding this comment

AThousandShips commented Oct 18, 2024

fire commented Oct 18, 2024

AThousandShips commented Oct 18, 2024 • edited Loading

Repiteo commented Oct 21, 2024

AThousandShips commented Oct 22, 2024

[Core] Fix sorting of `Dictionary` keys #97542

[Core] Fix sorting of `Dictionary` keys #97542

AThousandShips Sep 27, 2024 •

edited

Loading

Macksaur Oct 4, 2024 •

edited

Loading

AThousandShips commented Oct 18, 2024 •

edited

Loading