Fixed order of generated edges and guidance turns #4706

oxidase · 2017-11-27T16:25:16Z

Issue

This step is required before implementation of #4674

The order of edges is fixed by the order of node-based graph nodes and adjacent intersection edges incoming_edges and outgoing_edges and the implicit intersection connectivity matrix that is specified by a free function isTurnAllowed.

Tasklist

Requirements / Relations

Blocks #4674

oxidase · 2017-11-29T16:31:12Z

For Berlin extract instructions changed at locations ( - instruction generated by master, and not by the branch, + instruction is generated by the branch and not by master):

+(lon:13.362239, lat:52.499917), (noturn), slight left
+(lon:13.389962, lat:52.509648), (noturn), straight
-(lon:13.416905, lat:52.547719), continue, left
+(lon:13.429998, lat:52.608707), (suppressed), straight
-(lon:13.533043, lat:52.489697), (noturn), slight right
-(lon:13.533043, lat:52.489697), (noturn), slight left
+(lon:13.533043, lat:52.489697), (noturn), straight
+(lon:13.579658, lat:52.459044), (suppressed), slight left

Still has to check missing continue, left instruction at https://www.openstreetmap.org/node/60103176, but that could related to a bug with spurious U-turns

oxidase · 2017-11-30T14:46:54Z

Compared generated turn instruction with respect to master (modifications are additions or deletions only):

BER 8 modifications in 401144 generated instructions
CA 186 modifications for 12151513 generated instructions
DE 583 modifications for
30656676 generated instructions

The number of modified instructions is vanishingly small ~0.002% and almost all locations are either suppressed or invalid turns.

There are some manually selected locations that show edge cases in bearings merging and must be fixed later:

TheMarex · 2017-12-01T11:30:18Z

include/extractor/guidance/statistics_handler.hpp

        }

        return intersection;
    }

  private:
    mutable std::mutex lock;
-    mutable std::unordered_map<TurnType::Enum, std::uint64_t> type_hist;
-    mutable std::unordered_map<DirectionModifier::Enum, std::uint64_t> modifier_hist;
+    mutable std::map<TurnType::Enum, std::uint64_t> type_hist;


Is there a particular reason we need to preserve the insertion order here? Just curious.

yes, map preserves order, so it is easier to do line-by-line comparison with diff

TheMarex · 2017-12-01T11:44:51Z

src/extractor/intersection/intersection_analysis.cpp

+    }
+
+    // Enforce ordering of outgoing edges
+    std::sort(result.begin(), result.end());


This sequence should be sorted by construction.

it depends on GetAdjacentEdgeRange implementation, but can add std::is_sorted assertion in both places

danpat · 2017-12-04T16:38:50Z

@oxidase minor - can you add a CHANGELOG entry for this? It'll help when reviewing changes for the next release.

TheMarex

Just a few smaller clarifications needed, otherwise looks good. Thanks for this refactor! 👍

TheMarex · 2017-12-05T11:19:50Z

include/extractor/guidance/statistics_handler.hpp

-
-            type_hist[type] += 1;
-            modifier_hist[modifier] += 1;
+            if (road.entry_allowed)


Good catch!

TheMarex · 2017-12-05T11:21:37Z

include/extractor/guidance/turn_lane_types.hpp

@@ -111,8 +113,7 @@ transformTurnLaneMapIntoArrays(const LaneDescriptionMap &turn_lane_map)
    //
    // turn lane offsets points into the locations of the turn_lane_masks array. We use a standard
    // adjacency array like structure to store the turn lane masks.
-    std::vector<std::uint32_t> turn_lane_offsets(turn_lane_map.data.size() +
-                                                 2); // empty ID + sentinel
+    std::vector<std::uint32_t> turn_lane_offsets(turn_lane_map.data.size() + 1); // + sentinel


Do you know why it was + 2 before?

probably it is for empty ID but an empty entry is already included into turn_lane_map.data

TheMarex · 2017-12-05T11:24:56Z

src/extractor/edge_based_graph_factory.cpp

@@ -419,22 +421,43 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges(

    TurnDataExternalContainer turn_data_container;

+    // TODO: add a MergableRoadDetector instance, to be deleted later


I don't fully understand this TODO, there is an instance of this class a few lines below.

it is better to keep MergableRoadDetector for now, but it also influences intersections connectivity via edges merging and creating U-turns

TheMarex · 2017-12-05T11:26:16Z

src/extractor/edge_based_graph_factory.cpp

+                    const auto &incoming_edges = intersection::getIncomingEdges(
+                        m_node_based_graph, node_at_center_of_intersection);
+                    const auto &outgoing_edges = intersection::getOutgoingEdges(
+                        m_node_based_graph, node_at_center_of_intersection);


node_at_center_of_intersection maybe we can use intersection_node or something less verbose?

TheMarex · 2017-12-05T11:28:17Z

src/extractor/edge_based_graph_factory.cpp

+                    const auto &outgoing_edges = intersection::getOutgoingEdges(
+                        m_node_based_graph, node_at_center_of_intersection);
+                    const auto &edge_geometries_and_merged_edges =
+                        intersection::getIntersectionGeometries(m_node_based_graph,


Maybe this would be more concise using std::tie()?

TheMarex · 2017-12-05T11:57:22Z

src/extractor/guidance/mergable_road_detector.cpp

@@ -165,8 +170,9 @@ bool MergableRoadDetector::EdgeDataSupportsMerge(
 bool MergableRoadDetector::IsTrafficLoop(const NodeID intersection_node,


Is this still needed? I thought we compress traffic lights now.

yes, it is needed to prevent merges of loops like https://www.openstreetmap.org/?mlat=43.6174&mlon=7.05839#map=19/43.61695/7.05820

TheMarex · 2017-12-05T12:22:23Z

src/extractor/intersection/intersection_analysis.cpp

+
+    for (const auto outgoing_edge : graph.GetAdjacentEdgeRange(intersection_node))
+    {
+        // TODO: to use TurnAnalysis all outgoing edges are required, to be uncommented later


I don't quite understand yet how we could skip reversed edges in the future. How will this be possible?

TheMarex · 2017-12-05T12:30:57Z

src/extractor/intersection/intersection_analysis.cpp

+            return true;
+
+        // Allow U-turn if the incoming edge has a U-turn lane
+        const auto &incoming_edge_annotation_id = graph.GetEdgeData(from.edge).annotation_data;


We should probably discuss if using the presence of a uturn lane is a good idea here, or if we should base this on country specific settings. Making the accessibility depend on turn lanes is just quite ugly.

In general, we should investigate how often this code path is even triggered - I have a feeling that most streets with a u-turn lane also have segregated ways for both directions (so it is not a "simple" uturn).

added a TODO line here, but i would keep it as it is now

TheMarex · 2017-12-05T12:38:09Z

unit_tests/extractor/intersection_analysis_tests.cpp

+using InputEdge = util::NodeBasedDynamicGraph::InputEdge;
+using Graph = util::NodeBasedDynamicGraph;
+
+BOOST_AUTO_TEST_CASE(simple_intersection_connectivity)


Nice! Good to see some unit tests for this code.

karenzshea

🙌

karenzshea · 2017-12-05T13:32:05Z

include/extractor/intersection/intersection_analysis.hpp

+                  const guidance::TurnLanesIndexedArray &turn_lanes_data,
+                  const IntersectionEdge &incoming_edge);
+
+IntersectionEdge skipDegreeTwoNodes(const util::NodeBasedDynamicGraph &graph,


Should we keep the explanatory comments from

osrm-backend/include/extractor/guidance/intersection_generator.hpp

Lines 67 to 75 in 353829a

// Graph Compression cannot compress every setting. For example any barrier/traffic light cannot

// be compressed. As a result, a simple road of the form `a ----- b` might end up as having an

// intermediate intersection, if there is a traffic light in between. If we want to look farther

// down a road, finding the next actual decision requires the look at multiple intersections.

// Here we follow the road until we either reach a dead end or find the next intersection with

// more than a single next road. This function skips over degree two nodes to find coorect input

// for GetConnectedRoads.

OSRM_ATTR_WARN_UNUSED

IntersectionGenerationParameters SkipDegreeTwoNodes(const NodeID starting_node,

? I think they're still applicable, and useful to have.

karenzshea · 2017-12-05T14:46:21Z

src/extractor/intersection/intersection_analysis.cpp

+        road.edge = graph.GetTarget(next_edge) == road.node ? next_edge + 1 : next_edge;
+        road.node = next_node;
+        next_node = graph.GetTarget(road.edge);
+    }


It looks like we lost a CanBeCompressed() check from

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

Line 270 in 353829a

if (!CanBeCompressed(node_based_graph.GetEdgeData(query_edge),

Do we not need this check anymore?

This check always returns true because of an assignment query_edge = next_edge; one line above and it was a comparison of two identical data structures.

oxidase · 2017-12-07T16:11:53Z

Checked modified instruction in SF https://gist.github.com/oxidase/f0492bd6ffff80a7f24a91a2f07a4602
There are 45 modified instructions in 11 unique places and most changes are new U-turn instructions due to 701ac2c that also fixes #4704.

Also ce17c78 changes distance_to_extract from 150 to 120 meters. This makes are_parallel check less dependent on roads divergence, like South Van Ness Avenue at https://www.openstreetmap.org/node/65299217

TheMarex · 2017-12-18T15:26:24Z

@oxidase heads up I rebased this on master for a pre-release.

oxidase · 2018-01-03T08:06:20Z

@TheMarex @daniel-j-h 5.14.3 release fixes assertions

For intersection at https://www.openstreetmap.org/node/65299217 `are_parallel` in MergableRoadDetector::HaveSameDirection is false for South Van Ness Avenue with 150 meters

oxidase · 2018-01-03T18:12:02Z

The commit 154ceb1 changes turn instructions at places on the diff map http://geojson.io/#id=gist:oxidase/221b0b27779fbb5ca662a35d699708bc&map=12/37.8198/-122.3956

oxidase added Experimental - Do not merge Guidance Refactor labels Nov 27, 2017

oxidase force-pushed the refactor/guidance branch 2 times, most recently from 3f65550 to e7efb5b Compare November 27, 2017 21:40

oxidase force-pushed the refactor/guidance branch 3 times, most recently from 3c51d08 to e509a7c Compare November 30, 2017 13:19

oxidase added the Review label Nov 30, 2017

oxidase force-pushed the refactor/guidance branch from e509a7c to a9f2011 Compare November 30, 2017 15:49

TheMarex reviewed Dec 1, 2017

View reviewed changes

oxidase force-pushed the refactor/guidance branch 2 times, most recently from 2f00908 to 18b058d Compare December 1, 2017 21:39

oxidase force-pushed the refactor/guidance branch 3 times, most recently from b3c8a5d to 2f47634 Compare December 5, 2017 11:39

TheMarex approved these changes Dec 5, 2017

View reviewed changes

karenzshea requested changes Dec 5, 2017

View reviewed changes

TheMarex added Review - In feedback and removed Experimental - Do not merge Review labels Dec 5, 2017

oxidase force-pushed the refactor/guidance branch 2 times, most recently from 80ae5c5 to 09585e0 Compare December 7, 2017 15:22

TheMarex force-pushed the refactor/guidance branch from 09585e0 to 9d2e57a Compare December 18, 2017 15:24

karenzshea approved these changes Jan 2, 2018

View reviewed changes

oxidase added 4 commits January 2, 2018 20:44

Don't fail if a registered printer has no name

b0238da

Print statistics only for allowed turns

2c13d55

Allow to specify empty bearings string in input parameters

fdf670d

Free functions for guidance intersections analysis

d959318

oxidase force-pushed the refactor/guidance branch 4 times, most recently from 52c0293 to 97f29d7 Compare January 2, 2018 21:05

oxidase added Ready To Merge and removed Review - In feedback labels Jan 3, 2018

oxidase added this to the 5.15.0 milestone Jan 3, 2018

oxidase and others added 11 commits January 3, 2018 17:05

Remove usage of IntersectionGenerator in EBGF

5b58aac

Remove GetConnectedRoads from IntersectionGenerator

5b5f662

Remove IntersectionGenerator

c1248d2

Move roads re-ordering to convertToIntersectionView

94827cb

Remove handling of U-turns on motorways

da66091

Adjust to review findings

b09730f

Add low precision intersection views back

93ace40

Still use low precision bearings

4c007ed

Reduce extraction distance to 120 meters

b1cf995

For intersection at https://www.openstreetmap.org/node/65299217 `are_parallel` in MergableRoadDetector::HaveSameDirection is false for South Van Ness Avenue with 150 meters

Make distance_between_roads symmetrical

f2d1a37

Bumped version to latest.4

d8185eb

oxidase force-pushed the refactor/guidance branch 2 times, most recently from a2140c2 to 154ceb1 Compare January 3, 2018 18:09

oxidase force-pushed the refactor/guidance branch from 154ceb1 to d8185eb Compare January 4, 2018 13:27

oxidase merged commit 6d801e7 into master Jan 5, 2018

oxidase deleted the refactor/guidance branch January 5, 2018 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed order of generated edges and guidance turns #4706

Fixed order of generated edges and guidance turns #4706

oxidase commented Nov 27, 2017 •

edited

Loading

oxidase commented Nov 29, 2017

oxidase commented Nov 30, 2017

TheMarex Dec 1, 2017

oxidase Dec 1, 2017

TheMarex Dec 1, 2017

oxidase Dec 1, 2017

danpat commented Dec 4, 2017

TheMarex left a comment

TheMarex Dec 5, 2017

TheMarex Dec 5, 2017

oxidase Dec 5, 2017

TheMarex Dec 5, 2017

oxidase Dec 5, 2017

TheMarex Dec 5, 2017

TheMarex Dec 5, 2017

TheMarex Dec 5, 2017

oxidase Dec 5, 2017

TheMarex Dec 5, 2017

TheMarex Dec 5, 2017

oxidase Dec 5, 2017

TheMarex Dec 5, 2017

karenzshea left a comment

karenzshea Dec 5, 2017

karenzshea Dec 5, 2017

oxidase Dec 5, 2017

oxidase commented Dec 7, 2017

TheMarex commented Dec 18, 2017

oxidase commented Jan 3, 2018

oxidase commented Jan 3, 2018

		@@ -419,22 +421,43 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges(

		TurnDataExternalContainer turn_data_container;

		// TODO: add a MergableRoadDetector instance, to be deleted later

		@@ -165,8 +170,9 @@ bool MergableRoadDetector::EdgeDataSupportsMerge(
		bool MergableRoadDetector::IsTrafficLoop(const NodeID intersection_node,

	// Graph Compression cannot compress every setting. For example any barrier/traffic light cannot
	// be compressed. As a result, a simple road of the form `a ----- b` might end up as having an
	// intermediate intersection, if there is a traffic light in between. If we want to look farther
	// down a road, finding the next actual decision requires the look at multiple intersections.
	// Here we follow the road until we either reach a dead end or find the next intersection with
	// more than a single next road. This function skips over degree two nodes to find coorect input
	// for GetConnectedRoads.
	OSRM_ATTR_WARN_UNUSED
	IntersectionGenerationParameters SkipDegreeTwoNodes(const NodeID starting_node,

Fixed order of generated edges and guidance turns #4706

Fixed order of generated edges and guidance turns #4706

Conversation

oxidase commented Nov 27, 2017 • edited Loading

Issue

Tasklist

Requirements / Relations

oxidase commented Nov 29, 2017

oxidase commented Nov 30, 2017

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 Dec 4, 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

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

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

oxidase commented Dec 7, 2017

TheMarex commented Dec 18, 2017

oxidase commented Jan 3, 2018

oxidase commented Jan 3, 2018

oxidase commented Nov 27, 2017 •

edited

Loading