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

Relax restrictions on bool ops between polygon and multipolygon #1205

Closed
wants to merge 4 commits into from

Conversation

urschrei
Copy link
Member

Whereas previously, boolean ops could only be performed between two geometries of the same type, it should now be possible to perform them between any combination of two Polygons and MultiPolygons

Closes #1203

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Whereas previously, boolean ops could only be performed between two geometries of the same type, it should now be possible to perform them between any combination of two Polygons and MultiPolygons
@urschrei
Copy link
Member Author

@rmanoka I'd love some feedback here to make sure I haven't accidentally tied myself into knots w/r/t/ types – the existing tests weren't modified and all pass, however.

The main reason we need this is in order to implement a unary union: a t::f(g) -> multipoly bool op isn't an obstacle, but the current restriction which says t and g must be the same type (Polygon or MultiPolygon) does cause an issue, because it requires an intermediate conversion of Polygon to MultiPolygon during the fold and reduction ops, and those conversions needlessly allocate because MultiPolygon is Vec under the hood.

WDYT?

@rmanoka
Copy link
Contributor

rmanoka commented Jul 30, 2024

Would it be clean to implement new traits: Intersection, Union, Difference similair to our existing Intersects, Contains; then impl Polygon<T>: Intersection<Polygon<T>> and also Polygon<T>: Intersection<MultiPolygon<T>> and the impls take care of routing to the necessary impl.

So for eg. the clip would be LineString<T>: Intersection<Polygon<T>> ?

@urschrei
Copy link
Member Author

Yeah I'm open to it – where would that leave the public boolops API?

Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>
@rmanoka
Copy link
Contributor

rmanoka commented Aug 1, 2024

I suppose we can deprecate the bool-ops trait, and re-route to the new traits. The method names could probably remain same for ease, and we can provide new alias too.

@urschrei
Copy link
Member Author

I don't think we need this anymore now that #1234 has merged. I think it also enables the major cascaded union use case: https://gist.github.com/urschrei/cd80b4d2ec3c75f12fa541a5bdbf6489

@urschrei urschrei closed this Oct 28, 2024
@michaelkirk
Copy link
Member

I totally forgot about this one. Sorry — we should have merged it months ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Boolean ops should be available between differing geometry types
3 participants