Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure route/leg summaries are unique #2874

Merged
merged 17 commits into from
Mar 1, 2021
Merged

Ensure route/leg summaries are unique #2874

merged 17 commits into from
Mar 1, 2021

Conversation

ktatterso
Copy link
Contributor

@ktatterso ktatterso commented Feb 18, 2021

Issue

Summary: When multiple routes are returned, depending on the nature of the routes chosen, each route might have the same route summary. This is because the original logic would limit the number of route-names in the summary to the two longest legs.

To make the route/leg summaries unique, the new logic keeps adding the next longest named-segment into the summary until that summary is unique among all route/leg summaries.

Real world example #1

coordinates (lon,lat):
10.169593009081325,50.342127671261125;8.633053521619985,49.918773879486025

Curl request:
curl http://localhost:8002/route --data '{"locations":[{"lat":50.342127671261125,"lon":10.169593009081325,"type":"break"},{"lat":49.918773879486025,"lon":8.633053521619985,"type":"break"}],"costing":"auto","alternates":3,"directions_options":{"units":"kilometers","format":"osrm"}}'

image

Two routes are returned. Previously, these summaries were returned:
route 1 summary: B 279, A 66
route 2 summary: B 279, A 66

With my fix these summaries are returned:
route 1 summary: B 279, A 66, A 661
route 2 summary: B 279, L 2304, A 66

Real world example #2

coordinates (lon,lat):
-115.46257799894421,32.67751487912436;-121.48215941379783,38.55516780677243

Curl request:
curl http://localhost:8002/route --data '{"locations":[{"lat":32.67751487912436,"lon":-115.46257799894421,"type":"break"},{"lat":38.55516780677243,"lon":-121.48215941379783,"type":"break"}],"costing":"auto","alternates":3,"directions_options":{"units":"kilometers","format":"osrm"}}'

image

Two routes are returned. Previously these summaries were returned:
route 1 summary: CA 86 North, I 5 North
route 2 summary: CA 86 North, I 5 North

With my fix these summaries are returned:
route 1 summary: CA 86 North, Foothill Freeway, I 5 North
route 2 summary: CA 86 North, CA 58, I 5 North

Real world example #3

coordinates (lon,lat):
0.697933819865483,47.3949235422551;7.510451765734274,51.58151535543996

Curl request:
curl http://localhost:8002/route --data '{"locations":[{"lat":47.3949235422551,"lon":0.697933819865483,"type":"break"},{"lat":51.58151535543996,"lon":7.510451765734274,"type":"break"}],"costing":"auto","alternates":3,"directions_options":{"units":"kilometers","format":"osrm"}}'

image

Two routes are returned. Previously these summaries were returned:
route 1 summary: A 10, A 1
route 2 summary: A 10, A 1

With my fix these summaries are returned:
route 1 summary: A 10, A 1, A 2, E40
route 2 summary: A 10, A 1, A 2, E314

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

…it doesn't have support for RepeatedPtrField<Element>::at(int index). So I switched it to use the array operator.
@dgearhart
Copy link
Member

@ktatterso can you supply some before and after examples?

Copy link
Contributor

@merkispavel merkispavel left a comment

Choose a reason for hiding this comment

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

  1. A few minor suggestions to use std::move() where we don't need the object anymore
  2. I left a comment src/tyr/route_serializer_osrm.cc with another possible solution within your approach. Let me know what you think about it

src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved
src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved
src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved
src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved
src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved
src/tyr/route_serializer_osrm.cc Show resolved Hide resolved
src/tyr/route_serializer_osrm.cc Show resolved Hide resolved
Copy link
Contributor

@merkispavel merkispavel left a comment

Choose a reason for hiding this comment

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

well done! thx

@dgearhart
Copy link
Member

@ktatterso I will check out branch and try some more examples. I will post what I was mentioning the other day

@nilsnolde
Copy link
Member

@ktatterso re format.sh script: I know the pain.. I added cat scripts/format.sh > .git/hooks/pre-commit && tail -n +7 scripts/error_on_dirty.sh >> .git/hooks/pre-commit to have it as pre-commit hook and error out when it's dirty before committing

@ktatterso ktatterso merged commit da75fe9 into master Mar 1, 2021
@ktatterso ktatterso deleted the summarize-leg-impr branch March 1, 2021 21:36
Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

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

Works nicely on most of the test cases I ran. My only comment was what I mentioned the other day - since we collapsed some maneuvers with the obvious maneuver logic - summary may not always reflect primary street names based on the edges. Thanks @ktatterso

@ktatterso
Copy link
Contributor Author

Thanks @dgearhart

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.

None yet

5 participants