Suppress turns emitted while on ferry #3420

karenzshea · 2016-12-07T21:32:14Z

Issue

Fixes #3225

Tasklist

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

Requirements / Relations

cc @MoKob @daniel-j-h

emiltin · 2016-12-07T22:46:42Z

the same should probably apply to other types of routes, like trains.

TheMarex · 2016-12-07T23:17:42Z

Hm could we handle this in pre-processing instead of post-processing? Turning from a starting_point = false to another starting_point = false node should be a NoTurn.

/cc @MoKob

MoKob · 2016-12-08T09:29:54Z

@TheMarex you are correct. If we assume for ferries always to be staring_point == false. But we don't event need this assumption. We know the travel modes. So we can check TravelMode==Ferry in Preprocessing and not emit any instruction.

daniel-j-h · 2016-12-08T09:48:32Z

Sorry my mistake here. I implemented lane removal from roundabouts in post-processing and was under the impression @karenzshea could do the same quick fix there, too.

Now as @MoKob today told me in the context of #2705 is that suppressed + use lane could still happen. The easier and correct solution is to not emit them in pre-processing only.

@karenzshea take a look at the changes in https://github.com/Project-OSRM/osrm-backend and how @MoKob handles not collapsing inside roundabouts and not assigning lanes there.

Maybe this is a good start.

MoKob · 2016-12-09T08:35:02Z

src/extractor/guidance/intersection_handler.cpp

@@ -51,6 +51,11 @@ TurnType::Enum IntersectionHandler::findBasicTurnType(const EdgeID via_edge,
    const auto &in_data = node_based_graph.GetEdgeData(via_edge);
    const auto &out_data = node_based_graph.GetEdgeData(road.eid);

+    // suppress instructions on edges that the can't control anyway
+    if (!in_data.startpoint && !out_data.startpoint)


In my view, we shouldn't abuse is startpoint here (and below). We want to filter out Ferry turns. As already stated in the discussion, we should use the TravelMode Ferry to do so. Someone might decide that starting on other roads is invalid, but we know that jumping ship is not possible. If someone disables motorways as start points, we might still want to give instructions.

I disagree here. This would mean that you need to add explicit handling based on mode for every mode you want to support. That means the probability that we need to find and adjust this piece of code in the future is very high, which I think is undesirable and duplicates logic from the profile.

Using startingpoint uses a concept that we already expose in the profiles: Flagging ways that should be threaded as a single 'unit' from a user perspective. The name might be good or bad but this is effectively what this does. I don't see any scenario where we would reasonably emit an instruction between edges of this type. Any division between steps is done by the routing engine for technical reasons not user intend.

I feel this will be easier to use from a user perspective and offer less surprises then explicitly needing to cycle back every time a user finds a use-case for having ferry-like edges.

The problem I see here is that what we snap to and what we consider as connections in forms of a turn are two entirely different concepts to me. Ferries are special in terms of that they often share ways in OSM data but don't allow switching from one item to another in between.

If we want to specify that snapping to is equivalent to does not allow turning, then you would be right. But as long as we don't make a distinction here, using startingpoint would not capture the issue we see in ferries.

The concept we need are ways that share segments but are distinct (right now we don't handle these correctly in all cases, since we can right now actually switch ferries in between). Ferries will need specific handling anyhow. The main question for me here is: do we really want to tie snapping to turn instructions?

/cc @daniel-j-h @danpat for additional oppinions

MoKob · 2016-12-09T09:13:19Z

Just a heads up. I've added a small test

MoKob · 2016-12-09T11:54:04Z

To explain the reasoning behind my test-case:

I have the feeling that the two locations currently modified in this PR might not catch all ferry cases. In my view it would be easier to do a dedicated ferry handler in the beginning that checks for allFerry and immediately returns all NoTurns.

daniel-j-h · 2016-12-10T11:46:48Z

include/extractor/travel_mode.hpp

@@ -53,4 +55,7 @@ const constexpr osrm::extractor::TravelMode TRAVEL_MODE_RIVER_UP = 10;
 const constexpr osrm::extractor::TravelMode TRAVEL_MODE_RIVER_DOWN = 11;
 const constexpr osrm::extractor::TravelMode TRAVEL_MODE_ROUTE = 12;

+// travel modes for which navigation should be suppressed
+const std::array<osrm::extractor::TravelMode, 2> constexpr SUPPRESS_MODE_LIST = {{TRAVEL_MODE_TRAIN, TRAVEL_MODE_FERRY}};


The problem with using std::array here is the size has to be known at compile time. If you now add an additional mode you have to go through the code base and change all array<T, N> to array<T, N + 1>.

Yeah this could be solved using a type alias though. using SupressionModeArray = std::array<osrm::extractor::TravelMode, 2>;

MoKob · 2016-12-13T09:35:08Z

features/guidance/collapse-ferry.feature

+        Given the profile "car"
+        Given a grid size of 20 meters
+
+    Scenario: Collapse Steps While On Ferry


could you maybe directly add the ascii-art to make the tests more readable to begin with? (here and on the other tests)

all the tests are rendered in ASCII now

MoKob · 2016-12-13T09:38:51Z

features/guidance/collapse-ferry.feature

+            | waypoints | route              | turns                                      | modes                         |
+            | f,j       | land,sea,land,land | depart,notification right,turn left,arrive | driving,ferry,driving,driving |
+
+    Scenario: Collapse Steps While On Ferry


I believe we could remove this test. It looks like the only difference here to the test above is an additional coordinate on the ferry route, which should not influence any turn decisions.

MoKob · 2016-12-13T09:41:17Z

features/guidance/collapse-ferry.feature

+            | bc    | primary | ferry | pennydog-island-ferry |
+
+        When I route I should get
+            | waypoints | route                                                                           | turns                                                                | modes                               |


misaligned, also I miss my monkey island reference...

MoKob · 2016-12-13T09:42:48Z

include/extractor/guidance/intersection_handler.hpp

+                                const TravelMode &out_mode,
+                                const SuppressModeListT &SUPPRESS_MODE_LIST) const;
+
+    bool SuppressModeNavigation(const EdgeID &via_edge,


via_edge/connected are unintuitive to me. how about from_edge and to_edge? or in_edge and out_edge?

MoKob · 2016-12-13T09:47:05Z

src/extractor/guidance/intersection_handler.cpp

+{
+    const auto suppress_in_mode = std::find(begin(SUPPRESS_MODE_LIST), end(SUPPRESS_MODE_LIST), in_mode);
+    if (suppress_in_mode != end(SUPPRESS_MODE_LIST))
+    {


minor nitpick: the check for in_mode == out_mode is cheaper than the algorithm call. So doing it the other way around would be faster:

if( in_mode != out_mode ) return false; const auto suppress_in_mode = std::find(begin(SUPPRESS_MODE_LIST), end(SUPPRESS_MODE_LIST), in_mode); return (suppress_in_mode != SUPPRESS_MODE_LIST.end();

In addition: using end like this requires the inclusion of iterator and a using std::end.
In this case, it's probably easier to simply call SUPPRESS_MODE_LIST.end().

MoKob · 2016-12-13T09:50:32Z

src/extractor/guidance/intersection_handler.cpp

@@ -37,13 +38,41 @@ IntersectionHandler::IntersectionHandler(const util::NodeBasedDynamicGraph &node
 {
 }

+bool IntersectionHandler::SuppressModeNavigation(const EdgeID &via_edge,
+                                                 const EdgeID &connected,
+                                                 const SuppressModeListT &SUPPRESS_MODE_LIST) const


I believe it would be cleaner to not pass the mode list as a parameter.
Since you already define it globally in travel_mode.hpp, you are essentially shadowing the name.

Shadowing describes a situation where you use the same name (here SUPPRESS_MODE_LIST) for two variables that are available at the same time. (https://en.wikipedia.org/wiki/Variable_shadowing). Simply accessing the global variable would be cleaner, in my opinion, since I don't see a reason where we would require greater flexibility at some point.

MoKob · 2016-12-13T10:01:05Z

src/extractor/guidance/intersection_handler.cpp

 TurnType::Enum IntersectionHandler::findBasicTurnType(const EdgeID via_edge,
                                                      const ConnectedRoad &road) const
 {

    const auto &in_data = node_based_graph.GetEdgeData(via_edge);
    const auto &out_data = node_based_graph.GetEdgeData(road.eid);

+    const auto in_mode = node_based_graph.GetEdgeData(via_edge).travel_mode;
+    const auto out_mode = node_based_graph.GetEdgeData(road.eid).travel_mode;
+    if (SuppressModeNavigation(in_mode, out_mode, SUPPRESS_MODE_LIST))


As per #3420 (comment), I've added a test. Using the three fallbacks is not sufficient to suppress all turns in ferries. I've added a test that still emits a turn on a ferry.

To fully solve this, I'd propose a dedicated handler (see previous comment). The handler should check all pairs of roads for same travel mode (e.g std::all_of(intersection,SuppressModeNavigation(first_mode,road_mode)) to check if it is applicable and simply assign NoTurn to all turns in its operator().

danpat · 2016-12-14T01:09:43Z

src/extractor/guidance/suppress_mode_handler.cpp

+            return node_based_graph.GetEdgeData(way.eid).travel_mode == in_mode;
+        });
+
+    if (all_share_mode)


I'm not sure I understand the reasoning for only suppressing when all outgoing edges share the mode.

The scenario I'm thinking of is a multi-stop ferry journey like this:

http://map.project-osrm.org/?z=12&center=48.908511%2C-123.166180&loc=49.035392%2C-123.086014&loc=48.782211%2C-123.181801&hl=en&alt=0

Should we suppress the instruction at the stopover point? I'm kind of torn.

What you've got should work great for ferry route splits in the middle of the ocean though. 👍

Originally I'd been selectively suppressing only instructions onto ways that involved modes on the mode blacklist, but this failed a test added by @MoKob wherein there is an intersection with a new ferry line and a land-based way.

https://github.com/Project-OSRM/osrm-backend/pull/3420/files#diff-0bf26f655163cdd60bedcd61e763028dR35

The test is written such that the instruction at the stopover point is expected to be suppressed.

My thinking on this is that without a destination based instruction when the first ferry line is boarded, it's not exactly useful to emit an instruction at the "stopover" if there really is a stopover at all or just a change of direction? Do we have a way of knowing like in the test above whether or not a switch to a new ferry involves user action? I actually would kind of assume that switching to a new ferry line necessitates user action, but I defer to @MoKob who wrote the original test.

Ah, ok. I guess there's a larger discussion to be had around route relations, ferries/trains changing name, stopovers, etc, but I think the logic here handles the egregious cases that the original ticket identified. 👍 on this approach for now.

daniel-j-h · 2016-12-14T13:06:15Z

include/extractor/guidance/suppress_mode_handler.hpp

+namespace guidance
+{
+
+class SuppressModeHandler : public IntersectionHandler


I think you can make it SuppressModeHandler final : - all handler could be final except the base class IntersectionHandler

daniel-j-h · 2016-12-14T13:06:59Z

include/extractor/travel_mode.hpp

@@ -53,4 +56,7 @@ const constexpr osrm::extractor::TravelMode TRAVEL_MODE_RIVER_UP = 10;
 const constexpr osrm::extractor::TravelMode TRAVEL_MODE_RIVER_DOWN = 11;
 const constexpr osrm::extractor::TravelMode TRAVEL_MODE_ROUTE = 12;

+// travel modes for which navigation should be suppressed
+const osrm::extractor::SuppressModeListT constexpr SUPPRESS_MODE_LIST = {{TRAVEL_MODE_TRAIN, TRAVEL_MODE_FERRY}};
+


Should this be in the SuppressModeHandler then?

ohh good thinking, this doesn't need to be a global anymore

daniel-j-h · 2016-12-14T13:07:23Z

src/extractor/guidance/intersection_handler.cpp

@@ -44,6 +45,9 @@ TurnType::Enum IntersectionHandler::findBasicTurnType(const EdgeID via_edge,
    const auto &in_data = node_based_graph.GetEdgeData(via_edge);
    const auto &out_data = node_based_graph.GetEdgeData(road.eid);

+    const auto in_mode = node_based_graph.GetEdgeData(via_edge).travel_mode;
+    const auto out_mode = node_based_graph.GetEdgeData(road.eid).travel_mode;


Hm? Dead code?

daniel-j-h · 2016-12-14T13:08:56Z

src/extractor/guidance/suppress_mode_handler.cpp

+Intersection SuppressModeHandler::
+operator()(const NodeID /*nid*/, const EdgeID source_edge_id, Intersection intersection) const
+{
+    const auto in_mode = node_based_graph.GetEdgeData(intersection[0].eid).travel_mode;


This is the travel mode for the UTurn road?

daniel-j-h · 2016-12-14T13:09:20Z

src/extractor/guidance/suppress_mode_handler.cpp

+
+    // if all out ways share the same mode as the approach way, suppress all the turns
+    const auto all_share_mode =
+        std::all_of(begin(intersection) + 1, end(intersection), [this, &in_mode](ConnectedRoad way) {


const auto& road

daniel-j-h · 2016-12-14T13:10:06Z

src/extractor/guidance/suppress_mode_handler.cpp

+        std::for_each(begin(intersection) + 1, end(intersection), [&](ConnectedRoad &out_way) {
+            out_way.instruction = {TurnType::Suppressed, getTurnDirection(out_way.angle)};
+        });
+    }


If you

auto first = begin(intersection) + 1; auto last = end(intersection);

above you can re-use them here

daniel-j-h · 2016-12-14T13:11:40Z

src/extractor/guidance/turn_analysis.cpp

+    {
+        intersection = suppress_mode_handler(
+            node_prior_to_intersection, entering_via_edge, std::move(intersection));
+    }


Your handler should come first, since it could influence others.

Also if your handler fires you have to disable all other handlers. Otherwise it could be that e.g. your handler changes instructions to NoTurn and now the Sliproad Handler accidentally is getting confused with the NoTurn and classifies this as a Sliproad.

Make the handler come first and if it triggers skip all other handler.

no specific reason

daniel-j-h · 2016-12-21T10:48:52Z

Mason macOS build is failing due to std::abs issue already stumbled upon in #1000.

@springmeyer you looked into this back then, any ideas why it is failing only now on this branch and e.g. not on master? The file failing to compile is the same as on master after all.

Build is

https://travis-ci.org/Project-OSRM/osrm-backend/builds/183957985

and error is at

https://travis-ci.org/Project-OSRM/osrm-backend/jobs/183957988#L1179

oxidase · 2016-12-21T10:53:54Z

@daniel-j-h just rebase is needed. It was "fixed" by b1aced3#diff-4ff77436d81ab7e8c0c026734a2d79cdR6
in #3409

daniel-j-h · 2016-12-21T11:34:01Z

Rebased and refactored a bit. @MoKob could you have a final look over this.

Note: needs to be squashed before merging.

Taking this over - dismissing my own review here.

daniel-j-h · 2016-12-23T13:46:25Z

CHANGELOG.md

    - Bugfixes
      - Fix #3475 removed an invalid `exit` field from the `arrive` maneuver
+    - Guidance
+      - No longer emitting turns on ferries, if a ferry should use multiple docking locations


Hm @MoKob, I just wanted to add one but it looks like you already added a changelog entry here - what's missing then?

yeah. I just added one, after I said it in chat. Sorry for not updating you. Other than that, if you are OK with my last changes, it should be good to go.

karenzshea added this to the 5.6.0 milestone Dec 7, 2016

karenzshea requested a review from MoKob December 7, 2016 21:32

karenzshea added the Guidance label Dec 7, 2016

karenzshea removed the request for review from MoKob December 7, 2016 22:07

MoKob reviewed Dec 9, 2016

View reviewed changes

karenzshea force-pushed the fairy-collapse branch from cdfc27a to a06f343 Compare December 9, 2016 17:04

daniel-j-h reviewed Dec 10, 2016

View reviewed changes

karenzshea force-pushed the fairy-collapse branch from aa0359d to 2b2860b Compare December 12, 2016 13:25

karenzshea added the Review label Dec 12, 2016

MoKob previously requested changes Dec 13, 2016

View reviewed changes

MoKob added Work In Progress and removed Review labels Dec 13, 2016

karenzshea changed the title ~~Collapse turns emitted while on ferry~~ Suppress turns emitted while on ferry Dec 13, 2016

danpat reviewed Dec 14, 2016

View reviewed changes

daniel-j-h previously requested changes Dec 14, 2016

View reviewed changes

karenzshea added Review and removed Work In Progress labels Dec 14, 2016

daniel-j-h force-pushed the fairy-collapse branch from 8159a94 to 59edabc Compare December 21, 2016 11:31

daniel-j-h requested a review from MoKob December 21, 2016 11:34

daniel-j-h self-assigned this Dec 21, 2016

karenzshea and others added 2 commits December 23, 2016 14:20

do not emit turns on ferries

9ba507e

Refactors Karen's work a bit

c2b65c0

MoKob force-pushed the fairy-collapse branch from 59edabc to b8b0a64 Compare December 23, 2016 13:27

clean-up unused headers, use NoTurn for ferries instead of suppressed

6fe54a9

MoKob force-pushed the fairy-collapse branch from b8b0a64 to 6fe54a9 Compare December 23, 2016 13:33

daniel-j-h reviewed Dec 23, 2016

View reviewed changes

daniel-j-h added Ready To Merge and removed Review labels Dec 23, 2016

daniel-j-h merged commit b9b52cb into master Dec 23, 2016

TheMarex deleted the fairy-collapse branch December 23, 2016 15:34

Suppress turns emitted while on ferry #3420

Suppress turns emitted while on ferry #3420

Conversation

karenzshea commented Dec 7, 2016 • edited by daniel-j-h

Issue

Tasklist

Requirements / Relations

emiltin commented Dec 7, 2016 • edited

TheMarex commented Dec 7, 2016

MoKob commented Dec 8, 2016

daniel-j-h commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoKob commented Dec 9, 2016

MoKob commented Dec 9, 2016

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

danpat Dec 14, 2016 • edited

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

daniel-j-h commented Dec 21, 2016

oxidase commented Dec 21, 2016

daniel-j-h commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenzshea commented Dec 7, 2016 •

edited by daniel-j-h

emiltin commented Dec 7, 2016 •

edited

danpat Dec 14, 2016 •

edited