New Verbal Succinct Transition Instruction #2844

kdiluca · 2021-02-08T17:45:25Z

Description

Added logic to directions.proto, maneuver, maneuvers_builder and narrative_builder for a verbal succinct transition instruction.

Issue

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.

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?

…aneuvers_builder and narrative_builder

src/odin/maneuversbuilder.cc

danpaz · 2021-02-08T23:34:55Z

src/odin/narrativebuilder.cc

+}
+
+std::string NarrativeBuilder::FormVerbalSuccinctDestinationTransitionInstruction(Maneuver& maneuver) {
+  // "0": "You have arrived at your destination."


Any chance this can be even more succinct, You have arrived.?

kdiluca · 2021-02-09T18:31:20Z

1: Drive north on Carretera de Sant Carles. | 0.2 mi
   VERBAL_SUCCINCT: Drive north.
   VERBAL_PRE: Drive north on Carretera de Sant Carles. Then Turn left onto Carretera de Santa Agnès de Malanyanes al Coll.
   VERBAL_POST: Continue for 900 feet.
----------------------------------------------
2: Turn left onto Carretera de Santa Agnès de Malanyanes al Coll. | 0.1 mi
   VERBAL_SUCCINCT: Turn left.
   VERBAL_ALERT: Turn left onto Carretera de Santa Agnès de Malanyanes al Coll.
   VERBAL_PRE: Turn left onto Carretera de Santa Agnès de Malanyanes al Coll.
   VERBAL_POST: Continue for 700 feet.
----------------------------------------------
3: Turn left to stay on Carretera de Santa Agnès de Malanyanes al Coll. | 0.6 mi
   VERBAL_SUCCINCT: Turn left.
   VERBAL_ALERT: Turn left to stay on Carretera de Santa Agnès de Malanyanes al Coll.
   VERBAL_PRE: Turn left to stay on Carretera de Santa Agnès de Malanyanes al Coll.
   VERBAL_POST: Continue for a half mile.
----------------------------------------------
4: Your destination is on the left. | 0.0 mi
   VERBAL_SUCCINCT: You have arrived at your destination.
   VERBAL_ALERT: Your destination will be on the left.
   VERBAL_PRE: Your destination is on the left.
==============================================
Total time: 3 minutes
Total length: 0.9 mi

dgearhart · 2021-02-09T18:42:23Z

VERBAL_SUCCINCT: You have arrived at your destination.

We will have to discuss this one

…names

kdiluca · 2021-02-10T17:56:42Z

VERBAL_SUCCINCT: You have arrived at your destination.

We will have to discuss this one

We have quite a few we need to discuss 😄

…names

dgearhart · 2021-02-25T14:33:33Z

src/odin/maneuversbuilder.cc

+void ManeuversBuilder::ProcessVerbalSuccinctTransitionInstruction(std::list<Maneuver>& maneuvers) {
+  for (auto& maneuver : maneuvers) {
+    if (maneuver.HasStreetNames()) {
+      std::string street_name = maneuver.street_names().front()->value();


Update to look at word count that matches kVerbalPreElementMaxCount

dgearhart · 2021-03-02T14:51:34Z

src/odin/narrativebuilder.cc

+  return instruction;
+}
+
+std::string NarrativeBuilder::FormVerbalSuccinctDestinationTransitionInstruction() {


remove for now

I'm going to leave for now since we discussed this.

dgearhart · 2021-03-02T14:52:19Z

valhalla/odin/narrativebuilder.h

+  /////////////////////////////////////////////////////////////////////////////
+  std::string FormVerbalSuccinctStartTransitionInstruction(Maneuver& maneuver);
+
+  std::string FormVerbalSuccinctDestinationTransitionInstruction();


remove for now

I'm going to leave for now since we discussed this.

…names

…ructions

dgearhart · 2021-04-21T16:03:42Z

src/odin/narrativebuilder.cc

@@ -79,7 +79,7 @@ void NarrativeBuilder::Build(std::list<Maneuver>& maneuvers) {

        // Set verbal succinct transition instruction
        maneuver.set_verbal_succinct_transition_instruction(


If the string is large then we will want to set the succinct to either phrase 0 or 2

// "0": "You have arrived at your destination." // "2": "Your destination is on the <RELATIVE_DIRECTION>."

Otherwise, we do not want to set succinct instruction for the destination maneuver

dgearhart · 2021-04-22T01:37:52Z

test/instructions.cc

@@ -538,7 +546,8 @@ TEST(Instructions, validate_obvious_maneuver_instructions) {
                     "test/pinpoints/instructions/obvious_maneuver_short_continue.pbf"},
                    expected_routes_size, expected_legs_size, expected_maneuvers_size, maneuver_index,
                    "Take the PA 39 West/Hersheypark Drive exit toward Attractions.",
-                    "Take the exit.", "Take the Pennsylvania 39 West exit.",
+                    "Take the Pennsylvania 39 West, Hersheypark Drive exit toward Attractions.",


Does not look right - maybe for initial implementation, concentrate on maneuvers that are impacted by long street name - what are your thoughts?

…names

…ive direction

…names

dgearhart · 2021-04-26T11:58:22Z

src/odin/narrativebuilder.cc

@@ -206,6 +225,10 @@ void NarrativeBuilder::Build(std::list<Maneuver>& maneuvers) {
          // Set stay on instruction
          maneuver.set_instruction(FormKeepToStayOnInstruction(maneuver));

+          // Set verbal succinct transition instruction
+          maneuver.set_verbal_succinct_transition_instruction(


Remove for now

dgearhart · 2021-04-26T11:58:37Z

src/odin/narrativebuilder.cc

@@ -308,6 +347,10 @@ void NarrativeBuilder::Build(std::list<Maneuver>& maneuvers) {
        // Set instruction
        maneuver.set_instruction(FormEnterFerryInstruction(maneuver));

+        // Set verbal succinct transition instruction
+        maneuver.set_verbal_succinct_transition_instruction(


Remove for now

dgearhart · 2021-04-26T12:01:37Z

src/odin/narrativebuilder.cc

+}
+
+std::string
+NarrativeBuilder::FormVerbalSuccinctKeepTransitionInstruction(Maneuver& maneuver,


Remove for now

dgearhart · 2021-04-26T12:01:55Z

src/odin/narrativebuilder.cc

+}
+
+std::string
+NarrativeBuilder::FormVerbalSuccinctEnterFerryTransitionInstruction(Maneuver& maneuver,


Remove for now

dgearhart · 2021-04-26T13:48:18Z

test/gurka/gurka.cc

@@ -964,6 +966,7 @@ void expect_instructions_at_maneuver_index(
    const valhalla::Api& result,
    int maneuver_index,
    const std::string& expected_text_instruction,
+    const std::string& expected_verbal_succinct_transition_instruction,


probably not needed until - maybe only after focused gurka test are complete

probably not needed until - maybe only after focused gurka test are complete

or leave in if it is easier to just fix the tests after code is complete

…names

…ordingly

kdiluca · 2021-06-04T01:51:23Z

Closing and opened new rebased PR #3128

kdiluca added 2 commits February 8, 2021 12:41

added new verbal_succinct_transition_instruction logic to maneuver, m…

7b2ab37

…aneuvers_builder and narrative_builder

fixed imports and check if streetnames exist per maneuver

6d2ba97

danpaz reviewed Feb 8, 2021

View reviewed changes

src/odin/maneuversbuilder.cc Outdated Show resolved Hide resolved

danpaz reviewed Feb 8, 2021

View reviewed changes

kdiluca added 3 commits February 9, 2021 12:33

added more testing

a337ab9

fixed FormVerbalSuccinctStartTransitionInstruction

2fb15b3

Merge branch 'master' into kdiluca-long-streetnames

f248903

kdiluca added 4 commits February 10, 2021 12:28

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

39b1f5c

…names

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

a233607

…names

reorganized and updated narrativebuilder test

8a07c27

lint

4fccdc0

kdiluca added 6 commits February 10, 2021 15:34

more test edits

5da6131

update tests with succinct logic updates

756195d

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

30b46c7

…names

fixed formatting

56d963e

fixed formatting

ee34ff7

updated changelog

dc944bb

kdiluca requested a review from dgearhart February 16, 2021 15:52

kdiluca added 4 commits February 16, 2021 11:18

removing maneuver where not needed for now

92f41c1

updated gurka tests

a3e4fd7

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

2a3a217

…names

Merge branch 'master' into kdiluca-long-streetnames

fcd44ac

dgearhart reviewed Feb 25, 2021

View reviewed changes

dgearhart reviewed Mar 2, 2021

View reviewed changes

kdiluca added 2 commits March 2, 2021 12:14

Merge branch 'master' into kdiluca-long-streetnames

ef9d7e9

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

303a36d

…names

kdiluca added 5 commits April 19, 2021 12:24

Merge remote-tracking branch 'origin/master' into kdiluca-strlen

6376d04

updated changelog

6527509

fixed merged conflict

6ea65d4

updated util with new utf-8 strlen functions

dbc58cd

updated to use new strlen function and added towards to succinct inst…

5024d83

…ructions

kdiluca requested a review from dgearhart April 19, 2021 17:42

kdiluca mentioned this pull request Apr 19, 2021

Added word length and count functions to util Odin #3016

Closed

4 tasks

kdiluca added 2 commits April 20, 2021 11:01

fix conflicts after master merge

ab33787

removed unused succinct functions and fixed destination succinct use

86d93c6

dgearhart reviewed Apr 21, 2021

View reviewed changes

updating tests for succinct

71ceba7

dgearhart reviewed Apr 22, 2021

View reviewed changes

kdiluca added 6 commits April 22, 2021 09:25

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

0edc7b3

…names

fixed FormVerbalSuccinctDestinationTransitionInstruction to use relat…

dbd8e7a

…ive direction

update

685665c

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

ca8141a

…names

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

c7076dd

…names

removed the setting of succinct if we are currently not going to use it

edccd19

dgearhart reviewed Apr 26, 2021

View reviewed changes

kdiluca added 3 commits May 28, 2021 13:18

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

68640de

…names

Merge remote-tracking branch 'origin/master' into kdiluca-long-street…

79166c8

…names

removed succinct enter ferry and succinct keep and adjusted tests acc…

221e5d5

…ordingly

kdiluca mentioned this pull request Jun 4, 2021

Succinct Verbal Instruction #3128

Merged

5 tasks

kdiluca closed this Jun 4, 2021

nilsnolde deleted the kdiluca-long-streetnames branch February 24, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Verbal Succinct Transition Instruction #2844

New Verbal Succinct Transition Instruction #2844

kdiluca commented Feb 8, 2021

danpaz Feb 8, 2021

kdiluca commented Feb 9, 2021

dgearhart commented Feb 9, 2021

kdiluca commented Feb 10, 2021

dgearhart Feb 25, 2021

dgearhart Mar 2, 2021

kdiluca Mar 4, 2021

dgearhart Mar 2, 2021

kdiluca Mar 4, 2021

dgearhart Apr 21, 2021

dgearhart Apr 22, 2021

dgearhart Apr 26, 2021

dgearhart Apr 26, 2021

dgearhart Apr 26, 2021

dgearhart Apr 26, 2021

dgearhart Apr 26, 2021

dgearhart Apr 26, 2021

kdiluca commented Jun 4, 2021

		@@ -79,7 +79,7 @@ void NarrativeBuilder::Build(std::list<Maneuver>& maneuvers) {

		// Set verbal succinct transition instruction
		maneuver.set_verbal_succinct_transition_instruction(

New Verbal Succinct Transition Instruction #2844

New Verbal Succinct Transition Instruction #2844

Conversation

kdiluca commented Feb 8, 2021

Description

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

kdiluca commented Feb 9, 2021

dgearhart commented Feb 9, 2021

kdiluca commented Feb 10, 2021

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

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

Choose a reason for hiding this comment

kdiluca commented Jun 4, 2021