-
Notifications
You must be signed in to change notification settings - Fork 198
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
Implement Stitch
trait
#1087
Implement Stitch
trait
#1087
Conversation
e573f00
to
de6c679
Compare
cc0bd92
to
587d332
Compare
This PR is in a reviewable state now. Please feel free to take a look. It's important for the |
d068818
to
be9ed92
Compare
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.
still reviewing, but looking good so far!
how does this compare to the boolean operation "union"? |
Compared to the On the other hand, this is just another safe (non-panic) alternative to Thanks for asking those kinds of questions! |
88fd5ef
to
85828b4
Compare
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.
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 |
Thanks for the thorough review @michaelkirk. In the end we got a
performance improvement. |
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.
lgtm! thank you for your patience!
@michaelkirk Do you have any outstanding reservations before merging this? |
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. |
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.
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?
/// 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>( |
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.
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?
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.
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.
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.
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?
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.
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
c45df08
to
c11f8fe
Compare
@RobWalt |
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! |
In the review process we figured that this still was a bit ambiguous. Authored-by: RobWalt <[email protected]>
adding more details to the `find_boundary_lines` functions since it's quiet important for performance to get the cencepts right there Authored-by: RobWalt <[email protected]>
Factor out common code and call it directly in the `Triangle` and `MultiPolygon` impls to prevent needless cloning and indirection. Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
As stated in one of the review comments, the main purpose of the algo is to stitch together triangles that resulted from a triangulation. Although it would also be nice to generalize the algo for polygons and multi polygons, it makes the code harder to read. It is advised to just triangulate in those cases and then run the stitching on the compound triangulation. On another note: special casing the algorithm to triangles also gave us a real good performance boost again. We gained another ~50% boost from all of the improvements in this commit. Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
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 <[email protected]>
Add some notes that triangles shouldn't even overlap instead of just stating that duplicates are not allowed. Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
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 <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
Authored-by: RobWalt <[email protected]>
73e573b
to
715a319
Compare
It finally went through 👍🏼 |
Last call, would like to merge this tomorrow. |
@urschrei bump 🥺👉👈 |
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 doesCHANGES.md
if knowledge of this change could be valuable to users.TODOs