diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 9a6e0ebb51..f6b21813df 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(), + ) + } + } } } }