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/adjust angles #4630

Merged
merged 2 commits into from Oct 25, 2017
Merged

Fix/adjust angles #4630

merged 2 commits into from Oct 25, 2017

Conversation

MoKob
Copy link

@MoKob MoKob commented Oct 20, 2017

Issue

The result is best visualised by comparing the testcases in perception.feature.
This PR changes the behaviour of how we adjust angles at an intersection right before a merge.
It turns out that we can introduce worse turn-angles, if the straightest turn out of the intersection isn't improved.

This is a changed branched out from #4426

Tasklist

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

Requirements / Relations

A map for all affected locations can be found here. In all of these cases, the angle isn't messed with. It is hard to clearly visualise what happens, but the angles would reflect most turn angles better.

Copy link
Contributor

@oxidase oxidase left a comment

Choose a reason for hiding this comment

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

LGTM! But i found one weired thing that happens with the new test that is not related to the PR: bearing computation is not translational invariant. Just adding one node like

                a
          k     b
              / |
            c   d - e
            |   |
            |   |
            |   |
            |   |
            |   |
            |   |
            |   |
            |   |
            g   f

makes expectations completely different

@@ -105,6 +106,8 @@ class GeojsonLogger
std::lock_guard<std::mutex> guard(lock);
ofs.open(logfile, std::ios::binary);

ofs << std::setprecision(12);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 why not auto old_precision = ofs.precision(12); where custom precision is needed (i guess for geometry) and after the output ofs.precision(old_precision);?

Copy link
Author

Choose a reason for hiding this comment

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

Well, since the only purpose of this logger is to output map geometry, and the guard manages the stream and does the writing, I felt like a general increase in accuracy would not hurt

Copy link
Member

Choose a reason for hiding this comment

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

There's a state restorer for this in Boost

http://www.boost.org/doc/libs/1_64_0/libs/io/doc/ios_state.html

Copy link
Contributor

@oxidase oxidase left a comment

Choose a reason for hiding this comment

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

as per chat with @MoKob: adding a single node on the left outside of a OSM test bounding box shifts a single row but not all nodes to the right

@MoKob MoKob merged commit 23fd274 into master Oct 25, 2017
@MoKob MoKob deleted the fix/adjust_angles branch October 25, 2017 08:17
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

3 participants