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.

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

Add Transform::is_finite #10592

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Conversation

johanhelsing
Copy link
Contributor

Objective

  • Sometimes it's very useful to know if a Transform contains any NaN or infinite values. It's a bit boiler-plate heavy to check translation, rotation and scale individually.

Solution

  • Add a new method is_finite that returns true if, and only if translation, rotation and scale all are finite.
  • It's a natural extension of Quat::is_finite, and Vec3::is_finite, which return true if, and only if all their components' is_finite() returns true.

Changelog

  • Added Transform::is_finite

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales labels Nov 16, 2023
@johanhelsing
Copy link
Contributor Author

I wanted this when implementing a checksum function for Transforms with bevy_ggrs: https://johanhelsing.studio/posts/extreme-bevy-desync-detection#back-to-synctest-sessions

    let mut hasher = FixedState::build_hasher();

    assert!(
        transform.translation.is_finite() && transform.rotation.is_finite(),
        "Hashing is not stable for NaN f32 values."
    );

    transform.translation.x.to_bits().hash(&mut hasher);
    transform.translation.y.to_bits().hash(&mut hasher);
    transform.translation.z.to_bits().hash(&mut hasher);

    transform.rotation.x.to_bits().hash(&mut hasher);
    transform.rotation.y.to_bits().hash(&mut hasher);
    transform.rotation.z.to_bits().hash(&mut hasher);
    transform.rotation.w.to_bits().hash(&mut hasher);

    // skip transform.scale as it's not used for gameplay

    hasher.finish()

Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

this function should probably be marked as inlined, I think the glam versions are already

@johanhelsing
Copy link
Contributor Author

this function should probably be marked as inlined, I think the glam versions are already

They are. Added it. I also added must_use even though glam doesn't (f32 has it though).

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 19, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2023
@mockersf mockersf added this pull request to the merge queue Nov 20, 2023
Merged via the queue into bevyengine:main with commit ef50b3c Nov 20, 2023
22 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Sometimes it's very useful to know if a `Transform` contains any `NaN`
or infinite values. It's a bit boiler-plate heavy to check translation,
rotation and scale individually.

## Solution

- Add a new method `is_finite` that returns true if, and only if
translation, rotation and scale all are finite.
- It's a natural extension of `Quat::is_finite`, and `Vec3::is_finite`,
which return true if, and only if all their components' `is_finite()`
returns true.

---

## Changelog

- Added `Transform::is_finite`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants