Merge pull request #627 from hannobraun/edge

Replace problematic `Edge` constructor with a simpler one

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])))?;
 

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,

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)

crates/fj-kernel/src/topology/edge.rs

@@ -39,42 +39,11 @@ pub struct Edge<const D: usize> {
 
 impl Edge<3> {
     /// Construct an instance of `Edge`
-    ///
-    /// # Implementation Note
-    ///
-    /// This method accepts two `Handle<Vertex>`, 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:
-    /// <https://github.com/hannobraun/Fornjot/issues/399>
     pub fn new(
         curve: Handle<Curve<3>>,
-        vertices: Option<[Handle<Vertex>; 2]>,
+        vertices: Option<[LocalForm<Point<1>, 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 }
     }
 

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))?;