Implement `Stitch` trait #1087

RobWalt · 2023-10-10T13:46:43Z

This is an implementation of a trait which offers functionality to stitch up geometry parts that were split at some point of time. It's an alternative to boolean operations which doesn't panic and returns user friendly errors instead.

This is mostly used within the new SpadeBoolops trait which needs to stitch together some parts of a triangulation. Hence, this trait is mostly used on triangles but isn't constraint on those and was implemented here for a more general scenario. Here are some rough visuals of what the operation does

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.

TODOs

add docs for main trait
Add simple benchmarks
try to optimize performance

geo/src/algorithm/stitch.rs

RobWalt · 2023-11-14T19:40:21Z

This PR is in a reviewable state now. Please feel free to take a look. It's important for the SpadeBoolops PR.

frewsxcv

still reviewing, but looking good so far!

geo/src/algorithm/stitch.rs

frewsxcv · 2023-12-04T19:32:39Z

how does this compare to the boolean operation "union"?

RobWalt · 2023-12-06T09:12:03Z

how does this compare to the boolean operation "union"?

Compared to the BooleanOps trait this is probably not as performant. One of the checks which reassociates holes needs to construct Polygons from LineString-rings just for containment checks. This and some other allocations probably make it less performant.

On the other hand, this is just another safe (non-panic) alternative to BooleanOps which provides robustness in all cases if the invariants of the algo are fulfilled at it's start. While writing this I wonder though if BooleanOps would be safe operation under exactly the same invariants though. I probably need to do some more testing on it and we might be able to scratch this algo. Nevertheless, even if that were the case, I couldn't prove that it wouldn't hit one of the BooleanOps panics.

Thanks for asking those kinds of questions!

michaelkirk

Sorry this sat here so long @RobWalt!

All of the performance related commentary is "take it or leave it". I'd be happy to follow up on that later. I'm primarily interested in the correctness aspect, and have asked a few questions that I'd like to understand better.

geo/src/algorithm/stitch.rs

RobWalt · 2024-01-11T09:50:46Z

Sorry this sat here so long @RobWalt!

All of the performance related commentary is "take it or leave it". I'd be happy to follow up on that later. I'm primarily interested in the correctness aspect, and have asked a few questions that I'd like to understand better.

Thanks for your efforts in reviewing this. I'd also like to improve performance here since it's a bottleneck in the SpadeBoolops so I'll take another shot in optimizing here. I'll try to add some benchmarks first so we can meassure improvements.

RobWalt · 2024-01-11T14:00:30Z

Thanks for the thorough review @michaelkirk. In the end we got a

27% + 88% + 8% which roughly equates 92% total

performance improvement.

geo/src/algorithm/stitch.rs

geo/CHANGES.md

geo/src/algorithm/mod.rs

frewsxcv

lgtm! thank you for your patience!

frewsxcv · 2024-01-20T18:52:10Z

@michaelkirk Do you have any outstanding reservations before merging this?

geo/src/algorithm/stitch.rs

michaelkirk · 2024-01-22T20:23:31Z

Yes, I'd like to take another look now that it's been reformulated in terms of triangles. I'll try to do that in the next couple days.

geo/src/algorithm/stitch.rs

michaelkirk

Hi @RobWalt - I've left some little comments that should be easy to address, and conceptually I've made a lot of progress, but am still a struggling a bit understanding the inner workings of find_and_fix_holes_in_exterior - maybe you can elaborate?

geo/src/algorithm/stitch.rs

michaelkirk · 2024-01-31T23:02:48Z

geo/src/algorithm/stitch.rs

+/// This is important for scenarios like the banana polygon. Which is considered invalid
+/// https://www.postgis.net/workshops/postgis-intro/validity.html#repairing-invalidity
+fn find_and_fix_holes_in_exterior<F: GeoFloat>(mut poly: Polygon<F>) -> Polygon<F> {
+    fn detect_if_rings_closed_with_point<F: GeoFloat>(


I'm having a hard time reconciling the name of this method with what it seems to do.

What I think it does: if p is in points, we return a new ring from the subsequence of points starting with wherever p is found.

So e.g. given as args:

let mut points = [a, b, c, d, e, f, g]; let point = d; let ring = detect_if_rings_closed_with_point(&mut points, point);

Would result in:

ring = [d, e, f, g, d] points: [a, b, c] // left over

Is that right?

TBH I'm not really sure what it's for... I'd of assumed the way that we constructed the rings precludes unclosed rings from occurring. Do any of the test cases exemplify what this method is for?

I think the main problem here is that we don't really know which vertex we start at. We could get a list of points

[a3, a1, b1, b2, b3, b4, b1, a1, a2]

where the last (or first, whatever floats your boat) ring is looping around the Vec. For this ring we don't really want to get the points between the duplicates (a1, b1, b2, b3, b4, b1, a1) but the opposite or left over. I just gave it a try to find a better and more obvious way of doing that but for now I can't find a more concise way actually. Handling this special case for the last ring always results in a mess.

I'm pretty far from understanding what this method is for, so I'll continue to ask dumb questions.

Is this method given points that are not necessarily in the ring? Where did they come from? What happens to those points, do we just drop them?

Given these triangles here

We could potentially end up with a exterior ring of the form

[F, C, E, D, C, B, A, H, G]

and we want to convert that into a nice polygon with a hole instead. As the order of the coords above is not deterministic we need to do the "one step at a time and remove finished rings" procedure to not miss the ring that will be added last. Here's how the algo works in steps for this example:

[F] [F, C] [F, C, E] [F, C, E, D] <-- adding another C closes a ring remove and remember [C, E, D, C] and continue [F, B] [F, B, A] [F, B, A, H] [F, B, A, H, G] <-- nothing left so this is also a ring

geo/src/algorithm/stitch.rs

Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

In particular the latest `home` crate used by bindgen, required by proj, needs rust-1.70+ Also updates CI containers to latest

This fixes a new lint introduced in rust 1.75 Fixes georust#1088

The previous approach worked, but then there was feedback that the trait should maybe be "more complete" and implemented on additional types. Really, I introduced the trait to solve a problem: we have some undesirable unwraps in the codebase with some of our floating type geometries. I didn't care to actually introduce a general-purpose TotalOrd trait that was useful outside of that context.

There's so little you could do with HasKernel that isn't also a GeoNum. I'm not sure there's value in having two traits.

Authored-by: RobWalt <robwalter96@gmail.com>

We decided to do this since we're not sure yet about how users might use this functionality. In the worst case the API is still confusing and leads to a lot of error reports due to the implicit assumptions which are only documented in doc strings and not enforced by code or won't be caught by errors Authored-by: RobWalt <robwalter96@gmail.com>

Add some notes that triangles shouldn't even overlap instead of just stating that duplicates are not allowed. Authored-by: RobWalt <robwalter96@gmail.com>

Authored-by: RobWalt <robwalter96@gmail.com>

This benchmark needs a public export of the stitch trait which isn't given anymore. I added some comments for the after world. Authored-by: RobWalt <robwalter96@gmail.com>

Authored-by: RobWalt <robwalter96@gmail.com>

piaoger · 2024-04-19T05:49:01Z

@RobWalt
I tried your is-geo-boolops-still-broken tool, stitch/spade-boolops PRs. I am using them in my in-house tool and polygon boolean works really good. What make these great PRs suspended, need more tests or codeview?

RobWalt · 2024-04-19T07:24:59Z

Currently, this PR here boils down to this comment here #1087 (comment) . Once that is resolved, we should be able to merge this PR and continue with the Boolops PR. Given that someone (you) actually uses the new feature I might be motivated enough to tackle it! Thanks for the feedback by the way!

piaoger · 2024-05-01T00:12:27Z

Currently, this PR here boils down to this comment here #1087 (comment) . Once that is resolved, we should be able to merge this PR and continue with the Boolops PR. Given that someone (you) actually uses the new feature I might be motivated enough to tackle it! Thanks for the feedback by the way!

A robust geometry boolean library is always welcome and I find SpadeBoolops is quit good :)

RobWalt force-pushed the feat/implement-stitch-trait branch from e573f00 to de6c679 Compare October 10, 2023 13:48

RobWalt mentioned this pull request Oct 11, 2023

Implement SpadeBoolops trait #1089

Open

6 tasks

RobWalt marked this pull request as draft October 12, 2023 12:41

RobWalt changed the title ~~Implement Stitch trait~~ Draft: Implement Stitch trait Oct 28, 2023

RobWalt marked this pull request as ready for review October 31, 2023 20:35

RobWalt changed the title ~~Draft: Implement Stitch trait~~ Implement Stitch trait Oct 31, 2023

RobWalt commented Nov 9, 2023

View reviewed changes

geo/src/algorithm/stitch.rs Outdated Show resolved Hide resolved

RobWalt force-pushed the feat/implement-stitch-trait branch from cc0bd92 to 587d332 Compare November 9, 2023 09:20

RobWalt force-pushed the feat/implement-stitch-trait branch from d068818 to be9ed92 Compare November 28, 2023 07:56

RobWalt self-assigned this Nov 28, 2023

frewsxcv reviewed Dec 4, 2023

View reviewed changes

geo/src/algorithm/stitch.rs Show resolved Hide resolved

RobWalt force-pushed the feat/implement-stitch-trait branch from 88fd5ef to 85828b4 Compare December 12, 2023 07:23

michaelkirk reviewed Jan 10, 2024

View reviewed changes

michaelkirk reviewed Jan 11, 2024

View reviewed changes

geo/src/algorithm/stitch.rs Show resolved Hide resolved

frewsxcv reviewed Jan 19, 2024

View reviewed changes

geo/CHANGES.md Outdated Show resolved Hide resolved

frewsxcv reviewed Jan 19, 2024

View reviewed changes

geo/src/algorithm/mod.rs Outdated Show resolved Hide resolved

frewsxcv approved these changes Jan 20, 2024

View reviewed changes

frewsxcv reviewed Jan 20, 2024

View reviewed changes

geo/src/algorithm/stitch.rs Outdated Show resolved Hide resolved

michaelkirk reviewed Jan 23, 2024

View reviewed changes

geo/src/algorithm/stitch.rs Outdated Show resolved Hide resolved

geo/src/algorithm/stitch.rs Outdated Show resolved Hide resolved

geo/src/algorithm/stitch.rs Show resolved Hide resolved

michaelkirk reviewed Jan 31, 2024

View reviewed changes

wtchen and others added 3 commits February 1, 2024 10:47

Implement ChaikinSmoothing for Geometry

7ccd4e6

Address comments

50f4f36

fix: n - 1 linestrings at small distances

c97bbff

urschrei and others added 25 commits February 1, 2024 10:47

clarify relationship between IntersectionMatrix and DE9-IM matrix

505da65

Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

Bump MSRV in line with some of our dependencies.

7aef532

In particular the latest `home` crate used by bindgen, required by proj, needs rust-1.70+ Also updates CI containers to latest

New TotalOrd trait avoids exploding when working with NaN.

315965e

This fixes a new lint introduced in rust 1.75 Fixes georust#1088

Move total_cmp to GeoNum

483c77c

Merge HasKernel into GeoNum for simplicity.

3cc9bf6

There's so little you could do with HasKernel that isn't also a GeoNum. I'm not sure there's value in having two traits.

fixup changelog

7c5448f

Update changelog

4309e8b

Update changelog

9fae8e3

Fix typos

b95367a

Update geo/src/algorithm/stitch.rs

ab13b4e

fix(test): repair doc test

68911eb

Authored-by: RobWalt <robwalter96@gmail.com>

fix(review): don't use custom capacity

360d8a9

Authored-by: RobWalt <robwalter96@gmail.com>

chore(cleanup): remove unused error variants

8248661

Authored-by: RobWalt <robwalter96@gmail.com>

docs(stitch): improve doc string

ec38c7a

Add some notes that triangles shouldn't even overlap instead of just stating that duplicates are not allowed. Authored-by: RobWalt <robwalter96@gmail.com>

fix(test): remote pub export

5c6b7e7

Authored-by: RobWalt <robwalter96@gmail.com>

chore(cleanup): comment benchmark out

cf396d8

This benchmark needs a public export of the stitch trait which isn't given anymore. I added some comments for the after world. Authored-by: RobWalt <robwalter96@gmail.com>

chore(changelog): remove trait from changelog

fdef059

Authored-by: RobWalt <robwalter96@gmail.com>

fix(tests): comment out failing doc test

8df3d16

Authored-by: RobWalt <robwalter96@gmail.com>

refactor(helpers): move triangle winding function

bd55e9b

Authored-by: RobWalt <robwalter96@gmail.com>

chore: fix small typo

13de574

Authored-by: RobWalt <robwalter96@gmail.com>

chore: improve wording

66fc4e0

Authored-by: RobWalt <robwalter96@gmail.com>

chore: improve explaining comment

93c8261

Authored-by: RobWalt <robwalter96@gmail.com>

chore(tests): use Relate for testing

c11f8fe

Authored-by: RobWalt <robwalter96@gmail.com>

RobWalt force-pushed the feat/implement-stitch-trait branch from c45df08 to c11f8fe Compare February 1, 2024 10:36

wlinna mentioned this pull request Apr 28, 2024

Panic in BooleanOps::union for Polygon<f32> #1103

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Stitch` trait #1087

Implement `Stitch` trait #1087

RobWalt commented Oct 10, 2023 •

edited

RobWalt commented Nov 14, 2023

frewsxcv left a comment

frewsxcv commented Dec 4, 2023

RobWalt commented Dec 6, 2023

michaelkirk left a comment

RobWalt commented Jan 11, 2024

RobWalt commented Jan 11, 2024

frewsxcv left a comment

frewsxcv commented Jan 20, 2024

michaelkirk commented Jan 22, 2024

michaelkirk left a comment

michaelkirk Jan 31, 2024

RobWalt Feb 1, 2024

michaelkirk Feb 1, 2024

RobWalt Feb 2, 2024 •

edited

piaoger commented Apr 19, 2024

RobWalt commented Apr 19, 2024 •

edited

piaoger commented May 1, 2024

Implement Stitch trait #1087

Are you sure you want to change the base?

Implement Stitch trait #1087

Conversation

RobWalt commented Oct 10, 2023 • edited

RobWalt commented Nov 14, 2023

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv commented Dec 4, 2023

RobWalt commented Dec 6, 2023

michaelkirk left a comment

Choose a reason for hiding this comment

RobWalt commented Jan 11, 2024

RobWalt commented Jan 11, 2024

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv commented Jan 20, 2024

michaelkirk commented Jan 22, 2024

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Jan 31, 2024

Choose a reason for hiding this comment

RobWalt Feb 1, 2024

Choose a reason for hiding this comment

michaelkirk Feb 1, 2024

Choose a reason for hiding this comment

RobWalt Feb 2, 2024 • edited

Choose a reason for hiding this comment

piaoger commented Apr 19, 2024

RobWalt commented Apr 19, 2024 • edited

piaoger commented May 1, 2024

Implement `Stitch` trait #1087

Implement `Stitch` trait #1087

RobWalt commented Oct 10, 2023 •

edited

RobWalt Feb 2, 2024 •

edited

RobWalt commented Apr 19, 2024 •

edited