Feature Addition: Polygon boolean (#1) #1319
Conversation
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
| # insert intersection points into rings | ||
| function _insertintersections!(intersections, seginds, allrings) |
There was a problem hiding this comment.
So is this function reimplementing Base.splice!? If you take the vertices(ring) as a CircularVector, you should be able to splice multiple points efficiently.
There was a problem hiding this comment.
CircularVector does not have a splice! method (weirdly). insert! also doesn't update the size of CircularVector until after insertion, which means it places points at the beginning of the vector. If we add multiple points to the end (e.g., the final segment of a poly has multiple intersections), their order gets reversed. Append is much cleaner.
The other key part here is making sure the right points go into the right rings in the right orientation. This means the checking of ps and pe, the sorting, etc, are necessary to keep proper geometry.
There was a problem hiding this comment.
Should we open a PR in CircularArrays.jl to add splice! support?
| """ | ||
| polygonbooleanop(geometry, other, operation) | ||
|
|
||
| Performs boolean `operation` of `geometry` with `other`. |
There was a problem hiding this comment.
Can the algorithm be used with any kind of geometry or just polygons (and their rings)?
There was a problem hiding this comment.
as of right now, polygons and rings, but in the future polygon-segment and segment-polygon should be added. I'll knock these down to polygon/ring for the time being.
src/polygonboolean.jl
Outdated
| Base.union(a::Geometry, b::Geometry) = polygonbooleanop(a, b, union) | ||
| Base.setdiff(a::Geometry, b::Geometry) = polygonbooleanop(a, b, setdiff) | ||
| Base.symdiff(a::Geometry, b::Geometry) = polygonbooleanop(a, b, symdiff) | ||
| Base.xor(a::Geometry, b::Geometry) = polygonbooleanop(a, b, symdiff) |
There was a problem hiding this comment.
I believe the dispatch should be done with Polygon instead of general Geometry given the name of the function used in the implementation.
Also, please move these definitions to the source file where the Polygon is defined.
There was a problem hiding this comment.
And the docstrings should be attached to these specific methods. You are currently adding docstrings to the function name, not the method.
juliohm
left a comment
juliohm
left a comment
There was a problem hiding this comment.
It is not very clear what this PR is trying to accomplish. I can review it again after you consider the first round of comments.
…Fixing method dispatch for polygonbooleanop! on polygons.
The primary goal is build a rough shell of the polygonboolean operation and implement the first major feature, which is splitting segments at intersections and inserting those into the polygon. |
2 participants
In line with feedback in #1318 here is an initial small PR that adds a shell for the broader algorithm and the first necessary step for this algorithm - which is to compute and insert intersections into polygons.
I'm also adding a utility that's used through the algorithm to take a 4 bit number marking whether a segment has a subject or clipping polygon above or below it.
These are small, simple internals that should be easy to review.
The next PR will be taking these modified polygons and performing a sweep line to fill in those segment region fills.
Edit: current failing test is
pairwiseintersect, which hasn't been changed.