Obvious Turn Handling, code transfer from #4426 #4867

oxidase · 2018-02-09T16:25:58Z

Issue

PR #4426 has an initial implementation of the obvious turn detection restructuring.

This PR rebases parts of the branch https://github.com/Project-OSRM/osrm-backend/tree/fix/obvious-2
0001-Don-t-show-time-by-default-in-osrm-runner.patch.txt
to the current master:

obvious-turn-discovery.feature
include/extractor/guidance/road_classification.hpp
findObviousTurn -> findObviousTurnNew
src/extractor/guidance/motorway_handler.cpp
src/extractor/guidance/turn_handler.cpp
src/engine/guidance/collapse_scenario_detection.cpp
src/extractor/scripting_environment_lua.cpp and changes in Lua scripts

Tasklist

Finish implementation of findObviousTurnNew -> no failing cucumber tests
Revisit cases, open points and related issues in Restructure Obvious Turn Handling #4426
review
adjust for comments
cherry pick to release branch

Additional cases to check

Make a slight right instruction at https://www.openstreetmap.org/node/568760458
Turn left instruction at https://www.openstreetmap.org/node/985525653

Requirements / Relations

Related #3105, #4426, #4414

oxidase · 2018-02-23T16:22:25Z

Finished initial code transfer from #4426 and prepared PR for the first review iteration

The PR extends RoadPriorityClass and adds priority groups.

findObviousTurn makes the following checks:

Detect the straight-most road that has the same or higher priority and does not require a name announcement. Return the road index if such road exists and the road is distinct from other roads
Detect the straight-most road that has the same or higher priority. Return the road index if such road exists and the road is distinct from other roads.
Detect the straight-most valid road. If such road does not exist then return no obvious turn.
Handle special cases

IsDistinctTurn returns true if an intersection has no roads similar to the candidate road.

For SF-area comparison of the branch with 5.16 release shows 2447 modified turns out of 812570 (0.3%)
http://bl.ocks.org/d/8ac513b7cfa8739726649628031093ad

TheMarex

Looks good! I have a few questions regarding some test cases and I think there might be some debugging code left.

TheMarex · 2018-02-28T12:10:15Z

features/guidance/collapse.feature

@@ -1010,7 +1010,7 @@ Feature: Collapse
            | f,j       | hohe,hohe             | depart,arrive               | f,j       |
            | a,t       | hohe,a100,a100        | depart,on ramp right,arrive | a,b,t     |
            | f,e       |                       |                             |           |
-            | q,j       | a100,hohe,hohe        | depart,turn right,arrive    | q,p,j     |
+            | q,j       | a100,hohe,hohe        | depart,turn right,arrive    | q,i,j     |


Did the turn location logic also change, or is this a side-effect of an obvious turn change?

location logic was not changed, but obvious turn changed at node p

TheMarex · 2018-02-28T12:12:22Z

features/guidance/continue.feature

@@ -108,7 +108,7 @@ Feature: Continue Instructions
       When I route I should get
            | waypoints | route       | turns                        |
            | a,c       | abc,abc,abc | depart,continue right,arrive |
-            | a,d       | abc,bd,bd   | depart,turn straight,arrive  |
+            | a,d       | abc,bd      | depart,arrive                |


I'm not sure if this expectation is right? I would expect a turn instruction since you are not following the street abc.

i have adjusted conditions. Now the expectation is again depart,turn straight,arrive.

TheMarex · 2018-02-28T12:16:37Z

features/guidance/merge-segregated-roads.feature

-            | a,d       | left,circle,circle,right,right  | true:90;false:90 true:120 false:270;true:60 true:180 false:300;true:90 false:240 true:270;true:270  |
-            | g,d       | bottom,circle,right,right       | true:0;true:60 false:180 false:300;true:90 false:240 true:270;true:270                              |
+            | waypoints | route              | intersections                                                                                      |
+            | a,d       | left,right,right   | true:90,false:90 true:120 false:270;true:60 true:180 false:300,true:90 false:240 true:270;true:270 |


The functional change here is that going straight is an obvious turn, even though the street name changes?

TheMarex · 2018-02-28T12:39:41Z

features/guidance/obvious-turn-discovery.feature

+
+
+  # https://www.openstreetmap.org/#map=19/37.63829/-122.46345
+  @todo


This one is marked as todo, maybe we should add a comment what is missing here?

i have reviewed new test cases and marked as @todo in the following cases:

tests fail due to geometry conditions compare_road_deviation_is_distinct and compare_road_deviation_is_slightly_distinct. I would keep ratios 2 and 1.4 in these conditions as was proposed in the original PR

additional post-processing logic is required to collapse instructions.

TheMarex · 2018-02-28T12:39:52Z

features/guidance/obvious-turn-discovery.feature

+
+
+  # https://www.openstreetmap.org/#map=18/48.99155/8.43520
+  @todo


TheMarex · 2018-03-01T10:57:55Z

include/guidance/intersection_handler.hpp

    // Find the most obvious turn to follow. The function returns an index into the intersection
    // determining whether there is a road that can be seen as obvious turn in the presence of many
    // other possible turns. The function will consider road categories and other inputs like the
    // turn angles.
    template <typename IntersectionType> // works with Intersection and IntersectionView
    std::size_t findObviousTurn(const EdgeID via_edge, const IntersectionType &intersection) const;

+    template <typename IntersectionType> // works with Intersection and IntersectionView
+    std::size_t findObviousTurnOld(const EdgeID via_edge,


These two declarations seem to be leftover from debugging?

TheMarex · 2018-03-01T11:30:51Z

include/guidance/intersection_handler.hpp

-    // indicate that a low priority road is obvious
-    if (IsLowPriority(best_option_edge) &&
-        best_option_edge.flags.road_classification != in_way_edge.flags.road_classification)
+    if (candidate_deviation < GROUP_ANGLE)


Nit pick: It might make sense to split this function into two sub-functions each for handling a case of this if block. The function is quite huge.

TheMarex · 2018-03-01T11:31:48Z

include/guidance/intersection_handler.hpp

-        return best_continue;
+    // Special case handling for roads splitting up, all the same name (exactly the same)
+    if (intersection.size() == 3 &&
+        std::all_of(intersection.begin(),


This function call should be moved out of the if head for readability.

TheMarex · 2018-03-01T11:33:34Z

profiles/lib/tags.lua

+        if data.is_forward_oneway then
+           forward = forward or common
+        end
+        if data.is_reverse_oneway then


Is this an added bug fix for the profile? It might make sense to capture that separately.

I found some bugs and will open a separate PR for review, namely here is a fix for double counting of lanes in case of oneways.

TheMarex · 2018-03-01T11:34:03Z

src/extractor/intersection/coordinate_extractor.cpp

@@ -185,7 +185,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     */
    const bool first_coordinate_is_far_away = [&first_distance, considered_lanes]() {
        const auto required_distance =
-            considered_lanes * 0.5 * ASSUMED_LANE_WIDTH + LOOKAHEAD_DISTANCE_WITHOUT_LANES;


Any background why we removed the magic parameter here?

i don't know exactly where 0.5 comes from, but assume that it is needed to reduce number of considered_lanes by factor 2 due to double counting.

oxidase · 2018-03-04T14:17:43Z

The current state of PR:

~1% turns are modified:
11 test case in features/guidance/obvious-turn-discovery.feature are commented out as to-do's.

oxidase added Guidance Work In Progress labels Feb 9, 2018

oxidase mentioned this pull request Feb 9, 2018

Restructure Obvious Turn Handling #4426

Closed

16 tasks

oxidase self-assigned this Feb 9, 2018

oxidase mentioned this pull request Feb 13, 2018

Confusing "Go straight" guidance instructions #4842

Closed

oxidase force-pushed the guidance/obvious-turn branch 7 times, most recently from c156b84 to c74ad95 Compare February 19, 2018 16:03

oxidase force-pushed the guidance/obvious-turn branch 4 times, most recently from 4c289ba to 22cdc06 Compare February 23, 2018 16:10

oxidase added Review and removed Work In Progress labels Feb 23, 2018

oxidase force-pushed the guidance/obvious-turn branch 5 times, most recently from 5177947 to 2ca9b11 Compare February 28, 2018 15:22

TheMarex requested changes Mar 1, 2018

View reviewed changes

oxidase mentioned this pull request Mar 2, 2018

Fixes in guidance #4929

Merged

6 tasks

oxidase force-pushed the guidance/obvious-turn branch 4 times, most recently from 3726dae to 82241ed Compare March 4, 2018 13:53

TheMarex added Review - In feedback and removed Review labels Mar 5, 2018

oxidase force-pushed the guidance/obvious-turn branch from 82241ed to 09b5564 Compare March 5, 2018 12:23

oxidase added 2 commits March 20, 2018 07:31

Restructure Obvious Turn Handling, code transfer from #4426

bc83714

Update tests expectations and obvious detection

edfca0c

oxidase force-pushed the guidance/obvious-turn branch from 09b5564 to 3f5ff84 Compare March 20, 2018 06:31

TheMarex approved these changes Mar 20, 2018

View reviewed changes

TheMarex added Ready To Merge and removed Review - In feedback labels Mar 20, 2018

Review updates

f4a9f1f

oxidase force-pushed the guidance/obvious-turn branch from 3f5ff84 to f4a9f1f Compare March 20, 2018 14:18

oxidase merged commit b56a757 into master Mar 20, 2018

oxidase deleted the guidance/obvious-turn branch March 20, 2018 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Obvious Turn Handling, code transfer from #4426 #4867

Obvious Turn Handling, code transfer from #4426 #4867

oxidase commented Feb 9, 2018 •

edited

Loading

oxidase commented Feb 23, 2018

TheMarex left a comment

TheMarex Feb 28, 2018

oxidase Mar 2, 2018

TheMarex Feb 28, 2018

oxidase Mar 2, 2018

TheMarex Feb 28, 2018

oxidase Mar 2, 2018

TheMarex Feb 28, 2018

oxidase Mar 2, 2018

TheMarex Feb 28, 2018

TheMarex Mar 1, 2018

TheMarex Mar 1, 2018

TheMarex Mar 1, 2018

TheMarex Mar 1, 2018

oxidase Mar 2, 2018

TheMarex Mar 1, 2018

oxidase Mar 2, 2018

oxidase commented Mar 4, 2018



		# https://www.openstreetmap.org/#map=19/37.63829/-122.46345
		@todo



		# https://www.openstreetmap.org/#map=18/48.99155/8.43520
		@todo

Obvious Turn Handling, code transfer from #4426 #4867

Obvious Turn Handling, code transfer from #4426 #4867

Conversation

oxidase commented Feb 9, 2018 • edited Loading

Issue

Tasklist

Additional cases to check

Requirements / Relations

oxidase commented Feb 23, 2018

TheMarex 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxidase commented Mar 4, 2018

oxidase commented Feb 9, 2018 •

edited

Loading