diff --git a/crates/fj-core/src/algorithms/approx/curve.rs b/crates/fj-core/src/algorithms/approx/curve.rs index fcd2dc82b..82bff3dc3 100644 --- a/crates/fj-core/src/algorithms/approx/curve.rs +++ b/crates/fj-core/src/algorithms/approx/curve.rs @@ -31,7 +31,12 @@ impl Approx for (&Handle, &HalfEdgeGeom, &Handle) { Some(approx) => approx, None => { let approx = approx_curve( - &half_edge.path, + &geometry + .of_curve(curve) + .unwrap() + .local_on(surface) + .unwrap() + .path, geometry.of_surface(surface), half_edge.boundary, tolerance, @@ -198,7 +203,7 @@ mod tests { let curve = Curve::from_path_and_surface(path, surface.clone(), &mut core); let boundary = CurveBoundary::from(boundary); - let half_edge = HalfEdgeGeom { path, boundary }; + let half_edge = HalfEdgeGeom { boundary }; let tolerance = 1.; let approx = (&curve, &half_edge, &surface) @@ -221,7 +226,7 @@ mod tests { let curve = Curve::from_path_and_surface(path, surface.clone(), &mut core); let boundary = CurveBoundary::from(boundary); - let half_edge = HalfEdgeGeom { path, boundary }; + let half_edge = HalfEdgeGeom { boundary }; let tolerance = 1.; let approx = (&curve, &half_edge, &surface) @@ -243,7 +248,7 @@ mod tests { let curve = Curve::from_path_and_surface(path, surface.clone(), &mut core); let boundary = CurveBoundary::from([[0.], [TAU]]); - let half_edge = HalfEdgeGeom { path, boundary }; + let half_edge = HalfEdgeGeom { boundary }; let tolerance = 1.; let approx = (&curve, &half_edge, &surface) @@ -274,7 +279,7 @@ mod tests { let curve = Curve::from_path_and_surface(path, surface.clone(), &mut core); let boundary = CurveBoundary::from([[0.], [TAU]]); - let half_edge = HalfEdgeGeom { path, boundary }; + let half_edge = HalfEdgeGeom { boundary }; let tolerance = 1.; let approx = (&curve, &half_edge, &surface) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 19fc1d799..bc9ea427b 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -61,7 +61,10 @@ impl Approx for (&Handle, &Handle) { approx.points.into_iter().map(|point| { let point_surface = geometry - .of_half_edge(half_edge) + .of_curve(half_edge.curve()) + .unwrap() + .local_on(surface) + .unwrap() .path .point_from_path_coords(point.local_form); diff --git a/crates/fj-core/src/algorithms/bounding_volume/cycle.rs b/crates/fj-core/src/algorithms/bounding_volume/cycle.rs index 138583387..a934a38a1 100644 --- a/crates/fj-core/src/algorithms/bounding_volume/cycle.rs +++ b/crates/fj-core/src/algorithms/bounding_volume/cycle.rs @@ -1,15 +1,19 @@ use fj_math::Aabb; -use crate::{geometry::Geometry, topology::Cycle}; +use crate::{ + geometry::Geometry, + storage::Handle, + topology::{Cycle, Surface}, +}; -impl super::BoundingVolume<2> for &Cycle { +impl super::BoundingVolume<2> for (&Cycle, &Handle) { fn aabb(self, geometry: &Geometry) -> Option> { - let cycle = self; + let (cycle, surface) = self; let mut aabb: Option> = None; for half_edge in cycle.half_edges() { - let new_aabb = half_edge + let new_aabb = (half_edge, surface) .aabb(geometry) .expect("`HalfEdge` can always compute AABB"); aabb = Some(aabb.map_or(new_aabb, |aabb| aabb.merged(&new_aabb))); diff --git a/crates/fj-core/src/algorithms/bounding_volume/face.rs b/crates/fj-core/src/algorithms/bounding_volume/face.rs index dd2411435..375d54bcf 100644 --- a/crates/fj-core/src/algorithms/bounding_volume/face.rs +++ b/crates/fj-core/src/algorithms/bounding_volume/face.rs @@ -1,3 +1,5 @@ +use std::ops::Deref; + use fj_math::Aabb; use crate::{ @@ -7,27 +9,30 @@ use crate::{ impl super::BoundingVolume<3> for &Face { fn aabb(self, geometry: &Geometry) -> Option> { - self.region().exterior().aabb(geometry).map(|aabb2| { - let surface = geometry.of_surface(self.surface()); + (self.region().exterior().deref(), self.surface()) + .aabb(geometry) + .map(|aabb2| { + let surface = geometry.of_surface(self.surface()); - match surface.u { - GlobalPath::Circle(circle) => { - // This is not the most precise way to calculate the AABB, - // doing it for the whole circle, but it should do. + match surface.u { + GlobalPath::Circle(circle) => { + // This is not the most precise way to calculate the + // AABB, doing it for the whole circle, but it should + // do. - let aabb_bottom = circle.aabb(); - let aabb_top = Aabb { - min: aabb_bottom.min + surface.v, - max: aabb_bottom.max + surface.v, - }; + let aabb_bottom = circle.aabb(); + let aabb_top = Aabb { + min: aabb_bottom.min + surface.v, + max: aabb_bottom.max + surface.v, + }; - aabb_bottom.merged(&aabb_top) + aabb_bottom.merged(&aabb_top) + } + GlobalPath::Line(_) => Aabb { + min: surface.point_from_surface_coords(aabb2.min), + max: surface.point_from_surface_coords(aabb2.max), + }, } - GlobalPath::Line(_) => Aabb { - min: surface.point_from_surface_coords(aabb2.min), - max: surface.point_from_surface_coords(aabb2.max), - }, - } - }) + }) } } diff --git a/crates/fj-core/src/algorithms/bounding_volume/half_edge.rs b/crates/fj-core/src/algorithms/bounding_volume/half_edge.rs index 690cf6acc..5f5322f62 100644 --- a/crates/fj-core/src/algorithms/bounding_volume/half_edge.rs +++ b/crates/fj-core/src/algorithms/bounding_volume/half_edge.rs @@ -3,15 +3,20 @@ use fj_math::{Aabb, Vector}; use crate::{ geometry::{Geometry, SurfacePath}, storage::Handle, - topology::HalfEdge, + topology::{HalfEdge, Surface}, }; -impl super::BoundingVolume<2> for &Handle { +impl super::BoundingVolume<2> for (&Handle, &Handle) { fn aabb(self, geometry: &Geometry) -> Option> { - let half_edge = self; + let (half_edge, surface) = self; let half_edge_geom = geometry.of_half_edge(half_edge); - let path = half_edge_geom.path; + let path = geometry + .of_curve(half_edge.curve()) + .unwrap() + .local_on(surface) + .unwrap() + .path; match path { SurfacePath::Circle(circle) => { diff --git a/crates/fj-core/src/geometry/half_edge.rs b/crates/fj-core/src/geometry/half_edge.rs index 0ea12b321..6f8e2730b 100644 --- a/crates/fj-core/src/geometry/half_edge.rs +++ b/crates/fj-core/src/geometry/half_edge.rs @@ -5,33 +5,6 @@ use super::{CurveBoundary, SurfacePath}; /// The geometry of a half-edge #[derive(Copy, Clone, Debug)] pub struct HalfEdgeGeom { - /// # The path of the half-edge - /// - /// ## Implementation Note - /// - /// Currently, all curve-related geometry is defined locally, in terms of - /// the surface that the curve is on (or purely in 2D, if there is no - /// surface associated with this geometry). However, curves exist globally, - /// independently of surfaces. Half-edges in multiple surfaces can refer to - /// the same curve, and in fact, that is the whole reason for their - /// existence as a topological object. - /// - /// This contradiction, globally defined curves but locally defined curve - /// geometry, is the reason that this curve geometry is defined right here, - /// associated with a locally existing half-edge. (And, I might add, - /// redundantly so, as multiple half-edges within the same surface context - /// can refer to the same curve.) - /// - /// Instead, it should be possible to define curve geometry *either* locally - /// or globally. Then that respective definition can be associated with the - /// curve (and possibly, in addition, a surface). How exactly that is going - /// to work is up in the air. - /// - /// The point of all this exposition is to clarify that this field doesn't - /// really belong here. It exists here for practical reasons that are, - /// hopefully, temporary. - pub path: SurfacePath, - /// # The boundary of the half-edge on its curve pub boundary: CurveBoundary>, } diff --git a/crates/fj-core/src/operations/build/half_edge.rs b/crates/fj-core/src/operations/build/half_edge.rs index 1eb04bdfb..e1c6074ce 100644 --- a/crates/fj-core/src/operations/build/half_edge.rs +++ b/crates/fj-core/src/operations/build/half_edge.rs @@ -73,7 +73,6 @@ pub trait BuildHalfEdge { core.layers.geometry.define_half_edge( half_edge.clone(), HalfEdgeGeom { - path, boundary: boundary.into(), }, ); @@ -100,14 +99,6 @@ pub trait BuildHalfEdge { core.layers.geometry.define_half_edge( half_edge.clone(), HalfEdgeGeom { - path: core - .layers - .geometry - .of_curve(half_edge.curve()) - .expect("Curve geometry was just defined in same function") - .local_on(&surface) - .expect("Curve geometry was just defined in same function") - .path, boundary: boundary.unwrap_or_default(), }, ); diff --git a/crates/fj-core/src/operations/build/shell.rs b/crates/fj-core/src/operations/build/shell.rs index 7bdcaee46..c0d94e47a 100644 --- a/crates/fj-core/src/operations/build/shell.rs +++ b/crates/fj-core/src/operations/build/shell.rs @@ -102,23 +102,7 @@ pub trait BuildShell { .update_curve(|_, _| curve.clone(), core) .insert(core) .set_geometry( - HalfEdgeGeom { - path: core - .layers - .geometry - .of_curve(&curve) - .expect( - "Curve geometry was just \ - defined in same function", - ) - .local_on(&surface) - .expect( - "Curve geometry was just \ - defined in same function", - ) - .path, - boundary, - }, + HalfEdgeGeom { boundary }, &mut core.layers.geometry, ) }) diff --git a/crates/fj-core/src/operations/reverse/half_edge.rs b/crates/fj-core/src/operations/reverse/half_edge.rs index aa241ab12..eb7d3c4c5 100644 --- a/crates/fj-core/src/operations/reverse/half_edge.rs +++ b/crates/fj-core/src/operations/reverse/half_edge.rs @@ -17,7 +17,6 @@ impl ReverseCurveCoordinateSystems for (&Handle, &Handle) { let (half_edge, surface) = self; let mut half_edge_geom = *core.layers.geometry.of_half_edge(half_edge); - half_edge_geom.path = half_edge_geom.path.reverse(); half_edge_geom.boundary = half_edge_geom.boundary.reverse(); let curve = diff --git a/crates/fj-core/src/operations/sweep/half_edge.rs b/crates/fj-core/src/operations/sweep/half_edge.rs index f964f98bc..3e350152a 100644 --- a/crates/fj-core/src/operations/sweep/half_edge.rs +++ b/crates/fj-core/src/operations/sweep/half_edge.rs @@ -143,23 +143,7 @@ impl SweepHalfEdge for Handle { .update_curve(|_, _| curve.clone(), core) .insert(core) .set_geometry( - HalfEdgeGeom { - path: core - .layers - .geometry - .of_curve(&curve) - .expect( - "Curve geometry was just defined in same \ - function", - ) - .local_on(&surface) - .expect( - "Curve geometry was just defined in same \ - function", - ) - .path, - boundary, - }, + HalfEdgeGeom { boundary }, &mut core.layers.geometry, ); diff --git a/crates/fj-core/src/validation/checks/coincident_half_edges_are_not_siblings.rs b/crates/fj-core/src/validation/checks/coincident_half_edges_are_not_siblings.rs index 1a81d4794..e2956053e 100644 --- a/crates/fj-core/src/validation/checks/coincident_half_edges_are_not_siblings.rs +++ b/crates/fj-core/src/validation/checks/coincident_half_edges_are_not_siblings.rs @@ -124,17 +124,21 @@ impl ValidationCheck for CoincidentHalfEdgesAreNotSiblings { continue; } - // If all points on distinct curves are within - // `distinct_min_distance`, that's a problem. - if distances( + let Some(mut distances) = distances( half_edge_a.clone(), surface_a, half_edge_b.clone(), surface_b, geometry, - ) - .all(|d| d < config.distinct_min_distance) - { + ) else { + // The geometry to compute the distances is not available, + // hence these half-edges can't be coincident. + continue; + }; + + // If all points on distinct curves are within + // `distinct_min_distance`, that's a problem. + if distances.all(|d| d < config.distinct_min_distance) { let boundaries = [half_edge_a, half_edge_b].map(|half_edge| { geometry.of_half_edge(half_edge).boundary @@ -174,20 +178,27 @@ fn distances( half_edge_b: Handle, surface_b: &Handle, geometry: &Geometry, -) -> impl Iterator { +) -> Option> { fn sample( percent: f64, half_edge: &Handle, surface: &Handle, geometry: &Geometry, - ) -> Point<3> { + ) -> Option> { let [start, end] = geometry.of_half_edge(half_edge).boundary.inner; let path_coords = start + (end - start) * percent; - let path = geometry.of_half_edge(half_edge).path; + // let path = geometry.of_half_edge(half_edge).path; + let path = geometry + .of_curve(half_edge.curve())? + .local_on(surface)? + .path; + // assert_eq!(path, path_from_curve); let surface_coords = path.point_from_path_coords(path_coords); - geometry - .of_surface(surface) - .point_from_surface_coords(surface_coords) + Some( + geometry + .of_surface(surface) + .point_from_surface_coords(surface_coords), + ) } // Three samples (start, middle, end), are enough to detect weather lines @@ -199,11 +210,11 @@ fn distances( let mut distances = Vec::new(); for i in 0..sample_count { let percent = i as f64 * step; - let sample1 = sample(percent, &half_edge_a, surface_a, geometry); - let sample2 = sample(1.0 - percent, &half_edge_b, surface_b, geometry); + let sample1 = sample(percent, &half_edge_a, surface_a, geometry)?; + let sample2 = sample(1.0 - percent, &half_edge_b, surface_b, geometry)?; distances.push(sample1.distance_to(&sample2)) } - distances.into_iter() + Some(distances.into_iter()) } #[cfg(test)] diff --git a/crates/fj-core/src/validation/checks/curve_geometry_mismatch.rs b/crates/fj-core/src/validation/checks/curve_geometry_mismatch.rs index b0c96b1ef..bd8fb440b 100644 --- a/crates/fj-core/src/validation/checks/curve_geometry_mismatch.rs +++ b/crates/fj-core/src/validation/checks/curve_geometry_mismatch.rs @@ -70,61 +70,103 @@ impl ValidationCheck for CurveGeometryMismatch { .clone() .into_iter() .cartesian_product(edges_and_surfaces) - .filter_map(|((edge_a, surface_a), (edge_b, surface_b))| { - // We only care about edges referring to the same curve. - if edge_a.curve().id() != edge_b.curve().id() { - return None; - } - - // No need to check an edge against itself. - if edge_a.id() == edge_b.id() { - return None; - } - - let surface_a = geometry.of_surface(&surface_a); - let surface_b = geometry.of_surface(&surface_b); - - // 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] = geometry.of_half_edge(&edge_a).boundary.inner; - let b = a + (d - a) * 1. / 3.; - let c = a + (d - a) * 2. / 3.; - - let mut errors: Vec = Vec::new(); - - for point_curve in [a, b, c, d] { - let a_surface = geometry - .of_half_edge(&edge_a) - .path - .point_from_path_coords(point_curve); - let b_surface = geometry - .of_half_edge(&edge_b) - .path - .point_from_path_coords(point_curve); - - let a_global = - surface_a.point_from_surface_coords(a_surface); - let b_global = - surface_b.point_from_surface_coords(b_surface); - - let distance = (a_global - b_global).magnitude(); - - if distance > config.identical_max_distance { - errors.push(Self { - half_edge_a: edge_a.clone(), - half_edge_b: edge_b.clone(), - point_curve, - point_a: a_global, - point_b: b_global, - distance, - }); + .filter_map( + |((half_edge_a, surface_a), (half_edge_b, surface_b))| { + // We only care about edges referring to the same curve. + if half_edge_a.curve().id() != half_edge_b.curve().id() { + return None; } - } - Some(errors) - }) + // No need to check an edge against itself. + if half_edge_a.id() == half_edge_b.id() { + return None; + } + + let Some(curve_geom) = + geometry.of_curve(half_edge_a.curve()) + else { + // No geometry defined for the curve. Nothing to test + // here. + return None; + }; + + let local_curve_geom_a = curve_geom.local_on(&surface_a); + let local_curve_geom_b = curve_geom.local_on(&surface_b); + + let (local_curve_geom_a, local_curve_geom_b) = + match (local_curve_geom_a, local_curve_geom_b) { + (Some(a), Some(b)) => (a, b), + (None, None) => { + // No geometry defined for the curve on the + // respective surface. Nothing to test here. + return None; + } + _ => { + // This means that geometry is only defined for + // one of the curves, which is a mismatch. But + // it is a mismatch that can't be represented + // with the validation error struct, as + // currently defined. + // + // I don't want to put work into addressing this + // right now, as in the future, curve geometry + // hopefully won't be redundantly defined, and + // this whole validation check will become + // redundant. + // + // For this reason, I'm choosing the easy way + // here, and that should do just fine for now. + panic!( + "Curve geometry mismatch: Curve not \ + defined on all required surfaces." + ); + } + }; + + let surface_geom_a = geometry.of_surface(&surface_a); + let surface_geom_b = geometry.of_surface(&surface_b); + + // 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] = + geometry.of_half_edge(&half_edge_a).boundary.inner; + let b = a + (d - a) * 1. / 3.; + let c = a + (d - a) * 2. / 3.; + + let mut errors: Vec = Vec::new(); + + for point_curve in [a, b, c, d] { + let a_surface = local_curve_geom_a + .path + .point_from_path_coords(point_curve); + let b_surface = local_curve_geom_b + .path + .point_from_path_coords(point_curve); + + let a_global = + surface_geom_a.point_from_surface_coords(a_surface); + let b_global = + surface_geom_b.point_from_surface_coords(b_surface); + + let distance = (a_global - b_global).magnitude(); + + if distance > config.identical_max_distance { + errors.push(Self { + half_edge_a: half_edge_a.clone(), + half_edge_b: half_edge_b.clone(), + point_curve, + point_a: a_global, + point_b: b_global, + distance, + }); + } + } + + Some(errors) + }, + ) .flatten() } } @@ -166,23 +208,41 @@ mod tests { cycle.update_half_edge( cycle.half_edges().nth_circular(0), |half_edge, core| { - let mut geometry = *core + let mut half_edge_geom = *core .layers .geometry .of_half_edge(half_edge); - geometry.path = geometry.path.reverse(); - geometry.boundary = - geometry.boundary.reverse(); + half_edge_geom.boundary = + half_edge_geom.boundary.reverse(); - [HalfEdge::new( + let mut curve_geom = core + .layers + .geometry + .of_curve(half_edge.curve()) + .unwrap() + .local_on(face.surface()) + .unwrap() + .clone(); + curve_geom.path = + curve_geom.path.reverse(); + + let half_edge = HalfEdge::new( half_edge.curve().clone(), half_edge.start_vertex().clone(), ) .insert(core) .set_geometry( - geometry, + half_edge_geom, &mut core.layers.geometry, - )] + ); + + core.layers.geometry.define_curve( + half_edge.curve().clone(), + face.surface().clone(), + curve_geom, + ); + + [half_edge] }, core, ) diff --git a/crates/fj-core/src/validation/checks/half_edge_connection.rs b/crates/fj-core/src/validation/checks/half_edge_connection.rs index 636ca1b56..9142e9ef3 100644 --- a/crates/fj-core/src/validation/checks/half_edge_connection.rs +++ b/crates/fj-core/src/validation/checks/half_edge_connection.rs @@ -101,7 +101,10 @@ fn check_cycle<'r>( let end_pos_of_first_half_edge = { let [_, end] = geometry.of_half_edge(first).boundary.inner; geometry - .of_half_edge(first) + .of_curve(first.curve()) + .unwrap() + .local_on(surface) + .unwrap() .path .point_from_path_coords(end) };