Implement Polygon Boolean Operations (Union, Intersection, Difference)#693
Implement Polygon Boolean Operations (Union, Intersection, Difference)#693yungyuc merged 1 commit intosolvcon:masterfrom
Conversation
docs/polygon_boolean_algorithm.html
Outdated
| @@ -0,0 +1,951 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Only for PR review purpose. Please use a broswer to open it. Will delete this file once the PR is approved.
There was a problem hiding this comment.
Please move the extended document in a gist. Make sure this file is not in your commit history.
| * 5. Emitting result trapezoids as polygons | ||
| */ | ||
| template <typename T> | ||
| std::shared_ptr<PolygonPad<T>> compute_boolean_with_decomposition( |
There was a problem hiding this comment.
Key function. Please check the PR description or docs/polygon_boolean_algorithm.html for the details.
There was a problem hiding this comment.
This function can be made to a helper class in the future, but the length is OK for now.
|
|
||
| # TODO: Verify and implement boolean operations | ||
| # once the C++ side is complete. | ||
| def _compute_total_area(self, result_pad): |
There was a problem hiding this comment.
Addressed previous TODOs.
|
@yungyuc Could you please take a look at this PR? The C++ implementation is about 400 LOC, but it mainly contains a single critical function and quite a few comment lines. The pytest code is around 300 LOC. My preference would be to keep everything as is. However, if we need to strictly enforce the 500 LOC limit, I can help trim some comments and test cases. |
docs/polygon_boolean_algorithm.html
Outdated
| @@ -0,0 +1,951 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Please move the extended document in a gist. Make sure this file is not in your commit history.
| * Critical Y-values: {0, 1, 2, 3} | ||
| * Three y-bands: [0,1], [1,2], [2,3] | ||
| * | ||
| * y=3 - - - - - +----------------+- - - - - - - - - - |
There was a problem hiding this comment.
Did you use a tool to generate the AA? If you did, please leave a comment stating what and how as a reference that may help other contributors.
There was a problem hiding this comment.
I asked AI to leave explanation comments for the code. AI generated the AA directly. However, most of the time I need to manually adjust the chart for annotation or alignment.
There was a problem hiding this comment.
Hand-creation of AA usually took most of the efforts. I guess the manual adjustment did not take too much time.
cpp/modmesh/universe/polygon.hpp
Outdated
| Union, | ||
| Intersection, | ||
| Difference | ||
| }; |
cpp/modmesh/universe/polygon.hpp
Outdated
| namespace detail | ||
| { | ||
|
|
||
| enum class BoolOp |
There was a problem hiding this comment.
I prefer BooleanOperation instead of BoolOp, but this is a must-do. Up to the author.
cpp/modmesh/universe/polygon.hpp
Outdated
| } | ||
|
|
||
| // Step 1: Decompose both polygons using the pad's TrapezoidalDecomposer | ||
| auto [begin1, end1] = pad->decompose_to_trapezoid(polygon_id1); |
There was a problem hiding this comment.
The return type is not written in the RHS. Do not use auto here.
There was a problem hiding this comment.
Use std::tie instead here.
| // The source polygon is determined by the index range: | ||
| // [begin1, end1) -> polygon1 (source 0) | ||
| // [begin2, end2) -> polygon2 (source 1) | ||
| auto source_of = [begin1, end1, begin2, end2](size_t idx) -> int |
There was a problem hiding this comment.
Many if not all of the lambdas should be made as a member function when this helper function is refactored as a helper class for better testability and performance.
| }; | ||
|
|
||
| // Collect all unique Y values from trapezoid boundaries | ||
| std::set<value_type> y_set; |
There was a problem hiding this comment.
Watch for potential performance hotspot with STL containers in the future.
cpp/modmesh/universe/polygon.hpp
Outdated
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Use an end marker for long functions.
|
|
||
| # TODO: Verify and implement boolean operations | ||
| # once the C++ side is complete. | ||
| def _compute_total_area(self, result_pad): |
| result_diff = pad.boolean_difference(polygon1, polygon2) | ||
| self.assertEqual(result_diff.num_polygons, 0) | ||
|
|
||
| def test_boolean_shared_edge(self): |
There was a problem hiding this comment.
It's good to test for edge touch. Point touch should be tested too, but it can be supplemented in the future.
|
@c1ydehhx please help review if you have time. |
d2ea798 to
ca52d13
Compare
|
|
||
| // Step 1: Decompose both polygons using the pad's TrapezoidalDecomposer | ||
| size_t begin1, end1, begin2, end2; | ||
| std::tie(begin1, end1) = pad->decompose_to_trapezoid(polygon_id1); |
There was a problem hiding this comment.
With the type the lines look clearer. It's a pity the types cannot be written in the same line of the function call.
| size_t begin1, end1, begin2, end2; | ||
| std::tie(begin1, end1) = pad->decompose_to_trapezoid(polygon_id1); | ||
| std::tie(begin2, end2) = pad->decompose_to_trapezoid(polygon_id2); | ||
| std::shared_ptr<TrapezoidPad<T>> trap_pad = pad->decomposed_trapezoids(); |
| } | ||
|
|
||
| return result; | ||
| } /* end function compute_boolean_with_decomposition */ |
| value_type x_lo, x_hi; // X at y_low and y_high | ||
| int source; | ||
| bool is_start; // true = entering the polygon interval, false = leaving | ||
| }; /* end struct Event */ |
|
@yungyuc The issues are addressed. Please take a look. Thank you.
|
| * Critical Y-values: {0, 1, 2, 3} | ||
| * Three y-bands: [0,1], [1,2], [2,3] | ||
| * | ||
| * y=3 - - - - - +----------------+- - - - - - - - - - |
There was a problem hiding this comment.
Hand-creation of AA usually took most of the efforts. I guess the manual adjustment did not take too much time.
|
|
||
| // Step 1: Decompose both polygons using the pad's TrapezoidalDecomposer | ||
| size_t begin1, end1, begin2, end2; | ||
| std::tie(begin1, end1) = pad->decompose_to_trapezoid(polygon_id1); |
There was a problem hiding this comment.
With the type the lines look clearer. It's a pity the types cannot be written in the same line of the function call.
| // Find the Y-value where two edges cross within the overlap range. | ||
| // Each edge is defined by its X-values at y_lo and y_hi. | ||
| // clang-format off | ||
| auto find_crossing = [&](value_type edge_a_x_at_bottom, value_type edge_a_x_at_top, |
There was a problem hiding this comment.
Leave a comment to remind us to profile later and use the timing results to refactor. An issue should be created to track it after the PR is merged.
I am not totally sure about the performance of a closure in the nested loop. My gut feeling is that this is not gonna be fast. It may be hard to profile too.
For the sake of clarity, the nested loop may use a distinct helper class. But it is for future work.
There was a problem hiding this comment.
I am not seeing the comment (code remark) to remind to profile in the future. Please point it out using inline review annotation.
There was a problem hiding this comment.
The beginning of the lambda is the primary location to leave a TODO comment for potential performance hotspot.
cpp/modmesh/universe/polygon.hpp
Outdated
| Union, | ||
| Intersection, | ||
| Difference | ||
| }; /* end enum BooleanOperation */ |
There was a problem hiding this comment.
Should be end enum class BooleanOperation, not end enum BooleanOperation.
Use exact declaration will help grepping.
ca52d13 to
b46771d
Compare
|
@yungyuc should be good now. :D |
yungyuc
left a comment
There was a problem hiding this comment.
A previous point was not addressed. Please clarify.
| // Find the Y-value where two edges cross within the overlap range. | ||
| // Each edge is defined by its X-values at y_lo and y_hi. | ||
| // clang-format off | ||
| auto find_crossing = [&](value_type edge_a_x_at_bottom, value_type edge_a_x_at_top, |
There was a problem hiding this comment.
I am not seeing the comment (code remark) to remind to profile in the future. Please point it out using inline review annotation.
| return bottom_point.x() + t * (top_point.x() - bottom_point.x()); | ||
| }; | ||
|
|
||
| // TODO: potential performance issue: std container |
| value_type trap_j_right_at_bottom = lerp_x(trap_j_right_bottom, trap_j_right_top, y_lo); | ||
| value_type trap_j_right_at_top = lerp_x(trap_j_right_bottom, trap_j_right_top, y_hi); | ||
|
|
||
| // TODO: potential performance issue: closure in the nested loop |
There was a problem hiding this comment.
This is not the best place for the TODO comment, but it makes sense to have one note here too. Don't delete it.
|
@yungyuc I have specified where the TODOs are. Please check the comments above. Thanks! |
yungyuc
left a comment
There was a problem hiding this comment.
@yungyuc I have specified where the TODOs are. Please check the comments above. Thanks!
Thank you, now I see them, but one is not at a proper location. If I have time I will modify it myself, but you can do it too. Let's see who's gonna be faster.
| value_type trap_j_right_at_bottom = lerp_x(trap_j_right_bottom, trap_j_right_top, y_lo); | ||
| value_type trap_j_right_at_top = lerp_x(trap_j_right_bottom, trap_j_right_top, y_hi); | ||
|
|
||
| // TODO: potential performance issue: closure in the nested loop |
There was a problem hiding this comment.
This is not the best place for the TODO comment, but it makes sense to have one note here too. Don't delete it.
| // Find the Y-value where two edges cross within the overlap range. | ||
| // Each edge is defined by its X-values at y_lo and y_hi. | ||
| // clang-format off | ||
| auto find_crossing = [&](value_type edge_a_x_at_bottom, value_type edge_a_x_at_top, |
There was a problem hiding this comment.
The beginning of the lambda is the primary location to leave a TODO comment for potential performance hotspot.
Replace the three TODO stubs (union, intersection, difference) with a working implementation that decomposes both input polygons into trapezoids, collects critical Y-values including cross-polygon edge crossings, then sweeps X-intervals per band to emit result trapezoids. Add comprehensive tests covering overlapping squares, containment, identical polygons, shared edges, commutativity, concave polygons, and CCW winding order verification.
b46771d to
bc7ca7d
Compare
|
@yungyuc I am faster. 🥇 |
|
@tigercosmos Issue #694 is created for you. |
Implement Polygon Boolean Operations (Union, Intersection, Difference)
Summary
Implement polygon boolean operations using trapezoidal decomposition + Y-band sweep with X-interval merge. This approach is based on the algorithm described in:
The three operations —
boolean_union,boolean_intersection, andboolean_difference— are implemented as a single unified algorithm parameterized by a boolean predicate.Visualization
The visualization code is not included in this PR and is provided only for reference.
Order (left to right, top to down): original outlined polygonss, original colored polygons, the union reseult, the intersection result, the difference result.
Algorithm web explaination
Please use browser to read the web page:
https://gist.github.com/tigercosmos/4a358accfae620ada14a5ffa0776c668
Algorithm Overview
The algorithm has two phases:
TrapezoidalDecomposerPhase 2 in Detail
Boolean Predicates
count_p1 > 0 OR count_p2 > 0count_p1 > 0 AND count_p2 > 0count_p1 > 0 AND count_p2 == 0Worked Example
Two overlapping squares: P1 = (0,0)-(2,2), P2 = (1,1)-(3,3).
Y-values: {0, 1, 2, 3} → 3 bands.
Band [1, 2] event sweep:
Handling Slanted Edges
Each event stores two X-values — one at band bottom, one at band top — because polygon edges can be slanted. The emitted result is a general trapezoid with four corners:
The bottom and top edges may have different widths, naturally handling triangles and trapezoids.
Edge-Crossing Y-Split
When trapezoid edges from different polygons cross within a Y-band, the crossing Y-value is detected and inserted into the critical Y-values set. This subdivides the band so that no two cross-polygon edges swap order within a single sub-band, ensuring correctness.
Key Design Decisions
TrapezoidalDecomposer— no new decomposition code neededcompute_boolean_with_decomposition) parameterized by aBoolOpenumTrapezoidPadindices and asource_of()lambdaFiles Changed
cpp/modmesh/universe/polygon.hppdetail::BoolOpenum,detail::compute_boolean_with_decomposition()function,decomposed_trapezoids()accessor, implementedAreaBooleanUnion/Intersection/Difference::compute()tests/test_polygon3d.pyTest Plan
float32andfloat64AI Usage
Used Claude Code with Opus 4.6 to implement the draft. Used Codex with 5.3-codex high effort for review. Fine-tuned the implementation manually with both Claude Code and Codex support. Used AI to generate all documents and the PR description.