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

Create bevy_platform_support Crate #17250

Merged

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Jan 9, 2025

Objective

Solution

  • Initial creation of bevy_platform_support crate.
  • Moved bevy_utils::Instant into new bevy_platform_support crate.
  • Moved portable-atomic, portable-atomic-util, and critical-section into new bevy_platform_support crate.

Testing

  • CI

Showcase

Instead of needing code like this to import an Arc:

#[cfg(feature = "portable-atomic")]
use portable_atomic_util::Arc;

#[cfg(not(feature = "portable-atomic"))]
use alloc::sync::Arc;

We can now use:

use bevy_platform_support::sync::Arc;

This applies to many other types, but the goal is overall the same: allowing crates to use std-like types without the boilerplate of conditional compilation and platform-dependencies.

Migration Guide

  • Replace imports of bevy_utils::Instant with bevy_platform_support::time::Instant
  • Replace imports of bevy::utils::Instant with bevy::platform_support::time::Instant

Notes

  • bevy_platform_support hasn't been reserved on crates.io
  • bevy_platform_support is not re-exported from bevy at this time. It may be worthwhile exporting this crate, but I am unsure of a reasonable name to export it under (platform_support may be a bit wordy for user-facing).
  • I've included an implementation of Instant which is suitable for no_std platforms that are not Wasm for the sake of eliminating feature gates around its use. It may be a controversial inclusion, so I'm happy to remove it if required.
  • There are many other items (spin, bevy_utils::Sync(Unsafe)Cell, etc.) which should be added to this crate. I have kept the initial scope small to demonstrate utility without making this too unwieldy.

@bushrat011899 bushrat011899 added C-Dependencies A change to the crates that Bevy depends on A-Utils Utility functions and types X-Controversial There is active debate or serious implications around merging this PR D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward O-Embedded Weird hardware and no_std platforms labels Jan 9, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 9, 2025
@alice-i-cecile
Copy link
Member

bevy_platform_support is not re-exported from bevy at this time. It may be worthwhile exporting this crate, but I am unsure of a reasonable name to export it under (platform_support may be a bit wordy for user-facing).

Just re-export it as is, the name is fine and this is less surprising.

@bushrat011899
Copy link
Contributor Author

Just re-export it as is, the name is fine and this is less surprising.

Done!

@alice-i-cecile
Copy link
Member

Marking as blessed due to this comment by @cart. I agree with it too!

@alice-i-cecile alice-i-cecile added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers and removed X-Controversial There is active debate or serious implications around merging this PR labels Jan 9, 2025
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

I don't really understand the fallback implementation for Instant, but the rest looks good to me

crates/bevy_platform_support/README.md Outdated Show resolved Hide resolved
@TimJentzsch TimJentzsch added the D-Unsafe Touches with unsafe code in some way label Jan 12, 2025
bushrat011899 and others added 2 commits January 13, 2025 08:43
Provide instructions more robust to changing version numbers.

Co-Authored-By: TimJentzsch <[email protected]>
@bushrat011899
Copy link
Contributor Author

I don't really understand the fallback implementation for Instant, but the rest looks good to me

I should update the documentation to be more clear on how this works. Basically it's a straight copy of the Instant from std, but it includes two key changes:

  1. Instant::set_elapsed is a new method that lets you tell Instant how to know how much time has elapsed. Since we don't have an OS, we can't use the typical syscalls. A simpler alternative would be to just provide Instant with the elapsed time directly, rather than giving it a function to determine elapsed time. But the benefit here is now Instant can update itself only when it needs to, instead of the user needing to eagerly update it (e.g., in the mainloop)
  2. Instant::now calls the method provided to Instant::set_elapsed to determine how much time has elapsed, instead of using a syscall directly like in std.

For the sake of convenience, I've included an unset_getter method, which is the default function called by Instant::now if you haven't set one yourself. If you're on x86 or aarch64, we have access to a built-in instruction for getting a system tick in nanoseconds, so we can actually provide a working default implementation on those platforms. For all others, this method panics, explaining that you need to call Instant::set_elapsed before Instant::now is called.

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

Pretty straightforward and well done!

@BenjaminBrienen BenjaminBrienen 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 Jan 16, 2025
@Bleachfuel
Copy link
Contributor

Why not just call it bevy_platform? It is already reserved on crates.io

@bushrat011899
Copy link
Contributor Author

Why not just call it bevy_platform? It is already reserved on crates.io

It could be called that, but that's a pretty prime crate name that'd have a lot of uses. platform_support, while verbose, is pretty specific to the idea of providing cross-platform support utilities.

@mockersf
Copy link
Member

could you add the new crate (in the correct order by depdendency) in https://github.com/bevyengine/bevy/blob/main/tools/publish.sh?

@bushrat011899
Copy link
Contributor Author

could you add the new crate (in the correct order by depdendency) in https://github.com/bevyengine/bevy/blob/main/tools/publish.sh?

Done! bevy_platform_support is required by bevy_utils, so I've placed it at the very top of the list directly above it.

@BD103
Copy link
Member

BD103 commented Jan 17, 2025

  • bevy_platform_support hasn't been reserved on crates.io

Once this PR is merged, we should add the crate to bevy-crate-reservations.

cc @cart :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2025
Merged via the queue into bevyengine:main with commit a64446b Jan 20, 2025
32 checks passed
@cart
Copy link
Member

cart commented Jan 20, 2025

Reserved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Dependencies A change to the crates that Bevy depends on D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way O-Embedded Weird hardware and no_std platforms S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants