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.

Sign up for GitHub

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

Jump to bottom

Use async fn in trait instead of BoxedFuture #11308

Closed
tguichaoua opened this issue Jan 11, 2024 · 1 comment
Closed

Use async fn in trait instead of BoxedFuture #11308

tguichaoua opened this issue Jan 11, 2024 · 1 comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible

Comments

@tguichaoua
Copy link
Contributor

tguichaoua commented Jan 11, 2024 •
edited
Loading

What problem does this solve or what need does it fill?

async fn and return-position impl Trait in trait as been stabilized in 1.75 (rust-lang/rust#115822)

What solution would you like?

Replace usage of BoxedFuture in traits like AssetLoader by async fn.

bevy/crates/bevy_asset/src/loader.rs

Lines 33 to 38 in ce5bae5

fn load<'a>(
&'a self,
reader: &'a mut Reader,
settings: &'a Self::Settings,
load_context: &'a mut LoadContext,
) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>>; 

 async fn load<'a>( 
     &'a self, 
     reader: &'a mut Reader, 
     settings: &'a Self::Settings, 
     load_context: &'a mut LoadContext, 
 ) -> Result<Self::Asset, Self::Error>;

What alternative(s) have you considered?

Keep using BoxedFuture with Box::pin(async move { /* */ });.

Current limitations

Because of the send bound problem, there is no way, yet, to add the Send bound on the returned future.
Alternatively it's possible to use impl Future + Send:

 fn load<'a>( 
     &'a self, 
     reader: &'a mut Reader, 
     settings: &'a Self::Settings, 
     load_context: &'a mut LoadContext, 
 ) -> impl Future<Output = Result<Self::Asset, Self::Error>> + Send + 'a;
@tguichaoua tguichaoua added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 11, 2024
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Jan 11, 2024
@ArthurBrussee ArthurBrussee mentioned this issue Jan 15, 2024
Closed
@ArthurBrussee ArthurBrussee mentioned this issue Mar 18, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 18, 2024
@ArthurBrussee
Use async-fn in traits rather than BoxedFuture (#12550)
ac49dce 
# 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
](https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html)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](#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.
@james7132
Copy link
Member

This was implemented in #12550 and follow-up PRs.

dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this issue Jul 7, 2024
@ArthurBrussee
Use async-fn in traits rather than BoxedFuture (#12550)
6b93094 
# Objective

Simplify implementing some asset traits without Box::pin(async move{})
shenanigans.
Fixes (in part) bevyengine/bevy#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
](https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html)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](bevyengine/bevy#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@james7132 @alice-i-cecile @tguichaoua