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 no_std Support to bevy_math #15810

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Oct 10, 2024

Objective

Solution

  • Added two new features, std (default) and alloc, gating std and alloc behind them respectively.
  • Added missing f32 functions to std_ops as required. These f32 methods have been added to the clippy.toml deny list to aid in no_std development.

Testing

  • CI
  • cargo clippy -p bevy_math --no-default-features --features libm --target "x86_64-unknown-none"
  • cargo test -p bevy_math --no-default-features --features libm
  • cargo test -p bevy_math --no-default-features --features "libm, alloc"
  • cargo test -p bevy_math --no-default-features --features "libm, alloc, std"
  • cargo test -p bevy_math --no-default-features --features "std"

Notes

The following items require the alloc feature to be enabled:

  • CubicBSpline
  • CubicBezier
  • CubicCardinalSpline
  • CubicCurve
  • CubicGenerator
  • CubicHermite
  • CubicNurbs
  • CyclicCubicGenerator
  • RationalCurve
  • RationalGenerator
  • BoxedPolygon
  • BoxedPolyline2d
  • BoxedPolyline3d
  • SampleCurve
  • SampleAutoCurve
  • UnevenSampleCurve
  • UnevenSampleAutoCurve
  • EvenCore
  • UnevenCore
  • ChunkedUnevenCore

This requirement could be relaxed in certain cases, but I had erred on the side of gating rather than modifying. Since no_std is a new set of platforms we are adding support to, and the alloc feature is enabled by default, this is not a breaking change.

@bushrat011899 bushrat011899 added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 10, 2024
crates/bevy_math/Cargo.toml Show resolved Hide resolved
crates/bevy_math/src/ops.rs Outdated Show resolved Hide resolved
Co-Authored-By: Benjamin Brienen <benjamin.brienen@outlook.com>
Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

A few notes:

  1. The purpose of the ops module is (or perhaps was) to guarantee that alternatives to std floating-point functions with unspecified precision were available, in the interest of cross-platform determinism. Since libm is no_std-compatible, I think this extension of that is pretty reasonable, but this is a break from its original purpose.

  2. I have concerns about the ongoing maintenance burden of these changes. For example, we lint against std math operations with unspecified precision for the aforementioned determinism reasons; right now, as far as I'm aware, this doesn't extend to the new methods added, so I'm not sure if breaks to no_std compatibility are actually discoverable.

  3. cubic_splines is feature-gated behind alloc, but plenty of other areas use alloc as well. What is the logic behind this?

@mweatherley mweatherley added X-Contentious There are nontrivial implications that should be thought through and removed X-Uncontroversial This work is generally agreed upon labels Oct 10, 2024
@bushrat011899
Copy link
Contributor Author

  1. The purpose of the ops module is (or perhaps was) to guarantee that alternatives to std floating-point functions with unspecified precision were available, in the interest of cross-platform determinism. Since libm is no_std-compatible, I think this extension of that is pretty reasonable, but this is a break from its original purpose.

That makes sense, thanks for clarifying the original intent of the module for me! I think to highlight this change in intent I'll update the module documentation to say there are two reasons you'd want to use std_ops: determinism, and no_std compatibility.

  1. I have concerns about the ongoing maintenance burden of these changes. For example, we lint against std math operations with unspecified precision for the aforementioned determinism reasons; right now, as far as I'm aware, this doesn't extend to the new methods added, so I'm not sure if breaks to no_std compatibility are actually discoverable.

I'll look into adding these new methods to the lint as well, with some documentation explaining that it's for no_std compatibility. It's not included in this PR, but my intention is to add no_std testing to the CI as well. Right now it's sufficient to attempt compilation for a no_std target (e.g., x86_64-unknown-none), but this is still an open question (which no_std platforms are people actually using Bevy on in this hypothetical future? Bare metal x86 is an example, but perhaps not realistic)

  1. cubic_splines is feature-gated behind alloc, but plenty of other areas use alloc as well. What is the logic behind this?

My reasoning here was I saw a large amount of alloc usage contained within this module, so I feature gated the whole lot. I will look into it more closely to see if there are some elements which can still be exported without alloc, and if so, move the feature gate inside the module.

@bushrat011899 bushrat011899 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 10, 2024
- Only referenced new `ops` methods via `ops` namespace (no direct import)
- Moved the new `no_std` ops into their own module within `ops` to clarify intent
- Updated documentation for `ops` module clarifying it can also be used to assist with `no_std` compatibility
- Moved `alloc` feature gate to within `cubic_spline` to clarify which items require its inclusion
- Updated tests to improve compatibility with `no_std` environments
- Added `f32` methods which are not available in `no_std` to the `clippy.toml` deny list with an explanation around the `no_std` compatibility.

Co-Authored-By: Matty <2975848+mweatherley@users.noreply.github.com>
Co-Authored-By: Joona Aalto <jondolf.dev@gmail.com>
@bushrat011899 bushrat011899 added this to the 0.16 milestone Oct 10, 2024
@bushrat011899
Copy link
Contributor Author

Added to the 0.16 milestone to indicate this is not intended to be merged during the 0.15 release cycle.

@bushrat011899 bushrat011899 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 10, 2024
Also fixed issues with `serialize` being enabled without `alloc`
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

This does increase maintenance burden somewhat / make some things slightly uglier, but I would say it's worth it for no_std support, and to move the no_std efforts as a whole forward. Changes look good to me, didn't notice anything wrong.

@bushrat011899 bushrat011899 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 Oct 12, 2024
@mweatherley mweatherley self-requested a review October 13, 2024 14:22
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-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants