From d012b44b7f433fe91a438338ee1d0375700b623f Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Thu, 28 Jul 2016 14:09:55 -0700 Subject: [PATCH] Filter out edges that have any speed=0 segments. They become non-snappable and non-routable. Note that a single segment of speed=0 will eliminate the entire edge. --- features/car/traffic_speeds.feature | 66 +++++++++++++++++---- include/engine/geospatial_query.hpp | 89 +++++++++++++++++++++++----- src/contractor/contractor.cpp | 15 ++++- unit_tests/mocks/mock_datafacade.hpp | 4 +- unit_tests/util/static_rtree.cpp | 3 + 5 files changed, 149 insertions(+), 28 deletions(-) diff --git a/features/car/traffic_speeds.feature b/features/car/traffic_speeds.feature index 84335d0f8..1a0104a30 100644 --- a/features/car/traffic_speeds.feature +++ b/features/car/traffic_speeds.feature @@ -23,8 +23,8 @@ Feature: Traffic - speeds | fb | primary | And the speed file """ - 1,2,27 - 2,1,27 + 1,2,0 + 2,1,0 2,3,27 3,2,27 1,4,27 @@ -36,12 +36,58 @@ Feature: Traffic - speeds Given the extract extra arguments "--generate-edge-lookup" Given the contract extra arguments "--segment-speed-file speeds.csv" And I route I should get - | from | to | route | speed | - | a | b | ab,ab | 27 km/h | - | a | c | ab,bc,bc | 27 km/h | - | b | c | bc,bc | 27 km/h | - | a | d | ad,ad | 27 km/h | - | d | c | dc,dc | 36 km/h | - | g | b | ab,ab | 27 km/h | - | a | g | ab,ab | 27 km/h | + | from | to | route | speed | + | a | b | ad,de,eb,eb | 30 km/h | + | a | c | ad,dc,dc | 31 km/h | + | b | c | bc,bc | 27 km/h | + | a | d | ad,ad | 27 km/h | + | d | c | dc,dc | 36 km/h | + | g | b | fb,fb | 36 km/h | + | a | g | ad,df,fb,fb | 30 km/h | + + Scenario: Speeds that isolate a single node (a) + Given the profile "testbot" + Given the extract extra arguments "--generate-edge-lookup" + Given the contract extra arguments "--segment-speed-file speeds.csv" + Given the speed file + """ + 1,2,0 + 2,1,0 + 2,3,27 + 3,2,27 + 1,4,0 + 4,1,0 + """ + And I route I should get + | from | to | route | speed | + | a | b | fb,fb | 36 km/h | + | a | c | fb,bc,bc | 30 km/h | + | b | c | bc,bc | 27 km/h | + | a | d | fb,df,df | 36 km/h | + | d | c | dc,dc | 36 km/h | + | g | b | fb,fb | 36 km/h | + | a | g | fb,fb | 36 km/h | + + Scenario: Verify that negative values are treated like 0 + Given the profile "testbot" + Given the extract extra arguments "--generate-edge-lookup" + Given the contract extra arguments "--segment-speed-file speeds.csv" + Given the speed file + """ + 1,2,-10 + 2,1,-20 + 2,3,27 + 3,2,27 + 1,4,-3 + 4,1,-5 + """ + And I route I should get + | from | to | route | speed | + | a | b | fb,fb | 36 km/h | + | a | c | fb,bc,bc | 30 km/h | + | b | c | bc,bc | 27 km/h | + | a | d | fb,df,df | 36 km/h | + | d | c | dc,dc | 36 km/h | + | g | b | fb,fb | 36 km/h | + | a | g | fb,fb | 36 km/h | diff --git a/include/engine/geospatial_query.hpp b/include/engine/geospatial_query.hpp index f61dfebce..f115b7d96 100644 --- a/include/engine/geospatial_query.hpp +++ b/include/engine/geospatial_query.hpp @@ -20,6 +20,11 @@ namespace osrm namespace engine { +inline std::pair boolPairAnd(const std::pair &A, const std::pair &B) +{ + return std::make_pair(A.first && B.first, A.second && B.second); +} + // Implements complex queries on top of an RTree and builds PhantomNodes from it. // // Only holds a weak reference on the RTree and coordinates! @@ -48,7 +53,7 @@ template class GeospatialQuery { auto results = rtree.Nearest(input_coordinate, - [](const CandidateSegment &) { return std::make_pair(true, true); }, + [this](const CandidateSegment &segment) { return HasValidEdge(segment); }, [this, max_distance, input_coordinate](const std::size_t, const CandidateSegment &segment) { return CheckSegmentDistance(input_coordinate, segment, max_distance); @@ -68,7 +73,7 @@ template class GeospatialQuery auto results = rtree.Nearest( input_coordinate, [this, bearing, bearing_range, max_distance](const CandidateSegment &segment) { - return CheckSegmentBearing(segment, bearing, bearing_range); + return boolPairAnd(CheckSegmentBearing(segment, bearing, bearing_range),HasValidEdge(segment)); }, [this, max_distance, input_coordinate](const std::size_t, const CandidateSegment &segment) { @@ -89,7 +94,7 @@ template class GeospatialQuery auto results = rtree.Nearest(input_coordinate, [this, bearing, bearing_range](const CandidateSegment &segment) { - return CheckSegmentBearing(segment, bearing, bearing_range); + return boolPairAnd(CheckSegmentBearing(segment, bearing, bearing_range), HasValidEdge(segment)); }, [max_results](const std::size_t num_results, const CandidateSegment &) { return num_results >= max_results; @@ -111,7 +116,7 @@ template class GeospatialQuery auto results = rtree.Nearest(input_coordinate, [this, bearing, bearing_range](const CandidateSegment &segment) { - return CheckSegmentBearing(segment, bearing, bearing_range); + return boolPairAnd(CheckSegmentBearing(segment, bearing, bearing_range), HasValidEdge(segment)); }, [this, max_distance, max_results, input_coordinate]( const std::size_t num_results, const CandidateSegment &segment) { @@ -129,7 +134,7 @@ template class GeospatialQuery { auto results = rtree.Nearest(input_coordinate, - [](const CandidateSegment &) { return std::make_pair(true, true); }, + [this](const CandidateSegment &segment) { return HasValidEdge(segment); }, [max_results](const std::size_t num_results, const CandidateSegment &) { return num_results >= max_results; }); @@ -146,7 +151,7 @@ template class GeospatialQuery { auto results = rtree.Nearest(input_coordinate, - [](const CandidateSegment &) { return std::make_pair(true, true); }, + [this](const CandidateSegment &segment) { return HasValidEdge(segment); }, [this, max_distance, max_results, input_coordinate]( const std::size_t num_results, const CandidateSegment &segment) { return num_results >= max_results || @@ -166,14 +171,18 @@ template class GeospatialQuery bool has_big_component = false; auto results = rtree.Nearest( input_coordinate, - [&has_big_component, &has_small_component](const CandidateSegment &segment) { + [this, &has_big_component, &has_small_component](const CandidateSegment &segment) { auto use_segment = (!has_small_component || (!has_big_component && !segment.data.component.is_tiny)); auto use_directions = std::make_pair(use_segment, use_segment); + const auto valid_edges = HasValidEdge(segment); - has_big_component = has_big_component || !segment.data.component.is_tiny; - has_small_component = has_small_component || segment.data.component.is_tiny; - + if (valid_edges.first || valid_edges.second) + { + has_big_component = has_big_component || !segment.data.component.is_tiny; + has_small_component = has_small_component || segment.data.component.is_tiny; + } + use_directions = boolPairAnd(use_directions, valid_edges); return use_directions; }, [this, &has_big_component, max_distance, input_coordinate]( @@ -201,14 +210,21 @@ template class GeospatialQuery bool has_big_component = false; auto results = rtree.Nearest( input_coordinate, - [&has_big_component, &has_small_component](const CandidateSegment &segment) { + [this, &has_big_component, &has_small_component](const CandidateSegment &segment) { auto use_segment = (!has_small_component || (!has_big_component && !segment.data.component.is_tiny)); auto use_directions = std::make_pair(use_segment, use_segment); + if (!use_directions.first && !use_directions.second) return use_directions; + const auto valid_edges = HasValidEdge(segment); - has_big_component = has_big_component || !segment.data.component.is_tiny; - has_small_component = has_small_component || segment.data.component.is_tiny; + if (valid_edges.first || valid_edges.second) + { + has_big_component = has_big_component || !segment.data.component.is_tiny; + has_small_component = has_small_component || segment.data.component.is_tiny; + } + + use_directions = boolPairAnd(use_directions, valid_edges); return use_directions; }, [&has_big_component](const std::size_t num_results, const CandidateSegment &) { @@ -239,10 +255,11 @@ template class GeospatialQuery auto use_segment = (!has_small_component || (!has_big_component && !segment.data.component.is_tiny)); auto use_directions = std::make_pair(use_segment, use_segment); + use_directions = boolPairAnd(use_directions, HasValidEdge(segment)); if (use_segment) { - use_directions = CheckSegmentBearing(segment, bearing, bearing_range); + use_directions = boolPairAnd(CheckSegmentBearing(segment, bearing, bearing_range), HasValidEdge(segment)); if (use_directions.first || use_directions.second) { has_big_component = has_big_component || !segment.data.component.is_tiny; @@ -283,10 +300,11 @@ template class GeospatialQuery auto use_segment = (!has_small_component || (!has_big_component && !segment.data.component.is_tiny)); auto use_directions = std::make_pair(use_segment, use_segment); + use_directions = boolPairAnd(use_directions, HasValidEdge(segment)); if (use_segment) { - use_directions = CheckSegmentBearing(segment, bearing, bearing_range); + use_directions = boolPairAnd(CheckSegmentBearing(segment, bearing, bearing_range), HasValidEdge(segment)); if (use_directions.first || use_directions.second) { has_big_component = has_big_component || !segment.data.component.is_tiny; @@ -440,6 +458,47 @@ template class GeospatialQuery return std::make_pair(forward_bearing_valid, backward_bearing_valid); } + /** + * Checks to see if the edge weights are valid. We might have an edge, + * but a traffic update might set the speed to 0 (weight == INVALID_EDGE_WEIGHT). + * which means that this edge is not currently traversible. If this is the case, + * then we shouldn't snap to this edge. + */ + std::pair HasValidEdge(const CandidateSegment &segment) const + { + + bool forward_edge_valid = false; + bool reverse_edge_valid = false; + + if (segment.data.forward_packed_geometry_id != SPECIAL_EDGEID) + { + std::vector forward_weight_vector; + datafacade.GetUncompressedWeights(segment.data.forward_packed_geometry_id, + forward_weight_vector); + + if (forward_weight_vector[segment.data.fwd_segment_position] != INVALID_EDGE_WEIGHT) + { + forward_edge_valid = segment.data.forward_segment_id.enabled; + } + } + + if (segment.data.reverse_packed_geometry_id != SPECIAL_EDGEID) + { + std::vector reverse_weight_vector; + datafacade.GetUncompressedWeights(segment.data.reverse_packed_geometry_id, + reverse_weight_vector); + + BOOST_ASSERT(segment.data.fwd_segment_position < reverse_weight_vector.size()); + + if (reverse_weight_vector[reverse_weight_vector.size() - segment.data.fwd_segment_position - 1] != INVALID_EDGE_WEIGHT) + { + reverse_edge_valid = segment.data.reverse_segment_id.enabled; + } + } + + return std::make_pair(forward_edge_valid, reverse_edge_valid); + } + const RTreeT &rtree; const CoordinateList &coordinates; DataFacadeT &datafacade; diff --git a/src/contractor/contractor.cpp b/src/contractor/contractor.cpp index 13bd739d0..ea6437f69 100644 --- a/src/contractor/contractor.cpp +++ b/src/contractor/contractor.cpp @@ -758,6 +758,7 @@ EdgeID Contractor::LoadEdgeExpandedGraph( if (update_edge_weights || update_turn_penalties) { + bool skip_this_edge = false; auto header = reinterpret_cast( edge_segment_byte_ptr); edge_segment_byte_ptr += sizeof(extractor::lookup::SegmentHeaderBlock); @@ -787,8 +788,11 @@ EdgeID Contractor::LoadEdgeExpandedGraph( } else { - // This edge is blocked, we don't need to continue updating - new_weight = INVALID_EDGE_WEIGHT; + // If we hit a 0-speed edge, then it's effectively not traversible. + // We don't want to include it in the edge_based_edge_list, so + // we set a flag and `continue` the parent loop as soon as we can. + // This would be a perfect place to use `goto`, but Patrick vetoed it. + skip_this_edge = true; break; } } @@ -801,6 +805,13 @@ EdgeID Contractor::LoadEdgeExpandedGraph( previous_osm_node_id = segmentblocks[i].this_osm_node_id; } + // We found a zero-speed edge, so we'll skip this whole edge-based-edge which + // effectively removes it from the routing network. + if (skip_this_edge) { + penaltyblock++; + continue; + } + const auto turn_iter = turn_penalty_lookup.find( std::make_tuple(penaltyblock->from_id, penaltyblock->via_id, penaltyblock->to_id)); if (turn_iter != turn_penalty_lookup.end()) diff --git a/unit_tests/mocks/mock_datafacade.hpp b/unit_tests/mocks/mock_datafacade.hpp index f36276c6a..f18b9b539 100644 --- a/unit_tests/mocks/mock_datafacade.hpp +++ b/unit_tests/mocks/mock_datafacade.hpp @@ -62,8 +62,10 @@ class MockDataFacade final : public engine::datafacade::BaseDataFacade { } void GetUncompressedWeights(const EdgeID /* id */, - std::vector & /* result_weights */) const override + std::vector &result_weights) const override { + result_weights.resize(1); + result_weights[0] = 1; } void GetUncompressedDatasources(const EdgeID /*id*/, std::vector & /*data_sources*/) const override diff --git a/unit_tests/util/static_rtree.cpp b/unit_tests/util/static_rtree.cpp index 66b81c508..702449283 100644 --- a/unit_tests/util/static_rtree.cpp +++ b/unit_tests/util/static_rtree.cpp @@ -165,6 +165,9 @@ struct GraphFixture // to examine during tests. d.forward_segment_id = {pair.second, true}; d.reverse_segment_id = {pair.first, true}; + d.fwd_segment_position = 0; + d.forward_packed_geometry_id = 0; + d.reverse_packed_geometry_id = 0; edges.emplace_back(d); } }