Add osm nodes to nearest response
#4436
Conversation
MoKob
changed the title
nearest response [wip]nearest response
include/engine/api/nearest_api.hpp
Outdated
| nodes.values.push_back(from_node); | ||
| nodes.values.push_back(to_node); | ||
| } | ||
| waypoint.values["nodes"] = std::move(nodes); |
There was a problem hiding this comment.
Could you assert we never return the special osm id sentinel set above here please
There was a problem hiding this comment.
The current implementation may return SPECIAL_OSM_NODEID . This happens while doing a scan of Monaco area.
Would you suggest using an another special osm id value? or not returning such value in the response?
There was a problem hiding this comment.
Yeah we should not expose the osrm internal special node id to the outside. We should probably just ail with an error code in this edge case.
There was a problem hiding this comment.
Hrm, would be interesting to see how this could happen. From my understanding there is a guarantee that at least one of forward and backward is enabled. The code in nearest.cpp should already bail out with NoSegement if we didn't found candidates.
There was a problem hiding this comment.
Most of edge case are one way case. In that case reverse_segment_id.enabled is false.
I wonder if it would not be better to use forward_segment_id only and get the previous node like this
auto osm_node_id = facade.GetOSMNodeIDOfNode(geometry[phantom_node.fwd_segment_position - 1]);
As @danpat commented in #2581 , the edge case would be when fwd_segment_position is zero. I guess this would require to find segments linked to forward_segment_id to access to their geometry.
Do you think this would be correct?
There was a problem hiding this comment.
Any feedback on my comment?
There was a problem hiding this comment.
Thanks for pushing this forward! We have some integration tests in features/nearest/pick.feature that could use a test case. You might want to extend features/step_definitions/nearest.js to be able to check the nodes in the response.
This change also needs a CHANGELOG.md entry for adding the nodes property. Thanks!
include/engine/api/nearest_api.hpp
Outdated
| std::uint64_t to_node = static_cast<std::uint64_t>(SPECIAL_OSM_NODEID); | ||
|
|
||
| auto segment_id = phantom_node.forward_segment_id.id; | ||
| if (segment_id != SPECIAL_SEGMENTID) |
There was a problem hiding this comment.
This checks should be replaced with phantom_node.forward_segment_id.enabled. There are cases where the ID is valid but the direction should not be used.
include/engine/api/nearest_api.hpp
Outdated
| nodes.values.push_back(from_node); | ||
| nodes.values.push_back(to_node); | ||
| } | ||
| waypoint.values["nodes"] = std::move(nodes); |
There was a problem hiding this comment.
Hrm, would be interesting to see how this could happen. From my understanding there is a guarantee that at least one of forward and backward is enabled. The code in nearest.cpp should already bail out with NoSegement if we didn't found candidates.
|
@DavidAudrain sorry I completely forgot about this PR. Anyway I've looked at your changes again and this iteration looks great. However our CI is complaining about code formatting. Could you rerun |
|
Thank you @TheMarex for your feedback, I ran |
|
Great, going to include this in |
|
Ah just noticed, this should probably have a changelog entry and should show up in the release announcements. |
|
@daniel-j-h I've added an entry to the changelog on the same branch. Should I do another PR to submit this modification? |
|
Either that or @TheMarex please include it in the final release changelog if you tag a release the next days. |
|
This is handy, thanks! However there's no mention in the documentation about this new attribute: https://github.com/Project-OSRM/osrm-backend/blob/faff2c774d09a7227c77ae4fa40d22d54bb00b45/docs/http.md#nearest-service Is there anything left on this pull request? Otherwise, I wouldn't mind to update the docs. |
Sorry about that, a PR would be much appreciated? |
Issue
The goal is to provide OSM node ids in
nearestresponse as requested by #2548.TODO list
Requirements / Relations
Related to PR #2581