Interpolation bug fix

[yuzheyan]

Issue

This PR is targeting on the fix of interpolation bug. The previous logic has a flaw that, the interpolated locations are allowed to go beyond the boundaries of the actual matched point. See following example of interpolated point's (B) distance_along of the edge is smaller than matched point (A). Such behavior will lead to discontinuity for the serializer, the serializer thinks there is a discontinuity which is incorrect.

after the fix, the clip the distance_along of B to A's distance along.

Tasklist

• Add tests
• Add #fixes with the issue number that this PR addresses
• Generally use squash merge to rebase and clean comments before merging
• Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[kevinkreiser]

I would like to propose a simpler alternative that uses the interpolation to move the window rather than an unordered map and extra looping.

diff --git a/src/meili/map_matcher.cc b/src/meili/map_matcher.cc
index 3924651a2..21ef43470 100644
--- a/src/meili/map_matcher.cc
+++ b/src/meili/map_matcher.cc
@@ -35,6 +35,7 @@ struct Interpolation {
   float route_distance;
   float route_time;
   float edge_distance;
+  std::vector<EdgeSegment>::const_iterator segment;
 
   float sortcost(const EmissionCostModel& emission_model,
                  const TransitionCostModel& transition_model,
@@ -61,6 +62,7 @@ inline MatchResult CreateMatchResult(const Measurement& measurement, const Inter
 Interpolation InterpolateMeasurement(const MapMatcher& mapmatcher,
                                      const Measurement& measurement,
                                      std::vector<EdgeSegment>::const_iterator begin,
+                                     float begin_source_offset,
                                      std::vector<EdgeSegment>::const_iterator end,
                                      float match_measurement_distance,
                                      float match_measurement_time) {
@@ -75,6 +77,8 @@ Interpolation InterpolateMeasurement(const MapMatcher& mapmatcher,
   Interpolation best_interp;
 
   for (auto segment = begin; segment != end; segment++) {
+    // TODO: skip if this edgeid is the same as the previous one we dont need to check it twice
+
     const auto directededge = mapmatcher.graphreader().directededge(segment->edgeid, tile);
     if (!directededge) {
       continue;
@@ -96,6 +100,16 @@ Interpolation InterpolateMeasurement(const MapMatcher& mapmatcher,
       offset = 1.f - offset;
     }
 
+    // clip distance along to the match boundaries
+    if (segment == begin && offset < begin_source_offset) {
+      offset = begin_source_offset;
+      // TODO: update the snap to this offset along the edge (projected_point, sq_distance)
+    }
+    if (segment == end - 1 && offset > segment->target) {
+      offset = segment->target;
+      // TODO: update the snap to this offset along the edge (projected_point, sq_distance)
+    }
+
     // Distance from the projected point to the segment begin, or
     // segment begin if we walk reversely
     const auto distance_to_segment_ends =
@@ -109,8 +123,8 @@ Interpolation InterpolateMeasurement(const MapMatcher& mapmatcher,
     auto edge_percent = segment->target - segment->source;
     auto route_time = mapmatcher.costing()->EdgeCost(directededge, tile).secs * edge_percent;
 
-    Interpolation interp{projected_point, segment->edgeid, sq_distance,
-                         route_distance,  route_time,      offset};
+    Interpolation interp{projected_point, segment->edgeid, sq_distance, route_distance,
+                         route_time,      offset,          segment};
 
     const auto cost =
         interp.sortcost(mapmatcher.emission_cost_model(), mapmatcher.transition_cost_model(),
@@ -153,13 +167,22 @@ std::vector<MatchResult> InterpolateMeasurements(const MapMatcher& mapmatcher,
                                               mapmatcher.state_container().state(next_stateid));
 
   // for each point that needs interpolated
+  std::vector<EdgeSegment>::const_iterator left_most_segment = route.begin();
+  float left_most_offset = route.begin()->source;
   for (const auto& measurement : measurements) {
     const auto& match_measurement = mapmatcher.state_container().measurement(stateid.time());
     const auto match_measurement_distance = GreatCircleDistance(measurement, match_measurement);
     const auto match_measurement_time = ClockDistance(measurement, match_measurement);
+
     // interpolate this point along the route
-    const auto& interp = InterpolateMeasurement(mapmatcher, measurement, route.begin(), route.end(),
-                                                match_measurement_distance, match_measurement_time);
+    const auto interp =
+        InterpolateMeasurement(mapmatcher, measurement, left_most_segment, left_most_offset,
+                               route.end(), match_measurement_distance, match_measurement_time);
+
+    // shift the point at which we are allowed to start interpolating from to the right
+    // so that its on the interpolation point this way the next interpolation happens after
+    left_most_segment = interp.segment;
+    left_most_offset = interp.edge_distance;
 
     // if it was able to do the interpolation
     if (interp.edgeid.Is_Valid()) {

[kevinkreiser]

what is this doing, what tests do we have to show that this is doing what its supposed to

[yuzheyan]

yes, after the changes. Some existing unit test in the test matcher got broken if we do not have this protection.

[kevinkreiser]

this shouldnt be a reference and if you do the move iterator you need to remove const as well

[kevinkreiser]

excellent debugging once again!