Simplify now generates Polygon rings with at least four coordiantes
#943
Conversation
frewsxcv
changed the title
Simplify generate Polygons with invalid rings.Simplify now always generates Polygon rings with at least four coordiantes
frewsxcv
force-pushed
the
simplify-polygon
branch
from
8021f5e
to
7b7a60a
Compare
frewsxcv
changed the title
Simplify now always generates Polygon rings with at least four coordiantesSimplify now generates Polygon rings with at least four coordiantes
frewsxcv
force-pushed
the
simplify-polygon
branch
from
7b7a60a
to
b45b7f9
Compare
|
|
||
| // If `simplified_len` is now lower than the minimum number of indices needed, then don't | ||
| // perform the culling and return the original input. | ||
| if *simplified_len <= INITIAL_MIN { |
There was a problem hiding this comment.
Can you imagine a situation where this returns a polygon with many more points than INITIAL_MIN?
I'm not very familiar with this algorithm, but it seems like a pathological 1000 coord polygon could be simplified in 1 step to 2 points, and then because that's below INITIAL_MIN, we'll instead return the original 1000 coord polygon. But I think I'd want to get a 4-pt polygon in that case.
Or maybe that's not possible and I am misunderstanding.
There was a problem hiding this comment.
Agreed we would ideally return a 4 coordinate polygon in that case, but I'm not sure how to modify this algorithm to make that happen, and which is maybe worthy of a separate issue. Unless you know a good solution for this case
There was a problem hiding this comment.
@michaelkirk How do you feel about opening up a follow-up issue for optimizing in that scenario?
There was a problem hiding this comment.
I'm happy for incremental improvements.
My hesitation is that for this change, I wonder if this new behavior could be worse for some people. Like I'm imagining getting an invalid small polygon might be better for some people than getting back a huge unsimplified polygon.
There was a problem hiding this comment.
I actually don't feel that strongly about it, it's just what occurred to me. I'm happy to see it merged if you think it's an improvement.
|
bors d=frewsxcv |
|
✌️ frewsxcv can now approve this pull request. To approve and merge a pull request, simply reply with |
|
bors r+ |
943: `Simplify` now generates `Polygon` rings with at least four coordiantes r=frewsxcv a=frewsxcv - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Prior to this pull request, running `Simplify` on `Polygon` could generate rings with two coordinates, which would make it no longer a valid ring. This pull request updates `Simplify` to ensure resulting `Polygon` rings always have at least four coordinates. Fixes #142 Co-authored-by: Corey Farwell <coreyf@rwell.org>
|
Build failed: |
|
bors r=michaelkirk |
943: `Simplify` now generates `Polygon` rings with at least four coordiantes r=michaelkirk a=frewsxcv - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Prior to this pull request, running `Simplify` on `Polygon` could generate rings with two coordinates, which would make it no longer a valid ring. This pull request updates `Simplify` to ensure resulting `Polygon` rings always have at least four coordinates. Fixes #142 Co-authored-by: Corey Farwell <coreyf@rwell.org>
|
Build failed: |
|
bors r=michaelkirk |
|
Build succeeded: |
CHANGES.mdif knowledge of this change could be valuable to users.Prior to this pull request, running
SimplifyonPolygoncould generate rings with two coordinates, which would make it no longer a valid ring. This pull request updatesSimplifyto ensure resultingPolygonrings always have at least four coordinates.Fixes #142