Merge pull request #714 from hannobraun/validation

Don't rely on access to `Handle`s in validation

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

@@ -40,7 +40,7 @@ use fj_math::Scalar;
 
 use crate::{
     objects::{Curve, Cycle, Edge, Surface, Vertex},
-    shape::{Handle, Shape},
+    shape::Shape,
 };
 
 /// Validate the given [`Shape`]
@@ -54,31 +54,31 @@ pub fn validate(
     let mut surfaces = HashSet::new();
     let mut vertices = HashSet::new();
 
-    for curve in shape.curves() {
+    for curve in shape.curves().map(|handle| handle.get()) {
         curves.insert(curve);
     }
-    for vertex in shape.vertices() {
-        uniqueness::validate_vertex(&vertex.get(), &vertices)?;
+    for vertex in shape.vertices().map(|handle| handle.get()) {
+        uniqueness::validate_vertex(&vertex, &vertices)?;
 
         vertices.insert(vertex);
     }
-    for edge in shape.edges() {
-        coherence::validate_edge(&edge.get(), config.identical_max_distance)?;
-        structural::validate_edge(&edge.get(), &curves, &vertices)?;
-        uniqueness::validate_edge(&edge.get(), &edges)?;
+    for edge in shape.edges().map(|handle| handle.get()) {
+        coherence::validate_edge(&edge, config.identical_max_distance)?;
+        structural::validate_edge(&edge, &curves, &vertices)?;
+        uniqueness::validate_edge(&edge, &edges)?;
 
         edges.insert(edge);
     }
-    for cycle in shape.cycles() {
-        structural::validate_cycle(&cycle.get(), &edges)?;
+    for cycle in shape.cycles().map(|handle| handle.get()) {
+        structural::validate_cycle(&cycle, &edges)?;
 
         cycles.insert(cycle);
     }
-    for surface in shape.surfaces() {
+    for surface in shape.surfaces().map(|handle| handle.get()) {
         surfaces.insert(surface);
     }
-    for face in shape.faces() {
-        structural::validate_face(&face.get(), &cycles, &surfaces)?;
+    for face in shape.faces().map(|handle| handle.get()) {
+        structural::validate_face(&face, &cycles, &surfaces)?;
     }
 
     Ok(Validated(shape))
@@ -160,7 +160,7 @@ pub enum ValidationError {
 
 impl ValidationError {
     /// Indicate whether validation found a missing curve
-    pub fn missing_curve(&self, curve: &Handle<Curve<3>>) -> bool {
+    pub fn missing_curve(&self, curve: &Curve<3>) -> bool {
         if let Self::Structural(StructuralIssues { missing_curve, .. }) = self {
             return missing_curve.as_ref() == Some(curve);
         }
@@ -169,7 +169,7 @@ impl ValidationError {
     }
 
     /// Indicate whether validation found a missing vertex
-    pub fn missing_vertex(&self, vertex: &Handle<Vertex>) -> bool {
+    pub fn missing_vertex(&self, vertex: &Vertex) -> bool {
         if let Self::Structural(StructuralIssues {
             missing_vertices, ..
         }) = self
@@ -181,7 +181,7 @@ impl ValidationError {
     }
 
     /// Indicate whether validation found a missing edge
-    pub fn missing_edge(&self, edge: &Handle<Edge<3>>) -> bool {
+    pub fn missing_edge(&self, edge: &Edge<3>) -> bool {
         if let Self::Structural(StructuralIssues { missing_edges, .. }) = self {
             return missing_edges.contains(edge);
         }
@@ -190,7 +190,7 @@ impl ValidationError {
     }
 
     /// Indicate whether validation found a missing surface
-    pub fn missing_surface(&self, surface: &Handle<Surface>) -> bool {
+    pub fn missing_surface(&self, surface: &Surface) -> bool {
         if let Self::Structural(StructuralIssues {
             missing_surface, ..
         }) = self
@@ -202,7 +202,7 @@ impl ValidationError {
     }
 
     /// Indicate whether validation found a missing cycle
-    pub fn missing_cycle(&self, cycle: &Handle<Cycle<3>>) -> bool {
+    pub fn missing_cycle(&self, cycle: &Cycle<3>) -> bool {
         if let Self::Structural(StructuralIssues { missing_cycles, .. }) = self
         {
             return missing_cycles.contains(cycle);
@@ -274,7 +274,7 @@ mod tests {
         shape.insert(Cycle::new(vec![edge.clone()]));
         let err =
             validate(shape.clone(), &ValidationConfig::default()).unwrap_err();
-        assert!(err.missing_edge(&edge));
+        assert!(err.missing_edge(&edge.get()));
 
         // Referring to edge that *is* from the same shape. Should work.
         let edge = Edge::builder(&mut shape)
@@ -301,9 +301,9 @@ mod tests {
         });
         let err =
             validate(shape.clone(), &ValidationConfig::default()).unwrap_err();
-        assert!(err.missing_curve(&curve));
-        assert!(err.missing_vertex(&a.canonical()));
-        assert!(err.missing_vertex(&b.canonical()));
+        assert!(err.missing_curve(&curve.get()));
+        assert!(err.missing_vertex(&a.canonical().get()));
+        assert!(err.missing_vertex(&b.canonical().get()));
 
         let curve = shape.insert(Curve::x_axis());
         let a = Vertex::builder(&mut shape).build_from_point([1., 0., 0.]);
@@ -339,8 +339,8 @@ mod tests {
         ));
         let err =
             validate(shape.clone(), &ValidationConfig::default()).unwrap_err();
-        assert!(err.missing_surface(&surface));
-        assert!(err.missing_cycle(&cycle.canonical()));
+        assert!(err.missing_surface(&surface.get()));
+        assert!(err.missing_cycle(&cycle.canonical().get()));
 
         let surface = shape.insert(Surface::xy_plane());
         let cycle =

crates/fj-kernel/src/validation/structural.rs

@@ -1,24 +1,21 @@
 use std::{collections::HashSet, fmt};
 
-use crate::{
-    objects::{Curve, Cycle, Edge, Face, Surface, Vertex},
-    shape::Handle,
-};
+use crate::objects::{Curve, Cycle, Edge, Face, Surface, Vertex};
 
 pub fn validate_edge(
     edge: &Edge<3>,
-    curves: &HashSet<Handle<Curve<3>>>,
-    vertices: &HashSet<Handle<Vertex>>,
+    curves: &HashSet<Curve<3>>,
+    vertices: &HashSet<Vertex>,
 ) -> Result<(), StructuralIssues> {
     let mut missing_curve = None;
     let mut missing_vertices = HashSet::new();
 
-    if !curves.contains(&edge.curve.canonical()) {
-        missing_curve = Some(edge.curve.canonical());
+    if !curves.contains(&edge.curve()) {
+        missing_curve = Some(edge.curve.canonical().get());
     }
-    for vertex in edge.vertices.iter() {
-        if !vertices.contains(&vertex.canonical()) {
-            missing_vertices.insert(vertex.canonical().clone());
+    for vertex in edge.vertices().into_iter().flatten() {
+        if !vertices.contains(&vertex) {
+            missing_vertices.insert(vertex);
         }
     }
 
@@ -35,14 +32,12 @@ pub fn validate_edge(
 
 pub fn validate_cycle(
     cycle: &Cycle<3>,
-    edges: &HashSet<Handle<Edge<3>>>,
+    edges: &HashSet<Edge<3>>,
 ) -> Result<(), StructuralIssues> {
     let mut missing_edges = HashSet::new();
-    for edge in &cycle.edges {
-        let edge = edge.canonical();
-
+    for edge in cycle.edges() {
         if !edges.contains(&edge) {
-            missing_edges.insert(edge.clone());
+            missing_edges.insert(edge);
         }
     }
 
@@ -58,19 +53,17 @@ pub fn validate_cycle(
 
 pub fn validate_face(
     face: &Face,
-    cycles: &HashSet<Handle<Cycle<3>>>,
-    surfaces: &HashSet<Handle<Surface>>,
+    cycles: &HashSet<Cycle<3>>,
+    surfaces: &HashSet<Surface>,
 ) -> Result<(), StructuralIssues> {
     if let Face::Face(face) = face {
         let mut missing_surface = None;
         let mut missing_cycles = HashSet::new();
 
-        if !surfaces.contains(&face.surface) {
-            missing_surface = Some(face.surface.clone());
+        if !surfaces.contains(&face.surface()) {
+            missing_surface = Some(face.surface.get());
         }
-        for cycle in
-            face.exteriors.as_handle().chain(face.interiors.as_handle())
-        {
+        for cycle in face.all_cycles() {
             if !cycles.contains(&cycle) {
                 missing_cycles.insert(cycle);
             }
@@ -94,50 +87,50 @@ pub fn validate_face(
 #[derive(Debug, Default, thiserror::Error)]
 pub struct StructuralIssues {
     /// Missing curve found in edge validation
-    pub missing_curve: Option<Handle<Curve<3>>>,
+    pub missing_curve: Option<Curve<3>>,
 
     /// Missing vertices found in edge validation
-    pub missing_vertices: HashSet<Handle<Vertex>>,
+    pub missing_vertices: HashSet<Vertex>,
 
     /// Missing edges found in cycle validation
-    pub missing_edges: HashSet<Handle<Edge<3>>>,
+    pub missing_edges: HashSet<Edge<3>>,
 
     /// Missing surface found in face validation
-    pub missing_surface: Option<Handle<Surface>>,
+    pub missing_surface: Option<Surface>,
 
     /// Missing cycles found in face validation
-    pub missing_cycles: HashSet<Handle<Cycle<3>>>,
+    pub missing_cycles: HashSet<Cycle<3>>,
 }
 
 impl fmt::Display for StructuralIssues {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         writeln!(f, "Structural issues found:")?;
 
         if let Some(curve) = &self.missing_curve {
-            writeln!(f, "- Missing curve: {:?}", curve.get())?;
+            writeln!(f, "- Missing curve: {:?}", curve)?;
         }
         if !self.missing_vertices.is_empty() {
             writeln!(f, "- Missing vertices:")?;
 
             for vertex in &self.missing_vertices {
-                writeln!(f, "  - {:?}", vertex.get())?;
+                writeln!(f, "  - {:?}", vertex)?;
             }
         }
         if !self.missing_edges.is_empty() {
             writeln!(f, "- Missing edges:")?;
 
             for edge in &self.missing_edges {
-                writeln!(f, "  - {}", edge.get())?;
+                writeln!(f, "  - {}", edge)?;
             }
         }
         if let Some(surface) = &self.missing_surface {
-            writeln!(f, "- Missing surface: {:?}", surface.get())?;
+            writeln!(f, "- Missing surface: {:?}", surface)?;
         }
         if !self.missing_cycles.is_empty() {
             writeln!(f, "- Missing cycles:")?;
 
             for cycle in &self.missing_cycles {
-                writeln!(f, "  - {:?}", cycle.get())?;
+                writeln!(f, "  - {:?}", cycle)?;
             }
         }
 

crates/fj-kernel/src/validation/uniqueness.rs

@@ -1,18 +1,15 @@
 use std::{collections::HashSet, fmt};
 
-use crate::{
-    objects::{Edge, Vertex},
-    shape::Handle,
-};
+use crate::objects::{Edge, Vertex};
 
 pub fn validate_vertex(
     vertex: &Vertex,
-    vertices: &HashSet<Handle<Vertex>>,
+    vertices: &HashSet<Vertex>,
 ) -> Result<(), UniquenessIssues> {
     for existing in vertices {
-        if &existing.get() == vertex {
+        if existing == vertex {
             return Err(UniquenessIssues {
-                duplicate_vertex: Some(existing.clone()),
+                duplicate_vertex: Some(*existing),
                 ..UniquenessIssues::default()
             });
         }
@@ -30,13 +27,13 @@ pub fn validate_vertex(
 /// this code will have to be updated.
 pub fn validate_edge(
     edge: &Edge<3>,
-    edges: &HashSet<Handle<Edge<3>>>,
+    edges: &HashSet<Edge<3>>,
 ) -> Result<(), UniquenessIssues> {
     for existing in edges {
-        if existing.get().vertices.are_same(&edge.vertices) {
+        if existing.vertices.are_same(&edge.vertices) {
             return Err(UniquenessIssues {
                 duplicate_edge: Some(DuplicateEdge {
-                    existing: existing.get(),
+                    existing: existing.clone(),
                     new: edge.clone(),
                 }),
                 ..UniquenessIssues::default()
@@ -61,7 +58,7 @@ pub fn validate_edge(
 #[derive(Debug, Default, thiserror::Error)]
 pub struct UniquenessIssues {
     /// Duplicate vertex found
-    pub duplicate_vertex: Option<Handle<Vertex>>,
+    pub duplicate_vertex: Option<Vertex>,
 
     /// Duplicate edge found
     pub duplicate_edge: Option<DuplicateEdge>,