feat: add way_ids to route annotations#7444
feat: add way_ids to route annotations#7444inigma07 wants to merge 7 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 |
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