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