diff --git a/crates/fj-core/src/algorithms/intersect/curve_face.rs b/crates/fj-core/src/algorithms/intersect/curve_face.rs index 23375ca3f..663b936b4 100644 --- a/crates/fj-core/src/algorithms/intersect/curve_face.rs +++ b/crates/fj-core/src/algorithms/intersect/curve_face.rs @@ -205,8 +205,6 @@ mod tests { let expected = CurveFaceIntersection::from_intervals([[[1.], [2.]], [[4.], [5.]]]); assert_eq!(CurveFaceIntersection::compute(&path, &face), expected); - - services.only_validate(face); } #[test] diff --git a/crates/fj-core/src/algorithms/intersect/face_face.rs b/crates/fj-core/src/algorithms/intersect/face_face.rs index 67c9d9fc4..b007a470e 100644 --- a/crates/fj-core/src/algorithms/intersect/face_face.rs +++ b/crates/fj-core/src/algorithms/intersect/face_face.rs @@ -102,8 +102,6 @@ mod tests { let intersection = FaceFaceIntersection::compute([&a, &b]); assert!(intersection.is_none()); - - services.only_validate([a, b]); } #[test] @@ -147,7 +145,5 @@ mod tests { intersection_intervals: expected_intervals }) ); - - services.only_validate([a, b]); } } diff --git a/crates/fj-core/src/algorithms/intersect/face_point.rs b/crates/fj-core/src/algorithms/intersect/face_point.rs index c1cd0d673..f18c95820 100644 --- a/crates/fj-core/src/algorithms/intersect/face_point.rs +++ b/crates/fj-core/src/algorithms/intersect/face_point.rs @@ -165,8 +165,6 @@ mod tests { let intersection = (&face, &point).intersect(); assert_eq!(intersection, None); - - services.only_validate(face); } #[test] @@ -193,8 +191,6 @@ mod tests { intersection, Some(FacePointIntersection::PointIsInsideFace) ); - - services.only_validate(face); } #[test] @@ -221,8 +217,6 @@ mod tests { intersection, Some(FacePointIntersection::PointIsInsideFace) ); - - services.only_validate(face); } #[test] @@ -249,8 +243,6 @@ mod tests { intersection, Some(FacePointIntersection::PointIsInsideFace) ); - - services.only_validate(face); } #[test] @@ -277,8 +269,6 @@ mod tests { intersection, Some(FacePointIntersection::PointIsInsideFace) ); - - services.only_validate(face); } #[test] @@ -311,8 +301,6 @@ mod tests { intersection, Some(FacePointIntersection::PointIsInsideFace) ); - - services.only_validate(face); } #[test] @@ -347,8 +335,6 @@ mod tests { intersection, Some(FacePointIntersection::PointIsOnEdge(edge.clone())) ); - - services.only_validate(face); } #[test] @@ -384,7 +370,5 @@ mod tests { intersection, Some(FacePointIntersection::PointIsOnVertex(vertex)) ); - - services.only_validate(face); } } diff --git a/crates/fj-core/src/algorithms/intersect/ray_face.rs b/crates/fj-core/src/algorithms/intersect/ray_face.rs index 8e1462d8e..24ce26c20 100644 --- a/crates/fj-core/src/algorithms/intersect/ray_face.rs +++ b/crates/fj-core/src/algorithms/intersect/ray_face.rs @@ -183,8 +183,6 @@ mod tests { let face = face.translate([-1., 0., 0.], &mut services); assert_eq!((&ray, &face).intersect(), None); - - services.only_validate(face); } #[test] @@ -212,8 +210,6 @@ mod tests { (&ray, &face).intersect(), Some(RayFaceIntersection::RayHitsFace) ); - - services.only_validate(face); } #[test] @@ -238,8 +234,6 @@ mod tests { let face = face.translate([0., 0., 2.], &mut services); assert_eq!((&ray, &face).intersect(), None); - - services.only_validate(face); } #[test] @@ -274,8 +268,6 @@ mod tests { (&ray, &face).intersect(), Some(RayFaceIntersection::RayHitsEdge(edge.clone())) ); - - services.only_validate(face); } #[test] @@ -311,8 +303,6 @@ mod tests { (&ray, &face).intersect(), Some(RayFaceIntersection::RayHitsVertex(vertex)) ); - - services.only_validate(face); } #[test] @@ -339,8 +329,6 @@ mod tests { (&ray, &face).intersect(), Some(RayFaceIntersection::RayHitsFaceAndAreParallel) ); - - services.only_validate(face); } #[test] @@ -365,7 +353,5 @@ mod tests { let face = face.translate([0., 0., 1.], &mut services); assert_eq!((&ray, &face).intersect(), None); - - services.only_validate(face); } } diff --git a/crates/fj-core/src/algorithms/triangulate/mod.rs b/crates/fj-core/src/algorithms/triangulate/mod.rs index d84787dc0..e0340dbce 100644 --- a/crates/fj-core/src/algorithms/triangulate/mod.rs +++ b/crates/fj-core/src/algorithms/triangulate/mod.rs @@ -109,7 +109,6 @@ mod tests { }) .insert(&mut services) }); - services.only_validate(&face); let a = Point::from(a).to_xyz(); let b = Point::from(b).to_xyz(); @@ -157,7 +156,6 @@ mod tests { .insert(&mut services) }, ); - services.only_validate(&face); let triangles = triangulate(face)?; @@ -222,7 +220,6 @@ mod tests { .insert(&mut services) }, ); - services.only_validate(&face); let triangles = triangulate(face)?; diff --git a/crates/fj-core/src/objects/mod.rs b/crates/fj-core/src/objects/mod.rs index 606edc845..be6383c04 100644 --- a/crates/fj-core/src/objects/mod.rs +++ b/crates/fj-core/src/objects/mod.rs @@ -42,7 +42,6 @@ mod handles; mod kinds; mod object; -mod set; mod stores; pub use self::{ @@ -60,6 +59,5 @@ pub use self::{ vertex::Vertex, }, object::{Bare, BehindHandle, Form, Object, WithHandle}, - set::ObjectSet, stores::{Objects, Surfaces}, }; diff --git a/crates/fj-core/src/objects/set.rs b/crates/fj-core/src/objects/set.rs deleted file mode 100644 index 72117a153..000000000 --- a/crates/fj-core/src/objects/set.rs +++ /dev/null @@ -1,108 +0,0 @@ -use std::collections::{btree_set, BTreeSet}; - -use super::{ - BehindHandle, Curve, Cycle, Face, HalfEdge, Object, Surface, Vertex, -}; - -/// A graph of objects and their relationships -pub struct ObjectSet { - inner: BTreeSet>, -} - -impl From<&Face> for ObjectSet { - fn from(face: &Face) -> Self { - let mut self_ = Self { - inner: BTreeSet::new(), - }; - - face.insert_into_set(&mut self_); - - self_ - } -} - -impl From for ObjectSet { - fn from(face: Face) -> Self { - Self::from(&face) - } -} - -impl From for ObjectSet -where - Faces: IntoIterator, -{ - fn from(faces: Faces) -> Self { - let mut self_ = Self { - inner: BTreeSet::new(), - }; - - for face in faces { - face.insert_into_set(&mut self_); - } - - self_ - } -} - -impl IntoIterator for ObjectSet { - type Item = Object; - type IntoIter = btree_set::IntoIter; - - fn into_iter(self) -> Self::IntoIter { - self.inner.into_iter() - } -} - -trait InsertIntoSet { - fn insert_into_set(&self, objects: &mut ObjectSet); -} - -impl InsertIntoSet for Curve { - fn insert_into_set(&self, _: &mut ObjectSet) {} -} - -impl InsertIntoSet for Cycle { - fn insert_into_set(&self, objects: &mut ObjectSet) { - for edge in self.half_edges() { - objects.inner.insert(edge.clone().into()); - edge.insert_into_set(objects); - } - } -} - -impl InsertIntoSet for Face { - fn insert_into_set(&self, objects: &mut ObjectSet) { - objects.inner.insert(self.surface().clone().into()); - self.surface().insert_into_set(objects); - - objects - .inner - .insert(self.region().exterior().clone().into()); - self.region().exterior().insert_into_set(objects); - - for interior in self.region().interiors() { - objects.inner.insert(interior.clone().into()); - } - for interior in self.region().interiors() { - interior.insert_into_set(objects); - } - } -} - -impl InsertIntoSet for HalfEdge { - fn insert_into_set(&self, objects: &mut ObjectSet) { - objects.inner.insert(self.curve().clone().into()); - self.curve().insert_into_set(objects); - - objects.inner.insert(self.start_vertex().clone().into()); - self.start_vertex().insert_into_set(objects); - } -} - -impl InsertIntoSet for Surface { - fn insert_into_set(&self, _: &mut ObjectSet) {} -} - -impl InsertIntoSet for Vertex { - fn insert_into_set(&self, _: &mut ObjectSet) {} -} diff --git a/crates/fj-core/src/operations/insert/insert_trait.rs b/crates/fj-core/src/operations/insert/insert_trait.rs index 9a26b27f8..c83edccf6 100644 --- a/crates/fj-core/src/operations/insert/insert_trait.rs +++ b/crates/fj-core/src/operations/insert/insert_trait.rs @@ -22,6 +22,11 @@ pub trait Insert: Sized { type Inserted; /// Insert the object into its respective store + /// + /// Inserted objects will automatically be validated in the background. You + /// should not insert an invalid object into the stores, unless you have a + /// specific reason to do so, and you are handling validation errors in a + /// non-standard way. #[must_use] fn insert(self, services: &mut Services) -> Self::Inserted; } diff --git a/crates/fj-core/src/services/mod.rs b/crates/fj-core/src/services/mod.rs index ee3c48539..b2820a436 100644 --- a/crates/fj-core/src/services/mod.rs +++ b/crates/fj-core/src/services/mod.rs @@ -7,7 +7,7 @@ mod service; mod validation; use crate::{ - objects::{Object, ObjectSet, Objects, WithHandle}, + objects::{Object, Objects, WithHandle}, validate::ValidationErrors, }; @@ -56,15 +56,6 @@ impl Services { } } - /// Validate the provided objects and forget all other validation errors - pub fn only_validate(&mut self, objects: impl Into) { - let objects = objects.into(); - - let mut events = Vec::new(); - self.validation - .execute(ValidationCommand::OnlyValidate { objects }, &mut events); - } - /// Drop `Services`; return any unhandled validation error pub fn drop_and_validate(self) -> Result<(), ValidationErrors> { let errors = ValidationErrors( diff --git a/crates/fj-core/src/services/validation.rs b/crates/fj-core/src/services/validation.rs index a127cb6b8..cbfea30f6 100644 --- a/crates/fj-core/src/services/validation.rs +++ b/crates/fj-core/src/services/validation.rs @@ -1,7 +1,7 @@ use std::{collections::BTreeMap, error::Error, thread}; use crate::{ - objects::{BehindHandle, Object, ObjectSet}, + objects::{BehindHandle, Object}, storage::ObjectId, validate::ValidationError, }; @@ -31,9 +31,11 @@ impl Drop for Validation { // https://doc.rust-lang.org/std/error/struct.Report.html let mut source = err.source(); while let Some(err) = source { - println!("Caused by:\n\t{err}"); + println!("\nCaused by:\n\t{err}"); source = err.source(); } + + print!("\n\n"); } if !thread::panicking() { @@ -61,20 +63,6 @@ impl State for Validation { }); } } - ValidationCommand::OnlyValidate { objects } => { - events.push(ValidationEvent::ClearErrors); - - for object in objects { - object.validate(&mut errors); - - for err in errors.drain(..) { - events.push(ValidationEvent::ValidationFailed { - object: object.clone(), - err, - }); - } - } - } } } @@ -83,7 +71,6 @@ impl State for Validation { ValidationEvent::ValidationFailed { object, err } => { self.errors.insert(object.id(), err.clone()); } - ValidationEvent::ClearErrors => self.errors.clear(), } } } @@ -95,12 +82,6 @@ pub enum ValidationCommand { /// The object to validate object: Object, }, - - /// Validate the provided objects, discard all other validation errors - OnlyValidate { - /// The objects to validate - objects: ObjectSet, - }, } /// The event produced by the validation service @@ -114,7 +95,4 @@ pub enum ValidationEvent { /// The validation error err: ValidationError, }, - - /// All stored validation errors are being cleared - ClearErrors, } diff --git a/crates/fj-core/src/validate/cycle.rs b/crates/fj-core/src/validate/cycle.rs index 742ec0594..cbe37fdab 100644 --- a/crates/fj-core/src/validate/cycle.rs +++ b/crates/fj-core/src/validate/cycle.rs @@ -10,8 +10,7 @@ impl Validate for Cycle { config: &ValidationConfig, errors: &mut Vec, ) { - CycleValidationError::check_edges_disconnected(self, config, errors); - CycleValidationError::check_enough_edges(self, config, errors); + CycleValidationError::check_half_edge_connections(self, config, errors); } } @@ -20,12 +19,13 @@ impl Validate for Cycle { pub enum CycleValidationError { /// [`Cycle`]'s edges are not connected #[error( - "Adjacent `Edge`s are distinct\n\ - - End position of first `Edge`: {end_of_first:?}\n\ - - Start position of second `Edge`: {start_of_second:?}\n\ - - `Edge`s: {half_edges:#?}" + "Adjacent `HalfEdge`s are not connected\n\ + - End position of first `HalfEdge`: {end_of_first:?}\n\ + - Start position of second `HalfEdge`: {start_of_second:?}\n\ + - Distance between vertices: {distance}\n\ + - `HalfEdge`s: {half_edges:#?}" )] - EdgesDisconnected { + HalfEdgesNotConnected { /// The end position of the first [`HalfEdge`] end_of_first: Point<2>, @@ -38,25 +38,10 @@ pub enum CycleValidationError { /// The edges half_edges: Box<(HalfEdge, HalfEdge)>, }, - - /// [`Cycle`]'s should have at least one [`HalfEdge`] - #[error("Expected at least one `Edge`\n")] - NotEnoughEdges, } impl CycleValidationError { - fn check_enough_edges( - cycle: &Cycle, - _config: &ValidationConfig, - errors: &mut Vec, - ) { - // If there are no half edges - if cycle.half_edges().iter().next().is_none() { - errors.push(Self::NotEnoughEdges.into()); - } - } - - fn check_edges_disconnected( + fn check_half_edge_connections( cycle: &Cycle, config: &ValidationConfig, errors: &mut Vec, @@ -72,7 +57,7 @@ impl CycleValidationError { if distance > config.identical_max_distance { errors.push( - Self::EdgesDisconnected { + Self::HalfEdgesNotConnected { end_of_first, start_of_second, distance, @@ -133,15 +118,10 @@ mod tests { assert_contains_err!( disconnected, ValidationError::Cycle( - CycleValidationError::EdgesDisconnected { .. } + CycleValidationError::HalfEdgesNotConnected { .. } ) ); - let empty = Cycle::new([]); - assert_contains_err!( - empty, - ValidationError::Cycle(CycleValidationError::NotEnoughEdges) - ); Ok(()) } } diff --git a/crates/fj-core/src/validate/face.rs b/crates/fj-core/src/validate/face.rs index ca365135f..e0a3fe7ef 100644 --- a/crates/fj-core/src/validate/face.rs +++ b/crates/fj-core/src/validate/face.rs @@ -10,6 +10,7 @@ impl Validate for Face { _: &ValidationConfig, errors: &mut Vec, ) { + FaceValidationError::check_boundary(self, errors); FaceValidationError::check_interior_winding(self, errors); } } @@ -17,6 +18,10 @@ impl Validate for Face { /// [`Face`] validation error #[derive(Clone, Debug, thiserror::Error)] pub enum FaceValidationError { + /// The [`Face`] has no exterior cycle + #[error("The `Face` has no exterior cycle")] + MissingBoundary, + /// Interior of [`Face`] has invalid winding; must be opposite of exterior #[error( "Interior of `Face` has invalid winding; must be opposite of exterior\n\ @@ -37,6 +42,15 @@ pub enum FaceValidationError { } impl FaceValidationError { + fn check_boundary(face: &Face, errors: &mut Vec) { + if face.region().exterior().half_edges().is_empty() { + errors.push(ValidationError::from(Self::MissingBoundary)); + } + + // Checking *that* a boundary exists is enough. There are validation + // checks for `Cycle` to make sure that the cycle is closed properly. + } + fn check_interior_winding(face: &Face, errors: &mut Vec) { if face.region().exterior().half_edges().is_empty() { // Can't determine winding, if the cycle has no edges. Sounds like a @@ -72,19 +86,49 @@ impl FaceValidationError { mod tests { use crate::{ assert_contains_err, - objects::{Cycle, Face, Region}, + objects::{Cycle, Face, HalfEdge, Region}, operations::{ - build::{BuildCycle, BuildFace}, + build::{BuildCycle, BuildFace, BuildHalfEdge}, insert::Insert, reverse::Reverse, - update::{UpdateFace, UpdateRegion}, + update::{UpdateCycle, UpdateFace, UpdateRegion}, }, services::Services, validate::{FaceValidationError, Validate, ValidationError}, }; #[test] - fn face_invalid_interior_winding() -> anyhow::Result<()> { + fn boundary() -> anyhow::Result<()> { + let mut services = Services::new(); + + let invalid = + Face::unbound(services.objects.surfaces.xy_plane(), &mut services); + let valid = invalid.update_region(|region| { + region + .update_exterior(|cycle| { + cycle + .add_half_edges([HalfEdge::circle( + [0., 0.], + 1., + &mut services, + ) + .insert(&mut services)]) + .insert(&mut services) + }) + .insert(&mut services) + }); + + valid.validate_and_return_first_error()?; + assert_contains_err!( + invalid, + ValidationError::Face(FaceValidationError::MissingBoundary) + ); + + Ok(()) + } + + #[test] + fn interior_winding() -> anyhow::Result<()> { let mut services = Services::new(); let valid = @@ -132,8 +176,6 @@ mod tests { ) ); - services.only_validate(valid); - Ok(()) } } diff --git a/crates/fj-core/src/validate/mod.rs b/crates/fj-core/src/validate/mod.rs index 1bcf151bb..3fac7d5bf 100644 --- a/crates/fj-core/src/validate/mod.rs +++ b/crates/fj-core/src/validate/mod.rs @@ -59,9 +59,12 @@ //! on, and this fact is taken into account by allowing for configuration via //! [`Validate::validate_with_config`] and [`ValidationConfig`]. //! -//! However, it is currently not possible to define this configuration for the -//! background validation done by the [`Services`] API. If this is a problem for -//! you, please comment on this issue: +//! +//! ## Implementation Note +//! +//! It is currently not possible to define this configuration for the background +//! validation done by the [`Services`] API. If this is a problem for you, +//! please comment on this issue: //! //! //!