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

Fix pedestrian fork #1886

Merged
merged 4 commits into from
Jul 18, 2019
Merged

Fix pedestrian fork #1886

merged 4 commits into from
Jul 18, 2019

Conversation

dgearhart
Copy link
Member

@dgearhart dgearhart commented Jul 17, 2019

Issue

Pedestrian fork was being called out when it was not required. Updated logic to prevent incorrect behavior.

< BEFORE
< 2: Keep straight to take Avenue Ledru-Rollin. | 0.1 km
< 3: Keep right to stay on Avenue Ledru-Rollin. | 0.2 km
> AFTER
> 2: Keep straight to take Avenue Ledru-Rollin. | 0.3 km

Tasklist

  • Test
  • Review - you must request approval to merge any PR to master
  • Generally use squash merge to rebase and clean comments before merging
  • Update the changelog
  • Update relevant documentation

@dgearhart dgearhart self-assigned this Jul 17, 2019
uint32_t straight_delta = (intersecting_turn_degree > 180) ? (360 - intersecting_turn_degree)
: intersecting_turn_degree;
if (xedge_traversable_outbound && (straight_delta < staightest_delta)) {
staightest_delta = straight_delta;
staightest_turn_degree = intersecting_turn_degree;
// If a use pointer was passed in and the intersecting edge has a use then set
if ((use != nullptr) && xedge->has_use()) {
*use = xedge->use();
Copy link
Member Author

Choose a reason for hiding this comment

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

this will set the use of the straightest traversable intersecting edge

if (is_pedestrian_travel_mode && is_relative_straight(path_turn_degree)) {
// and less than 3 intersecting edges
if (is_pedestrian_travel_mode && is_relative_straight(path_turn_degree) &&
(node->intersecting_edge_size() < 3)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

intersecting edge count needed to be limited for a fork

if (((((xedge_counts.left_similar_traversable_outbound > 0) ||
(xedge_counts.right_similar_traversable_outbound > 0)) ||
is_relative_straight(straightest_traversable_xedge_turn_degree)) &&
(curr_edge->use() == xedge_use)) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

the use for current edge and intersecting edge must match - therefore, we will not get alley forks off of main roads - this fixes the user issue

case valhalla::DirectionsLeg_Maneuver_Type_kExitRight:
return "slight right";
case valhalla::DirectionsLeg_Maneuver_Type_kRight:
case valhalla::DirectionsLeg_Maneuver_Type_kStayRight:
Copy link
Member Author

Choose a reason for hiding this comment

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

update so right fork has modifier of right

@@ -722,12 +722,12 @@ std::string turn_modifier(const valhalla::DirectionsLeg::Maneuver& maneuver,
case valhalla::DirectionsLeg_Maneuver_Type_kSharpLeft:
return "sharp left";
case valhalla::DirectionsLeg_Maneuver_Type_kLeft:
case valhalla::DirectionsLeg_Maneuver_Type_kStayLeft:
Copy link
Member Author

Choose a reason for hiding this comment

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

update so left fork has modifier of left

@@ -40,3 +40,4 @@
-j '{"locations":[{"lat":40.041744,"lon":-76.308380,"street":"Roburrito's"},{"lat":40.042534,"lon":-76.299171,"street":"East Fulton Street"}],"costing":"pedestrian","directions_options":{"units":"miles"}}'
-j '{"locations":[{"lat":45.848179,"lon":6.226011},{"lat":45.859264,"lon":6.249594}],"costing":"pedestrian","costing_options":{"pedestrian":{"max_hiking_difficulty":1}},"directions_options":{"units":"miles"}}'
-j '{"locations":[{"lat":45.848179,"lon":6.226011},{"lat":45.859264,"lon":6.249594}],"costing":"pedestrian","costing_options":{"pedestrian":{"max_hiking_difficulty":2}},"directions_options":{"units":"miles"}}'
-j '{"locations":[{"lat":48.85752653288673,"lon":2.379750442551377},{"lat":48.8550339,"lon":2.3781273}],"costing":"pedestrian"}'
Copy link
Member Author

Choose a reason for hiding this comment

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

added the user test case

@kevinkreiser
Copy link
Member

some day in the future we should do some "integration tests" on the utrecht dataset so that all the code in odin is hit. RAD is awesome, but itd be nice to have a small automatic RAD that gets run in CI to detect anything obvious.

@kdiluca
Copy link
Member

kdiluca commented Jul 18, 2019

Needs some additional code coverage

@dgearhart dgearhart merged commit b745911 into master Jul 18, 2019
@dgearhart
Copy link
Member Author

Agreed - we have plans in the future for the improved automatic tests. I think we have some other datasets that will be more beneficial then utrecht dataset - since local knowledge is helpful.

@dgearhart dgearhart added the bug label Jul 18, 2019
@dgearhart dgearhart deleted the fix_pedestrian_fork branch July 18, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants