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

Fixed getting entry bearing for maneuver. #4353

Merged
merged 1 commit into from
Aug 11, 2017
Merged

Conversation

vng
Copy link
Contributor

@vng vng commented Jul 28, 2017

To get entry bearing for maneuver, we need to reverse 'in' bearing for intersection, how it's made in other functions.

@@ -123,7 +123,7 @@ bool isStaggeredIntersection(const RouteStepIterator step_prior_to_intersection,

const auto angle = [](const RouteStep &step) {
const auto &intersection = step.intersections.front();
const auto entry_bearing = intersection.bearings[intersection.in];
const auto entry_bearing = util::bearing::reverse(intersection.bearings[intersection.in]);
Copy link

Choose a reason for hiding this comment

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

Can you elaborate a bit on how you verified that this is necessary?

Off the top of my head and without having checked again:

The bearings for an intersection could look like [0,90,180,270] while in = 0, out = 2.

This would represent a straight traversal on the north-south axis (note that the bearings are stored from the center of the intersection, so in is already reversed).

The angle between in and out in this representation should be 180 degree, which would be the case without reversing the bearing.
(see is_right: > 45 < 135).
With reversing, both bearings would be identical and there would be a turn angle of zero.

I might be totally off here, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'in' bearing in the intersection struct is stored opposite to the driving direction. You can ensure about it in the assembleSteps function here

'in' bearing from intersection also reversed here and here while passing to bearing::angleBetween.

But now I'm confused with the bearings::angleBetween function:

bearings::angleBetween(0, 0) == bearings::angleBetween(180, 180) == 180;
bearings::angleBetween(0, 180) == bearings::angleBetween(180, 0) == 0;

Seems like it's made specially for the opposite driving direction 'in' bearing. In that case, the logic in collapse_turns.cpp or post_processing.cpp should also be reviewed.

Copy link
Member

Choose a reason for hiding this comment

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

@MoKob @vng what are next actions here - can we ticket reviewing the post-processing bearing calculations to work on as a next step and move forward here?

Copy link

Choose a reason for hiding this comment

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

From the values @vng posted, the change should be fine. It is void, since is_right and is_left are used in combination (right + left), but it might prevent future bugs.

@MoKob MoKob added the Review label Aug 1, 2017
@MoKob MoKob added Refactor and removed Review labels Aug 11, 2017
@MoKob MoKob merged commit f34320a into Project-OSRM:master Aug 11, 2017
@danpat danpat added this to the 5.11.0 milestone Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants