From 15c8fd326f96ea41382a202f06d88dbc5c63a1e4 Mon Sep 17 00:00:00 2001 From: Moritz Kobitzsch Date: Sat, 7 Jan 2017 02:10:02 +0100 Subject: [PATCH] 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 https://github.com/Project-OSRM/osrm-backend/issues/3470. --- features/guidance/turn-angles.feature | 26 ++++++ .../guidance/coordinate_extractor.hpp | 3 +- .../guidance/coordinate_extractor.cpp | 83 +++++++++++-------- .../guidance/intersection_generator.cpp | 15 ++++ src/extractor/guidance/roundabout_handler.cpp | 14 ++++ src/extractor/guidance/sliproad_handler.cpp | 15 +++- src/util/coordinate_calculation.cpp | 5 ++ unit_tests/library/tile.cpp | 2 +- unit_tests/util/coordinate_calculation.cpp | 11 +++ 9 files changed, 132 insertions(+), 42 deletions(-) diff --git a/features/guidance/turn-angles.feature b/features/guidance/turn-angles.feature index 1d2561a51..eac73c482 100644 --- a/features/guidance/turn-angles.feature +++ b/features/guidance/turn-angles.feature @@ -1314,3 +1314,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 | diff --git a/include/extractor/guidance/coordinate_extractor.hpp b/include/extractor/guidance/coordinate_extractor.hpp index 08acd8d01..9927919fd 100644 --- a/include/extractor/guidance/coordinate_extractor.hpp +++ b/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, diff --git a/src/extractor/guidance/coordinate_extractor.cpp b/src/extractor/guidance/coordinate_extractor.cpp index 5697a9abb..455d7c7c1 100644 --- a/src/extractor/guidance/coordinate_extractor.cpp +++ b/src/extractor/guidance/coordinate_extractor.cpp @@ -93,23 +93,21 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate( const std::uint8_t intersection_lanes, std::vector 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 @@ -117,8 +115,8 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate( */ 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,22 +128,19 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate( if (intersection_node == to_node) { const auto result = ExtractCoordinateAtLength(skipping_inaccuracies_distance, coordinates); - BOOST_ASSERT(is_valid_result(result)); + // 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)); + // TODO: possibly re-enable with https://github.com/Project-OSRM/osrm-backend/issues/3470 + // BOOST_ASSERT(not_same_as_start(result)); return result; } @@ -166,21 +161,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 @@ -194,7 +202,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]; } @@ -233,7 +241,8 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate( // if we are now left with two, well than we don't have to worry, or the segment is very small 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(); } @@ -250,12 +259,12 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate( { const auto result = ExtractCoordinateAtLength(skipping_inaccuracies_distance, coordinates); - BOOST_ASSERT(is_valid_result(result)); + 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(); } } @@ -295,7 +304,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; } @@ -329,7 +338,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; } @@ -350,7 +359,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; } @@ -381,7 +390,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; } @@ -407,7 +416,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; } } @@ -417,7 +426,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; } @@ -984,7 +993,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 diff --git a/src/extractor/guidance/intersection_generator.cpp b/src/extractor/guidance/intersection_generator.cpp index 98cb57318..c2f114b21 100644 --- a/src/extractor/guidance/intersection_generator.cpp +++ b/src/extractor/guidance/intersection_generator.cpp @@ -5,8 +5,10 @@ #include "util/bearing.hpp" #include "util/coordinate_calculation.hpp" +#include "util/log.hpp" #include +#include #include // mem_fn #include #include @@ -106,6 +108,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}); } diff --git a/src/extractor/guidance/roundabout_handler.cpp b/src/extractor/guidance/roundabout_handler.cpp index fb1c7e805..b0cca8975 100644 --- a/src/extractor/guidance/roundabout_handler.cpp +++ b/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; } } diff --git a/src/extractor/guidance/sliproad_handler.cpp b/src/extractor/guidance/sliproad_handler.cpp index 338d49edf..16ed2e2c4 100644 --- a/src/extractor/guidance/sliproad_handler.cpp +++ b/src/extractor/guidance/sliproad_handler.cpp @@ -300,12 +300,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 @@ -543,14 +549,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); diff --git a/src/util/coordinate_calculation.cpp b/src/util/coordinate_calculation.cpp index 540c6d593..cae9a34ce 100644 --- a/src/util/coordinate_calculation.cpp +++ b/src/util/coordinate_calculation.cpp @@ -145,6 +145,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; } diff --git a/unit_tests/library/tile.cpp b/unit_tests/library/tile.cpp index 0f204e5dc..0ceb0f678 100644 --- a/unit_tests/library/tile.cpp +++ b/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(); diff --git a/unit_tests/util/coordinate_calculation.cpp b/unit_tests/util/coordinate_calculation.cpp index 21f41e763..7bd256b5b 100644 --- a/unit_tests/util/coordinate_calculation.cpp +++ b/unit_tests/util/coordinate_calculation.cpp @@ -378,4 +378,15 @@ BOOST_AUTO_TEST_CASE(parallel_lines_slight_offset) BOOST_CHECK(are_parallel); } +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()