Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sliproads scenario 4 #4566

Merged
merged 2 commits into from
Oct 4, 2017
Merged

Fix sliproads scenario 4 #4566

merged 2 commits into from
Oct 4, 2017

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented Oct 2, 2017

Issue

PR fixes scenario 4 of #4348 by allowing sliproads from links via links.

Tasklist

  • add regression / cucumber cases (see docs/testing.md)
  • make a map of locations with modified instructions
  • review
  • adjust for comments

Requirements / Relations

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

@oxidase oxidase requested a review from MoKob October 2, 2017 17:16
@oxidase oxidase added this to the 5.13.0 milestone Oct 2, 2017
@daniel-j-h
Copy link
Member

Removing the link-class check here looks good but we really need to look at real-world situations to make a call here.

  • needs map to inspect situations we now classify as sliproads
  • needs update to our maneuver classification paper

@MoKob
Copy link

MoKob commented Oct 4, 2017

@oxidase before reviewing, could you provide the map of real-world situations that @daniel-j-h asked for. Without that, the implications of your changes are impossible to assess.
With that map, look at approximately hundred changes and make a note of improved / worsened classification (jugdement call: is this a single turn or is it two).

Copy link

@MoKob MoKob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of the changes I've seen on the map look fine. I am questioning whether allowing links in general as main road could be a bit too generous, though. This PR is mostly targeting ramps, right? Do you think we could achieve the same with just allowing motorway-links, but not all links?


// nor the second road coming from the intersection we shotcut must be links
// the second road coming from the intersection we shotcut must be a link
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*shortcut

const auto &sliproad_data = node_based_graph.GetEdgeData(sliproad.eid);
if (!sliproad_data.road_classification.IsLinkClass())
{
return true;
}

// otherwise neither the first road leading to the intersection we shortcut
const auto &first_road_data = node_based_graph.GetEdgeData(first.eid);
if (first_road_data.road_classification.IsLinkClass())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried what happens, if you explicitly allow MotorwayLink classes here? Ramps are different than the primary/.. links, right?

@oxidase oxidase force-pushed the guidance/sliproads-scenario4 branch from 1338d12 to ca065a3 Compare October 4, 2017 10:30
@oxidase
Copy link
Contributor Author

oxidase commented Oct 4, 2017

@MoKob I have updated the map with a condition first_road_data.road_classification.IsLinkClass() && !first_road_data.road_classification.IsRampClass() to use only ramp-> link -> non-link (red nodes) instead of any -> link -> non-link and number of affected intersections halved
https://api.mapbox.com/styles/v1/oxidase/cj8ctmb3z8il92qmnnpzkt3q2.html?title=true&access_token=pk.eyJ1Ijoib3hpZGFzZSIsImEiOiJjaXcxNWMxYmswMDRlMnlxOWtlem0yamU3In0.PjtHAtgQDzxhZ1BE484Sqw#18.27/49.46387/8.50602

A typical example is https://www.mapbox.com/get-directions/#18.27/49.46407/8.50528?coordinates=8.50620676926198,49.464227828009285;8.505281415385014,49.46406952432184
with a primary link -> primary link -> primary sliproad

@MoKob
Copy link

MoKob commented Oct 4, 2017

@oxidase in that case, we can keep it with the complete removal. It looks like we didn't regress anywhere (or at least in no location I've seen). Good to go from my point of view.

@oxidase oxidase force-pushed the guidance/sliproads-scenario4 branch 2 times, most recently from a402079 to 71916a4 Compare October 4, 2017 15:18
@oxidase oxidase force-pushed the guidance/sliproads-scenario4 branch from 71916a4 to 5e30b4a Compare October 4, 2017 15:20
@oxidase oxidase merged commit a900f52 into master Oct 4, 2017
@oxidase oxidase deleted the guidance/sliproads-scenario4 branch October 4, 2017 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants