From 51a4cf38a5a79bbed8de02361600f1223c146497 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Jun 2022 14:52:54 +0200 Subject: [PATCH 1/6] Derive a bunch of traits for `Validated` --- crates/fj-kernel/src/validation/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index 9eb52f9b7..3a173b094 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -84,6 +84,7 @@ impl Default for ValidationConfig { /// Wrapper around an object that indicates the object has been validated /// /// Returned by implementations of `Validate`. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Validated(T); impl Validated { From 6414fa106ed561ba56b0d78aac061728421a2b3b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Jun 2022 14:28:19 +0200 Subject: [PATCH 2/6] Move structural validation to new infrastructure --- crates/fj-kernel/src/shape/api.rs | 34 +++++++++--------- crates/fj-kernel/src/shape/mod.rs | 4 +-- crates/fj-kernel/src/shape/validate/mod.rs | 13 ++----- crates/fj-kernel/src/validation/mod.rs | 36 +++++++++++++++++-- .../validate => validation}/structural.rs | 20 ++++++----- 5 files changed, 66 insertions(+), 41 deletions(-) rename crates/fj-kernel/src/{shape/validate => validation}/structural.rs (88%) diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 1abf19f30..817269c36 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -264,7 +264,7 @@ mod tests { use crate::{ objects::{Curve, Cycle, Edge, Face, Surface, Vertex, VerticesOfEdge}, shape::{LocalForm, Shape}, - validation::ValidationError, + validation::{validate, ValidationConfig, ValidationError}, }; #[test] @@ -344,12 +344,12 @@ mod tests { let b = LocalForm::new(Point::from([2.]), b); // Shouldn't work. Nothing has been added to `shape`. - let err = shape - .insert(Edge { - curve: LocalForm::canonical_only(curve.clone()), - vertices: VerticesOfEdge::from_vertices([a.clone(), b.clone()]), - }) - .unwrap_err(); + shape.insert(Edge { + curve: LocalForm::canonical_only(curve.clone()), + vertices: VerticesOfEdge::from_vertices([a.clone(), b.clone()]), + })?; + 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())); @@ -402,7 +402,9 @@ mod tests { // Trying to refer to edge that is not from the same shape. Should fail. let edge = Edge::builder(&mut other) .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]])?; - let err = shape.insert(Cycle::new(vec![edge.clone()])).unwrap_err(); + shape.insert(Cycle::new(vec![edge.clone()]))?; + let err = + validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); assert!(err.missing_edge(&edge)); // Referring to edge that *is* from the same shape. Should work. @@ -425,14 +427,14 @@ mod tests { .build_polygon(triangle)?; // Nothing has been added to `shape`. Should fail. - let err = shape - .insert(Face::new( - surface.clone(), - vec![cycle.clone()], - Vec::new(), - [255, 0, 0, 255], - )) - .unwrap_err(); + shape.insert(Face::new( + surface.clone(), + vec![cycle.clone()], + Vec::new(), + [255, 0, 0, 255], + ))?; + let err = + validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); assert!(err.missing_surface(&surface)); assert!(err.missing_cycle(&cycle.canonical())); diff --git a/crates/fj-kernel/src/shape/mod.rs b/crates/fj-kernel/src/shape/mod.rs index 8867b825e..93147e48d 100644 --- a/crates/fj-kernel/src/shape/mod.rs +++ b/crates/fj-kernel/src/shape/mod.rs @@ -17,7 +17,5 @@ pub use self::{ object::Object, stores::{Handle, Iter}, update::Update, - validate::{ - DuplicateEdge, StructuralIssues, UniquenessIssues, ValidationResult, - }, + validate::{DuplicateEdge, UniquenessIssues, ValidationResult}, }; diff --git a/crates/fj-kernel/src/shape/validate/mod.rs b/crates/fj-kernel/src/shape/validate/mod.rs index 56e2ea342..9ce7fff90 100644 --- a/crates/fj-kernel/src/shape/validate/mod.rs +++ b/crates/fj-kernel/src/shape/validate/mod.rs @@ -1,10 +1,6 @@ -mod structural; mod uniqueness; -pub use self::{ - structural::StructuralIssues, - uniqueness::{DuplicateEdge, UniquenessIssues}, -}; +pub use self::uniqueness::{DuplicateEdge, UniquenessIssues}; use fj_math::Scalar; @@ -74,7 +70,6 @@ impl Validate for Edge<3> { _: Scalar, stores: &Stores, ) -> Result<(), ValidationError> { - structural::validate_edge(self, stores)?; uniqueness::validate_edge(self, handle, &stores.edges)?; Ok(()) @@ -94,9 +89,8 @@ impl Validate for Cycle<3> { &self, _: Option<&Handle>, _: Scalar, - stores: &Stores, + _: &Stores, ) -> Result<(), ValidationError> { - structural::validate_cycle(self, stores)?; Ok(()) } } @@ -106,9 +100,8 @@ impl Validate for Face { &self, _: Option<&Handle>, _: Scalar, - stores: &Stores, + _: &Stores, ) -> Result<(), ValidationError> { - structural::validate_face(self, stores)?; Ok(()) } } diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index 3a173b094..c9f021745 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -25,16 +25,20 @@ //! [`Shape`]: crate::shape::Shape mod coherence; +mod structural; -pub use self::coherence::{CoherenceIssues, CoherenceMismatch}; +pub use self::{ + coherence::{CoherenceIssues, CoherenceMismatch}, + structural::StructuralIssues, +}; -use std::ops::Deref; +use std::{collections::HashSet, ops::Deref}; use fj_math::Scalar; use crate::{ objects::{Curve, Cycle, Edge, Surface, Vertex}, - shape::{Handle, Shape, StructuralIssues, UniquenessIssues}, + shape::{Handle, Shape, UniquenessIssues}, }; /// Validate the given [`Shape`] @@ -42,8 +46,34 @@ pub fn validate( shape: Shape, config: &ValidationConfig, ) -> Result, ValidationError> { + let mut curves = HashSet::new(); + let mut cycles = HashSet::new(); + let mut edges = HashSet::new(); + let mut surfaces = HashSet::new(); + let mut vertices = HashSet::new(); + + for curve in shape.curves() { + curves.insert(curve); + } + for vertex in shape.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)?; + + edges.insert(edge); + } + for cycle in shape.cycles() { + structural::validate_cycle(&cycle.get(), &edges)?; + + cycles.insert(cycle); + } + for surface in shape.surfaces() { + surfaces.insert(surface); + } + for face in shape.faces() { + structural::validate_face(&face.get(), &cycles, &surfaces)?; } Ok(Validated(shape)) diff --git a/crates/fj-kernel/src/shape/validate/structural.rs b/crates/fj-kernel/src/validation/structural.rs similarity index 88% rename from crates/fj-kernel/src/shape/validate/structural.rs rename to crates/fj-kernel/src/validation/structural.rs index 59b6279df..596176dc5 100644 --- a/crates/fj-kernel/src/shape/validate/structural.rs +++ b/crates/fj-kernel/src/validation/structural.rs @@ -2,21 +2,22 @@ use std::{collections::HashSet, fmt}; use crate::{ objects::{Curve, Cycle, Edge, Face, Surface, Vertex}, - shape::{stores::Stores, Handle}, + shape::Handle, }; pub fn validate_edge( edge: &Edge<3>, - stores: &Stores, + curves: &HashSet>>, + vertices: &HashSet>, ) -> Result<(), StructuralIssues> { let mut missing_curve = None; let mut missing_vertices = HashSet::new(); - if !stores.curves.contains(&edge.curve.canonical()) { + if !curves.contains(&edge.curve.canonical()) { missing_curve = Some(edge.curve.canonical()); } for vertex in edge.vertices.iter() { - if !stores.vertices.contains(&vertex.canonical()) { + if !vertices.contains(&vertex.canonical()) { missing_vertices.insert(vertex.canonical().clone()); } } @@ -34,13 +35,13 @@ pub fn validate_edge( pub fn validate_cycle( cycle: &Cycle<3>, - stores: &Stores, + edges: &HashSet>>, ) -> Result<(), StructuralIssues> { let mut missing_edges = HashSet::new(); for edge in &cycle.edges { let edge = edge.canonical(); - if !stores.edges.contains(&edge) { + if !edges.contains(&edge) { missing_edges.insert(edge.clone()); } } @@ -57,19 +58,20 @@ pub fn validate_cycle( pub fn validate_face( face: &Face, - stores: &Stores, + cycles: &HashSet>>, + surfaces: &HashSet>, ) -> Result<(), StructuralIssues> { if let Face::Face(face) = face { let mut missing_surface = None; let mut missing_cycles = HashSet::new(); - if !stores.surfaces.contains(&face.surface) { + if !surfaces.contains(&face.surface) { missing_surface = Some(face.surface.clone()); } for cycle in face.exteriors.as_handle().chain(face.interiors.as_handle()) { - if !stores.cycles.contains(&cycle) { + if !cycles.contains(&cycle) { missing_cycles.insert(cycle); } } From 42634e669f789ee124d88bc1ce929cf226c065e3 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Jun 2022 14:57:32 +0200 Subject: [PATCH 3/6] Update test name I decided that this naming scheme makes more sense, given the validation implementations are sorted into modules by type of validation, not by object. --- crates/fj-kernel/src/validation/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index c9f021745..65ab6fd66 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -218,7 +218,7 @@ mod tests { }; #[test] - fn edge_coherence() -> anyhow::Result<()> { + fn coherence_edge() -> anyhow::Result<()> { let mut shape = Shape::new(); Edge::builder(&mut shape) .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]])? From facc820c23b3d27ca7a88b034a3a6cf97270758a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Jun 2022 15:01:32 +0200 Subject: [PATCH 4/6] Refactor --- crates/fj-kernel/src/validation/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index 65ab6fd66..e067ca9ff 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -214,7 +214,7 @@ mod tests { use crate::{ objects::Edge, shape::{LocalForm, Shape}, - validation::ValidationConfig, + validation::{validate, ValidationConfig}, }; #[test] @@ -242,7 +242,7 @@ mod tests { }) .validate()?; - let result = super::validate( + let result = validate( shape.clone(), &ValidationConfig { identical_max_distance: deviation * 2., @@ -251,7 +251,7 @@ mod tests { ); assert!(result.is_ok()); - let result = super::validate( + let result = validate( shape, &ValidationConfig { identical_max_distance: deviation / 2., From ed2be4443d43630a43bde984dc87abb90679605e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Jun 2022 15:02:47 +0200 Subject: [PATCH 5/6] Move tests They exercise the new validation infrastructure, so it makes sense to move them there. --- crates/fj-kernel/src/shape/api.rs | 100 +----------------------- crates/fj-kernel/src/validation/mod.rs | 102 ++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 101 deletions(-) diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 817269c36..9eb516fff 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -264,7 +264,7 @@ mod tests { use crate::{ objects::{Curve, Cycle, Edge, Face, Surface, Vertex, VerticesOfEdge}, shape::{LocalForm, Shape}, - validation::{validate, ValidationConfig, ValidationError}, + validation::ValidationError, }; #[test] @@ -331,45 +331,6 @@ mod tests { Ok(()) } - #[test] - fn add_edge() -> anyhow::Result<()> { - let mut shape = Shape::new(); - let mut other = Shape::new(); - - let curve = other.insert(Curve::x_axis())?; - let a = Vertex::builder(&mut other).build_from_point([1., 0., 0.])?; - let b = Vertex::builder(&mut other).build_from_point([2., 0., 0.])?; - - let a = LocalForm::new(Point::from([1.]), a); - let b = LocalForm::new(Point::from([2.]), b); - - // Shouldn't work. Nothing has been added to `shape`. - shape.insert(Edge { - curve: LocalForm::canonical_only(curve.clone()), - vertices: VerticesOfEdge::from_vertices([a.clone(), b.clone()]), - })?; - 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())); - - let curve = shape.insert(Curve::x_axis())?; - let a = Vertex::builder(&mut shape).build_from_point([1., 0., 0.])?; - let b = Vertex::builder(&mut shape).build_from_point([2., 0., 0.])?; - - let a = LocalForm::new(Point::from([1.]), a); - let b = LocalForm::new(Point::from([2.]), b); - - // Everything has been added to `shape` now. Should work! - shape.insert(Edge { - curve: LocalForm::canonical_only(curve), - vertices: VerticesOfEdge::from_vertices([a, b]), - })?; - - Ok(()) - } - #[test] fn add_edge_uniqueness() -> anyhow::Result<()> { let mut shape = Shape::new(); @@ -393,63 +354,4 @@ mod tests { Ok(()) } - - #[test] - fn add_cycle() -> anyhow::Result<()> { - let mut shape = Shape::new(); - let mut other = Shape::new(); - - // Trying to refer to edge that is not from the same shape. Should fail. - let edge = Edge::builder(&mut other) - .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]])?; - shape.insert(Cycle::new(vec![edge.clone()]))?; - let err = - validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); - assert!(err.missing_edge(&edge)); - - // Referring to edge that *is* from the same shape. Should work. - let edge = Edge::builder(&mut shape) - .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]])?; - shape.insert(Cycle::new(vec![edge]))?; - - Ok(()) - } - - #[test] - fn add_face() -> anyhow::Result<()> { - let mut shape = Shape::new(); - let mut other = Shape::new(); - - let triangle = [[0., 0.], [1., 0.], [0., 1.]]; - - let surface = other.insert(Surface::xy_plane())?; - let cycle = Cycle::builder(surface.get(), &mut other) - .build_polygon(triangle)?; - - // Nothing has been added to `shape`. Should fail. - shape.insert(Face::new( - surface.clone(), - vec![cycle.clone()], - Vec::new(), - [255, 0, 0, 255], - ))?; - let err = - validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); - assert!(err.missing_surface(&surface)); - assert!(err.missing_cycle(&cycle.canonical())); - - let surface = shape.insert(Surface::xy_plane())?; - let cycle = Cycle::builder(surface.get(), &mut shape) - .build_polygon(triangle)?; - - // Everything has been added to `shape` now. Should work! - shape.insert(Face::new( - surface, - vec![cycle], - Vec::new(), - [255, 0, 0, 255], - ))?; - - Ok(()) - } } diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index e067ca9ff..d3aa60238 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -209,10 +209,10 @@ impl ValidationError { #[cfg(test)] mod tests { - use fj_math::Scalar; + use fj_math::{Point, Scalar}; use crate::{ - objects::Edge, + objects::{Curve, Cycle, Edge, Face, Surface, Vertex, VerticesOfEdge}, shape::{LocalForm, Shape}, validation::{validate, ValidationConfig}, }; @@ -262,4 +262,102 @@ mod tests { Ok(()) } + + #[test] + fn add_cycle() -> anyhow::Result<()> { + let mut shape = Shape::new(); + let mut other = Shape::new(); + + // Trying to refer to edge that is not from the same shape. Should fail. + let edge = Edge::builder(&mut other) + .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]])?; + shape.insert(Cycle::new(vec![edge.clone()]))?; + let err = + validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); + assert!(err.missing_edge(&edge)); + + // Referring to edge that *is* from the same shape. Should work. + let edge = Edge::builder(&mut shape) + .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]])?; + shape.insert(Cycle::new(vec![edge]))?; + + Ok(()) + } + + #[test] + fn add_edge() -> anyhow::Result<()> { + let mut shape = Shape::new(); + let mut other = Shape::new(); + + let curve = other.insert(Curve::x_axis())?; + let a = Vertex::builder(&mut other).build_from_point([1., 0., 0.])?; + let b = Vertex::builder(&mut other).build_from_point([2., 0., 0.])?; + + let a = LocalForm::new(Point::from([1.]), a); + let b = LocalForm::new(Point::from([2.]), b); + + // Shouldn't work. Nothing has been added to `shape`. + shape.insert(Edge { + curve: LocalForm::canonical_only(curve.clone()), + vertices: VerticesOfEdge::from_vertices([a.clone(), b.clone()]), + })?; + 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())); + + let curve = shape.insert(Curve::x_axis())?; + let a = Vertex::builder(&mut shape).build_from_point([1., 0., 0.])?; + let b = Vertex::builder(&mut shape).build_from_point([2., 0., 0.])?; + + let a = LocalForm::new(Point::from([1.]), a); + let b = LocalForm::new(Point::from([2.]), b); + + // Everything has been added to `shape` now. Should work! + shape.insert(Edge { + curve: LocalForm::canonical_only(curve), + vertices: VerticesOfEdge::from_vertices([a, b]), + })?; + + Ok(()) + } + + #[test] + fn add_face() -> anyhow::Result<()> { + let mut shape = Shape::new(); + let mut other = Shape::new(); + + let triangle = [[0., 0.], [1., 0.], [0., 1.]]; + + let surface = other.insert(Surface::xy_plane())?; + let cycle = Cycle::builder(surface.get(), &mut other) + .build_polygon(triangle)?; + + // Nothing has been added to `shape`. Should fail. + shape.insert(Face::new( + surface.clone(), + vec![cycle.clone()], + Vec::new(), + [255, 0, 0, 255], + ))?; + let err = + validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); + assert!(err.missing_surface(&surface)); + assert!(err.missing_cycle(&cycle.canonical())); + + let surface = shape.insert(Surface::xy_plane())?; + let cycle = Cycle::builder(surface.get(), &mut shape) + .build_polygon(triangle)?; + + // Everything has been added to `shape` now. Should work! + shape.insert(Face::new( + surface, + vec![cycle], + Vec::new(), + [255, 0, 0, 255], + ))?; + + Ok(()) + } } From 0388106c1a541fca13fd09670702f94444d0e965 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Jun 2022 15:04:05 +0200 Subject: [PATCH 6/6] Update test names --- crates/fj-kernel/src/validation/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index d3aa60238..18a08bdda 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -264,7 +264,7 @@ mod tests { } #[test] - fn add_cycle() -> anyhow::Result<()> { + fn structural_cycle() -> anyhow::Result<()> { let mut shape = Shape::new(); let mut other = Shape::new(); @@ -285,7 +285,7 @@ mod tests { } #[test] - fn add_edge() -> anyhow::Result<()> { + fn structural_edge() -> anyhow::Result<()> { let mut shape = Shape::new(); let mut other = Shape::new(); @@ -324,7 +324,7 @@ mod tests { } #[test] - fn add_face() -> anyhow::Result<()> { + fn structural_face() -> anyhow::Result<()> { let mut shape = Shape::new(); let mut other = Shape::new();