Use async-fn-in-trait (RPIT) for various Asset traits.

[ArthurBrussee]

trafficstars

(First OSS contribution, so apologies if I mess up the format, roll-ups, or err the rest! Picked a random issue to see what it's like)

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.

---

Changelog

• AssetReader, AssetWriter, AssetLoader, AssetSaver and Process now use async-fn in traits, so they're easier to implement.

Migration Guide

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

[github-actions[bot]]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[JMS55]

Note for reviewers: Much easier to review if you hide the whitespace diff.

[github-actions[bot]]

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

[ArthurBrussee]

Ah didn't realise Bevy MSRV isn't 1.75 yet, that makes this a bit pointless.

Also WASM doesn't build - trait fns need to be optionally Send, only on non-wasm platforms. That's... tricky. Maybe too early for async-fn-in-trait after all, but, will do some reading on what the state of things is there currently.

[JMS55]

Ah didn't realise Bevy MSRV isn't 1.75 yet, that makes this a bit pointless.

Shouldn't be a blocker, I think we can bump it.

[github-actions[bot]]

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

[github-actions[bot]]

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

[ArthurBrussee]

Ah didn't realise Bevy MSRV isn't 1.75 yet, that makes this a bit pointless.

Shouldn't be a blocker, I think we can bump it.

Oh ok great! Would that be a seperate PR or included in this? Given the insistent bot I've added it in here.

That leaves the WASM problem. The recap, mainly for myself:

• impl Future<> functions can (usually) be Send fine, as by design RPIT allows trait bounds to be inferred
• However, for dynamic dispatch you need to KNOW the future is Send,. Hence the boxed future has an explicit Send bound.
• On WASM however, wasm_bindgen uses JsPromise objects for futures(?) which are NOT send.
• Bevy's boxed future is only Send on non-wasm platforms.
• For impl Future<> however, we now need a way to specify MaybeSend for each function....

The potential solutions as far as I can see:

• Duplicate every trait function with a different return cfg, urgh
• Some type incantations I'm not aware of to make a MaybeSendOnlyOnWasm trait bound? Trait aliases would do this https://rust-lang.github.io/rfcs/1733-trait-alias.html
• Switch to only using spawn_local for assets.
• Switch to a non-work-stealing executor (eg. what this is arguing https://maciej.codes/2022-06-09-local-async.html)
• Wait a while longer until rust sorts more async stuff out :)

So yeah, not sure, might have hit a dead end for wasm!

[JMS55]

This PR is fine for bumping MSRV.

I don't have time to think about WASM atm, will let someone else respond. Feel free to join the -dev channels in discord (engine-dev/asset-dev) and brainstorm solutions with other people though :)

[mockersf]

No issues to update the MSRV, this is usually done in the first PR that needs the new version of Rust.

I would love to have this, but I also think this is blocked for Wasm for now 🙁

[ArthurBrussee]

So it seems there is a way to make conditional Send bounds, and more people do it this way. Have made an attempt with a "FutureSend" (could be MaybeSend? ConditionalSend?) that seems to work for wasm!

Also tried to understand where this problem really comes from, and it basically comes down to: wasm_bindgen uses JS promises which aren't Send for futures for web APIs, including Fetch, which is why the HttpWasmAssetReader can't have Send futures. That really seems to be the only reason wasm needs this special treatment, though of course that propogates all the way through.

Let me know if this seems workable, still happy to say this is getting too weird and it should wait for rust to stabilise more async features, or idk use #[async_trait] instead and call it a day haha.

Thanks!

[ArthurBrussee]

Thanks a lot for that! Really nice to hear your design thoughts :) Great that the Wasm solution sounds workable! I've renamed FutureSend to WasmNotSend as per giooschi's suggestion on Discord to match WGPU.

And yeah ErasedX is the main actual visible usability downside... Good point that that's maybe more what is exposed to the user even :/ My hope was that since a bunch of the other Asset traits already have these it's not a whole other concep to lear.

Really annoying that #[async-trait] CAN do without this but 'native' RPITIT can't! From yosh & boats blog seems the async-WG thinks dyn + async traits might happen in 2024, and could get rid of all these wrappers.

[cart]

WasmNotSend

Not a huge deal, but I'd prefer it if we made the naming a bit more generic to avoid having "wasm" in the ~~function signature~~ name. I think the trait name should make sense with or without the wasm context.

The module name seems like a good choice: ConditionalSend

[ArthurBrussee]

Oh neat! Yeah prefer that too, made it ConditionalSend now. There could even be a generic feature for bevy tasks to support a work-stealing scheduler or not (which then just isn't supported on wasm).

[ArthurBrussee]

Have updated to latest main in case this is still the way to go :) Some weird CI failures atm that I think are unrelated, other PRs seem to have the same.

[ArthurBrussee]

Wellll seems like I fucked the merge pretty bad :') I'll redo this PR at head, it was getting a bit messy anyway.