Added post process logic to collapse segregated turn instructions #4925

dgearhart · 2018-02-28T15:27:34Z

Issue

Collapse turns at segregated intersections to simplify guidance

Tasklist

Before

After

Updated left turn road name

…eet - features/guidance/collapse.feature:79

dgearhart · 2018-03-01T21:08:53Z

@danpat @oxidase I updated several of the cucumber tests since when we collapse the segregated edges the transition point will be the same for right, left, or U turns. Seem reasonable?

… Intersection belongs to Second - features/guidance/collapse.feature:219

…, Two Segregated Roads, Intersection belongs to Second - features/guidance/collapse.feature:219

…s Belonging to Mixed Streets - Slight Angles (2) - features/guidance/collapse.feature:318

…uidance/collapse.feature:581

…rn-lanes.feature:1220

…guidance/turn-lanes.feature:936

…rn-lanes.feature:961

dgearhart · 2018-03-20T20:05:07Z

@oxidase @danpat Above are the remaining failures - could you please review code updates and possibly discuss/help with the remaining issues?
1) and 4) are issues because when the segregated step is collapsed the result is a slight turn. Might need more logic to know if it is the most straight and should be collapsed. Any other suggestions?
2) is an issue because the segregated identification code needs to handle the circular attribute like it handles the roundabout attribute. This will be fixed when the identification logic is updated.
3) I am not sure why the lane information is not correct on this test - any insights here?

…gated

oxidase

Looks good to me! I have some inline comments and there are still 5 failing test cases:

1) Scenario: Segregated Intersection, Cross Belonging to Mixed Streets - Slight Angles (2) - features/guidance/collapse.feature:318
Two turn instructions were suppressed in isNameOszillation branch that is not the case after merging instructions in collapseSegregatedTurnInstructions. I think it is ok to update expectations

     | a,d       | (-) first,first          | (-) depart,arrive                | (-) a,d       |
     | a,d       | (+) first,first,first    | (+) depart,turn left,arrive      | (+) a,b,d     |

For

     | a,d       | (-) first,first          | (-) depart,arrive                | (-) a,d       |
     | a,d       | (+) first,first,first    | (+) depart,turn left,arrive      | (+) a,b,d     |

the reason is in overwriting Suppressed instruction by Turn in SegregatedTurnStrategy,but i think it is also ok to update the expectation.

2) Scenario: Entering a segregated road - features/guidance/collapse.feature:363

     | g,c       | second,first,first  | (-) depart,end of road left,arrive | (-) g,b,c     |
     | g,c       | second,first,first  | (+) depart,turn left,arrive        | (+) g,e,c     |

This is similar to other end of road to turn changes that are related to PR #2587

3) Scenario: Crossing Segregated before a turn - features/guidance/obvious-turn-discovery.feature:1052

     | g    | f  | woll,scho,scho | (-) depart,continue sharp left,arrive |
     | g    | f  | woll,scho,scho | (+) depart,turn sharp left,arrive     |

The expectation can be updated to turn sharp left.

4) Scenario: Turn Lanes at Segregated Road - features/guidance/turn-lanes.feature:271

     | i,l       | cross,cross,cross | depart,continue uturn,arrive | (-) ;,left:true straight:false;left:true straight:false;left:false straight:true, |
     | i,l       | cross,cross,cross | depart,continue uturn,arrive | (+) ,left:true straight:false;left:true straight:false;left:true straight:false;left:false straight:true, |

The expectation for lanes can be updated to the new value.

5) Scenario: No Slight Right at Stralauer Strasse - features/guidance/turn.feature:1212

     | a,d       | (-) depart,arrive | (-) Stralauer Str,Holzmarktstr |
     | a,d       | (+) depart,turn slight right,arrive | (+) Stralauer Str,Holzmarktstr,Holzmarktstr |

The test explicitly checks instruction collapsing that happens in a sequence of buildIntersections and suppressShortNameSegments calls. I would suggest to update the test map as the real intersection has different geometry or remove the test as a duplicate of features/guidance/collapse.feature tests.

It is also possible to add an intersections column to Segregated Intersection, Cross Belonging to Single Street test in features/guidance/collapse.feature to check outgoing bearings after collapsing

       When I route I should get
         | waypoints | intersections        |
         | e,j       | true:90;false:0 true:90 true:180 false:270;true:0                                                                                                             |
         | e,h       | true:90,false:0 true:90 true:180 false:270,true:0 true:90 false:180 false:270;true:270                                                                        |
         | e,l        | true:90;false:0 true:90 true:180 false:270,true:0 true:90 false:180 false:270,true:0 false:90 false:180 true:270;true:180                                     |
         | e,d       | true:90;false:0 true:90 true:180 false:270,true:0 true:90 false:180 false:270,true:0 false:90 false:180 true:270,false:0 false:90 true:180 true:270;true:90   |

oxidase · 2018-03-23T21:31:47Z

features/guidance/turn-lanes.feature

-                |   c
-            a - b ' |
-                g   d
+                    f   e


The mini map was updated so there would be an intersection prior to the turn so end of road would be marked. I think before the end of road was being marked on a false intersection - since the turn is at the intersection

oxidase · 2018-03-24T10:21:31Z

features/guidance/divided-highways.feature

@@ -13,19 +13,23 @@ Feature: Divided road entry
            d-------e-----f
                    |
                    |
-                    g
+                i---g---j


The test modification changes result of previous_step.intersections.size() < MIN_END_OF_ROAD_INTERSECTIONS condition to false. But the PR modifies intersections clustering and respectively the value of previous_step.intersections.size() at node g now is 1 instead of 2, so it is ok to adjust expectation to turn instruction.

If end of road instructions are still required then the commit or PR #2587 must be revisited.

Same as above - intersection before the turn was added so the end of road would still be marked properly

oxidase · 2018-03-24T10:25:36Z

features/car/conditional_restrictions.feature

@@ -691,14 +691,14 @@ Feature: Car - Turn restrictions
        #    """
        Given the node locations
            | node | lat     | lon      |
-            | a    | 38.9113 | -77.0091 |


The change of locations does not seem necessary.

updated to match current map - it was producing odd results

oxidase · 2018-03-24T11:03:18Z

src/engine/guidance/collapse_turns.cpp

+void CombineSegregatedStepsStrategy::operator()(RouteStep &step_at_turn_location,
+                                                const RouteStep &transfer_from_step) const
+{
+    // TODO


Please add TODO description

oxidase · 2018-03-24T14:46:23Z

src/engine/guidance/collapse_turns.cpp

+
+    const auto is_straight_step = [](const RouteStep &step) {
+        return ((hasTurnType(step, TurnType::NewName) || hasTurnType(step, TurnType::Continue) ||
+                 hasTurnType(step, TurnType::Suppressed) || hasTurnType(step, TurnType::Turn)) &&


can be change to is_turn_step(step) || (....) as

const auto is_turn_step = [](const RouteStep &step) { return (hasTurnType(step, TurnType::Turn) || hasTurnType(step, TurnType::Continue) || hasTurnType(step, TurnType::NewName) || hasTurnType(step, TurnType::Suppressed)); }; const auto is_straight_step = [&is_turn_step](const RouteStep &step) { return is_turn_step(step) && (hasModifier(step, DirectionModifier::Straight) || hasModifier(step, DirectionModifier::SlightLeft) || hasModifier(step, DirectionModifier::SlightRight)); };

oxidase · 2018-03-24T15:30:33Z

src/engine/guidance/collapse_turns.cpp

+    else if (hasTurnType(step_at_turn_location, TurnType::Fork))
+    {
+        // Keep fork
+        setInstructionType(step_at_turn_location, TurnType::Fork);


step_at_turn_location has already TurnType::Fork
What will happen if hasTurnType(transfer_from_step, TurnType::Fork) is true?

oxidase · 2018-03-24T15:31:27Z

src/engine/guidance/collapse_turns.cpp

+        setInstructionType(step_at_turn_location, TurnType::EndOfRoad);
+
+        // Set modifier based on turn angle
+        setModifier(step_at_turn_location, turn_direction);


this duplicates setModifier on line 378

…into gdg_segregated_post_process

…d the expectation

Fixed tests

dgearhart · 2018-03-28T00:44:33Z

@oxidase i pushed up logic to handle the wider straight intersections - all of the tests now pass

Added #4925

…into gdg_segregated_post_process

…ect-OSRM/osrm-backend into gdg_segregated_post_process

oxidase

LGTM!

dgearhart · 2018-03-30T11:42:37Z

Thank you @oxidase for your advice! Everything passed

Added post process logic to collapse segregated turn instructions

7b74910

dgearhart requested review from danpat and oxidase February 28, 2018 15:27

format updates

1d3eac5

dgearhart changed the title ~~Added post process logic to collapse segregated turn instructions~~ [WIP] Added post process logic to collapse segregated turn instructions Feb 28, 2018

dgearhart added 2 commits February 28, 2018 16:07

Fixed coordinates to reflect reality

eea551d

Updated left turn road name

fixed coordinates to fix test

b56d669

TheMarex added the Work In Progress label Mar 1, 2018

TheMarex changed the title ~~[WIP] Added post process logic to collapse segregated turn instructions~~ Added post process logic to collapse segregated turn instructions Mar 1, 2018

dgearhart added 4 commits March 1, 2018 14:03

Skip last step when processing segregated steps

ea47f79

updated segregated turn test

9a11432

Updated segregated test

20f47e6

Updated test: Segregated Intersection, Cross Belonging to Correct Str…

482529e

…eet - features/guidance/collapse.feature:79

karenzshea assigned dgearhart Mar 13, 2018

dgearhart added 15 commits March 13, 2018 15:20

Fixed all but one for features/guidance/collapse.feature:124

e7c7f3e

Fixed Scenario: Partly Segregated Intersection, Two Segregated Roads,…

9c81b73

… Intersection belongs to Second - features/guidance/collapse.feature:219

Fixed 7 of th 9 failures for Scenario: Partly Segregated Intersection…

b72b9de

…, Two Segregated Roads, Intersection belongs to Second - features/guidance/collapse.feature:219

Fixed 7 of the 9 failures for Scenario: Segregated Intersection, Cros…

0bf22f8

…s Belonging to Mixed Streets - Slight Angles (2) - features/guidance/collapse.feature:318

Fixed Scenario: Segregated Intersection into Slight Turn - features/g…

e9c6116

…uidance/collapse.feature:581

Updated Scenario: U-turn after a traffic light - features/guidance/tu…

7d5e912

…rn-lanes.feature:1220

Updated how we combine segregated steps

f956ddd

Added test to Verify end of road left turn across divided roads

4690c93

Fixed divided highwat tests

8385f6a

Fixed test failure

c6b1d57

fixed Scenario: Partitioned turn, Slight Curve - maxspeed - features/…

07a2a2d

…guidance/turn-lanes.feature:936

Fixed Scenario: Partitioned turn, Slight Curve - features/guidance/tu…

06d11a0

…rn-lanes.feature:961

Added strategies to combine segrgated intersections

832b52d

Added setModifier alias for readability

99275c6

Added strategies to combine segrgated intersections

3e0084a

Format updates

6ad4d27

dgearhart added the Review label Mar 20, 2018

Fixes segregated indentification to not mark circular edge as segre…

492e2fa

…gated

TheMarex removed the Work In Progress label Mar 22, 2018

danpat added this to the 5.17.0 milestone Mar 23, 2018

oxidase requested changes Mar 24, 2018

View reviewed changes

dgearhart added 5 commits March 26, 2018 10:22

Merge branch 'master' of https://github.com/Project-OSRM/osrm-backend …

6578825

…into gdg_segregated_post_process

Added intersection prior to turn so we still call out end of road

6ed640c

updated expectation to be turn instead of continue

7027cb1

Confirmed with @oxidase that the lane information if correct - update…

acad9f1

…d the expectation

Added logic to handle wider straights

468bbec

Fixed tests

dgearhart and others added 5 commits March 27, 2018 20:58

Update CHANGELOG.md

6749f8b

Added #4925

Merge branch 'master' into gdg_segregated_post_process

fbbba61

Merge branch 'master' of https://github.com/Project-OSRM/osrm-backend …

498ee5e

…into gdg_segregated_post_process

Merge branch 'gdg_segregated_post_process' of https://github.com/Proj…

18e2ad3

…ect-OSRM/osrm-backend into gdg_segregated_post_process

Removed TODO

37f74ca

oxidase approved these changes Mar 28, 2018

View reviewed changes

Process straight step prior to wider straight step

2f341b1

dgearhart merged commit 8a63ad9 into master Mar 30, 2018

dgearhart deleted the gdg_segregated_post_process branch March 30, 2018 11:44

dgearhart removed the Review label Mar 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added post process logic to collapse segregated turn instructions #4925

Added post process logic to collapse segregated turn instructions #4925

dgearhart commented Feb 28, 2018 •

edited

Loading

dgearhart commented Mar 1, 2018

dgearhart commented Mar 20, 2018 •

edited

Loading

oxidase left a comment

oxidase Mar 23, 2018

dgearhart Mar 26, 2018

oxidase Mar 24, 2018

dgearhart Mar 28, 2018

oxidase Mar 24, 2018

dgearhart Mar 26, 2018

oxidase Mar 24, 2018

oxidase Mar 24, 2018

oxidase Mar 24, 2018

oxidase Mar 24, 2018

dgearhart commented Mar 28, 2018

oxidase left a comment

dgearhart commented Mar 30, 2018

Added post process logic to collapse segregated turn instructions #4925

Added post process logic to collapse segregated turn instructions #4925

Conversation

dgearhart commented Feb 28, 2018 • edited Loading

Issue

Tasklist

dgearhart commented Mar 1, 2018

dgearhart commented Mar 20, 2018 • edited Loading

oxidase 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

dgearhart commented Mar 28, 2018

oxidase left a comment

Choose a reason for hiding this comment

dgearhart commented Mar 30, 2018

dgearhart commented Feb 28, 2018 •

edited

Loading

dgearhart commented Mar 20, 2018 •

edited

Loading