Skip to content

feat(engine): let a path point say where it is - #7694

Merged
DennisOSRM merged 1 commit into
masterfrom
path-data-coordinate
Aug 16, 2026
Merged

feat(engine): let a path point say where it is#7694
DennisOSRM merged 1 commit into
masterfrom
path-data-coordinate

Conversation

@DennisOSRM

Copy link
Copy Markdown
Collaborator

Issue

No issue. This is preparatory work for smoothing paths across open areas, split out
because it touches a struct every route in OSRM goes through and deserves its own review.

The problem

Every point of a route currently lands on a node of the graph, and assembly finds its
position by looking the node up:

auto coordinate = facade.GetCoordinateOfNode(path_point.turn_via_node);

area_route.cpp already leans on this: a bend across a plaza is an area vertex, every
vertex of a meshed area carries a way, so it has a node id and the lookup works.

A point that is computed rather than traversed has nowhere to be looked up. Smoothing a
path across an open area produces exactly such points. They are not vertices of anything,
and there is no node id that names them.

What this adds

PathData::coordinate, a util::Coordinate that is invalid by default, and
coordinateOf(facade, point) which returns it when it is valid and falls back to the node
lookup when it is not.

All four resolution sites go through it. The leg geometry, the reported distance in
assembleLeg, the intersection location in assembleSteps, and getPathDistance. That is
the point of putting the field here rather than post-processing the geometry: a line that is
drawn one way and measured another is worse than either, and there is already one open
defect about leg annotations disagreeing with the leg (#7683) without adding a second.

The node id is left alone. A producer that sets a coordinate decides separately which
node the point is attributed to. That is what annotations=nodes reports, and it is not a
question this field can answer. Making that explicit here rather than guessing seems better
than the alternatives, but it is the part I would most like a second opinion on.

Cost

PathData grows from 36 to 44 bytes. It is built per query and not stored, so this is
peak memory during unpacking rather than anything persistent.

Nothing sets it

No behaviour changes. All fourteen unit suites pass, and the full cucumber suite comes out
at 1478 scenarios, 1463 passed, 15 skipped, 0 failed, which is the baseline exactly. That
is the whole claim: the field is inert until something writes to it.

Tests

unit_tests/engine/path_data_coordinate.cpp, four cases:

  • a point with no coordinate is still placed by its node,
  • a coordinate overrides the node lookup,
  • the assembled geometry follows the coordinate, and the node ids do not move,
  • the reported distance follows it too, out to the moved point and back.

The last two are the ones that matter, and they were checked by reverting the two call
sites: all four assertions fail without the change.

Was this change primarily generated using an AI tool? Yes.

🤖 Claude Code, Claude Opus 5

Tasklist

  • self-review code for correctness and following the coding guidelines
  • add tests
  • update relevant wiki pages
  • review
  • adjust for comments

Requirements / Relations

Independent of #7693. Both are prerequisites for smoothing plaza paths, and neither changes
any behaviour on its own.

Copilot AI lite review requested due to automatic review settings August 16, 2026 11:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an optional per-PathData coordinate override (invalid by default) and routes all path-point coordinate resolution through a single helper, enabling future “computed” (non-node) path points to be rendered and measured consistently without changing current behavior.

Changes:

  • Extend engine::PathData with util::Coordinate coordinate{} (invalid by default) and add coordinateOf(facade, point) fallback helper.
  • Switch geometry assembly, leg distance computation, intersection step location, and getPathDistance to use coordinateOf.
  • Add unit tests covering fallback behavior, override behavior, geometry placement, and distance impact.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
unit_tests/engine/path_data_coordinate.cpp Adds unit tests validating fallback vs. override behavior and downstream geometry/distance consistency.
include/engine/internal_route_result.hpp Extends PathData and introduces coordinateOf helper for unified coordinate resolution.
include/engine/guidance/assemble_geometry.hpp Uses coordinateOf so drawn geometry follows overridden coordinates.
include/engine/guidance/assemble_leg.hpp Uses coordinateOf so reported leg distance matches geometry when coordinates are overridden.
include/engine/guidance/assemble_steps.hpp Uses coordinateOf so intersection locations match overridden coordinates.
include/engine/routing_algorithms/routing_base.hpp Uses coordinateOf so path distance computation matches overridden coordinates.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1 to +10
#include "engine/internal_route_result.hpp"

#include "engine/guidance/assemble_geometry.hpp"
#include "engine/guidance/assemble_leg.hpp"

#include "mocks/mock_datafacade.hpp"

#include <boost/test/unit_test.hpp>

#include <vector>
@codecov

codecov Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.72%. Comparing base (9229f17) to head (5e3113f).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7694      +/-   ##
==========================================
- Coverage   94.75%   94.72%   -0.04%     
==========================================
  Files         519      521       +2     
  Lines       41582    41609      +27     
==========================================
+ Hits        39402    39414      +12     
- Misses       2180     2195      +15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Adds PathData::coordinate, invalid by default, and coordinateOf() to resolve it
against the node lookup.

Every point of a route currently lands on a node of the graph, and assembly
finds its position by looking the node up. A point that is computed rather than
traversed has nowhere to be looked up. Smoothing a path across an open area
produces exactly such points: they are not vertices of anything, and there is no
node id that names them.

Four places resolve a path point to a position: the leg geometry, the reported
distance, the intersection location in the steps, and getPathDistance. All four
now go through coordinateOf(), because a line that is drawn one way and measured
another is worse than either.

The node id is left alone. A producer that sets a coordinate decides separately
which node the point is attributed to, since that is what annotations report and
it is not a question this field can answer.

Nothing sets the field, so no behaviour changes: the existing unit suites and the
full cucumber suite are unaffected. PathData grows from 36 to 44 bytes, which is
per query and not stored.
@DennisOSRM
DennisOSRM force-pushed the path-data-coordinate branch from e096cc5 to 5e3113f Compare August 16, 2026 17:24
@DennisOSRM
DennisOSRM merged commit 6441421 into master Aug 16, 2026
22 of 23 checks passed
@DennisOSRM
DennisOSRM deleted the path-data-coordinate branch August 16, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants