Repository: bitshifter/glam-rs
Pull request #127 (2021-02-05 12:39:04)

Add Vec::normalize_or_zero()
See discussion in https://github.com/bitshifter/glam-rs/issues/74

This adds the method `normalize_or_zero()` which will either return a unit-length vector or zero, but never a non-finite vector.

On my MacBook Pro 2019 ~I get `4.6 ns` for `Vec3::normalize()` and `8.1 ns` for `Vec3::normalize_or_zero()`.~
**EDIT**: `Vec3::normalize_or_zero()` takes about 40% more time compared to `Vec3::normalize()`.

## Motivation
In many real-world cases, the safe choice is zero when a vector cannot be normalized (regardless of the reason). This PR makes this common use case very ergonomic.

## Alternatives
Instead of normalizing and checking if all values are finite we could take the length, check if it is small or non-finite, and if so return zero. This has the potential of being slightly faster, but the cutoff for "small" must be chosen very carefully.

We could also consider splitting this into two functions: `v.normalize().finite_or_zero()`, i.e. have a special function that returns the zero vector for non-finite vectors. This choice would make it slightly longer than `normalize_or_zero()` and also preclude future optimizations as suggested above. That being said, a `finite_or_zero` might still be a nice separate addition (for another PR).

bitshifter: @emilk I don't know if github notified you but it looks like this PR has a conflict after I merged your other PR.
emilk: With the `length_recip` optimization `normalize()` is only 20% slower than `normalize()`
emilk: Ok, I think this is good to go now finally :)
bitshifter: I can merge this as is although I am beginning to wonder if we should go with testing squared length against some epsilon  and not use `is_finite`. The reason being dividing by zero before testing will trigger floating point exceptions, which I often use on work projects to try track down NaNs (in C++ projects, but `f32` is not radically different in Rust). Because this method will divides by zero and then check the result it would trigger that exception which would be annoying if debugging with float exceptions.
emilk: Well, how the function in implemented can always be changed later. I think what’s more important is to nail down the name and the semantics of it. In particular, I think it is a good idea to never return a non-finite value from `normalize_or_zero`, even when the input is infinite or NaN.

Patch:
diff --git a/benches/vec3.rs b/benches/vec3.rs
index 8b3af95..edbcbc7 100644
--- a/benches/vec3.rs
+++ b/benches/vec3.rs
@@ -97,6 +97,30 @@ op => vec3_into_tuple,
 from => random_vec3
 );
 
+#[inline]
+fn vec3_normalize(v: Vec3) -> Vec3 {
+    v.normalize()
+}
+
+bench_func!(
+    vec3_normalize_bench,
+    "vec3 normalize",
+    op => vec3_normalize,
+    from => random_vec3
+);
+
+#[inline]
+fn vec3_normalize_or_zero(v: Vec3) -> Vec3 {
+    v.normalize_or_zero()
+}
+
+bench_func!(
+    vec3_normalize_or_zero_bench,
+    "vec3 normalize_or_zero",
+    op => vec3_normalize_or_zero,
+    from => random_vec3
+);
+
 euler!(vec3_euler, "vec3 euler", ty => Vec3, storage => Vec3, zero => Vec3::ZERO, rand => random_vec3);
 
 bench_binop!(
@@ -123,6 +147,8 @@ criterion_group!(
     quat_mul_vec3,
     vec3_add_vec3,
     vec3_angle_between,
+    vec3_normalize_bench,
+    vec3_normalize_or_zero_bench,
     vec3_euler,
     vec3_select,
     vec3_to_array_fields,
diff --git a/src/vec.rs b/src/vec.rs
index ca3c866..9ed546f 100644
--- a/src/vec.rs
+++ b/src/vec.rs
@@ -246,12 +246,27 @@ macro_rules! impl_vecn_float_methods {
 
         /// Returns `self` normalized to length 1.0.
         ///
-        /// For valid results, `self` must _not_ be of length zero.
+        /// For valid results, `self` must _not_ be of length zero, nor very close to zero.
+        /// You can check the results with [`Self::is_finite`], or use [`Self::normalize_or_zero`].
         #[inline(always)]
         pub fn normalize(self) -> Self {
             Self($flttrait::normalize(self.0))
         }
 
+        /// Returns `self` normalized to length 1.0 if possible, else returns zero.
+        ///
+        /// In particular, if the input is zero (or very close to zero), or non-finite,
+        /// the result of this operation will be zero.
+        #[inline(always)]
+        pub fn normalize_or_zero(self) -> Self {
+            let rcp = self.length_recip();
+            if rcp.is_finite() && rcp > 0.0 {
+                self * rcp
+            } else {
+                Self::ZERO
+            }
+        }
+
         /// Returns whether `self` is length `1.0` or not.
         ///
         /// Uses a precision threshold of `1e-6`.


Test patch:
diff --git a/tests/support/macros.rs b/tests/support/macros.rs
index 88a3b9b..4f62657 100644
--- a/tests/support/macros.rs
+++ b/tests/support/macros.rs
@@ -30,3 +30,56 @@ macro_rules! assert_approx_eq {
         );
     }};
 }
+
+/// Test vector normalization for float vector
+#[macro_export]
+macro_rules! impl_vec_float_normalize_tests {
+    ($t:ident, $vec:ident) => {
+        use core::$t::MAX;
+        use core::$t::MIN_POSITIVE;
+
+        /// Works for vec2, vec3, vec4
+        fn from_x_y(x: $t, y: $t) -> $vec {
+            let mut v = $vec::ZERO;
+            v.x = x;
+            v.y = y;
+            v
+        }
+
+        #[test]
+        fn test_normalize() {
+            assert_eq!(from_x_y(-42.0, 0.0).normalize(), from_x_y(-1.0, 0.0));
+            assert_eq!(from_x_y(MAX.sqrt(), 0.0).normalize(), from_x_y(1.0, 0.0));
+            // assert_eq!(from_x_y(MAX, 0.0).normalize(), from_x_y(1.0, 0.0)); // normalize fails for huge vectors and returns zero
+
+            // We expect not to be able to normalize small numbers:
+            assert!(!from_x_y(0.0, 0.0).normalize().is_finite());
+            assert!(!from_x_y(MIN_POSITIVE, 0.0).normalize().is_finite());
+
+            // We expect not to be able to normalize non-finite vectors:
+            assert!(!from_x_y(INFINITY, 0.0).normalize().is_finite());
+            assert!(!from_x_y(NAN, 0.0).normalize().is_finite());
+        }
+
+        #[test]
+        fn test_normalize_or_zero() {
+            assert_eq!(from_x_y(-42.0, 0.0).normalize(), from_x_y(-1.0, 0.0));
+            assert_eq!(
+                from_x_y(MAX.sqrt(), 0.0).normalize_or_zero(),
+                from_x_y(1.0, 0.0)
+            );
+
+            // We expect `normalize_or_zero` to return zero when inputs are very small:
+            assert_eq!(from_x_y(0.0, 0.0).normalize_or_zero(), $vec::ZERO);
+            assert_eq!(from_x_y(MIN_POSITIVE, 0.0).normalize_or_zero(), $vec::ZERO);
+
+            // We expect `normalize_or_zero` to return zero when inputs are non-finite:
+            assert_eq!(from_x_y(INFINITY, 0.0).normalize_or_zero(), $vec::ZERO);
+            assert_eq!(from_x_y(NAN, 0.0).normalize_or_zero(), $vec::ZERO);
+
+            // We expect `normalize_or_zero` to return zero when inputs are very large:
+            assert_eq!(from_x_y(MAX, 0.0).normalize_or_zero(), $vec::ZERO);
+            assert_eq!(from_x_y(MAX, MAX).normalize_or_zero(), $vec::ZERO);
+        }
+    };
+}
diff --git a/tests/vec2.rs b/tests/vec2.rs
index dd959ba..cf7963d 100644
--- a/tests/vec2.rs
+++ b/tests/vec2.rs
@@ -406,6 +406,7 @@ macro_rules! impl_vec2_signed_tests {
 macro_rules! impl_vec2_float_tests {
     ($t:ident, $const_new:ident, $new:ident, $vec2:ident, $vec3:ident, $mask:ident, $mat2:ident) => {
         impl_vec2_signed_tests!($t, $const_new, $new, $vec2, $vec3, $mask);
+        impl_vec_float_normalize_tests!($t, $vec2);
 
         use core::$t::INFINITY;
         use core::$t::NAN;
diff --git a/tests/vec3.rs b/tests/vec3.rs
index acc613c..98a82b4 100644
--- a/tests/vec3.rs
+++ b/tests/vec3.rs
@@ -451,6 +451,7 @@ macro_rules! impl_vec3_signed_tests {
 macro_rules! impl_vec3_float_tests {
     ($t:ident, $const_new:ident, $new:ident, $vec3:ident, $mask:ident) => {
         impl_vec3_signed_tests!($t, $const_new, $new, $vec3, $mask);
+        impl_vec_float_normalize_tests!($t, $vec3);
 
         use core::$t::INFINITY;
         use core::$t::NAN;
diff --git a/tests/vec4.rs b/tests/vec4.rs
index d74ab5c..8fc5097 100644
--- a/tests/vec4.rs
+++ b/tests/vec4.rs
@@ -507,6 +507,7 @@ macro_rules! impl_vec4_signed_tests {
 macro_rules! impl_vec4_float_tests {
     ($t:ident, $const_new:ident, $new:ident, $vec4:ident, $mask:ident) => {
         impl_vec4_signed_tests!($t, $const_new, $new, $vec4, $mask);
+        impl_vec_float_normalize_tests!($t, $vec4);
 
         use core::$t::INFINITY;
         use core::$t::NAN;