Obvious maneuvers #2436

dgearhart · 2020-06-24T19:20:03Z

Issue

Identify and collapse obvious maneuvers

Fixes #1744
Fixes #91
Fixes #82

Stats for 1k user routes

Improved 67% of the route narratives
Reduced maneuvers by 12%

Example#1

< BEFORE
< 2: Turn left onto US 422/West Chocolate Avenue.
< 3: Continue on US 322 West.
< 4: Take the exit toward Middletown/Hummelstown.
< 5: Turn left onto Quarry Road.
< 6: Continue on Waltonville Road.
< 7: You have arrived at your destination.
> AFTER
> 2: Turn left onto US 422/West Chocolate Avenue.
> 3: Take the exit toward Middletown/Hummelstown.
> 4: Turn left onto Quarry Road.
> 5: You have arrived at your destination.

Example#2

< BEFORE
< 2: Take exit 31C on the left onto White Marsh Boulevard toward White Marsh.
< 3: Continue on MD 43/White Marsh Boulevard.
< 4: Turn right onto Perry Hills Court.
> AFTER
> 2: Take exit 31C on the left onto MD 43 East/White Marsh Boulevard toward White Marsh.
> 3: Turn right onto Perry Hills Court.

Example#3

< BEFORE
< 2: Turn left onto Britzer Damm.
< 3: Continue on Britzer Brücke.
< 4: Continue on Britzer Damm.
< 5: Turn left onto Franz-Körner-Straße.
> AFTER
> 2: Turn left onto Britzer Damm.
> 3: Turn left onto Franz-Körner-Straße.

Tasklist

Add RAD tests
Update gurka tests
Add pinpoint tests
Add gurka tests
Add #fixes with the issue number that this PR addresses
Generally use squash merge to rebase and clean comments before merging
Update the changelog

disabled turn channel for obvious maneuver

…vious_maneuvers

mandeepsandhu · 2020-06-24T19:28:05Z

src/odin/enhancedtrippath.cc

@@ -1297,6 +1300,24 @@ bool EnhancedTripLeg_Node::HasIntersectingEdgeCurrNameConsistency() const {
  return false;
 }

+bool EnhancedTripLeg_Node::HasNonBackwardTraversableSameNameIntersectingEdge(


Can you add some comments on what this function does?

@mandeepsandhu i added comments

danpaz · 2020-06-24T22:07:20Z

src/odin/maneuversbuilder.cc

@@ -2421,8 +2519,14 @@ void ManeuversBuilder::ProcessRoundaboutNames(std::list<Maneuver>& maneuvers) {
      // Process roundabout exit names and signs
      if (next_man->type() == DirectionsLeg_Maneuver_Type_kRoundaboutExit) {
        if (next_man->HasBeginStreetNames()) {
-          curr_man->set_roundabout_exit_begin_street_names(next_man->begin_street_names().clone());
-          curr_man->set_roundabout_exit_street_names(next_man->street_names().clone());
+          if (next_man->contains_obvious_maneuver()) {


Any concerns that this will cause issues when we merge it with #2437 ?

@danpaz I tested - this looks good

Fixed the FormVerbalPostTransitionInstruction method for obvious maneuvers No default values for the test_instructions method

…vious_maneuvers

danpaz · 2020-06-29T17:10:24Z

test/gurka/test_instructions_turn_toward.cc

-      "Continue toward B1, Little Italy. Then You will arrive at your destination.",
-      "Continue for 200 meters.");
-}
+// TODO: expand map for obvious maneuver


Is this still a TODO @dgearhart ?

yes - I will update in separate PR

mandeepsandhu · 2020-06-30T02:31:06Z

src/odin/maneuversbuilder.cc

@@ -2369,6 +2397,76 @@ bool ManeuversBuilder::AreRampManeuversCombinable(std::list<Maneuver>::iterator
  return false;
 }

+bool ManeuversBuilder::IsNextManeuverObvious(std::list<Maneuver>& maneuvers,


I think all the args can be const refs? Or are we modifying them somewhere within (I couldn't find any place where we were, but I might've missed something)?

made params const

mandeepsandhu · 2020-06-30T02:38:38Z

src/odin/maneuversbuilder.cc

+
+    // Return true if a short continue maneuver
+    // and the following maneuver is not a continue
+    if (next_man->length(Options_Units_kilometers) < kShortContinueThreshold) {


Could this function be called with next_man pointing to the last/arrival maneuver? If so, should we return false if next_next_man == maneuvers.end() ?

next_man must be a DirectionsLeg_Maneuver_Type_kContinue

mandeepsandhu · 2020-06-30T02:46:22Z

test/gurka/test_instructions_obvious_maneuver.cc

@@ -0,0 +1,128 @@
+#include "gurka.h"


Thank you for these tests! Its really easy to visualize how the continue collapsing is working! 🙏

mandeepsandhu

Just a few minor comments. Rest LGTM!

…vious_maneuvers

danpaz

👍 nice work!

dgearhart added 11 commits June 17, 2020 17:58

Initial logic to collapse obvious maneuvers

d59a584

removed magic numbers

627c702

disabled turn channel for obvious maneuver

update the IsNextManeuverObvious method to handle ramp forks

cadcb5c

added the obvious_maneuver_routes.txt tests

3eda9bc

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

b99b820

…vious_maneuvers

fixed format

88ff297

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

2fbd58f

…vious_maneuvers

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

4b7b8da

…vious_maneuvers

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

95f67d4

…vious_maneuvers

updated gurka tests due to obvious maneuver logic

baf24bb

updated tests due to obvious maneuvers

d32a56d

mandeepsandhu reviewed Jun 24, 2020

View reviewed changes

dgearhart added 2 commits June 24, 2020 15:37

update comment

7fc6dd2

Added comments

45902f1

danpaz reviewed Jun 24, 2020

View reviewed changes

dgearhart added 3 commits June 25, 2020 11:57

Added obvious maneuver pinpoints

93ec320

Fixed the FormVerbalPostTransitionInstruction method for obvious maneuvers No default values for the test_instructions method

Added obvious maneuver gurka tests

699e125

added #2436 to the changelog

d6681a3

dgearhart self-assigned this Jun 26, 2020

dgearhart added 2 commits June 26, 2020 00:56

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

d9b6e0a

…vious_maneuvers

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

5eab38d

…vious_maneuvers

danpaz reviewed Jun 29, 2020

View reviewed changes

mandeepsandhu reviewed Jun 30, 2020

View reviewed changes

mandeepsandhu previously approved these changes Jun 30, 2020

View reviewed changes

dgearhart added 2 commits June 30, 2020 01:04

Merge branch 'master' of https://github.com/valhalla/valhalla into ob…

f9c1ce5

…vious_maneuvers

made param const

e1851f8

dgearhart dismissed mandeepsandhu’s stale review via e1851f8 June 30, 2020 05:45

danpaz approved these changes Jun 30, 2020

View reviewed changes

dgearhart merged commit 0f160fd into master Jun 30, 2020

dgearhart deleted the obvious_maneuvers branch June 30, 2020 14:43

1ec5 mentioned this pull request Jul 28, 2020

MapboxNavigationNative v18.0.1 mapbox/mapbox-navigation-ios#2477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Obvious maneuvers #2436

Obvious maneuvers #2436

dgearhart commented Jun 24, 2020 •

edited

Loading

mandeepsandhu Jun 24, 2020

dgearhart Jun 24, 2020

danpaz Jun 24, 2020

dgearhart Jun 24, 2020

dgearhart Jun 30, 2020

danpaz Jun 29, 2020

dgearhart Jun 30, 2020 •

edited

Loading

mandeepsandhu Jun 30, 2020

dgearhart Jun 30, 2020

mandeepsandhu Jun 30, 2020

dgearhart Jun 30, 2020

mandeepsandhu Jun 30, 2020

mandeepsandhu left a comment

danpaz left a comment

Obvious maneuvers #2436

Obvious maneuvers #2436

Conversation

dgearhart commented Jun 24, 2020 • edited Loading

Issue

Stats for 1k user routes

Example#1

Example#2

Example#3

Tasklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgearhart Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandeepsandhu left a comment

Choose a reason for hiding this comment

danpaz left a comment

Choose a reason for hiding this comment

dgearhart commented Jun 24, 2020 •

edited

Loading

dgearhart Jun 30, 2020 •

edited

Loading