Skip to content

Fix broken bezier curve benchmark#14677

Merged
alice-i-cecile merged 3 commits intobevyengine:mainfrom
mweatherley:bezier-fix
Aug 12, 2024
Merged

Fix broken bezier curve benchmark#14677
alice-i-cecile merged 3 commits intobevyengine:mainfrom
mweatherley:bezier-fix

Conversation

@mweatherley
Copy link
Copy Markdown
Contributor

Objective

Apparently #14382 broke this, but it's not a part of CI, so it wasn't found until earlier today.

Solution

Update the benchmark like we updated the examples.

Testing

Running cargo bench actually works now.

@mweatherley mweatherley added C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy A-Math Fundamental domain-agnostic mathematical operations C-Benchmarks Stress tests and benchmarks used to measure how fast things are D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 9, 2024
Copy link
Copy Markdown
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

I should be running the benchmarks more often when we touch curves,

@ItsDoot ItsDoot added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 9, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 12, 2024
Merged via the queue into bevyengine:main with commit 4ace888 Aug 12, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024
…unspecified precision (#14693)

# Objective

Closes #14474

Previously, the `libm` feature of bevy_math would just pass the same
feature flag down to glam. However, bevy_math itself had many uses of
floating-point arithmetic with unspecified precision. For example,
`f32::sin_cos` and `f32::powi` have unspecified precision, which means
that the exact details of their output are not guaranteed to be stable
across different systems and/or versions of Rust. This means that users
of bevy_math could observe slightly different behavior on different
systems if these methods were used.

The goal of this PR is to make it so that the `libm` feature flag
actually guarantees some degree of determinacy within bevy_math itself
by switching to the libm versions of these functions when the `libm`
feature is enabled.

## Solution

bevy_math now has an internal module `bevy_math::ops`, which re-exports
either the standard versions of the operations or the libm versions
depending on whether the `libm` feature is enabled. For example,
`ops::sin` compiles to `f32::sin` without the `libm` feature and to
`libm::sinf` with it.

This approach has a small shortfall, which is that `f32::powi` (integer
powers of floating point numbers) does not have an equivalent in `libm`.
On the other hand, this method is only used for squaring and cubing
numbers in bevy_math. Accordingly, this deficit is covered by the
introduction of a trait `ops::FloatPow`:
```rust
pub(crate) trait FloatPow {
    fn squared(self) -> Self;
    fn cubed(self) -> Self;
}
```

Next, each current usage of the unspecified-precision methods has been
replaced by its equivalent in `ops`, so that when `libm` is enabled, the
libm version is used instead. The exception, of course, is that
`.powi(2)`/`.powi(3)` have been replaced with `.squared()`/`.cubed()`.

Finally, the usage of the plain `f32` methods with unspecified precision
is now linted out of bevy_math (and hence disallowed in CI). For
example, using `f32::sin` within bevy_math produces a warning that tells
the user to use the `ops::sin` version instead.

## Testing

Ran existing tests. It would be nice to check some benchmarks on NURBS
things once #14677 merges. I'm happy to wait until then if the rest of
this PR is fine.

---

## Discussion

In the future, it might make sense to actually expose `bevy_math::ops`
as public if any downstream Bevy crates want to provide similar
determinacy guarantees. For now, it's all just `pub(crate)`.

This PR also only covers `f32`. If we find ourselves using `f64`
internally in parts of bevy_math for better robustness, we could extend
the module and lints to cover the `f64` versions easily enough.

I don't know how feasible it is, but it would also be nice if we could
standardize the bevy_math tests with the `libm` feature in CI, since
their success is currently platform-dependent (e.g. 8 of them fail on my
machine when run locally).

---------

Co-authored-by: IQuick 143 <IQuick143cz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Math Fundamental domain-agnostic mathematical operations C-Benchmarks Stress tests and benchmarks used to measure how fast things are C-Bug An unexpected or incorrect behavior D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills D-Trivial Nice and easy! A great choice to get started with Bevy 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.

5 participants