From f81c4ca40267e33d0745579f0a1f1e56fb78b1a3 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 11 Mar 2022 13:23:03 +0100 Subject: [PATCH 1/6] Remove obsolete comment --- src/kernel/shape/topology.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/kernel/shape/topology.rs b/src/kernel/shape/topology.rs index cd5d970c2..b9a8b0d41 100644 --- a/src/kernel/shape/topology.rs +++ b/src/kernel/shape/topology.rs @@ -54,10 +54,6 @@ impl Topology<'_> { /// uniqueness. See documentation of [`crate::kernel`] for some context on /// that. pub fn add_vertex(&mut self, vertex: Vertex) -> ValidationResult { - // 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. for existing in &*self.vertices { let distance = (existing.point() - vertex.point()).magnitude(); From 12d362e6f4e768565d86373164c30a4f3f221da4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 11 Mar 2022 13:24:27 +0100 Subject: [PATCH 2/6] Implement structural validation of vertices --- src/kernel/shape/topology.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/kernel/shape/topology.rs b/src/kernel/shape/topology.rs index b9a8b0d41..f4fbd8912 100644 --- a/src/kernel/shape/topology.rs +++ b/src/kernel/shape/topology.rs @@ -54,6 +54,9 @@ impl Topology<'_> { /// uniqueness. See documentation of [`crate::kernel`] for some context on /// that. pub fn add_vertex(&mut self, vertex: Vertex) -> ValidationResult { + if !self.geometry.points.contains(vertex.point.storage()) { + return Err(ValidationError::Structural(())); + } for existing in &*self.vertices { let distance = (existing.point() - vertex.point()).magnitude(); @@ -223,7 +226,7 @@ mod tests { use crate::{ kernel::{ - shape::{handle::Handle, Shape}, + shape::{handle::Handle, Shape, ValidationError}, topology::{ edges::{Cycle, Edge}, vertices::Vertex, @@ -237,10 +240,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.])); From 90fc71c38d6fe66979c7b05c79e2a60914295e23 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 11 Mar 2022 16:14:19 +0100 Subject: [PATCH 3/6] Complete structural validation of edges --- src/kernel/shape/topology.rs | 37 ++++++++++++++++++++++++++++++------ src/kernel/shape/validate.rs | 25 ++++++++++++++++++------ 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/kernel/shape/topology.rs b/src/kernel/shape/topology.rs index f4fbd8912..12afc9176 100644 --- a/src/kernel/shape/topology.rs +++ b/src/kernel/shape/topology.rs @@ -97,7 +97,12 @@ impl Topology<'_> { /// 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 { + 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()) { @@ -106,8 +111,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); @@ -226,6 +234,7 @@ mod tests { use crate::{ kernel::{ + geometry::{Curve, Line}, shape::{handle::Handle, Shape, ValidationError}, topology::{ edges::{Cycle, Edge}, @@ -269,22 +278,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(()) } @@ -324,6 +342,13 @@ mod tests { } } + fn add_curve(&mut self) -> Handle { + self.geometry().add_curve(Curve::Line(Line::from_points([ + Point::from([0., 0., 0.]), + Point::from([1., 0., 0.]), + ]))) + } + fn add_vertex(&mut self) -> anyhow::Result> { let point = self.next_point; self.next_point.x += Scalar::ONE; diff --git a/src/kernel/shape/validate.rs b/src/kernel/shape/validate.rs index f08fa6d7a..819f0b935 100644 --- a/src/kernel/shape/validate.rs +++ b/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, + topology::{ + edges::{Cycle, Edge}, + faces::Face, + vertices::Vertex, + }, }; use super::handle::Handle; @@ -41,11 +44,21 @@ pub enum ValidationError { } impl ValidationError { + /// Indicate whether validation found a missing curve + #[cfg(test)] + pub fn missing_curve(&self, curve: &Handle) -> 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) -> bool { if let Self::Structural(missing) = self { - return missing.contains(vertex); + return missing.1.contains(vertex); } false @@ -76,7 +89,7 @@ impl Validatable for Vertex { } impl Validatable for Edge { - type Structural = HashSet>; + type Structural = (Option>, HashSet>); } impl Validatable for Cycle { From ea3144bf203221093a9b1bbb6784fd3ed10b0fd2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 11 Mar 2022 16:29:29 +0100 Subject: [PATCH 4/6] Implement structural validation of faces --- src/kernel/shape/topology.rs | 66 +++++++++++++++++++++++++++++++++++- src/kernel/shape/validate.rs | 26 ++++++++++++-- 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/src/kernel/shape/topology.rs b/src/kernel/shape/topology.rs index 12afc9176..48d46adab 100644 --- a/src/kernel/shape/topology.rs +++ b/src/kernel/shape/topology.rs @@ -203,6 +203,27 @@ impl Topology<'_> { /// Add a face to the shape pub fn add_face(&mut self, face: Face) -> ValidationResult { + 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(); @@ -234,10 +255,11 @@ mod tests { use crate::{ kernel::{ - geometry::{Curve, Line}, + geometry::{Curve, Line, Surface}, shape::{handle::Handle, Shape, ValidationError}, topology::{ edges::{Cycle, Edge}, + faces::Face, vertices::Vertex, }, }, @@ -329,6 +351,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>, @@ -349,6 +402,10 @@ mod tests { ]))) } + fn add_surface(&mut self) -> Handle { + self.geometry().add_surface(Surface::x_y_plane()) + } + fn add_vertex(&mut self) -> anyhow::Result> { let point = self.next_point; self.next_point.x += Scalar::ONE; @@ -364,6 +421,13 @@ mod tests { let edge = self.topology().add_line_segment(vertices)?; Ok(edge) } + + fn add_cycle(&mut self) -> anyhow::Result> { + let edge = self.add_edge()?; + let cycle = + self.topology().add_cycle(Cycle { edges: vec![edge] })?; + Ok(cycle) + } } impl Deref for TestShape { diff --git a/src/kernel/shape/validate.rs b/src/kernel/shape/validate.rs index 819f0b935..9d0785a3f 100644 --- a/src/kernel/shape/validate.rs +++ b/src/kernel/shape/validate.rs @@ -1,7 +1,7 @@ use std::collections::HashSet; use crate::kernel::{ - geometry::Curve, + geometry::{Curve, Surface}, topology::{ edges::{Cycle, Edge}, faces::Face, @@ -77,6 +77,28 @@ impl ValidationError { } } +impl ValidationError { + /// Indicate whether validation found a missing surface + #[cfg(test)] + pub fn missing_surface(&self, surface: &Handle) -> 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) -> 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. @@ -97,5 +119,5 @@ impl Validatable for Cycle { } impl Validatable for Face { - type Structural = (); + type Structural = (Option>, HashSet>); } From c0a2ef269de770227e9d7f2ebe538b40ece629f4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 11 Mar 2022 17:22:14 +0100 Subject: [PATCH 5/6] Update documentation of `Topology` --- src/kernel/shape/topology.rs | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/kernel/shape/topology.rs b/src/kernel/shape/topology.rs index 48d46adab..8fc3f71d9 100644 --- a/src/kernel/shape/topology.rs +++ b/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,9 +54,8 @@ 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 { if !self.geometry.points.contains(vertex.point.storage()) { return Err(ValidationError::Structural(())); @@ -84,18 +87,18 @@ impl Topology<'_> { /// 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 { let mut missing_curve = None; let mut missing_vertices = HashSet::new(); @@ -167,9 +170,8 @@ impl Topology<'_> { /// 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 /// @@ -202,6 +204,10 @@ impl Topology<'_> { } /// 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 { if let Face::Face { surface, cycles } = &face { let mut missing_surface = None; From 21dd27773791f8de7bb066967fcc02db010f06ff Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 11 Mar 2022 17:23:09 +0100 Subject: [PATCH 6/6] Group method in a way that makes more sense --- src/kernel/shape/topology.rs | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/kernel/shape/topology.rs b/src/kernel/shape/topology.rs index 8fc3f71d9..41fb5532a 100644 --- a/src/kernel/shape/topology.rs +++ b/src/kernel/shape/topology.rs @@ -78,13 +78,6 @@ 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> + '_ { - 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 @@ -161,13 +154,6 @@ 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> + '_ { - self.edges.iter().map(|storage| storage.handle()) - } - /// Add a cycle to the shape /// /// Validates that the cycle is structurally sound (i.e. the edges it refers @@ -198,11 +184,6 @@ impl Topology<'_> { Ok(handle) } - /// Access an iterator over all cycles - pub fn cycles(&self) -> impl Iterator> + '_ { - 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 @@ -238,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> + '_ { + 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> + '_ { + self.edges.iter().map(|storage| storage.handle()) + } + + /// Access an iterator over all cycles + pub fn cycles(&self) -> impl Iterator> + '_ { + self.cycles.iter().map(|storage| storage.handle()) + } + /// Access an iterator over all faces pub fn faces(&self) -> impl Iterator> + '_ { self.faces.iter().map(|storage| storage.handle())