Skip to content

Contractor rewrite #7442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MarcelloPerathoner wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Contractor rewrite #7442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a98d46c
Rebase origin/master
MarcelloPerathoner Apr 2, 2026
b55d41f
WIP
MarcelloPerathoner Apr 2, 2026
c768db2
WIP
MarcelloPerathoner Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions features/bicycle/distance.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ Feature: Bike - Use distance weight
When I route I should get
| from | to | route | weight | time | distance |
| a | b | abc,abc | 200 | 48s | 200m +-1 |
| b | c | abc,abc | 200 | 48s | 200m +-1 |
| a | c | abc,abc | 400 | 96s | 400m +-1 |
| b | a | abc,abc | 200 | 48s | 200m +-1 |
| c | b | abc,abc | 200 | 48s | 200m +-1 |
| c | a | abc,abc | 400 | 96s | 400m +-1 |
4 changes: 4 additions & 0 deletions features/foot/distance.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ Feature: Foot - Use distance weight
When I route I should get
| from | to | route | weight | time | distance |
| a | b | abc,abc | 200 | 144s | 200m +-1 |
| b | c | abc,abc | 200 | 144s | 200m +-1 |
| a | c | abc,abc | 400 | 288s | 400m +-1 |
| b | a | abc,abc | 200 | 144s | 200m +-1 |
| c | b | abc,abc | 200 | 144s | 200m +-1 |
| c | a | abc,abc | 400 | 288s | 400m +-1 |
10 changes: 6 additions & 4 deletions features/lib/osrm_loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ class OSRMBaseLoader {

this.child.stdout.on('data', (data) => {
if (data.includes('running and waiting for requests')) {
log('Routed running and waiting for requests');
resolve();
resolve();
}
log(`${data}`.trim());
});

this.child.on('exit', (code) => {
this.child.on('close', (code) => {
log(`osrm-routed completed with exit code ${code}`);
this.child = null;
});
Expand Down Expand Up @@ -209,7 +209,9 @@ export class OSRMDatastoreLoader extends OSRMBaseLoader {
// we MUST consume stdout and stderr or the osrm-routed process will block eventually
this.child.stderr.on('data', (data) => this.logSync(`osrm-routed stderr:\n${data}`));

this.child.on('exit', (code, signal) => {
this.child.on('close', (code, signal) => {
this.child.stdout.read();
this.child.stderr.read();
this.child = null;
if (signal != null) {
const msg = `osrm-routed aborted with signal ${signal}`;
Expand Down
1 change: 1 addition & 0 deletions features/support/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export default class Route {
if (bearings.length) {
params.bearings = bearings
.map((b) => {
if (b === '*') return '';
const bs = b.split(',');
if (bs.length === 2) return b;
else return (b += ',10');
Expand Down
2 changes: 1 addition & 1 deletion features/support/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function runBin(bin, args, options, log) {
log(`${bin} aborted with error ${err}`);
throw(err);
});
child.on('exit', (code, signal) => {
child.on('close', (code, signal) => {
if (signal != null) {
const msg = `${bin} aborted with signal ${child.signal}`;
log(msg);
Expand Down
45 changes: 45 additions & 0 deletions features/testbot/self_loop.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
@routing @testbot @self-loop
Feature: Self-loops
Background:
Given the profile "testbot"

Scenario: Waypoints on same edge with approaches
Given the node map
"""
4 3
a-------b
1 2
"""

And the ways
| nodes | highway |
| ab | residential |

When I route I should get
| waypoints | approaches | route |
| 1,2 | curb curb | ab,ab |
| 2,3 | curb curb | ab,ab,ab |
| 3,4 | curb curb | ab,ab |
| 4,1 | curb curb | ab,ab,ab |
| 4,3 | curb curb | ab,ab,ab,ab |
| 3,2 | curb curb | ab,ab,ab |
| 2,1 | curb curb | ab,ab,ab,ab |
| 1,4 | curb curb | ab,ab,ab |


Scenario: Waypoints on same edge with bearings
Given the node map
"""
4 3
a-------b
1 2
"""

And the ways
| nodes | highway |
| ab | residential |

When I route I should get
| waypoints | bearings | route |
| 2,1 | 90 90 | ab,ab,ab,ab |
| 4,3 | 270 270 | ab,ab,ab,ab |
5 changes: 3 additions & 2 deletions include/contractor/contractor_graph.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ struct ContractorEdgeData
EdgeDuration duration,
EdgeDistance distance,
unsigned original_edges,
unsigned id,
NodeID id,
bool shortcut,
bool forward,
bool backward)
Expand All @@ -30,7 +30,8 @@ struct ContractorEdgeData
EdgeWeight weight;
EdgeDuration duration;
EdgeDistance distance;
unsigned id;
NodeID id;
/** Recursive count of how many edges are replaced by this shortcut. */
unsigned originalEdges : 29;
bool shortcut : 1;
bool forward : 1;
Expand Down
17 changes: 3 additions & 14 deletions include/contractor/contractor_heap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,9 @@

namespace osrm::contractor
{
struct ContractorHeapData
{
ContractorHeapData() {}
ContractorHeapData(short hop_, bool target_) : hop(hop_), target(target_) {}

short hop = 0;
bool target = false;
};

using ContractorHeap = util::QueryHeap<NodeID,
NodeID,
EdgeWeight,
ContractorHeapData,
util::LinearHashStorage<NodeID, NodeID>>;
// Data: if true signals that node is a target
using ContractorHeap =
util::QueryHeap<NodeID, NodeID, EdgeWeight, bool, util::LinearHashStorage<NodeID, NodeID>>;

} // namespace osrm::contractor

Expand Down
3 changes: 2 additions & 1 deletion include/contractor/contractor_search.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ namespace osrm::contractor

void search(ContractorHeap &heap,
const ContractorGraph &graph,
const std::vector<bool> &contractable,
const NodeID start,
const std::vector<bool> &contractible,
const unsigned number_of_targets,
const int node_limit,
const EdgeWeight weight_limit,
Expand Down
21 changes: 6 additions & 15 deletions include/contractor/graph_contractor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,44 +4,35 @@
#include "contractor/contractor_graph.hpp"
#include "contractor/query_graph.hpp"

#include "util/filtered_graph.hpp"

#include <vector>

namespace osrm::contractor
{

using GraphAndFilter = std::tuple<QueryGraph, std::vector<std::vector<bool>>>;

GraphAndFilter contractFullGraph(ContractorGraph contractor_graph,
std::vector<EdgeWeight> node_weights);
GraphAndFilter contractFullGraph(ContractorGraph contractor_graph);

GraphAndFilter contractExcludableGraph(ContractorGraph contractor_graph_,
std::vector<EdgeWeight> node_weights,
const std::vector<std::vector<bool>> &filters);

std::vector<bool> contractGraph(ContractorGraph &graph,
std::vector<bool> node_is_uncontracted,
std::vector<bool> node_is_contractable,
std::vector<EdgeWeight> node_weights,
std::vector<bool> node_is_contractible,
double core_factor = 1.0);

Comment on lines 19 to 23
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this API, the boolean vector is still named node_is_contractable, but the rest of the PR consistently uses “contractible” (is_contractible, node_is_contractible_, etc.). Renaming the parameter improves consistency and avoids having both spellings in the public header.

Copilot uses AI. Check for mistakes.
// Overload for contracting all nodes
inline auto contractGraph(ContractorGraph &graph,
std::vector<EdgeWeight> node_weights,
double core_factor = 1.0)
inline auto contractGraph(ContractorGraph &graph, double core_factor = 1.0)
{
return contractGraph(graph, {}, {}, std::move(node_weights), core_factor);
return contractGraph(graph, {}, {}, core_factor);
}

// Overload no contracted nodes
inline auto contractGraph(ContractorGraph &graph,
std::vector<bool> node_is_contractable,
std::vector<EdgeWeight> node_weights,
std::vector<bool> node_is_contractible,
double core_factor = 1.0)
{
return contractGraph(
graph, {}, std::move(node_is_contractable), std::move(node_weights), core_factor);
return contractGraph(graph, {}, std::move(node_is_contractible), core_factor);
}

} // namespace osrm::contractor
Expand Down
10 changes: 5 additions & 5 deletions include/contractor/graph_contractor_adaptors.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ ContractorGraph toContractorGraph(NodeID number_of_nodes, const InputEdgeContain
input_edge.data.distance,
1,
input_edge.data.turn_id,
false,
input_edge.data.forward ? true : false,
input_edge.data.backward ? true : false);
false, // shortcut
input_edge.data.forward,
input_edge.data.backward);

edges.emplace_back(input_edge.target,
input_edge.source,
Expand All @@ -56,8 +56,8 @@ ContractorGraph toContractorGraph(NodeID number_of_nodes, const InputEdgeContain
1,
input_edge.data.turn_id,
false,
input_edge.data.backward ? true : false,
input_edge.data.forward ? true : false);
input_edge.data.backward,
input_edge.data.forward);
};
tbb::parallel_sort(edges.begin(), edges.end());

Expand Down
13 changes: 13 additions & 0 deletions include/engine/guidance/assemble_steps.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "util/coordinate_calculation.hpp"
#include "util/guidance/entry_class.hpp"
#include "util/guidance/turn_lanes.hpp"
#include "util/log.hpp"
#include "util/typedefs.hpp"

#include <cstddef>
Expand Down Expand Up @@ -153,6 +154,11 @@ inline std::vector<RouteStep> assembleSteps(const datafacade::BaseDataFacade &fa
{intersection},
is_left_hand_driving});

#ifndef NDEBUG
util::Log(logDEBUG) << "pushing RouteStep of duration "
<< from_alias<double>(segment_duration) / 10.;
Comment on lines 156 to +159
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This debug log runs in the main step-assembly loop. Constructing util::Log(logDEBUG) always takes the global log mutex (see src/util/log.cpp:70), so this adds overhead even when debug logging is disabled. Please guard this behind #ifdef ENABLE_DEBUG_LOGGING / #ifndef NDEBUG, or check the log policy before constructing the logger.

Suggested change
util::Log(logDEBUG) << "pushing RouteStep of duration "
<< from_alias<double>(segment_duration) / 10.;
#if defined(ENABLE_DEBUG_LOGGING) || !defined(NDEBUG)
util::Log(logDEBUG) << "pushing RouteStep of duration "
<< from_alias<double>(segment_duration) / 10.;
#endif

Copilot uses AI. Check for mistakes.
#endif

if (leg_data_index + 1 < leg_data.size())
{
step_name_id =
Expand Down Expand Up @@ -272,6 +278,13 @@ inline std::vector<RouteStep> assembleSteps(const datafacade::BaseDataFacade &fa
// u-------------v
// | |---------| source_weight
// | |---| target_weight

#ifndef NDEBUG
if (source_weight > target_weight)
util::Log(logDEBUG) << "error: source_weight = " << source_weight
<< " target_weight = " << target_weight;
#endif

BOOST_ASSERT(target_weight >= source_weight);
const EdgeWeight weight = target_weight - source_weight;

Expand Down
59 changes: 33 additions & 26 deletions include/engine/routing_algorithms/routing_base_ch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "engine/routing_algorithms/routing_base.hpp"
#include "engine/search_engine_data.hpp"

#include "util/log.hpp"
#include "util/typedefs.hpp"

#include <boost/assert.hpp>
Expand Down Expand Up @@ -120,41 +121,36 @@ void routingStep(const DataFacade<Algorithm> &facade,
if (reverseHeapNode)
{
const EdgeWeight new_weight = reverseHeapNode->weight + heapNode.weight;
if (new_weight < upper_bound)

if (new_weight >= EdgeWeight{0} && new_weight <= upper_bound &&
!shouldForceStep(force_step_nodes, heapNode, *reverseHeapNode))
{
middle_node_id = heapNode.node;
upper_bound = new_weight;
}
else
{
if (shouldForceStep(force_step_nodes, heapNode, *reverseHeapNode) ||
// in this case we are looking at a bi-directional way where the source
// and target phantom are on the same edge based node
new_weight < EdgeWeight{0})
// The two identical nodes did not match after all.
// Before forcing step, check whether there is a loop present at the node.
// We may find a valid weight path by following the loop.
for (const auto edge : facade.GetAdjacentEdgeRange(heapNode.node))
{
// Before forcing step, check whether there is a loop present at the node.
// We may find a valid weight path by following the loop.
for (const auto edge : facade.GetAdjacentEdgeRange(heapNode.node))
const auto &data = facade.GetEdgeData(edge);
if (DIRECTION == FORWARD_DIRECTION ? data.forward : data.backward)
{
const auto &data = facade.GetEdgeData(edge);
if (DIRECTION == FORWARD_DIRECTION ? data.forward : data.backward)
const NodeID to = facade.GetTarget(edge);
if (to == heapNode.node)
{
const NodeID to = facade.GetTarget(edge);
if (to == heapNode.node)
const EdgeWeight edge_weight = data.weight;
const EdgeWeight loop_weight = new_weight + edge_weight;
if (loop_weight >= EdgeWeight{0} && loop_weight < upper_bound)
{
const EdgeWeight edge_weight = data.weight;
const EdgeWeight loop_weight = new_weight + edge_weight;
if (loop_weight >= EdgeWeight{0} && loop_weight < upper_bound)
{
middle_node_id = heapNode.node;
upper_bound = loop_weight;
}
middle_node_id = heapNode.node;
upper_bound = loop_weight;
}
}
}
}
else
{
BOOST_ASSERT(new_weight >= EdgeWeight{0});

middle_node_id = heapNode.node;
upper_bound = new_weight;
}
}
}

Expand Down Expand Up @@ -389,6 +385,17 @@ void unpackPath(const FacadeT &facade,
}

annotatePath(facade, route_endpoints, unpacked_nodes, unpacked_edges, unpacked_path);

#ifndef NDEBUG
{
auto log = util::Log(logDEBUG);
log << "unpacked path =";
for (const auto &p : unpacked_path)
{
log << " " << p.from_edge_based_node;
}
}
#endif
}

/**
Expand Down
9 changes: 9 additions & 0 deletions include/util/d_ary_heap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ template <typename HeapData, int Arity, typename Comparator = std::less<HeapData
heapifyUp(handle, std::forward<ReorderHandler>(reorderHandler));
}

template <typename ReorderHandler>
void increase(HeapHandle handle, HeapData &&data, ReorderHandler &&reorderHandler)
{
BOOST_ASSERT(handle < heap.size());

heap[handle] = std::forward<HeapData>(data);
heapifyDown(handle, std::forward<ReorderHandler>(reorderHandler));
}

void clear() { heap.clear(); }

template <typename ReorderHandler> void pop(ReorderHandler &&reorderHandler)
Expand Down
8 changes: 6 additions & 2 deletions include/util/dynamic_graph.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,8 +415,11 @@ template <typename EdgeDataT> class DynamicGraph

void Renumber(const std::vector<NodeID> &old_to_new_node)
{
bool renumber = old_to_new_node.size() != 0;

// permutate everything but the sentinel
util::inplacePermutation(node_array.begin(), node_array.end(), old_to_new_node);
if (renumber)
util::inplacePermutation(node_array.begin(), node_array.end(), old_to_new_node);

// Build up edge permutation
if (edge_list.size() >= std::numeric_limits<EdgeID>::max())
Expand All @@ -432,7 +435,8 @@ template <typename EdgeDataT> class DynamicGraph
// move all filled edges
for (auto edge : GetAdjacentEdgeRange(node))
{
edge_list[edge].target = old_to_new_node[edge_list[edge].target];
if (renumber)
edge_list[edge].target = old_to_new_node[edge_list[edge].target];
BOOST_ASSERT(edge_list[edge].target != SPECIAL_NODEID);
old_to_new_edge[edge] = new_edge_index++;
}
Expand Down
Loading
Loading