From e033e0b5538e0a07abdcd71acbc329afeea1062b Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Wed, 7 Oct 2020 22:58:13 +0100 Subject: [PATCH] Fix table result when source and destination on same one-way segment (#5828) Fixes #5788 Table queries where source and destination are phantom nodes on the same one-way segment can fail to find valid routes. This is due to a bug in the MLD table generation for the special case where the query can be simplified to a one-to-many search. If the destination is before the source on the one-way segment, it will fail to find a route. We fix this case by not marking the node as visited at the start, so that valid paths to this node can be found later in the search. We also remove redundant initialization for the source node as the same actions are performed by a search step. --- CHANGELOG.md | 1 + features/testbot/oneway_phantom.feature | 85 +++++++++++ .../routing_algorithms/many_to_many_mld.cpp | 138 +++++++++--------- 3 files changed, 152 insertions(+), 72 deletions(-) create mode 100644 features/testbot/oneway_phantom.feature diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a370ff2514..40cc724b796 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - CHANGED: default car weight was reduced to 2000 kg. [#5371](https://github.com/Project-OSRM/osrm-backend/pull/5371) - CHANGED: default car height was reduced to 2 meters. [#5389](https://github.com/Project-OSRM/osrm-backend/pull/5389) - FIXED: treat `bicycle=use_sidepath` as no access on the tagged way. [#5622](https://github.com/Project-OSRM/osrm-backend/pull/5622) + - FIXED: fix table result when source and destination on same one-way segment. [#5828](https://github.com/Project-OSRM/osrm-backend/pull/5828) - FIXED: fix occasional segfault when swapping data with osrm-datastore and using `exclude=` [#5844](https://github.com/Project-OSRM/osrm-backend/pull/5844) - Misc: - CHANGED: Reduce memory usage for raster source handling. [#5572](https://github.com/Project-OSRM/osrm-backend/pull/5572) diff --git a/features/testbot/oneway_phantom.feature b/features/testbot/oneway_phantom.feature new file mode 100644 index 00000000000..9d728a507a1 --- /dev/null +++ b/features/testbot/oneway_phantom.feature @@ -0,0 +1,85 @@ +@routing @testbot @oneway +Feature: Handle multiple phantom nodes in one-way segment + +# Check we handle routes where source and destination are +# phantom nodes on the same one-way segment. +# See: https://github.com/Project-OSRM/osrm-backend/issues/5788 + + Background: + Given the profile "testbot" + + Scenario: One-way segment with adjacent phantom nodes + Given the node map + """ + d c + + a12b + """ + + And the ways + | nodes | oneway | + | ab | yes | + | bc | no | + | cd | no | + | da | no | + + When I route I should get + | from | to | route | time | distance | + | 1 | 2 | ab,ab | 5s +-0.1 | 50m ~1% | + | 1 | c | ab,bc,bc | 30s +-0.1 | 300m ~1% | + | 2 | 1 | ab,bc,cd,da,ab | 65s +-0.1 | 650m ~1% | + | 2 | c | ab,bc,bc | 25s +-0.1 | 250m ~1% | + | c | 1 | cd,da,ab | 40s +-0.1 | 400m ~1% | + | c | 2 | cd,da,ab | 45s +-0.1 | 450m ~1% | + + When I request a travel time matrix I should get + | | 1 | 2 | c | + | 1 | 0 | 5 +-0.1 | 30 +-0.1 | + | 2 | 65 +-0.1 | 0 | 25 +-0.1 | + | c | 40 +-0.1 | 45 +-0.1 | 0 | + + When I request a travel time matrix I should get + | | 1 | 2 | c | + | 1 | 0 | 5 +-0.1 | 30 +-0.1 | + + When I request a travel time matrix I should get + | | 1 | 2 | c | + | 2 | 65 +-0.1 | 0 | 25 +-0.1 | + + When I request a travel time matrix I should get + | | 1 | + | 1 | 0 | + | 2 | 65 +-0.1 | + | c | 40 +-0.1 | + + When I request a travel time matrix I should get + | | 2 | + | 1 | 5 +-0.1 | + | 2 | 0 | + | c | 45 +-0.1 | + + When I request a travel distance matrix I should get + | | 1 | 2 | c | + | 1 | 0 | 50 ~1% | 300 ~1% | + | 2 | 650 ~1% | 0 | 250 ~1% | + | c | 400 ~1% | 450 ~1% | 0 | + + When I request a travel distance matrix I should get + | | 1 | 2 | c | + | 1 | 0 | 50 ~1% | 300 ~1% | + + When I request a travel distance matrix I should get + | | 1 | 2 | c | + | 2 | 650 ~1% | 0 | 250 ~1% | + + When I request a travel distance matrix I should get + | | 1 | + | 1 | 0 | + | 2 | 650 ~1% | + | c | 400 ~1% | + + When I request a travel distance matrix I should get + | | 2 | + | 1 | 50 ~1% | + | 2 | 0 | + | c | 450 ~1% | diff --git a/src/engine/routing_algorithms/many_to_many_mld.cpp b/src/engine/routing_algorithms/many_to_many_mld.cpp index 55544c4bbf7..bd41f7aba80 100644 --- a/src/engine/routing_algorithms/many_to_many_mld.cpp +++ b/src/engine/routing_algorithms/many_to_many_mld.cpp @@ -36,6 +36,59 @@ inline LevelID getNodeQueryLevel(const MultiLevelPartition &partition, return node_level; } +template +void relaxBorderEdges(const DataFacade &facade, + const NodeID node, + const EdgeWeight weight, + const EdgeDuration duration, + const EdgeDistance distance, + typename SearchEngineData::ManyToManyQueryHeap &query_heap, + LevelID level) +{ + for (const auto edge : facade.GetBorderEdgeRange(level, node)) + { + const auto &data = facade.GetEdgeData(edge); + if ((DIRECTION == FORWARD_DIRECTION) ? facade.IsForwardEdge(edge) + : facade.IsBackwardEdge(edge)) + { + const NodeID to = facade.GetTarget(edge); + if (facade.ExcludeNode(to)) + { + continue; + } + + const auto turn_id = data.turn_id; + const auto node_id = DIRECTION == FORWARD_DIRECTION ? node : facade.GetTarget(edge); + const auto node_weight = facade.GetNodeWeight(node_id); + const auto node_duration = facade.GetNodeDuration(node_id); + const auto node_distance = facade.GetNodeDistance(node_id); + const auto turn_weight = node_weight + facade.GetWeightPenaltyForEdgeID(turn_id); + const auto turn_duration = node_duration + facade.GetDurationPenaltyForEdgeID(turn_id); + + BOOST_ASSERT_MSG(node_weight + turn_weight > 0, "edge weight is invalid"); + const auto to_weight = weight + turn_weight; + const auto to_duration = duration + turn_duration; + const auto to_distance = distance + node_distance; + + // New Node discovered -> Add to Heap + Node Info Storage + if (!query_heap.WasInserted(to)) + { + query_heap.Insert(to, to_weight, {node, false, to_duration, to_distance}); + } + // Found a shorter Path -> Update weight and set new parent + else if (std::tie(to_weight, to_duration, to_distance, node) < + std::tie(query_heap.GetKey(to), + query_heap.GetData(to).duration, + query_heap.GetData(to).distance, + query_heap.GetData(to).parent)) + { + query_heap.GetData(to) = {node, false, to_duration, to_distance}; + query_heap.DecreaseKey(to, to_weight); + } + } + } +} + template void relaxOutgoingEdges(const DataFacade &facade, const NodeID node, @@ -140,48 +193,7 @@ void relaxOutgoingEdges(const DataFacade &facade, } } - for (const auto edge : facade.GetBorderEdgeRange(level, node)) - { - const auto &data = facade.GetEdgeData(edge); - if ((DIRECTION == FORWARD_DIRECTION) ? facade.IsForwardEdge(edge) - : facade.IsBackwardEdge(edge)) - { - const NodeID to = facade.GetTarget(edge); - if (facade.ExcludeNode(to)) - { - continue; - } - - const auto turn_id = data.turn_id; - const auto node_id = DIRECTION == FORWARD_DIRECTION ? node : facade.GetTarget(edge); - const auto node_weight = facade.GetNodeWeight(node_id); - const auto node_duration = facade.GetNodeDuration(node_id); - const auto node_distance = facade.GetNodeDistance(node_id); - const auto turn_weight = node_weight + facade.GetWeightPenaltyForEdgeID(turn_id); - const auto turn_duration = node_duration + facade.GetDurationPenaltyForEdgeID(turn_id); - - BOOST_ASSERT_MSG(node_weight + turn_weight > 0, "edge weight is invalid"); - const auto to_weight = weight + turn_weight; - const auto to_duration = duration + turn_duration; - const auto to_distance = distance + node_distance; - - // New Node discovered -> Add to Heap + Node Info Storage - if (!query_heap.WasInserted(to)) - { - query_heap.Insert(to, to_weight, {node, false, to_duration, to_distance}); - } - // Found a shorter Path -> Update weight and set new parent - else if (std::tie(to_weight, to_duration, to_distance, node) < - std::tie(query_heap.GetKey(to), - query_heap.GetData(to).duration, - query_heap.GetData(to).distance, - query_heap.GetData(to).parent)) - { - query_heap.GetData(to) = {node, false, to_duration, to_distance}; - query_heap.DecreaseKey(to, to_weight); - } - } - } + relaxBorderEdges(facade, node, weight, duration, distance, query_heap, level); } // @@ -297,37 +309,19 @@ oneToManySearch(SearchEngineData &engine_working_data, EdgeWeight initial_weight, EdgeDuration initial_duration, EdgeDistance initial_distance) { - - // Update single node paths - update_values(node, initial_weight, initial_duration, initial_distance); - - query_heap.Insert(node, initial_weight, {node, initial_duration, initial_distance}); - - // Place adjacent nodes into heap - for (auto edge : facade.GetAdjacentEdgeRange(node)) + if (target_nodes_index.count(node)) { - const auto &data = facade.GetEdgeData(edge); - const auto to = facade.GetTarget(edge); - - if (facade.ExcludeNode(to)) - { - continue; - } - - if ((DIRECTION == FORWARD_DIRECTION ? facade.IsForwardEdge(edge) - : facade.IsBackwardEdge(edge)) && - !query_heap.WasInserted(to)) - { - const auto turn_id = data.turn_id; - const auto node_id = DIRECTION == FORWARD_DIRECTION ? node : to; - const auto edge_weight = initial_weight + facade.GetNodeWeight(node_id) + - facade.GetWeightPenaltyForEdgeID(turn_id); - const auto edge_duration = initial_duration + facade.GetNodeDuration(node_id) + - facade.GetDurationPenaltyForEdgeID(turn_id); - const auto edge_distance = initial_distance + facade.GetNodeDistance(node_id); - - query_heap.Insert(to, edge_weight, {node, edge_duration, edge_distance}); - } + // Source and target on the same edge node. If target is not reachable directly via + // the node (e.g destination is before source on oneway segment) we want to allow + // node to be visited later in the search along a reachable path. + // Therefore, we manually run first step of search without marking node as visited. + update_values(node, initial_weight, initial_duration, initial_distance); + relaxBorderEdges( + facade, node, initial_weight, initial_duration, initial_distance, query_heap, 0); + } + else + { + query_heap.Insert(node, initial_weight, {node, initial_duration, initial_distance}); } };