Open-Harbor/osrm-backend
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 3 Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Moritz Kobitzsch 2017-01-07 02:10:02 +01:00 committed by Daniel Patterson
parent f7e8581a1b
commit 15c8fd326f
9 changed files with 132 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
features/guidance/turn-angles.feature
View File

@ -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 |

3
include/extractor/guidance/coordinate_extractor.hpp
View File

@ -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,

83
src/extractor/guidance/coordinate_extractor.cpp
View File

@ -93,23 +93,21 @@ 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
@ -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

15
src/extractor/guidance/intersection_generator.cpp
View File

@ -5,8 +5,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>
@ -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});
}

14
src/extractor/guidance/roundabout_handler.cpp
View File

@ -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;
}
}

15
src/extractor/guidance/sliproad_handler.cpp
View File

@ -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);

5
src/util/coordinate_calculation.cpp
View File

@ -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;
}

2
unit_tests/library/tile.cpp
View File

@ -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();

11
unit_tests/util/coordinate_calculation.cpp
View File

@ -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()