bug-fix: invalid contract node

[fenwuyaoji]

Issue

To fix issue #7054

Currently, in OSRM CH algo, the graph contract node and search routes update the weight of each edge. OSRM uses filters to exclude some specific roads with tags like toll, ferry, etc. In the function contractExcludableGraph, if filters are more than 1, OSRM will first contract a non-core graph and contract a corresponding graph of each filter. However, the weight update process uses all edges to search routes and doesn't consider that we can not use edges that should be excluded. This will lead to no route found issue when using the exlclude=toll, addressed in #7054.
In that case, node 3 has 2 alternative routes to node 7, while 3->1->0->6->7 is faster than 3->2->0. When OSRM wants to contract node 2, edge (3,2), (7,2) are deleted and no shortcut is inserted, because 3->1->0->6->7 is the actual shortcut (shorter than 3->2->0). However, updating the route weight doesn’t consider whether node 0 or 1 or others are accessible in this filtered graph. So I try to pass the array contractable into function relaxNode(), if a finding node is inaccessible, we shouldn’t insert this node into the searching heap or update the weight.
Notice: this change will leave more edges in graph and enlarge the memory usage, but I think it's worth it.

I'm not sure this is the best way to fix it. Waiting for your feedback, and appreciate it in advance.

Please read our documentation on release and version management.
If your PR is still work in progress please attach the relevant label.

Tasklist

• CHANGELOG.md entry (How to write a changelog entry)
• update relevant Wiki pages
• add tests (see testing documentation)
• review
• adjust for comments
• cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[DennisOSRM]

requires a formatting change for CI to proceed:

src/contractor/contractor_search.cpp src/contractor/graph_contractor.cpp
diff --git a/src/contractor/contractor_search.cpp b/src/contractor/contractor_search.cpp
index 8226e51..620cd2a 100644
--- a/src/contractor/contractor_search.cpp
+++ b/src/contractor/contractor_search.cpp
@@ -35,7 +35,8 @@ void relaxNode(ContractorHeap &heap,
         // New Node discovered -> Add to Heap + Node Info Storage
         if (!toHeapNode)
         {
-            if (!contractable[to]) {
+            if (!contractable[to])
+            {
                 continue;
             }
             heap.Insert(to, to_weight, ContractorHeapData{current_hop, false});
diff --git a/src/contractor/graph_contractor.cpp b/src/contractor/graph_contractor.cpp
index c449590..6a32182 100644
--- a/src/contractor/graph_contractor.cpp
+++ b/src/contractor/graph_contractor.cpp
@@ -246,12 +246,24 @@ void ContractNode(ContractorThreadData *data,
         if (RUNSIMULATION)
         {
             const int constexpr SIMULATION_SEARCH_SPACE_SIZE = 1000;
-            search(heap, graph, contractable, number_of_targets, SIMULATION_SEARCH_SPACE_SIZE, max_weight, node);
+            search(heap,
+                   graph,
+                   contractable,
+                   number_of_targets,
+                   SIMULATION_SEARCH_SPACE_SIZE,
+                   max_weight,
+                   node);
         }
         else
         {
             const int constexpr FULL_SEARCH_SPACE_SIZE = 2000;
-            search(heap, graph, contractable, number_of_targets, FULL_SEARCH_SPACE_SIZE, max_weight, node);
+            search(heap,
+                   graph,
+                   contractable,
+                   number_of_targets,
+                   FULL_SEARCH_SPACE_SIZE,
+                   max_weight,
+                   node);
         }
         for (auto out_edge : graph.GetAdjacentEdgeRange(node))
         {
@@ -490,7 +502,8 @@ bool UpdateNodeNeighbours(ContractorNodeData &node_data,
         if (node_data.contractable[u])
         {
             node_data.priorities[u] = EvaluateNodePriority(
-                SimulateNodeContraction(data, graph, u, node_data.weights, node_data.contractable), node_data.depths[u]);
+                SimulateNodeContraction(data, graph, u, node_data.weights, node_data.contractable),
+                node_data.depths[u]);
         }
     }
     return true;
@@ -621,19 +634,21 @@ std::vector<bool> contractGraph(ContractorGraph &graph,
     {
         util::UnbufferedLog log;
         log << "initializing node priorities...";
-        tbb::parallel_for(tbb::blocked_range<std::size_t>(0, remaining_nodes.size(), PQGrainSize),
-                          [&](const auto &range)
-                          {
-                              ContractorThreadData *data = thread_data_list.GetThreadData();
-                              for (auto x = range.begin(), end = range.end(); x != end; ++x)
-                              {
-                                  auto node = remaining_nodes[x].id;
-                                  BOOST_ASSERT(node_data.contractable[node]);
-                                  node_data.priorities[node] = EvaluateNodePriority(
-                                      SimulateNodeContraction(data, graph, node, node_data.weights, node_data.contractable),
-                                      node_data.depths[node]);
-                              }
-                          });
+        tbb::parallel_for(
+            tbb::blocked_range<std::size_t>(0, remaining_nodes.size(), PQGrainSize),
+            [&](const auto &range)
+            {
+                ContractorThreadData *data = thread_data_list.GetThreadData();
+                for (auto x = range.begin(), end = range.end(); x != end; ++x)
+                {
+                    auto node = remaining_nodes[x].id;
+                    BOOST_ASSERT(node_data.contractable[node]);
+                    node_data.priorities[node] = EvaluateNodePriority(
+                        SimulateNodeContraction(
+                            data, graph, node, node_data.weights, node_data.contractable),
+                        node_data.depths[node]);
+                }
+            });
         log << " ok.";
     }

[fenwuyaoji]

@DennisOSRM Thanks for your kindly reply, I just fixed the format issue, plz have a look again

[DennisOSRM]

Still failing. Please have a look at the failing test output.

[fenwuyaoji]

@DennisOSRM . I've run on ubuntu with docker and passed the pre-push hook. plz check again. sry to bother you again

[fenwuyaoji]

@DennisOSRM thank you. all checks passed, could you review this PR? is it ok to merge?

[DennisOSRM]

I plan to review over the weekend.