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.

Sign up for GitHub

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

[Core] Fix sorting of Dictionary keys #97542

Merged
merged 2 commits into from
Oct 21, 2024
Merged

[Core] Fix sorting of Dictionary keys #97542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79f654c
[Core] Fix sorting of `Dictionary` keys
AThousandShips Sep 27, 2024
610635e
Add test
AThousandShips Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/io/json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ String JSON::_stringify(const Variant &p_var, const String &p_indent, int p_cur_
d.get_key_list(&keys);

if (p_sort_keys) {
keys.sort();
keys.sort_custom<StringLikeVariantOrder>();
}

bool first_key = true;
Expand Down
13 changes: 13 additions & 0 deletions core/variant/variant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,19 @@ struct StringLikeVariantComparator {
static bool compare(const Variant &p_lhs, const Variant &p_rhs);
};

struct StringLikeVariantOrder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Equality (a == b). Not suitable for sorting.
  2. Strict or non-strict comparison (a < b or a <= b).
  3. 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 {

Copy link
Member Author

@AThousandShips AThousandShips Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

static _ALWAYS_INLINE_ bool compare(const Variant &p_lhs, const Variant &p_rhs) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (p_lhs.is_string() && p_rhs.is_string()) {
return p_lhs.operator String() < p_rhs.operator String();
Copy link
Contributor

@Macksaur Macksaur Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take it, thank you!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return p_lhs < p_rhs;
}

_ALWAYS_INLINE_ bool operator()(const Variant &p_lhs, const Variant &p_rhs) const {
return compare(p_lhs, p_rhs);
}
};

Variant::ObjData &Variant::_get_obj() {
return *reinterpret_cast<ObjData *>(&_data._mem[0]);
}
Expand Down
2 changes: 1 addition & 1 deletion core/variant/variant_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2245,7 +2245,7 @@ Error VariantWriter::write(const Variant &p_variant, StoreStringFunc p_store_str
} else {
List<Variant> keys;
dict.get_key_list(&keys);
keys.sort();
keys.sort_custom<StringLikeVariantOrder>();

if (keys.is_empty()) {
// Avoid unnecessary line break.
Expand Down
9 changes: 4 additions & 5 deletions editor/plugins/visual_shader_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2128,12 +2128,11 @@ void VisualShaderEditor::_update_nodes() {
}
}

Array keys = added.keys();
keys.sort();

for (int i = 0; i < keys.size(); i++) {
const Variant &key = keys.get(i);
List<Variant> keys;
added.get_key_list(&keys);
keys.sort_custom<StringLikeVariantOrder>();

for (const Variant &key : keys) {
const Dictionary &value = (Dictionary)added[key];

add_custom_type(value["name"], value["type"], value["script"], value["description"], value["return_icon_type"], value["category"], value["highend"]);
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/editor/gdscript_docgen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ String GDScriptDocGen::_docvalue_from_variant(const Variant &p_variant, int p_re

List<Variant> keys;
dict.get_key_list(&keys);
keys.sort();
keys.sort_custom<StringLikeVariantOrder>();

for (List<Variant>::Element *E = keys.front(); E; E = E->next()) {
if (E->prev()) {
Expand Down
8 changes: 8 additions & 0 deletions tests/core/variant/test_variant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1806,6 +1806,14 @@ TEST_CASE("[Variant] Writer and parser dictionary") {
CHECK_MESSAGE(d_parsed == Variant(d), "Should parse back.");
}

TEST_CASE("[Variant] Writer key sorting") {
Dictionary d = build_dictionary(StringName("C"), 3, "A", 1, StringName("B"), 2, "D", 4);
String d_str;
VariantWriter::write_to_string(d, d_str);

CHECK_EQ(d_str, "{\n\"A\": 1,\n&\"B\": 2,\n&\"C\": 3,\n\"D\": 4\n}");
}

TEST_CASE("[Variant] Writer recursive dictionary") {
// There is no way to accurately represent a recursive dictionary,
// the only thing we can do is make sure the writer doesn't blow up
Expand Down
Loading