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
Update the turn and continue phrases to include junction names and guide signs #2386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some questions. LGTM.
*(next_man->mutable_signs()) = curr_man->signs(); | ||
} | ||
// NOTE: Do not copy signs from internal maneuver | ||
// It would produce invalid results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on this note? Was there a bug that this fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danpaz Found during RAD testing - when people incorrectly tag internal edges with sign info - therefore, it led to an incorrect turn toward phrase. Created gurka test to make sure we do not see in the future
std::string guide_sign; | ||
|
||
if (maneuver.HasGuideSign()) { | ||
// Set the toward phrase - it takes priority over street names and junction name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting we may want to revisit the priority ordering of GuideSign > JunctionName > StreetName at some point. Also it may be preferable to announce JunctionName and GuideSign together Turn left at X toward Y
. No change needed for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the same possibility but kept to the current plan for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Issue
Updated the following phrases to include junction names and guide signs
Example#1 junction name
Example#2 junction name
Example#3 junction name
Example#4 guide signs
Example#5 guide signs
Impacts 20% of routes
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?