Loop match walk or snap fix (valhalla#4895)

CHANGELOG.md

@@ -3,6 +3,7 @@
 * **Bug Fix**
    * FIXED: All logging in `valhalla_export_edges` now goes to stderr [#4892](https://github.com/valhalla/valhalla/pull/4892)
    * FIXED: Iterate over only `kLandmark` tagged values in `AddLandmarks()` [#4873](https://github.com/valhalla/valhalla/pull/4873)
+   * FIXED: `walk_or_snap` mode edge case with loop routes [#4895](https://github.com/valhalla/valhalla/pull/4895)
 * **Enhancement**
    * CHANGED: voice instructions for OSRM serializer to work better in real-world environment [#4756](https://github.com/valhalla/valhalla/pull/4756)
    * ADDED: Add option `edge.forward` to trace attributes [#4876](https://github.com/valhalla/valhalla/pull/4876)

src/thor/route_matcher.cc

@@ -492,20 +492,23 @@ bool RouteMatcher::FormPath(const sif::mode_costing_t& mode_costing,
       index++;
     }
 
-    // Did not find the end of the origin edge. Check for trivial route on a single edge
-    for (const auto& end : end_nodes) {
-      if (end.second.first.graph_id() == edge.graph_id()) {
-        // Update the elapsed time based on edge cost
-        uint8_t flow_sources;
-        elapsed += mode_costing[static_cast<int>(mode)]->EdgeCost(de, end_node_tile, time_info,
-                                                                  flow_sources) *
-                   (end.second.first.percent_along() - edge.percent_along());
-        if (options.use_timestamps())
-          elapsed.secs = options.shape().rbegin()->time() - options.shape(0).time();
+    // Look for trivial cases if we didn't bail based on checking more than the edge length
+    if (length <= de_length) {
+      // Did not find the end of the origin edge. Check for trivial route on a single edge
+      for (const auto& end : end_nodes) {
+        if (end.second.first.graph_id() == edge.graph_id()) {
+          // Update the elapsed time based on edge cost
+          uint8_t flow_sources;
+          elapsed += mode_costing[static_cast<int>(mode)]->EdgeCost(de, end_node_tile, time_info,
+                                                                    flow_sources) *
+                     (end.second.first.percent_along() - edge.percent_along());
+          if (options.use_timestamps())
+            elapsed.secs = options.shape().rbegin()->time() - options.shape(0).time();
 
-        // Add end edge
-        path_infos.emplace_back(mode, elapsed, GraphId(edge.graph_id()), 0, 0.f, -1);
-        return true;
+          // Add end edge
+          path_infos.emplace_back(mode, elapsed, GraphId(edge.graph_id()), 0, 0.f, -1);
+          return true;
+        }
       }
     }
   }

test/mapmatch.cc

@@ -1232,6 +1232,33 @@ TEST(Mapmatch, test_loop_matching) {
   }
 }
 
+TEST(Mapmatch, test_loop_matching_walk_or_snap) {
+  // The walk_or_snap case sometimes incorrectly identifies trivial cases where it can optimize away.
+  // This tests loops which are not trivial.
+  std::vector<std::string> test_cases = {
+      // Simple loop around a block; incorrectly identified as trivial in such a way that
+      // it caused an exception in TripLegBuilder (no maneuvers)
+      R"({"shape_match":"walk_or_snap", "shape":[
+          {"lat": 52.11870746741026, "lon": 5.117783546447755, "type": "break", "node_snap_tolerance": 0},
+          {"lat": 52.11905661947785, "lon": 5.118234157562257, "type": "via", "node_snap_tolerance": 0},
+          {"lat": 52.11934977334695, "lon": 5.117751359939576, "type": "via", "node_snap_tolerance": 0},
+          {"lat": 52.118852398789194, "lon": 5.116935968399049, "type": "via", "node_snap_tolerance": 0},
+          {"lat": 52.11852959643748, "lon": 5.117445588111878, "type": "via", "node_snap_tolerance": 0},
+          {"lat": 52.11870746741026, "lon": 5.117783546447755, "type": "break", "node_snap_tolerance": 0}],
+          "costing":"auto"})",
+  };
+
+  api_tester tester;
+  for (size_t i = 0; i < test_cases.size(); ++i) {
+    // Verify that the map_snap output matches the walk_or_snap output.
+    // This approach is inspired by the above.
+    auto map_snap_case = R"({"shape_match":"map_snap)" + test_cases[i].substr(28);
+    auto map_snapped = tester.match(map_snap_case);
+    auto matched = tester.match(test_cases[i]);
+    compare_results(map_snapped, matched);
+  }
+}
+
 TEST(Mapmatch, test_intersection_matching) {
   std::vector<std::string> test_cases = {
       R"({"shape":[