Implement SpadeBoolops trait
#1089
Conversation
RobWalt
force-pushed
the
feat/spade-boolops
branch
from
5f9b675
to
409b815
Compare
|
@RobWalt this is a cool implementation! I don't fully understand the correctness yet, but are you getting this to pass all the JTS test fixtures that we run the current bool. ops. on? If so, I'm happy to merge this as the default boolean op. |
RobWalt
changed the title
SpadeBoolops traitSpadeBoolops trait
RobWalt
changed the title
SpadeBoolops traitSpadeBoolops trait
|
@rmanoka How would you define "passing the current tests"? My algorithm won't find the outputs specified in these tests with the points in the linestrings in the exact same order. Is that allowed? |
|
I ported the tests under the name "legacy_tests". They all run through. There is a small caveat though: The 894 test runs awfully long. (~18s for 3 intersections on my machine). This is probably due to the fact that the new algo isn't very efficient for bigger geometries. |
RobWalt
changed the title
SpadeBoolops traitSpadeBoolops trait
How long does the test take using the existing bool ops? How long in release mode? I don't think worse perf than the existing impl is a problem, but I'm interested in the performance characteristics more generally. |
It's
So you really should consider if you want to use that and if the input is ok for it. I might add a warning. I can also look into subtle optimizations (via bounding boxes) to squeeze out a bit of performance. Please note that the Multipolygons in this example are absolutely giant (23 KB, 150 polygons per Multipolygon, 2600 vertices total per Multipolygon) |
|
I implemented some minor bounding box optimizations and now the big test runs through in
There were two test results I needed to change for it still to work:
|
RobWalt
force-pushed
the
feat/spade-boolops
branch
2 times, most recently
from
436110d
to
5128160
Compare
This trait is a non-panicking alternative for the `BooleanOps::union` and actually doesn't modify the location of any of the coordinates. Instead it returns an error in failing scenarios. Co-Authored-By: RobWalt <[email protected]> Co-Authored-By: Azorlogh <[email protected]>
Co-Authored-By: RobWalt <[email protected]> Co-Authored-By: Azorlogh <[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]>
RobWalt
force-pushed
the
feat/spade-boolops
branch
from
ccc5df5
to
4d4d92f
Compare
Relevant commits start from 409b815
This PR depends on #1083 and #1087
Feel free to test the latest version of the algo with this app
... and report errors if you find some 🤞🏼
Description
This is a draft for a new algorithm for boolops.
It was created because the current implementation in the repo has some issues with panics. This spawns a lot of issues in the repository.
The new algorithm might not be the most performant candidate. Still, it poses a good alternative for scenarios in which not every nano second is counted. Overall, I couldn't notice any performance issues in my application yet. That being said, I'm still open for a discussion about performance if that's a critical thing for you.
Algorithm on a high level
The algorithm itself is dead simple:
Stitchtrait to reassemble the triangles intoMultiPolygonsCHANGES.mdif knowledge of this change could be valuable to users.