Added an option to split the output proto files by top level category #367

klingbolt · 2024-12-09T03:44:33Z

No description provided.

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel1.guildpoint

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel2.guildpoint

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel3.guildpoint

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel1.guildpoint

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel2.guildpoint

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel3.guildpoint

github-actions · 2024-12-09T03:53:05Z

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel1.guildpoint

Full Diff

--- xml_converter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel1.guildpoint.textproto._old 2024-12-09 03:53:05.031311821 +0000 +++ xml_converter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel1.guildpoint.textproto._new 2024-12-09 03:53:05.035311909 +0000 @@ -0,0 +1,12 @@ +category { + name: "TopLevel1" + icon { + map_id: 50 + position { + x: 169.81 + y: 210.65 + z: 215.83 + } + } + id: "\010\007L]\332B\361\323" +}

github-actions · 2024-12-09T03:53:06Z

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel2.guildpoint

Full Diff

--- xml_converter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel2.guildpoint.textproto._old 2024-12-09 03:53:05.043312083 +0000 +++ xml_converter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel2.guildpoint.textproto._new 2024-12-09 03:53:05.051312257 +0000 @@ -0,0 +1,12 @@ +category { + name: "TopLevel2" + icon { + map_id: 50 + position { + x: 169.81 + y: 210.65 + z: 215.83 + } + } + id: "\010\007L]\333B\361\323" +}

github-actions · 2024-12-09T03:53:07Z

...nverter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel3.guildpoint

Full Diff

--- xml_converter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel3.guildpoint.textproto._old 2024-12-09 03:53:05.059312431 +0000 +++ xml_converter/integration_tests/test_cases/xml_split_by_category/output_proto/toplevel3.guildpoint.textproto._new 2024-12-09 03:53:05.067312606 +0000 @@ -0,0 +1,12 @@ +category { + name: "TopLevel3" + icon { + map_id: 50 + position { + x: 169.81 + y: 210.65 + z: 215.83 + } + } + id: "\010\007L]\334B\361\323" +}

AsherGlick

A good proof of concept but I think some of the minor design decisions chosen here did not take enough into consideration and need to be reevaluated.

AsherGlick · 2024-12-09T07:04:37Z

xml_converter/integration_tests/test_cases/xml_split_by_category/input/pack/xml_file.xml

+    <MarkerCategory DisplayName="TopLevel1" Name="toplevel1" XPos="169.81" YPos="210.65" ZPos="215.83" MapID="50" ></MarkerCategory>
+
+    <MarkerCategory DisplayName="TopLevel2" Name="toplevel2" XPos="169.81" YPos="210.65" ZPos="215.83" MapID="50" ></MarkerCategory>
+
+    <MarkerCategory DisplayName="TopLevel3" Name="toplevel3" XPos="169.81" YPos="210.65" ZPos="215.83" MapID="50" ></MarkerCategory>


Lets differentiate these by more than just DisplayName and Name. Otherwise this test does a silly job at not catching the issue of "we write the same top level node but with different display/names for each category". The attributes could also be given to the POIs instead of the categories because XPos YPos ZPos dont make a huge amount of sense as category attributes.

Lets also add some subcategories here too so that we can test that "more than just the top level category gets written". Because we do want the entire category tree.

AsherGlick · 2024-12-09T07:10:51Z

xml_converter/src/packaging_protobin.cpp

+    StringHierarchy category_filter;
+    category_filter.add_path({}, true);


Why are you hardcoding the global wildcard here and preventing it from being passed in to the function?

I believe that category_filter is not global in the same sense as the other two large inputs. It is created just before this function is called, and always for a single path. In this sense, it is more similar in scope to the ProtoWriterState, which is also created in this function.

AsherGlick · 2024-12-09T07:15:31Z

xml_converter/src/packaging_protobin.cpp

+void write_protobuf_file_per_category(
+    const string& marker_pack_root_directory,
+    const map<string, Category>* marker_categories,
+    const vector<Parseable*>* parsed_pois) {
+    std::map<string, std::vector<Parseable*>> category_to_pois;
+    ProtoWriterState state;
+    state.marker_pack_root_directory = marker_pack_root_directory;
+
+    for (size_t i = 0; i < parsed_pois->size(); i++) {
+        Parseable* parsed_poi = (*parsed_pois)[i];
+        if (parsed_poi->classname() == "POI") {
+            Icon* icon = dynamic_cast<Icon*>(parsed_poi);
+            // TODO(331): This is the wrong place to lowercase() the category and is hiding some crimes elsewhere
+            category_to_pois[lowercase(icon->category.category)].push_back(parsed_poi);
+        }
+        else if (parsed_poi->classname() == "Trail") {
+            Trail* trail = dynamic_cast<Trail*>(parsed_poi);
+            // TODO(331): This is the wrong place to lowercase() the category and is hiding some crimes elsewhere
+            category_to_pois[lowercase(trail->category.category)].push_back(parsed_poi);
+        }
+        else {
+            std::cout << "Unknown type" << std::endl;
+        }
+    }
+
+    for (map<string, Category>::const_iterator it = marker_categories->begin(); it != marker_categories->end(); it++) {
+        StringHierarchy category_filter;
+        category_filter.add_path({it->first}, true);
+        _write_protobuf_file(
+            join_file_paths(state.marker_pack_root_directory, it->first + ".guildpoint"),
+            category_filter,
+            marker_categories,
+            category_to_pois,
+            &state);
+    }
+}
+


And then this function duplicates all of the logic of write_protobuf_file in order to access the earlier removed ability to pass in a category_fiter.

If the counter argument is the building of the category_to_pois dict for each file, aka: performance optimizations. I would necessarily have expected to see benchmarks somewhere in this PR

This function actually duplicates write_protobuf_file_per_map_id. Since that is now 3 places, perhaps the creation of category_to_pois should be its own function.

My other thought was that I am already iterating over the marker_categories map in order to get the name and would need a second iteration if I were to do it for the category filter first before this function is called.

AsherGlick · 2024-12-09T07:16:25Z

xml_converter/src/packaging_protobin.cpp

+    StringHierarchy category_filter;
+    category_filter.add_path({}, true);
+


Ditto

Why are you hardcoding the global wildcard here and preventing it from being passed in to the function?

AsherGlick · 2024-12-09T07:20:27Z

xml_converter/src/xml_converter.cpp

+    if (split_proto_by_category) {
+        write_protobuf_file_per_category(output_path, marker_categories, parsed_pois);
+    }
+    else {
+        write_protobuf_file(output_path, marker_categories, parsed_pois);
+    }


This logic should probably be nearby the logic for split by map id. It is convoluted that split by map id is in the parent function but split by category is in this child function.

I have actually considered moving map_id into this function but it would require reworking some other variables so thought it outside this PR scope.

AsherGlick · 2024-12-09T07:23:43Z

xml_converter/src/xml_converter.cpp

+        else if (!strcmp(argv[i], "--split-guildpoint-by-category")) {
+            split_proto_by_category = true;


What happens if --output-split-guildpoint-path and --split-guildpoint-by-category are both specified. Does the program act in a way that a user could reasonably expect? I think it wont, because it will only split by map id as the current logic for splitting by map id does not interact with the splitting by category logic.

Maybe this is the wrong argument to be exposing to the user? Think through some user stories for this holistically because we can still alter how other command line flags for xml_converter are presented to the user if we think there are better paradigms to use than what we are currently using.

klingbolt · 2024-12-09T23:34:17Z

I agree this PR is incomplete. It is also heavily affected by a bug in type that is addressed in #331. In hindsight I should have marked this as a draft.
I would like your opinion. This is now the third optional flag if you consider, split by mapid, split by category, and allow duplicates. I think it should be possible to want all three of these at the same time. Could we change the arguments to that there is one filepath argument and then all three of these, or any other writing modifiers, as options? Or should it be split so that every optional writing option is its own filepath.

AsherGlick · 2024-12-10T05:04:40Z

I have not given the ux much thought yet. But I would think about "is there a situation where we want to output multiple files". I could think that an automated service that is testing these would want to output multiple types at once, maybe for automatic downloads. And we use that within our test harness, but the harness could be changed.

Added an option to split the output proto files by top level category

7390e45