feat: add way_ids to route annotations#7444
feat: add way_ids to route annotations#7444inigma07 wants to merge 8 commits intoProject-OSRM:masterfrom
Conversation
|
Any insights on memory overhead? |
There was a problem hiding this comment.
Pull request overview
This PR adds a new per-segment route annotation, way_ids, allowing clients to retrieve the OSM way ID for each traversed segment across route-like APIs (route/match/trip) and response formats (JSON + FlatBuffers), including HTTP and Node.js parameter parsing.
Changes:
- Add
RouteParameters::AnnotationsType::WayIdsand include it inAnnotationsType::All(annotations=truenow includesway_ids). - Thread OSM way IDs through extraction → facade access → geometry assembly → API serialization (JSON + FlatBuffers).
- Update Node.js bindings/docs and add integration/unit tests + changelog entry.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
include/engine/api/route_parameters.hpp |
Adds WayIds bitflag and includes it in All. |
include/server/api/route_parameters_grammar.hpp |
Enables parsing annotations=way_ids for HTTP API. |
include/nodejs/node_osrm_support.hpp |
Adds Node.js option parsing for annotations: ['way_ids']. |
src/nodejs/node_osrm.cpp |
Updates Node.js API docs/comments to mention way_ids. |
src/extractor/extractor_callbacks.cpp |
Captures the input OSM way ID and stores it in edge annotation data. |
include/extractor/node_based_edge.hpp |
Stores OSMWayID on NodeBasedEdgeAnnotation and factors it into comparison/combine logic. |
include/extractor/node_data_container.hpp |
Exposes GetWayID() from edge-based node annotation data. |
include/engine/datafacade/datafacade_base.hpp |
Adds BaseDataFacade::GetOSMWayID() API. |
include/engine/datafacade/contiguous_internalmem_datafacade.hpp |
Implements GetOSMWayID() for the primary in-memory facade. |
include/engine/guidance/leg_geometry.hpp |
Extends per-segment annotation struct to include way_id. |
include/engine/guidance/assemble_geometry.hpp |
Populates way_id for each assembled annotation segment. |
include/engine/api/route_api.hpp |
Serializes way_ids into JSON + FlatBuffers annotation outputs. |
include/engine/api/flatbuffers/route.fbs |
Adds way_ids: [ulong] to FlatBuffers Annotation. |
generated/include/engine/api/flatbuffers/route_generated.h |
Regenerates FlatBuffers header to include way_ids. |
docs/http.md |
Documents annotations=way_ids, updates examples/field descriptions. |
docs/nodejs/api.md |
Documents Node.js options.annotations support for way_ids. |
features/testbot/way_ids.feature |
Adds cucumber coverage asserting way_ids per segment. |
features/support/shared_steps.js |
Allows a:way_ids fields in step validation whitelist. |
features/step_definitions/matching.js |
Allows way_ids (and speed) in matching annotation whitelist. |
test/nodejs/route.js |
Extends Node.js route tests to request/assert annotation.way_ids. |
test/nodejs/match.js |
Extends Node.js match tests to request/assert annotation.way_ids. |
test/nodejs/trip.js |
Extends Node.js trip tests to request/assert annotation.way_ids. |
CHANGELOG.md |
Adds release note entry for annotations=way_ids. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| virtual OSMNodeID GetOSMNodeIDOfNode(const NodeID node_based_node_id) const = 0; | ||
|
|
||
| virtual OSMWayID GetOSMWayID(const NodeID edge_based_node_id) const = 0; |
There was a problem hiding this comment.
Adding the new pure-virtual BaseDataFacade::GetOSMWayID requires updating every BaseDataFacade implementation. In the current tree there is at least unit_tests/engine/offline_facade.cpp (ContiguousInternalMemoryDataFacadeoffline::Algorithm) that still derives from BaseDataFacade but does not implement GetOSMWayID, which will break compilation of unit tests. Either add the override there (and any other derived facades), or provide a non-pure default implementation if appropriate.
| virtual OSMWayID GetOSMWayID(const NodeID edge_based_node_id) const = 0; | |
| virtual OSMWayID GetOSMWayID(const NodeID /*edge_based_node_id*/) const | |
| { | |
| throw util::exception("GetOSMWayID is not implemented for this data facade"); | |
| } |
| OSMWayID way_id; // 64 8 | ||
| TravelMode travel_mode : 4; // 4 | ||
| bool is_left_hand_driving : 1; // 1 | ||
|
|
There was a problem hiding this comment.
The new OSMWayID way_id member is inserted in the middle of NodeBasedEdgeAnnotation, which changes the positional meaning of existing aggregate/brace initializers across the codebase (e.g. {..., TRAVEL_MODE_DRIVING, false} will now initialize way_id and travel_mode with the wrong values). To avoid silent mis-initialization, update all brace initializers to include way_id, or add an explicit constructor / named factory so call sites can’t accidentally shift fields when new members are added.
| NodeBasedEdgeAnnotation() = default; | |
| NodeBasedEdgeAnnotation(const StringViewID string_view_id, | |
| const LaneDescriptionID lane_description_id, | |
| const ClassData classes, | |
| const OSMWayID way_id, | |
| const TravelMode travel_mode, | |
| const bool is_left_hand_driving) | |
| : string_view_id(string_view_id), lane_description_id(lane_description_id), | |
| classes(classes), way_id(way_id), travel_mode(travel_mode), | |
| is_left_hand_driving(is_left_hand_driving) | |
| { | |
| } |
The way_ids change increases steady-state memory/storage overhead by about 4% to 5% on the Bengaluru (Indian city) dataset. Runtime memory (osrm-routed startup): about +4.9% Overall roughly a 5% memory overhead |
Thanks for these numbers. Could you verify that these numbers hold also on larger data sets, eg. all of India? Thanks |
Couldn't run for entire India dataset due to memory constraint of my machine, but ran for central zone (~330MB pbf size) with below findings - The way_ids change increases memory overhead by about 1% to 3% on the Central Zone dataset. |
|
Ok, I'll try with a larger data set some time next week. |
|
@inigma07 please have a look at the failing CI runs. It seems the tests are not compiling cleanly. |
|
Related PR #5968 |
Done |
|
@inigma07 Would you mind rebase this and resolving the merge conflict. We got rid of |
dbfa295 to
69c644e
Compare
|
Thanks. Have this on my plate for the weekend to help get it over the finish line. |
Thanks |
The iterator was missing ordering operators. C++20 ranges concepts checks require these for random access iterator. Fixes compilation of #7444.
|
The performance numbers look good and en par with master branch. Once #7538 is merged, we can update this PR and also merge to master. |
|
A +5% memory requirement increase could be significant for large areas and a real bottleneck for both extraction and runtime. If I recall correctly this feature has been dismissed in the past for that very reason. Since this can be both very useful in some situations and totally unneeded in others, did you consider trying to make it somehow optional? For what it's worth there is an existing mechanism used in similar situations ( |
Issue
No corresponding issue yet.
This PR adds support for
annotations=way_idsin route-like responses so clients can retrieve the OSM way ID for each traversed segment.Tasklist
Requirements / Relations
No additional requirements.
Not based on any other pull request.
Implementation notes:
RouteParameters::AnnotationsType::WayIdsway_idsinAnnotationsType::All, soannotations=truealso returns itOSMWayIDthrough extraction, edge-based node data, facade access, geometry assembly, and route annotation serializationway_idsin JSON and FlatBuffers responsesway_idsin HTTP and Node.js annotation parsing