Improve error messages for denormalized directions #12278
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
the
C-Usability
Jondolf
commented
Mar 3, 2024
| /// 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) { |
There was a problem hiding this comment.
This feels like it could be a macro, but that might be overkill. It could be nice to have different messages for the warning and error though?
There was a problem hiding this comment.
Yeah, let's avoid adding macros unless we really need to.
alice-i-cecile
approved these changes
Mar 3, 2024
james7132
approved these changes
Mar 3, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
spectria-limina
pushed a commit
to spectria-limina/bevy
that referenced
this pull request
Mar 9, 2024
# 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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Dir3andDir3Acan be rotated usingQuats. 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 likebevy_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:
I gave the same treatment to
new_unchecked: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_mathdoesn't havebevy_log, so we can't usewarn!orerror!. This is why I made it use justeprintln!andpanic!for now. Let me know if there's a better way of logging errors inbevy_math.