Merge pull request #453 from hannobraun/triangle

Fix triangulation edge case

fj-kernel/src/algorithms/triangulation/mod.rs

@@ -57,7 +57,7 @@ pub fn triangulate(
                 let mut triangles = delaunay(points);
                 triangles.retain(|triangle| {
                     face_as_polygon.contains_triangle(
-                        &triangle.map(|point| point.native()),
+                        triangle.map(|point| point.native()),
                         debug_info,
                     )
                 });

fj-kernel/src/algorithms/triangulation/polygon.rs

@@ -12,6 +12,21 @@ pub struct Polygon {
 }
 
 impl Polygon {
+    /// Construct an instance of `Polygon`
+    ///
+    /// # Implementation note
+    ///
+    /// This method takes a `Surface`, but `Polygon` only uses that for
+    /// generating debug info. It might be better, if `Polygon` had a field
+    /// where it stored debug info specific to its algorithm. Then code using
+    /// `Polygon` could access that `Polygon`-specific debug info and translate
+    /// that into `DebugInfo`, as necessary.
+    ///
+    /// This would have the advantage of removing this dependency on `Surface`.
+    /// It would also make the test code a bit cleaner, as it wouldn't have to
+    /// bother with the `DebugInfo` anymore. Also, the `Polygon`-specific debug
+    /// info could potentially be more useful in test code, as a debugging tool
+    /// there.
     pub fn new(surface: Surface) -> Self {
         Self {
             surface,
@@ -46,22 +61,41 @@ impl Polygon {
 
     pub fn contains_triangle(
         &self,
-        &[a, b, c]: &[Point<2>; 3],
+        triangle: [impl Into<Point<2>>; 3],
         debug_info: &mut DebugInfo,
     ) -> bool {
+        let [a, b, c] = triangle.map(Into::into);
+
+        let mut might_be_hole = true;
+
         for edge in [a, b, c, a].windows(2) {
             // This can't panic, as we passed `2` to `windows`. It can be
             // cleaned up a bit, once `array_windows` is stable.
             let edge = Segment::from([edge[0], edge[1]]);
 
-            // If the segment is an edge of the face, we don't need to take a
-            // closer look.
-            if self.contains_edge(edge) {
+            let is_exterior_edge = self.contains_exterior_edge(edge);
+            let is_interior_edge = self.contains_interior_edge(edge);
+
+            // If the triangle edge is not an interior edge of the polygon, we
+            // can rule out that the triangle is identical with a hole in the
+            // polygon.
+            if !is_interior_edge {
+                might_be_hole = false;
+            }
+
+            // If the triangle edge is an edge of the face, we don't need to
+            // take a closer look.
+            if is_exterior_edge || is_interior_edge {
                 continue;
             }
 
             // To determine if the edge is within the polygon, we determine if
             // its center point is in the polygon.
+            //
+            // Since we already checked above, whether the triangle edge is a
+            // polygon edge (and if we reached this point, it isn't), we don't
+            // need to care about the distinction between "inside the polygon"
+            // and "on the polygon boundary".
             if !self.contains_point(edge.center(), debug_info) {
                 // The segment is outside of the face. This means we can throw
                 // away the whole triangle.
@@ -69,15 +103,26 @@ impl Polygon {
             }
         }
 
+        // We haven't rules out that the triangle is a polygon hole. Since we
+        // checked all its edges, this means we now know for certain that is is.
+        if might_be_hole {
+            return false;
+        }
+
         // If we didn't throw away the triangle up till now, this means all its
         // edges are within the face.
         true
     }
 
-    pub fn contains_edge(&self, edge: Segment<2>) -> bool {
+    pub fn contains_exterior_edge(&self, edge: Segment<2>) -> bool {
+        self.exterior.segments().contains(&edge)
+            || self.exterior.segments().contains(&edge.reverse())
+    }
+
+    pub fn contains_interior_edge(&self, edge: Segment<2>) -> bool {
         let mut contains = false;
 
-        for chain in Some(&self.exterior).into_iter().chain(&self.interiors) {
+        for chain in &self.interiors {
             contains |= chain.segments().contains(&edge);
             contains |= chain.segments().contains(&edge.reverse());
         }
@@ -123,26 +168,26 @@ impl Polygon {
                     }
                     (Some(Hit::UpperVertex), Some(Hit::LowerVertex))
                     | (Some(Hit::LowerVertex), Some(Hit::UpperVertex)) => {
-                        // If we're hitting a vertex, only count it if we've hit the
-                        // other kind of vertex right before.
+                        // If we're hitting a vertex, only count it if we've hit
+                        // the other kind of vertex right before.
                         //
-                        // That means, we're passing through the polygon boundary
-                        // at where two edges touch. Depending on the order in which
-                        // edges are checked, we're seeing this as a hit to one
-                        // edge's lower/upper vertex, then the other edge's opposite
-                        // vertex.
+                        // That means, we're passing through the polygon
+                        // boundary at where two edges touch. Depending on the
+                        // order in which edges are checked, we're seeing this
+                        // as a hit to one edge's lower/upper vertex, then the
+                        // other edge's opposite vertex.
                         //
-                        // If we're seeing two of the same vertices in a row, we're
-                        // not actually passing through the polygon boundary. Then
-                        // we're just touching a vertex without passing through
-                        // anything.
+                        // If we're seeing two of the same vertices in a row,
+                        // we're not actually passing through the polygon
+                        // boundary. Then we're just touching a vertex without
+                        // passing through anything.
                         true
                     }
                     (Some(Hit::Parallel), _) => {
-                        // A parallel edge must be completely ignored. Its presence
-                        // won't change anything, so we can treat it as if it
-                        // wasn't there, and its neighbors were connected to each
-                        // other.
+                        // A parallel edge must be completely ignored. Its
+                        // presence won't change anything, so we can treat it as
+                        // if it wasn't there, and its neighbors were connected
+                        // to each other.
                         continue;
                     }
                     _ => {
@@ -180,6 +225,23 @@ mod tests {
 
     use super::Polygon;
 
+    #[test]
+    fn contains_triangle_with_triangular_hole() {
+        let a = [0., 0.];
+        let b = [3., 0.];
+        let c = [0., 3.];
+
+        let d = [1., 1.];
+        let e = [2., 1.];
+        let f = [1., 2.];
+
+        let polygon = Polygon::new(Surface::x_y_plane())
+            .with_exterior(PolyChain::from([a, b, c]).close())
+            .with_interiors([PolyChain::from([d, e, f]).close()]);
+
+        assert!(!polygon.contains_triangle([d, e, f], &mut DebugInfo::new()));
+    }
+
     #[test]
     fn contains_point_ray_hits_vertex_while_passing_outside() {
         let a = [0., 0.];