fix(engine): a table cell with one end inside an open area came out short - #7689
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an open-area snapping edge case that caused asymmetric/short /table cells when one endpoint snapped inside a meshed open area. It extends the many-to-many implementations (CH + MLD) to carry the open-area “approach” component separately and applies reachability/loop logic to the graph-only portion of weight.
Changes:
- Add an
approachcomponent to many-to-many heap/bucket data and use it to prevent approach-walk costs from masking negative graph weights in CH/MLD many-to-many searches. - Extend open-area snapping to support a new
ApproachRole::Bothto offer both candidate “shapes” (departing/arriving) for table-like use cases. - Add a cucumber regression scenario for a foot table with one endpoint on a plaza and one off.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/engine/routing_algorithms/many_to_many_mld.cpp | Propagates and subtracts “approach” weight in acceptance logic to prevent degenerate table cells (MLD). |
| src/engine/routing_algorithms/many_to_many_ch.cpp | Applies loop/acceptance checks to graph-only weight by separating approach cost (CH). |
| src/engine/area_snapping.cpp | Adds handling for ApproachRole::Both when snapping inside open areas. |
| include/engine/search_engine_data.hpp | Extends heap data types with an approach field for many-to-many. |
| include/engine/routing_algorithms/routing_base.hpp | Seeds many-to-many heaps with phantom_node.approach_weight. |
| include/engine/routing_algorithms/many_to_many.hpp | Extends bucket entries with approach to carry through bidirectional many-to-many. |
| include/engine/area_snapping.hpp | Documents/introduces ApproachRole::Both for table-like cases. |
| features/foot/area_table_trip.feature | Adds regression scenario asserting symmetry and correctness for /route and /table. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * `Both` offers a candidate of each shape, for a service where one coordinate is both | ||
| * ends of different journeys. Every coordinate of a table is: getting one shape only | ||
| * makes half the matrix short by the stretch between the candidate and the node boundary, | ||
| * and an asymmetric matrix for a walking profile is the defect announcing itself. It is | ||
| * not the default because two candidates can then claim one way, which a route matched |
There was a problem hiding this comment.
Correct, and it means more than a missing call site. The plumbing to make the table ask for Both was never added, and the test passes anyway. So the loop-detection fix alone is sufficient and the shape machinery is dead code.
I have removed it. The enum is back to three values and the candidate loop is unchanged from master. Verified by running the new scenario on both algorithms after the removal: 4 of 4 pass on ch and on mld.
The PR description said two things had to change together. That was wrong and I have corrected it.
| /** | ||
| * @param weight the whole weight, walk included, which is what the cell reports | ||
| * @param approach how much of it is the walk into an open area | ||
| * | ||
| * The test is on `weight - approach`. A source and a target on one edge-based node with | ||
| * the target the earlier of the two need a loop to get from one to the other, and what | ||
| * says so is the *graph* part of the weight being negative. The walk into an area is | ||
| * charged at both ends and is not travel, so it can lift the sum back above zero and hide | ||
| * the case, and the cell then comes out short by the stretch between them. | ||
| */ |
There was a problem hiding this comment.
Converted to //.
| /** | ||
| * How much of the weight is the walk into an open area rather than travel through the | ||
| * graph (`PhantomNode::approach_weight`). Set by the seed and carried unchanged as | ||
| * the search relaxes, because a walk to where the graph starts is not made up by | ||
| * travelling along it. | ||
| * | ||
| * It has to be separable: a source and a target on one edge-based node with the | ||
| * target the earlier of the two need a loop, and what says so is the *graph* part of | ||
| * the weight going negative. A walk charged at both ends can lift the sum back above | ||
| * zero and hide it, and the cell then comes out short by the stretch in between. | ||
| */ |
There was a problem hiding this comment.
Converted to //.
| toHeapNode->data = { | ||
| heapNode.node, true, to_duration, to_distance, heapNode.data.approach}; | ||
| toHeapNode->weight = to_weight; | ||
| query_heap.DecreaseKey(*toHeapNode); | ||
| } |
There was a problem hiding this comment.
Fixed. The call at the end of relaxOutgoingEdges now passes heapNode.data.approach. Without it a border-edge relaxation started again from zero, so the walk looked like graph travel and the loop test could pass when it should not.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7689 +/- ##
==========================================
- Coverage 94.73% 94.72% -0.01%
==========================================
Files 519 519
Lines 41380 41435 +55
==========================================
+ Hits 39201 39250 +49
- Misses 2179 2185 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…hort The last of the sign problems from #7683, and the one that survived #7688 because it was never about the sign. A candidate departed from stands at an edge-based node's start. One arrived at stands at its end. Snapping into an open area picks the shape from the coordinate's position in the request, which works for a route. It does not work for a table, where every coordinate is a source and a destination at once. Used in the role it was not shaped for, the candidate sits before the source on the same node, and the cell comes out short by the stretch between them: 61.8 m where /route says 161.7 m. The matrix was asymmetric for a walking profile, which is the defect announcing itself. A target that sits before the source on one edge-based node is unreachable that way and needs a loop to get to. The weight going negative is what says so. But the walk into an open area is charged at both ends and is not travel, so it lifts the sum back above zero and the degenerate pairing is accepted. Both many-to-many implementations now carry how much of the weight is that walk, from the seed through the relaxation into the buckets, and test the graph part rather than the whole. The two implementations hide the same case in different places. CH hides it in addLoopWeight behind new_weight < 0. MLD hides it in the acceptance test new_weight >= 0 that otherwise keeps a target in the search to be found later by a real path. The new scenario fails on either algorithm if only one is fixed.
b158906 to
e1c0f66
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/engine/routing_algorithms/many_to_many_ch.cpp:151
- In the loop-adjustment branch, the result update uses per-field std::min on weight/duration/distance and then unconditionally overwrites middle_nodes_table. This can mix metrics from different candidate paths and can also record a middle node even when the (weight,duration) pair wasn’t improved, which may yield inconsistent duration/distance outputs relative to the selected weight-minimal path.
Update the cell using the same (weight,duration) tuple comparison used in the non-loop branch, and update all three metrics + middle node together only when the candidate is better.
current_weight = std::min(current_weight, new_weight);
current_duration = std::min(current_duration, new_duration);
current_distance = std::min(current_distance, new_distance);
middle_nodes_table[row_index * number_of_targets + column_index] = heapNode.node;
Closes the last of the sign problems in #7683, and the one that survived #7688 because it was never about the sign.
What was wrong
A candidate departed from stands at an edge-based node's start; one arrived at stands at its end. Snapping into an open area chose the shape from the coordinate's position in the request, which works for a route and not for a table, where every coordinate is a source and a destination at once.
Used in the role it was not shaped for, the candidate sits before the source on the same node, and the cell comes out short by the stretch between them. On a plaza with
minside it andeoutside:m -> ee -> m161.7 m is 50 m along the footway and then 111.8 m straight across the plaza, computed from the node coordinates rather than from OSRM. The asymmetry is the tell: a walking profile should never produce one.
The fix
A target that sits before the source on one edge-based node is unreachable that way and needs a loop to get to. The weight going negative is what says so. The walk into an open area is charged at both ends and is not travel, so it lifts the sum back above zero and the degenerate pairing is accepted:
Both many-to-many implementations now carry how much of the weight is that walk, from the seed through the relaxation into the buckets, and test the graph part rather than the whole.
The two hide the same case in different places. CH hides it in
addLoopWeightbehindnew_weight < 0. MLD hides it in the acceptance testnew_weight >= 0that otherwise keeps a target in the search to be found later by a real path. Fixing either alone leaves the scenario failing on the other algorithm.Verification
The new scenario asserts both cells of the matrix and both directions of
/route, so it fails on either algorithm if either half of the change is removed. Also all four unit suites and the full cucumber matrix,chandmldacrossmmap,directlyanddatastore, 1478 scenarios each, withENABLE_ASSERTIONS=1.Written with AI assistance: Claude Code, Claude Opus 5 🤖