use enter + exit for roundabout instructions #4358

MoKob · 2017-07-31T11:20:54Z

Issue

Work towards #3108.

It contains two main parts:

adding exit instructions for roundabouts
refactoring the processing on the way, as it becomes much easier with having both enter and exit (e.g. we do not have to consider mode-changes on exit)

The PR also removes a large set of duplicated tests. E.g. circular tests a lot of general roundabout behaviour. The only thing we need to test in addition to roundabouts is that the tag is recognised.
The same goes for the bicycle profile, as long as we can detect a single roundabout, we do not need to repeat all tests for car, as the test processes the exact same steps.

Things to discuss:

How to treat immediate exits like here. These do not offer two locations / steps to use for outputting multiple instructions. I see the following choices:
- output a single instruction of known type (either enter or exit roundabout)
- add a dedicated instruction immediately exit the roundabout
As a heads up, we have some cases that might require handling from the consumers. These were previously handled in OSRM. /cc @1ec5 @bsudekum
- only enter (the route ends in a roundabout) -- same as before
- only exit (the route starts in a roundabout) -- here we loose a roundabout
- the already mentioned immediate exit
do we want to only announce a single exit type (what name?) or dedicated exits for rotary/roundabout/roundabout turn?
do we want to have exit-nr on both enter/exit? I would argue that it might be adding more confusion than benefit when re-routing inside a roundabout
Further todos: at some point I would like to make the full thing consistent with on/off ramp and use enter roundabout instead of roundabout to be consistent with exit roundabout

Tasklist

add regression / cucumber cases (see docs/testing.md)
review
adjust for comments

Impact of this PR

Immediate exit from a rotary (or roundabout)

{
	waypoints: [{
			location: [
				13.348487,
				52.515122
			],
			hint: "EwEBgP___38DAAAACAAAALkAAAAAAAAAAwAAAAgAAAC5AAAAAAAAAMAAAACHrssAMlEhA3uuywAnUSEDBQAPAN5S6PA=",
			name: "Altonaer Straße"
		},
		{
			location: [
				13.348233,
				52.514443
			],
			hint: "rScAgP___38GAAAACQAAAAkAAABhAgAABgAAAAkAAAAJAAAAYQIAAMAAAACJrcsAi04hA4mtywCNTiEDAwAPAN5S6PA=",
			name: "Straße des 17. Juni"
		}
	],
	routes: [{
		distance: 131.1,
		duration: 17.5,
		weight_name: "routability",
		legs: [{
			distance: 131.1,
			steps: [{
					distance: 66.3,
					duration: 9.6,
					weight: 9.6,
					intersections: [{
						location: [
							13.348487,
							52.515122
						],
						bearings: [
							125
						],
						entry: [
							true
						],
						out: 0
					}],
					name: "Altonaer Straße",
					mode: "driving",
					maneuver: {
						type: "depart",
						location: [
							13.348487,
							52.515122
						],
						bearing_before: 0,
						bearing_after: 125
					},
					geometry: "ozo_IacnpABKBGBIDKHSJQLKHIXQ"
				},
				{
					distance: 64.9,
					duration: 7.9,
					weight: 7.9,
					intersections: [{
						location: [
							13.349093,
							52.514673
						],
						bearings: [
							15,
							195,
							240,
							330
						],
						entry: [
							false,
							true,
							true,
							false
						],
						in: 3,
						out: 2
					}],
					ref: "B 2; B 5",
					name: "Straße des 17. Juni",
					mode: "driving",
					maneuver: {
						modifier: "right",
						bearing_before: 153,
						exit: 1,
						bearing_after: 239,
						type: "exit rotary",
						location: [
							13.349093,
							52.514673
						]
					},
					geometry: "uwo_IyfnpADTHTBJHTFTBT?D@N?F@R"
				},
				{
					distance: 0,
					duration: 0,
					weight: 0,
					intersections: [{
						location: [
							13.348233,
							52.514443
						],
						bearings: [
							84
						],
						entry: [
							true
						],
						in: 0
					}],
					ref: "B 2; B 5",
					name: "Straße des 17. Juni",
					mode: "driving",
					maneuver: {
						type: "arrive",
						location: [
							13.348233,
							52.514443
						],
						bearing_before: 264,
						bearing_after: 0
					},
					geometry: "gvo_ImanpA"
				}
			],
			duration: 17.5,
			weight: 17.5,
			summary: "Altonaer Straße, Straße des 17. Juni"
		}],
		weight: 17.5
	}],
	code: "Ok"
}

Enter And Exit Rotary

{
	waypoints: [{
			location: [
				13.320369,
				52.513357
			],
			hint: "ZwgBgP___38CAAAAEQAAACsAAAAJAAAAAgAAABEAAAArAAAACQAAAKYAAACxQMsATUohA1dAywDcSSEDAwAPAEdw-qs=",
			name: "Otto-Suhr-Allee"
		},
		{
			location: [
				13.322632,
				52.513596
			],
			hint: "StwBgP___38FAAAACAAAAB0AAABZAAAABQAAAAgAAAAdAAAAWQAAAKYAAACIScsAPEshA5VJywA8SyEDBAAPAEdw-qs=",
			name: "Marchstraße"
		}
	],
	routes: [{
		distance: 443.8,
		weight_name: "routability",
		legs: [{
			distance: 443.8,
			steps: [{
					distance: 72,
					duration: 8.5,
					weight: 8.5,
					intersections: [{
						location: [
							13.320369,
							52.513357
						],
						bearings: [
							116
						],
						entry: [
							true
						],
						out: 0
					}],
					ref: "L 1000",
					name: "Otto-Suhr-Allee",
					mode: "driving",
					maneuver: {
						type: "depart",
						modifier: "right",
						bearing_before: 0,
						bearing_after: 116,
						location: [
							13.320369,
							52.513357
						]
					},
					geometry: "ooo_IishpALk@@KDOJYFIJGFCJAND"
				},
				{
					duration: 33.4,
					weight: 33.4,
					intersections: [{
							location: [
								13.32095,
								52.512905
							],
							bearings: [
								15,
								45,
								210
							],
							entry: [
								false,
								false,
								true
							],
							in: 0,
							out: 2
						},
						{
							location: [
								13.320866,
								52.512812
							],
							bearings: [
								30,
								195,
								225
							],
							entry: [
								false,
								true,
								true
							],
							in: 0,
							out: 1
						},
						{
							location: [
								13.320963,
								52.512264
							],
							bearings: [
								150,
								315,
								330
							],
							entry: [
								true,
								false,
								false
							],
							in: 2,
							out: 0
						},
						{
							location: [
								13.32143,
								52.512041
							],
							bearings: [
								105,
								120,
								285
							],
							entry: [
								true,
								true,
								false
							],
							in: 2,
							out: 0
						},
						{
							location: [
								13.322648,
								52.51218
							],
							bearings: [
								45,
								225,
								240
							],
							entry: [
								true,
								false,
								false
							],
							in: 2,
							out: 0
						},
						{
							location: [
								13.322855,
								52.512363
							],
							bearings: [
								15,
								45,
								210
							],
							entry: [
								true,
								true,
								false
							],
							in: 2,
							out: 0
						},
						{
							location: [
								13.322812,
								52.512902
							],
							bearings: [
								135,
								165,
								330
							],
							entry: [
								false,
								false,
								true
							],
							in: 1,
							out: 2
						}
					],
					rotary_name: "Ernst-Reuter-Platz",
					distance: 306.3,
					name: "Marchstraße",
					mode: "driving",
					maneuver: {
						modifier: "straight",
						bearing_before: 191,
						exit: 4,
						bearing_after: 210,
						type: "rotary",
						location: [
							13.32095,
							52.512905
						]
					},
					geometry: "ulo_I}vhpAHFHFTFJBH?NANCHEJGJIFKHMFMFSDUBKBU@U?U?U?[CSCWGa@G[IUEGGIIKKKMEOEKAK?I@OBKDOJEDGDIN"
				},
				{
					distance: 65.5,
					duration: 7.7,
					weight: 7.7,
					intersections: [{
						location: [
							13.322668,
							52.513023
						],
						bearings: [
							135,
							300,
							345
						],
						entry: [
							false,
							true,
							true
						],
						in: 0,
						out: 2
					}],
					name: "Marchstraße",
					mode: "driving",
					maneuver: {
						modifier: "slight right",
						bearing_before: 320,
						exit: 4,
						bearing_after: 341,
						type: "exit rotary",
						location: [
							13.322668,
							52.513023
						]
					},
					geometry: "kmo_IuaipA[PIBG@EAMAUGOCK?"
				},
				{
					distance: 0,
					duration: 0,
					weight: 0,
					intersections: [{
						location: [
							13.322632,
							52.513596
						],
						bearings: [
							182
						],
						entry: [
							true
						],
						in: 0
					}],
					name: "Marchstraße",
					mode: "driving",
					maneuver: {
						type: "arrive",
						location: [
							13.322632,
							52.513596
						],
						bearing_before: 2,
						bearing_after: 0
					},
					geometry: "_qo_ImaipA"
				}
			],
			duration: 49.6,
			weight: 49.6,
			summary: "Ernst-Reuter-Platz, Ernst-Reuter-Platz"
		}],
		duration: 49.6,
		weight: 49.6,
		geometry: "ooo_IishpALk@@KDOJYFIJGFCJANDHFHFTFJBH?NANCHEJGJIFKHMFMFSDUBKBU@U?U?U?[CSCWGa@G[IUEGGIIKKKMEOEKAK?I@OBKDOJEDGDIN[PIBG@EAMAUGOCK?"
	}],
	code: "Ok"
}

Only Exiting a Roundabout

{
	waypoints: [{
			location: [
				13.269083,
				52.40313
			],
			hint: "jyUBgP___38DAAAABAAAAAQAAAAEAAAAAwAAAAQAAAAEAAAABAAAAKYAAABbeMoAupsfA4l4ygDNmx8DAQAPAEdw-qs=",
			name: ""
		},
		{
			location: [
				13.267494,
				52.403333
			],
			hint: "px8BgDwFAoAGAAAACgAAANYAAAAoAAAABgAAAAoAAADWAAAAKAAAAKYAAAAmcsoAhZwfAyVyygCEnB8DBgAPAEdw-qs=",
			name: "Zeppelinufer"
		}
	],
	routes: [{
		distance: 114.4,
		duration: 10.5,
		weight_name: "routability",
		legs: [{
			distance: 114.4,
			steps: [{
					distance: 21.9,
					duration: 2.1,
					weight: 2.1,
					intersections: [{
							location: [
								13.269083,
								52.40313
							],
							bearings: [
								305
							],
							entry: [
								true
							],
							out: 0
						},
						{
							location: [
								13.269018,
								52.403161
							],
							bearings: [
								0,
								120,
								285
							],
							entry: [
								true,
								false,
								true
							],
							in: 1,
							out: 2
						},
						{
							location: [
								13.268915,
								52.403169
							],
							bearings: [
								0,
								90,
								255
							],
							entry: [
								false,
								false,
								true
							],
							in: 1,
							out: 2
						}
					],
					name: "",
					mode: "driving",
					maneuver: {
						type: "depart",
						location: [
							13.269083,
							52.40313
						],
						bearing_before: 0,
						bearing_after: 305
					},
					geometry: "s~y~Hwr~oACJAH?H@LDH"
				},
				{
					distance: 92.5,
					duration: 8.4,
					weight: 8.4,
					intersections: [{
						location: [
							13.268799,
							52.40313
						],
						bearings: [
							45,
							210,
							270
						],
						entry: [
							false,
							true,
							true
						],
						in: 0,
						out: 2
					}],
					name: "Zeppelinufer",
					destinations: "A 115, Potsdam, Stahnsdorf",
					mode: "driving",
					maneuver: {
						modifier: "right",
						bearing_before: 230,
						exit: 2,
						bearing_after: 274,
						type: "exit roundabout",
						location: [
							13.268799,
							52.40313
						]
					},
					geometry: "q~y~H_q~oAA\CpAEd@Gf@Kj@GZ"
				},
				{
					distance: 0,
					duration: 0,
					weight: 0,
					intersections: [{
						location: [
							13.267494,
							52.403333
						],
						bearings: [
							120
						],
						entry: [
							true
						],
						in: 0
					}],
					name: "Zeppelinufer",
					mode: "driving",
					maneuver: {
						type: "arrive",
						location: [
							13.267494,
							52.403333
						],
						bearing_before: 300,
						bearing_after: 0
					},
					geometry: "y_z~Hyh~oA"
				}
			],
			duration: 10.5,
			weight: 10.5,
			summary: "Zeppelinufer, Zeppelinufer"
		}],
		weight: 10.5
	}],
	code: "Ok"
}

oxidase · 2017-07-31T12:46:00Z

features/guidance/rotary.feature

+           | waypoints | route       | turns                           |
+           | b,d       | bcegb,cd,cd | depart,exit rotary right,arrive |
+           | b,f       | bcegb,ef,ef | depart,exit rotary right,arrive |
+           | b,h       | bcegb,gh,gh | depart,exit rotary right,arrive |


I am just confused here, but understand the current expectation.
Would it make sense to add intermediate roundabout steps as Continue and use number of such steps before Exit instruction as an exit number?

@oxidase this is a special case about us only exiting the roundabout. The exit nr is part of the actual instruction. The cucumber case simply does not repeat the exit, which is usually announced at the enter instruction.

Do you think we would benefit a lot, from exposing exit-nr in the exit-instruction as well?

In terms of announcements, I would expect something like exit now announced here, just for context.

I think so, it makes no sense to start on roundabout, but it is possible to make a re-routing request being inside a roundabout, so information about exit number will be very helpful.

@oxidase again, the exit number is part of the instruction. We can add it to cucumber as well. I would be careful, though, since the exit-number would change based on you driving in the roundabout.
So you may already have passed an exit before the route is in, you may have passed a second one before it is announced. So in case of re-routing on a roundabout, I would expect that exit now would actually be more helpful than take the 3rd exit, which by now might be the first.

oxidase

LGTM! Thanks for cleaning up tests. I have just small comments.

The discussion about exit numbers in exit instructions is up to you. Mainly it is the question do we want to have exit-nr on both enter/exit?, but as soon we return numbers it is better to check in tests to avoid regressions even if it will be removed later.

oxidase · 2017-07-31T13:01:33Z

src/engine/guidance/post_processing.cpp

+    const auto exit = std::count_if(begin, end, passes_exit_or_leaves_roundabout);
+
+    // removes all intermediate instructions, assigns names and exit numbers
+    BOOST_ASSERT(leavesRoundabout((end - 1)->maneuver.instruction));


i would alias end - 1 as last or back

oxidase · 2017-07-31T13:02:17Z

src/engine/guidance/post_processing.cpp

+            if (currently_on_roundabout)
+                currently_on_roundabout = !leavesRoundabout(step.maneuver.instruction);
+            return result;
+            ;


oxidase · 2017-07-31T14:11:57Z

include/engine/api/route_api.hpp

@@ -167,12 +167,13 @@ class RouteAPI : public BaseAPI
                 * the overall response consistent.
                 *
                 * ⚠ CAUTION: order of post-processing steps is important
-                 *    - postProcess must be called before collapseTurnInstructions that expects
+                 *    - handleRoundabouts must be called before collapseTurnInstructions that


This caution comment now can be removed

MoKob · 2017-07-31T14:25:48Z

@oxidase for the enter/exit discussions I asked @1ec5 for his input. @bsudekum maybe you also want to jump in here. What would you guys want for enter/exit instructions and associated data?

bsudekum · 2017-07-31T15:46:56Z

Ideally, I'd be looking for something like:

In 400 ft, enter the roundabout and take the 4th exit on Foo Street
Exit the roundabout onto Foo Street
Continue on Foo Street for 2 miles

MoKob · 2017-08-01T10:04:21Z

@bsudekum I was more looking for guidance on how you imagine the API response to look like.

We currently have:

roundabout/rotary/roundabout turn without an exit (if its the same location)
roundabout + exit roundabout
rotary + exit rotary
roundabout turn + exit roundabout turn
exit roundabout (without roundabout)
exit rotary (without rotary)
exit roundabout turn (without roundabout turn)

An alternative proposal would be:

exit (for all types, if we start on the roundabout/immediately exit)
roundabout + exit
rotary + exit
roundabout turn + exit

(exit to be named appropriately).
We can essentially do everything between these two extremes, but I'd love to know what the easiest thing would be for you all to work with.

Also up for discussion: what flags do you need on the exit.
E.g. should we always include exit-nr, in addition to the enter instruction, or only if there is no enter instruction?

danpat · 2017-08-01T23:01:13Z

One big blocker for this is that the new instruction type will break existing users of the OSRM v5.x API - they'll encounter the type: 'exit rotary' instruction and barf. This kind of implies that this change means we need to launch OSRM v6.x

What about just issuing a regular type: turn step at the roundabout exit point? This would allow clients to announce "take a slight right onto XXX st" from within the roundabout, which seems sensible, and would be compatible with existing users.

Where it would get confusing I think would be small roundabouts, where the sequence of instructions would look like:

Enter the roundabout and take the second exit onto Foo St. {type: roundabout, exit_number: 2}
Take a slight right onto Foo St. {type: turn, modifier: slight_right}

This seems like it would make sense as audible instructions (with (2) being announced inside the roundabout), but doesn't make a lot of sense as a sequence of written instructions.

I think the new instruction type is ideal, but the compatibility changes concern me greatly.

@MoKob @oxidase I don't see any discussion about the API compatibility of this change, have I missed something somewhere? Are there any pathological cases introduced by re-using the existing turn instruction?

MoKob · 2017-08-02T07:55:08Z

@danpat according to our API docs, issuing a exit rotary and turn are the same. According to https://github.com/Project-OSRM/osrm-backend/blob/master/docs/http.md#stepmaneuver-object:

A string indicating the type of maneuver. new identifiers might be introduced without API change Types unknown to the client should be handled like the turn type, the existence of correct modifier values is guranteed.

We require clients to fail gracefully back to turn on new identifiers. So I would strongly prefer adding a correct type.

MoKob · 2017-08-02T10:45:10Z

I've updated the PR with some example situations and API responses.

danpat · 2017-08-18T00:35:48Z

@MoKob Heads up - I've just re-based this on master and force-pushed.

emiltin · 2017-08-18T07:06:12Z

if the roundabout tests are not specific to the car of bicycle profiles, i think it would be better to place then under features/testbot, rather than in features/car.

MoKob · 2017-08-18T07:20:20Z

@emiltin they are specific in that they need to be addressed in terms of flags (which the profiles have to do). Thus having a test showing that roundabouts are handled is fine. Though testing the same information (e.g. does counting work for different scenarios) is independent of the profile paths. If you transfer the roundabout flag, you will see the same specific handling.

We could think about putting the main logic tests (e.g. rotary, roundabout turn... ) into the testbot profile and only ensure with basic tests that bike/car handle roundabouts correctly.

1ec5 · 2017-08-23T08:51:40Z

Sorry for the delayed response; still slogging through vacation e-mail. 🙂

only exit (the route starts in a roundabout) -- here we loose a roundabout

I think it’s OK to have exit roundabout without roundabout or rotary in this case. However, if there would’ve been anything useful in the rotary instruction, like the rotary’s name, I think that information should appear in the depart instruction.

do we want to only announce a single exit type (what name?) or dedicated exits for rotary/roundabout/roundabout turn?

roundabout turn instructions should be atomic. The point of this maneuver is to avoid language around exits, since the user would perceive the junction as a normal intersection with an island crammed in the middle.

MoKob added Guidance Refactor Work In Progress Discussion Review and removed Work In Progress labels Jul 31, 2017

MoKob requested review from oxidase and 1ec5 July 31, 2017 12:29

oxidase reviewed Jul 31, 2017

View reviewed changes

oxidase approved these changes Jul 31, 2017

View reviewed changes

danpat force-pushed the guidance/roundabout branch from eb86e78 to 314bf51 Compare August 18, 2017 00:34

MoKob added this to the 5.12.0 milestone Aug 18, 2017

1ec5 mentioned this pull request Aug 31, 2017

Do not overload user with info at roundabouts mapbox/mapbox-navigation-ios#561

Merged

TheMarex modified the milestones: 5.13.0, 5.12.0 Aug 31, 2017

daniel-j-h mentioned this pull request Aug 31, 2017

Modifier in roundabout turns in can be confusing Project-OSRM/osrm-text-instructions#140

Closed

1ec5 mentioned this pull request Sep 1, 2017

Roundabout icon assumes right-hand driving mapbox/mapbox-navigation-ios#575

Closed

danpat force-pushed the guidance/roundabout branch from 314bf51 to 9f8bdd3 Compare September 2, 2017 03:35

Moritz Kobitzsch and others added 2 commits September 3, 2017 01:24

Expose roundabout/rotary exit instructions as a new instruction type.

3e32d88

Modify test tile size - guidance has changed

8d5c2cf

danpat force-pushed the guidance/roundabout branch from 9553043 to 8d5c2cf Compare September 3, 2017 08:25

danpat added 2 commits September 5, 2017 11:45

Fix indenting

63c2fa5

Add docs for new roundabout exit instructions

2227371

danpat modified the milestones: 5.12.0, 5.13.0 Sep 5, 2017

danpat merged commit c2dc7e9 into master Sep 5, 2017

MoKob deleted the guidance/roundabout branch September 18, 2017 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use enter + exit for roundabout instructions #4358

use enter + exit for roundabout instructions #4358

MoKob commented Jul 31, 2017 •

edited

Loading

oxidase Jul 31, 2017

MoKob Jul 31, 2017 •

edited

Loading

oxidase Jul 31, 2017

MoKob Jul 31, 2017

oxidase left a comment

oxidase Jul 31, 2017

oxidase Jul 31, 2017

oxidase Jul 31, 2017

MoKob commented Jul 31, 2017

bsudekum commented Jul 31, 2017

MoKob commented Aug 1, 2017

danpat commented Aug 1, 2017

MoKob commented Aug 2, 2017 •

edited

Loading

MoKob commented Aug 2, 2017

danpat commented Aug 18, 2017

emiltin commented Aug 18, 2017

MoKob commented Aug 18, 2017

1ec5 commented Aug 23, 2017

use enter + exit for roundabout instructions #4358

use enter + exit for roundabout instructions #4358

Conversation

MoKob commented Jul 31, 2017 • edited Loading

Issue

Tasklist

Impact of this PR

Immediate exit from a rotary (or roundabout)

Enter And Exit Rotary

Only Exiting a Roundabout

oxidase Jul 31, 2017

Choose a reason for hiding this comment

MoKob Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

oxidase Jul 31, 2017

Choose a reason for hiding this comment

MoKob Jul 31, 2017

Choose a reason for hiding this comment

oxidase left a comment

Choose a reason for hiding this comment

oxidase Jul 31, 2017

Choose a reason for hiding this comment

oxidase Jul 31, 2017

Choose a reason for hiding this comment

oxidase Jul 31, 2017

Choose a reason for hiding this comment

MoKob commented Jul 31, 2017

bsudekum commented Jul 31, 2017

MoKob commented Aug 1, 2017

danpat commented Aug 1, 2017

MoKob commented Aug 2, 2017 • edited Loading

MoKob commented Aug 2, 2017

danpat commented Aug 18, 2017

emiltin commented Aug 18, 2017

MoKob commented Aug 18, 2017

1ec5 commented Aug 23, 2017

MoKob commented Jul 31, 2017 •

edited

Loading

MoKob Jul 31, 2017 •

edited

Loading

MoKob commented Aug 2, 2017 •

edited

Loading