Merge pull request #465 from hannobraun/tolerance

Add `Tolerance` struct to enforce validity of tolerance values

fj-app/src/args.rs

@@ -1,4 +1,7 @@
-use std::path::PathBuf;
+use std::{path::PathBuf, str::FromStr as _};
+
+use fj_kernel::algorithms::Tolerance;
+use fj_math::Scalar;
 
 /// Fornjot - Experimental CAD System
 #[derive(clap::Parser)]
@@ -16,8 +19,8 @@ pub struct Args {
     pub parameters: Vec<String>,
 
     /// Model deviation tolerance
-    #[clap[short, long]]
-    pub tolerance: Option<f64>,
+    #[clap[short, long, parse(try_from_str = parse_tolerance)]]
+    pub tolerance: Option<Tolerance>,
 }
 
 impl Args {
@@ -29,3 +32,11 @@ impl Args {
         <Self as clap::Parser>::parse()
     }
 }
+
+fn parse_tolerance(input: &str) -> anyhow::Result<Tolerance> {
+    let tolerance = f64::from_str(input)?;
+    let tolerance = Scalar::from_f64(tolerance);
+    let tolerance = Tolerance::from_scalar(tolerance)?;
+
+    Ok(tolerance)
+}

fj-app/src/main.rs

@@ -10,7 +10,7 @@ use std::{collections::HashMap, time::Instant};
 
 use fj_host::Model;
 use fj_interop::{debug::DebugInfo, mesh::Mesh};
-use fj_kernel::algorithms::triangulate;
+use fj_kernel::algorithms::{triangulate, Tolerance};
 use fj_math::{Aabb, Point, Scalar};
 use fj_operations::ToShape as _;
 use futures::executor::block_on;
@@ -78,7 +78,9 @@ fn main() -> anyhow::Result<()> {
         parameters.insert(key, value);
     }
 
-    let shape_processor = ShapeProcessor::new(args.tolerance)?;
+    let shape_processor = ShapeProcessor {
+        tolerance: args.tolerance,
+    };
 
     if let Some(path) = args.export {
         let shape = model.load_once(&parameters)?;
@@ -238,26 +240,10 @@ fn main() -> anyhow::Result<()> {
 }
 
 struct ShapeProcessor {
-    tolerance: Option<Scalar>,
+    tolerance: Option<Tolerance>,
 }
 
 impl ShapeProcessor {
-    fn new(tolerance: Option<f64>) -> anyhow::Result<Self> {
-        if let Some(tolerance) = tolerance {
-            if tolerance <= 0. {
-                anyhow::bail!(
-                    "Invalid user defined model deviation tolerance: {}.\n\
-                    Tolerance must be larger than zero",
-                    tolerance
-                );
-            }
-        }
-
-        let tolerance = tolerance.map(Scalar::from_f64);
-
-        Ok(Self { tolerance })
-    }
-
     fn process(&self, shape: &fj::Shape) -> ProcessedShape {
         let aabb = shape.bounding_volume();
 
@@ -273,11 +259,8 @@ impl ShapeProcessor {
                     }
                 }
 
-                // `tolerance` must not be zero, or we'll run into trouble.
                 let tolerance = min_extent / Scalar::from_f64(1000.);
-                assert!(tolerance > Scalar::ZERO);
-
-                tolerance
+                Tolerance::from_scalar(tolerance).unwrap()
             }
             Some(user_defined_tolerance) => user_defined_tolerance,
         };

fj-kernel/src/algorithms/approximation.rs

@@ -25,7 +25,7 @@ impl FaceApprox {
     ///
     /// `tolerance` defines how far the approximation is allowed to deviate from
     /// the actual face.
-    pub fn new(face: &Face, tolerance: Scalar) -> Self {
+    pub fn new(face: &Face, tolerance: Tolerance) -> Self {
         // Curved faces whose curvature is not fully defined by their edges
         // are not supported yet. For that reason, we can fully ignore `face`'s
         // `surface` field and just pass the edges to `Self::for_edges`.
@@ -88,7 +88,7 @@ impl CycleApprox {
     ///
     /// `tolerance` defines how far the approximation is allowed to deviate from
     /// the actual face.
-    pub fn new(cycle: &Cycle, tolerance: Scalar) -> Self {
+    pub fn new(cycle: &Cycle, tolerance: Tolerance) -> Self {
         let mut points = Vec::new();
 
         for edge in cycle.edges() {
@@ -160,6 +160,61 @@ where
     }
 }
 
+/// A tolerance value
+///
+/// A tolerance value is used during approximation. It defines the maximum
+/// allowed deviation of the approximation from the actual shape.
+///
+/// The `Tolerance` type enforces that the tolerance value is always larger than
+/// zero, which is an attribute that the approximation code relies on.
+///
+/// # Failing [`From`]/[`Into`] implementation
+///
+/// The [`From`]/[`Into`] implementations of tolerance are fallible, which goes
+/// against the explicit mandate of those traits, as stated in their
+/// documentation.
+///
+/// A fallible [`Into`] provides a lot of convenience in test code. Since said
+/// documentation doesn't provide any actual reasoning for this requirement, I'm
+/// feeling free to just ignore it.
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
+pub struct Tolerance(Scalar);
+
+impl Tolerance {
+    /// Construct a `Tolerance` from a [`Scalar`]
+    ///
+    /// Returns an error, if the passed scalar is not larger than zero.
+    pub fn from_scalar(
+        scalar: impl Into<Scalar>,
+    ) -> Result<Self, InvalidTolerance> {
+        let scalar = scalar.into();
+
+        if scalar <= Scalar::ZERO {
+            return Err(InvalidTolerance(scalar));
+        }
+
+        Ok(Self(scalar))
+    }
+
+    /// Return the [`Scalar`] that defines the tolerance
+    pub fn inner(&self) -> Scalar {
+        self.0
+    }
+}
+
+impl<S> From<S> for Tolerance
+where
+    S: Into<Scalar>,
+{
+    fn from(scalar: S) -> Self {
+        Self::from_scalar(scalar).unwrap()
+    }
+}
+
+#[derive(Debug, thiserror::Error)]
+#[error("Invalid tolerance ({0}); must be above zero")]
+pub struct InvalidTolerance(Scalar);
+
 #[cfg(test)]
 mod tests {
     use fj_math::{Point, Scalar};
@@ -171,7 +226,7 @@ mod tests {
         topology::{Face, Vertex},
     };
 
-    use super::{CycleApprox, FaceApprox};
+    use super::{CycleApprox, FaceApprox, Tolerance};
 
     #[test]
     fn approximate_edge() -> anyhow::Result<()> {
@@ -201,7 +256,7 @@ mod tests {
     fn for_face_closed() -> anyhow::Result<()> {
         // Test a closed face, i.e. one that is completely encircled by edges.
 
-        let tolerance = Scalar::ONE;
+        let tolerance = Tolerance::from_scalar(Scalar::ONE).unwrap();
 
         let mut shape = Shape::new();
 

fj-kernel/src/algorithms/mod.rs

@@ -8,7 +8,7 @@ mod sweep;
 mod triangulation;
 
 pub use self::{
-    approximation::{CycleApprox, FaceApprox},
+    approximation::{CycleApprox, FaceApprox, Tolerance},
     sweep::sweep_shape,
     triangulation::triangulate,
 };

fj-kernel/src/algorithms/sweep.rs

@@ -1,6 +1,6 @@
 use std::collections::HashMap;
 
-use fj_math::{Scalar, Transform, Triangle, Vector};
+use fj_math::{Transform, Triangle, Vector};
 
 use crate::{
     geometry::{Surface, SweptCurve},
@@ -8,13 +8,13 @@ use crate::{
     topology::{Cycle, Edge, Face, Vertex},
 };
 
-use super::CycleApprox;
+use super::{CycleApprox, Tolerance};
 
 /// Create a new shape by sweeping an existing one
 pub fn sweep_shape(
     mut source: Shape,
     path: Vector<3>,
-    tolerance: Scalar,
+    tolerance: Tolerance,
     color: [u8; 4],
 ) -> Shape {
     let mut target = Shape::new();
@@ -316,6 +316,7 @@ mod tests {
     use fj_math::{Point, Scalar, Vector};
 
     use crate::{
+        algorithms::Tolerance,
         geometry::{Surface, SweptCurve},
         shape::{Handle, Shape},
         topology::{Cycle, Edge, Face},
@@ -325,12 +326,14 @@ mod tests {
 
     #[test]
     fn sweep() -> anyhow::Result<()> {
+        let tolerance = Tolerance::from_scalar(Scalar::ONE).unwrap();
+
         let sketch = Triangle::new([[0., 0., 0.], [1., 0., 0.], [0., 1., 0.]])?;
 
         let mut swept = sweep_shape(
             sketch.shape,
             Vector::from([0., 0., 1.]),
-            Scalar::from_f64(0.),
+            tolerance,
             [255, 0, 0, 255],
         );
 

fj-kernel/src/algorithms/triangulation/mod.rs

@@ -3,18 +3,18 @@ mod polygon;
 mod ray;
 
 use fj_interop::{debug::DebugInfo, mesh::Mesh};
-use fj_math::{Point, Scalar};
+use fj_math::Point;
 
 use crate::{shape::Shape, topology::Face};
 
 use self::polygon::Polygon;
 
-use super::FaceApprox;
+use super::{FaceApprox, Tolerance};
 
 /// Triangulate a shape
 pub fn triangulate(
     mut shape: Shape,
-    tolerance: Scalar,
+    tolerance: Tolerance,
     debug_info: &mut DebugInfo,
 ) -> Mesh<Point<3>> {
     let mut mesh = Mesh::new();
@@ -83,7 +83,9 @@ mod tests {
     use fj_interop::{debug::DebugInfo, mesh::Mesh};
     use fj_math::{Point, Scalar};
 
-    use crate::{geometry::Surface, shape::Shape, topology::Face};
+    use crate::{
+        algorithms::Tolerance, geometry::Surface, shape::Shape, topology::Face,
+    };
 
     #[test]
     fn simple() -> anyhow::Result<()> {
@@ -143,7 +145,7 @@ mod tests {
     }
 
     fn triangulate(shape: Shape) -> Mesh<Point<3>> {
-        let tolerance = Scalar::ONE;
+        let tolerance = Tolerance::from_scalar(Scalar::ONE).unwrap();
 
         let mut debug_info = DebugInfo::new();
         super::triangulate(shape, tolerance, &mut debug_info)

fj-kernel/src/geometry/curves/circle.rs

@@ -2,6 +2,8 @@ use std::f64::consts::PI;
 
 use fj_math::{Point, Scalar, Transform, Vector};
 
+use crate::algorithms::Tolerance;
+
 /// A circle
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Circle {
@@ -80,7 +82,7 @@ impl Circle {
     ///
     /// `tolerance` specifies how much the approximation is allowed to deviate
     /// from the circle.
-    pub fn approx(&self, tolerance: Scalar, out: &mut Vec<Point<3>>) {
+    pub fn approx(&self, tolerance: Tolerance, out: &mut Vec<Point<3>>) {
         let radius = self.radius.magnitude();
 
         // To approximate the circle, we use a regular polygon for which
@@ -98,12 +100,11 @@ impl Circle {
         }
     }
 
-    fn number_of_vertices(tolerance: Scalar, radius: Scalar) -> u64 {
-        assert!(tolerance > Scalar::ZERO);
-        if tolerance > radius / Scalar::TWO {
+    fn number_of_vertices(tolerance: Tolerance, radius: Scalar) -> u64 {
+        if tolerance.inner() > radius / Scalar::TWO {
             3
         } else {
-            (Scalar::PI / (Scalar::ONE - (tolerance / radius)).acos())
+            (Scalar::PI / (Scalar::ONE - (tolerance.inner() / radius)).acos())
                 .ceil()
                 .into_u64()
         }
@@ -116,6 +117,8 @@ mod tests {
 
     use fj_math::{Point, Scalar, Vector};
 
+    use crate::algorithms::Tolerance;
+
     use super::Circle;
 
     #[test]
@@ -150,7 +153,7 @@ mod tests {
         verify_result(1., 100., 23);
 
         fn verify_result(
-            tolerance: impl Into<Scalar>,
+            tolerance: impl Into<Tolerance>,
             radius: impl Into<Scalar>,
             n: u64,
         ) {
@@ -159,9 +162,9 @@ mod tests {
 
             assert_eq!(n, Circle::number_of_vertices(tolerance, radius));
 
-            assert!(calculate_error(radius, n) <= tolerance);
+            assert!(calculate_error(radius, n) <= tolerance.inner());
             if n > 3 {
-                assert!(calculate_error(radius, n - 1) >= tolerance);
+                assert!(calculate_error(radius, n - 1) >= tolerance.inner());
             }
         }
 

fj-kernel/src/geometry/curves/mod.rs

@@ -1,9 +1,11 @@
 mod circle;
 mod line;
 
+use crate::algorithms::Tolerance;
+
 pub use self::{circle::Circle, line::Line};
 
-use fj_math::{Point, Scalar, Transform, Vector};
+use fj_math::{Point, Transform, Vector};
 
 /// A one-dimensional shape
 ///
@@ -96,7 +98,7 @@ impl Curve {
     /// The `approximate_between` methods of the curves then need to make sure
     /// to only return points in between those vertices, not the vertices
     /// themselves.
-    pub fn approx(&self, tolerance: Scalar, out: &mut Vec<Point<3>>) {
+    pub fn approx(&self, tolerance: Tolerance, out: &mut Vec<Point<3>>) {
         match self {
             Self::Circle(circle) => circle.approx(tolerance, out),
             Self::Line(_) => {}

fj-operations/src/circle.rs

@@ -1,5 +1,6 @@
 use fj_interop::debug::DebugInfo;
 use fj_kernel::{
+    algorithms::Tolerance,
     geometry::Surface,
     shape::Shape,
     topology::{Cycle, Edge, Face},
@@ -9,7 +10,7 @@ use fj_math::{Aabb, Point, Scalar};
 use super::ToShape;
 
 impl ToShape for fj::Circle {
-    fn to_shape(&self, _: Scalar, _: &mut DebugInfo) -> Shape {
+    fn to_shape(&self, _: Tolerance, _: &mut DebugInfo) -> Shape {
         let mut shape = Shape::new();
 
         // Circles have just a single round edge with no vertices. So none need

fj-operations/src/difference_2d.rs

@@ -2,15 +2,20 @@ use std::collections::HashMap;
 
 use fj_interop::debug::DebugInfo;
 use fj_kernel::{
+    algorithms::Tolerance,
     shape::{Handle, Shape},
     topology::{Cycle, Edge, Face, Vertex},
 };
-use fj_math::{Aabb, Scalar};
+use fj_math::Aabb;
 
 use super::ToShape;
 
 impl ToShape for fj::Difference2d {
-    fn to_shape(&self, tolerance: Scalar, debug_info: &mut DebugInfo) -> Shape {
+    fn to_shape(
+        &self,
+        tolerance: Tolerance,
+        debug_info: &mut DebugInfo,
+    ) -> Shape {
         // This method assumes that `b` is fully contained within `a`:
         // https://github.com/hannobraun/Fornjot/issues/92
 

fj-operations/src/group.rs

@@ -2,15 +2,20 @@ use std::collections::HashMap;
 
 use fj_interop::debug::DebugInfo;
 use fj_kernel::{
+    algorithms::Tolerance,
     shape::Shape,
     topology::{Cycle, Edge, Face, Vertex},
 };
-use fj_math::{Aabb, Scalar};
+use fj_math::Aabb;
 
 use super::ToShape;
 
 impl ToShape for fj::Group {
-    fn to_shape(&self, tolerance: Scalar, debug_info: &mut DebugInfo) -> Shape {
+    fn to_shape(
+        &self,
+        tolerance: Tolerance,
+        debug_info: &mut DebugInfo,
+    ) -> Shape {
         let mut shape = Shape::new();
 
         let a = self.a.to_shape(tolerance, debug_info);

fj-operations/src/lib.rs

@@ -14,13 +14,13 @@ mod sweep;
 mod transform;
 
 use fj_interop::debug::DebugInfo;
-use fj_kernel::shape::Shape;
-use fj_math::{Aabb, Scalar};
+use fj_kernel::{algorithms::Tolerance, shape::Shape};
+use fj_math::Aabb;
 
 /// Implemented for all operations from the [`fj`] crate
 pub trait ToShape {
     /// Compute the boundary representation of the shape
-    fn to_shape(&self, tolerance: Scalar, debug: &mut DebugInfo) -> Shape;
+    fn to_shape(&self, tolerance: Tolerance, debug: &mut DebugInfo) -> Shape;
 
     /// Access the axis-aligned bounding box of a shape
     ///
@@ -70,7 +70,7 @@ macro_rules! dispatch {
 
 dispatch! {
     to_shape(
-        tolerance: Scalar,
+        tolerance: Tolerance,
         debug: &mut DebugInfo,
     ) -> Shape;
     bounding_volume() -> Aabb<3>;

fj-operations/src/sketch.rs

@@ -1,15 +1,16 @@
 use fj_interop::debug::DebugInfo;
 use fj_kernel::{
+    algorithms::Tolerance,
     geometry::Surface,
     shape::Shape,
     topology::{Cycle, Edge, Face, Vertex},
 };
-use fj_math::{Aabb, Point, Scalar};
+use fj_math::{Aabb, Point};
 
 use super::ToShape;
 
 impl ToShape for fj::Sketch {
-    fn to_shape(&self, _: Scalar, _: &mut DebugInfo) -> Shape {
+    fn to_shape(&self, _: Tolerance, _: &mut DebugInfo) -> Shape {
         let mut shape = Shape::new();
         let mut vertices = Vec::new();
 

fj-operations/src/sweep.rs

@@ -1,11 +1,18 @@
 use fj_interop::debug::DebugInfo;
-use fj_kernel::{algorithms::sweep_shape, shape::Shape};
-use fj_math::{Aabb, Scalar, Vector};
+use fj_kernel::{
+    algorithms::{sweep_shape, Tolerance},
+    shape::Shape,
+};
+use fj_math::{Aabb, Vector};
 
 use super::ToShape;
 
 impl ToShape for fj::Sweep {
-    fn to_shape(&self, tolerance: Scalar, debug_info: &mut DebugInfo) -> Shape {
+    fn to_shape(
+        &self,
+        tolerance: Tolerance,
+        debug_info: &mut DebugInfo,
+    ) -> Shape {
         sweep_shape(
             self.shape().to_shape(tolerance, debug_info),
             Vector::from([0., 0., self.length()]),

fj-operations/src/transform.rs

@@ -1,12 +1,16 @@
 use fj_interop::debug::DebugInfo;
-use fj_kernel::shape::Shape;
-use fj_math::{Aabb, Scalar, Transform};
+use fj_kernel::{algorithms::Tolerance, shape::Shape};
+use fj_math::{Aabb, Transform};
 use parry3d_f64::math::Isometry;
 
 use super::ToShape;
 
 impl ToShape for fj::Transform {
-    fn to_shape(&self, tolerance: Scalar, debug_info: &mut DebugInfo) -> Shape {
+    fn to_shape(
+        &self,
+        tolerance: Tolerance,
+        debug_info: &mut DebugInfo,
+    ) -> Shape {
         let mut shape = self.shape.to_shape(tolerance, debug_info);
         let transform = transform(self);