Merge pull request #744 from hannobraun/reverse

Clean up and consolidate face reversal code

crates/fj-kernel/Cargo.toml

@@ -32,3 +32,4 @@ path = "../fj-math"
 
 [dev-dependencies]
 anyhow = "1.0.58"
+pretty_assertions = "1.2.1"

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

@@ -4,6 +4,7 @@
 //! on their respective purpose.
 
 mod approx;
+mod reverse;
 mod sweep;
 mod transform;
 mod triangulate;
@@ -12,6 +13,7 @@ pub mod intersection;
 
 pub use self::{
     approx::{CycleApprox, FaceApprox, InvalidTolerance, Tolerance},
+    reverse::reverse_face,
     sweep::sweep,
     transform::transform_shape,
     triangulate::triangulate,

crates/fj-kernel/src/algorithms/reverse.rs

@@ -0,0 +1,82 @@
+use crate::{
+    objects::{Cycle, CyclesInFace, Edge, Face},
+    shape::LocalForm,
+};
+
+/// Reverse the direction of a face
+pub fn reverse_face(face: &Face) -> Face {
+    let face = match face {
+        Face::Face(face) => face,
+        Face::Triangles(_) => {
+            panic!("Reversing tri-rep faces is not supported")
+        }
+    };
+
+    let surface = face.surface().reverse();
+
+    let exteriors = reverse_local_coordinates_in_cycle(&face.exteriors);
+    let interiors = reverse_local_coordinates_in_cycle(&face.interiors);
+
+    Face::new(
+        surface,
+        exteriors.as_local_form().cloned(),
+        interiors.as_local_form().cloned(),
+        face.color,
+    )
+}
+
+fn reverse_local_coordinates_in_cycle(cycles: &CyclesInFace) -> CyclesInFace {
+    let cycles = cycles.as_local_form().map(|cycle| {
+        let edges = cycle
+            .local()
+            .edges
+            .iter()
+            .map(|edge| {
+                let curve = LocalForm::new(
+                    // This is wrong. We have reversed the direction of the
+                    // surface, thereby modifying its coordinate system. So we
+                    // can't just use the local form of the curve, which is
+                    // expressed in surface coordinates, as-is.
+                    //
+                    // This is a coherence issue, but since coherence validation
+                    // is not complete, and the whole local form stuff is still
+                    // a work in progress, this doesn't lead to any observable
+                    // bugs.
+                    *edge.local().curve.local(),
+                    edge.local().curve.canonical(),
+                );
+                let vertices = edge.local().vertices.clone().map(|vertex| {
+                    LocalForm::new(*vertex.local(), vertex.canonical())
+                });
+                let local = Edge { curve, vertices };
+                LocalForm::new(local, edge.canonical())
+            })
+            .collect();
+        let local = Cycle { edges };
+        LocalForm::new(local, cycle.canonical())
+    });
+
+    CyclesInFace::new(cycles)
+}
+
+#[cfg(test)]
+mod tests {
+    use pretty_assertions::assert_eq;
+
+    use crate::objects::{Face, Surface};
+
+    #[test]
+    fn reverse_face() {
+        let original = Face::builder(Surface::xy_plane())
+            .with_exterior_polygon([[0., 0.], [1., 0.], [0., 1.]])
+            .build();
+
+        let reversed = super::reverse_face(&original);
+
+        let expected = Face::builder(Surface::xy_plane().reverse())
+            .with_exterior_polygon([[0., 0.], [1., 0.], [0., -1.]])
+            .build();
+
+        assert_eq!(expected, reversed);
+    }
+}

crates/fj-kernel/src/algorithms/sweep.rs

@@ -2,13 +2,11 @@ use fj_math::{Point, Scalar, Transform, Triangle, Vector};
 
 use crate::{
     iter::ObjectIters,
-    objects::{
-        Curve, Cycle, CyclesInFace, Edge, Face, Surface, Vertex, VerticesOfEdge,
-    },
+    objects::{Curve, Cycle, Edge, Face, Surface, Vertex, VerticesOfEdge},
     shape::LocalForm,
 };
 
-use super::{transform::transform_face, CycleApprox, Tolerance};
+use super::{reverse_face, transform::transform_face, CycleApprox, Tolerance};
 
 /// Create a solid by sweeping a sketch
 pub fn sweep(
@@ -61,24 +59,12 @@ fn create_bottom_faces(
     is_sweep_along_negative_direction: bool,
     target: &mut Vec<Face>,
 ) {
-    let mut surface = face.surface();
-
-    let mut exteriors = face.brep().exteriors.clone();
-    let mut interiors = face.brep().interiors.clone();
-
-    if !is_sweep_along_negative_direction {
-        surface = surface.reverse();
-
-        exteriors = reverse_local_coordinates_in_cycle(&exteriors);
-        interiors = reverse_local_coordinates_in_cycle(&interiors);
+    let face = if is_sweep_along_negative_direction {
+        face.clone()
+    } else {
+        reverse_face(face)
     };
 
-    let face = Face::new(
-        surface,
-        exteriors.as_local_form().cloned(),
-        interiors.as_local_form().cloned(),
-        face.color(),
-    );
     target.push(face);
 }
 
@@ -89,63 +75,15 @@ fn create_top_face(
     target: &mut Vec<Face>,
 ) {
     let translation = Transform::translation(path);
-    let face = transform_face(face, &translation);
-
-    let mut surface = face.surface();
-
-    let mut exteriors = face.brep().exteriors.clone();
-    let mut interiors = face.brep().interiors.clone();
+    let mut face = transform_face(face, &translation);
 
     if is_sweep_along_negative_direction {
-        surface = surface.reverse();
-
-        exteriors = reverse_local_coordinates_in_cycle(&exteriors);
-        interiors = reverse_local_coordinates_in_cycle(&interiors);
+        face = reverse_face(&face);
     };
 
-    let face = Face::new(
-        surface,
-        exteriors.as_local_form().cloned(),
-        interiors.as_local_form().cloned(),
-        face.color(),
-    );
     target.push(face);
 }
 
-fn reverse_local_coordinates_in_cycle(cycles: &CyclesInFace) -> CyclesInFace {
-    let cycles = cycles.as_local_form().map(|cycle| {
-        let edges = cycle
-            .local()
-            .edges
-            .iter()
-            .map(|edge| {
-                let curve = LocalForm::new(
-                    // This is wrong. We have reversed the direction of the
-                    // surface, thereby modifying its coordinate system. So we
-                    // can't just use the local form of the curve, which is
-                    // expressed in surface coordinates, as-is.
-                    //
-                    // This is a coherence issue, but since coherence validation
-                    // is not complete, and the whole local form stuff is still
-                    // a work in progress, this doesn't lead to any observable
-                    // bugs.
-                    *edge.local().curve.local(),
-                    edge.local().curve.canonical(),
-                );
-                let vertices = edge.local().vertices.clone().map(|vertex| {
-                    LocalForm::new(*vertex.local(), vertex.canonical())
-                });
-                let local = Edge { curve, vertices };
-                LocalForm::new(local, edge.canonical())
-            })
-            .collect();
-        let local = Cycle { edges };
-        LocalForm::new(local, cycle.canonical())
-    });
-
-    CyclesInFace::new(cycles)
-}
-
 fn create_non_continuous_side_face(
     path: Vector<3>,
     is_sweep_along_negative_direction: bool,
@@ -283,8 +221,8 @@ mod tests {
     fn bottom_positive() -> anyhow::Result<()> {
         test_bottom_top(
             [0., 0., 1.],
-            [[0., 0., 0.], [-1., 0., 0.], [0., 1., 0.]],
-            [[0., 0.], [-1., 0.], [0., 1.]],
+            [[0., 0., 0.], [1., 0., 0.], [0., -1., 0.]],
+            [[0., 0.], [1., 0.], [0., -1.]],
         )
     }
 
@@ -310,8 +248,8 @@ mod tests {
     fn top_negative() -> anyhow::Result<()> {
         test_bottom_top(
             [0., 0., -1.],
-            [[0., 0., -1.], [-1., 0., -1.], [0., 1., -1.]],
-            [[0., 0.], [-1., 0.], [0., 1.]],
+            [[0., 0., -1.], [1., 0., -1.], [0., -1., -1.]],
+            [[0., 0.], [1., 0.], [0., -1.]],
         )
     }
 

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

@@ -119,7 +119,7 @@ impl SweptCurve {
     /// Create a new instance that is reversed
     #[must_use]
     pub fn reverse(mut self) -> Self {
-        self.curve = self.curve.reverse();
+        self.path = -self.path;
         self
     }
 
@@ -174,13 +174,35 @@ impl SweptCurve {
 
 #[cfg(test)]
 mod tests {
-
     use fj_math::{Line, Point, Vector};
+    use pretty_assertions::assert_eq;
 
     use crate::objects::Curve;
 
     use super::SweptCurve;
 
+    #[test]
+    fn reverse() {
+        let original = SweptCurve {
+            curve: Curve::Line(Line {
+                origin: Point::from([1., 0., 0.]),
+                direction: Vector::from([0., 2., 0.]),
+            }),
+            path: Vector::from([0., 0., 3.]),
+        };
+
+        let reversed = original.reverse();
+
+        let expected = SweptCurve {
+            curve: Curve::Line(Line {
+                origin: Point::from([1., 0., 0.]),
+                direction: Vector::from([0., 2., 0.]),
+            }),
+            path: Vector::from([0., 0., -3.]),
+        };
+        assert_eq!(expected, reversed);
+    }
+
     #[test]
     fn point_to_surface_coords() {
         let plane = SweptCurve {