Refactor post-processing and add test for turn location #3495

MoKob · 2016-12-23T15:58:32Z

Issue

Works towards #3158 and #3293

Tasklist

remove all debug output
clean-up scenarios
review
adjust for comments

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

Guide:

The refactor fixes an issue that prevented new-names from being suppressed when they are shorter than 100 meters. This was in before, but didn't work apparently. Many tests are adjusted for this in one of two ways:
- added a given a grid size of 200 meters (possible if tests only rely on shape, not on lengths)
- removed additional road names from route field
I removed collapse considerations from lane processing. Lane processing should not handle collapse functionality. Collapse should run after lane processing and consider lanes to decide if collapse is possible.
many tests are given the locations check as well, especially for collapse scenarios
to avoid thresholds that are too large, many cases require lanes to check for road widths now

oxidase · 2017-01-02T14:38:37Z

src/engine/guidance/collapse_turns.cpp

+        // both angles are in the same direction, the total turn gets increased
+        //
+        // a ---- b
+        //           \


Not really a code change, but for collapse_turns.cpp:61:9: warning: multi-line comment [-Wcomment] it is possible to put a NO-BREAK SPACE character 0xA0 ' '. It will not be removed as a trailing white space, is invisible and suppresses the warning. The same for src/engine/guidance/post_processing.cpp:343:9: warning: multi-line comment [-Wcomment]

daniel-j-h · 2017-01-17T11:49:59Z

Note: test case for collapsing away signage data is here #3097 - could potentially be included here.

MoKob · 2017-01-26T15:44:52Z

features/guidance/turn.feature

-                  |   |'d._
-                  |   |    'e
+                  |   |' d._
+                  |   |     'e
                  j   k


I had the choice to try and hack something into the engine to possibly detect cases like this. Since the original situation works just fine (see screenshot), I opted to reduce the severity of this turn to ensure that I don't spent ages on unnecessary optimisation.

MoKob · 2017-01-27T09:22:49Z

features/bicycle/bridge.feature

-            | a    | g  | abc,cde,efg,efg | cycling,cycling,cycling,cycling | 5 km/h |
-            | b    | f  | abc,cde,efg,efg | cycling,cycling,cycling,cycling | 4 km/h |
+            | a    | g  | abc,cde,efg,efg | cycling,cycling,cycling,cycling | 6 km/h |
+            | b    | f  | abc,cde,efg,efg | cycling,cycling,cycling,cycling | 5 km/h |


longer ways -> better average speed

MoKob · 2017-01-27T09:23:11Z

features/car/bridge.feature

-            | c    | e  | cde,cde         | driving,driving                 | 2 km/h |
-            | e    | c  | cde,cde         | driving,driving                 | 2 km/h |
+            | from | to | route           | modes                           | speed   |
+            | a    | g  | abc,cde,efg,efg | driving,driving,driving,driving | 12 km/h |


longer ways -> less impact of turn penalties -> better speed

MoKob · 2017-01-27T09:23:58Z

features/guidance/merge-segregated-roads.feature

+            | restriction | fg       | gb     | g        | no_left_turn     |
+            | restriction | gb       | bc     | b        | no_left_turn     |
+            | restriction | cf       | fg     | f        | no_left_turn     |
+            | restriction | xb       | xf     | x        | only_straight_on |


changed paradigm to more easily give correct turn restrictions

MoKob · 2017-01-27T09:24:16Z

features/guidance/merge-segregated-roads.feature

-            | a,d       | horiz,horiz      | true:90,false:0 true:60 true:90 true:180 false:270,false:0 true:90 false:180 false:270 true:300;true:270									   |
-            | j,h       | vert,horiz,horiz | true:0;true:0 true:90 false:180 false:270 true:300,true:60 false:120 true:240 true:300,false:0 false:90 false:120 false:180 true:270;true:90  |
-            | j,l       | vert,vert        | true:0,true:0 true:90 false:180 false:270 true:300,true:0 false:90 false:180 true:240 false:270;true:180									   |
+            | waypoints | route            | intersections																																    |


all due to the now correct turn restrictions

TheMarex · 2017-02-07T09:31:48Z

Rebased to fix the merge conflicts caused by merging #3628 but getting the following build errors locally with GCC 6:

/home/patrick/Code/osrm-backend/src/engine/guidance/post_processing.cpp: In function ‘std::vector<osrm::engine::guidance::RouteStep> osrm::engine::guidance::postProcess(std::vector<osrm::engine::guidance::RouteStep>)’:
/home/patrick/Code/osrm-backend/src/engine/guidance/post_processing.cpp:261:38: error: reference to ‘TurnType’ is ambiguous
         else if (instruction.type == TurnType::StayOnRoundabout)
                                      ^~~~~~~~
/home/patrick/Code/osrm-backend/src/engine/guidance/post_processing.cpp:24:57: note: candidates are: namespace TurnType = osrm::extractor::guidance::osrm::extractor::guidance::TurnType;
 namespace TurnType = osrm::extractor::guidance::TurnType;

MoKob · 2017-02-08T15:47:16Z

include/extractor/guidance/sliproad_handler.hpp

@@ -65,6 +65,11 @@ class SliproadHandler final : public IntersectionHandler
                             const IntersectionViewData &first,
                             const IntersectionViewData &second) const;

+    // check if no mode changes are involved
+    bool allSameMode(const EdgeID in_road,


fixes a problem in sliproad handling, since we didn't check for this before

MoKob · 2017-02-14T08:10:00Z

src/engine/guidance/collapse_scenario_detection.cpp

+}
+
+// Just like a link step, but shorter and no name restrictions.
+bool isCollapsableSegment(const RouteStep &step)


Carrying over from #3705 (review), the name of this function should be made a bit clearer.

daniel-j-h · 2017-02-14T09:52:15Z

src/engine/guidance/collapse_scenario_detection.cpp

@@ -55,7 +55,7 @@ bool isLinkroad(const RouteStep &pre_link_step,
 }

 // Just like a link step, but shorter and no name restrictions.
-bool isCollapsableSegment(const RouteStep &step)
+bool isShortAndUndisturbed(const RouteStep &step)


TheMarex · 2017-02-23T21:39:47Z

@daniel-j-h @oxidase was this reviewed?

daniel-j-h · 2017-02-23T23:48:40Z

No.

oxidase

LGTM! But i have doubts in using distance measure to suppress names: 105 for a car is ~ 6-8 seconds, but walking it will be ~ 1-2 minutes. Would it be possible to use duration in reduce_verbosity_if_possible instead of NAME_SEGMENT_CUTOFF_LENGTH after #77?

start assigning turn locations to test / further locations add locations/make roads not overlapping - staggered larger grid size for utf tests (new name)

- moves collapse into a dedicated set of functions / files - make collapse scenarios distinct (slight performance cost) - reduce verbosity for short name segments (now actually working, was supposed to do so before)

MoKob · 2017-02-24T12:13:02Z

@oxidase sure. distance is not the best measure here. The full guidance system is only reasonable for car as of now. So we need a complete overhaul here.

MoKob added Guidance Refactor Work In Progress labels Dec 23, 2016

MoKob force-pushed the refactor/guidance/post-processing branch from 140de4e to dfe975d Compare January 2, 2017 13:38

oxidase reviewed Jan 2, 2017

View reviewed changes

MoKob force-pushed the refactor/guidance/post-processing branch from 3d0524b to 9df440f Compare January 3, 2017 16:58

MoKob force-pushed the refactor/guidance/post-processing branch 2 times, most recently from 84e6e8c to e3f179c Compare January 26, 2017 13:21

MoKob commented Jan 26, 2017

View reviewed changes

MoKob commented Jan 27, 2017

View reviewed changes

MoKob force-pushed the refactor/guidance/post-processing branch from 12e85b7 to 01f59ff Compare January 27, 2017 10:29

MoKob force-pushed the refactor/guidance/post-processing branch from 01f59ff to 91e822d Compare February 6, 2017 09:24

TheMarex changed the title ~~Refactor/guidance/post processing~~ Refactor post-processing and add test for turn location Feb 7, 2017

TheMarex force-pushed the refactor/guidance/post-processing branch from 4a528ab to abe2652 Compare February 7, 2017 09:30

MoKob force-pushed the refactor/guidance/post-processing branch from abe2652 to 98f45ab Compare February 7, 2017 09:48

MoKob commented Feb 8, 2017

View reviewed changes

MoKob force-pushed the refactor/guidance/post-processing branch 3 times, most recently from 643e63e to b36af4a Compare February 10, 2017 14:20

MoKob added Review and removed Work In Progress labels Feb 10, 2017

MoKob commented Feb 14, 2017

View reviewed changes

daniel-j-h reviewed Feb 14, 2017

View reviewed changes

MoKob force-pushed the refactor/guidance/post-processing branch 2 times, most recently from 14ba76d to 8dfecda Compare February 15, 2017 15:04

MoKob force-pushed the refactor/guidance/post-processing branch from 8dfecda to 524b107 Compare February 17, 2017 14:31

oxidase approved these changes Feb 24, 2017

View reviewed changes

Moritz Kobitzsch added 2 commits February 24, 2017 13:06

add locations to turn scenarios

0910a53

start assigning turn locations to test / further locations add locations/make roads not overlapping - staggered larger grid size for utf tests (new name)

refactor of post-processing

de82dee

- moves collapse into a dedicated set of functions / files - make collapse scenarios distinct (slight performance cost) - reduce verbosity for short name segments (now actually working, was supposed to do so before)

MoKob force-pushed the refactor/guidance/post-processing branch from 6684820 to de82dee Compare February 24, 2017 12:11

MoKob added Ready To Merge and removed Review labels Feb 24, 2017

remove suppressed new names from new tests

9d4467f

TheMarex merged commit 18a50d3 into master Feb 25, 2017

TheMarex deleted the refactor/guidance/post-processing branch February 25, 2017 12:17

daniel-j-h mentioned this pull request Mar 1, 2017

Demo Server Hit ElongateBy Same Mode Assertion #3762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor post-processing and add test for turn location #3495

Refactor post-processing and add test for turn location #3495

MoKob commented Dec 23, 2016 •

edited

Loading

oxidase Jan 2, 2017

daniel-j-h commented Jan 17, 2017

MoKob Jan 26, 2017 •

edited

Loading

MoKob Jan 27, 2017

MoKob Jan 27, 2017

MoKob Jan 27, 2017 •

edited

Loading

MoKob Jan 27, 2017

TheMarex commented Feb 7, 2017

MoKob Feb 8, 2017

MoKob Feb 14, 2017

daniel-j-h Feb 14, 2017

TheMarex commented Feb 23, 2017

daniel-j-h commented Feb 23, 2017

oxidase left a comment

MoKob commented Feb 24, 2017

Refactor post-processing and add test for turn location #3495

Refactor post-processing and add test for turn location #3495

Conversation

MoKob commented Dec 23, 2016 • edited Loading

Issue

Tasklist

Requirements / Relations

Guide:

oxidase Jan 2, 2017

Choose a reason for hiding this comment

daniel-j-h commented Jan 17, 2017

MoKob Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

MoKob Jan 27, 2017

Choose a reason for hiding this comment

MoKob Jan 27, 2017

Choose a reason for hiding this comment

MoKob Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

MoKob Jan 27, 2017

Choose a reason for hiding this comment

TheMarex commented Feb 7, 2017

MoKob Feb 8, 2017

Choose a reason for hiding this comment

MoKob Feb 14, 2017

Choose a reason for hiding this comment

daniel-j-h Feb 14, 2017

Choose a reason for hiding this comment

TheMarex commented Feb 23, 2017

daniel-j-h commented Feb 23, 2017

oxidase left a comment

Choose a reason for hiding this comment

MoKob commented Feb 24, 2017

MoKob commented Dec 23, 2016 •

edited

Loading

MoKob Jan 26, 2017 •

edited

Loading

MoKob Jan 27, 2017 •

edited

Loading