Merge pull request #336 from hannobraun/structural

Implement full structural validation of `Shape`

src/kernel/shape/topology.rs

@@ -36,6 +36,10 @@ pub struct Topology<'r> {
 impl Topology<'_> {
     /// Add a vertex to the shape
     ///
+    /// Validates that the vertex is structurally sound (i.e. the point it
+    /// refers to is part of the shape). Returns an error, if that is not the
+    /// case.
+    ///
     /// Logs a warning, if the vertex is not unique, meaning if another vertex
     /// defined by the same point already exists.
     ///
@@ -50,14 +54,12 @@ impl Topology<'_> {
     /// as the presence of bugs in the sweep and transform code would basically
     /// break ever model, due to validation errors.
     ///
-    /// In the future, this method is likely to validate more than just vertex
-    /// uniqueness. See documentation of [`crate::kernel`] for some context on
-    /// that.
+    /// In the future, this method is likely to validate more than it already
+    /// does. See documentation of [`crate::kernel`] for some context on that.
     pub fn add_vertex(&mut self, vertex: Vertex) -> ValidationResult<Vertex> {
-        // Make sure the new vertex is a minimum distance away from all existing
-        // vertices. This minimum distance is defined to be half a µm, which
-        // should provide more than enough precision for common use cases, while
-        // being large enough to catch all invalid cases.
+        if !self.geometry.points.contains(vertex.point.storage()) {
+            return Err(ValidationError::Structural(()));
+        }
         for existing in &*self.vertices {
             let distance = (existing.point() - vertex.point()).magnitude();
 
@@ -76,29 +78,27 @@ impl Topology<'_> {
         Ok(handle)
     }
 
-    /// Access iterator over all vertices
-    ///
-    /// The caller must not make any assumptions about the order of vertices.
-    pub fn vertices(&self) -> impl Iterator<Item = Handle<Vertex>> + '_ {
-        self.vertices.iter().map(|storage| storage.handle())
-    }
-
     /// Add an edge to the shape
     ///
+    /// Validates that the edge is structurally sound (i.e. the curve and
+    /// vertices it refers to are part of the shape). Returns an error, if that
+    /// is not the case.
+    ///
+    /// # Vertices
+    ///
     /// If vertices are provided in `vertices`, they must be on `curve`.
     ///
     /// This constructor will convert the vertices into curve coordinates. If
     /// they are not on the curve, this will result in their projection being
     /// converted into curve coordinates, which is likely not the caller's
     /// intention.
-    ///
-    /// # Implementation notes
-    ///
-    /// Right now this is just an overly complicated constructor for `Edge`. In
-    /// the future, it can add the edge to the proper internal data structures,
-    /// and validate any constraints that apply to edge creation.
     pub fn add_edge(&mut self, edge: Edge) -> ValidationResult<Edge> {
+        let mut missing_curve = None;
         let mut missing_vertices = HashSet::new();
+
+        if !self.geometry.curves.contains(edge.curve.storage()) {
+            missing_curve = Some(edge.curve.clone());
+        }
         for vertices in &edge.vertices {
             for vertex in vertices {
                 if !self.vertices.contains(vertex.storage()) {
@@ -107,8 +107,11 @@ impl Topology<'_> {
             }
         }
 
-        if !missing_vertices.is_empty() {
-            return Err(ValidationError::Structural(missing_vertices));
+        if missing_curve.is_some() || !missing_vertices.is_empty() {
+            return Err(ValidationError::Structural((
+                missing_curve,
+                missing_vertices,
+            )));
         }
 
         let storage = Storage::new(edge);
@@ -151,18 +154,10 @@ impl Topology<'_> {
         })
     }
 
-    /// Access iterator over all edges
-    ///
-    /// The caller must not make any assumptions about the order of edges.
-    pub fn edges(&self) -> impl Iterator<Item = Handle<Edge>> + '_ {
-        self.edges.iter().map(|storage| storage.handle())
-    }
-
     /// Add a cycle to the shape
     ///
-    /// # Panics
-    ///
-    /// Panics, if the edges of the cycles are not part of this shape.
+    /// Validates that the cycle is structurally sound (i.e. the edges it refers
+    /// to are part of the shape). Returns an error, if that is not the case.
     ///
     /// # Implementation note
     ///
@@ -189,13 +184,33 @@ impl Topology<'_> {
         Ok(handle)
     }
 
-    /// Access an iterator over all cycles
-    pub fn cycles(&self) -> impl Iterator<Item = Handle<Cycle>> + '_ {
-        self.cycles.iter().map(|storage| storage.handle())
-    }
-
     /// Add a face to the shape
+    ///
+    /// Validates that the face is structurally sound (i.e. the surface and
+    /// cycles it refers to are part of the shape). Returns an error, if that is
+    /// not the case.
     pub fn add_face(&mut self, face: Face) -> ValidationResult<Face> {
+        if let Face::Face { surface, cycles } = &face {
+            let mut missing_surface = None;
+            let mut missing_cycles = HashSet::new();
+
+            if !self.geometry.surfaces.contains(surface.storage()) {
+                missing_surface = Some(surface.clone());
+            }
+            for cycle in cycles {
+                if !self.cycles.contains(cycle.storage()) {
+                    missing_cycles.insert(cycle.clone());
+                }
+            }
+
+            if missing_surface.is_some() || !missing_cycles.is_empty() {
+                return Err(ValidationError::Structural((
+                    missing_surface,
+                    missing_cycles,
+                )));
+            }
+        }
+
         let storage = Storage::new(face);
         let handle = storage.handle();
 
@@ -204,6 +219,25 @@ impl Topology<'_> {
         Ok(handle)
     }
 
+    /// Access iterator over all vertices
+    ///
+    /// The caller must not make any assumptions about the order of vertices.
+    pub fn vertices(&self) -> impl Iterator<Item = Handle<Vertex>> + '_ {
+        self.vertices.iter().map(|storage| storage.handle())
+    }
+
+    /// Access iterator over all edges
+    ///
+    /// The caller must not make any assumptions about the order of edges.
+    pub fn edges(&self) -> impl Iterator<Item = Handle<Edge>> + '_ {
+        self.edges.iter().map(|storage| storage.handle())
+    }
+
+    /// Access an iterator over all cycles
+    pub fn cycles(&self) -> impl Iterator<Item = Handle<Cycle>> + '_ {
+        self.cycles.iter().map(|storage| storage.handle())
+    }
+
     /// Access an iterator over all faces
     pub fn faces(&self) -> impl Iterator<Item = Handle<Face>> + '_ {
         self.faces.iter().map(|storage| storage.handle())
@@ -227,9 +261,11 @@ mod tests {
 
     use crate::{
         kernel::{
-            shape::{handle::Handle, Shape},
+            geometry::{Curve, Line, Surface},
+            shape::{handle::Handle, Shape, ValidationError},
             topology::{
                 edges::{Cycle, Edge},
+                faces::Face,
                 vertices::Vertex,
             },
         },
@@ -241,10 +277,16 @@ mod tests {
     #[test]
     fn add_vertex() -> anyhow::Result<()> {
         let mut shape = Shape::new().with_min_distance(MIN_DISTANCE);
+        let mut other = Shape::new();
 
         let point = shape.geometry().add_point(Point::from([0., 0., 0.]));
         shape.topology().add_vertex(Vertex { point })?;
 
+        // Should fail, as `point` is not part of the shape.
+        let point = other.geometry().add_point(Point::from([1., 0., 0.]));
+        let result = shape.topology().add_vertex(Vertex { point });
+        assert!(matches!(result, Err(ValidationError::Structural(()))));
+
         // `point` is too close to the original point. `assert!` is commented,
         // because that only causes a warning to be logged right now.
         let point = shape.geometry().add_point(Point::from([5e-6, 0., 0.]));
@@ -264,22 +306,31 @@ mod tests {
         let mut shape = TestShape::new();
         let mut other = TestShape::new();
 
+        let curve = other.add_curve();
         let a = other.add_vertex()?;
         let b = other.add_vertex()?;
 
-        // Shouldn't work. None of the vertices have been added to `shape`.
+        // Shouldn't work. Nothing has been added to `shape`.
         let err = shape
             .topology()
-            .add_line_segment([a.clone(), b.clone()])
+            .add_edge(Edge {
+                curve: curve.clone(),
+                vertices: Some([a.clone(), b.clone()]),
+            })
             .unwrap_err();
+        assert!(err.missing_curve(&curve));
         assert!(err.missing_vertex(&a));
         assert!(err.missing_vertex(&b));
 
+        let curve = shape.add_curve();
         let a = shape.add_vertex()?;
         let b = shape.add_vertex()?;
 
-        // Both `a` and `b` have been added to `shape`. Should work!
-        shape.topology().add_line_segment([a, b])?;
+        // Everything has been added to `shape` now. Should work!
+        shape.topology().add_edge(Edge {
+            curve,
+            vertices: Some([a, b]),
+        })?;
 
         Ok(())
     }
@@ -306,6 +357,37 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn add_face() -> anyhow::Result<()> {
+        let mut shape = TestShape::new();
+        let mut other = TestShape::new();
+
+        let surface = other.add_surface();
+        let cycle = other.add_cycle()?;
+
+        // Nothing has been added to `shape`. Should fail.
+        let err = shape
+            .topology()
+            .add_face(Face::Face {
+                surface: surface.clone(),
+                cycles: vec![cycle.clone()],
+            })
+            .unwrap_err();
+        assert!(err.missing_surface(&surface));
+        assert!(err.missing_cycle(&cycle));
+
+        let surface = shape.add_surface();
+        let cycle = shape.add_cycle()?;
+
+        // Everything has been added to `shape` now. Should work!
+        shape.topology().add_face(Face::Face {
+            surface,
+            cycles: vec![cycle],
+        })?;
+
+        Ok(())
+    }
+
     struct TestShape {
         inner: Shape,
         next_point: Point<3>,
@@ -319,6 +401,17 @@ mod tests {
             }
         }
 
+        fn add_curve(&mut self) -> Handle<Curve> {
+            self.geometry().add_curve(Curve::Line(Line::from_points([
+                Point::from([0., 0., 0.]),
+                Point::from([1., 0., 0.]),
+            ])))
+        }
+
+        fn add_surface(&mut self) -> Handle<Surface> {
+            self.geometry().add_surface(Surface::x_y_plane())
+        }
+
         fn add_vertex(&mut self) -> anyhow::Result<Handle<Vertex>> {
             let point = self.next_point;
             self.next_point.x += Scalar::ONE;
@@ -334,6 +427,13 @@ mod tests {
             let edge = self.topology().add_line_segment(vertices)?;
             Ok(edge)
         }
+
+        fn add_cycle(&mut self) -> anyhow::Result<Handle<Cycle>> {
+            let edge = self.add_edge()?;
+            let cycle =
+                self.topology().add_cycle(Cycle { edges: vec![edge] })?;
+            Ok(cycle)
+        }
     }
 
     impl Deref for TestShape {

src/kernel/shape/validate.rs

@@ -1,9 +1,12 @@
 use std::collections::HashSet;
 
-use crate::kernel::topology::{
-    edges::{Cycle, Edge},
-    faces::Face,
-    vertices::Vertex,
+use crate::kernel::{
+    geometry::{Curve, Surface},
+    topology::{
+        edges::{Cycle, Edge},
+        faces::Face,
+        vertices::Vertex,
+    },
 };
 
 use super::handle::Handle;
@@ -41,11 +44,21 @@ pub enum ValidationError<T: Validatable> {
 }
 
 impl ValidationError<Edge> {
+    /// Indicate whether validation found a missing curve
+    #[cfg(test)]
+    pub fn missing_curve(&self, curve: &Handle<Curve>) -> bool {
+        if let Self::Structural(missing) = self {
+            return missing.0.as_ref() == Some(curve);
+        }
+
+        false
+    }
+
     /// Indicate whether validation found a missing vertex
     #[cfg(test)]
     pub fn missing_vertex(&self, vertex: &Handle<Vertex>) -> bool {
         if let Self::Structural(missing) = self {
-            return missing.contains(vertex);
+            return missing.1.contains(vertex);
         }
 
         false
@@ -64,6 +77,28 @@ impl ValidationError<Cycle> {
     }
 }
 
+impl ValidationError<Face> {
+    /// Indicate whether validation found a missing surface
+    #[cfg(test)]
+    pub fn missing_surface(&self, surface: &Handle<Surface>) -> bool {
+        if let Self::Structural(missing) = self {
+            return missing.0.as_ref() == Some(surface);
+        }
+
+        false
+    }
+
+    /// Indicate whether validation found a missing cycle
+    #[cfg(test)]
+    pub fn missing_cycle(&self, cycle: &Handle<Cycle>) -> bool {
+        if let Self::Structural(missing) = self {
+            return missing.1.contains(cycle);
+        }
+
+        false
+    }
+}
+
 /// Implemented for topological types, which can be validated
 ///
 /// Used by [`ValidationError`] to provide context on how validation failed.
@@ -76,13 +111,13 @@ impl Validatable for Vertex {
 }
 
 impl Validatable for Edge {
-    type Structural = HashSet<Handle<Vertex>>;
+    type Structural = (Option<Handle<Curve>>, HashSet<Handle<Vertex>>);
 }
 
 impl Validatable for Cycle {
     type Structural = HashSet<Handle<Edge>>;
 }
 
 impl Validatable for Face {
-    type Structural = ();
+    type Structural = (Option<Handle<Surface>>, HashSet<Handle<Cycle>>);
 }