Use async-fn in traits rather than BoxedFuture #12550

ArthurBrussee · 2024-03-18T00:48:06Z

Objective

Simplify implementing some asset traits without Box::pin(async move{}) shenanigans.
Fixes (in part) #11308

Solution

Use async-fn in traits when possible in all traits. Traits with return position impl trait are not object safe however, and as AssetReader and AssetWriter are both used with dynamic dispatch, you need a Boxed version of these futures anyway.

In the future, Rust is adding proc macros to generate these traits automatically, and at some point in the future dyn traits should 'just work'. Until then.... this seemed liked the right approach given more ErasedXXX already exist, but, no clue if there's plans here! Especially since these are public now, it's a bit of an unfortunate API, and means this is a breaking change.

In theory this saves some performance when these traits are used with static dispatch, but, seems like most code paths go through dynamic dispatch, which boxes anyway.

I also suspect a bunch of the lifetime annotations on these function could be simplified now as the BoxedFuture was often the only thing returned which needed a lifetime annotation, but I'm not touching that for now as traits + lifetimes can be so tricky.

This is a revival of pull/11362 after a spectacular merge f*ckup, with updates to the latest Bevy. Just to recap some discussion:

Overall this seems like a win for code quality, especially when implementing these traits, but a loss for having to deal with ErasedXXX variants.
ConditionalSend was the preferred name for the trait that might be Send, to deal with wasm platforms.
When reviewing be sure to disable whitespace difference, as that's 95% of the PR.

Changelog

AssetReader, AssetWriter, AssetLoader, AssetSaver and Process now use async-fn in traits rather than boxed futures.

Migration Guide

Custom implementations of AssetReader, AssetWriter, AssetLoader, AssetSaver and Process should switch to async fn rather than returning a bevy_utils::BoxedFuture.
Simultaniously, to use dynamic dispatch on these traits you should instead use dyn ErasedXXX.

bushrat011899

Excellent work! It's a "small" change being able to use async fn directly for implementing these traits, but every little bit helps in an area already as hairy as async Rust.

bushrat011899 · 2024-03-18T01:28:41Z

crates/bevy_utils/src/lib.rs

 #[cfg(not(target_arch = "wasm32"))]
-pub type BoxedFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a>>;
+mod conditional_send {
+    /// Use [`ConditionalSend`] to mark an optional Send trait bound. Useful as on certain platforms (eg. WASM),
+    /// futures aren't Send.
+    pub trait ConditionalSend: Send {}
+    impl<T: Send> ConditionalSend for T {}
+}

-#[allow(missing_docs)]
 #[cfg(target_arch = "wasm32")]
-pub type BoxedFuture<'a, T> = Pin<Box<dyn Future<Output = T> + 'a>>;
+#[allow(missing_docs)]
+mod conditional_send {
+    pub trait ConditionalSend {}
+    impl<T> ConditionalSend for T {}
+}
+
+pub use conditional_send::*;
+
+/// Use [`ConditionalSendFuture`] for a future with an optional Send trait bound, as on certain platforms (eg. WASM),
+/// futures aren't Send.
+pub trait ConditionalSendFuture: std::future::Future + ConditionalSend {}
+impl<T: std::future::Future + ConditionalSend> ConditionalSendFuture for T {}
+
+/// An owned and dynamically typed Future used when you can't statically type your result or need to add some indirection.
+pub type BoxedFuture<'a, T> = std::pin::Pin<Box<dyn ConditionalSendFuture<Output = T> + 'a>>;


As discussed in the previous PR, this is a clean solution to the WASM problem, whilst leaving itself available for other uses as required.

bushrat011899 · 2024-03-18T01:30:03Z

examples/asset/custom_asset.rs

+    async fn load<'a>(
        &'a self,
-        reader: &'a mut Reader,
+        reader: &'a mut Reader<'_>,
        _settings: &'a (),
-        _load_context: &'a mut LoadContext,
-    ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
-        Box::pin(async move {
-            let mut bytes = Vec::new();
-            reader.read_to_end(&mut bytes).await?;
-            let custom_asset = ron::de::from_bytes::<CustomAsset>(&bytes)?;
-            Ok(custom_asset)
-        })
+        _load_context: &'a mut LoadContext<'_>,
+    ) -> Result<Self::Asset, Self::Error> {
+        let mut bytes = Vec::new();
+        reader.read_to_end(&mut bytes).await?;
+        let custom_asset = ron::de::from_bytes::<CustomAsset>(&bytes)?;
+        Ok(custom_asset)
    }


This change alone almost makes the whole PR worth it. A much cleaner option in an area we do expect end-users to work in at some point.

crates/bevy_render/src/texture/compressed_image_saver.rs

bushrat011899 · 2024-03-18T01:38:31Z

crates/bevy_asset/src/io/mod.rs

+
+/// Equivalent to an [`AssetWriter`] but using boxed futures, necessary eg. when using a `dyn AssetWriter`,
+/// as [`AssetWriter`] isn't currently object safe.
+pub trait ErasedAssetWriter: Send + Sync + 'static {


It'd be lovely if (somehow) we could provide a hint like "AssetWriter is not object safe; use ErasedAssetWriter instead." if that particular compiler error is throw. I don't know if that's possible, let alone how, sadly.

Ah yeah good idea! Though I'm also not sure how you'd do that :/ I've at least added a note to AssetReader/AssetWriter about it.

github-actions · 2024-03-18T03:32:09Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

james7132

Looks good to me. For other reviewers, this PR looks big but the overwhelming vast majority of it is indentation changes from dropping the Box::pin context. Strong preference from me to expedite since a large change like this tends to accumulate merge conflicts quickly.

…async functions) (#12638) # Objective get_asset_paths tries to check whether a folder is empty, and if so delete it. However rather than checking whether any subfolder contains files it checks whether _all_ subfolders have files. Also cleanup various BoxedFutures in async recursive functions like these, rust 1.77 now allows recursive async functions (albeit still by boxing), hurray! This is a followup to #12550 (sorta). More BoxedFuture stuff can be removed now that rust 1.77 is out, which can use async recursive functions! This is mainly just cleaner code wise - the recursion still boxes the future so not much to win there. PR is mainly whitespace changes so do disable whitespace diffs for easier review.

# Objective Fixes #15541 A bunch of lifetimes were added during the Assets V2 rework, but after moving to async traits in #12550 they can be elided. That PR mentions that this might be the case, but apparently it wasn't followed up on at the time. ~~I ended up grepping for `<'a` and finding a similar case in `bevy_reflect` which I also fixed.~~ (edit: that one was needed apparently) Note that elided lifetimes are unstable in `impl Trait`. If that gets stabilized then we can elide even more. ## Solution Remove the extra lifetimes. ## Testing Everything still compiles. If I have messed something up there is a small risk that some user code stops compiling, but all the examples still work at least. --- ## Migration Guide The traits `AssetLoader`, `AssetSaver` and `Process` traits from `bevy_asset` now use elided lifetimes. If you implement these then remove the named lifetime.

# Objective Fixes bevyengine#15541 A bunch of lifetimes were added during the Assets V2 rework, but after moving to async traits in bevyengine#12550 they can be elided. That PR mentions that this might be the case, but apparently it wasn't followed up on at the time. ~~I ended up grepping for `<'a` and finding a similar case in `bevy_reflect` which I also fixed.~~ (edit: that one was needed apparently) Note that elided lifetimes are unstable in `impl Trait`. If that gets stabilized then we can elide even more. ## Solution Remove the extra lifetimes. ## Testing Everything still compiles. If I have messed something up there is a small risk that some user code stops compiling, but all the examples still work at least. --- ## Migration Guide The traits `AssetLoader`, `AssetSaver` and `Process` traits from `bevy_asset` now use elided lifetimes. If you implement these then remove the named lifetime.

Use async-fn in traits

ed1aaf2

bushrat011899 approved these changes Mar 18, 2024

View reviewed changes

james7132 added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 18, 2024

james7132 approved these changes Mar 18, 2024

View reviewed changes

james7132 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 Mar 18, 2024

james7132 requested a review from alice-i-cecile March 18, 2024 06:33

SolarLiner added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Mar 18, 2024

ArthurBrussee added 4 commits March 18, 2024 14:21

Missing format

9e7a872

Fix a few CI issues.

09dc8ad

Add a note to the AssetReader/AssetWriter docs about the erased variant.

36caba5

Fix asset_processing example.

692bb98

alice-i-cecile added this pull request to the merge queue Mar 18, 2024

Merged via the queue into bevyengine:main with commit ac49dce Mar 18, 2024
26 checks passed

ArthurBrussee mentioned this pull request Mar 22, 2024

Fix get_asset_paths not properly deleting empty folders (& recursive async functions) #12638

Merged

james7132 mentioned this pull request Mar 23, 2024

Use async fn in trait instead of BoxedFuture #11308

Closed

alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label May 27, 2024

rparrett mentioned this pull request Jun 8, 2024

Simplify async loading in ldtk/tiled helpers ChristopherBiscardi/bevy_ecs_tilemap#1

Merged

mnmaita mentioned this pull request Jun 15, 2024

Remove or shrink bevy_utils #11478

Open

16 tasks

kristoff3r mentioned this pull request Sep 30, 2024

Cleanup unneeded lifetimes in bevy_asset #15546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use async-fn in traits rather than BoxedFuture #12550

Use async-fn in traits rather than BoxedFuture #12550

ArthurBrussee commented Mar 18, 2024 •

edited

Loading

bushrat011899 left a comment

bushrat011899 Mar 18, 2024

bushrat011899 Mar 18, 2024

bushrat011899 Mar 18, 2024

ArthurBrussee Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024

james7132 left a comment

Use async-fn in traits rather than BoxedFuture #12550

Use async-fn in traits rather than BoxedFuture #12550

Conversation

ArthurBrussee commented Mar 18, 2024 • edited Loading

Objective

Solution

Changelog

Migration Guide

bushrat011899 left a comment

Choose a reason for hiding this comment

bushrat011899 Mar 18, 2024

Choose a reason for hiding this comment

bushrat011899 Mar 18, 2024

Choose a reason for hiding this comment

bushrat011899 Mar 18, 2024

Choose a reason for hiding this comment

ArthurBrussee Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2024

james7132 left a comment

Choose a reason for hiding this comment

ArthurBrussee commented Mar 18, 2024 •

edited

Loading

ArthurBrussee Mar 18, 2024 •

edited

Loading