Merge pull request #592 from hannobraun/cycle

Make `Cycle` generic over dimensionality

crates/fj-kernel/src/algorithms/approx/cycles.rs

@@ -16,7 +16,7 @@ impl CycleApprox {
     ///
     /// `tolerance` defines how far the approximation is allowed to deviate from
     /// the actual face.
-    pub fn new(cycle: &Cycle, tolerance: Tolerance) -> Self {
+    pub fn new(cycle: &Cycle<3>, tolerance: Tolerance) -> Self {
         let mut points = Vec::new();
 
         for edge in cycle.edges() {

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

@@ -65,12 +65,8 @@ pub fn sweep_shape(
         let edges_bottom = source_to_bottom.edges_for_cycle(&cycle_source);
         let edges_top = source_to_top.edges_for_cycle(&cycle_source);
 
-        let cycle_bottom = target
-            .insert(Cycle {
-                edges: edges_bottom,
-            })
-            .unwrap();
-        let cycle_top = target.insert(Cycle { edges: edges_top }).unwrap();
+        let cycle_bottom = target.insert(Cycle::new(edges_bottom)).unwrap();
+        let cycle_top = target.insert(Cycle::new(edges_top)).unwrap();
 
         source_to_bottom
             .cycles
@@ -152,6 +148,8 @@ pub fn sweep_shape(
             let mut vertex_bottom_to_edge = HashMap::new();
 
             for edge_source in &cycle_source.get().edges {
+                let edge_source = edge_source.canonical();
+
                 // Can't panic. We already ruled out the continuous edge case
                 // above, so this edge must have vertices.
                 let vertices_source =
@@ -206,14 +204,12 @@ pub fn sweep_shape(
                     .unwrap();
 
                 let cycle = target
-                    .insert(Cycle {
-                        edges: vec![
-                            bottom_edge,
-                            top_edge,
-                            side_edge_a,
-                            side_edge_b,
-                        ],
-                    })
+                    .insert(Cycle::new(vec![
+                        bottom_edge,
+                        top_edge,
+                        side_edge_a,
+                        side_edge_b,
+                    ]))
                     .unwrap();
 
                 target
@@ -234,7 +230,7 @@ pub fn sweep_shape(
 struct Relation {
     vertices: HashMap<Handle<Vertex<3>>, Handle<Vertex<3>>>,
     edges: HashMap<Handle<Edge<3>>, Handle<Edge<3>>>,
-    cycles: HashMap<Handle<Cycle>, Handle<Cycle>>,
+    cycles: HashMap<Handle<Cycle<3>>, Handle<Cycle<3>>>,
 }
 
 impl Relation {
@@ -257,16 +253,19 @@ impl Relation {
         })
     }
 
-    fn edges_for_cycle(&self, cycle: &Handle<Cycle>) -> Vec<Handle<Edge<3>>> {
+    fn edges_for_cycle(
+        &self,
+        cycle: &Handle<Cycle<3>>,
+    ) -> Vec<Handle<Edge<3>>> {
         cycle
             .get()
             .edges
             .iter()
-            .map(|edge| self.edges.get(edge).unwrap().clone())
+            .map(|edge| self.edges.get(edge.canonical()).unwrap().clone())
             .collect()
     }
 
-    fn exteriors_for_face(&self, face: &Face) -> Vec<Handle<Cycle>> {
+    fn exteriors_for_face(&self, face: &Face) -> Vec<Handle<Cycle<3>>> {
         let exteriors = match face {
             Face::Face { exteriors, .. } => exteriors,
             _ => {
@@ -282,7 +281,7 @@ impl Relation {
             .collect()
     }
 
-    fn interiors_for_face(&self, face: &Face) -> Vec<Handle<Cycle>> {
+    fn interiors_for_face(&self, face: &Face) -> Vec<Handle<Cycle<3>>> {
         let interiors = match face {
             Face::Face { interiors, .. } => interiors,
             _ => {
@@ -379,9 +378,7 @@ mod tests {
             let ca = Edge::builder(&mut shape)
                 .build_line_segment_from_points([c, a])?;
 
-            let cycles = shape.insert(Cycle {
-                edges: vec![ab, bc, ca],
-            })?;
+            let cycles = shape.insert(Cycle::new(vec![ab, bc, ca]))?;
 
             let surface =
                 Surface::SweptCurve(SweptCurve::plane_from_points([a, b, c]));

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

@@ -175,7 +175,7 @@ impl Shape {
     /// Access an iterator over all cycles
     ///
     /// The caller must not make any assumptions about the order of cycles.
-    pub fn cycles(&self) -> Iter<Cycle> {
+    pub fn cycles(&self) -> Iter<Cycle<3>> {
         self.stores.cycles.iter()
     }
 
@@ -240,7 +240,7 @@ mod tests {
         assert!(shape.get_handle(&vertex.get()).as_ref() == Some(&vertex));
         assert!(shape.get_handle(&edge.get()).as_ref() == Some(&edge));
 
-        let cycle = Cycle { edges: vec![edge] };
+        let cycle = Cycle::new(vec![edge]);
         assert!(shape.get_handle(&cycle).is_none());
 
         let cycle = shape.insert(cycle)?;
@@ -321,16 +321,12 @@ mod tests {
 
         // Trying to refer to edge that is not from the same shape. Should fail.
         let edge = other.add_edge()?;
-        let err = shape
-            .insert(Cycle {
-                edges: vec![edge.clone()],
-            })
-            .unwrap_err();
+        let err = shape.insert(Cycle::new(vec![edge.clone()])).unwrap_err();
         assert!(err.missing_edge(&edge));
 
         // Referring to edge that *is* from the same shape. Should work.
         let edge = shape.add_edge()?;
-        shape.insert(Cycle { edges: vec![edge] })?;
+        shape.insert(Cycle::new(vec![edge]))?;
 
         Ok(())
     }
@@ -402,9 +398,9 @@ mod tests {
             Ok(edge)
         }
 
-        fn add_cycle(&mut self) -> anyhow::Result<Handle<Cycle>> {
+        fn add_cycle(&mut self) -> anyhow::Result<Handle<Cycle<3>>> {
             let edge = self.add_edge()?;
-            let cycle = self.insert(Cycle { edges: vec![edge] })?;
+            let cycle = self.insert(Cycle::new(vec![edge]))?;
             Ok(cycle)
         }
     }

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

@@ -19,7 +19,7 @@ impl private::Sealed for Surface {}
 
 impl private::Sealed for Vertex<3> {}
 impl private::Sealed for Edge<3> {}
-impl private::Sealed for Cycle {}
+impl private::Sealed for Cycle<3> {}
 impl private::Sealed for Face {}
 
 impl Object for Point<3> {}
@@ -28,7 +28,7 @@ impl Object for Surface {}
 
 impl Object for Vertex<3> {}
 impl Object for Edge<3> {}
-impl Object for Cycle {}
+impl Object for Cycle<3> {}
 impl Object for Face {}
 
 mod private {

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

@@ -57,7 +57,7 @@ pub type Surfaces = Store<Surface>;
 
 pub type Vertices = Store<Vertex<3>>;
 pub type Edges = Store<Edge<3>>;
-pub type Cycles = Store<Cycle>;
+pub type Cycles = Store<Cycle<3>>;
 pub type Faces = Store<Face>;
 
 #[derive(Debug)]

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

@@ -95,7 +95,7 @@ impl Validate for Edge<3> {
     }
 }
 
-impl Validate for Cycle {
+impl Validate for Cycle<3> {
     /// Validate the cycle
     ///
     /// # Implementation note
@@ -111,6 +111,8 @@ impl Validate for Cycle {
     ) -> Result<(), ValidationError> {
         let mut missing_edges = HashSet::new();
         for edge in &self.edges {
+            let edge = edge.canonical();
+
             if !stores.edges.contains(edge) {
                 missing_edges.insert(edge.clone());
             }
@@ -248,7 +250,7 @@ impl ValidationError {
 
     /// Indicate whether validation found a missing cycle
     #[cfg(test)]
-    pub fn missing_cycle(&self, cycle: &Handle<Cycle>) -> bool {
+    pub fn missing_cycle(&self, cycle: &Handle<Cycle<3>>) -> bool {
         if let Self::Structural(StructuralIssues { missing_cycles, .. }) = self
         {
             return missing_cycles.contains(cycle);
@@ -282,5 +284,5 @@ pub struct StructuralIssues {
     pub missing_surface: Option<Handle<Surface>>,
 
     /// Missing cycles found in face validation
-    pub missing_cycles: HashSet<Handle<Cycle>>,
+    pub missing_cycles: HashSet<Handle<Cycle<3>>>,
 }

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

@@ -107,7 +107,7 @@ impl<'r> CycleBuilder<'r> {
     pub fn build_polygon(
         self,
         points: impl IntoIterator<Item = impl Into<Point<3>>>,
-    ) -> ValidationResult<Cycle> {
+    ) -> ValidationResult<Cycle<3>> {
         // A polygon is closed, so we need to add the first point at the end
         // again, for the next step.
         let mut points: Vec<_> = points.into_iter().map(Into::into).collect();
@@ -127,7 +127,7 @@ impl<'r> CycleBuilder<'r> {
             edges.push(edge);
         }
 
-        self.shape.insert(Cycle { edges })
+        self.shape.insert(Cycle::new(edges))
     }
 }
 

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

@@ -1,6 +1,4 @@
-use std::hash::{Hash, Hasher};
-
-use crate::shape::{Handle, Shape};
+use crate::shape::{Handle, LocalForm, Shape};
 
 use super::{CycleBuilder, Edge};
 
@@ -19,13 +17,20 @@ use super::{CycleBuilder, Edge};
 ///
 /// A cycle that is part of a [`Shape`] must be structurally sound. That means
 /// the edges it refers to, must be part of the same shape.
-#[derive(Clone, Debug, Eq, Ord, PartialOrd)]
-pub struct Cycle {
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
+pub struct Cycle<const D: usize> {
     /// The edges that make up the cycle
-    pub edges: Vec<Handle<Edge<3>>>,
+    pub edges: Vec<LocalForm<Edge<D>, Edge<3>>>,
 }
 
-impl Cycle {
+impl Cycle<3> {
+    /// Construct a `Cycle`
+    pub fn new(edges: impl IntoIterator<Item = Handle<Edge<3>>>) -> Self {
+        let edges = edges.into_iter().map(LocalForm::canonical_only).collect();
+
+        Self { edges }
+    }
+
     /// Build a cycle using the [`CycleBuilder`] API
     pub fn builder(shape: &mut Shape) -> CycleBuilder {
         CycleBuilder::new(shape)
@@ -36,20 +41,6 @@ impl Cycle {
     /// This is a convenience method that saves the caller from dealing with the
     /// [`Handle`]s.
     pub fn edges(&self) -> impl Iterator<Item = Edge<3>> + '_ {
-        self.edges.iter().map(|handle| handle.get())
-    }
-}
-
-impl PartialEq for Cycle {
-    fn eq(&self, other: &Self) -> bool {
-        self.edges().eq(other.edges())
-    }
-}
-
-impl Hash for Cycle {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        for edge in self.edges() {
-            edge.hash(state);
-        }
+        self.edges.iter().map(|handle| handle.canonical().get())
     }
 }

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>>,
+        exteriors: Vec<Handle<Cycle<3>>>,
 
         /// 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>>,
+        interiors: Vec<Handle<Cycle<3>>>,
 
         /// The color of the face
         color: [u8; 4],
@@ -90,7 +90,7 @@ impl Face {
     ///
     /// This is a convenience method that saves the caller from dealing with the
     /// [`Handle`]s.
-    pub fn exteriors(&self) -> impl Iterator<Item = Cycle> + '_ {
+    pub fn exteriors(&self) -> impl Iterator<Item = Cycle<3>> + '_ {
         match self {
             Self::Face { exteriors, .. } => {
                 exteriors.iter().map(|handle| handle.get())
@@ -107,7 +107,7 @@ impl Face {
     ///
     /// This is a convenience method that saves the caller from dealing with the
     /// [`Handle`]s.
-    pub fn interiors(&self) -> impl Iterator<Item = Cycle> + '_ {
+    pub fn interiors(&self) -> impl Iterator<Item = Cycle<3>> + '_ {
         match self {
             Self::Face { interiors, .. } => {
                 interiors.iter().map(|handle| handle.get())
@@ -124,7 +124,7 @@ impl Face {
     ///
     /// This is equivalent to chaining the iterators returned by
     /// [`Face::exteriors`] and [`Face::interiors`].
-    pub fn all_cycles(&self) -> impl Iterator<Item = Cycle> + '_ {
+    pub fn all_cycles(&self) -> impl Iterator<Item = Cycle<3>> + '_ {
         self.exteriors().chain(self.interiors())
     }
 }

crates/fj-operations/src/circle.rs

@@ -19,7 +19,7 @@ impl ToShape for fj::Circle {
         let edge = Edge::builder(&mut shape)
             .build_circle(Scalar::from_f64(self.radius()))
             .unwrap();
-        shape.insert(Cycle { edges: vec![edge] }).unwrap();
+        shape.insert(Cycle::new(vec![edge])).unwrap();
 
         let cycles = shape.cycles().collect();
         let surface = shape.insert(Surface::xy_plane()).unwrap();

crates/fj-operations/src/difference_2d.rs

@@ -92,11 +92,11 @@ impl ToShape for fj::Difference2d {
 }
 
 fn add_cycle(
-    cycle: Handle<Cycle>,
+    cycle: Handle<Cycle<3>>,
     vertices: &mut HashMap<Vertex<3>, Handle<Vertex<3>>>,
     shape: &mut Shape,
     reverse: bool,
-) -> Handle<Cycle> {
+) -> Handle<Cycle<3>> {
     let mut edges = Vec::new();
     for edge in cycle.get().edges() {
         let curve = edge.curve();
@@ -129,5 +129,5 @@ fn add_cycle(
         edges.reverse();
     }
 
-    shape.insert(Cycle { edges }).unwrap()
+    shape.insert(Cycle::new(edges)).unwrap()
 }

crates/fj-operations/src/group.rs

@@ -74,14 +74,13 @@ fn copy_shape(orig: Shape, target: &mut Shape) {
     }
     for cycle_orig in orig.cycles() {
         let cycle = target
-            .insert(Cycle {
-                edges: cycle_orig
+            .insert(Cycle::new(
+                cycle_orig
                     .get()
                     .edges
                     .iter()
-                    .map(|edge| edges[edge].clone())
-                    .collect(),
-            })
+                    .map(|edge| edges[edge.canonical()].clone()),
+            ))
             .unwrap();
         cycles.insert(cycle_orig, cycle);
     }