Split EBG and guidance pipelines in EBGF #4674

oxidase · 2017-11-09T15:12:42Z

Issue

The PR is the implementation of #4664.

Tasklist

Split intersection shape generation stage from processor_stage
Duplicate EBG and guidance stage in one pipeline
Cleanup the EBG stage of guidance code, use only EBG-related processing of generate_edge
Cleanup the guidance stage of EBG code, use only guidance-related processing of generate_edge
Split EBG and guidance into two pipelines with duplicated intersection shape generation Check if needed in this PR
check regression / cucumber cases (see docs/testing.md)
review
adjust for comments

Requirements / Relations

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

oxidase · 2017-11-16T16:11:20Z

@ghoshkaj pushed the current branch state that is working, but almost stuck with the third point Cleanup the EBG stage of guidance code, use only EBG-related processing of generate_edge because it is nearly impossible in the current implementation. The problem is that turn and lane analysis code influences intersection connectivity, eg, removing

osrm-backend/src/extractor/edge_based_graph_factory.cpp

Line 716 in e874f65

turn_lane_handler.assignTurnLanes(incoming_edge, std::move(intersection));

will prevent running this line of code

osrm-backend/src/extractor/guidance/turn_lane_matcher.cpp

Line 231 in e874f65

intersection[u_turn].entry_allowed = true;

and EBG will miss a U-turn edge. Ideally intersections shape and connectivity must be analyzed before edges and turns generation, but atm intersection views can be mutated by the turn and lanes handlers during the generation phase. This makes the splitting of code extremely fragile with respect to indexing turns and consequences of the incorrect indexing can be visible only in post-processing.

The drawback of this PR without intersection connectivity pre-analysis is in increasing EBGF processing time without any facilitation in solving current open issues, like multiple via-way restrictions or U-turns penalization in segregated intersections.

TheMarex

Looks good to me, I just have some minor comments and nitpicks. 👍

TheMarex · 2018-01-11T14:36:34Z

include/engine/guidance/assemble_geometry.hpp

@@ -70,7 +70,7 @@ inline LegGeometry assembleGeometry(const datafacade::BaseDataFacade &facade,
        cumulative_distance += current_distance;

        // all changes to this check have to be matched with assemble_steps
-        if (path_point.turn_instruction.type != extractor::guidance::TurnType::NoTurn)
+        if (path_point.turn_instruction.type != osrm::guidance::TurnType::NoTurn)


Hrm this should not need a global namespace prefix.

using guidance::TurnType leads to an error "engine::guidance::TurnType is undefined type". To avoid ambiguity osrm::guidance::TurnType must be used here.

TheMarex · 2018-01-11T15:07:22Z

src/extractor/edge_based_graph_factory.cpp

                m_node_based_graph.GetOutDegree(intersection_node),
-                turn.instruction.IsUTurn(),
+                guidance::getTurnDirection(turn_angle) == guidance::DirectionModifier::UTurn,


Does this also match sharp turns or really just the 180 deg turns?

just 0 (360) degrees turns.

TheMarex · 2018-01-11T15:08:36Z

src/extractor/edge_based_graph_factory.cpp

-                buffer->nodes_processed =
-                    intersection_node_range.end() - intersection_node_range.begin();
-
-                // If we get fed a 0-length range for some reason, we can just return right away


Why could this be removed? Was it superfluous?

yes, intersection_node_range size will also be checked in the for loop pre-condition below

TheMarex · 2018-01-11T15:11:28Z

src/extractor/edge_based_graph_factory.cpp


+                // Copy data from local buffers into global EBG data
+                std::for_each(
+                    buffer->continuous_data.begin(), buffer->continuous_data.end(), transfer_data);


Is there a reason why the transfer_data lambda is not inlined here?

transfer_data is used in two places to copy buffer->continuous_data and delayed_data

TheMarex · 2018-01-15T23:53:06Z

include/extractor/extractor.hpp

+        const std::vector<TurnRestriction> &turn_restrictions,
+        const std::vector<ConditionalTurnRestriction> &conditional_turn_restrictions,
+        const util::NameTable &name_table,
+        LaneDescriptionMap lane_description_map,


Is this passed on purpose by value?

yes, lane_description_map is modified by the turn lane handler. Before lane_description_map was a non-const reference, now it is moved into the function scope, modified and serialized in ProcessGuidanceTurns

TheMarex · 2018-01-15T23:55:13Z

src/updater/updater.cpp

@@ -112,9 +114,6 @@ void checkWeightsConsistency(
    extractor::EdgeBasedNodeDataContainer node_data;
    extractor::files::readNodeData(config.GetPath(".osrm.ebg_nodes"), node_data);

-    extractor::TurnDataContainer turn_data;
-    extractor::files::readTurnData(config.GetPath(".osrm.edges"), turn_data);


Oh nice where is this simplification coming from?

it is a left over from some other refactoring step. turn_data is loaded, but not used

TheMarex · 2018-01-15T23:58:52Z

src/guidance/guidance_processing.cpp

+namespace guidance
+{
+
+void processGuidanceTurns(const util::NodeBasedDynamicGraph &node_based_graph,


Nitpick on naming here: What we are doing is annotation the turn with guidance information, so for example something like guidance::annotateTurns might be sufficient as a name.

TheMarex · 2018-01-16T00:02:22Z

include/guidance/turn_data_container.hpp

+    Vector<guidance::TurnInstruction> turn_instructions;
+    Vector<LaneDataID> lane_data_ids;
+    Vector<EntryClassID> entry_class_ids;
+    Vector<util::guidance::TurnBearing> pre_turn_bearings;


Just a nitpick: We should probably move TurnBearing to the guidance module/namespace.

moved to include/guidance/turn_bearing.hpp

Intersection analysis occupy in osrm::extractor::intersection namespace and guidance code osrm::guidance

oxidase changed the title ~~Refactor/ebgf guidance~~ Split EDG and guidance pipelines in EBGF Nov 9, 2017

miccolis assigned oxidase Nov 9, 2017

ghoshkaj self-assigned this Nov 9, 2017

oxidase added Experimental - Do not merge Guidance Work In Progress labels Nov 9, 2017

ghoshkaj changed the title ~~Split EDG and guidance pipelines in EBGF~~ Split EBG and guidance pipelines in EBGF Nov 10, 2017

ghoshkaj force-pushed the refactor/ebgf-guidance branch 3 times, most recently from d72c8e1 to e28c37e Compare November 10, 2017 19:55

miccolis unassigned ghoshkaj Nov 14, 2017

oxidase force-pushed the refactor/ebgf-guidance branch 3 times, most recently from 40344a0 to e874f65 Compare November 16, 2017 15:32

oxidase mentioned this pull request Nov 27, 2017

Fixed order of generated edges and guidance turns #4706

Merged

10 tasks

oxidase force-pushed the refactor/ebgf-guidance branch 5 times, most recently from a2872db to b5390a4 Compare December 7, 2017 15:25

oxidase force-pushed the refactor/ebgf-guidance branch 5 times, most recently from a83bfa3 to 16dde94 Compare January 10, 2018 21:30

oxidase added Review and removed Work In Progress labels Jan 11, 2018

oxidase force-pushed the refactor/ebgf-guidance branch from 16dde94 to 853d4b3 Compare January 11, 2018 11:55

TheMarex approved these changes Jan 16, 2018

View reviewed changes

oxidase force-pushed the refactor/ebgf-guidance branch from fe0215e to f5fa343 Compare January 16, 2018 12:28

oxidase mentioned this pull request Jan 18, 2018

Making the turn function more flexible #4789

Merged

6 tasks

oxidase force-pushed the refactor/ebgf-guidance branch 8 times, most recently from 6c6323f to 1cbfb43 Compare January 29, 2018 20:54

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

oxidase added Ready To Merge and removed Experimental - Do not merge labels Feb 1, 2018

TheMarex removed the Review - In feedback label Feb 2, 2018

oxidase force-pushed the refactor/ebgf-guidance branch from 68a500f to 9bf892f Compare February 2, 2018 15:09

oxidase and others added 9 commits February 2, 2018 10:54

Remove unnecessary loading of turns data in updater

b42a361

Duplicate edges and turns generation code

4084c31

comments

5a61e5c

remove TurnData from generate_edge function

7c14173

Refine logging output

db31b79

Move guidance pre-processing code into GUIDANCE library

c7c3e0a

Split intersection analysis and guidance code

397dae4

Intersection analysis occupy in osrm::extractor::intersection namespace and guidance code osrm::guidance

Move guidance turn generation out of EBGF

1c19f77

Add CRC checksums to EBG and turns data

2dc70a3

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

oxidase merged commit 9e93f19 into master Feb 2, 2018

chaupow added this to the 5.16.0 milestone Feb 5, 2018

DennisOSRM deleted the refactor/ebgf-guidance branch November 6, 2022 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split EBG and guidance pipelines in EBGF #4674

Split EBG and guidance pipelines in EBGF #4674

oxidase commented Nov 9, 2017 •

edited

Loading

oxidase commented Nov 16, 2017

TheMarex left a comment

TheMarex Jan 11, 2018

oxidase Jan 16, 2018

TheMarex Jan 11, 2018

oxidase Jan 16, 2018

TheMarex Jan 11, 2018

oxidase Jan 16, 2018

TheMarex Jan 11, 2018

oxidase Jan 16, 2018 •

edited

Loading

TheMarex Jan 15, 2018

oxidase Jan 16, 2018

TheMarex Jan 15, 2018

oxidase Jan 16, 2018

TheMarex Jan 15, 2018

TheMarex Jan 16, 2018

oxidase Jan 16, 2018

Split EBG and guidance pipelines in EBGF #4674

Split EBG and guidance pipelines in EBGF #4674

Conversation

oxidase commented Nov 9, 2017 • edited Loading

Issue

Tasklist

Requirements / Relations

oxidase commented Nov 16, 2017

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

oxidase Jan 16, 2018 • edited Loading

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 Nov 9, 2017 •

edited

Loading

oxidase Jan 16, 2018 •

edited

Loading