Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Respect initial bearings order #4443

Merged
merged 3 commits into from
Sep 18, 2017
Merged

Respect initial bearings order #4443

merged 3 commits into from
Sep 18, 2017

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented Aug 25, 2017 •
edited
Loading

Issue

PR fixes #4331.

For the sample intersection

way initial bearing extracted bearing
394198761 325.895 322.542
118426709 162.921 163.646
6323556 151.491 170.53

PR adds ordering by initial bearings and adjustment of bearings to keep the original order.
For the above example, bearings ordered in the initial order are 322.542, 170.53, 163.646 and after the adjustment are 322.542,170.53,171.03.

The adjustment angle is at most 0.5 degree or a half-angle between the road bearing and the base bearing.

Tasklist

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

@oxidase oxidase requested a review from MoKob August 25, 2017 10:14
@oxidase oxidase added the Review label Aug 30, 2017
@MoKob MoKob mentioned this pull request Sep 18, 2017
Closed
MoKob
MoKob approved these changes Sep 18, 2017
View reviewed changes
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.

In general this PR looks fine. I am a bit confused by the one comment though, for which it would be unclear to me what CW/CCW refer to. I think that this comment could benefit from some clarification.

src/extractor/guidance/intersection_generator.cpp Outdated
makeCompareShapeDataAngleToBearing(base_bearing));

// sort roads with respect to the initial bearings, tie-breaker for equal initial bearings
// is to order CCW road before CW one
Copy link

Choose a reason for hiding this comment

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

@oxidase personally I do not understand what wold be a cw road/ccw road. Could you clarify here a bit?

@oxidase oxidase force-pushed the guidance/respect_initial_bearings_order branch 2 times, most recently from c92c412 to 7370efc Compare September 18, 2017 12:22
@oxidase
Copy link
Contributor Author

oxidase commented Sep 18, 2017

@MoKob i have updated comment, we have to keep roads in the clockwise order with respect to "final" bearings if initial bearings are equal. This is the original behavior, that potentially should not be observed in real data, but in some tests where roads can overlap

features/guidance/perception.feature:76
features/guidance/roundabout-turn.feature:247
features/guidance/roundabout.feature:770

@oxidase oxidase force-pushed the guidance/respect_initial_bearings_order branch from 7370efc to 160ae66 Compare September 18, 2017 16:32
@oxidase oxidase merged commit fa1a4e8 into master Sep 18, 2017
@oxidase oxidase deleted the guidance/respect_initial_bearings_order branch September 18, 2017 19:33
@MoKob MoKob mentioned this pull request Oct 23, 2017
Open
@oxidase oxidase mentioned this pull request Oct 23, 2017
Closed
@oxidase oxidase mentioned this pull request Nov 25, 2017
Closed
karenzshea added a commit that referenced this pull request Nov 29, 2017
@karenzshea
revert #4443, use initial_bearing to sort intersection ways
f566f8a
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@MoKob MoKob MoKob approved these changes

Assignees
No one assigned
Labels
Bug Report Guidance Ready To Merge Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Turn angle calculation needs to respect initial road order
2 participants
@oxidase @MoKob