Support manual instruction type override relations #4676

danpat · 2017-11-09T23:00:34Z

This is an experimental PR to tinker with the idea of data-driven instruction generation.

Rationale: sometimes, you want a specific instruction at a particular location, and the heuristics we're using just aren't fancy enough to make the right decision, or the OSM tagging scheme is incomplete for the structure of the map at that location and doesn't supply enough information to make a good decision about which instruction to issue.

This PR adds support for a new, totally made up relation we're calling a maneuver relation. They look like this:

<relation>
  <tag k="type" v="maneuver"/>
  <member type="way" ref="1234" role="from"/>
  <member type="way" ref="5678" role="to"/>
  <member type="node" ref="9999" role="via"/>
  <tag k="maneuver" v="turn"/>
  <tag k="direction" v="slight_right"/>
</relation>

The basic process is that after a route is found, we scan along the path and find maneuver relations that match (they pass through from, via, and to in that order). If a match is found, the type value for the via point is replaced with the value from the relation (same for the direction).

We will come up with a rough mapping of reasonable-looking type values to OSRM turn types - the intent is for the OSM data to be relatively routing-engine agnostic. The intent of this relation type is to be a hint to the navigation engine in locations where heuristics have trouble.

TODO:

Complete the RestrictionCompressor work so that it also updates the ManeuverOverride from/to IDs @karenzshea
Implement the lookup to find matching ManeuverOverrides in the EdgeBasedGraphFactory (currently stubbed out)
Serialize the ManeuverOverride list at the end of extraction @danpat
Fully implement the facade.GetOverridesThatStartAt method
Extend this so that from/to ways don't need to be directly connected **

Multi-via-way support

Fix relation compressor for multi-nodes
Verify which nodes are needed `
File saving for multi-nodes
Edge case test scenarios

** Do this bit after we have directly-connected overrides working

/cc @karenzshea

Tasklist

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

karenzshea · 2017-11-15T11:01:35Z

src/extractor/extraction_containers.cpp

+ }
+ };
+
+ const auto string_to_turn_direction = [](const std::string &direction_string) {


I think here we will also need to pass in the turn_string to be able to set a uturn direction modifier

karenzshea · 2018-01-04T11:38:40Z

@danpat Do you have some unpushed additions to the IO operations for reading/writing maneuver overrides? Locally and on every travis build I'm seeing a

/home/travis/build/Project-OSRM/osrm-backend/include/extractor/serialization.hpp:339:39:   required from here
/home/travis/build/Project-OSRM/osrm-backend/include/storage/io.hpp:101:9: error: static assertion failed: bytewise reading requires trivially copyable type

error, where it looks like we're missing the read methods for the ManeuverOverride type.

karenzshea

Also see the comment about the missing overrides io methods!

karenzshea · 2018-01-04T11:40:40Z

features/guidance/maneuver-tag.feature

+ # Testing disconnected via ways (first turn is only modified if you end up
+ # on a particular way)
+ # | h,a | J Street,C Street,A Street,A Street | depart,turn left,turn left,arrive |
+ # | h,e | J Street,C Street,B Street,B Street | depart,turn sharp left,turn left,arrive |


Should these tests pass? Should we add a test that makes sure the first turn is not modified when the ending street is not in the maneuver? e.g. j -> e should return depart,turn right,arrive

karenzshea · 2018-01-04T11:52:20Z

include/engine/api/route_api.hpp

@@ -101,6 +104,27 @@ class RouteAPI : public BaseAPI
 const std::vector<bool> &source_traversed_in_reverse,
 const std::vector<bool> &target_traversed_in_reverse) const
 {
+ for (const auto &a : unpacked_path_segments)


Should this logging be wrapped into debug output only?

karenzshea · 2018-01-04T12:09:20Z

include/engine/api/route_api.hpp

+ ++route_iter;
+ continue;
+ }
+ // Skip over duplicated EBNs in the step array


When would the EBNs in the step array be duplicated?

karenzshea · 2018-01-04T12:19:40Z

include/engine/api/route_api.hpp

+ const auto via_node_coords = BaseAPI::facade.GetCoordinateOfNode(
+ maneuver_relation.instruction_node);
+ // Find the step that has the instruction_node at the intersection point
+ auto step_to_update = std::find_if(


Thought: could we also store the offset of the instruction_node in the override from the first part of the sequence, and just count forward that many steps in the route sequence? If the current route steps sequence actually matches the override, it should be 1:1 right? The coordinate comparison should be an assert or a second check then.

🤔 This is a good idea and would definitely speed things up. We're also being wasteful here by checking every coordinate - only intersection coordinates should get checked.

karenzshea · 2018-01-04T13:49:10Z

include/engine/datafacade/contiguous_internalmem_datafacade.hpp

+ // Copy all the overrides into the results array.
+ // m_maneuver_overrides is sorted by node_sequence.front(),
+ // so we can stop searching after we get to a node > edge_based_node_id
+ for (const auto & override : m_maneuver_overrides)


Would it be any faster to do a std::equal_range search for the edge_based_node_id and just write all of those results?

Good catch - yeah, it would be faster, std::equal_range has sub-linear performance if the thing being searched is random-access (as opposed to just forward iterable).

The way we're comparing a member variable (override.start_node) doesn't fit with the std::equal_range pattern though, I'll see if I can re-jigger it to work.

karenzshea · 2018-01-04T13:52:02Z

include/engine/internal_route_result.hpp

- // id of via node of the turn
+ // from edge-based-node id
+ NodeID from_edge_based_node;
+ // the internal OSRM id of the OSM node id that is the via node of the turn


this comment is so sad and great at the same time

karenzshea · 2018-01-04T14:06:16Z

include/extractor/maneuver_override.hpp

+ // check if all parts of the restriction reference an actual node
+ bool Valid() const
+ {
+ return turn_sequence.size() >= 2 || std::none_of(turn_sequence.begin(),


I would expect that all of these || are &&'s. Isn't an override only valid if it has at least 2 turns, none of the nodes are SPECIAL_NODEID, and the direction and override types are valid?

@danpat I'm still unclear on whether or not this check is the right one. Should the logic here be inverted?

karenzshea · 2018-01-04T14:10:42Z

include/extractor/maneuver_override_relation_parser.hpp

+class ScriptingEnvironment;
+
+/**
+ * Parses the relations that represents turn restrictions.


I think these comments need to be updated a bit for the maneuver relations

karenzshea · 2018-01-04T17:33:59Z

src/extractor/extraction_containers.cpp

+ };
+
+ // wrapper function to handle distinction between conditional and unconditional turn
+ // restrictions


Is this comment unrelated?

oxidase

Looks good! Just small cleanup remarks before merging.

oxidase · 2018-02-01T15:35:50Z

src/extractor/extraction_containers.cpp

+ guidance::TurnType::Invalid,
+ guidance::DirectionModifier::MaxDirectionModifier};
+
+ std::clog << " ****** EXTERNAL WAY SEQUENCE " << std::endl;


could it be removed?

oxidase · 2018-02-01T16:23:21Z

include/engine/api/route_api.hpp

@@ -140,6 +168,156 @@ class RouteAPI : public BaseAPI
 reversed_source,
 reversed_target);

+ // Find overrides that match, and apply them


would it make sense to move the code block into a guidance handler function guidance::handleManeuverOverride?

oxidase · 2018-02-01T19:34:55Z

include/engine/guidance/route_step.hpp

@@ -78,6 +79,7 @@ struct RouteStep
 std::size_t geometry_end;
 std::vector<IntermediateIntersection> intersections;
 bool is_left_hand_driving;
+ bool is_overridden;


is_overridden is only set but never used, can it be removed?

oxidase · 2018-02-01T19:51:52Z

src/storage/storage.cpp

@@ -1074,6 +1086,22 @@ void Storage::PopulateData(const DataLayout &layout, char *memory_ptr)
 " in " + config.GetPath(".osrm.edges").string());
 }
 }
+
+ // load turn duration penalties


// load maneuver overrides?

- Changes from 5.15.2: - Guidance - ADDED Project-OSRM#4676: Support for maneuver override relation, allowing data-driven overrides for turn-by-turn instructions [Project-OSRM#4676](Project-OSRM#4676) - CHANGED Project-OSRM#4830: Announce reference change if names are empty - CHANGED Project-OSRM#4835: MAXIMAL_ALLOWED_SEPARATION_WIDTH increased to 12 meters - CHANGED Project-OSRM#4842: Lower priority links from a motorway now are used as motorway links [Project-OSRM#4842](Project-OSRM#4842) - CHANGED Project-OSRM#4895: Use ramp bifurcations as fork intersections [Project-OSRM#4895](Project-OSRM#4895) - CHANGED Project-OSRM#4893: Handle motorway forks with links as normal motorway intersections[Project-OSRM#4893](Project-OSRM#4893) - FIXED Project-OSRM#4905: Check required tags of `maneuver` relations [Project-OSRM#4905](Project-OSRM#4905) - Profile: - FIXED: `highway=service` will now be used for restricted access, `access=private` is still disabled for snapping. - ADDED Project-OSRM#4775: Exposes more information to the turn function, now being able to set turn weights with highway and access information of the turn as well as other roads at the intersection [Project-OSRM#4775](Project-OSRM#4775) - FIXED Project-OSRM#4763: Add support for non-numerical units in car profile for maxheight [Project-OSRM#4763](Project-OSRM#4763) - ADDED Project-OSRM#4872: Handling of `barrier=height_restrictor` nodes [Project-OSRM#4872](Project-OSRM#4872)

karenzshea assigned karenzshea and danpat and unassigned karenzshea Nov 10, 2017

karenzshea added the Work In Progress label Nov 10, 2017

danpat added the Experimental - Do not merge label Nov 11, 2017

karenzshea reviewed Nov 15, 2017

View reviewed changes

miccolis unassigned karenzshea Nov 17, 2017

danpat force-pushed the guidance/silver-bullet branch from 72220ed to 1be89bb Compare November 30, 2017 22:41

karenzshea requested changes Jan 4, 2018

View reviewed changes

danpat force-pushed the guidance/silver-bullet branch 2 times, most recently from 0c15d5d to 7944351 Compare January 17, 2018 00:22

danpat force-pushed the guidance/silver-bullet branch from 7944351 to df1ef2f Compare January 23, 2018 08:08

danpat changed the base branch from master to refactor/ebgf-guidance January 23, 2018 18:25

danpat added Review and removed Experimental - Do not merge Work In Progress labels Jan 24, 2018

oxidase force-pushed the refactor/ebgf-guidance branch 2 times, most recently from 5a3dcd7 to 8320f25 Compare January 24, 2018 12:14

danpat force-pushed the guidance/silver-bullet branch from 56dbaae to 6f1aa4e Compare January 24, 2018 17:36

oxidase force-pushed the refactor/ebgf-guidance branch 3 times, most recently from f2757e5 to 4577f29 Compare January 29, 2018 14:17

oxidase force-pushed the guidance/silver-bullet branch from 8ba682d to fe994f6 Compare January 29, 2018 17:13

oxidase force-pushed the refactor/ebgf-guidance branch 5 times, most recently from 50807b4 to 3d9d1aa Compare February 1, 2018 15:02

oxidase force-pushed the guidance/silver-bullet branch from fe994f6 to dc43288 Compare February 1, 2018 15:05

oxidase force-pushed the refactor/ebgf-guidance branch from 3d9d1aa to 68a500f Compare February 1, 2018 16:15

oxidase force-pushed the guidance/silver-bullet branch from dc43288 to edb6478 Compare February 1, 2018 16:17

oxidase approved these changes Feb 1, 2018

View reviewed changes

TheMarex added Review - In feedback and removed Review labels Feb 2, 2018

oxidase force-pushed the refactor/ebgf-guidance branch 2 times, most recently from 9bf892f to 2dc70a3 Compare February 2, 2018 16:03

chaupow added this to the 5.16.0 milestone Feb 5, 2018

danpat force-pushed the guidance/silver-bullet branch from edb6478 to 5b4c4d1 Compare February 8, 2018 20:23

danpat changed the base branch from refactor/ebgf-guidance to master February 9, 2018 08:15

danpat force-pushed the guidance/silver-bullet branch 2 times, most recently from 67d5bb6 to 54fe4be Compare February 9, 2018 17:53

Support maneuver relations

2ea559e

danpat force-pushed the guidance/silver-bullet branch from 54fe4be to 2ea559e Compare February 9, 2018 17:54

danpat merged commit 5531cac into master Feb 9, 2018

danpat added Ready To Merge and removed Review - In feedback labels Feb 9, 2018

1ec5 mentioned this pull request Mar 11, 2018

Allow maneuver relations to be spelled the British way #4950

Closed

DennisOSRM deleted the guidance/silver-bullet branch November 6, 2022 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support manual instruction type override relations #4676

Support manual instruction type override relations #4676

danpat commented Nov 9, 2017 •

edited

Loading

karenzshea Nov 15, 2017

karenzshea commented Jan 4, 2018

karenzshea left a comment

karenzshea Jan 4, 2018

karenzshea Jan 4, 2018

karenzshea Jan 4, 2018

karenzshea Jan 4, 2018

danpat Jan 23, 2018

karenzshea Jan 4, 2018

danpat Jan 24, 2018

karenzshea Jan 4, 2018

karenzshea Jan 4, 2018

karenzshea Feb 9, 2018

karenzshea Jan 4, 2018

karenzshea Jan 4, 2018

oxidase left a comment

oxidase Feb 1, 2018

oxidase Feb 1, 2018

oxidase Feb 1, 2018

oxidase Feb 1, 2018

Support manual instruction type override relations #4676

Support manual instruction type override relations #4676

Conversation

danpat commented Nov 9, 2017 • edited Loading

TODO:

Multi-via-way support

Tasklist

Choose a reason for hiding this comment

karenzshea commented Jan 4, 2018

karenzshea 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

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

danpat commented Nov 9, 2017 •

edited

Loading