Merge pull request #415 from hannobraun/shape

Clean up shape validation internals

fj-kernel/src/shape/api.rs

@@ -1,26 +1,19 @@
+use std::marker::PhantomData;
+
 use fj_math::Scalar;
 
 use super::{
-    stores::{Curves, Cycles, Edges, Faces, Points, Surfaces, Vertices},
+    stores::{
+        Curves, Cycles, Edges, Faces, Points, Stores, Surfaces, Vertices,
+    },
     Geometry, Topology,
 };
 
 /// The boundary representation of a shape
 #[derive(Clone, Debug)]
 pub struct Shape {
-    /// The minimum distance between two vertices
-    ///
-    /// Use for vertex validation, to determine whether vertices are unique.
     min_distance: Scalar,
-
+    stores: Stores,
-    points: Points,
-    curves: Curves,
-    surfaces: Surfaces,
-
-    vertices: Vertices,
-    edges: Edges,
-    cycles: Cycles,
-    faces: Faces,
 }
 
 impl Shape {
@@ -32,6 +25,7 @@ impl Shape {
             // be `const` yet.
             min_distance: Scalar::from_f64(5e-7), // 0.5 µm
 
+            stores: Stores {
                 points: Points::new(),
                 curves: Curves::new(),
                 surfaces: Surfaces::new(),
@@ -40,11 +34,14 @@ impl Shape {
                 edges: Edges::new(),
                 cycles: Cycles::new(),
                 faces: Faces::new(),
+            },
         }
     }
 
     /// Override the minimum distance for this shape
     ///
+    /// Used for vertex validation, to determine whether vertices are unique.
+    ///
     /// # Implementation note
     ///
     /// This functionality should be exposed to models, eventually. For now it's
@@ -61,11 +58,11 @@ impl Shape {
     /// Access the shape's geometry
     pub fn geometry(&mut self) -> Geometry {
         Geometry {
-            points: &mut self.points,
+            points: &mut self.stores.points,
-            curves: &mut self.curves,
+            curves: &mut self.stores.curves,
-            surfaces: &mut self.surfaces,
+            surfaces: &mut self.stores.surfaces,
 
-            faces: &mut self.faces,
+            faces: &mut self.stores.faces,
         }
     }
 
@@ -73,18 +70,8 @@ impl Shape {
     pub fn topology(&mut self) -> Topology {
         Topology {
             min_distance: self.min_distance,
-
+            stores: self.stores.clone(),
-            geometry: Geometry {
+            _lifetime: PhantomData,
-                points: &mut self.points,
-                curves: &mut self.curves,
-                surfaces: &mut self.surfaces,
-
-                faces: &mut self.faces,
-            },
-
-            vertices: &mut self.vertices,
-            edges: &mut self.edges,
-            cycles: &mut self.cycles,
         }
     }
 }

fj-kernel/src/shape/geometry.rs

@@ -43,17 +43,17 @@ pub struct Geometry<'r> {
 impl Geometry<'_> {
     /// Add a point to the shape
     pub fn add_point(&mut self, point: Point<3>) -> Handle<Point<3>> {
-        self.points.add(point)
+        self.points.insert(point)
     }
 
     /// Add a curve to the shape
     pub fn add_curve(&mut self, curve: Curve) -> Handle<Curve> {
-        self.curves.add(curve)
+        self.curves.insert(curve)
     }
 
     /// Add a surface to the shape
     pub fn add_surface(&mut self, surface: Surface) -> Handle<Surface> {
-        self.surfaces.add(surface)
+        self.surfaces.insert(surface)
     }
 
     /// Transform the geometry of the shape

fj-kernel/src/shape/stores.rs

@@ -12,6 +12,18 @@ use crate::{
     topology::{Cycle, Edge, Face, Vertex},
 };
 
+#[derive(Clone, Debug)]
+pub struct Stores {
+    pub points: Points,
+    pub curves: Curves,
+    pub surfaces: Surfaces,
+
+    pub vertices: Vertices,
+    pub edges: Edges,
+    pub cycles: Cycles,
+    pub faces: Faces,
+}
+
 pub type Points = Store<Point<3>>;
 pub type Curves = Store<Curve>;
 pub type Surfaces = Store<Surface>;
@@ -33,15 +45,15 @@ impl<T> Store<T> {
         }
     }
 
-    pub fn contains(&self, object: &Handle<T>) -> bool {
+    pub fn insert(&mut self, object: T) -> Handle<T> {
-        object.store() == self && self.objects.read().contains_key(object.key())
-    }
-
-    pub fn add(&mut self, object: T) -> Handle<T> {
         let key = self.objects.write().insert(object);
         Handle::new(key, self.clone())
     }
 
+    pub fn contains(&self, object: &Handle<T>) -> bool {
+        object.store() == self && self.objects.read().contains_key(object.key())
+    }
+
     pub fn read(&self) -> RwLockReadGuard<Objects<T>> {
         self.objects.read()
     }

fj-kernel/src/shape/topology.rs

@@ -1,23 +1,16 @@
-use std::collections::HashSet;
+use std::marker::PhantomData;
 
 use fj_math::Scalar;
 
 use crate::topology::{Cycle, Edge, Face, Vertex};
 
-use super::{
+use super::{stores::Stores, validate::Validate as _, Iter, ValidationResult};
-    stores::{Cycles, Edges, Vertices},
-    Geometry, Iter, StructuralIssues, ValidationError, ValidationResult,
-};
 
 /// The vertices of a shape
 pub struct Topology<'r> {
     pub(super) min_distance: Scalar,
-
+    pub(super) stores: Stores,
-    pub(super) geometry: Geometry<'r>,
+    pub(super) _lifetime: PhantomData<&'r ()>,
-
-    pub(super) vertices: &'r mut Vertices,
-    pub(super) edges: &'r mut Edges,
-    pub(super) cycles: &'r mut Cycles,
 }
 
 impl Topology<'_> {
@@ -44,19 +37,8 @@ impl Topology<'_> {
     /// 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> {
-        if !self.geometry.points.contains(&vertex.point) {
+        vertex.validate(self.min_distance, &self.stores)?;
-            return Err(StructuralIssues::default().into());
+        let handle = self.stores.vertices.insert(vertex);
-        }
-        for existing in self.vertices.iter() {
-            let distance =
-                (existing.get().point() - vertex.point()).magnitude();
-
-            if distance < self.min_distance {
-                return Err(ValidationError::Uniqueness);
-            }
-        }
-
-        let handle = self.vertices.add(vertex);
         Ok(handle)
     }
 
@@ -75,30 +57,8 @@ impl Topology<'_> {
     /// converted into curve coordinates, which is likely not the caller's
     /// intention.
     pub fn add_edge(&mut self, edge: Edge) -> ValidationResult<Edge> {
-        let mut missing_curve = None;
+        edge.validate(self.min_distance, &self.stores)?;
-        let mut missing_vertices = HashSet::new();
+        let handle = self.stores.edges.insert(edge);
-
-        if !self.geometry.curves.contains(&edge.curve) {
-            missing_curve = Some(edge.curve.clone());
-        }
-        for vertices in &edge.vertices {
-            for vertex in vertices {
-                if !self.vertices.contains(vertex) {
-                    missing_vertices.insert(vertex.clone());
-                }
-            }
-        }
-
-        if missing_curve.is_some() || !missing_vertices.is_empty() {
-            return Err(StructuralIssues {
-                missing_curve,
-                missing_vertices,
-                ..StructuralIssues::default()
-            }
-            .into());
-        }
-
-        let handle = self.edges.add(edge);
         Ok(handle)
     }
 
@@ -114,22 +74,8 @@ impl Topology<'_> {
     /// - That the cycle is not self-overlapping.
     /// - That there exists no duplicate cycle, with the same edges.
     pub fn add_cycle(&mut self, cycle: Cycle) -> ValidationResult<Cycle> {
-        let mut missing_edges = HashSet::new();
+        cycle.validate(self.min_distance, &self.stores)?;
-        for edge in &cycle.edges {
+        let handle = self.stores.cycles.insert(cycle);
-            if !self.edges.contains(edge) {
-                missing_edges.insert(edge.clone());
-            }
-        }
-
-        if !missing_edges.is_empty() {
-            return Err(StructuralIssues {
-                missing_edges,
-                ..StructuralIssues::default()
-            }
-            .into());
-        }
-
-        let handle = self.cycles.add(cycle);
         Ok(handle)
     }
 
@@ -139,36 +85,8 @@ impl Topology<'_> {
     /// 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 {
+        face.validate(self.min_distance, &self.stores)?;
-            surface,
+        let handle = self.stores.faces.insert(face);
-            exteriors,
-            interiors,
-            ..
-        } = &face
-        {
-            let mut missing_surface = None;
-            let mut missing_cycles = HashSet::new();
-
-            if !self.geometry.surfaces.contains(surface) {
-                missing_surface = Some(surface.clone());
-            }
-            for cycle in exteriors.iter().chain(interiors) {
-                if !self.cycles.contains(cycle) {
-                    missing_cycles.insert(cycle.clone());
-                }
-            }
-
-            if missing_surface.is_some() || !missing_cycles.is_empty() {
-                return Err(StructuralIssues {
-                    missing_surface,
-                    missing_cycles,
-                    ..StructuralIssues::default()
-                }
-                .into());
-            }
-        }
-
-        let handle = self.geometry.faces.add(face);
         Ok(handle)
     }
 
@@ -176,28 +94,28 @@ impl Topology<'_> {
     ///
     /// The caller must not make any assumptions about the order of vertices.
     pub fn vertices(&self) -> Iter<Vertex> {
-        self.vertices.iter()
+        self.stores.vertices.iter()
     }
 
     /// Access iterator over all edges
     ///
     /// The caller must not make any assumptions about the order of edges.
     pub fn edges(&self) -> Iter<Edge> {
-        self.edges.iter()
+        self.stores.edges.iter()
     }
 
     /// Access an iterator over all cycles
     ///
     /// The caller must not make any assumptions about the order of cycles.
     pub fn cycles(&self) -> Iter<Cycle> {
-        self.cycles.iter()
+        self.stores.cycles.iter()
     }
 
     /// Access an iterator over all faces
     ///
     /// The caller must not make any assumptions about the order of faces.
     pub fn faces(&self) -> Iter<Face> {
-        self.geometry.faces.iter()
+        self.stores.faces.iter()
     }
 }
 

fj-kernel/src/shape/validate.rs

@@ -1,11 +1,139 @@
 use std::collections::HashSet;
 
+use fj_math::Scalar;
+
 use crate::{
     geometry::{Curve, Surface},
-    topology::{Cycle, Edge, Vertex},
+    topology::{Cycle, Edge, Face, Vertex},
 };
 
-use super::Handle;
+use super::{stores::Stores, Handle};
+
+pub trait Validate {
+    fn validate(
+        &self,
+        min_distance: Scalar,
+        stores: &Stores,
+    ) -> Result<(), ValidationError>;
+}
+
+impl Validate for Vertex {
+    fn validate(
+        &self,
+        min_distance: Scalar,
+        stores: &Stores,
+    ) -> Result<(), ValidationError> {
+        if !stores.points.contains(&self.point) {
+            return Err(StructuralIssues::default().into());
+        }
+        for existing in stores.vertices.iter() {
+            let distance = (existing.get().point() - self.point()).magnitude();
+
+            if distance < min_distance {
+                return Err(ValidationError::Uniqueness);
+            }
+        }
+
+        Ok(())
+    }
+}
+
+impl Validate for Edge {
+    fn validate(
+        &self,
+        _: Scalar,
+        stores: &Stores,
+    ) -> Result<(), ValidationError> {
+        let mut missing_curve = None;
+        let mut missing_vertices = HashSet::new();
+
+        if !stores.curves.contains(&self.curve) {
+            missing_curve = Some(self.curve.clone());
+        }
+        for vertices in &self.vertices {
+            for vertex in vertices {
+                if !stores.vertices.contains(vertex) {
+                    missing_vertices.insert(vertex.clone());
+                }
+            }
+        }
+
+        if missing_curve.is_some() || !missing_vertices.is_empty() {
+            return Err(StructuralIssues {
+                missing_curve,
+                missing_vertices,
+                ..StructuralIssues::default()
+            }
+            .into());
+        }
+
+        Ok(())
+    }
+}
+
+impl Validate for Cycle {
+    fn validate(
+        &self,
+        _: Scalar,
+        stores: &Stores,
+    ) -> Result<(), ValidationError> {
+        let mut missing_edges = HashSet::new();
+        for edge in &self.edges {
+            if !stores.edges.contains(edge) {
+                missing_edges.insert(edge.clone());
+            }
+        }
+
+        if !missing_edges.is_empty() {
+            return Err(StructuralIssues {
+                missing_edges,
+                ..StructuralIssues::default()
+            }
+            .into());
+        }
+
+        Ok(())
+    }
+}
+
+impl Validate for Face {
+    fn validate(
+        &self,
+        _: Scalar,
+        stores: &Stores,
+    ) -> Result<(), ValidationError> {
+        if let Face::Face {
+            surface,
+            exteriors,
+            interiors,
+            ..
+        } = self
+        {
+            let mut missing_surface = None;
+            let mut missing_cycles = HashSet::new();
+
+            if !stores.surfaces.contains(surface) {
+                missing_surface = Some(surface.clone());
+            }
+            for cycle in exteriors.iter().chain(interiors) {
+                if !stores.cycles.contains(cycle) {
+                    missing_cycles.insert(cycle.clone());
+                }
+            }
+
+            if missing_surface.is_some() || !missing_cycles.is_empty() {
+                return Err(StructuralIssues {
+                    missing_surface,
+                    missing_cycles,
+                    ..StructuralIssues::default()
+                }
+                .into());
+            }
+        }
+
+        Ok(())
+    }
+}
 
 /// Returned by the various `add_` methods of the [`Shape`] API
 pub type ValidationResult<T> = Result<Handle<T>, ValidationError>;