- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 1
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
update geo and i_overlay #80
Conversation
Note: this "minor" version bump of i_overlay introduces breaking changes. The i_overlay maintainer intentionally does not follow semver, so I've pegged the versions more conservatively, ensuring we only update explicitly.
let graph = overlay.into_graph(FillRule::NonZero); | ||
let shapes = graph.extract_shapes(StringRule::Slice); | ||
// geo Polygon's are explicitly closed LineStrings, but i_overlay Polygon's are not. | ||
shape.pop(); |
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.
Nail (i_overlay author) told me once (and it's documented) that the i_overlay polygons are implicitly closed, but I got the impression that explicitly closed ones were still acceptable. However with the update to 1.9 in this lib, I was getting bad output when sending an explicitly closed polygon - hence this pop()
to remove the redundant first/last coordinate.
i_overlay 1.9 without this pop()
line notice almost half of the study area isn't colored:
![Screenshot 2025-01-13 at 18 48 12](https://private-user-images.githubusercontent.com/217057/402780478-1f0352c7-7063-41d4-8daf-2a4b026aca77.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4NjE0MjUsIm5iZiI6MTczOTg2MTEyNSwicGF0aCI6Ii8yMTcwNTcvNDAyNzgwNDc4LTFmMDM1MmM3LTcwNjMtNDFkNC04ZGFmLTJhNGIwMjZhY2E3Ny5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE4JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxOFQwNjQ1MjVaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1iZTczMzIxYjNmM2I1YTg2NWM5NTJkYTE2MmM1ZDc0NjgyZDI0ZTAxNGNlMmI1MTY0OWNkNmI3OGEyYTAxMmZjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.yywKRInOrRc_tknGTtWefQnpdEfzdYYheYRbq8MKuns)
i_overlay 1.9 with this pop()
line:
![Screenshot 2025-01-13 at 18 04 48](https://private-user-images.githubusercontent.com/217057/402779903-15099a0e-a193-4520-bfe0-230b4bcad4bf.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4NjE0MjUsIm5iZiI6MTczOTg2MTEyNSwicGF0aCI6Ii8yMTcwNTcvNDAyNzc5OTAzLTE1MDk5YTBlLWExOTMtNDUyMC1iZmUwLTIzMGI0YmNhZDRiZi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE4JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxOFQwNjQ1MjVaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT03YWE1YjVjOWNkMDg2NGIwZjhjYjcxYmI1ZjI3ODYzMGE2NjBjMDY0ZDQwODQ3MWE3MDkyNTFjNzZkNTE1NDhiJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.1MRsORUgxg1f_IaWiVIxoeZlh6xLRNxa0vjF_xkmJmY)
I opted not to dig further into this, but FYI.
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.
Good catch, sounds good.
FYI I was thinking of upstreaming something like this method to geo
when there's spare time, as I'm already using this in a second project. I guess similar to the questions about exposing all of the power of spade triangulation in geo, as iOverlay gets more functionality, it'll be interesting to figure out how much effort it is to also expose nicely through geo.
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.
Brilliant, thanks for the PR. I'm very happy with this fix for now -- the user ought to notice they're drawing a polygon that looks really odd, and so if the next page shows something odd, the next step should be clear enough. We could think about improving the area snapping tool to avoid "dangles" (and other problems) in the first place, but it's much lower priority.
let graph = overlay.into_graph(FillRule::NonZero); | ||
let shapes = graph.extract_shapes(StringRule::Slice); | ||
// geo Polygon's are explicitly closed LineStrings, but i_overlay Polygon's are not. | ||
shape.pop(); |
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.
Good catch, sounds good.
FYI I was thinking of upstreaming something like this method to geo
when there's spare time, as I'm already using this in a second project. I guess similar to the questions about exposing all of the power of spade triangulation in geo, as iOverlay gets more functionality, it'll be interesting to figure out how much effort it is to also expose nicely through geo.
I agree! I actually initially started down that road yesterday, but quickly scaled back my ambition due to the orphan rule. It's a source of endless frustration. I see a way forward, but it might be controversial and didn't want to bog down moving this forward. |
FIXES #54
Given an invalid geometry, the geo bool ops code hit an unexpected state, triggering a debug_assert, which would crash a development build (see before).
The easiest duplication I know of is to have a one dimensional "spike" coming out of the polygon.
The latest (unreleased) geo introduced a validation feature, so that invalid geometries no longer trigger the debug assert.
So I've updated geo to the yet-to-be-released version, which in turn requires a newer version of i_overlay.
Note: given invalid input, the output of BoolOps can't be guaranteed to be reasonable - it's a garbage in / garbage out situation. So far it looks like the output just trims off the spike. I'm not 100% convinced that this current solution is the best UX, but I think it's a big enough improvement to make the remaining problem lower priority. LMK what you think @dabreegster
before
after