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

make end of road non obvious #4411

Merged
merged 2 commits into from
Aug 18, 2017
Merged

make end of road non obvious #4411

merged 2 commits into from
Aug 18, 2017

Conversation

MoKob
Copy link

@MoKob MoKob commented Aug 15, 2017

Issue

When reaching a oneway street at the end of the road, we currently do not emit a turn instruction. Even though there is no choice, it can make the user feel uncertain whether he is doing the right thing. E.g. if a road is modelled as dual carriage-way and has no obvious separation, turning right instead of left might be the clearer choice, an announcement of this turn will avoid confusion.

This PR introduces exactly this behaviour, announcing turns onto oneway through streets at the end of the road.

It affects quite a few intersections, as can be seen on this map -- contains values for Germany and California.

I've looked at many of the turns and from what I can tell, the selection shows a promising set.
All of these locations show a turn where we previously wouldn't have announced anything and now would announce turn/continue right.

Tasklist

  • add regression / cucumber cases (see docs/testing.md)
  • review
  • adjust for comments

@@ -327,6 +327,42 @@ std::size_t IntersectionHandler::findObviousTurn(const EdgeID via_edge,
return false;
}();

// check whether we turn onto a oneway through street. These typically happen at the end of
Copy link
Member

Choose a reason for hiding this comment

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

This is getting a bit out of hand with all the inline lambdas etc.

Do you think we could refactor the intersection handler a bit to make it easier to follow the logic here?

Copy link
Author

Choose a reason for hiding this comment

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

we could, but that is not the scope of this PR

// check whether we turn onto a oneway through street. These typically happen at the end of
// roads and might not seem obvious, since it isn't always as visible that you cannot turn
// left/right. To be on the safe side, we announce these as non-obvious
const auto turns_onto_through_street = [&](const typename IntersectionType::value_type &road) {
Copy link
Member

Choose a reason for hiding this comment

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

arg can be const auto& (?)

// left/right. To be on the safe side, we announce these as non-obvious
const auto turns_onto_through_street = [&](const typename IntersectionType::value_type &road) {
const auto in_through_candidate =
intersection.FindClosestBearing(util::bearing::reverse(road.bearing));
Copy link
Member

Choose a reason for hiding this comment

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

Why reverse?

const auto same_class = in_data.road_classification == out_data.road_classification;

const auto in_deviation = angularDeviation(in_through_candidate->angle, STRAIGHT_ANGLE);
const auto out_deviaiton = angularDeviation(road.angle, STRAIGHT_ANGLE);
Copy link
Member

Choose a reason for hiding this comment

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

We seem to only use these further down, care to move them down to where we need them?

const bool is_oneway = !in_through_candidate->entry_allowed && road.entry_allowed;

const bool not_roundabout =
!(in_data.roundabout || in_data.circular || out_data.roundabout || out_data.circular);
Copy link
Member

Choose a reason for hiding this comment

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

Care to remind me about the difference between checking for the roundabout instruction and the roundabout data here?

Copy link
Author

Choose a reason for hiding this comment

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

roundabout instructions can only be checked after they have been assigned. We are in the process of assigning turn-types, so they cannot be checked.


// by asking for the same class, we ensure that we do not overrule obvious by road-class
// decisions
const auto same_class = in_data.road_classification == out_data.road_classification;
Copy link
Member

Choose a reason for hiding this comment

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

The implication here is a change in road class is always obvious already? Is this always the case? Where is it determined?

Copy link
Author

Choose a reason for hiding this comment

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

not necessarily, but we check for a through street. A street that changes class in between is probably not just a normal road but a different scenario

// consider it non-obvious. The threshold here requires a slight (60) vs sharp (120)
// degree variation, at lest (120/60 == 2)
return is_oneway && same_class && not_roundabout && not_low_priority &&
(in_deviation / out_deviaiton <= 2);
Copy link
Member

Choose a reason for hiding this comment

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

Can out deviation ever be zero?

Copy link
Author

Choose a reason for hiding this comment

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

good catch!

@MoKob MoKob force-pushed the fix/make-end-of-road-non-obvious branch 2 times, most recently from 889b89a to 2613ebb Compare August 16, 2017 07:04
@MoKob MoKob force-pushed the fix/make-end-of-road-non-obvious branch from 2613ebb to f1b9dc7 Compare August 18, 2017 12:48
@MoKob MoKob added this to the 5.12.0 milestone Aug 18, 2017
@MoKob
Copy link
Author

MoKob commented Aug 18, 2017

Capturing from voice: @daniel-j-h says 👍 on merge.

@MoKob MoKob merged commit f347efb into master Aug 18, 2017
@MoKob MoKob deleted the fix/make-end-of-road-non-obvious branch August 18, 2017 14:33
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.

None yet

2 participants