diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 747e4b012..911d46cea 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -292,7 +292,7 @@ mod tests { use crate::{ geometry::{Curve, Surface}, - shape::{Handle, Shape, ValidationError, ValidationResult}, + shape::{Handle, LocalForm, Shape, ValidationError, ValidationResult}, topology::{Cycle, Edge, Face, Vertex}, }; @@ -382,18 +382,24 @@ mod tests { let a = Vertex::builder(&mut other).build_from_point([1., 0., 0.])?; let b = Vertex::builder(&mut other).build_from_point([2., 0., 0.])?; + let a = LocalForm::new(Point::from([1.]), a); + let b = LocalForm::new(Point::from([2.]), b); + // Shouldn't work. Nothing has been added to `shape`. let err = shape .insert(Edge::new(curve.clone(), Some([a.clone(), b.clone()]))) .unwrap_err(); assert!(err.missing_curve(&curve)); - assert!(err.missing_vertex(&a)); - assert!(err.missing_vertex(&b)); + assert!(err.missing_vertex(&a.canonical())); + assert!(err.missing_vertex(&b.canonical())); let curve = shape.add_curve()?; let a = Vertex::builder(&mut shape).build_from_point([1., 0., 0.])?; let b = Vertex::builder(&mut shape).build_from_point([2., 0., 0.])?; + let a = LocalForm::new(Point::from([1.]), a); + let b = LocalForm::new(Point::from([2.]), b); + // Everything has been added to `shape` now. Should work! shape.insert(Edge::new(curve, Some([a, b])))?; diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index a08cd4636..d84e3c818 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -5,7 +5,10 @@ use crate::{ topology::{Cycle, Edge, Face, Vertex}, }; -use super::{validate::Validate, Handle, Mapping, Shape, ValidationResult}; +use super::{ + validate::Validate, Handle, LocalForm, Mapping, Shape, ValidationError, + ValidationResult, +}; /// Marker trait for geometric and topological objects pub trait Object: @@ -116,12 +119,18 @@ impl Object for Edge<3> { // Can be cleaned up using `try_map`, once that is stable: // https://doc.rust-lang.org/std/primitive.array.html#method.try_map - let vertices = self.vertices.map(|vertices| { - vertices.map(|vertex| { - let vertex = vertex.canonical(); - vertex.get().merge_into(Some(vertex), shape, mapping) - }) - }); + let vertices: Option<[Result<_, ValidationError>; 2]> = + self.vertices.map(|vertices| { + vertices.map(|vertex| { + let canonical = vertex.canonical(); + let canonical = canonical.get().merge_into( + Some(canonical), + shape, + mapping, + )?; + Ok(LocalForm::new(*vertex.local(), canonical)) + }) + }); let vertices = match vertices { Some([a, b]) => Some([a?, b?]), None => None, diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index a79de2a6b..2d363250e 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -2,7 +2,7 @@ use fj_math::{Circle, Line, Point, Scalar, Vector}; use crate::{ geometry::{Curve, Surface}, - shape::{Handle, Shape, ValidationResult}, + shape::{Handle, LocalForm, Shape, ValidationResult}, }; use super::{Cycle, Edge, Face, Vertex}; @@ -85,6 +85,13 @@ impl<'r> EdgeBuilder<'r> { let curve = self.shape.insert(Curve::Line(Line::from_points( vertices.clone().map(|vertex| vertex.get().point()), )))?; + + let [a, b] = vertices; + let vertices = [ + LocalForm::new(Point::from([0.]), a), + LocalForm::new(Point::from([1.]), b), + ]; + let edge = self.shape.insert(Edge::new(curve, Some(vertices)))?; Ok(edge) diff --git a/crates/fj-kernel/src/topology/edge.rs b/crates/fj-kernel/src/topology/edge.rs index 424ef0218..0f3289ead 100644 --- a/crates/fj-kernel/src/topology/edge.rs +++ b/crates/fj-kernel/src/topology/edge.rs @@ -39,42 +39,11 @@ pub struct Edge { impl Edge<3> { /// Construct an instance of `Edge` - /// - /// # Implementation Note - /// - /// This method accepts two `Handle`, and projects them onto the - /// curve, to create the local representation that is stored in `Edge`. This - /// could lead to incorrect results, as the caller could provide vertices - /// that don't actually lie on `curve`. - /// - /// The good news is, that whole thing seems to be unnecessary. Or rather, - /// this whole method seems to be unnecessary. I reviewed all the call - /// sites, and in all cases, there seems to be a better way to construct the - /// `Edge`, without using this constructor. - /// - /// Ideally, we'd just fix all those call sites and remove this method, to - /// completely avoid the potential for any bugs to occur here. Problem is, - /// some of those call sites can't be fixed easily, without building further - /// infrastructure. Here's one such piece of infrastructure, for which an - /// issue already exists: - /// pub fn new( curve: Handle>, - vertices: Option<[Handle; 2]>, + vertices: Option<[LocalForm, Vertex>; 2]>, ) -> Self { let curve = LocalForm::canonical_only(curve); - - let vertices = vertices.map(|vertices| { - vertices.map(|canonical| { - let local = curve - .canonical() - .get() - .point_to_curve_coords(canonical.get().point()) - .local(); - LocalForm::new(local, canonical) - }) - }); - Self { curve, vertices } } diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index f39df6da0..4cf300e0b 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/crates/fj-operations/src/difference_2d.rs @@ -1,7 +1,7 @@ use fj_interop::debug::DebugInfo; use fj_kernel::{ algorithms::Tolerance, - shape::{Handle, Shape, ValidationError, ValidationResult}, + shape::{Handle, LocalForm, Shape, ValidationError, ValidationResult}, topology::{Cycle, Edge, Face}, }; use fj_math::Aabb; @@ -103,14 +103,17 @@ fn add_cycle( let curve = if reverse { curve.reverse() } else { curve }; let curve = shape.insert(curve)?; - let vertices = edge.vertices.clone().map(|vs| { - let mut vs = vs.map(|vertex| vertex.canonical()); - + let vertices = edge.vertices.clone().map(|[a, b]| { if reverse { - vs.reverse(); + // Switch `a` and `b`, but make sure the local forms are still + // correct, after we reversed the curve above. + [ + LocalForm::new(-(*b.local()), b.canonical()), + LocalForm::new(-(*a.local()), a.canonical()), + ] + } else { + [a, b] } - - vs }); let edge = shape.merge(Edge::new(curve, vertices))?;