Merge pull request #308 from hannobraun/shape

Integrate `Faces` into `Shape`

src/kernel/algorithms/sweep.rs

@@ -1,8 +1,5 @@
 use crate::{
-    kernel::{
+    kernel::{shape::Shape, topology::faces::Face},
-        shape::Shape,
-        topology::faces::{Face, Faces},
-    },
     math::{Scalar, Transform, Vector},
 };
 
@@ -22,9 +19,9 @@ pub fn sweep_shape(
     let mut top_faces = Vec::new();
     let mut side_faces = Vec::new();
 
-    for face in &original.faces.0 {
+    for face in original.faces().all() {
         bottom_faces.push(face.clone());
-        top_faces.push(transform_face(face, &translation, &mut shape));
+        top_faces.push(transform_face(&face, &translation, &mut shape));
     }
 
     for cycle in original.cycles().all() {
@@ -55,12 +52,15 @@ pub fn sweep_shape(
         side_faces.push(Face::Triangles(side_face));
     }
 
-    let mut faces = Vec::new();
+    for face in bottom_faces {
-    faces.extend(bottom_faces);
+        shape.faces().add((*face).clone());
-    faces.extend(top_faces);
+    }
-    faces.extend(side_faces);
+    for face in top_faces {
-
+        shape.faces().add(face);
-    shape.faces = Faces(faces);
+    }
+    for face in side_faces {
+        shape.faces().add(face);
+    }
 
     shape
 }
@@ -70,7 +70,7 @@ mod tests {
     use crate::{
         kernel::{
             geometry::{surfaces::Swept, Surface},
-            shape::Shape,
+            shape::{handle::Handle, Shape},
             topology::{
                 edges::{Cycle, Edge},
                 faces::Face,
@@ -85,7 +85,7 @@ mod tests {
     fn sweep() {
         let sketch = Triangle::new([[0., 0., 0.], [1., 0., 0.], [0., 1., 0.]]);
 
-        let swept = sweep_shape(
+        let mut swept = sweep_shape(
             sketch.shape,
             Vector::from([0., 0., 1.]),
             Scalar::from_f64(0.),
@@ -95,8 +95,8 @@ mod tests {
         let top_face =
             Triangle::new([[0., 0., 1.], [1., 0., 1.], [0., 1., 1.]]).face;
 
-        assert!(swept.faces.0.contains(&bottom_face));
+        assert!(swept.faces().contains(&bottom_face));
-        assert!(swept.faces.0.contains(&top_face));
+        assert!(swept.faces().contains(&top_face));
 
         // Side faces are not tested, as those use triangle representation. The
         // plan is to start testing them, as they are transitioned to b-rep.
@@ -104,7 +104,7 @@ mod tests {
 
     pub struct Triangle {
         shape: Shape,
-        face: Face,
+        face: Handle<Face>,
     }
 
     impl Triangle {
@@ -134,9 +134,9 @@ mod tests {
                 }],
             };
 
-            shape.faces.0.push(abc.clone());
+            let face = shape.faces().add(abc);
 
-            Self { shape, face: abc }
+            Self { shape, face }
         }
     }
 }

src/kernel/algorithms/transform.rs

@@ -19,12 +19,12 @@ use crate::{
 ///
 /// Addressing the shortcomings in this method probably doesn't make sense,
 /// except as a side effect of addressing the shortcomings of `Shape`.
-pub fn transform_shape(original: &Shape, transform: &Transform) -> Shape {
+pub fn transform_shape(mut original: Shape, transform: &Transform) -> Shape {
     let mut transformed = Shape::new();
 
-    for face in &original.faces.0 {
+    for face in original.faces().all() {
-        let face = transform_face(face, transform, &mut transformed);
+        let face = transform_face(&face, transform, &mut transformed);
-        transformed.faces.0.push(face);
+        transformed.faces().add(face);
     }
 
     transformed

src/kernel/shape/cycles.rs

@@ -29,7 +29,7 @@ impl Cycles<'_> {
     }
 
     /// Access an iterator over all cycles
-    pub fn all(&self) -> impl Iterator<Item = Storage<Cycle>> + '_ {
+    pub fn all(&self) -> impl Iterator<Item = Handle<Cycle>> + '_ {
-        self.cycles.iter().cloned()
+        self.cycles.iter().map(|storage| storage.handle())
     }
 }

src/kernel/shape/faces.rs

@@ -0,0 +1,49 @@
+use crate::{
+    debug::DebugInfo,
+    kernel::topology::faces::Face,
+    math::{Scalar, Triangle},
+};
+
+use super::{
+    handle::{Handle, Storage},
+    FacesInner,
+};
+
+/// The faces of a shape
+pub struct Faces<'r> {
+    pub(super) faces: &'r mut FacesInner,
+}
+
+impl Faces<'_> {
+    /// Add a face to the shape
+    pub fn add(&mut self, face: Face) -> Handle<Face> {
+        let storage = Storage::new(face);
+        let handle = storage.handle();
+
+        self.faces.push(storage);
+
+        handle
+    }
+
+    /// Check whether the shape contains a specific face
+    #[cfg(test)]
+    pub fn contains(&self, face: &Face) -> bool {
+        self.faces.contains(&Storage::new(face.clone()))
+    }
+
+    /// Access an iterator over all faces
+    pub fn all(&self) -> impl Iterator<Item = Handle<Face>> + '_ {
+        self.faces.iter().map(|storage| storage.handle())
+    }
+
+    pub fn triangles(
+        &self,
+        tolerance: Scalar,
+        out: &mut Vec<Triangle<3>>,
+        debug_info: &mut DebugInfo,
+    ) {
+        for face in &*self.faces {
+            face.triangles(tolerance, out, debug_info);
+        }
+    }
+}

src/kernel/shape/mod.rs

@@ -1,21 +1,19 @@
 pub mod cycles;
 pub mod edges;
+pub mod faces;
 pub mod handle;
 pub mod vertices;
 
 use crate::math::Scalar;
 
-use super::topology::{edges::Cycle, faces::Faces, vertices::Vertex};
+use super::topology::{edges::Cycle, faces::Face, vertices::Vertex};
 
-use self::{cycles::Cycles, edges::Edges, handle::Storage, vertices::Vertices};
+use self::{
+    cycles::Cycles, edges::Edges, faces::Faces, handle::Storage,
+    vertices::Vertices,
+};
 
 /// The boundary representation of a shape
-///
-/// # Implementation note
-///
-/// The goal for `Shape` is to enforce full self-consistency, through the API it
-/// provides. Steps have been made in that direction, but right now, the API is
-/// still full of holes, forcing callers to just be careful for the time being.
 #[derive(Clone, Debug)]
 pub struct Shape {
     /// The minimum distance between two vertices
@@ -25,8 +23,7 @@ pub struct Shape {
 
     vertices: VerticesInner,
     cycles: CyclesInner,
-
+    faces: FacesInner,
-    pub faces: Faces,
 }
 
 impl Shape {
@@ -40,7 +37,7 @@ impl Shape {
 
             vertices: VerticesInner::new(),
             cycles: CyclesInner::new(),
-            faces: Faces(Vec::new()),
+            faces: FacesInner::new(),
         }
     }
 
@@ -78,7 +75,15 @@ impl Shape {
             cycles: &mut self.cycles,
         }
     }
+
+    /// Access the shape's faces
+    pub fn faces(&mut self) -> Faces {
+        Faces {
+            faces: &mut self.faces,
+        }
+    }
 }
 
 type VerticesInner = Vec<Storage<Vertex>>;
 type CyclesInner = Vec<Storage<Cycle>>;
+type FacesInner = Vec<Storage<Face>>;

src/kernel/shapes/circle.rs

@@ -5,7 +5,7 @@ use crate::{
         shape::Shape,
         topology::{
             edges::{Cycle, Edge},
-            faces::{Face, Faces},
+            faces::Face,
         },
     },
     math::{Aabb, Point, Scalar},
@@ -25,14 +25,15 @@ impl ToShape for fj::Circle {
             .add(Edge::circle(Scalar::from_f64(self.radius)));
         shape.cycles().add(Cycle { edges: vec![edge] });
 
-        shape.faces = Faces(vec![Face::Face {
+        let cycles = shape
-            cycles: shape
+            .cycles()
-                .cycles()
+            .all()
-                .all()
+            .map(|handle| (*handle).clone())
-                .map(|handle| (*handle).clone())
+            .collect();
-                .collect(),
+        shape.faces().add(Face::Face {
+            cycles,
             surface: Surface::x_y_plane(),
-        }]);
+        });
 
         shape
     }

src/kernel/shapes/difference_2d.rs

@@ -2,10 +2,7 @@ use crate::{
     debug::DebugInfo,
     kernel::{
         shape::Shape,
-        topology::{
+        topology::{edges::Cycle, faces::Face},
-            edges::Cycle,
-            faces::{Face, Faces},
-        },
     },
     math::{Aabb, Scalar},
 };
@@ -41,10 +38,15 @@ impl ToShape for fj::Difference2d {
             );
         }
 
-        shape.faces = {
+        {
-            let (a, b) = if a.faces.0.len() == 1 && b.faces.0.len() == 1 {
+            let (a, b) = if a.faces().all().count() == 1
+                && b.faces().all().count() == 1
+            {
                 // Can't panic. We just checked that length of `a` and `b` is 1.
-                (a.faces.0.pop().unwrap(), b.faces.0.pop().unwrap())
+                (
+                    a.faces().all().next().unwrap(),
+                    b.faces().all().next().unwrap(),
+                )
             } else {
                 // See issue:
                 // https://github.com/hannobraun/Fornjot/issues/95
@@ -54,22 +56,24 @@ impl ToShape for fj::Difference2d {
                 );
             };
 
-            let (a, b, surface_a, surface_b) = match (a, b) {
+            let (a, b, surface_a, surface_b) =
-                (
+                match ((*a).clone(), (*b).clone()) {
-                    Face::Face {
+                    (
-                        cycles: a,
+                        Face::Face {
-                        surface: surface_a,
+                            cycles: a,
-                    },
+                            surface: surface_a,
-                    Face::Face {
+                        },
-                        cycles: b,
+                        Face::Face {
-                        surface: surface_b,
+                            cycles: b,
-                    },
+                            surface: surface_b,
-                ) => (a, b, surface_a, surface_b),
+                        },
-                _ => {
+                    ) => (a, b, surface_a, surface_b),
-                    // None of the 2D types still use the triangles representation.
+                    _ => {
-                    unreachable!()
+                        // None of the 2D types still use triangle
-                }
+                        // representation.
-            };
+                        unreachable!()
+                    }
+                };
 
             assert!(
                 surface_a == surface_b,
@@ -82,7 +86,7 @@ impl ToShape for fj::Difference2d {
             let mut cycles = a;
             cycles.extend(b);
 
-            Faces(vec![Face::Face { cycles, surface }])
+            shape.faces().add(Face::Face { cycles, surface });
         };
 
         shape

src/kernel/shapes/sketch.rs

@@ -5,7 +5,7 @@ use crate::{
         shape::Shape,
         topology::{
             edges::{Cycle, Edge},
-            faces::{Face, Faces},
+            faces::Face,
         },
     },
     math::{Aabb, Point, Scalar},
@@ -55,7 +55,7 @@ impl ToShape for fj::Sketch {
                 .collect(),
             surface: Surface::x_y_plane(),
         };
-        shape.faces = Faces(vec![face]);
+        shape.faces().add(face);
 
         shape
     }

src/kernel/shapes/transform.rs

@@ -12,7 +12,7 @@ impl ToShape for fj::Transform {
     fn to_shape(&self, tolerance: Scalar, debug_info: &mut DebugInfo) -> Shape {
         let shape = self.shape.to_shape(tolerance, debug_info);
         let transform = transform(self);
-        transform_shape(&shape, &transform)
+        transform_shape(shape, &transform)
     }
 
     fn bounding_volume(&self) -> Aabb<3> {

src/kernel/shapes/union.rs

@@ -1,6 +1,6 @@
 use crate::{
     debug::DebugInfo,
-    kernel::{shape::Shape, topology::faces::Faces},
+    kernel::shape::Shape,
     math::{Aabb, Scalar},
 };
 
@@ -10,19 +10,20 @@ impl ToShape for fj::Union {
     fn to_shape(&self, tolerance: Scalar, debug_info: &mut DebugInfo) -> Shape {
         let mut shape = Shape::new();
 
-        let a = self.a.to_shape(tolerance, debug_info).faces;
+        let mut a = self.a.to_shape(tolerance, debug_info);
-        let b = self.b.to_shape(tolerance, debug_info).faces;
+        let mut b = self.b.to_shape(tolerance, debug_info);
 
         // This doesn't create a true union, as it doesn't eliminate, merge, or
         // split faces.
         //
         // See issue:
         // https://github.com/hannobraun/Fornjot/issues/42
-        let mut faces = Vec::new();
+        for face in a.faces().all() {
-        faces.extend(a.0);
+            shape.faces().add((*face).clone());
-        faces.extend(b.0);
+        }
-
+        for face in b.faces().all() {
-        shape.faces = Faces(faces);
+            shape.faces().add((*face).clone());
+        }
 
         shape
     }

src/kernel/topology/faces.rs

@@ -16,23 +16,6 @@ use crate::{
 
 use super::edges::Cycle;
 
-/// The faces of a shape
-#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
-pub struct Faces(pub Vec<Face>);
-
-impl Faces {
-    pub fn triangles(
-        &self,
-        tolerance: Scalar,
-        out: &mut Vec<Triangle<3>>,
-        debug_info: &mut DebugInfo,
-    ) {
-        for face in &self.0 {
-            face.triangles(tolerance, out, debug_info);
-        }
-    }
-}
-
 /// A face of a shape
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub enum Face {

src/main.rs

@@ -100,10 +100,11 @@ fn main() -> anyhow::Result<()> {
     };
 
     let mut debug_info = DebugInfo::new();
-    let faces = shape.to_shape(tolerance, &mut debug_info).faces;
-
     let mut triangles = Vec::new();
-    faces.triangles(tolerance, &mut triangles, &mut debug_info);
+    shape
+        .to_shape(tolerance, &mut debug_info)
+        .faces()
+        .triangles(tolerance, &mut triangles, &mut debug_info);
 
     if let Some(path) = args.export {
         let mut mesh_maker = MeshMaker::new();
@@ -226,10 +227,11 @@ fn main() -> anyhow::Result<()> {
                 debug_info.clear();
                 triangles.clear();
 
-                let faces = shape.to_shape(tolerance, &mut debug_info).faces;
-
                 aabb = shape.bounding_volume();
-                faces.triangles(tolerance, &mut triangles, &mut debug_info);
+                shape
+                    .to_shape(tolerance, &mut debug_info)
+                    .faces()
+                    .triangles(tolerance, &mut triangles, &mut debug_info);
 
                 renderer.update_geometry(
                     (&triangles).into(),