Skip to content

Bump i_overlay dependency to 3.4.1 #1359

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mthh
Copy link
Member

@mthh mthh commented May 14, 2025

  • 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.

Since v3.0.0 iOverlay has changed the winding order of polygon rings to match the one in use in geo so I thought it could be a good thing to update iOverlay (I also wanted to update it because I was starting to look at how much work was needed to use iOverlay's path offsetting for a buffering function for geo)

However there is now two tests that fails:

thread 'algorithm::bool_ops::i_overlay_integration::tests::one_empty_polygon' panicked at geo/src/algorithm/bool_ops/i_overlay_integration.rs:158:9:
assertion `left == right` failed
  left: MULTIPOLYGON(((0.0 1.0,0.0 0.0,1.0 0.0,1.0 1.0,0.0 1.0)))
 right: MULTIPOLYGON(((0.0 0.0,1.0 0.0,1.0 1.0,0.0 1.0,0.0 0.0)))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So when we union POLYGON((0.0 0.0,1.0 0.0,1.0 1.0,0.0 1.0,0.0 0.0) with POLYGON EMPTY, it returns the correct polygon, but the order of coordinates is not preserved. In practical terms, this doesn't change the polygon in question, but it would have been nice if it hadn't changed the order of the coordinates...

thread 'algorithm::bool_ops::tests::gh_issues::gh_issue_1193' panicked at geo/src/algorithm/bool_ops/tests.rs:367:9:
assertion `left == right` failed
  left: MULTIPOLYGON(((-19.458324 -3.623188,-22.058823 -3.623188,-19.064932 -6.57369,-19.458324 -3.623188)),((-14.705883 -7.649791,-17.60358 -8.013862,-17.60358 -8.013863,-14.705883 -7.6497912,-14.705883 -7.649791)))
 right: MULTIPOLYGON(((-22.058823 -3.623188,-19.064932 -6.57369,-19.458324 -3.623188,-22.058823 -3.623188)),((-17.60358 -8.013862,-17.60358 -8.013863,-14.705883 -7.6497912,-14.705883 -7.649791,-17.60358 -8.013862)))

Again, it's a question of the order of the coordinates (I think I can just modify the expected value since there was no expected result in the issue from which this test case came).

This changes the winding order of polygon rings to match the one
in use in geo.
@dabreegster
Copy link
Contributor

(I also wanted to update it because I was starting to look at how much work was needed to use iOverlay's path offsetting for a buffering function for geo)

FYI https://github.com/georust/geo/tree/mkirk/geo-buffer by @michaelkirk is in progress. I wanted to ask though if you mean offsetting for linestrings (as in, offset a linestring left or right and deal with the problems at interior points). I'm interested in an iOverlay-backed approach for that too, but when I last glanced through the API, I only found things exposed for polygons.

@mthh
Copy link
Member Author

mthh commented May 14, 2025

FYI https://github.com/georust/geo/tree/mkirk/geo-buffer by @michaelkirk is in progress

Oh thanks, I think I missed it (or forgot about it) and you're welcome to point it out to me :)

I wanted to ask though if you mean offsetting for linestrings (as in, offset a linestring left or right and deal with the problems at interior points)

I was thinking (at least initially) of a “simple” buffer function, which would have offset both sides of the linestrings (as already exposed by iOverlay).
But to be honest, I was just getting ready to investigate all this and I haven't implemented anything yet (which is a good thing since @michaelkirk already has work in progress on this subject!).

@dabreegster
Copy link
Contributor

Check out https://github.com/a-b-street/ltn/blob/84bc46f5b6e08880385b2bbb61cfb04818ff833e/backend/src/boundary_stats.rs#L97 for an example use. I think there were some open questions about the API that georust should expose, so if you have more use cases, it'd be helpful to understand them

@michaelkirk
Copy link
Member

One thing to check - geo supports MSRV of at least 6 months. IIRC, i_overlay is more aggressive about dropping recent versions. Please check that this builds on geo's MSRV, or whether we'd need to bump. We won't bump to anything more recent than 6 months old.

@@ -31,7 +31,7 @@ proj = { version = "0.30.0", optional = true }
robust = "1.1.0"
rstar = "0.12.0"
serde = { version = "1.0", optional = true, features = ["derive"] }
i_overlay = { version = "2.0.0, < 2.1.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Since i_overlay breaks API's on minor versions as a matter of policy, we need to keep i_overlay pinned to a minor version.

@urschrei
Copy link
Member

I think the 3.0 series is on the 2024 edition (unless I imagined it) so we can't bump yet…

@mthh
Copy link
Member Author

mthh commented May 14, 2025

I think the 3.0 series is on the 2024 edition (unless I imagined it) so we can't bump yet…

Indeed, I just checked and you're right!

We won't bump to anything more recent than 6 months old.

Yep, totally understandable.

Since the contribution of upgrading iOverlay was really small as it is (it just saves us from reversing polygon rings in a function), maybe I should close this PR and we'll reconsider an upgrade in due course?

@urschrei
Copy link
Member

The 2024 edition released with 1.85, and 1.87 releases tomorrow (?), so we could release on the 20th of August by calendar, or Rust 1.90 if we go by version (which is the current practice I believe)

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.

4 participants