Add support for exits. #116

willwhite · 2017-07-05T03:42:30Z

This adds support for the upcoming exits support in OSRM. We are introducing "Take exit {exit}" text here but it's applied pretty narrowly right now - it is only used for off ramps that are tagged with exit information.

Fixes #114

willwhite · 2017-07-05T03:43:27Z

languages/translations/en.json

            },
            "left": {
                "default": "Take the ramp on the left",
                "name": "Take the ramp on the left onto {way_name}",
-                "destination": "Take the ramp on the left towards {destination}"
+                "destination": "Take the ramp on the left towards {destination}",
+                "exit": "Take exit {exit} on the left",


I'm explicit about "on the left" here because left exits are more unusual.

That is only true for right-hand driving countries. Ideally we'd have left-hand/right-hand driving country information here. This is ticketed upstream Project-OSRM/osrm-backend#2269, assuming it would also be exposed in the API responses.

For now, I would remove direction on ramps completely. (Take the ramp on the {left/right} -> Take the ramp)

Makes sense, but I went in the other direction with 38c6630 and added specific instructions until we have enough information to simplify.

@freenerd just saw your note further down:

Decide on exit directions (i vote for remove left/right)

I learn toward more specificity here because it can be super jarring when there is a left exit and driver really do expect it to be specifically called out.

willwhite · 2017-07-05T03:44:21Z

languages/translations/en.json

-                "destination": "Take the ramp towards {destination}"
+                "destination": "Take the ramp towards {destination}",
+                "exit": "Take exit {exit}",
+                "exit_destination": "Take exit {exit} towards {destination}"


Wasn't sure about using an _ here style wise. There isn't a precedent for complex/multiple fields.

daniel-j-h

Thanks for this quick follow-up! I had a look and left some comments on the fixtures: some situations could benefit from adding the exit information in my opinion.

daniel-j-h · 2017-07-05T08:53:23Z

test/fixtures/v5/continue/uturn_exit.json

+    },
+    "instructions": {
+        "de": "180°-Wendung auf Way Name",
+        "en": "Make a U-turn onto Way Name",


Hm, thinking about situations for which a u-turn taking an exit occurs. As it's a complex maneuver for the driver do we really not want to add the exits information in addition here?

Exits only make sense in the off-ramp case as far as I know. I would be surprised if there are any tags used in other context, but I think we should focus on the highway/ramp use case first.

daniel-j-h · 2017-07-05T08:53:53Z

test/fixtures/v5/continue/uturn_exit_destination.json

+    },
+    "instructions": {
+        "de": "180°-Wendung Richtung Destination 1",
+        "en": "Make a U-turn towards Destination 1",


daniel-j-h · 2017-07-05T08:55:12Z

test/fixtures/v5/end_of_road/left_exit.json

+            "modifier": "left"
+        },
+        "name": "Way Name",
+        "exits": "4A,4B"


Not sure when end-of-road can occur in combination with an exit - these may be good candidates for validation handlers? cc @MoKob

daniel-j-h · 2017-07-05T08:56:08Z

test/fixtures/v5/fork/left_exit.json

+    },
+    "instructions": {
+        "de": "Links halten an der Gabelung auf Way Name",
+        "en": "Keep left at the fork onto Way Name",


Is there a reason we don't add the exit information here? I can see how the "keep left" and the exit information might seem redundant but wouldn't it be a confirmation for the user to take a specific exit while keeping left?

daniel-j-h · 2017-07-05T08:58:09Z

test/fixtures/v5/merge/left_exit.json

+            "modifier": "left"
+        },
+        "name": "Way Name",
+        "exits": "4A,4B"


Also a case for validation. cc @MoKob

I don't think a merge and exit should ever be able to be in the same instruction.

daniel-j-h · 2017-07-05T08:59:02Z

test/fixtures/v5/notification/left_exit.json

+    },
+    "instructions": {
+        "de": "Links weiterfahren auf Way Name",
+        "en": "Continue left onto Way Name",


Why no exit information here?

The use case for cars here would be to have an exit on/off a ferry? Not sure if these will ever be well tagged.

daniel-j-h · 2017-07-05T08:59:32Z

test/fixtures/v5/off_ramp/left_exit.json

+    },
+    "instructions": {
+        "de": "Rampe auf der linken Seite nehmen auf Way Name",
+        "en": "Take exit 4A on the left",


Exit information is only in english translation (yet?)

daniel-j-h · 2017-07-05T09:00:35Z

test/fixtures/v5/off_ramp/uturn_exit.json

+        "exits": "4A,4B"
+    },
+    "instructions": {
+        "de": "Rampe nehmen auf Way Name",


German nitpick - are these really called "Rampe" and not rather "Ausfahrt"? cc @freenerd

Moving to #118

daniel-j-h · 2017-07-05T09:01:48Z

test/fixtures/v5/rotary/default_exit.json

+    },
+    "instructions": {
+        "de": "In den Kreisverkehr fahren und auf Way Name verlassen",
+        "en": "Enter the rotary and exit onto Way Name",


In complex roundabouts/rotaries/traffic circles it could make sense to add the exit information if we have it.

MoKob

I have to say that I am a bit concerned with adding so many fixtures for impossible combinations.

merge uturn for example would never be possible. Just like a fork sharp right or a new name uturn. Adding fixtures for these seems like a lot of adjustment that might come up later if we want to change wordings (related: https://github.com/mapbox/api-directions/issues/1053)

Also: I see that most of the advanced types we generate are treated just the same. E.g. new name, end of road ... are all announced as turns. If we don't expose any of these at all, we can switch to emitting only turn and ramps in the backend.

MoKob · 2017-07-05T09:10:50Z

test/fixtures/v5/fork/sharp_left_exit.json

+        "exits": "4A,4B"
+    },
+    "instructions": {
+        "de": "Scharf links abbiegen an der Gabelung auf Way Name",


does this type come out of the backend? I would hope not 😨

Do you mean that fork + sharp left should never occur?

MoKob · 2017-07-05T09:12:29Z

test/fixtures/v5/merge/left_exit.json

+            "modifier": "left"
+        },
+        "name": "Way Name",
+        "exits": "4A,4B"


I don't think a merge and exit should ever be able to be in the same instruction.

MoKob · 2017-07-05T09:19:28Z

test/fixtures/v5/roundabout/exit_exit.json

+        "exits": "4A,4B"
+    },
+    "instructions": {
+        "de": "In den Kreisverkehr fahren und erste Ausfahrt nehmen auf Way Name",


German is kind of verbose on these. Wouldn't it be obvious to enter the roundabout? We could easily say Im Kreisverkehr die erste Ausfahrt auf Way Name nehmen, replacing In den fahren with Im.

Moving to #117

freenerd

I moved the german-specific issues to own tickets #117 #118.

I have to say that I am a bit concerned with adding so many fixtures for impossible combinations.

@MoKob It feels like which combinations are possible is a moving target and we don't even have solid data on what can occur and what not. We therefore opted to include all combinations that could ever occur, even if some are potentially never going to come up.

I see that most of the advanced types we generate are treated just the same. E.g. new name, end of road ... are all announced as turns. If we don't expose any of these at all, we can switch to emitting only turn and ramps in the backend.

@MoKob even if they are not used in text instructions, they may still get dedicated turn symbols or different announcement patterns. Thus they make sense.

@willwhite PR generally looks good. Let me know if I should take on some of the

Decide on exit directions (i vote for remove left/right)
Add exit for more instruction types
Connect new strings in Transifex

freenerd · 2017-07-05T11:42:33Z

languages/translations/en.json

-                "destination": "Take the ramp towards {destination}"
+                "destination": "Take the ramp towards {destination}",
+                "exit": "Take exit {exit}",
+                "exit_destination": "Take exit {exit} towards {destination}"


freenerd · 2017-07-05T11:45:29Z

languages/translations/en.json

            },
            "left": {
                "default": "Take the ramp on the left",
                "name": "Take the ramp on the left onto {way_name}",
-                "destination": "Take the ramp on the left towards {destination}"
+                "destination": "Take the ramp on the left towards {destination}",
+                "exit": "Take exit {exit} on the left",


That is only true for right-hand driving countries. Ideally we'd have left-hand/right-hand driving country information here. This is ticketed upstream Project-OSRM/osrm-backend#2269, assuming it would also be exposed in the API responses.

For now, I would remove direction on ramps completely. (Take the ramp on the {left/right} -> Take the ramp)

freenerd · 2017-07-05T11:49:44Z

languages/translations/en.json

We'll want to add the en.json to transifex, do some translations, then run the transifex script to pull the updated translations down again.

freenerd · 2017-07-05T11:58:44Z

test/fixtures_test.js

+            destinations: 'Destination 1,Destination 2',
+            exits: '4A,4B'
+        });
+        checkOrWrite(step, `${basePath}_exit_destination`);


Given that we only implement exits for off ramp right now, wouldn't we be okay with only fixtures for off ramp for now?

Or should we expose it for other types as well? @daniel-j-h noted the following:

rotary/roundabout/roundabout_turn https://github.com/Project-OSRM/osrm-text-instructions/pull/116/files/da902d902e59137a819c349758698f90f03fa3ff#r125590005

continue https://github.com/Project-OSRM/osrm-text-instructions/pull/116/files/da902d902e59137a819c349758698f90f03fa3ff#r125588118

fork https://github.com/Project-OSRM/osrm-text-instructions/pull/116/files/da902d902e59137a819c349758698f90f03fa3ff#r125588740

notification https://github.com/Project-OSRM/osrm-text-instructions/pull/116/files/da902d902e59137a819c349758698f90f03fa3ff#r125589415 (i don't think we would actually need it here, since it's very uncommon and we only handle ferries right now, which likely don't need/have exit information)

I went with this approach because destination was the most obvious pattern to model this work off of. I also liked how the fixtures guarantee that exit information isn't being added to instructions where it shouldn't be. I'm open to other options, but seems like the additional fixtures are easy enough to maintain.

I also lean away from implementing this for other types and just stay focused on off ramps, which will be 99% of cases.

freenerd · 2017-07-05T12:02:21Z

test/fixtures/v5/fork/sharp_left_exit.json

+        "exits": "4A,4B"
+    },
+    "instructions": {
+        "de": "Scharf links abbiegen an der Gabelung auf Way Name",


Do you mean that fork + sharp left should never occur?

freenerd · 2017-07-05T12:17:03Z

test/fixtures/v5/roundabout/exit_exit.json

+        "exits": "4A,4B"
+    },
+    "instructions": {
+        "de": "In den Kreisverkehr fahren und erste Ausfahrt nehmen auf Way Name",


Moving to #117

freenerd · 2017-07-05T12:23:13Z

test/fixtures/v5/notification/left_exit.json

+    },
+    "instructions": {
+        "de": "Links weiterfahren auf Way Name",
+        "en": "Continue left onto Way Name",


The use case for cars here would be to have an exit on/off a ferry? Not sure if these will ever be well tagged.

freenerd · 2017-07-05T12:29:37Z

test/fixtures/v5/off_ramp/uturn_exit.json

+        "exits": "4A,4B"
+    },
+    "instructions": {
+        "de": "Rampe nehmen auf Way Name",


Moving to #118

willwhite · 2017-07-06T02:05:24Z

Thanks for the feedback. I'm going to merge this into master so the new strings loaded into Transifex and we can get the ball rolling on translation but let me know if there are followup changes.

1ec5 · 2017-07-06T06:57:10Z

Tracking a Swift port in Project-OSRM/osrm-text-instructions.swift#25.

1ec5 · 2017-07-07T04:51:24Z

languages/translations/en.json

@@ -254,7 +254,7 @@
                "name": "Take the ramp on the right onto {way_name}",
                "destination": "Take the ramp on the right towards {destination}",
                "exit": "Take exit {exit}",
-                "exit_destination": "Take exit {exit} towards {destination}"
+                "exit_destination": "Take exit {exit} on the right towards {destination}"


@willwhite, if we’re saying “on the right” in the presence of a destination, why not also say “on the right” in the absence of a destination?

I'd love to remove the direction from instructions, except when we have no way/destination/exit at all: Take the ramp on the right.

"Fixed" this by removing the left/right indication in #124.

freenerd · 2017-07-07T14:44:31Z

Released this as version v0.5.0 and pushed to osrm-frontend Project-OSRM/osrm-frontend#252

Here is a route with an exit instruction http://map.project-osrm.org/?z=16&center=36.090924%2C-95.999672&loc=36.089927%2C-95.993342&loc=36.088991%2C-96.004672&hl=en&alt=0

Note that this runs against the demo server with osrm-backend@master and we'll need to wait for osrm-backend@5.9.0 until this is available in a release.

Add support for exits.

da902d9

willwhite requested review from freenerd and daniel-j-h July 5, 2017 03:42

willwhite commented Jul 5, 2017

View reviewed changes

daniel-j-h requested changes Jul 5, 2017

View reviewed changes

MoKob reviewed Jul 5, 2017

View reviewed changes

freenerd requested changes Jul 5, 2017

View reviewed changes

Specify 'on the right'.

38c6630

willwhite merged commit 78254fd into master Jul 6, 2017

willwhite deleted the exits branch July 6, 2017 02:06

This was referenced Jul 6, 2017

Include exit number in off-ramp instructions Project-OSRM/osrm-text-instructions.swift#25

Closed

Port OSRM Text Instructions to v0.5.0 Project-OSRM/osrm-text-instructions.swift#26

Merged

freenerd mentioned this pull request Jul 6, 2017

Update German translation #122

Merged

1ec5 reviewed Jul 7, 2017

View reviewed changes

willwhite mentioned this pull request Jul 7, 2017

Remove left/right from exit announcement. #124

Closed

freenerd mentioned this pull request Jul 13, 2017

Add missing 'on the right' to exit instructions. #125

Merged

Add support for exits. #116

Add support for exits. #116

Conversation

willwhite commented Jul 5, 2017 • 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

daniel-j-h left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoKob left a 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

freenerd left a comment • 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

willwhite commented Jul 6, 2017 • edited Loading

1ec5 commented Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freenerd commented Jul 7, 2017

willwhite commented Jul 5, 2017 •

edited

Loading

freenerd left a comment •

edited

Loading

willwhite commented Jul 6, 2017 •

edited

Loading

1ec5 commented Jul 6, 2017 •

edited

Loading