From 3a75c9ef92dbc3736de5442036e0d4d46eea2e7b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Sat, 19 Feb 2022 10:37:26 +0100 Subject: [PATCH 1/4] Check that scalar is finite on construction --- src/math/scalar.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/math/scalar.rs b/src/math/scalar.rs index f350cf990..72a1df265 100644 --- a/src/math/scalar.rs +++ b/src/math/scalar.rs @@ -24,7 +24,12 @@ impl Scalar { /// Construct a `Scalar` from an `f64` pub fn from_f64(scalar: f64) -> Self { - Self(scalar) + if scalar.is_finite() { + // `scalar` is neither infinite, nor NaN + Self(scalar) + } else { + panic!("Invalid scalar value: {scalar}"); + } } /// Construct a `Scalar` from a `u64` From 8f3d6f21781a1fafff41eadd3f99c7668f2635b6 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Sat, 19 Feb 2022 11:03:26 +0100 Subject: [PATCH 2/4] Implement `Eq`, `Ord`, and `Hash` for `Scalar` These implementations aren't available for `f64` and other floating-point values. Providing them is the whole point of why `Scalar` exists. --- src/math/scalar.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/math/scalar.rs b/src/math/scalar.rs index 72a1df265..fecf8bf5b 100644 --- a/src/math/scalar.rs +++ b/src/math/scalar.rs @@ -1,4 +1,4 @@ -use std::{cmp, f64::consts::PI, ops}; +use std::{cmp, f64::consts::PI, hash::Hash, ops}; use approx::AbsDiffEq; @@ -84,6 +84,22 @@ impl Scalar { } } +impl Eq for Scalar {} + +impl Ord for Scalar { + fn cmp(&self, other: &Self) -> cmp::Ordering { + // Should never panic, as `from_f64` checks that the wrapped value is + // finite. + self.partial_cmp(&other).unwrap() + } +} + +impl Hash for Scalar { + fn hash(&self, state: &mut H) { + self.0.to_bits().hash(state); + } +} + impl From for Scalar { fn from(scalar: f32) -> Self { Self::from_f64(scalar as f64) From a35eb3964a5e958283cb7b2c8745a65ca8839064 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Sat, 19 Feb 2022 11:05:10 +0100 Subject: [PATCH 3/4] Update documentation of `Scalar` --- src/math/scalar.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/math/scalar.rs b/src/math/scalar.rs index fecf8bf5b..8b7b2507d 100644 --- a/src/math/scalar.rs +++ b/src/math/scalar.rs @@ -2,7 +2,30 @@ use std::{cmp, f64::consts::PI, hash::Hash, ops}; use approx::AbsDiffEq; -/// A scalar +/// A rational, finite scalar value +/// +/// This is a wrapper around `f64`. On construction, it checks that the `f64` +/// value is neither infinite nor NaN. This allows `Scalar` to provide +/// implementations of [`Eq`], [`Ord`], and [`Hash`], enabling `Scalar` (and +/// types built on top of it), to be used as keys in hash maps, hash sets, and +/// similar types. +/// +/// # Failing `From`/`Into` implementations +/// +/// Please note that the [`From`]/[`Into`] implementation that convert floating +/// point numbers into `Scalar` can panic. These conversions call +/// [`Scalar::from_f64`] internally and panic under the same conditions. +/// +/// This explicitly goes against the mandate of [`From`]/[`Into`], whose +/// documentation mandate that implementations must not fail. This is a +/// deliberate design decision. The intended use case of `Scalar` is math code +/// that considers non-finite floating point values a bug, not a recoverable +/// error. +/// +/// For this use case, having easy conversions available is an advantage, and +/// explicit `unwrap`/`expect` calls would add nothing. In addition, the mandate +/// not to fail is not motivated in any way, in the [`From`]/[`Into`] +/// documentation. #[derive(Clone, Copy, Debug, PartialEq)] pub struct Scalar(f64); @@ -23,6 +46,8 @@ impl Scalar { pub const PI: Self = Self(PI); /// Construct a `Scalar` from an `f64` + /// + /// Panics, if `scalar` is infinite or NaN. pub fn from_f64(scalar: f64) -> Self { if scalar.is_finite() { // `scalar` is neither infinite, nor NaN From 991edf98377482859e5dbe732182a40528e97743 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Sat, 19 Feb 2022 11:07:15 +0100 Subject: [PATCH 4/4] Derive same set of traits for most math types This set includes `Eq`, `Ord`, and `Hash`, which is made possible by the `Scalar` type. --- src/math/aabb.rs | 1 + src/math/point.rs | 2 +- src/math/segment.rs | 2 +- src/math/triangle.rs | 2 +- src/math/vector.rs | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/math/aabb.rs b/src/math/aabb.rs index 80798a758..db1ba105f 100644 --- a/src/math/aabb.rs +++ b/src/math/aabb.rs @@ -3,6 +3,7 @@ use parry3d_f64::bounding_volume::BoundingVolume as _; use super::{Point, Vector}; /// An axis-aligned bounding box (AABB) +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct Aabb { /// The minimum coordinates of the AABB pub min: Point, diff --git a/src/math/point.rs b/src/math/point.rs index 839159753..1b0571241 100644 --- a/src/math/point.rs +++ b/src/math/point.rs @@ -12,7 +12,7 @@ use super::{Scalar, Vector}; /// /// The goal of this type is to eventually implement `Eq` and `Hash`, making it /// easier to work with vectors. This is a work in progress. -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct Point([Scalar; D]); impl Point { diff --git a/src/math/segment.rs b/src/math/segment.rs index 11d3ae273..6b0937841 100644 --- a/src/math/segment.rs +++ b/src/math/segment.rs @@ -1,7 +1,7 @@ use super::Point; /// A line segment, defined by its two end points -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct Segment { pub a: Point, pub b: Point, diff --git a/src/math/triangle.rs b/src/math/triangle.rs index cff5e9739..d1f230da4 100644 --- a/src/math/triangle.rs +++ b/src/math/triangle.rs @@ -1,7 +1,7 @@ use super::Point; /// A triangle -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct Triangle { pub a: Point<3>, pub b: Point<3>, diff --git a/src/math/vector.rs b/src/math/vector.rs index d122da117..26d78cc4a 100644 --- a/src/math/vector.rs +++ b/src/math/vector.rs @@ -12,7 +12,7 @@ use super::Scalar; /// /// The goal of this type is to eventually implement `Eq` and `Hash`, making it /// easier to work with vectors. This is a work in progress. -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct Vector([Scalar; D]); impl Vector {