Implement intersection #4
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 pull request adds comprehensive unit tests for geometric intersection operations and extends the triangulation module with new structures and algorithms for segment intersections. The changes include the introduction of functions for collinearity and intersection detection, as well as supporting types for event handling and ordered pairs. Additionally, the public interface is expanded by exposing the intersection module, and the point module is enhanced with new creation methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IntersectionModule as Intersection
participant EventQueue as SortedEvents
participant ActiveSet
Client->>IntersectionModule: call find_intersections(segments)
IntersectionModule->>EventQueue: generate and sort events
EventQueue-->>IntersectionModule: return sorted event list
IntersectionModule->>ActiveSet: process events (sweep-line)
ActiveSet-->>IntersectionModule: report intersection checks
IntersectionModule->>Client: return set of intersecting segment pairs
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 (
|
|
@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: 0
🧹 Nitpick comments (10)
crates/triangulation/src/intersection.rs (1)
338-424: Sweep-line event processing may be suboptimal for large input sizes.
Currently, all active segments are checked for intersection with new tops in O(n²) fashion at worst. For performance on large inputs, consider storing active segments in a balanced data structure keyed by their x-position on the sweep line.crates/triangulation/src/path_triangulation.rs (1)
48-48: Disambiguation ofVectoris clear.
Now thatmitterhas an explicitpoint::Vectortype, it’s consistent with the rest of the code. Alternatively, you coulduse point::Vector;at the file level for brevity.crates/bermuda/tests/intersection.rs (6)
6-20: Consider adding edge cases for collinearity tests.While the current test cases are good, consider adding edge cases such as:
- Points with very large coordinates
- Points with very small differences
- Points with negative coordinates
#[rstest] #[case(Point::new(0.0, 0.0), Point::new(0.5, 0.5), Point::new(1.0, 1.0), true)] #[case(Point::new(0.0, 0.0), Point::new(0.0, 0.5), Point::new(0.0, 1.0), true)] #[case(Point::new(0.0, 0.0), Point::new(0.5, 0.0), Point::new(1.0, 0.0), true)] #[case(Point::new_i(0, 0), Point::new_i(1, 1), Point::new(0.5, 0.5), false)] #[case(Point::new_i(0, 0), Point::new_i(0, 1), Point::new(0.0, 0.5), false)] #[case(Point::new_i(0, 0), Point::new_i(1, 0), Point::new(0.5, 0.0), false)] +#[case(Point::new(1e6, 1e6), Point::new(2e6, 2e6), Point::new(3e6, 3e6), true)] +#[case(Point::new(0.0, 0.0), Point::new(0.0001, 0.0001), Point::new(0.0002, 0.0002), true)] +#[case(Point::new(-1.0, -1.0), Point::new(-2.0, -2.0), Point::new(-3.0, -3.0), true)]
22-30: Add more orientation test cases for better coverage.Consider adding test cases for:
- Points forming a right angle
- Points with equal x or y coordinates but not collinear
- Points with floating-point precision edge cases
#[rstest] #[case(Point::new(0.0, 0.0), Point::new(0.0, 1.0), Point::new(0.0, 2.0), 0)] #[case(Point::new(0.0, 0.0), Point::new(0.0, 2.0), Point::new(0.0, 1.0), 0)] #[case(Point::new(0.0, 2.0), Point::new(0.0, 0.0), Point::new(0.0, 1.0), 0)] #[case(Point::new(0.0, 0.0), Point::new(0.0, 1.0), Point::new(1.0, 2.0), 1)] #[case(Point::new(0.0, 0.0), Point::new(0.0, 1.0), Point::new(-1.0, 2.0), 2)] +#[case(Point::new(0.0, 0.0), Point::new(1.0, 0.0), Point::new(1.0, 1.0), 1)] // Right angle +#[case(Point::new(1.0, 0.0), Point::new(1.0, 1.0), Point::new(1.0, -1.0), 0)] // Same x, not collinear +#[case(Point::new(0.0, 0.0), Point::new(0.0001, 0.0001), Point::new(-0.0001, 0.0001), 2)] // Precision case
32-70: Enhance intersection test coverage.Consider adding test cases for:
- Segments that share an endpoint
- Segments with floating-point coordinates near the intersection point
- Segments that barely intersect or barely miss
#[rstest] #[case(Segment::new_i((0, 0), (1, 1)), Segment::new_i((1, 0), (0, 1)))] #[case(Segment::new_i((1, 0), (0, 1)), Segment::new_i((0, 0), (1, 1)))] +#[case(Segment::new_f((0.0, 0.0), (1.0, 1.0)), Segment::new_f((1.0, 1.0), (2.0, 0.0)))] // Shared endpoint +#[case(Segment::new_f((0.0, 0.0), (1.0, 1.0)), Segment::new_f((0.999, 0.0), (0.001, 1.0)))] // Near intersection +#[case(Segment::new_f((0.0, 0.0), (1.0, 1.0)), Segment::new_f((1.001, 0.0), (-0.001, 1.0)))] // Barely miss
72-97: Add more robust intersection point test cases.Consider adding test cases for:
- Segments intersecting at non-integer coordinates
- Segments intersecting near their endpoints
- Segments with very small/large coordinates
#[rstest] #[case(Segment::new_i((0, 0), (2, 2)), Segment::new_i((2, 0), (0, 2)), Point::new_i(1, 1))] #[case(Segment::new_i((0, 0), (1, 0)), Segment::new_i((0, 1), (0, 0)), Point::new_i(0, 0))] #[case(Segment::new_i((0, 0), (2, 0)), Segment::new_i((1, 0), (1, 2)), Point::new_i(1, 0))] +#[case(Segment::new_f((0.0, 0.0), (2.0, 2.0)), Segment::new_f((2.0, 0.0), (0.0, 2.0)), Point::new_f(1.0, 1.0))] +#[case(Segment::new_f((0.0, 0.0), (1.0, 1.0)), Segment::new_f((0.99, 0.0), (0.0, 0.99)), Point::new_f(0.495, 0.495))] +#[case(Segment::new_f((1e6, 1e6), (2e6, 2e6)), Segment::new_f((2e6, 1e6), (1e6, 2e6)), Point::new_f(1.5e6, 1.5e6))]
99-112: Enhance test documentation for better clarity.The ASCII art diagram is helpful. Consider adding:
- Expected behavior explanation
- Edge case considerations
+/// Tests a simple square configuration with no intersections. +/// Each segment connects to the next at endpoints, but there +/// are no true intersections between non-adjacent segments. /// (1, 0) --- (1, 1) /// | | /// (0, 0) --- (0, 1)
114-138: Add explicit assertions and improve documentation.While the ASCII art is helpful, consider:
- Documenting why only one intersection is expected
- Adding assertions for the intersection point coordinates
+/// Tests a configuration with two intersecting diagonals. +/// Expected behavior: +/// - Only one intersection is recorded between segments 1 and 3 +/// - The intersection occurs at (0.5, 0.5) /// (1, 0) --- (1, 1) /// \ / /// \ / /// \ / /// X /// / \ /// / \ /// / \ /// (0, 0) --- (0, 1).github/workflows/wheels.yml (1)
53-61: Enhance cargo test job configuration.Consider adding:
- Rust toolchain setup
- Caching for faster builds
- Test result reporting
cargo-test: name: Run cargo tests needs: pre-commit runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + - uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: stable + - uses: Swatinem/rust-cache@v2 - name: run tests - run: cargo test + run: cargo test --all-features + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: cargo-test-results + path: target/debug/deps/crates/triangulation/src/point.rs (1)
192-198: Consider simplifying the constructor API.While both methods provide convenience,
new_fappears redundant as it's essentially equivalent to usingnewwith tuple destructuring. Consider:
- Keeping only
new_ifor the integer-to-float conversion use case- Documenting that
newcan be used directly with floating-point tuplesExample usage with
new:let segment = Segment::new( Point::new(p1.0, p1.1), Point::new(p2.0, p2.1) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/wheels.yml(1 hunks)Cargo.toml(1 hunks)crates/bermuda/Cargo.toml(1 hunks)crates/bermuda/tests/intersection.rs(1 hunks)crates/triangulation/src/intersection.rs(1 hunks)crates/triangulation/src/lib.rs(1 hunks)crates/triangulation/src/path_triangulation.rs(1 hunks)crates/triangulation/src/point.rs(3 hunks)
🔇 Additional comments (18)
crates/triangulation/src/intersection.rs (10)
1-5: All imports look good.
No issues spotted with these imports.
6-17: Caution with float-based equality inEvent.
#[derive(PartialEq, Eq)]on a struct containingf32/f64fields can lead to unexpected behavior if floating-point rounding issues arise. Consider verifying whether exact float comparisons are acceptable for your use case (especially in geometry).
19-39: Float ordering inEvent::cmpmay introduce subtle bugs.
Whenself.pis extremely close toother.pbut not exactly the same, it’s possible that float rounding or precision issues could lead to inconsistent ordering. If robust geometric algorithms are needed, consider a tolerance-based comparison (e.g., an epsilon check).
41-46:EventDatastruct is straightforward.
Storing separatetopsandbottomslists is an efficient design for sweep line events.
47-66:OrderedPairimplementation looks good.
Maintains consistent ordering of indices and overrides bothOrdandPartialOrd. No concerns here.
67-102:on_segment_if_collinearis clear and well-documented.
The bounding box check aligns with the documented assumption that collinearity is already established.
104-153: Exact float comparisons inorientationmight be risky.
Using==on floating-point differences could fail for points that are nearly collinear but differ by a tiny rounding error. Consider an epsilon-based check for more robust geometry.
155-214: Intersection check approach is standard and well-structured.
No issues spotted. The collinearity checks withon_segment_if_collinearare consistent.
216-248:share_endpointdepends on exact equality.
It relies on exact coordinate matching for endpoints. If two endpoints are "nearly" identical but differ by floating-point drift, they won't match. Confirm that strict equality is desired in your geometry context.
250-337: Potential confusion with the clamping logic infind_intersection.
Whent < 0.0ort > 1.0, returnings1.topors1.bottommight be unexpected if you only want intersection points strictly within the segment. Re-check whether you should return an empty vector instead of force-clamping.crates/triangulation/src/lib.rs (1)
1-1: Publicintersectionmodule export looks good.
Consider adding a top-level doc comment on this module to guide users to intersection functionalities.crates/bermuda/tests/intersection.rs (2)
1-5: LGTM! Well-structured test setup.Good choice using rstest for parameterized testing and HashSet for storing unique intersections.
140-196: LGTM! Comprehensive parameterized test cases.Excellent coverage of various intersection scenarios with clear test case names and expected outcomes.
Cargo.toml (1)
17-17: LGTM! Appropriate dependency addition.The rstest dependency is correctly added to the workspace dependencies.
crates/bermuda/Cargo.toml (1)
16-18: LGTM! Correct dev-dependency configuration.Good practice using workspace inheritance for the rstest dependency and properly categorizing it as a dev-dependency.
crates/triangulation/src/point.rs (3)
17-19: LGTM! Clean implementation of integer-based Point constructor.The
new_imethod provides a convenient way to create points from integer coordinates while maintaining type safety through explicit casting.
17-19: Ensure test coverage for new constructors.Please add tests to verify:
- Integer to float conversion accuracy in
Point::new_i- Ordering preservation in
Segment::new_i(ensuring bottom/top points are correctly ordered)- Edge cases like maximum/minimum integer values
Would you like me to help generate comprehensive test cases for these new constructors?
Also applies to: 192-198
7-7: Verify floating-point comparison behavior with derived PartialEq.The derived
PartialEqimplementation forPointdirectly comparesf32fields, which might lead to unexpected results due to floating-point precision issues. Consider implementing a customPartialEqwith an epsilon-based comparison if precise equality checks are required.✅ Verification successful
Derived PartialEq implementation is acceptable for current usage
While floating-point comparisons can be problematic, the current codebase shows no evidence of critical Point equality comparisons or precision-related issues. The derived implementation is sufficient for the existing use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any existing tests that verify Point equality with near-equal values # Search for Point equality tests rg -A 5 "assert.*eq.*Point" # Search for floating-point comparison tests ast-grep --pattern 'fn $test_name() { $$$ assert!($expr == $expr); $$$ }'Length of output: 705
Script:
#!/bin/bash # Search for test modules and custom PartialEq implementations rg -t rust "impl.*PartialEq.*Point" -A 5 rg -t rust "#\[test\]" -B 2 -A 10 crates/triangulation/src/point.rsLength of output: 112
Script:
#!/bin/bash # Search for Point equality comparisons in code rg -t rust "Point.*==|==.*Point"Length of output: 34
crates/triangulation/src/point.rs
Outdated
There was a problem hiding this comment.
The new_i (and new_f below) functions in here seem a bit off to me. I don't have a much better idea though as most ways to make this generic will become complicated. In general though I'd probably prefer a user needs to call with Point::new(0f32, 0f32) instead of adding new_i. Anyway, this is a nitpick.
There was a problem hiding this comment.
I have added them to make the test definition more compact. Otherwise, parametrization takes too much. But maybe I should add a helper function in the test definition?
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct OrderedPair(point::Index, point::Index); |
There was a problem hiding this comment.
Do the fields here need to be public for this to be useful? If so, I would also create accessors so you can't instantiate an invalid OrderedPair (i.e. without calling new. You can also encapsulate this in a module to prevent invalid construction even within this module, for example.
There was a problem hiding this comment.
No they dont.
I still have problem with understand rust field visibility.
https://doc.rust-lang.org/rust-by-example/mod/struct_visibility.html
Did I correctly understand that these fields are private and accessors will be needed in the next phase when I will use intersections for splitting paths?
There was a problem hiding this comment.
Yes, I think that's correct.
The visibility is likewise confusing to me, but I believe these fields will be private outside this module. So if you have an OrderedPair elsewhere you would not be able to access ordered_pair.0 or ordered_pair.1. I think this is what you want, as it also prevents instantiating like OrderedPair(10, 5) and forces construction via ::new where the order is determined. Then you'd add accessors to be able to use the properties but maintain invariant.
…est` (#6) Created based on #4. To simplify parallel working on multiple PR by setup test infrastructure in a separate PR. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added cargo testing to the GitHub Actions workflow. - Introduced a new function to calculate the Euclidean distance between two points. - **Tests** - Added new test functions for segment ordering and vector addition. - **Chores** - Updated job dependencies in the CI workflow. - Added workspace-level dependency management for testing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@Czaki Do you want to split out the items listed under the coderabbitai Chores section into its own PR? Seems like those could be merged now, and would keep PRs scoped to one subject. |
|
That said, I think this is fine to merge as is too since this is early days and we can iterate. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
crates/triangulation/src/intersection.rs (5)
6-11: Consider reducing field visibility or adding accessors.Currently, the
Eventstruct's fields are all public, which may allow unintended external mutations. Restricting visibility or introducing dedicated methods for updating fields can help maintain invariants.
41-45: Validate whetherEventDatamight benefit from storing additional context.
EventDatatracks only top/bottom indices. If usage grows, consider storing more data or clarifying the distinction betweentopsandbottomsfor improved readability.
75-109: Check for floating-point inaccuracies in collinearity checks.
on_segment_if_collinearrelies onpoint_on_line, which might have floating-point edge cases for very large or very small coordinates. If your domain involves high precision geometry, consider small epsilon comparisons.
171-203: Ensure consistent naming betweendo_share_endpointanddo_intersect.The naming convention is consistent with “do_” prefix. However, for clarity and uniform reading, consider aligning them (e.g.,
share_endpointvs.do_share_endpoint).
306-384: Sweep-line logic is prone to O(n²) complexity.
find_intersectionsperforms an incremental approach using aBTreeMapin reverse. For a large number of segments, this can grow expensive. If performance becomes a bottleneck, you might consider a more optimized sweep line or segment tree approach.crates/bermuda/tests/intersection.rs (2)
17-29: Add tests for near-collinear scenarios with larger floating differences.While you’re already covering various collinear checks, consider adding edge-case tests with slight floating-point deviations to ensure robust coverage of borderline cases.
60-69: Check approach to parallel segments.The test
test_do_intersect_parallel_segmentsverifies non-intersection, but also ensure coverage for parallel, overlapping, or collinear segments. The separate collinear tests exist but adding explicit parallel-overlapping might help maintain clarity.crates/triangulation/src/point.rs (1)
192-198: Consider a more consistent API design for Segment construction.The new tuple-based constructors (
new_iandnew_f) are inconsistent with the main Point-based constructor. Consider these alternatives:
- Use builder pattern
- Use factory methods that maintain consistency with
PointconstructorsExample of a more consistent approach:
impl Segment { // Factory method maintaining Point-based construction pub fn from_integers(x1: i32, y1: i32, x2: i32, y2: i32) -> Self { Self::new(Point::new_i(x1, y1), Point::new_i(x2, y2)) } // Or using builder pattern pub fn builder() -> SegmentBuilder { SegmentBuilder::new() } } pub struct SegmentBuilder { p1: Option<Point>, p2: Option<Point>, } impl SegmentBuilder { pub fn new() -> Self { Self { p1: None, p2: None, } } pub fn with_integers(mut self, x1: i32, y1: i32, x2: i32, y2: i32) -> Self { self.p1 = Some(Point::new_i(x1, y1)); self.p2 = Some(Point::new_i(x2, y2)); self } pub fn build(self) -> Result<Segment, &'static str> { match (self.p1, self.p2) { (Some(p1), Some(p2)) => Ok(Segment::new(p1, p2)), _ => Err("Both points must be set"), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/bermuda/tests/intersection.rs(1 hunks)crates/triangulation/src/intersection.rs(1 hunks)crates/triangulation/src/lib.rs(1 hunks)crates/triangulation/src/point.rs(3 hunks)
🔇 Additional comments (9)
crates/triangulation/src/intersection.rs (4)
19-39: Ensure consistent sorting logic for equal points.When two
Eventinstances have the same point (self.p == other.p), the fallback ordering logic toggles onis_top, then usesindex. Confirm this aligns with all usage scenarios, especially if multiple segments share the same endpoint.
47-73: Confirm thatOrderedPairprohibits invalid index ordering.The constructor enforces a sorted index pair, ensuring consistency. This is good. If you anticipate user-created
OrderedPairoutsidenew, consider making the inner fields private to prevent confusion or invalid states.
110-169: Consider additional bounding-box checks for reliability.
do_intersectuses orientation tests and collinearity checks. Typically, segment intersection also checks bounding boxes to guard against numerical ambiguity or extremely small floating deltas. Confirm this logic suffices for all intended use cases, especially in high-precision or large-scale scenarios.
205-211: Enum structure nicely captures intersection outcomes.The
Intersectionvariants cover typical intersection cases thoroughly. This design choice improves clarity over a numeric or boolean-based approach.crates/triangulation/src/lib.rs (1)
1-1: Publicly exposing theintersectionmodule is well-structured.This addition aligns with your design to make intersection functionalities broadly available. If further features are expected, consider re-exporting commonly used intersection types/functions directly at the crate root for convenience.
crates/bermuda/tests/intersection.rs (2)
6-11: Helper functions for constructing segments improve readability.
seg()andseg_f()are concise and make test code more approachable. Good approach to keep test definitions more readable and consistent.
161-217: Extensive parameterized testing is well-executed.The
test_find_intersections_paramfunction comprehensively verifies multiple intersection scenarios. This broad coverage is beneficial for regression and confidence.crates/triangulation/src/point.rs (2)
7-7: LGTM! Deriving PartialEq is a good choice.Using
#[derive(PartialEq)]is more maintainable than manual implementation.
17-19: Consider potential precision loss in integer to float conversion.The unchecked casting from
i32tof32could lose precision for large integers (beyond ±16,777,216).Additionally, as noted in past reviews, consider whether this convenience method is worth the added API surface. An alternative would be to use
Point::new(0f32, 0f32)directly.
| /// Finds the intersection point of two line segments, if it exists. | ||
| /// | ||
| /// This function calculates the intersection point of two given line segments. | ||
| /// Each segment is defined by two endpoints. If the segments do not intersect, | ||
| /// or are collinear and overlapping, the function returns a vector of the shared points. | ||
| /// If they are collinear and don't overlap, an empty vector is returned. | ||
| /// If they intersect at a single point, the function returns a vector containing that single point. | ||
| /// If the segments are not collinear but intersect, the function returns a vector containing the intersection point. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `s1` - The first line segment. | ||
| /// * `s2` - The second line segment. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// An element of Intersection enum with intersection points | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ``` | ||
| /// use triangulation::point::{Point, Segment}; | ||
| /// use triangulation::intersection::{find_intersection, Intersection}; | ||
| /// | ||
| /// let s1 = Segment::new(Point::new(0.0, 0.0), Point::new(2.0, 2.0)); | ||
| /// let s2 = Segment::new(Point::new(0.0, 2.0), Point::new(2.0, 0.0)); | ||
| /// let intersection = find_intersection(&s1, &s2); | ||
| /// assert_eq!(intersection, Intersection::PointIntersection(Point::new(1.0, 1.0))); // Intersecting segments | ||
| /// | ||
| /// let s3 = Segment::new(Point::new(0.0, 0.0), Point::new(1.0, 1.0)); | ||
| /// let s4 = Segment::new(Point::new(2.0, 2.0), Point::new(3.0, 3.0)); | ||
| /// let intersection = find_intersection(&s3, &s4); | ||
| /// assert!(matches!(intersection, Intersection::CollinearNoOverlap)); // Non-intersecting segments | ||
| /// | ||
| /// let s5 = Segment::new(Point::new(0.0, 0.0), Point::new(2.0, 0.0)); | ||
| /// let s6 = Segment::new(Point::new(1.0, 0.0), Point::new(3.0, 0.0)); | ||
| /// let intersection = find_intersection(&s5, &s6); | ||
| /// assert!(matches!(intersection, Intersection::CollinearWithOverlap(_))); // Overlapping collinear segments | ||
| /// | ||
| /// | ||
| /// ``` | ||
| pub fn find_intersection(s1: &point::Segment, s2: &point::Segment) -> Intersection { | ||
| let a1 = s1.top.y - s1.bottom.y; | ||
| let b1 = s1.bottom.x - s1.top.x; | ||
| let a2 = s2.top.y - s2.bottom.y; | ||
| let b2 = s2.bottom.x - s2.top.x; | ||
| let det = a1 * b2 - a2 * b1; | ||
|
|
||
| if det == 0.0 { | ||
| // collinear case | ||
| let mut res = Vec::new(); | ||
| if s1.point_on_line(s2.bottom) { | ||
| res.push(s2.bottom); | ||
| } | ||
| if s1.point_on_line(s2.top) { | ||
| res.push(s2.top); | ||
| } | ||
| if s2.point_on_line(s1.bottom) { | ||
| res.push(s1.bottom); | ||
| } | ||
| if s2.point_on_line(s1.top) { | ||
| res.push(s1.top); | ||
| } | ||
|
|
||
| // remove duplicates from the collinear intersection case | ||
| res.sort(); | ||
| res.dedup(); | ||
| if res.len() == 0 { | ||
| return Intersection::CollinearNoOverlap; | ||
| } | ||
| if res.len() == 1 { | ||
| return Intersection::PointIntersection(res[0]); | ||
| } | ||
| return Intersection::CollinearWithOverlap(res); | ||
| } | ||
|
|
||
| let t = ((s2.top.x - s1.top.x) * (s2.bottom.y - s2.top.y) | ||
| - (s2.top.y - s1.top.y) * (s2.bottom.x - s2.top.x)) | ||
| / det; | ||
|
|
||
| // clip to handle problems with floating point precision | ||
| if t < 0.0 { | ||
| return Intersection::PointIntersection(s1.top); | ||
| } | ||
| if t > 1.0 { | ||
| return Intersection::PointIntersection(s1.bottom); | ||
| } | ||
|
|
||
| let x = s1.top.x + t * b1; | ||
| let y = s1.top.y + t * (-a1); | ||
| Intersection::PointIntersection(point::Point { x, y }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate the parameter clamping in find_intersection.
For out-of-range intersections, the code returns PointIntersection(s1.top) if t < 0.0, or PointIntersection(s1.bottom) if t > 1.0. This might conflate “no intersection” with projecting the line onto endpoints. Ensure that this behavior matches your intended geometry logic. Otherwise, consider returning NoIntersection in such cases.
There was a problem hiding this comment.
This is to handle edge cases where t is out of range (0,1) because of float calculation precision.
There was a problem hiding this comment.
@Czaki Thank you for the clarification about handling floating-point precision edge cases. That makes perfect sense.
✏️ Learnings added
Learnt from: Czaki
PR: napari/bermuda#4
File: crates/triangulation/src/intersection.rs:213-305
Timestamp: 2025-02-04T15:07:30.308Z
Learning: In geometric intersection calculations, parameter clamping (e.g., clamping `t` to [0,1] in line-segment intersection) is used to handle floating-point precision edge cases where the parameter might be slightly out of range due to numerical instability.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Implement intersection module
Setup rust test for basic utility
Summary by CodeRabbit
New Features
Tests