From 071d9f8a956f13d187849c04eaf83ae43c33b900 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 2 Aug 2023 10:49:59 +0200 Subject: [PATCH 1/5] Document assumptions of join operation --- crates/fj-core/src/operations/join/cycle.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/fj-core/src/operations/join/cycle.rs b/crates/fj-core/src/operations/join/cycle.rs index 851dd874f..c7320dc85 100644 --- a/crates/fj-core/src/operations/join/cycle.rs +++ b/crates/fj-core/src/operations/join/cycle.rs @@ -35,6 +35,18 @@ pub trait JoinCycle { /// /// Panics, if the ranges have different lengths. /// + /// # Assumptions + /// + /// This method makes some assumptions that need to be met, if the operation + /// is to result in a valid shape: + /// + /// - **The joined half-edges must be coincident.** + /// - **The locally defined curve coordinate systems of the edges must + /// match.** + /// + /// If either of those assumptions are not met, this will result in a + /// validation error down the line. + /// /// # Implementation Note /// /// The use of the `RangeInclusive` type might be a bit limiting, as other From a03a6366f0687d73b3536c4d52198417bcf7dccf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 3 Aug 2023 07:52:05 +0200 Subject: [PATCH 2/5] Fix curve coordinate systems Make sure that each of the local definitions match. --- crates/fj-core/src/operations/build/shell.rs | 29 ++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/operations/build/shell.rs b/crates/fj-core/src/operations/build/shell.rs index 2fdc55442..be0df735b 100644 --- a/crates/fj-core/src/operations/build/shell.rs +++ b/crates/fj-core/src/operations/build/shell.rs @@ -3,8 +3,9 @@ use fj_math::Point; use crate::{ objects::{Face, Shell}, operations::{ - update::region::UpdateRegion, BuildFace, Insert, IsInserted, - IsInsertedNo, IsInsertedYes, JoinCycle, Polygon, UpdateFace, + reverse::ReverseCurveCoordinateSystems, update::region::UpdateRegion, + BuildFace, Insert, IsInserted, IsInsertedNo, IsInsertedYes, JoinCycle, + Polygon, UpdateCycle, UpdateFace, }, services::Services, }; @@ -45,6 +46,10 @@ pub trait BuildShell { region .update_exterior(|cycle| { cycle + .update_nth_half_edge(0, |edge| { + edge.reverse_curve_coordinate_systems(services) + .insert(services) + }) .join_to( abc.face.region().exterior(), 0..=0, @@ -59,12 +64,20 @@ pub trait BuildShell { region .update_exterior(|cycle| { cycle + .update_nth_half_edge(1, |edge| { + edge.reverse_curve_coordinate_systems(services) + .insert(services) + }) .join_to( abc.face.region().exterior(), 1..=1, 2..=2, services, ) + .update_nth_half_edge(0, |edge| { + edge.reverse_curve_coordinate_systems(services) + .insert(services) + }) .join_to( bad.face.region().exterior(), 0..=0, @@ -79,18 +92,30 @@ pub trait BuildShell { region .update_exterior(|cycle| { cycle + .update_nth_half_edge(0, |edge| { + edge.reverse_curve_coordinate_systems(services) + .insert(services) + }) .join_to( abc.face.region().exterior(), 0..=0, 1..=1, services, ) + .update_nth_half_edge(1, |edge| { + edge.reverse_curve_coordinate_systems(services) + .insert(services) + }) .join_to( bad.face.region().exterior(), 1..=1, 2..=2, services, ) + .update_nth_half_edge(2, |edge| { + edge.reverse_curve_coordinate_systems(services) + .insert(services) + }) .join_to( dac.face.region().exterior(), 2..=2, From 9b14782223bce655ee17b41bfa9066d269f2031b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 1 Aug 2023 12:32:56 +0200 Subject: [PATCH 3/5] Validate that curve coordinate systems match --- crates/fj-core/src/validate/shell.rs | 156 +++++++++++++++++++++++++++ 1 file changed, 156 insertions(+) diff --git a/crates/fj-core/src/validate/shell.rs b/crates/fj-core/src/validate/shell.rs index 4bb91cc2d..38e1763c8 100644 --- a/crates/fj-core/src/validate/shell.rs +++ b/crates/fj-core/src/validate/shell.rs @@ -17,6 +17,7 @@ impl Validate for Shell { config: &ValidationConfig, errors: &mut Vec, ) { + ShellValidationError::validate_curve_coordinates(self, config, errors); ShellValidationError::validate_edges_coincident(self, config, errors); ShellValidationError::validate_watertight(self, config, errors); } @@ -25,6 +26,14 @@ impl Validate for Shell { /// [`Shell`] validation failed #[derive(Clone, Debug, thiserror::Error)] pub enum ShellValidationError { + /// [`Shell`] contains curves whose coordinate systems don't match + #[error( + "Curve coordinate system mismatch ({} errors): {:#?}", + .0.len(), + .0 + )] + CurveCoordinateSystemMismatch(Vec), + /// [`Shell`] contains global_edges not referred to by two half-edges #[error("Shell is not watertight")] NotWatertight, @@ -107,6 +116,98 @@ fn distances( } impl ShellValidationError { + fn validate_curve_coordinates( + shell: &Shell, + config: &ValidationConfig, + errors: &mut Vec, + ) { + let mut edges_and_surfaces = Vec::new(); + shell.all_edges_with_surface(&mut edges_and_surfaces); + + for (edge_a, surface_a) in &edges_and_surfaces { + for (edge_b, surface_b) in &edges_and_surfaces { + // We only care about edges referring to the same curve. + if edge_a.curve().id() != edge_b.curve().id() { + continue; + } + + // No need to check an edge against itself. + if edge_a.id() == edge_b.id() { + continue; + } + + fn compare_curve_coords( + edge_a: &Handle, + surface_a: &Handle, + edge_b: &Handle, + surface_b: &Handle, + config: &ValidationConfig, + mismatches: &mut Vec, + ) { + // Let's check 4 points. Given that the most complex curves + // we have right now are circles, 3 would be enough to check + // for coincidence. But the first and last might be + // identical, so let's add an extra one. + let [a, d] = edge_a.boundary().inner; + let b = a + (d - a) * 1. / 3.; + let c = a + (d - a) * 2. / 3.; + + for point_curve in [a, b, c, d] { + let a_surface = + edge_a.path().point_from_path_coords(point_curve); + let b_surface = + edge_b.path().point_from_path_coords(point_curve); + + let a_global = surface_a + .geometry() + .point_from_surface_coords(a_surface); + let b_global = surface_b + .geometry() + .point_from_surface_coords(b_surface); + + let distance = (a_global - b_global).magnitude(); + + if distance > config.identical_max_distance { + mismatches.push(CurveCoordinateSystemMismatch { + edge_a: edge_a.clone(), + edge_b: edge_b.clone(), + point_curve, + point_a: a_global, + point_b: b_global, + distance, + }); + } + } + } + + let mut mismatches = Vec::new(); + + compare_curve_coords( + edge_a, + surface_a, + edge_b, + surface_b, + config, + &mut mismatches, + ); + compare_curve_coords( + edge_b, + surface_b, + edge_a, + surface_a, + config, + &mut mismatches, + ); + + if !mismatches.is_empty() { + errors.push( + Self::CurveCoordinateSystemMismatch(mismatches).into(), + ); + } + } + } + } + fn validate_edges_coincident( shell: &Shell, config: &ValidationConfig, @@ -249,6 +350,16 @@ impl ShellValidationError { } } +#[derive(Clone, Debug)] +pub struct CurveCoordinateSystemMismatch { + pub edge_a: Handle, + pub edge_b: Handle, + pub point_curve: Point<1>, + pub point_a: Point<3>, + pub point_b: Point<3>, + pub distance: Scalar, +} + #[cfg(test)] mod tests { use crate::{ @@ -262,6 +373,51 @@ mod tests { validate::{shell::ShellValidationError, Validate, ValidationError}, }; + #[test] + fn curve_coordinate_system_mismatch() -> anyhow::Result<()> { + let mut services = Services::new(); + + let valid = Shell::tetrahedron( + [[0., 0., 0.], [0., 1., 0.], [1., 0., 0.], [0., 0., 1.]], + &mut services, + ); + let invalid = valid.shell.replace_face( + &valid.abc.face, + valid + .abc + .face + .update_region(|region| { + region + .update_exterior(|cycle| { + cycle + .update_nth_half_edge(0, |half_edge| { + half_edge + .replace_path( + half_edge.path().reverse(), + ) + .replace_boundary( + half_edge.boundary().reverse(), + ) + .insert(&mut services) + }) + .insert(&mut services) + }) + .insert(&mut services) + }) + .insert(&mut services), + ); + + valid.shell.validate_and_return_first_error()?; + assert_contains_err!( + invalid, + ValidationError::Shell( + ShellValidationError::CurveCoordinateSystemMismatch(..) + ) + ); + + Ok(()) + } + #[test] fn coincident_not_identical() -> anyhow::Result<()> { let mut services = Services::new(); From c9c3f9685c60051dcd10e8d2f1df9416c0f95e8e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 3 Aug 2023 08:06:30 +0200 Subject: [PATCH 4/5] Don't do unnecessary work in validation check --- crates/fj-core/src/validate/shell.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/fj-core/src/validate/shell.rs b/crates/fj-core/src/validate/shell.rs index 38e1763c8..9f2007439 100644 --- a/crates/fj-core/src/validate/shell.rs +++ b/crates/fj-core/src/validate/shell.rs @@ -221,6 +221,11 @@ impl ShellValidationError { // data-structure like an octree. for (edge_a, surface_a) in &edges_and_surfaces { for (edge_b, surface_b) in &edges_and_surfaces { + // No need to check an edge against itself. + if edge_a.id() == edge_b.id() { + continue; + } + let identical_according_to_global_form = edge_a.global_form().id() == edge_b.global_form().id(); From f611694a81b3cc4d2c2c1af95b9c33a00273facf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 3 Aug 2023 08:06:57 +0200 Subject: [PATCH 5/5] Simplify validation check --- crates/fj-core/src/validate/shell.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/crates/fj-core/src/validate/shell.rs b/crates/fj-core/src/validate/shell.rs index 9f2007439..1ac1949fa 100644 --- a/crates/fj-core/src/validate/shell.rs +++ b/crates/fj-core/src/validate/shell.rs @@ -75,7 +75,6 @@ pub enum ShellValidationError { /// /// Returns an [`Iterator`] of the distance at each sample. fn distances( - config: &ValidationConfig, edge_a: Handle, surface_a: Handle, edge_b: Handle, @@ -91,11 +90,6 @@ fn distances( 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, (&edge_a, surface_a.geometry())) - .distance_to(&sample(0.0, (&edge_b, surface_b.geometry()))) - > config.identical_max_distance; - // 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. @@ -106,10 +100,7 @@ fn distances( for i in 0..sample_count { let percent = i as f64 * step; let sample1 = sample(percent, (&edge_a, surface_a.geometry())); - let sample2 = sample( - if flip { 1.0 - percent } else { percent }, - (&edge_b, surface_b.geometry()), - ); + let sample2 = sample(1.0 - percent, (&edge_b, surface_b.geometry())); distances.push(sample1.distance_to(&sample2)) } distances.into_iter() @@ -259,7 +250,6 @@ impl ShellValidationError { // identical_max_distance, so we shouldn't have any // greater than the max if distances( - config, edge_a.clone(), surface_a.clone(), edge_b.clone(), @@ -282,7 +272,6 @@ impl ShellValidationError { // If all points on distinct curves are within // distinct_min_distance, that's a problem. if distances( - config, edge_a.clone(), surface_a.clone(), edge_b.clone(),