Implement monotone polygons triangulation #10
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a new monotone polygon module and its triangulation functionality into the project. It adds a suite of tests using the Changes
Sequence Diagram(s)sequenceDiagram
participant TS as Test Suite
participant MP as MonotonePolygon
participant TA as Triangulation Algorithm
TS->>MP: Construct polygon (new_top, add vertices)
TS->>TA: Call triangulate_monotone_polygon(MP)
TA->>MP: Process left/right chains using stack
TA-->>TS: Return vector of PointTriangles
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
crates/triangulation/src/point.rs
Outdated
| @@ -271,6 +272,23 @@ impl Ord for Segment { | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| /// A structure representing a triangle using three indices of its vertices. | |||
|
I will update doc strings tomorrow. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/triangulation/src/monotone_polygon.rs (2)
88-117: Guard against emptystackbefore popping
Inbuild_triangles_opposite_edge, there's a direct call tostack.pop_back().unwrap(), which will panic if thestackis empty. While your logic might guarantee non-empty usage, it may be safer to add a debug assertion or an early return to handle unexpected cases.
239-245: Handle possible emptybottomunwrap
points.push((polygon.bottom.unwrap(), Side::TopOrBottom));here relies onbottombeing set. Ensure that callers checkpolygon.finished()or otherwise guarantee a validbottombefore callingtriangulate_monotone_polygon.Would you like me to add logic that returns an error if
bottomis unset?crates/bermuda/tests/test_monotone_polygon.rs (2)
6-22: Validate orientation assumptions intest_monotone_polygon_simple
Your test confirms the number of triangles and exact vertex arrangement. Consider adding a quick orientation check (e.g., ensuring the triangle is counterclockwise) or a small doc comment indicating that the expected triangle order depends on your reordering logic inPointTriangle.
24-67: Provide additional edge-case tests
These parameterized tests look comprehensive. You might add a test for nearly collinear points or a degenerate polygon to confirm the triangulation logic’s behavior in edge cases.crates/triangulation/src/point.rs (1)
373-373: Retain note about floating-point precision
The updated distance doc is concise, but floating-point operations can be tricky. Consider explicitly mentioning possible floating imprecision or referencing the orientation approach used elsewhere in your docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/bermuda/tests/test_monotone_polygon.rs(1 hunks)crates/triangulation/src/lib.rs(1 hunks)crates/triangulation/src/monotone_polygon.rs(1 hunks)crates/triangulation/src/point.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/triangulation/src/lib.rs
🔇 Additional comments (3)
crates/triangulation/src/monotone_polygon.rs (2)
64-71: Consider verifying chain completeness infinished()
Currently,finished()only checks for the presence of a bottom vertex. If your logic requires non-empty left and right chains as well, consider also verifying thatself.leftandself.rightare populated.
213-221: Double-check comparison logic in line 214
The conditionif polygon.left[left_index] < polygon.right[right_index] { points.push((polygon.right[right_index], Side::Right)); } else { ... }may look reversed for a top-down approach (usually you'd push the smaller point first). If it's intentional, please clarify in a comment to avoid confusion.crates/triangulation/src/point.rs (1)
274-295: Ensure no off-by-one or index misuse inTriangle
TheTrianglestruct references vertices by indices. If these indices come from outside user code or a large vertex list, consider validating them to avoid out-of-bounds issues.
crates/triangulation/src/point.rs
Outdated
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| /// A structure representing a triangle using three [`Point`] as its vertices. | ||
| /// | ||
| /// # Fields | ||
| /// * `p1` - The first vertex of the triangle. | ||
| /// * `p2` - The second vertex of the triangle. | ||
| /// * `p3` - The third vertex of the triangle. | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// use triangulation::point::{Point, PointTriangle}; | ||
| /// | ||
| /// let p1 = Point::new(0.0, 0.0); | ||
| /// let p2 = Point::new(1.0, 0.0); | ||
| /// let p3 = Point::new(0.0, 1.0); | ||
| /// | ||
| /// let triangle = PointTriangle::new(p1, p2, p3); | ||
| /// assert_eq!(triangle.p1, p1); | ||
| /// assert_eq!(triangle.p2, p2); | ||
| /// assert_eq!(triangle.p3, p3); | ||
| /// ``` | ||
| pub struct PointTriangle { | ||
| pub p1: Point, | ||
| pub p2: Point, | ||
| pub p3: Point, | ||
| } | ||
|
|
||
| impl PointTriangle { | ||
| /// Creates a new `PointTriangle`. | ||
| /// | ||
| /// The vertices of the triangle are reordered such that the triangle has a counterclockwise orientation. | ||
| /// | ||
| /// # Arguments | ||
| /// * `p1` - The first vertex of the triangle. | ||
| /// * `p2` - The second vertex of the triangle. | ||
| /// * `p3` - The third vertex of the triangle. | ||
| /// | ||
| /// # Returns | ||
| /// A `PointTriangle` instance with reordered vertices, if needed, to ensure counterclockwise orientation. | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// use triangulation::point::{Point, PointTriangle}; | ||
| /// | ||
| /// let p1 = Point::new(0.0, 0.0); | ||
| /// let p2 = Point::new(1.0, 0.0); | ||
| /// let p3 = Point::new(0.0, 1.0); | ||
| /// | ||
| /// let triangle = PointTriangle::new(p1, p2, p3); | ||
| /// assert_eq!(triangle.p1, p1); | ||
| /// assert_eq!(triangle.p2, p2); | ||
| /// assert_eq!(triangle.p3, p3); | ||
| /// ``` | ||
| pub fn new(p1: Point, p2: Point, p3: Point) -> Self { | ||
| let orientation = (p2.x - p1.x) * (p3.y - p1.y) - (p2.y - p1.y) * (p3.x - p1.x); | ||
| if orientation < 0.0 { | ||
| Self { p1: p3, p2, p3: p1 } | ||
| } else { | ||
| Self { p1, p2, p3 } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle collinearity in PointTriangle::new
When orientation is zero, the triangle remains in its original order. If strictly counterclockwise ordering is a requirement, consider clarifying how collinear points should be handled (e.g., reorder or treat as invalid).
Co-authored-by: Carol Willing <[email protected]>
Next steep for sweeping line triangulation
Summary by CodeRabbit
New Features
Tests