Skip to content

Commit

Permalink
Improve error messages for denormalized directions (#12278)
Browse files Browse the repository at this point in the history
# Objective

`Dir3` and `Dir3A` can be rotated using `Quat`s. However, if enough
floating point error accumulates or (more commonly) the rotation itself
is degenerate (like not normalized), the resulting direction can also
become denormalized.

Currently, with debug assertions enabled, it panics in these cases with
the message `rotated.is_normalized()`. This error message is unclear,
doesn't give information about *how* it is denormalized (like is the
length too large, NaN, or something else), and is overall not very
helpful. Panicking for small-ish error might also be a bit too strict,
and has lead to unwanted crashes in crates like `bevy_xpbd` (although it
has also helped in finding actual bugs).

The error message should be clearer and give more context, and it
shouldn't cause unwanted crashes.

## Solution

Change the `debug_assert!` to a warning for small error with a (squared
length) threshold of 2e-4 and a panic for clear error with a threshold
of 2e-2. The warnings mention the direction type and the length of the
denormalized vector.

Here's what the error and warning look like:

```
Error: `Dir3` is denormalized after rotation. The length is 1.014242.
```

```
Warning: `Dir3A` is denormalized after rotation. The length is 1.0001414.
```

I gave the same treatment to `new_unchecked`:

```
Error: The vector given to `Dir3::new_unchecked` is not normalized. The length is 1.014242.
```

```
Warning: The vector given to `Dir3A::new_unchecked` is not normalized. The length is 1.0001414.
```

---

## Discussion

### Threshold values

The thresholds are somewhat arbitrary. 2e-4 is what Glam uses for the
squared length in `is_normalized` (after I corrected it in
bitshifter/glam-rs#480), and 2e-2 is just what I thought could be a
clear sign of something being critically wrong. I can definitely tune
them if there are better thresholds though.

### Logging

`bevy_math` doesn't have `bevy_log`, so we can't use `warn!` or
`error!`. This is why I made it use just `eprintln!` and `panic!` for
now. Let me know if there's a better way of logging errors in
`bevy_math`.
  • Loading branch information
Jondolf authored Mar 4, 2024
1 parent 14c20a6 commit 983da76
Showing 1 changed file with 54 additions and 14 deletions.
68 changes: 54 additions & 14 deletions crates/bevy_math/src/direction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,30 @@ impl std::fmt::Display for InvalidDirectionError {
}
}

/// Checks that a vector with the given squared length is normalized.
///
/// Warns for small error with a length threshold of approximately `1e-4`,
/// and panics for large error with a length threshold of approximately `1e-2`.
///
/// The format used for the logged warning is `"Warning: {warning} The length is {length}`,
/// and similarly for the error.
#[cfg(debug_assertions)]
fn assert_is_normalized(message: &str, length_squared: f32) {
let length_error_squared = (length_squared - 1.0).abs();

// Panic for large error and warn for slight error.
if length_error_squared > 2e-2 || length_error_squared.is_nan() {
// Length error is approximately 1e-2 or more.
panic!("Error: {message} The length is {}.", length_squared.sqrt());
} else if length_error_squared > 2e-4 {
// Length error is approximately 1e-4 or more.
eprintln!(
"Warning: {message} The length is {}.",
length_squared.sqrt()
);
}
}

/// A normalized vector pointing in a direction in 2D space
#[deprecated(
since = "0.14.0",
Expand Down Expand Up @@ -81,9 +105,13 @@ impl Dir2 {
///
/// # Warning
///
/// `value` must be normalized, i.e it's length must be `1.0`.
/// `value` must be normalized, i.e its length must be `1.0`.
pub fn new_unchecked(value: Vec2) -> Self {
debug_assert!(value.is_normalized());
#[cfg(debug_assertions)]
assert_is_normalized(
"The vector given to `Dir2::new_unchecked` is not normalized.",
value.length_squared(),
);

Self(value)
}
Expand Down Expand Up @@ -211,9 +239,13 @@ impl Dir3 {
///
/// # Warning
///
/// `value` must be normalized, i.e it's length must be `1.0`.
/// `value` must be normalized, i.e its length must be `1.0`.
pub fn new_unchecked(value: Vec3) -> Self {
debug_assert!(value.is_normalized());
#[cfg(debug_assertions)]
assert_is_normalized(
"The vector given to `Dir3::new_unchecked` is not normalized.",
value.length_squared(),
);

Self(value)
}
Expand Down Expand Up @@ -289,11 +321,13 @@ impl std::ops::Mul<Dir3> for Quat {
fn mul(self, direction: Dir3) -> Self::Output {
let rotated = self * *direction;

// Make sure the result is normalized.
// This can fail for non-unit quaternions.
debug_assert!(rotated.is_normalized());
#[cfg(debug_assertions)]
assert_is_normalized(
"`Dir3` is denormalized after rotation.",
rotated.length_squared(),
);

Dir3::new_unchecked(rotated)
Dir3(rotated)
}
}

Expand Down Expand Up @@ -365,9 +399,13 @@ impl Dir3A {
///
/// # Warning
///
/// `value` must be normalized, i.e it's length must be `1.0`.
/// `value` must be normalized, i.e its length must be `1.0`.
pub fn new_unchecked(value: Vec3A) -> Self {
debug_assert!(value.is_normalized());
#[cfg(debug_assertions)]
assert_is_normalized(
"The vector given to `Dir3A::new_unchecked` is not normalized.",
value.length_squared(),
);

Self(value)
}
Expand Down Expand Up @@ -443,11 +481,13 @@ impl std::ops::Mul<Dir3A> for Quat {
fn mul(self, direction: Dir3A) -> Self::Output {
let rotated = self * *direction;

// Make sure the result is normalized.
// This can fail for non-unit quaternions.
debug_assert!(rotated.is_normalized());
#[cfg(debug_assertions)]
assert_is_normalized(
"`Dir3A` is denormalized after rotation.",
rotated.length_squared(),
);

Dir3A::new_unchecked(rotated)
Dir3A(rotated)
}
}

Expand Down

0 comments on commit 983da76

Please sign in to comment.