Updated segregated intersection identification #4845

dgearhart · 2018-02-01T16:29:45Z

Issue

Mark the segregated intersections using the internal intersection logic similar to Valhalla

Tasklist

kdiluca

Looks great. We will still need to investigate roundabouts more though.

TheMarex

Thanks for taking a stab at this! There are some minor code style things that should be fixed. I'm not sure if you found scripts/format.sh but it might take care of a few of them.

It would be great if we could get some sort of map for SF/Berlin (because they are small) on what edges change classification with this PR. I think @oxidase wrote a tool for this? From my understanding it the classification seems more strict?

TheMarex · 2018-02-02T10:37:30Z

src/extractor/guidance/segregated_intersection_classification.cpp

@@ -203,15 +219,17 @@ std::unordered_set<EdgeID> findSegregatedNodes(const NodeBasedGraphFactory &fact
        return info;
    };

-    auto const isSegregatedFn = [&](auto const &edgeData,
+    auto const isSegregatedFn = [&](EdgeID edgeID,


We use under_score convention when naming variables. So that would be edge_id and edge_data.

TheMarex · 2018-02-02T10:50:05Z

src/extractor/guidance/segregated_intersection_classification.cpp

+                            double edgeLength) {
+        // Internal intersection edges must be short and cannot be a roundabout
+        // TODO adjust length as needed with lamda
+        if (edgeLength > 32.0f || current.flags.roundabout) {


The constant 32.0f should be captured in a named constexpr somewhere to make it easier to look up and change.

TheMarex · 2018-02-02T10:50:35Z

src/extractor/guidance/segregated_intersection_classification.cpp

+                            std::vector<EdgeInfo> v1,
+                            std::vector<EdgeInfo> v2,
+                            EdgeInfo const &current,
+                            double edgeLength) {


edgeLength -> edge_length

TheMarex · 2018-02-02T10:51:19Z

src/extractor/guidance/segregated_intersection_classification.cpp

+            return false;
+        }
+
+        // Print information about angles at node 1 from all inbound edge to the current edge


Outdated comment?

TheMarex · 2018-02-02T10:52:54Z

src/extractor/guidance/segregated_intersection_classification.cpp

+                auto const turn_degree = get_angle(edge_from.node, edge_inbound, current.edge);
+                // Skip if the inbound edge is not somewhat perpendicular to the current edge
+                // TODO add turn degree lamda
+                if (turn_degree > 150 && turn_degree < 210) {


I think there is a helper function for determining if a turn is a straight angle somewhere. It might make sense to use it here.

TheMarex · 2018-02-02T10:53:35Z

src/extractor/guidance/segregated_intersection_classification.cpp

+            return false;
+        }
+
+        // Print information about angles at node 2 from the current edge to all outbound edges


Comment seems outdated?

TheMarex · 2018-02-02T10:57:15Z

src/extractor/guidance/segregated_intersection_classification.cpp

+
+        // Print information about angles at node 1 from all inbound edge to the current edge
+        bool oneway_inbound = false;
+        for (auto const &edge_from : v1) {


Reading the two for loops, it seems like both for loops check the same things on different data. Maybe we could introduce a helper function that we could reuse here?

TheMarex · 2018-02-02T10:58:37Z

features/guidance/internal-intersections.feature

+          | waypoints | route                                                              | turns                        |
+          | a,k       | Broken Land Parkway,Broken Land Parkway                            | depart,arrive                |
+          | l,b       | Broken Land Parkway,Broken Land Parkway                            | depart,arrive                |
+#          | g,j       | Patuxent Woods Drive,Snowden River Parkway,Snowden River Parkway   | depart,continue,arrive       |


A comment column can be added by naming the column #. It would be nice to add a comment on why the specific test case was commented out.

oxidase

Looks nice! Are TODOs planned as follow-up tasks or will be implemented in this PR?

oxidase · 2018-02-02T12:50:48Z

src/extractor/guidance/segregated_intersection_classification.cpp

    NodeID node;

    util::StringView name;

+    bool reversed;
+
    // 0 - outgoing (forward), 1 - incoming (reverse), 2 - both outgoing and incoming
    int direction;


direction and a part of code at

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

Lines 197 to 211 in 8d468cd

// Merge equal infos with correct direction.

auto curr = info.begin();

auto next = curr;

while (++next != info.end())

{

if (curr->node == next->node)

{

BOOST_ASSERT(curr->name == next->name);

BOOST_ASSERT(curr->road_class == next->road_class);

BOOST_ASSERT(curr->direction != next->direction);

curr->direction = 2;

}

else

curr = next;

}

can be removed

oxidase · 2018-02-02T12:54:08Z

src/extractor/guidance/segregated_intersection_classification.cpp

+                // Get the turn degree from the current edge to the outbound edge
+                auto const turn_degree = get_angle(node1, current.edge, edge_to.edge);
+                // Skip if the outbound edge is not somewhat perpendicular to the current edge
+                // TODO add turn degree lamda


👍 to move the check to lambda and remove code duplication

kdiluca · 2018-02-06T15:04:44Z

@TheMarex @oxidase

@dgearhart and I are working on the lambda as well as the differ tool to show the before/after of the edge classifications in SF.

dgearhart · 2018-02-06T16:20:57Z

BEFORE: 4 missing segregated intersection edges

AFTER: The segregated intersection edges are now marked

TheMarex

Thanks for iterating on this. 👍 Looks much better! Good to merge from my side.

TheMarex · 2018-02-07T23:04:31Z

@dgearhart If you could rebase this on master and squash some commits, we can merge this tomorrow. 👍

dgearhart · 2018-02-08T13:32:20Z

@TheMarex thanks for the review and suggestions. We found a few issues when we reviewed the diffs - we would like to fix before merging.

kdiluca · 2018-02-23T15:47:20Z

This PR is ready for review for internal intersection identification.
I will be opening a new issue/PR to resolve a few turn angle adjustments that need to be made, along with additional testing to make sure nothing else is impacted and will use this PR as the baseline. The turn angle adjustments should fix cases like this: THIS IS A NON ISSUE NOW AFTER REMOVAL OF BREAKS.

The intention of the Turn Angle adjustment is to not identify an internal intersection on the road where Miraloma Dr becomes Portola Dr for example.

@oxidase

paired with @oxidase and @kdiluca TODO fix tests and add in new ones

Added/Updated constexpr names and values

…e opposite turn degrees than outgoing edges or if the outgoing edges have opposing turn degrees; also merged with master v5.16

…ction updates

Breaks in code were a mistake and caused a change in the internal intersection identification.

oxidase

Looks great! I have rebased the branch to master and prepared a map with removed (red) and added (green) internal edges.

Some cases with removed edges like Peralta Avenue can be revisited after implementing post process logic.

dgearhart · 2018-02-27T15:42:48Z

@kdiluca I thought you said the Portola Drive case was fixed with the latest code (prior to turn adjustments)

kdiluca · 2018-02-27T16:33:49Z

It is fixed. I just rechecked it and @oxidase is going to do the same. This is what I now see.

oxidase · 2018-02-27T19:25:05Z

@dgearhart @kdiluca I have updated the map with correct OSM data and added unchanged (blue) edges:

Added CHANGED #4845: Updated segregated intersection identification to Unreleased

dgearhart · 2018-04-12T18:00:54Z

Before - production 5.16

After - staging 5.17

dgearhart requested review from TheMarex, oxidase and kdiluca February 1, 2018 16:29

kdiluca approved these changes Feb 1, 2018

View reviewed changes

TheMarex added the Review label Feb 2, 2018

TheMarex requested changes Feb 2, 2018

View reviewed changes

oxidase reviewed Feb 2, 2018

View reviewed changes

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

TheMarex assigned dgearhart Feb 2, 2018

chaupow added this to the 5.16.0 milestone Feb 5, 2018

kdiluca self-assigned this Feb 6, 2018

TheMarex approved these changes Feb 7, 2018

View reviewed changes

TheMarex added Ready To Merge and removed Review - In feedback labels Feb 7, 2018

TheMarex added Work In Progress and removed Ready To Merge labels Feb 8, 2018

TheMarex removed this from the 5.16.0 milestone Feb 10, 2018

dgearhart and others added 7 commits February 27, 2018 14:50

Initial internal intersection updates

c74205e

paired with @oxidase and @kdiluca TODO fix tests and add in new ones

Added Internal Intersection Model

7400b22

removed debug info

bc309ef

updates per PR 4845

eae9660

fixing build errors

d833e9d

fixing all compile errors

2028b3e

fixed EdgeID param

1fac1dc

dgearhart and others added 8 commits February 27, 2018 15:13

Added is_internal_straight lambda

8a2fae5

Added/Updated constexpr names and values

added rejection case turn degree logic

641628e

debug logging

1faa918

added turn angle logic to reject if there are incoming edges that hav…

47ef208

…e opposite turn degrees than outgoing edges or if the outgoing edges have opposing turn degrees; also merged with master v5.16

fixed formatting

6de4f4c

fix to decrease tile size based on latest turn angle internal interse…

921702a

…ction updates

Removed breaks

298bf1f

Breaks in code were a mistake and caused a change in the internal intersection identification.

Update segregated_intersection_classification.cpp

4ef649a

oxidase force-pushed the gdg_internal_intersection branch from eb8e81b to 4ef649a Compare February 27, 2018 14:19

oxidase approved these changes Feb 27, 2018

View reviewed changes

Update CHANGELOG.md

3175d1b

Added CHANGED #4845: Updated segregated intersection identification to Unreleased

kdiluca merged commit 33021d3 into master Feb 27, 2018

kdiluca removed the Work In Progress label Feb 27, 2018

oxidase deleted the gdg_internal_intersection branch February 28, 2018 08:48

javalver mentioned this pull request Mar 7, 2018

osrm-extract Error bad_alloc #4479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated segregated intersection identification #4845

Updated segregated intersection identification #4845

dgearhart commented Feb 1, 2018 •

edited by kdiluca

Loading

kdiluca left a comment

TheMarex left a comment

TheMarex Feb 2, 2018

kdiluca Feb 6, 2018

TheMarex Feb 2, 2018

kdiluca Feb 6, 2018

TheMarex Feb 2, 2018

kdiluca Feb 6, 2018

TheMarex Feb 2, 2018

kdiluca Feb 6, 2018

TheMarex Feb 2, 2018

TheMarex Feb 2, 2018

kdiluca Feb 6, 2018

TheMarex Feb 2, 2018 •

edited

Loading

TheMarex Feb 2, 2018

kdiluca Feb 6, 2018

oxidase left a comment

oxidase Feb 2, 2018

kdiluca Feb 6, 2018

oxidase Feb 2, 2018

kdiluca commented Feb 6, 2018 •

edited

Loading

dgearhart commented Feb 6, 2018

TheMarex left a comment

TheMarex commented Feb 7, 2018

dgearhart commented Feb 8, 2018

kdiluca commented Feb 23, 2018 •

edited

Loading

oxidase left a comment

dgearhart commented Feb 27, 2018

kdiluca commented Feb 27, 2018

oxidase commented Feb 27, 2018

dgearhart commented Apr 12, 2018

	// Merge equal infos with correct direction.
	auto curr = info.begin();
	auto next = curr;
	while (++next != info.end())
	{
	if (curr->node == next->node)
	{
	BOOST_ASSERT(curr->name == next->name);
	BOOST_ASSERT(curr->road_class == next->road_class);
	BOOST_ASSERT(curr->direction != next->direction);
	curr->direction = 2;
	}
	else
	curr = next;
	}

Updated segregated intersection identification #4845

Updated segregated intersection identification #4845

Conversation

dgearhart commented Feb 1, 2018 • edited by kdiluca Loading

Issue

Tasklist

kdiluca left a comment

Choose a reason for hiding this comment

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

TheMarex Feb 2, 2018 • edited Loading

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

kdiluca commented Feb 6, 2018 • edited Loading

dgearhart commented Feb 6, 2018

TheMarex left a comment

Choose a reason for hiding this comment

TheMarex commented Feb 7, 2018

dgearhart commented Feb 8, 2018

kdiluca commented Feb 23, 2018 • edited Loading

oxidase left a comment

Choose a reason for hiding this comment

dgearhart commented Feb 27, 2018

kdiluca commented Feb 27, 2018

oxidase commented Feb 27, 2018

dgearhart commented Apr 12, 2018

Before - production 5.16

After - staging 5.17

dgearhart commented Feb 1, 2018 •

edited by kdiluca

Loading

TheMarex Feb 2, 2018 •

edited

Loading

kdiluca commented Feb 6, 2018 •

edited

Loading

kdiluca commented Feb 23, 2018 •

edited

Loading