From 92609d2083c65aed64610d41c3a812f0b97eaa12 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Sun, 19 Mar 2023 12:32:22 +0200 Subject: [PATCH 01/10] Attempt at shell validation Attempt to validate `Shell`'s by making sure that each half-edge in a shell refers to a global-edge that is referred to by another half-edge in that same shell. My logic says that each edge in a shell should be a part of exactly two faces but I may be wrong. This validation check currently fails with `cargo run --package export- validator`, but I'm not sure if this is a bug in my validation checks, or whether I exposed a bug in existing code. For example looking at the cuboid example, many global edges are referred to by only one half_edge, which seems wrong to me... --- crates/fj-kernel/src/validate/mod.rs | 6 +++- crates/fj-kernel/src/validate/shell.rs | 42 ++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 5a0dcfc84..1a4286f70 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -9,7 +9,7 @@ mod solid; mod surface; mod vertex; -use self::cycle::CycleValidationError; +use self::{cycle::CycleValidationError, shell::ShellValidationError}; pub use self::{edge::HalfEdgeValidationError, face::FaceValidationError}; use std::convert::Infallible; @@ -92,6 +92,10 @@ pub enum ValidationError { /// `Cycle` validation error #[error("`Cycle` validation error:\n{0}")] Cycle(#[from] CycleValidationError), + + /// `Shell` validation error + #[error("`Shell` validation error:\n{0}")] + Shell(#[from] ShellValidationError), } impl From for ValidationError { diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index f6603ad55..c3b478937 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -1,12 +1,50 @@ -use crate::objects::Shell; +use std::collections::HashMap; + +use crate::{objects::Shell, storage::ObjectId}; use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Shell { fn validate_with_config( &self, + config: &ValidationConfig, + errors: &mut Vec, + ) { + ShellValidationError::validate_watertight(self, config, errors); + } +} + +/// [`Shell`] validation failed +#[derive(Clone, Debug, thiserror::Error)] +pub enum ShellValidationError { + /// [`Shell`] contains global_edges not referred to by two half_edges + #[error("Shell is not watertight")] + NotWaterTight, +} + +impl ShellValidationError { + fn validate_watertight( + shell: &Shell, _: &ValidationConfig, - _: &mut Vec, + errors: &mut Vec, ) { + let faces = shell.faces(); + let mut half_edge_to_faces: HashMap = HashMap::new(); + for face in faces { + let cycles = + face.interiors().chain(std::iter::once(face.exterior())); + for cycle in cycles { + for half_edge in cycle.half_edges() { + let id = half_edge.global_form().id(); + let entry = half_edge_to_faces.entry(id); + *entry.or_insert(0) += 1; + } + } + } + + // Each global edge should have exactly two half edges that are part of the shell + if half_edge_to_faces.iter().find(|(_, c)| **c != 2).is_some() { + errors.push(Self::NotWaterTight.into()) + } } } From e23a0fc72bf1b0ab3d8032cafb27e2b71af7b2fe Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Sun, 19 Mar 2023 19:54:46 +0200 Subject: [PATCH 02/10] Add checks for coincident HalfEdges Validate whether coincident HalfEdges refer to the same GlobalEdge. This should resolve: #1594. Checks whether they coincide by sampling. --- crates/fj-kernel/src/objects/full/face.rs | 2 +- crates/fj-kernel/src/validate/mod.rs | 8 ++ crates/fj-kernel/src/validate/shell.rs | 101 ++++++++++++++++++++-- 3 files changed, 105 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/objects/full/face.rs b/crates/fj-kernel/src/objects/full/face.rs index 95738ca87..1b22a0962 100644 --- a/crates/fj-kernel/src/objects/full/face.rs +++ b/crates/fj-kernel/src/objects/full/face.rs @@ -75,7 +75,7 @@ impl Face { self.interiors.iter() } - /// Access all cycles of the face + /// Access all cycles of the face (both exterior and interior) pub fn all_cycles(&self) -> impl Iterator> + '_ { [self.exterior()].into_iter().chain(self.interiors()) } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 1a4286f70..aae86e639 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -62,6 +62,11 @@ pub struct ValidationConfig { /// that distance is less than the one defined in this field, can not be /// considered identical. pub identical_max_distance: Scalar, + + /// How often to sample edges when checking if they coincide. This + /// represents the number of points we check on each Edge. + /// The higher this is the more precise our validation is, and the slower it is. + pub sample_count: usize, } impl Default for ValidationConfig { @@ -74,6 +79,9 @@ impl Default for ValidationConfig { // false positives due to floating-point accuracy issues), we can // adjust it. identical_max_distance: Scalar::from_f64(5e-14), + + // This value is completely arbitrary, but seems good enough for now. + sample_count: 16, } } } diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index c3b478937..3dc2e1a94 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -1,6 +1,12 @@ -use std::collections::HashMap; +use std::{collections::HashMap, iter::repeat}; -use crate::{objects::Shell, storage::ObjectId}; +use fj_math::Point; + +use crate::{ + geometry::surface::SurfaceGeometry, + objects::{HalfEdge, Shell}, + storage::{Handle, ObjectId}, +}; use super::{Validate, ValidationConfig, ValidationError}; @@ -11,6 +17,7 @@ impl Validate for Shell { errors: &mut Vec, ) { ShellValidationError::validate_watertight(self, config, errors); + ShellValidationError::validate_coincident(self, config, errors); } } @@ -20,9 +27,95 @@ pub enum ShellValidationError { /// [`Shell`] contains global_edges not referred to by two half_edges #[error("Shell is not watertight")] NotWaterTight, + /// [`Shell`] contains half_edges that are coincident, but refer to different global_edges + #[error( + "Shell contains HalfEdges which are coinciendent but refer to different GlobalEdges\n + Edge 1: {0:#?} + Edge 2: {1:#?} + " + )] + CoincidentEdgesNotIdentical(HalfEdge, HalfEdge), +} + +/// Check whether to [`HalfEdge`]s are coincident. Along with the edges you +/// provide the surface that they are on, so that we can check their positions +/// in 3D space. +/// We check whether they are coincident by comparing a configurable amount +/// of points, and seeing whether they are further than the maximum distance for +/// identical objects, see [`ValidationConfig`] +fn are_coincident( + config: &ValidationConfig, + (edge1, surface1): (Handle, SurfaceGeometry), + (edge2, surface2): (Handle, SurfaceGeometry), +) -> bool { + fn sample( + percent: f64, + (edge, surface): (&Handle, SurfaceGeometry), + ) -> Point<3> { + let boundary = edge.boundary(); + let path_coords = boundary[0] + (boundary[1] - boundary[0]) * percent; + let surface_coords = edge.curve().point_from_path_coords(path_coords); + surface.point_from_surface_coords(surface_coords) + } + + // Check whether start positions do not match. If they don't treat second edge as flipped + let flip = sample(0.0, (&edge1, surface1)) + .distance_to(&sample(0.0, (&edge2, surface2))) + > config.identical_max_distance; + + for i in 0..config.sample_count { + let percent = i as f64 * (1.0 / config.sample_count as f64); + let sample1 = sample(percent, (&edge1, surface1)); + let sample2 = sample( + if flip { 1.0 - percent } else { percent }, + (&edge2, surface2), + ); + if sample1.distance_to(&sample2) > config.identical_max_distance { + // If corresponding points are further than the max distance than these HalfEdges are not coincident + return false; + } + } + + true } impl ShellValidationError { + fn validate_coincident( + shell: &Shell, + config: &ValidationConfig, + errors: &mut Vec, + ) { + let faces: Vec<(Handle, SurfaceGeometry)> = shell + .faces() + .into_iter() + .flat_map(|face| { + face.all_cycles() + .flat_map(|cycle| cycle.half_edges().cloned()) + .zip(repeat(face.surface().geometry())) + }) + .collect(); + + // This is O(N) which isn't great, but we can't use a HashMap since we need to deal with float inaccuracies. + #[allow(unreachable_code)] + for edge in &faces { + for other_edge in &faces { + let identical = edge.0.global_form().id() + == other_edge.0.global_form().id(); + if are_coincident(config, edge.clone(), other_edge.clone()) + && !identical + { + errors.push( + Self::CoincidentEdgesNotIdentical( + edge.0.clone_object(), + other_edge.0.clone_object(), + ) + .into(), + ) + } + } + } + } + fn validate_watertight( shell: &Shell, _: &ValidationConfig, @@ -31,9 +124,7 @@ impl ShellValidationError { let faces = shell.faces(); let mut half_edge_to_faces: HashMap = HashMap::new(); for face in faces { - let cycles = - face.interiors().chain(std::iter::once(face.exterior())); - for cycle in cycles { + for cycle in face.all_cycles() { for half_edge in cycle.half_edges() { let id = half_edge.global_form().id(); let entry = half_edge_to_faces.entry(id); From a7455ca93861338ad665ca4f5596e669d61d26b1 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Mon, 20 Mar 2023 07:50:49 +0200 Subject: [PATCH 03/10] Add basic tests for watertight and coincident Currently there are no tests for valid cases --- crates/fj-kernel/src/validate/shell.rs | 104 ++++++++++++++++++++++++- 1 file changed, 101 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 3dc2e1a94..43335783f 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -16,8 +16,8 @@ impl Validate for Shell { config: &ValidationConfig, errors: &mut Vec, ) { - ShellValidationError::validate_watertight(self, config, errors); ShellValidationError::validate_coincident(self, config, errors); + ShellValidationError::validate_watertight(self, config, errors); } } @@ -26,7 +26,7 @@ impl Validate for Shell { pub enum ShellValidationError { /// [`Shell`] contains global_edges not referred to by two half_edges #[error("Shell is not watertight")] - NotWaterTight, + NotWatertight, /// [`Shell`] contains half_edges that are coincident, but refer to different global_edges #[error( "Shell contains HalfEdges which are coinciendent but refer to different GlobalEdges\n @@ -135,7 +135,105 @@ impl ShellValidationError { // Each global edge should have exactly two half edges that are part of the shell if half_edge_to_faces.iter().find(|(_, c)| **c != 2).is_some() { - errors.push(Self::NotWaterTight.into()) + errors.push(Self::NotWatertight.into()) } } } + +#[cfg(test)] +mod tests { + use crate::insert::Insert; + + use crate::validate::shell::ShellValidationError; + use crate::{ + builder::{CycleBuilder, FaceBuilder}, + objects::Shell, + services::Services, + validate::{Validate, ValidationError}, + }; + + #[test] + fn coincident_not_identical() -> anyhow::Result<()> { + let mut services = Services::new(); + let invalid = { + // Shell with single face is not watertight + let face1 = FaceBuilder::new(services.objects.surfaces.xy_plane()) + .with_exterior(CycleBuilder::polygon([ + [0., 0.], + [0., 1.], + [1., 1.], + [1., 0.], + ])) + .build(&mut services.objects) + .insert(&mut services.objects); + + let face2 = FaceBuilder::new(services.objects.surfaces.xz_plane()) + .with_exterior(CycleBuilder::polygon([ + [0., 0.], + [0., 1.], + [1., 1.], + [1., 0.], + ])) + .build(&mut services.objects) + .insert(&mut services.objects); + + Shell::new([face1, face2]) + }; + + assert!({ + let mut errors = Vec::new(); + invalid.validate(&mut errors); + errors + .iter() + .find(|e| { + matches!( + e, + ValidationError::Shell( + ShellValidationError::CoincidentEdgesNotIdentical( + .. + ) + ) + ) + }) + .is_some() + }); + + Ok(()) + } + #[test] + fn shell_not_watertight() -> anyhow::Result<()> { + let mut services = Services::new(); + + let invalid = { + // Shell with single face is not watertight + let face = FaceBuilder::new(services.objects.surfaces.xy_plane()) + .with_exterior(CycleBuilder::polygon([ + [0., 0.], + [0., 1.], + [1., 1.], + [1., 0.], + ])) + .build(&mut services.objects) + .insert(&mut services.objects); + Shell::new([face]) + }; + + assert!({ + let mut errors = Vec::new(); + invalid.validate(&mut errors); + errors + .iter() + .find(|e| { + matches!( + e, + ValidationError::Shell( + ShellValidationError::NotWatertight + ) + ) + }) + .is_some() + }); + + Ok(()) + } +} From ccee909918044f8f66eb868eb426fa04dfd651bb Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Mon, 20 Mar 2023 08:05:03 +0200 Subject: [PATCH 04/10] Add `assert_contains_err!` macro This allows us to easily check whether some object raises some validation error, without requiring it to be the first. Switched to this macro whenever there was an assertion that the **first** validation error matches some pattern. --- crates/fj-kernel/src/validate/cycle.rs | 23 +++++++------- crates/fj-kernel/src/validate/edge.rs | 11 ++++--- crates/fj-kernel/src/validate/face.rs | 11 ++++--- crates/fj-kernel/src/validate/mod.rs | 13 ++++++++ crates/fj-kernel/src/validate/shell.rs | 43 +++++++------------------- 5 files changed, 47 insertions(+), 54 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index bc2a96f31..5821519a1 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -94,6 +94,7 @@ impl CycleValidationError { mod tests { use crate::{ + assert_contains_err, builder::{CycleBuilder, HalfEdgeBuilder}, objects::Cycle, services::Services, @@ -120,21 +121,19 @@ mod tests { .add_half_edge(second) .build(&mut services.objects) }; - assert!(matches!( - disconnected.validate_and_return_first_error(), - Err(ValidationError::Cycle( + + assert_contains_err!( + disconnected, + ValidationError::Cycle( CycleValidationError::HalfEdgesDisconnected { .. } - )) - )); + ) + ); let empty = Cycle::new([]); - assert!(matches!( - empty.validate_and_return_first_error(), - Err(ValidationError::Cycle( - CycleValidationError::NotEnoughHalfEdges - )) - )); - + assert_contains_err!( + empty, + ValidationError::Cycle(CycleValidationError::NotEnoughHalfEdges) + ); Ok(()) } } diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 0f41f6629..e5399a7ce 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -76,6 +76,7 @@ mod tests { use fj_math::Point; use crate::{ + assert_contains_err, builder::HalfEdgeBuilder, objects::HalfEdge, services::Services, @@ -100,12 +101,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(matches!( - invalid.validate_and_return_first_error(), - Err(ValidationError::HalfEdge( + assert_contains_err!( + invalid, + ValidationError::HalfEdge( HalfEdgeValidationError::VerticesAreCoincident { .. } - )) - )); + ) + ); Ok(()) } diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 556725030..bf4857d27 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -72,6 +72,7 @@ impl FaceValidationError { mod tests { use crate::{ algorithms::reverse::Reverse, + assert_contains_err, builder::{CycleBuilder, FaceBuilder}, objects::Face, services::Services, @@ -110,12 +111,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(matches!( - invalid.validate_and_return_first_error(), - Err(ValidationError::Face( + assert_contains_err!( + invalid, + ValidationError::Face( FaceValidationError::InvalidInteriorWinding { .. } - )) - )); + ) + ); Ok(()) } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index aae86e639..2837eddc8 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -16,6 +16,19 @@ use std::convert::Infallible; use fj_math::Scalar; +/// Assert that some object has a validation error which matches a specifc pattern. +/// This is preferred to matching on [`Validate::validate_and_return_first_error`], since usually we don't care about the order. +#[macro_export] +macro_rules! assert_contains_err { + ($o:tt,$p:pat) => { + assert!({ + let mut errors = Vec::new(); + $o.validate(&mut errors); + errors.iter().find(|e| matches!(e, $p)).is_some() + }) + }; +} + /// Validate an object /// /// This trait is used automatically when inserting an object into a store. diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 43335783f..199634813 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -142,6 +142,7 @@ impl ShellValidationError { #[cfg(test)] mod tests { + use crate::assert_contains_err; use crate::insert::Insert; use crate::validate::shell::ShellValidationError; @@ -180,23 +181,12 @@ mod tests { Shell::new([face1, face2]) }; - assert!({ - let mut errors = Vec::new(); - invalid.validate(&mut errors); - errors - .iter() - .find(|e| { - matches!( - e, - ValidationError::Shell( - ShellValidationError::CoincidentEdgesNotIdentical( - .. - ) - ) - ) - }) - .is_some() - }); + assert_contains_err!( + invalid, + ValidationError::Shell( + ShellValidationError::CoincidentEdgesNotIdentical(..) + ) + ); Ok(()) } @@ -218,21 +208,10 @@ mod tests { Shell::new([face]) }; - assert!({ - let mut errors = Vec::new(); - invalid.validate(&mut errors); - errors - .iter() - .find(|e| { - matches!( - e, - ValidationError::Shell( - ShellValidationError::NotWatertight - ) - ) - }) - .is_some() - }); + assert_contains_err!( + invalid, + ValidationError::Shell(ShellValidationError::NotWatertight) + ); Ok(()) } From ec669d23b0b4462d4722f5f8ca1e4c9c2a6c8e87 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Mon, 20 Mar 2023 09:09:06 +0200 Subject: [PATCH 05/10] Cleanup and make clippy happy --- crates/fj-kernel/src/validate/mod.rs | 2 +- crates/fj-kernel/src/validate/shell.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 2837eddc8..d7a8c19c8 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -24,7 +24,7 @@ macro_rules! assert_contains_err { assert!({ let mut errors = Vec::new(); $o.validate(&mut errors); - errors.iter().find(|e| matches!(e, $p)).is_some() + errors.iter().any(|e| matches!(e, $p)) }) }; } diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 199634813..7d43ba34b 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -34,7 +34,7 @@ pub enum ShellValidationError { Edge 2: {1:#?} " )] - CoincidentEdgesNotIdentical(HalfEdge, HalfEdge), + CoincidentEdgesNotIdentical(Handle, Handle), } /// Check whether to [`HalfEdge`]s are coincident. Along with the edges you @@ -106,8 +106,8 @@ impl ShellValidationError { { errors.push( Self::CoincidentEdgesNotIdentical( - edge.0.clone_object(), - other_edge.0.clone_object(), + edge.0.clone(), + other_edge.0.clone(), ) .into(), ) @@ -134,7 +134,7 @@ impl ShellValidationError { } // Each global edge should have exactly two half edges that are part of the shell - if half_edge_to_faces.iter().find(|(_, c)| **c != 2).is_some() { + if half_edge_to_faces.iter().any(|(_, c)| *c != 2) { errors.push(Self::NotWatertight.into()) } } From 44f606fd4884eb82038fb7ffec656320825ffc9e Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Mon, 20 Mar 2023 11:52:26 +0200 Subject: [PATCH 06/10] Only sample 3 points Simplify code to only sample 3 points, since that is enough for cicrcles and lines. --- crates/fj-kernel/src/validate/mod.rs | 8 -------- crates/fj-kernel/src/validate/shell.rs | 8 ++++++-- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index d7a8c19c8..30f6b97a9 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -75,11 +75,6 @@ pub struct ValidationConfig { /// that distance is less than the one defined in this field, can not be /// considered identical. pub identical_max_distance: Scalar, - - /// How often to sample edges when checking if they coincide. This - /// represents the number of points we check on each Edge. - /// The higher this is the more precise our validation is, and the slower it is. - pub sample_count: usize, } impl Default for ValidationConfig { @@ -92,9 +87,6 @@ impl Default for ValidationConfig { // false positives due to floating-point accuracy issues), we can // adjust it. identical_max_distance: Scalar::from_f64(5e-14), - - // This value is completely arbitrary, but seems good enough for now. - sample_count: 16, } } } diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 7d43ba34b..9a6e0ebb5 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -63,8 +63,12 @@ fn are_coincident( .distance_to(&sample(0.0, (&edge2, surface2))) > config.identical_max_distance; - for i in 0..config.sample_count { - let percent = i as f64 * (1.0 / config.sample_count as f64); + // Three samples (start, middle, end), are enough to detect weather lines + // and circles match. If we were to add more complicated curves, this might + // need to change. + let sample_count = 3; + for i in 0..sample_count { + let percent = i as f64 * (1.0 / sample_count as f64); let sample1 = sample(percent, (&edge1, surface1)); let sample2 = sample( if flip { 1.0 - percent } else { percent }, From ae2a3ab5a5f0cb8d9d4562721393b065373944ab Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Mon, 20 Mar 2023 16:24:10 +0200 Subject: [PATCH 07/10] Fix Sweep edge duplication Fixed cycle builder like you commented, global_edge order, and added an edge cache. PS: We might want to make the `SweepCache` more general --- crates/fj-kernel/src/algorithms/sweep/edge.rs | 2 +- crates/fj-kernel/src/algorithms/sweep/mod.rs | 4 +++- crates/fj-kernel/src/algorithms/sweep/vertex.rs | 6 +++++- crates/fj-kernel/src/builder/cycle.rs | 3 ++- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index dfe033f8d..e52bd5db5 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -37,8 +37,8 @@ impl Sweep for (&HalfEdge, &Handle, &Surface, Option) { [ Some(edge.global_form().clone()), Some(edge_up), - Some(edge_down), None, + Some(edge_down), ], ) }; diff --git a/crates/fj-kernel/src/algorithms/sweep/mod.rs b/crates/fj-kernel/src/algorithms/sweep/mod.rs index e2e98704a..6cdc14430 100644 --- a/crates/fj-kernel/src/algorithms/sweep/mod.rs +++ b/crates/fj-kernel/src/algorithms/sweep/mod.rs @@ -11,7 +11,7 @@ use std::collections::BTreeMap; use fj_math::Vector; use crate::{ - objects::{Objects, Vertex}, + objects::{GlobalEdge, Objects, Vertex}, services::Service, storage::{Handle, ObjectId}, }; @@ -47,4 +47,6 @@ pub trait Sweep: Sized { pub struct SweepCache { /// Cache for global vertices pub global_vertex: BTreeMap>, + /// Cache for global edges + pub global_edge: BTreeMap>, } diff --git a/crates/fj-kernel/src/algorithms/sweep/vertex.rs b/crates/fj-kernel/src/algorithms/sweep/vertex.rs index 868f8e9ef..d2f86193c 100644 --- a/crates/fj-kernel/src/algorithms/sweep/vertex.rs +++ b/crates/fj-kernel/src/algorithms/sweep/vertex.rs @@ -26,7 +26,11 @@ impl Sweep for Handle { .clone(); let vertices = [a, b]; - let global_edge = GlobalEdge::new().insert(objects); + let global_edge = cache + .global_edge + .entry(self.id()) + .or_insert_with(|| GlobalEdge::new().insert(objects)) + .clone(); // The vertices of the returned `GlobalEdge` are in normalized order, // which means the order can't be relied upon by the caller. Return the diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 2f5a8cbb1..92fc63cd8 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -44,9 +44,10 @@ impl CycleBuilder { let half_edges = edges .into_iter() .circular_tuple_windows() - .map(|((prev, _, _), (_, curve, boundary))| { + .map(|((prev, _, _), (half_edge, curve, boundary))| { HalfEdgeBuilder::new(curve, boundary) .with_start_vertex(prev.start_vertex().clone()) + .with_global_form(half_edge.global_form().clone()) }) .collect(); From edcfffbc32b84bdd5fa9e3eb45541780869a1d70 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Tue, 21 Mar 2023 07:51:33 +0200 Subject: [PATCH 08/10] Fixup Fix many inaccuracies and cleanup code. --- crates/fj-kernel/src/validate/shell.rs | 160 ++++++++++++++++++++----- 1 file changed, 128 insertions(+), 32 deletions(-) diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 9a6e0ebb5..f6b21813d 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -1,10 +1,10 @@ use std::{collections::HashMap, iter::repeat}; -use fj_math::Point; +use fj_math::{Point, Scalar}; use crate::{ geometry::surface::SurfaceGeometry, - objects::{HalfEdge, Shell}, + objects::{HalfEdge, Shell, Vertex}, storage::{Handle, ObjectId}, }; @@ -29,25 +29,48 @@ pub enum ShellValidationError { NotWatertight, /// [`Shell`] contains half_edges that are coincident, but refer to different global_edges #[error( - "Shell contains HalfEdges which are coinciendent but refer to different GlobalEdges\n + "Shell contains HalfEdges that are coinciendent but refer to different GlobalEdges\n Edge 1: {0:#?} Edge 2: {1:#?} " )] CoincidentEdgesNotIdentical(Handle, Handle), + /// [`Shell`] contains half_edges that are identical, but do not coincide + #[error( + "Shell contains HalfEdges that are identical but do not coincide\n + Edge 1: {0:#?} + Edge 2: {1:#?} + " + )] + IdenticalEdgesNotCoincident(Handle, Handle), + + /// [`Shell`] contains vertices that are coincident, but not identical + #[error( + "Shell contains Vertices that are coinciendent but not identical\n + Vertex 1: {:#?} {:#?} + Vertex 2: {:#?} {:#?} + ", .0[0].0, .0[0].1,.0[1].0,.0[1].1 + )] + DistinctVertsCoincide([(Handle, Point<3>); 2]), + + /// [`Shell`] contains vertices that are identical, but do not coincide + #[error( + "Shell contains Vertices that are identical but do not coincide\n + Vertex 1: {:#?} {:#?} + Vertex 2: {:#?} {:#?} + ", .0[0].0, .0[0].1,.0[1].0,.0[1].1 + )] + IdenticalVertsNotCoincident([(Handle, Point<3>); 2]), } -/// Check whether to [`HalfEdge`]s are coincident. Along with the edges you -/// provide the surface that they are on, so that we can check their positions -/// in 3D space. -/// We check whether they are coincident by comparing a configurable amount -/// of points, and seeing whether they are further than the maximum distance for -/// identical objects, see [`ValidationConfig`] -fn are_coincident( +/// Sample two edges at various (currently 3) points in 3D along them. +/// +/// Returns an [`Iterator`] of the distance at each sample. +fn distances( config: &ValidationConfig, (edge1, surface1): (Handle, SurfaceGeometry), (edge2, surface2): (Handle, SurfaceGeometry), -) -> bool { +) -> impl Iterator { fn sample( percent: f64, (edge, surface): (&Handle, SurfaceGeometry), @@ -67,20 +90,19 @@ fn are_coincident( // and circles match. If we were to add more complicated curves, this might // need to change. let sample_count = 3; + let step = 1.0 / (sample_count as f64 - 1.0); + + let mut distances = Vec::new(); for i in 0..sample_count { - let percent = i as f64 * (1.0 / sample_count as f64); + let percent = i as f64 * step; let sample1 = sample(percent, (&edge1, surface1)); let sample2 = sample( if flip { 1.0 - percent } else { percent }, (&edge2, surface2), ); - if sample1.distance_to(&sample2) > config.identical_max_distance { - // If corresponding points are further than the max distance than these HalfEdges are not coincident - return false; - } + distances.push(sample1.distance_to(&sample2)) } - - true + distances.into_iter() } impl ShellValidationError { @@ -99,22 +121,96 @@ impl ShellValidationError { }) .collect(); - // This is O(N) which isn't great, but we can't use a HashMap since we need to deal with float inaccuracies. - #[allow(unreachable_code)] + // This is O(N^2) which isn't great, but we can't use a HashMap since we + // need to deal with float inaccuracies. Maybe we could use some smarter + // data-structure like an octree. for edge in &faces { for other_edge in &faces { - let identical = edge.0.global_form().id() - == other_edge.0.global_form().id(); - if are_coincident(config, edge.clone(), other_edge.clone()) - && !identical - { - errors.push( - Self::CoincidentEdgesNotIdentical( - edge.0.clone(), - other_edge.0.clone(), - ) - .into(), - ) + let id = edge.0.global_form().id(); + let other_id = other_edge.0.global_form().id(); + let identical = id == other_id; + match identical { + true => { + // All points on identical curves should be within + // identical_max_distance, so we shouldn't have any + // greater than the max + if distances(config, edge.clone(), other_edge.clone()) + .any(|d| d > config.identical_max_distance) + { + errors.push( + Self::IdenticalEdgesNotCoincident( + edge.0.clone(), + other_edge.0.clone(), + ) + .into(), + ) + } + } + false => { + // If all points on distinct curves are within + // distinct_min_distance, that's a problem. + if distances(config, edge.clone(), other_edge.clone()) + .all(|d| d < config.distinct_min_distance) + { + errors.push( + Self::CoincidentEdgesNotIdentical( + edge.0.clone(), + other_edge.0.clone(), + ) + .into(), + ) + } + } + } + } + } + + let vertices: Vec<(Point<3>, Handle)> = shell + .faces() + .into_iter() + .flat_map(|face| { + face.all_cycles() + .flat_map(|cycle| cycle.half_edges().cloned()) + .zip(repeat(face.surface().geometry())) + }) + .map(|(h, s)| { + ( + s.point_from_surface_coords(h.start_position()), + h.start_vertex().clone(), + ) + }) + .collect(); + + // This is O(N^2) which isn't great, but we can't use a HashMap since we + // need to deal with float inaccuracies. Maybe we could use some smarter + // data-structure like an octree. + for a in &vertices { + for b in &vertices { + match a.1.id() == b.1.id() { + true => { + if a.0.distance_to(&b.0) > config.identical_max_distance + { + errors.push( + Self::IdenticalVertsNotCoincident([ + (a.1.clone(), a.0), + (b.1.clone(), b.0), + ]) + .into(), + ) + } + } + false => { + if a.0.distance_to(&b.0) < config.distinct_min_distance + { + errors.push( + Self::DistinctVertsCoincide([ + (a.1.clone(), a.0), + (b.1.clone(), b.0), + ]) + .into(), + ) + } + } } } } From c88ac6c74e9f89ea0bb36ca60e8459636953250b Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Tue, 21 Mar 2023 08:01:44 +0200 Subject: [PATCH 09/10] Move vertex coincidence validation to solid Before it was in shell. This partially addresses #1689, holding off on validating sketches until we figure out sketch-face relationship: #1691 --- crates/fj-kernel/src/validate/cycle.rs | 1 + crates/fj-kernel/src/validate/mod.rs | 11 ++- crates/fj-kernel/src/validate/shell.rs | 74 +------------------- crates/fj-kernel/src/validate/solid.rs | 93 +++++++++++++++++++++++++- 4 files changed, 103 insertions(+), 76 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 5821519a1..7b638f438 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -42,6 +42,7 @@ pub enum CycleValidationError { /// The half-edge half_edges: Box<(HalfEdge, HalfEdge)>, }, + /// [`Cycle`]'s should have at least one `HalfEdge` #[error("Expected at least one `HalfEdge`\n")] NotEnoughHalfEdges, } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 30f6b97a9..7d155d3d3 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -9,8 +9,11 @@ mod solid; mod surface; mod vertex; -use self::{cycle::CycleValidationError, shell::ShellValidationError}; -pub use self::{edge::HalfEdgeValidationError, face::FaceValidationError}; +pub use self::{ + cycle::CycleValidationError, edge::HalfEdgeValidationError, + face::FaceValidationError, shell::ShellValidationError, + solid::SolidValidationError, +}; use std::convert::Infallible; @@ -109,6 +112,10 @@ pub enum ValidationError { /// `Shell` validation error #[error("`Shell` validation error:\n{0}")] Shell(#[from] ShellValidationError), + + /// `Solid` validation error + #[error("`Solid` validation error:\n{0}")] + Solid(#[from] SolidValidationError), } impl From for ValidationError { diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index f6b21813d..3f1bc690a 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -4,7 +4,7 @@ use fj_math::{Point, Scalar}; use crate::{ geometry::surface::SurfaceGeometry, - objects::{HalfEdge, Shell, Vertex}, + objects::{HalfEdge, Shell}, storage::{Handle, ObjectId}, }; @@ -16,7 +16,7 @@ impl Validate for Shell { config: &ValidationConfig, errors: &mut Vec, ) { - ShellValidationError::validate_coincident(self, config, errors); + ShellValidationError::validate_edges_coincident(self, config, errors); ShellValidationError::validate_watertight(self, config, errors); } } @@ -43,24 +43,6 @@ pub enum ShellValidationError { " )] IdenticalEdgesNotCoincident(Handle, Handle), - - /// [`Shell`] contains vertices that are coincident, but not identical - #[error( - "Shell contains Vertices that are coinciendent but not identical\n - Vertex 1: {:#?} {:#?} - Vertex 2: {:#?} {:#?} - ", .0[0].0, .0[0].1,.0[1].0,.0[1].1 - )] - DistinctVertsCoincide([(Handle, Point<3>); 2]), - - /// [`Shell`] contains vertices that are identical, but do not coincide - #[error( - "Shell contains Vertices that are identical but do not coincide\n - Vertex 1: {:#?} {:#?} - Vertex 2: {:#?} {:#?} - ", .0[0].0, .0[0].1,.0[1].0,.0[1].1 - )] - IdenticalVertsNotCoincident([(Handle, Point<3>); 2]), } /// Sample two edges at various (currently 3) points in 3D along them. @@ -106,7 +88,7 @@ fn distances( } impl ShellValidationError { - fn validate_coincident( + fn validate_edges_coincident( shell: &Shell, config: &ValidationConfig, errors: &mut Vec, @@ -164,56 +146,6 @@ impl ShellValidationError { } } } - - let vertices: Vec<(Point<3>, Handle)> = shell - .faces() - .into_iter() - .flat_map(|face| { - face.all_cycles() - .flat_map(|cycle| cycle.half_edges().cloned()) - .zip(repeat(face.surface().geometry())) - }) - .map(|(h, s)| { - ( - s.point_from_surface_coords(h.start_position()), - h.start_vertex().clone(), - ) - }) - .collect(); - - // This is O(N^2) which isn't great, but we can't use a HashMap since we - // need to deal with float inaccuracies. Maybe we could use some smarter - // data-structure like an octree. - for a in &vertices { - for b in &vertices { - match a.1.id() == b.1.id() { - true => { - if a.0.distance_to(&b.0) > config.identical_max_distance - { - errors.push( - Self::IdenticalVertsNotCoincident([ - (a.1.clone(), a.0), - (b.1.clone(), b.0), - ]) - .into(), - ) - } - } - false => { - if a.0.distance_to(&b.0) < config.distinct_min_distance - { - errors.push( - Self::DistinctVertsCoincide([ - (a.1.clone(), a.0), - (b.1.clone(), b.0), - ]) - .into(), - ) - } - } - } - } - } } fn validate_watertight( diff --git a/crates/fj-kernel/src/validate/solid.rs b/crates/fj-kernel/src/validate/solid.rs index e2982bd6a..b5fe04cc2 100644 --- a/crates/fj-kernel/src/validate/solid.rs +++ b/crates/fj-kernel/src/validate/solid.rs @@ -1,12 +1,99 @@ -use crate::objects::Solid; +use std::iter::repeat; + +use crate::{ + objects::{Solid, Vertex}, + storage::Handle, +}; +use fj_math::Point; use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Solid { fn validate_with_config( &self, - _: &ValidationConfig, - _: &mut Vec, + config: &ValidationConfig, + errors: &mut Vec, ) { + SolidValidationError::check_vertices(self, config, errors) + } +} + +/// [`Solid`] validation failed +#[derive(Clone, Debug, thiserror::Error)] +pub enum SolidValidationError { + /// [`Solid`] contains vertices that are coincident, but not identical + #[error( + "Solid contains Vertices that are coinciendent but not identical\n + Vertex 1: {:#?} {:#?} + Vertex 2: {:#?} {:#?} + ", .0[0].0, .0[0].1,.0[1].0,.0[1].1 + )] + DistinctVertsCoincide([(Handle, Point<3>); 2]), + + /// [`Solid`] contains vertices that are identical, but do not coincide + #[error( + "Solid contains Vertices that are identical but do not coincide\n + Vertex 1: {:#?} {:#?} + Vertex 2: {:#?} {:#?} + ", .0[0].0, .0[0].1,.0[1].0,.0[1].1 + )] + IdenticalVertsNotCoincident([(Handle, Point<3>); 2]), +} + +impl SolidValidationError { + fn check_vertices( + solid: &Solid, + config: &ValidationConfig, + errors: &mut Vec, + ) { + let vertices: Vec<(Point<3>, Handle)> = solid + .shells() + .flat_map(|s| s.faces()) + .flat_map(|face| { + face.all_cycles() + .flat_map(|cycle| cycle.half_edges().cloned()) + .zip(repeat(face.surface().geometry())) + }) + .map(|(h, s)| { + ( + s.point_from_surface_coords(h.start_position()), + h.start_vertex().clone(), + ) + }) + .collect(); + + // This is O(N^2) which isn't great, but we can't use a HashMap since we + // need to deal with float inaccuracies. Maybe we could use some smarter + // data-structure like an octree. + for a in &vertices { + for b in &vertices { + match a.1.id() == b.1.id() { + true => { + if a.0.distance_to(&b.0) > config.identical_max_distance + { + errors.push( + Self::IdenticalVertsNotCoincident([ + (a.1.clone(), a.0), + (b.1.clone(), b.0), + ]) + .into(), + ) + } + } + false => { + if a.0.distance_to(&b.0) < config.distinct_min_distance + { + errors.push( + Self::DistinctVertsCoincide([ + (a.1.clone(), a.0), + (b.1.clone(), b.0), + ]) + .into(), + ) + } + } + } + } + } } } From f6cd58a1a29cabcd3934d99f05e297cd0e65a7fc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Mar 2023 11:30:51 +0100 Subject: [PATCH 10/10] Remove misplaced comment --- crates/fj-kernel/src/validate/shell.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 3f1bc690a..109c6f452 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -189,7 +189,6 @@ mod tests { fn coincident_not_identical() -> anyhow::Result<()> { let mut services = Services::new(); let invalid = { - // Shell with single face is not watertight let face1 = FaceBuilder::new(services.objects.surfaces.xy_plane()) .with_exterior(CycleBuilder::polygon([ [0., 0.],