-
Notifications
You must be signed in to change notification settings - Fork 673
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 polygon area calculation: use Shoelace formula #2927
Conversation
I noticed that inner polygons have positive area sometimes. I think this might be the cause of the failed tests. Anyway, investigating.. |
@merkispavel just an FYI recently @nilsnolde was also computing polygon area (using boost) in #2750 i wonder if both PRs could use the same method of computing polygon area. just wanted to flag it for you to have a look |
Polygon area (line 552 in midgard/util.cc) I believe should be: area += (p1->first * p2->second) - (p1->second * p2->first) This should work for any simple polygon. |
I verified that the change described above is correct. I wrote a simple test with a rectangle and the current polygon_area computation fails and the update is correct. Problem is this breaks the isochrone tests. |
I think the standard convention is for counter-clockwise orientations to produce positive area and clockwise orientations should yield negative area. A simple test (rectangle) could be added to test/util_midgard.cc:
|
Right, these formulas are equivalent. I was too lazy to edit more than 2 symbols :) sorry |
Totally agreed! I have more changes locally with unittests + comments, haven't pushed it yet |
Another PR should be merged first in order to fix tests #2932 |
for (auto p1 = polygon.cbegin(), p2 = std::next(polygon.cbegin()); p2 != polygon.cend(); | ||
++p1, ++p2) { | ||
area += (p1->first + p2->first) * (p1->second + p2->second); | ||
area += p1->first * p2->second - p1->second * p2->first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dnesbitt61 I changed the formula to its classic form not to confuse people in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@merkispavel if you could visually inspect the differences in the isochrone unit test and then update the values then we can get CI passing and we can merge it. looks good to me |
* Track segment orientation This is need to correctly join segments into rings * Update changelog * Remove ring reversing * Fix: use multiline comment
…rmula # Conflicts: # test/isochrone.cc
@kevinkreiser @dnesbitt61 the tests are fixed. Looks shippable in my view |
CHANGELOG.md
Outdated
* FIXED: Skip bindings if there's no Python development version [#2893](https://github.com/valhalla/valhalla/pull/2893) | ||
* FIXED: Skip bindings if there's no Python development version [#2893](https://github.com/valhalla/valhalla/pull/2878) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems wrong 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrong link was used before so automerge decided to keep both entries. Fixed, thanks
@@ -98,7 +98,7 @@ template <std::size_t dimensions_t> class GriddedData : public Tiles<PointLL> { | |||
typename PointLL::first_type s[5]; // Values at the tile corners and center |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the code from #2932 in this PR as well? something seems messed up here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally i found 2 issues at once(polygon area bug and orientation bug) so I decided to make a hierarchy master <- 2927 <- 2932
for some reason 🤦(probably because of the unit tests) Now I think It brought more mess than clearness. Anyway, I had to fix unit tests so it didn't bring any profit
Would you like to split them @kevinkreiser ? (I guess I do, it'll just take a little more time and a few more approves)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valhalla/midgard/gridded_data.h
Outdated
std::array<std::function<void()>, 10> cases{ | ||
[]() {}, | ||
[&]() { from_pt = to_pt = {}; }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to change that back to a no-op?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, previous review was the difficult part. fixing the area calculation was straight forward
https://en.wikipedia.org/wiki/Shoelace_formula
Issue
Isochrone api creates polygon away from the origin location. The reason is the broken
polygon_area
function. The area of the small polygon turnes out to be larger than the whole convex-hull-ish polygon. So if you setdenoise = 1
you get the issue itself.If you want to reproduce
request:
'{"locations":[{"lat":51.2491202,"lon":0}],"costing":"pedestrian","contours":[{"time":20}], "show_locations":true, "polygons":true, "denoise":1}'
planet.tar.zip
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?