Merge pull request #593 from hannobraun/face

Make some cleanups in `Face`

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

@@ -90,20 +90,15 @@ pub fn sweep_shape(
         let interiors_top = source_to_top.interiors_for_face(&face_source);
 
         target
-            .insert(Face::Face {
-                surface: surface_bottom,
-                exteriors: exteriors_bottom,
-                interiors: interiors_bottom,
+            .insert(Face::new(
+                surface_bottom,
+                exteriors_bottom,
+                interiors_bottom,
                 color,
-            })
+            ))
             .unwrap();
         target
-            .insert(Face::Face {
-                surface: surface_top,
-                exteriors: exteriors_top,
-                interiors: interiors_top,
-                color,
-            })
+            .insert(Face::new(surface_top, exteriors_top, interiors_top, color))
             .unwrap();
     }
 
@@ -213,12 +208,7 @@ pub fn sweep_shape(
                     .unwrap();
 
                 target
-                    .insert(Face::Face {
-                        surface,
-                        exteriors: vec![cycle],
-                        interiors: Vec::new(),
-                        color,
-                    })
+                    .insert(Face::new(surface, vec![cycle], Vec::new(), color))
                     .unwrap();
             }
         }
@@ -276,7 +266,7 @@ impl Relation {
         };
 
         exteriors
-            .iter()
+            .as_handle()
             .map(|cycle| self.cycles.get(cycle).unwrap().clone())
             .collect()
     }
@@ -292,7 +282,7 @@ impl Relation {
         };
 
         interiors
-            .iter()
+            .as_handle()
             .map(|cycle| self.cycles.get(cycle).unwrap().clone())
             .collect()
     }
@@ -385,12 +375,8 @@ mod tests {
             let surface = if reverse { surface.reverse() } else { surface };
             let surface = shape.insert(surface)?;
 
-            let abc = Face::Face {
-                surface,
-                exteriors: vec![cycles],
-                interiors: Vec::new(),
-                color: [255, 0, 0, 255],
-            };
+            let abc =
+                Face::new(surface, vec![cycles], Vec::new(), [255, 0, 0, 255]);
 
             let face = shape.insert(abc)?;
 

crates/fj-kernel/src/shape/api.rs

@@ -246,12 +246,7 @@ mod tests {
         let cycle = shape.insert(cycle)?;
         assert!(shape.get_handle(&cycle.get()).as_ref() == Some(&cycle));
 
-        let face = Face::Face {
-            surface,
-            exteriors: Vec::new(),
-            interiors: Vec::new(),
-            color: [0, 0, 0, 0],
-        };
+        let face = Face::new(surface, Vec::new(), Vec::new(), [0, 0, 0, 0]);
         assert!(shape.get_handle(&face).is_none());
 
         let face = shape.insert(face)?;
@@ -341,12 +336,12 @@ mod tests {
 
         // Nothing has been added to `shape`. Should fail.
         let err = shape
-            .insert(Face::Face {
-                surface: surface.clone(),
-                exteriors: vec![cycle.clone()],
-                interiors: Vec::new(),
-                color: [255, 0, 0, 255],
-            })
+            .insert(Face::new(
+                surface.clone(),
+                vec![cycle.clone()],
+                Vec::new(),
+                [255, 0, 0, 255],
+            ))
             .unwrap_err();
         assert!(err.missing_surface(&surface));
         assert!(err.missing_cycle(&cycle));
@@ -355,12 +350,12 @@ mod tests {
         let cycle = shape.add_cycle()?;
 
         // Everything has been added to `shape` now. Should work!
-        shape.insert(Face::Face {
+        shape.insert(Face::new(
             surface,
-            exteriors: vec![cycle],
-            interiors: Vec::new(),
-            color: [255, 0, 0, 255],
-        })?;
+            vec![cycle],
+            Vec::new(),
+            [255, 0, 0, 255],
+        ))?;
 
         Ok(())
     }

crates/fj-kernel/src/shape/validate.rs

@@ -149,7 +149,7 @@ impl Validate for Face {
             if !stores.surfaces.contains(surface) {
                 missing_surface = Some(surface.clone());
             }
-            for cycle in exteriors.iter().chain(interiors) {
+            for cycle in exteriors.as_handle().chain(interiors.as_handle()) {
                 if !stores.cycles.contains(cycle) {
                     missing_cycles.insert(cycle.clone());
                 }

crates/fj-kernel/src/topology/builder.rs

@@ -197,11 +197,11 @@ impl<'r> FaceBuilder<'r> {
             interiors.push(cycle);
         }
 
-        self.shape.insert(Face::Face {
+        self.shape.insert(Face::new(
             surface,
             exteriors,
             interiors,
-            color: [255, 0, 0, 255],
-        })
+            [255, 0, 0, 255],
+        ))
     }
 }

crates/fj-kernel/src/topology/faces.rs

@@ -41,7 +41,7 @@ pub enum Face {
         ///
         /// It might be less error-prone to specify the cycles in surface
         /// coordinates.
-        exteriors: Vec<Handle<Cycle<3>>>,
+        exteriors: CyclesInFace,
 
         /// The cycles that bound the face on the inside
         ///
@@ -50,7 +50,7 @@ pub enum Face {
         /// # Implementation note
         ///
         /// See note on `exterior` field.
-        interiors: Vec<Handle<Cycle<3>>>,
+        interiors: CyclesInFace,
 
         /// The color of the face
         color: [u8; 4],
@@ -66,6 +66,23 @@ pub enum Face {
 }
 
 impl Face {
+    /// Construct a new instance of `Face`
+    pub fn new(
+        surface: Handle<Surface>,
+        exteriors: impl IntoIterator<Item = Handle<Cycle<3>>>,
+        interiors: impl IntoIterator<Item = Handle<Cycle<3>>>,
+        color: [u8; 4],
+    ) -> Self {
+        let exteriors = CyclesInFace::from_canonical(exteriors);
+        let interiors = CyclesInFace::from_canonical(interiors);
+
+        Self::Face {
+            surface,
+            exteriors,
+            interiors,
+            color,
+        }
+    }
     /// Build a face using the [`FaceBuilder`] API
     pub fn builder(surface: Surface, shape: &mut Shape) -> FaceBuilder {
         FaceBuilder::new(surface, shape)
@@ -92,9 +109,7 @@ impl Face {
     /// [`Handle`]s.
     pub fn exteriors(&self) -> impl Iterator<Item = Cycle<3>> + '_ {
         match self {
-            Self::Face { exteriors, .. } => {
-                exteriors.iter().map(|handle| handle.get())
-            }
+            Self::Face { exteriors, .. } => exteriors.as_canonical(),
             _ => {
                 // No code that still uses triangle representation is calling
                 // this method.
@@ -109,9 +124,7 @@ impl Face {
     /// [`Handle`]s.
     pub fn interiors(&self) -> impl Iterator<Item = Cycle<3>> + '_ {
         match self {
-            Self::Face { interiors, .. } => {
-                interiors.iter().map(|handle| handle.get())
-            }
+            Self::Face { interiors, .. } => interiors.as_canonical(),
             _ => {
                 // No code that still uses triangle representation is calling
                 // this method.
@@ -145,3 +158,24 @@ impl Hash for Face {
         }
     }
 }
+
+/// A list of cycles, as they are stored in `Face`
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
+pub struct CyclesInFace(Vec<Handle<Cycle<3>>>);
+
+impl CyclesInFace {
+    fn from_canonical(
+        cycles: impl IntoIterator<Item = Handle<Cycle<3>>>,
+    ) -> Self {
+        Self(cycles.into_iter().collect())
+    }
+
+    fn as_canonical(&self) -> impl Iterator<Item = Cycle<3>> + '_ {
+        self.as_handle().map(|edge| edge.get())
+    }
+
+    /// Access an iterator over handles to the cycles
+    pub fn as_handle(&self) -> impl Iterator<Item = &'_ Handle<Cycle<3>>> + '_ {
+        self.0.iter()
+    }
+}

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

@@ -26,6 +26,6 @@ pub use self::{
     builder::{CycleBuilder, EdgeBuilder, FaceBuilder, VertexBuilder},
     cycles::Cycle,
     edges::Edge,
-    faces::Face,
+    faces::{CyclesInFace, Face},
     vertices::Vertex,
 };

crates/fj-operations/src/circle.rs

@@ -21,15 +21,10 @@ impl ToShape for fj::Circle {
             .unwrap();
         shape.insert(Cycle::new(vec![edge])).unwrap();
 
-        let cycles = shape.cycles().collect();
+        let cycles = shape.cycles();
         let surface = shape.insert(Surface::xy_plane()).unwrap();
         shape
-            .insert(Face::Face {
-                exteriors: cycles,
-                interiors: Vec::new(),
-                surface,
-                color: self.color(),
-            })
+            .insert(Face::new(surface, cycles, Vec::new(), self.color()))
             .unwrap();
 
         shape

crates/fj-operations/src/difference_2d.rs

@@ -72,12 +72,7 @@ impl ToShape for fj::Difference2d {
         let surface = shape.insert(face_a.surface()).unwrap();
 
         shape
-            .insert(Face::Face {
-                surface,
-                exteriors,
-                interiors,
-                color: self.color(),
-            })
+            .insert(Face::new(surface, exteriors, interiors, self.color()))
             .unwrap();
 
         shape

crates/fj-operations/src/group.rs

@@ -94,18 +94,16 @@ fn copy_shape(orig: Shape, target: &mut Shape) {
                 color,
             } => {
                 target
-                    .insert(Face::Face {
-                        surface: surfaces[&surface].clone(),
-                        exteriors: exteriors
-                            .iter()
-                            .map(|cycle| cycles[cycle].clone())
-                            .collect(),
-                        interiors: interiors
-                            .iter()
-                            .map(|cycle| cycles[cycle].clone())
-                            .collect(),
+                    .insert(Face::new(
+                        surfaces[&surface].clone(),
+                        exteriors
+                            .as_handle()
+                            .map(|cycle| cycles[cycle].clone()),
+                        interiors
+                            .as_handle()
+                            .map(|cycle| cycles[cycle].clone()),
                         color,
-                    })
+                    ))
                     .unwrap();
             }
             face @ Face::Triangles(_) => {