Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #16480: Prevent exponential denormalization on Transform::rotate_axis #17604

Closed
wants to merge 1 commit into from
Closed

Fixes #16480: Prevent exponential denormalization on Transform::rotate_axis #17604

wants to merge 1 commit into from

Conversation

Novakasa
Copy link
Contributor

@Novakasa Novakasa commented Jan 29, 2025 •
edited
Loading

Objective

Fixes #16480

When repeatedly using Transform::roatate_axis and feeding a axis that is based on the current Transform::rotation, the floating point errors could accumulate exponentially, resulting in denormalized rotation, which panics.

Solution

Normalize the quaternion in Transform::rotate_axis.

Testing

I confirmed that this fixes the issue reported in #16480.

@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

alice-i-cecile
alice-i-cecile reviewed Jan 29, 2025
View reviewed changes
crates/bevy_transform/src/components/transform.rs Outdated
@@ -314,7 +314,7 @@ impl Transform {
/// If this [`Transform`] has a parent, the `axis` is relative to the rotation of the parent.
#[inline]
pub fn rotate_axis(&mut self, axis: Dir3, angle: f32) {
self.rotate(Quat::from_axis_angle(axis.into(), angle));
self.rotation = ((Quat::from_axis_angle(axis.into(), angle)) * self.rotation).normalize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link the issue here in a comment please; this normalization looks out of place otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Is it fine like this or should I add the full link?

@Novakasa Novakasa force-pushed the rotate-axis-normalize branch from 0f2a51d to a4a1078 Compare January 29, 2025 20:38
@Novakasa Novakasa force-pushed the rotate-axis-normalize branch from a4a1078 to e23fe5f Compare January 29, 2025 20:50
@mockersf mockersf added the A-Math Fundamental domain-agnostic mathematical operations label Jan 29, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Transform Translations, rotations and scales S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 29, 2025
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Jan 30, 2025
@greeble-dev
Copy link
Contributor

greeble-dev commented Feb 2, 2025 •
edited
Loading

TLDR: I think this is a pragmatic and narrowly scoped fix to an issue that particularly affects new users.

There was discord discussion on the broader topic of exploding rotations. The two most popular opinions seemed to be:

  1. The engine should warn if rotations denormalize and suggest common causes and fixes.
  2. The engine should not automatically renormalize in all rotation functions.

But there wasn't any consensus on this particular PR, which only changes a few functions on Transform.

So I'm gonna throw out my take - it's fine, and should be extended to all the Transform::rotate_* functions.

  • These functions are designed for people who struggle with rotation math and won't anticipate denormalization.
  • They're unlikely to be on hot paths - in the bevy repo they're only used in examples.
  • The performance cost is minor on x86/arm64 (1.3x-1.5x). Although other platforms will suffer if they have a software sqrtf.

A specific scenario this fixes is a user who copies the 3d_rotation example (which uses Transform::rotate_y) and changes it to a fixed framerate and smaller rotation. If the engine warns about denormalized rotations then it could trigger as early as 60 seconds. If the warning says "don't do that", the user can reasonably reply "but your example code does that?".

Alternatives:

  • Change the examples to normalize.
  • Automatically normalize in the transform propagate. This will probably be more controversial.
  • Use an incremental normalize based on Dir3::fast_renormalize. This might not fix extreme cases, but it's simple and low cost across all platforms. Could be a follow up to this PR.

@Novakasa Novakasa mentioned this pull request Feb 2, 2025
Merged
@Novakasa Novakasa force-pushed the rotate-axis-normalize branch from e23fe5f to 16bf056 Compare February 2, 2025 11:59
@Novakasa
Copy link
Contributor Author

Novakasa commented Feb 2, 2025 •
edited
Loading

I added the same fix to Transform::rotate_local_axis. I did not add it to the other rotate_ methods for now, because those do not have the risk for exponential accumulation of errors specifically. Don't have a strong opinion on it though.

Also opened an alternative PR, #17646 that instead just adds warnings and docs.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required and removed X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 2, 2025
@alice-i-cecile
Copy link
Member

Ultimately I feel like a solid half of the issue here comes down to using quaternions for 2D. Rotation2d isn't vulnerable to denormalization in the same way, and 3D users will have to confront quaternion denormalization at some point no matter what we do. After listening to the debate, I'm not convinced we should merge this, but I would merge documentation warning about this.

github-merge-queue bot pushed a commit that referenced this pull request Feb 3, 2025
@Novakasa
Warnings and docs for exponential denormalization in rotate functions…
bdf60d6 
… (alternative to #17604) (#17646)

# Objective

- When obtaining an axis from the transform and putting that into
`Transform::rotate_axis` or `Transform::rotate_axis_local`, floating
point errors could accumulate exponentially, resulting in denormalized
rotation.
- This is an alternative to and closes #17604, due to lack of consent
around this in the [discord
discussion](https://discord.com/channels/691052431525675048/1203087353850364004/1334232710658392227)
- Closes #16480 

## Solution

- Add a warning of this issue and a recommendation to normalize to the
API docs.
- Add a runtime warning that checks for denormalized axis in debug mode,
with a reference to the API docs.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@Novakasa @mrchantey
Warnings and docs for exponential denormalization in rotate functions…
a28832b 
… (alternative to bevyengine#17604) (bevyengine#17646)

# Objective

- When obtaining an axis from the transform and putting that into
`Transform::rotate_axis` or `Transform::rotate_axis_local`, floating
point errors could accumulate exponentially, resulting in denormalized
rotation.
- This is an alternative to and closes bevyengine#17604, due to lack of consent
around this in the [discord
discussion](https://discord.com/channels/691052431525675048/1203087353850364004/1334232710658392227)
- Closes bevyengine#16480 

## Solution

- Add a warning of this issue and a recommendation to normalize to the
API docs.
- Add a runtime warning that checks for denormalized axis in debug mode,
with a reference to the API docs.
joshua-holmes pushed a commit to joshua-holmes/bevy that referenced this pull request Feb 5, 2025
@Novakasa @joshua-holmes
Warnings and docs for exponential denormalization in rotate functions…
b082283 
… (alternative to bevyengine#17604) (bevyengine#17646)

# Objective

- When obtaining an axis from the transform and putting that into
`Transform::rotate_axis` or `Transform::rotate_axis_local`, floating
point errors could accumulate exponentially, resulting in denormalized
rotation.
- This is an alternative to and closes bevyengine#17604, due to lack of consent
around this in the [discord
discussion](https://discord.com/channels/691052431525675048/1203087353850364004/1334232710658392227)
- Closes bevyengine#16480 

## Solution

- Add a warning of this issue and a recommendation to normalize to the
API docs.
- Add a runtime warning that checks for denormalized axis in debug mode,
with a reference to the API docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloucks aloucks aloucks left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Carter0 Carter0 Carter0 approved these changes

@IQuick143 IQuick143 Awaiting requested review from IQuick143

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Transform Translations, rotations and scales C-Bug An unexpected or incorrect behavior S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transform::rotation denormalizes after applying successive rotations.
6 participants
@Novakasa @greeble-dev @alice-i-cecile @aloucks @Carter0 @mockersf