Merge pull request #1158 from hannobraun/face

Validate winding of interior cycles of `Face`

crates/fj-interop/src/mesh.rs

@@ -5,6 +5,7 @@ use std::{collections::HashMap, hash::Hash};
 use fj_math::Point;
 
 /// A triangle mesh
+#[derive(Debug)]
 pub struct Mesh<V> {
     vertices: Vec<V>,
     indices: Vec<Index>,

crates/fj-kernel/src/algorithms/intersect/curve_face.rs

@@ -184,9 +184,9 @@ mod tests {
         #[rustfmt::skip]
         let interior = [
             [-1., -1.],
-            [ 1., -1.],
-            [ 1.,  1.],
             [-1.,  1.],
+            [ 1.,  1.],
+            [ 1., -1.],
         ];
 
         let face = Face::builder(&stores, surface)

crates/fj-kernel/src/algorithms/triangulate/mod.rs

@@ -129,9 +129,9 @@ mod tests {
         let d = [0., 4.];
 
         let e = [1., 1.];
-        let f = [3., 1.];
+        let f = [1., 2.];
         let g = [3., 3.];
-        let h = [1., 2.];
+        let h = [3., 1.];
 
         let surface = Surface::xy_plane();
         let face = Face::builder(&stores, surface)
@@ -141,17 +141,24 @@ mod tests {
 
         let triangles = triangulate(face)?;
 
-        let a = Point::from(a).to_xyz();
-        let d = Point::from(d).to_xyz();
-        let e = Point::from(e).to_xyz();
-        let f = Point::from(f).to_xyz();
-        let g = Point::from(g).to_xyz();
-        let h = Point::from(h).to_xyz();
+        let a = surface.point_from_surface_coords(a);
+        let b = surface.point_from_surface_coords(b);
+        let e = surface.point_from_surface_coords(e);
+        let f = surface.point_from_surface_coords(f);
+        let g = surface.point_from_surface_coords(g);
+        let h = surface.point_from_surface_coords(h);
 
-        // Should contain some triangles from the polygon. Don't need to test
-        // them all.
-        assert!(triangles.contains_triangle([a, e, h]));
-        assert!(triangles.contains_triangle([a, d, h]));
+        // Let's test that some correct triangles are present. We don't need to
+        // test them all.
+        //
+        // Please note that there are multiple valid triangulations of any given
+        // polygon. So if you change the input data above, or the algorithm, the
+        // following assertions might break.
+        //
+        // This limits the usefulness of this test. It would be better to have a
+        // smarter way of verifying the triangulation.
+        assert!(triangles.contains_triangle([a, b, e]));
+        assert!(triangles.contains_triangle([b, e, h]));
 
         // Shouldn't contain any possible triangle from the hole.
         assert!(!triangles.contains_triangle([e, f, g]));
@@ -179,12 +186,12 @@ mod tests {
         //   a ---------- b
         //
 
-        let a = Point::from([0., 0.]);
-        let b = Point::from([0.4, 0.]);
-        //let b = Point::from([0.5, 0.]); // test passes with this change
-        let c = Point::from([0.4, 1.0]);
-        let d = Point::from([0.1, 0.1]);
-        let e = Point::from([0., 0.8]);
+        let a = [0., 0.];
+        let b = [0.4, 0.];
+        //let b = [0.5, 0.]; // test passes with this change
+        let c = [0.4, 1.0];
+        let d = [0.1, 0.1];
+        let e = [0., 0.8];
 
         let surface = Surface::xy_plane();
         let face = Face::builder(&stores, surface)
@@ -193,11 +200,11 @@ mod tests {
 
         let triangles = triangulate(face)?;
 
-        let a3 = a.to_xyz();
-        let b3 = b.to_xyz();
-        let c3 = c.to_xyz();
-        let d3 = d.to_xyz();
-        let e3 = e.to_xyz();
+        let a3 = surface.point_from_surface_coords(a);
+        let b3 = surface.point_from_surface_coords(b);
+        let c3 = surface.point_from_surface_coords(c);
+        let d3 = surface.point_from_surface_coords(d);
+        let e3 = surface.point_from_surface_coords(e);
 
         assert!(triangles.contains_triangle([a3, b3, d3]));
         assert!(triangles.contains_triangle([b3, c3, d3]));

crates/fj-kernel/src/objects/cycle.rs

@@ -95,6 +95,10 @@ impl Cycle {
     }
 
     /// Indicate the cycle's winding, assuming a right-handed coordinate system
+    ///
+    /// Please note that this is not *the* winding of the cycle, only one of the
+    /// two possible windings, depending on the direction you look at the
+    /// surface that the cycle is defined on from.
     pub fn winding(&self) -> Winding {
         // The cycle could be made up of one or two circles. If that is the
         // case, the winding of the cycle is determined by the winding of the

crates/fj-kernel/src/objects/face.rs

@@ -8,6 +8,29 @@ use crate::{builder::FaceBuilder, stores::Stores};
 use super::{Cycle, Surface};
 
 /// A face of a shape
+///
+/// A `Face` is a bounded area of a [`Surface`], the [`Surface`] itself being an
+/// infinite 2-dimensional object in 3D space. `Face`s are bound by one exterior
+/// cycle, which defines the outer boundary, and an arbitrary number of interior
+/// cycles (i.e. holes).
+///
+/// `Face` has a defined orientation, a front and a back side. When faces are
+/// combined into [`Shell`]s, the face orientation defines what is inside and
+/// outside of the shell. This stands in contrast to [`Surface`], which has no
+/// defined orientation.
+///
+/// You can look at a `Face` from two directions: front and back. The winding of
+/// the exterior cycle will be clockwise or counter-clockwise, depending on your
+/// perspective. The front side of the face, is the side where from which the
+/// exterior cycle appear counter-clockwise.
+///
+/// Interior cycles must have the opposite winding of the exterior cycle,
+/// meaning on the front side of the face, they must appear clockwise. This
+/// means that all [`HalfEdge`]s that bound a `Face` have the interior of the
+/// face on their left side (on the face's front side).
+///
+/// [`HalfEdge`]: super::HalfEdge
+/// [`Shell`]: super::Shell
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Face {
     surface: Surface,
@@ -47,18 +70,26 @@ impl Face {
     /// # Panics
     ///
     /// Panics, if the added cycles are not defined in the face's surface.
+    ///
+    /// Panics, if the winding of the interior cycles is not opposite that of
+    /// the exterior cycle.
     pub fn with_interiors(
         mut self,
         interiors: impl IntoIterator<Item = Cycle>,
     ) -> Self {
-        for cycle in interiors.into_iter() {
+        for interior in interiors.into_iter() {
             assert_eq!(
                 self.surface(),
-                cycle.surface(),
+                interior.surface(),
                 "Cycles that bound a face must be in face's surface"
             );
+            assert_ne!(
+                self.exterior().winding(),
+                interior.winding(),
+                "Interior cycles must have opposite winding of exterior cycle"
+            );
 
-            self.interiors.push(cycle);
+            self.interiors.push(interior);
         }
 
         self
@@ -77,7 +108,7 @@ impl Face {
         &self.surface
     }
 
-    /// Access the cycles that bound the face on the outside
+    /// Access the cycle that bounds the face on the outside
     pub fn exterior(&self) -> &Cycle {
         &self.exterior
     }
@@ -108,7 +139,7 @@ impl Face {
     ///
     /// Faces *do* have an orientation, meaning they have definite front and
     /// back sides. The front side is the side, where the face's exterior cycle
-    /// is wound clockwise.
+    /// is wound counter-clockwise.
     pub fn coord_handedness(&self) -> Handedness {
         match self.exterior().winding() {
             Winding::Ccw => Handedness::RightHanded,