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.

Sign up for GitHub

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

Fixed getting entry bearing for maneuver. #4353

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/engine/guidance/collapse_scenario_detection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace
{

// check bearings for u-turns.
// since bearings are wrapped around at 0 (we only support 0,360), we need to do some minor math to
// since bearings are wrapped around at 0 (we only support 0,360), we need to do some minor math to
// check if bearings `a` and `b` go in opposite directions. In general we accept some minor
// deviations for u-turns.
bool bearingsAreReversed(const double bearing_in, const double bearing_out)
Expand Down Expand Up @@ -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.

const auto exit_bearing = intersection.bearings[intersection.out];
return util::bearing::angleBetween(entry_bearing, exit_bearing);
};
Expand Down