From 9814630882da45500374fe982e0d25402668fd1d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 16:46:17 +0200 Subject: [PATCH 1/9] Add `Tolerance` Use this type instead of `Scalar` everywhere a tolerance value is needed. The next step is to enforce that tolerance values are always larger than zero. --- fj-app/src/main.rs | 9 ++--- fj-kernel/src/algorithms/approximation.rs | 36 ++++++++++++++++--- fj-kernel/src/algorithms/mod.rs | 2 +- fj-kernel/src/algorithms/sweep.rs | 9 ++--- fj-kernel/src/algorithms/triangulation/mod.rs | 10 +++--- fj-kernel/src/geometry/curves/circle.rs | 20 ++++++----- fj-kernel/src/geometry/curves/mod.rs | 6 ++-- fj-operations/src/circle.rs | 3 +- fj-operations/src/difference_2d.rs | 9 +++-- fj-operations/src/group.rs | 9 +++-- fj-operations/src/lib.rs | 8 ++--- fj-operations/src/sketch.rs | 5 +-- fj-operations/src/sweep.rs | 13 +++++-- fj-operations/src/transform.rs | 10 ++++-- 14 files changed, 104 insertions(+), 45 deletions(-) diff --git a/fj-app/src/main.rs b/fj-app/src/main.rs index 37f81303d..7d16e1a95 100644 --- a/fj-app/src/main.rs +++ b/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; @@ -238,7 +238,7 @@ fn main() -> anyhow::Result<()> { } struct ShapeProcessor { - tolerance: Option, + tolerance: Option, } impl ShapeProcessor { @@ -253,7 +253,8 @@ impl ShapeProcessor { } } - let tolerance = tolerance.map(Scalar::from_f64); + let tolerance = + tolerance.map(Scalar::from_f64).map(Tolerance::from_scalar); Ok(Self { tolerance }) } @@ -277,7 +278,7 @@ impl ShapeProcessor { let tolerance = min_extent / Scalar::from_f64(1000.); assert!(tolerance > Scalar::ZERO); - tolerance + Tolerance::from_scalar(tolerance) } Some(user_defined_tolerance) => user_defined_tolerance, }; diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index db0eea924..ffc39fce3 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/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,34 @@ where } } +/// A tolerance value +/// +/// A tolerance value is used during approximation. It defines the maximum +/// allowed deviation of the approximation from the actual shape. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub struct Tolerance(Scalar); + +impl Tolerance { + /// Construct a `Tolerance` from a [`Scalar`] + pub fn from_scalar(scalar: impl Into) -> Self { + Self(scalar.into()) + } + + /// Return the [`Scalar`] that defines the tolerance + pub fn inner(&self) -> Scalar { + self.0 + } +} + +impl From for Tolerance +where + S: Into, +{ + fn from(scalar: S) -> Self { + Self::from_scalar(scalar) + } +} + #[cfg(test)] mod tests { use fj_math::{Point, Scalar}; @@ -171,7 +199,7 @@ mod tests { topology::{Face, Vertex}, }; - use super::{CycleApprox, FaceApprox}; + use super::{CycleApprox, FaceApprox, Tolerance}; #[test] fn approximate_edge() -> anyhow::Result<()> { @@ -201,7 +229,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); let mut shape = Shape::new(); diff --git a/fj-kernel/src/algorithms/mod.rs b/fj-kernel/src/algorithms/mod.rs index 7c7ce5681..9e78b3501 100644 --- a/fj-kernel/src/algorithms/mod.rs +++ b/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, }; diff --git a/fj-kernel/src/algorithms/sweep.rs b/fj-kernel/src/algorithms/sweep.rs index 87279d4b1..10d1dbbbe 100644 --- a/fj-kernel/src/algorithms/sweep.rs +++ b/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}, @@ -330,7 +331,7 @@ mod tests { let mut swept = sweep_shape( sketch.shape, Vector::from([0., 0., 1.]), - Scalar::from_f64(0.), + Tolerance::from_scalar(Scalar::from_f64(0.)), [255, 0, 0, 255], ); diff --git a/fj-kernel/src/algorithms/triangulation/mod.rs b/fj-kernel/src/algorithms/triangulation/mod.rs index b388561c0..c6274941f 100644 --- a/fj-kernel/src/algorithms/triangulation/mod.rs +++ b/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> { let mut mesh = Mesh::new(); @@ -83,7 +83,7 @@ mod tests { use fj_interop::{debug::DebugInfo, mesh::Mesh}; use fj_math::{Point, Scalar}; - use crate::{geometry::Surface, shape::Shape, topology::Face}; + use crate::{geometry::Surface, shape::Shape, topology::Face, algorithms::Tolerance}; #[test] fn simple() -> anyhow::Result<()> { @@ -143,7 +143,7 @@ mod tests { } fn triangulate(shape: Shape) -> Mesh> { - let tolerance = Scalar::ONE; + let tolerance = Tolerance::from_scalar(Scalar::ONE); let mut debug_info = DebugInfo::new(); super::triangulate(shape, tolerance, &mut debug_info) diff --git a/fj-kernel/src/geometry/curves/circle.rs b/fj-kernel/src/geometry/curves/circle.rs index e2596e091..e65b29726 100644 --- a/fj-kernel/src/geometry/curves/circle.rs +++ b/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>) { + pub fn approx(&self, tolerance: Tolerance, out: &mut Vec>) { let radius = self.radius.magnitude(); // To approximate the circle, we use a regular polygon for which @@ -98,12 +100,12 @@ 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 { + assert!(tolerance.inner() > Scalar::ZERO); + 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 +118,8 @@ mod tests { use fj_math::{Point, Scalar, Vector}; + use crate::algorithms::Tolerance; + use super::Circle; #[test] @@ -150,7 +154,7 @@ mod tests { verify_result(1., 100., 23); fn verify_result( - tolerance: impl Into, + tolerance: impl Into, radius: impl Into, n: u64, ) { @@ -159,9 +163,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()); } } diff --git a/fj-kernel/src/geometry/curves/mod.rs b/fj-kernel/src/geometry/curves/mod.rs index 92344cbda..b26bc0a19 100644 --- a/fj-kernel/src/geometry/curves/mod.rs +++ b/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>) { + pub fn approx(&self, tolerance: Tolerance, out: &mut Vec>) { match self { Self::Circle(circle) => circle.approx(tolerance, out), Self::Line(_) => {} diff --git a/fj-operations/src/circle.rs b/fj-operations/src/circle.rs index 3c9671e9a..b4e84c850 100644 --- a/fj-operations/src/circle.rs +++ b/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 diff --git a/fj-operations/src/difference_2d.rs b/fj-operations/src/difference_2d.rs index 94d06f87c..068753f7b 100644 --- a/fj-operations/src/difference_2d.rs +++ b/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 diff --git a/fj-operations/src/group.rs b/fj-operations/src/group.rs index 6e139af29..7a648a360 100644 --- a/fj-operations/src/group.rs +++ b/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); diff --git a/fj-operations/src/lib.rs b/fj-operations/src/lib.rs index aef0c61bb..f1b51cb95 100644 --- a/fj-operations/src/lib.rs +++ b/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>; diff --git a/fj-operations/src/sketch.rs b/fj-operations/src/sketch.rs index 557fef23d..0d3823764 100644 --- a/fj-operations/src/sketch.rs +++ b/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(); diff --git a/fj-operations/src/sweep.rs b/fj-operations/src/sweep.rs index 898a02c7f..ecafc61b7 100644 --- a/fj-operations/src/sweep.rs +++ b/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()]), diff --git a/fj-operations/src/transform.rs b/fj-operations/src/transform.rs index f6602ea4e..2db6e35cd 100644 --- a/fj-operations/src/transform.rs +++ b/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); From 60b91201703da25f51617dedc97f5ea6436a3aca Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 16:55:36 +0200 Subject: [PATCH 2/9] Refactor --- fj-kernel/src/algorithms/sweep.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fj-kernel/src/algorithms/sweep.rs b/fj-kernel/src/algorithms/sweep.rs index 10d1dbbbe..2e98bbabe 100644 --- a/fj-kernel/src/algorithms/sweep.rs +++ b/fj-kernel/src/algorithms/sweep.rs @@ -326,12 +326,14 @@ mod tests { #[test] fn sweep() -> anyhow::Result<()> { + let tolerance = Tolerance::from_scalar(Scalar::from_f64(0.)); + 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.]), - Tolerance::from_scalar(Scalar::from_f64(0.)), + tolerance, [255, 0, 0, 255], ); From 93f20f4f99fc6121bc3983fc5f8ca58032a421e3 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 16:55:52 +0200 Subject: [PATCH 3/9] Don't use zero tolerance in test It doesn't make any difference for this specific test, but a tolerance of zero is invalid and will be disallowed soon. --- fj-kernel/src/algorithms/sweep.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fj-kernel/src/algorithms/sweep.rs b/fj-kernel/src/algorithms/sweep.rs index 2e98bbabe..f564f956b 100644 --- a/fj-kernel/src/algorithms/sweep.rs +++ b/fj-kernel/src/algorithms/sweep.rs @@ -326,7 +326,7 @@ mod tests { #[test] fn sweep() -> anyhow::Result<()> { - let tolerance = Tolerance::from_scalar(Scalar::from_f64(0.)); + let tolerance = Tolerance::from_scalar(Scalar::ONE); let sketch = Triangle::new([[0., 0., 0.], [1., 0., 0.], [0., 1., 0.]])?; From d84990c87ff8e6d052ca00ba2624a31d31978cb4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 16:58:19 +0200 Subject: [PATCH 4/9] Enforce that tolerance is always above zero --- fj-app/src/main.rs | 8 +++++--- fj-kernel/src/algorithms/approximation.rs | 14 ++++++++++---- fj-kernel/src/algorithms/sweep.rs | 2 +- fj-kernel/src/algorithms/triangulation/mod.rs | 6 ++++-- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/fj-app/src/main.rs b/fj-app/src/main.rs index 7d16e1a95..16544410e 100644 --- a/fj-app/src/main.rs +++ b/fj-app/src/main.rs @@ -253,8 +253,10 @@ impl ShapeProcessor { } } - let tolerance = - tolerance.map(Scalar::from_f64).map(Tolerance::from_scalar); + let tolerance = tolerance + .map(Scalar::from_f64) + .map(Tolerance::from_scalar) + .map(|result| result.unwrap()); Ok(Self { tolerance }) } @@ -278,7 +280,7 @@ impl ShapeProcessor { let tolerance = min_extent / Scalar::from_f64(1000.); assert!(tolerance > Scalar::ZERO); - Tolerance::from_scalar(tolerance) + Tolerance::from_scalar(tolerance).unwrap() } Some(user_defined_tolerance) => user_defined_tolerance, }; diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index ffc39fce3..86aa13afb 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -169,8 +169,14 @@ pub struct Tolerance(Scalar); impl Tolerance { /// Construct a `Tolerance` from a [`Scalar`] - pub fn from_scalar(scalar: impl Into) -> Self { - Self(scalar.into()) + pub fn from_scalar(scalar: impl Into) -> Result { + let scalar = scalar.into(); + + if scalar <= Scalar::ZERO { + return Err(scalar); + } + + Ok(Self(scalar)) } /// Return the [`Scalar`] that defines the tolerance @@ -184,7 +190,7 @@ where S: Into, { fn from(scalar: S) -> Self { - Self::from_scalar(scalar) + Self::from_scalar(scalar).unwrap() } } @@ -229,7 +235,7 @@ mod tests { fn for_face_closed() -> anyhow::Result<()> { // Test a closed face, i.e. one that is completely encircled by edges. - let tolerance = Tolerance::from_scalar(Scalar::ONE); + let tolerance = Tolerance::from_scalar(Scalar::ONE).unwrap(); let mut shape = Shape::new(); diff --git a/fj-kernel/src/algorithms/sweep.rs b/fj-kernel/src/algorithms/sweep.rs index f564f956b..9c830e21f 100644 --- a/fj-kernel/src/algorithms/sweep.rs +++ b/fj-kernel/src/algorithms/sweep.rs @@ -326,7 +326,7 @@ mod tests { #[test] fn sweep() -> anyhow::Result<()> { - let tolerance = Tolerance::from_scalar(Scalar::ONE); + let tolerance = Tolerance::from_scalar(Scalar::ONE).unwrap(); let sketch = Triangle::new([[0., 0., 0.], [1., 0., 0.], [0., 1., 0.]])?; diff --git a/fj-kernel/src/algorithms/triangulation/mod.rs b/fj-kernel/src/algorithms/triangulation/mod.rs index c6274941f..d07436cf7 100644 --- a/fj-kernel/src/algorithms/triangulation/mod.rs +++ b/fj-kernel/src/algorithms/triangulation/mod.rs @@ -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, algorithms::Tolerance}; + 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> { - let tolerance = Tolerance::from_scalar(Scalar::ONE); + let tolerance = Tolerance::from_scalar(Scalar::ONE).unwrap(); let mut debug_info = DebugInfo::new(); super::triangulate(shape, tolerance, &mut debug_info) From 6638751dab3ab893d15ad92254667c9ad7fdbc6a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 16:58:53 +0200 Subject: [PATCH 5/9] Update documentation of `Tolerance` --- fj-kernel/src/algorithms/approximation.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 86aa13afb..624f47d9a 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -164,11 +164,26 @@ where /// /// 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) -> Result { let scalar = scalar.into(); From 518d51387d82788b52c78d5431a95d3c8a608d95 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 17:00:10 +0200 Subject: [PATCH 6/9] Remove redundant `assert!`s --- fj-app/src/main.rs | 3 --- fj-kernel/src/geometry/curves/circle.rs | 1 - 2 files changed, 4 deletions(-) diff --git a/fj-app/src/main.rs b/fj-app/src/main.rs index 16544410e..ea1e832f4 100644 --- a/fj-app/src/main.rs +++ b/fj-app/src/main.rs @@ -276,10 +276,7 @@ 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::from_scalar(tolerance).unwrap() } Some(user_defined_tolerance) => user_defined_tolerance, diff --git a/fj-kernel/src/geometry/curves/circle.rs b/fj-kernel/src/geometry/curves/circle.rs index e65b29726..764781aeb 100644 --- a/fj-kernel/src/geometry/curves/circle.rs +++ b/fj-kernel/src/geometry/curves/circle.rs @@ -101,7 +101,6 @@ impl Circle { } fn number_of_vertices(tolerance: Tolerance, radius: Scalar) -> u64 { - assert!(tolerance.inner() > Scalar::ZERO); if tolerance.inner() > radius / Scalar::TWO { 3 } else { From 5f7ba6ae833355f2a37b61adfeb8e2e9baa42571 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 17:06:30 +0200 Subject: [PATCH 7/9] Improve error handling of `Tolerance` --- fj-kernel/src/algorithms/approximation.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 624f47d9a..29a467bf3 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -184,11 +184,13 @@ 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) -> Result { + pub fn from_scalar( + scalar: impl Into, + ) -> Result { let scalar = scalar.into(); if scalar <= Scalar::ZERO { - return Err(scalar); + return Err(InvalidTolerance(scalar)); } Ok(Self(scalar)) @@ -209,6 +211,10 @@ where } } +#[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}; From d96a2bf16bd7ec0bb1e3c38a993034c0b7371a2d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 17:09:07 +0200 Subject: [PATCH 8/9] Use `Tolerance` in `Args` --- fj-app/src/args.rs | 17 ++++++++++++++--- fj-app/src/main.rs | 17 +---------------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/fj-app/src/args.rs b/fj-app/src/args.rs index f47d14120..16cada701 100644 --- a/fj-app/src/args.rs +++ b/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, /// Model deviation tolerance - #[clap[short, long]] - pub tolerance: Option, + #[clap[short, long, parse(try_from_str = parse_tolerance)]] + pub tolerance: Option, } impl Args { @@ -29,3 +32,11 @@ impl Args { ::parse() } } + +fn parse_tolerance(input: &str) -> anyhow::Result { + let tolerance = f64::from_str(input)?; + let tolerance = Scalar::from_f64(tolerance); + let tolerance = Tolerance::from_scalar(tolerance)?; + + Ok(tolerance) +} diff --git a/fj-app/src/main.rs b/fj-app/src/main.rs index ea1e832f4..91008970e 100644 --- a/fj-app/src/main.rs +++ b/fj-app/src/main.rs @@ -242,22 +242,7 @@ struct ShapeProcessor { } impl ShapeProcessor { - fn new(tolerance: Option) -> anyhow::Result { - 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) - .map(Tolerance::from_scalar) - .map(|result| result.unwrap()); - + fn new(tolerance: Option) -> anyhow::Result { Ok(Self { tolerance }) } From c55649ead205f00bf739acc907f698b09274885c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 12 Apr 2022 17:09:53 +0200 Subject: [PATCH 9/9] Remove redundant constructor --- fj-app/src/main.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/fj-app/src/main.rs b/fj-app/src/main.rs index 91008970e..6589f1553 100644 --- a/fj-app/src/main.rs +++ b/fj-app/src/main.rs @@ -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(¶meters)?; @@ -242,10 +244,6 @@ struct ShapeProcessor { } impl ShapeProcessor { - fn new(tolerance: Option) -> anyhow::Result { - Ok(Self { tolerance }) - } - fn process(&self, shape: &fj::Shape) -> ProcessedShape { let aabb = shape.bounding_volume();