Remove assertions that could be triggered by bad data. (#3469)

When two consecutive nodes have identical coordinates, there is no valid
bearing.  For now, make equal nodes have bearing 0.

Full fix still needs to be done via #3470.

Author: Moritz Kobitzsch

features/guidance/turn-angles.feature

@@ -1260,3 +1260,29 @@ Feature: Simple Turns
         When I route I should get
             | waypoints | route              | intersections                                |
             | a,g       | ab,bcdefgh,bcdefgh | true:90;true:45 false:180 false:270;true:180 |
+
+    #https://github.com/Project-OSRM/osrm-backend/pull/3469#issuecomment-270806580
+    Scenario: Oszillating Lower Priority Road
+		#Given the node map
+	#		"""
+	#		a -db    c
+    #           f
+    #   	"""
+        Given the node locations
+            | node | lat                | lon                | #          |
+            | a    | 1.0                | 1.0                |            |
+            | b    | 1.0000179813587253 | 1.0                |            |
+            | c    | 1.0000204580571323 | 1.0                |            |
+            | d    | 1.0000179813587253 | 1.0                | same as b  |
+            | f    | 1.0000179813587253 | 1.0000179813587253 |            |
+
+        And the ways
+            | nodes | oneway | lanes | highway |
+            | ab    | yes    | 1     | primary |
+            | bf    | yes    | 1     | primary |
+            | bcd   | yes    | 1     | service |
+
+        # we don't care for turn instructions, this is a coordinate extraction bug check
+        When I route I should get
+            | waypoints | route |
+            | a,d       | ab,ab |

include/extractor/guidance/coordinate_extractor.hpp

@@ -28,7 +28,8 @@ class CoordinateExtractor
     /* Find a interpolated coordinate a long the compressed geometries. The desired coordinate
      * should be in a certain distance. This method is dedicated to find representative coordinates
      * at turns.
-     * Since we are computing the length of the segment anyhow, we also return it.
+     * Note: The segment between intersection and turn coordinate can be zero, if the OSM modelling
+     * is unfortunate. See https://github.com/Project-OSRM/osrm-backend/issues/3470
      */
     OSRM_ATTR_WARN_UNUSED
     util::Coordinate GetCoordinateAlongRoad(const NodeID intersection_node,

src/extractor/guidance/coordinate_extractor.cpp

@@ -94,32 +94,30 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     const std::uint8_t intersection_lanes,
     std::vector<util::Coordinate> coordinates) const
 {
-    const auto is_valid_result = [&](const util::Coordinate coordinate) {
+    // check if the coordinate is equal to the interseciton coordinate
+    const auto not_same_as_start = [&](const util::Coordinate coordinate) {
         return util::Coordinate(traversed_in_reverse
                                     ? node_coordinates[to_node]
                                     : node_coordinates[intersection_node]) != coordinate;
     };
     // this is only used for debug purposes in assertions. We don't want warnings about it
-    (void)is_valid_result;
-
-    // the lane count might not always be set. We need to assume a positive number, though. Here we
-    // select the number of lanes to operate on
-    const auto considered_lanes =
-        GetOffsetCorrectionFactor(node_based_graph.GetEdgeData(turn_edge).road_classification) *
-        ((intersection_lanes == 0) ? ASSUMED_LANE_COUNT : intersection_lanes);
+    (void)not_same_as_start;
 
     // Fallback. These roads are small broken self-loops that shouldn't be in the data at all
     if (intersection_node == to_node)
+    {
+        BOOST_ASSERT(coordinates.size() >= 2);
         return coordinates[1];
+    }
 
     /* if we are looking at a straight line, we don't care where exactly the coordinate
      * is. Simply return the final coordinate. Turn angles/turn vectors are the same no matter which
      * coordinate we look at.
      */
     if (coordinates.size() <= 2)
     {
-        // Here we can't check for validity, due to possible dead-ends with repeated coordinates
-        // BOOST_ASSERT(is_valid_result(coordinates.back()));
+        // TODO: possibly re-enable with https://github.com/Project-OSRM/osrm-backend/issues/3470
+        // BOOST_ASSERT(not_same_as_start(result));
         return coordinates.back();
     }
 
@@ -130,25 +128,20 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     // fallback, mostly necessary for dead ends
     if (intersection_node == to_node)
     {
-        const auto result = ExtractCoordinateAtLength(
-            skipping_inaccuracies_distance, coordinates);
-        BOOST_ASSERT(is_valid_result(result));
+        const auto result = ExtractCoordinateAtLength(skipping_inaccuracies_distance, coordinates);
+        // TODO: possibly re-enable with https://github.com/Project-OSRM/osrm-backend/issues/3470
+        // BOOST_ASSERT(not_same_as_start(result));
         return result;
     }
 
-    // If this reduction leaves us with only two coordinates, the turns/angles are represented in a
-    // valid way. Only curved roads and other difficult scenarios will require multiple coordinates.
-    if (coordinates.size() == 2)
-        return coordinates.back();
-
     const auto &turn_edge_data = node_based_graph.GetEdgeData(turn_edge);
 
     // roundabouts, check early to avoid other costly checks
     if (turn_edge_data.roundabout || turn_edge_data.circular)
     {
-        const auto result = ExtractCoordinateAtLength(
-            skipping_inaccuracies_distance, coordinates);
-        BOOST_ASSERT(is_valid_result(result));
+        const auto result = ExtractCoordinateAtLength(skipping_inaccuracies_distance, coordinates);
+        // TODO: possibly re-enable with https://github.com/Project-OSRM/osrm-backend/issues/3470
+        // BOOST_ASSERT(not_same_as_start(result));
         return result;
     }
 
@@ -169,21 +162,34 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
             util::coordinate_calculation::haversineDistance(turn_coordinate, coordinates[1]) <
                 ASSUMED_LANE_WIDTH)
         {
-            const auto result =
-                GetCorrectedCoordinate(turn_coordinate, coordinates[1], coordinates.back());
-            BOOST_ASSERT(is_valid_result(result));
-            return result;
-        }
-        else
-        {
-            BOOST_ASSERT(is_valid_result(coordinates.back()));
-            return coordinates.back();
+            const auto initial_distance =
+                util::coordinate_calculation::haversineDistance(turn_coordinate, coordinates[1]);
+            const auto total_distance = util::coordinate_calculation::haversineDistance(
+                turn_coordinate, coordinates.back());
+
+            if (initial_distance > ASSUMED_LANE_WIDTH && total_distance > initial_distance)
+            {
+                const auto result =
+                    GetCorrectedCoordinate(turn_coordinate, coordinates[1], coordinates.back());
+                BOOST_ASSERT(not_same_as_start(result));
+                return result;
+            }
         }
+        // TODO: possibly re-enable with
+        // https://github.com/Project-OSRM/osrm-backend/issues/3470
+        // BOOST_ASSERT(not_same_as_start(coordinates.back()));
+        return coordinates.back();
     }
 
     const auto first_distance =
         util::coordinate_calculation::haversineDistance(coordinates[0], coordinates[1]);
 
+    // the lane count might not always be set. We need to assume a positive number, though. Here we
+    // select the number of lanes to operate on
+    const auto considered_lanes =
+        GetOffsetCorrectionFactor(node_based_graph.GetEdgeData(turn_edge).road_classification) *
+        ((intersection_lanes == 0) ? ASSUMED_LANE_COUNT : intersection_lanes);
+
     /* if the very first coordinate along the road is reasonably far away from the road, we assume
      * the coordinate to correctly represent the turn. This could probably be improved using
      * information on the very first turn angle (requires knowledge about previous road) and the
@@ -197,7 +203,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
 
     if (first_coordinate_is_far_away)
     {
-        BOOST_ASSERT(is_valid_result(coordinates[1]));
+        BOOST_ASSERT(not_same_as_start(coordinates[1]));
         return coordinates[1];
     }
 
@@ -237,7 +243,8 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     if (coordinates.size() == 2 ||
         total_distance <= skipping_inaccuracies_distance)
     {
-        BOOST_ASSERT(is_valid_result(coordinates.back()));
+        // TODO: possibly re-enable with https://github.com/Project-OSRM/osrm-backend/issues/3470
+        // BOOST_ASSERT(not_same_as_start(coordinates.back()));
         return coordinates.back();
     }
 
@@ -252,14 +259,14 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
         // As a back-up, we have to check for this case
         if (coordinates.front() == coordinates.back())
         {
-            const auto result = ExtractCoordinateAtLength(
-                skipping_inaccuracies_distance, coordinates);
-            BOOST_ASSERT(is_valid_result(result));
+            const auto result =
+                ExtractCoordinateAtLength(skipping_inaccuracies_distance, coordinates);
+            BOOST_ASSERT(not_same_as_start(result));
             return result;
         }
         else
         {
-            BOOST_ASSERT(is_valid_result(coordinates.back()));
+            BOOST_ASSERT(not_same_as_start(coordinates.back()));
             return coordinates.back();
         }
     }
@@ -299,7 +306,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     {
         // skip over repeated coordinates
         const auto result = ExtractCoordinateAtLength(5, coordinates, segment_distances);
-        BOOST_ASSERT(is_valid_result(result));
+        BOOST_ASSERT(not_same_as_start(result));
         return result;
     }
 
@@ -333,7 +340,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
                 .second;
         const auto result =
             GetCorrectedCoordinate(turn_coordinate, coord_between_front, coord_between_back);
-        BOOST_ASSERT(is_valid_result(result));
+        BOOST_ASSERT(not_same_as_start(result));
         return result;
     }
 
@@ -354,7 +361,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
         const auto result = GetCorrectedCoordinate(
             turn_coordinate, coordinates[offset_index], coordinates[offset_index + 1]);
 
-        BOOST_ASSERT(is_valid_result(result));
+        BOOST_ASSERT(not_same_as_start(result));
         return result;
     }
 
@@ -388,7 +395,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
         BOOST_ASSERT(coordinates.size() >= 2);
         const auto result =
             GetCorrectedCoordinate(turn_coordinate, coordinates.back(), vector_head);
-        BOOST_ASSERT(is_valid_result(result));
+        BOOST_ASSERT(not_same_as_start(result));
         return result;
     }
 
@@ -414,7 +421,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
             {
                 const auto result = GetCorrectedCoordinate(
                     turn_coordinate, regression_line_trimmed.first, regression_line_trimmed.second);
-                BOOST_ASSERT(is_valid_result(result));
+                BOOST_ASSERT(not_same_as_start(result));
                 return result;
             }
         }
@@ -424,7 +431,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     // intersection.
     const auto result =
         ExtractCoordinateAtLength(LOOKAHEAD_DISTANCE_WITHOUT_LANES, coordinates, segment_distances);
-    BOOST_ASSERT(is_valid_result(result));
+    BOOST_ASSERT(not_same_as_start(result));
     return result;
 }
 
@@ -989,7 +996,9 @@ CoordinateExtractor::GetCorrectedCoordinate(const util::Coordinate fixpoint,
     // we can use the end-coordinate
     if (util::coordinate_calculation::haversineDistance(vector_base, vector_head) <
         DESIRED_COORDINATE_DIFFERENCE)
+    {
         return vector_head;
+    }
     else
     {
         /* to correct for the initial offset, we move the lookahead coordinate close

src/extractor/guidance/intersection_generator.cpp

@@ -2,8 +2,10 @@
 
 #include "util/bearing.hpp"
 #include "util/coordinate_calculation.hpp"
+#include "util/log.hpp"
 
 #include <algorithm>
+#include <cmath>
 #include <functional> // mem_fn
 #include <limits>
 #include <numeric>
@@ -101,6 +103,19 @@ IntersectionGenerator::ComputeIntersectionShape(const NodeID node_at_center_of_i
         bearing =
             util::coordinate_calculation::bearing(turn_coordinate, coordinate_along_edge_leaving);
 
+        // OSM data sometimes contains duplicated nodes with identical coordinates, or
+        // because of coordinate precision rounding, end up at the same coordinate.
+        // It's impossible to calculate a bearing between these, so we log a warning
+        // that the data should be checked.
+        // The bearing calculation should return 0 in these cases, which may not be correct,
+        // but is at least not random.
+        if (turn_coordinate == coordinate_along_edge_leaving)
+        {
+            util::Log(logDEBUG) << "Zero length segment at " << coordinate_along_edge_leaving
+                                << " could cause invalid intersection exit bearing.";
+            BOOST_ASSERT(std::abs(bearing) <= 0.1);
+        }
+
         intersection.push_back({edge_connected_to_intersection, bearing, segment_length});
     }
 

src/extractor/guidance/roundabout_handler.cpp

@@ -204,6 +204,20 @@ bool RoundaboutHandler::qualifiesAsRoundaboutIntersection(
 
                 result.push_back(
                     util::coordinate_calculation::bearing(src_coordinate, next_coordinate));
+
+                // OSM data sometimes contains duplicated nodes with identical coordinates, or
+                // because of coordinate precision rounding, end up at the same coordinate.
+                // It's impossible to calculate a bearing between these, so we log a warning
+                // that the data should be checked.
+                // The bearing calculation should return 0 in these cases, which may not be correct,
+                // but is at least not random.
+                if (src_coordinate == next_coordinate)
+                {
+                    util::Log(logDEBUG) << "Zero length segment at " << next_coordinate
+                                        << " could cause invalid roundabout exit bearings";
+                    BOOST_ASSERT(std::abs(result.back()) <= 0.1);
+                }
+
                 break;
             }
         }

src/extractor/guidance/sliproad_handler.cpp

@@ -301,12 +301,18 @@ operator()(const NodeID /*nid*/, const EdgeID source_edge_id, Intersection inter
         //
         //                    Sliproad           Not a Sliproad
         {
-            const auto &extractor = intersection_generator.GetCoordinateExtractor();
+            const auto &coordinate_extractor = intersection_generator.GetCoordinateExtractor();
 
             const NodeID start = intersection_node_id; // b
             const EdgeID edge = sliproad_edge;         // bd
 
-            const auto coords = extractor.GetForwardCoordinatesAlongRoad(start, edge);
+            const auto coords = coordinate_extractor.GetForwardCoordinatesAlongRoad(start, edge);
+
+            // due to filtering of duplicated coordinates, we can end up with empty segments.
+            // this can only happen as long as
+            // https://github.com/Project-OSRM/osrm-backend/issues/3470 persists
+            if (coords.size() < 2)
+                continue;
             BOOST_ASSERT(coords.size() >= 2);
 
             // Now keep start and end coordinate fix and check for curvature
@@ -544,14 +550,15 @@ bool SliproadHandler::nextIntersectionIsTooFarAway(const NodeID start, const Edg
     BOOST_ASSERT(start != SPECIAL_NODEID);
     BOOST_ASSERT(onto != SPECIAL_EDGEID);
 
-    const auto &extractor = intersection_generator.GetCoordinateExtractor();
+    const auto &coordinate_extractor = intersection_generator.GetCoordinateExtractor();
 
     // Base max distance threshold on the current road class we're on
     const auto &data = node_based_graph.GetEdgeData(onto);
     const auto threshold = scaledThresholdByRoadClass(MAX_SLIPROAD_THRESHOLD, // <- scales down
                                                       data.road_classification);
 
-    DistanceToNextIntersectionAccumulator accumulator{extractor, node_based_graph, threshold};
+    DistanceToNextIntersectionAccumulator accumulator{
+        coordinate_extractor, node_based_graph, threshold};
     const SkipTrafficSignalBarrierRoadSelector selector{};
 
     (void)graph_walker.TraverseRoad(start, onto, accumulator, selector);

src/util/coordinate_calculation.cpp

@@ -152,6 +152,11 @@ double bearing(const Coordinate first_coordinate, const Coordinate second_coordi
     {
         result -= 360.0;
     }
+    // If someone gives us two identical coordinates, then the concept of a bearing
+    // makes no sense.  However, because it sometimes happens, we'll at least
+    // return a consistent value of 0 so that the behaviour isn't random.
+    BOOST_ASSERT(first_coordinate != second_coordinate || result == 0.);
+
     return result;
 }
 

unit_tests/library/tile.cpp

@@ -32,7 +32,7 @@ BOOST_AUTO_TEST_CASE(test_tile)
     const auto rc = osrm.Tile(params, result);
     BOOST_CHECK(rc == Status::Ok);
 
-    BOOST_CHECK_EQUAL(result.size(), 114091);
+    BOOST_CHECK_EQUAL(result.size(), 114098);
 
     protozero::pbf_reader tile_message(result);
     tile_message.next();

unit_tests/util/coordinate_calculation.cpp

@@ -311,4 +311,15 @@ BOOST_AUTO_TEST_CASE(circleCenter)
     BOOST_CHECK(!result);
 }
 
+BOOST_AUTO_TEST_CASE(consistent_invalid_bearing_result)
+{
+    const auto pos1 = Coordinate(util::FloatLongitude{0.}, util::FloatLatitude{0.});
+    const auto pos2 = Coordinate(util::FloatLongitude{5.}, util::FloatLatitude{5.});
+    const auto pos3 = Coordinate(util::FloatLongitude{-5.}, util::FloatLatitude{-5.});
+
+    BOOST_CHECK_EQUAL(0., util::coordinate_calculation::bearing(pos1, pos1));
+    BOOST_CHECK_EQUAL(0., util::coordinate_calculation::bearing(pos2, pos2));
+    BOOST_CHECK_EQUAL(0., util::coordinate_calculation::bearing(pos3, pos3));
+}
+
 BOOST_AUTO_TEST_SUITE_END()