Extend roundabout phrases #2378

dgearhart · 2020-05-14T21:17:13Z

Issue

Extend roundabout phrase to include street names and toward signs.

Example#1

< Before
< 2: Enter the roundabout and take the 1st exit.
<    VERBAL_ALERT: Enter the roundabout and take the 1st exit.
<    VERBAL_PRE: Enter the roundabout and take the 1st exit.
> After
> 2: Enter the roundabout and take the 1st exit onto US 322/East Governor Road.
>    VERBAL_ALERT: Enter the roundabout and take the 1st exit onto U.S. 3 22.
>    VERBAL_PRE: Enter the roundabout and take the 1st exit onto U.S. 3 22, East Governor Road.

Example#2

< BEFORE
< 2: Enter the roundabout and take the 3rd exit. 
<    VERBAL_ALERT: Enter the roundabout and take the 3rd exit.
<    VERBAL_PRE: Enter the roundabout and take the 3rd exit.
> AFTER
> 2: Enter Dupont Circle Northwest and take the 3rd exit onto 19th Street Northwest. 
>    VERBAL_ALERT: Enter Dupont Circle Northwest and take the 3rd exit onto 19th Street Northwest.
>    VERBAL_PRE: Enter Dupont Circle Northwest and take the 3rd exit onto 19th Street Northwest.

Example#3

< BEFORE
< 2: Enter the roundabout and take the 3rd exit.
<    VERBAL_ALERT: Enter the roundabout and take the 3rd exit.
<    VERBAL_PRE: Enter the roundabout and take the 3rd exit.
< 3: Exit the roundabout onto Dhünner Straße/L 409.
<    VERBAL_PRE: Exit the roundabout onto Dhünner Straße, L 4 09. Then, in 200 feet, You will arrive at Dhünner Straße.
> AFTER
> 2: Enter the roundabout and take the 3rd exit toward A 1/Remscheid/Wermelskirchen.
>    VERBAL_ALERT: Enter the roundabout and take the 3rd exit toward A 1.
>    VERBAL_PRE: Enter the roundabout and take the 3rd exit toward A 1, Remscheid.
> 3: Exit the roundabout toward A 1/Remscheid/Wermelskirchen. 
>    VERBAL_PRE: Exit the roundabout toward A 1, Remscheid. Then, in 200 feet, You will arrive at Dhünner Straße.

Impacts 49% of routes

Tasklist

Add 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

…street names and signs

Added roundabout instructions gurka tests Fixed street name assignemtn in the NarrativeBuilder::FormEnterRoundaboutInstruction method

…rbal phrase blocks. Updated the narrative dictionary and builder for the new phrases. Added named roundabout gurka tests.

…tend_roundabout_phrases

danpaz · 2020-05-14T23:48:42Z

test/gurka/test_instuctions_roundabout.cc

+                                                DirectionsLeg_Maneuver_Type_kDestination});
+  int maneuver_index = 1;
+
+  // TODO: known issue - future update to end on a roundabout


What instruction is announced when you end on a roundabout now?

It has the ordinal exit value included

test/gurka/test_instuctions_roundabout.cc

test/gurka/test_instuctions_named_roundabout.cc

test/gurka/gurka.h

danpaz · 2020-05-14T23:52:04Z

test/gurka/gurka.h

+ * @param maneuver_index the specified maneuver index to inspect
+ * @param expected_instructions the set of four instructions expected in the DirectionsLeg for the
+ *                              route at specified maneuver index.
+ *                              The four instructions shall be in this order:


Nit: how about using an unordered_map instead of a vector so that order doesn't matter?

but then the function call would have more clutter with it - looks nice and simple now:

gurka::assert::raw::expect_instructions_at_maneuver_index( result, maneuver_index, {"Enter Dupont Circle and take the 1st exit toward A 95/B 2/München/Kürten.", "Enter Dupont Circle and take the 1st exit toward A 95.", "Enter Dupont Circle and take the 1st exit toward A 95, München.", ""});

The downside of using a vector is that the caller needs to know/remember the order in which the items need to be put. Using a hashmap makes it a bit more explicit:

gurka::assert::raw::expect_instructions_at_maneuver_index( result, maneuver_index, {{"text", "Enter Dupont Circle and take the 1st exit toward A 95/B 2/München/Kürten."}, {"verbal_transition", "Enter Dupont Circle and take the 1st exit toward A 95."}, {"verbal_pre_transition", "Enter Dupont Circle and take the 1st exit toward A 95, München."}, {"verbal_post_transition", ""}});

But I leave it to you. I guess its okay with vector too. We'll learn the order :)

Let's discuss Monday - we can add an issue to switch it if needed

…tend_roundabout_phrases

… enter_roundabout_verbal, exit_roundabout, exit_roundabout_verbal

…tend_roundabout_phrases

mandeepsandhu

I have only minor comments. Approving it since I don't want to hold you up.

mandeepsandhu · 2020-05-15T20:35:09Z

locales/en-US-x-pirate.json

@@ -353,7 +353,21 @@
    "enter_roundabout": {
      "phrases": {
        "0": "Enter thee Cap'n wheel.",
-        "1": "Enter thee Cap'n wheel and take th' <ORDINAL_VALUE> spoke."
+        "1": "Enter thee Cap'n wheel and take th' <ORDINAL_VALUE> spoke.",


What does ...-x-pirate mean?

non standard language extension - pirate language narrative FTW 🏴‍☠️

mandeepsandhu · 2020-05-15T20:50:44Z

locales/hi-IN.json

@@ -353,7 +353,21 @@
    "enter_roundabout": {
      "phrases": {
        "0": "सर्कल प्रवेश करें.",
-        "1": "सर्कल प्रवेश करें और <ORDINAL_VALUE> निकास लें."
+        "1": "सर्कल प्रवेश करें और <ORDINAL_VALUE> निकास लें.",


Not a comment for this PR, but this caught my eye - spelling out "circle" in Hindi seems a bit odd for roundabout. The colloquial term for traffic circle is "roundabout" (or "Gol Chakkra" in Hindi). Anyway, we dont need to change anything in this PR.

please fix anything that is incorrect

mandeepsandhu · 2020-05-15T22:13:02Z

src/odin/maneuver.cc

+  roundabout_exit_begin_street_names_ = std::move(roundabout_exit_begin_street_names);
+}
+
+bool Maneuver::HasRoundaboutExitBeginStreetNames() const {


Why is this camel-case while the others snake-case? Looking at maneuver.h, it seems like only getters/setters use camel-case? I must say its a strange convention.

when we started we followed the google spec - similar to how protobuf files are

mandeepsandhu · 2020-05-15T22:43:33Z

src/odin/signs.cc

+                                        const std::string& delim,
+                                        const VerbalTextFormatter* verbal_formatter) const {
+  std::string guide_string;
+  if (HasGuideBranch() && HasGuideToward() && ((max_count == 0) || (max_count > 1))) {


I dont follow the max_count check. Since its an unsigned int, it'll always be >0 or >1.

A slightly more concise version of what this function is doing:

std::string guide_string; if (HasGuideBranch()) { guide_string += GetGuideBranchString(static_cast<uint32_t>(std::round(max_count / 2)), limit_by_consecutive_count, delim, verbal_formatter); if (HasGuideToward()) guide_string += delim; } if (HasGuideToward()) { guide_string += GetGuideTowardString(max_count, limit_by_consecutive_count, delim, verbal_formatter); }

But you can leave it as-is too (except for maybe the max_count check?)

max_count == 0 means no limit
so this is checking if there is no limit or greater than one
we want to split the max_count between branch and toward

I get the split, but not the need for the check (((max_count == 0) || (max_count > 1)) is always true). Also std::round isn't doing anything, since we're using integer division which will return an integer.

A slightly more concise version of what this function is doing:

This is different functionality

I get the split, but not the need for the check (((max_count == 0) || (max_count > 1)) is always true).

It is not true if max_count == 1
I was trying to be explicit about either unlimited count or greater than one

Also std::round isn't doing anything, since we're using integer division which will return an integer.

@mandeepsandhu good catch - I will fix, thanks

mandeepsandhu · 2020-05-15T23:05:24Z

test/gurka/gurka.h

+ * @param maneuver_index the specified maneuver index to inspect
+ * @param expected_instructions the set of four instructions expected in the DirectionsLeg for the
+ *                              route at specified maneuver index.
+ *                              The four instructions shall be in this order:


The downside of using a vector is that the caller needs to know/remember the order in which the items need to be put. Using a hashmap makes it a bit more explicit:

gurka::assert::raw::expect_instructions_at_maneuver_index( result, maneuver_index, {{"text", "Enter Dupont Circle and take the 1st exit toward A 95/B 2/München/Kürten."}, {"verbal_transition", "Enter Dupont Circle and take the 1st exit toward A 95."}, {"verbal_pre_transition", "Enter Dupont Circle and take the 1st exit toward A 95, München."}, {"verbal_post_transition", ""}});

But I leave it to you. I guess its okay with vector too. We'll learn the order :)

dgearhart added 4 commits May 13, 2020 17:23

Extended the enter roundabout phrases to include the post roundabout …

fb5db1a

…street names and signs

Extended gurka to have the expect_instructions_at_maneuver_index method.

4cd1fd7

Added roundabout instructions gurka tests Fixed street name assignemtn in the NarrativeBuilder::FormEnterRoundaboutInstruction method

Added the toward phrase to the exit_roundabout and exit_roundabout_ve…

1e1e27d

…rbal phrase blocks. Updated the narrative dictionary and builder for the new phrases. Added named roundabout gurka tests.

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

e06a8b0

…tend_roundabout_phrases

dgearhart self-assigned this May 14, 2020

dgearhart requested review from danpaz and mandeepsandhu May 14, 2020 21:18

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

7907c2f

…tend_roundabout_phrases

danpaz reviewed May 14, 2020

View reviewed changes

test/gurka/test_instuctions_roundabout.cc Outdated Show resolved Hide resolved

danpaz reviewed May 14, 2020

View reviewed changes

test/gurka/test_instuctions_named_roundabout.cc Outdated Show resolved Hide resolved

danpaz reviewed May 14, 2020

View reviewed changes

test/gurka/gurka.h Outdated Show resolved Hide resolved

danpaz reviewed May 14, 2020

View reviewed changes

danpaz mentioned this pull request May 15, 2020

Add sk-SK.json (slovak) localization file #2376

Merged

4 tasks

dgearhart added 12 commits May 14, 2020 20:59

Fixed typo

c66e693

switched to ASSERT

52fd6d6

clean up

5daad85

more clang tidy clean up

202ede6

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

06d8e80

…tend_roundabout_phrases

Updated the following phrase blocks for sk-SK.json: enter_roundabout,…

c39e448

… enter_roundabout_verbal, exit_roundabout, exit_roundabout_verbal

updated changelog for #2376 and 2378

c080e1d

added a missing include

f225558

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

48b9dcd

…tend_roundabout_phrases

Only enhance empty street names if enter/exit are combined

335e2ac

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

ff0c9c4

…tend_roundabout_phrases

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

bfad28e

…tend_roundabout_phrases

mandeepsandhu approved these changes May 15, 2020

View reviewed changes

dgearhart merged commit b81aa89 into master May 16, 2020

dgearhart deleted the extend_roundabout_phrases branch May 16, 2020 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend roundabout phrases #2378

Extend roundabout phrases #2378

dgearhart commented May 14, 2020 •

edited

Loading

danpaz May 14, 2020

dgearhart May 15, 2020

danpaz May 14, 2020

dgearhart May 15, 2020

mandeepsandhu May 15, 2020

dgearhart May 16, 2020

mandeepsandhu left a comment

mandeepsandhu May 15, 2020

dgearhart May 16, 2020 •

edited

Loading

mandeepsandhu May 15, 2020

dgearhart May 16, 2020

mandeepsandhu May 15, 2020

dgearhart May 16, 2020

mandeepsandhu May 15, 2020

dgearhart May 16, 2020

mandeepsandhu May 16, 2020

dgearhart May 18, 2020

dgearhart May 18, 2020

dgearhart May 18, 2020

mandeepsandhu May 15, 2020

Extend roundabout phrases #2378

Extend roundabout phrases #2378

Conversation

dgearhart commented May 14, 2020 • edited Loading

Issue

Example#1

Example#2

Example#3

Impacts 49% of routes

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

mandeepsandhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgearhart May 16, 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

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 commented May 14, 2020 •

edited

Loading

dgearhart May 16, 2020 •

edited

Loading