Fix table result when source and destination on same one-way segment

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.
Project-OSRM · Sep 12, 2020 · f3bfb18 · f3bfb18
1 parent 523d9e9
commit f3bfb18
Show file tree

Hide file tree

Showing 3 changed files with 152 additions and 72 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,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)
     - Misc:
       - CHANGED: Reduce memory usage for raster source handling. [#5572](https://github.com/Project-OSRM/osrm-backend/pull/5572)
       - CHANGED: Add cmake option `ENABLE_DEBUG_LOGGING` to control whether output debug logging. [#3427](https://github.com/Project-OSRM/osrm-backend/issues/3427)

diff --git a/features/testbot/oneway_phantom.feature 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
@@ -36,6 +36,59 @@ inline LevelID getNodeQueryLevel(const MultiLevelPartition &partition,
     return node_level;
 }
 
+template <bool DIRECTION>
+void relaxBorderEdges(const DataFacade<mld::Algorithm> &facade,
+                      const NodeID node,
+                      const EdgeWeight weight,
+                      const EdgeDuration duration,
+                      const EdgeDistance distance,
+                      typename SearchEngineData<mld::Algorithm>::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 <bool DIRECTION, typename... Args>
 void relaxOutgoingEdges(const DataFacade<mld::Algorithm> &facade,
                         const NodeID node,
@@ -140,48 +193,7 @@ void relaxOutgoingEdges(const DataFacade<mld::Algorithm> &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<DIRECTION>(facade, node, weight, duration, distance, query_heap, level);
 }
 
 //
@@ -297,37 +309,19 @@ oneToManySearch(SearchEngineData<Algorithm> &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) > 0)
         {
-            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<DIRECTION>(
+                facade, node, initial_weight, initial_duration, initial_distance, query_heap, 0);
+        }
+        else
+        {
+            query_heap.Insert(node, initial_weight, {node, initial_duration, initial_distance});
         }
     };