Skip to content

Commit

Permalink
Use async-fn in traits rather than BoxedFuture (#12550)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
ArthurBrussee authored Mar 18, 2024
1 parent ce75dec commit ac49dce
Show file tree
Hide file tree
Showing 33 changed files with 1,092 additions and 1,097 deletions.
62 changes: 29 additions & 33 deletions crates/bevy_animation/src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use std::ops::{Index, IndexMut};
use bevy_asset::io::Reader;
use bevy_asset::{Asset, AssetId, AssetLoader, AssetPath, AsyncReadExt as _, Handle, LoadContext};
use bevy_reflect::{Reflect, ReflectSerialize};
use bevy_utils::BoxedFuture;
use petgraph::graph::{DiGraph, NodeIndex};
use ron::de::SpannedError;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -336,40 +335,37 @@ impl AssetLoader for AnimationGraphAssetLoader {

type Error = AnimationGraphLoadError;

fn load<'a>(
async fn load<'a>(
&'a self,
reader: &'a mut Reader,
reader: &'a mut Reader<'_>,
_: &'a Self::Settings,
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?;

// Deserialize a `SerializedAnimationGraph` directly, so that we can
// get the list of the animation clips it refers to and load them.
let mut deserializer = ron::de::Deserializer::from_bytes(&bytes)?;
let serialized_animation_graph =
SerializedAnimationGraph::deserialize(&mut deserializer)
.map_err(|err| deserializer.span_error(err))?;

// Load all `AssetPath`s to convert from a
// `SerializedAnimationGraph` to a real `AnimationGraph`.
Ok(AnimationGraph {
graph: serialized_animation_graph.graph.map(
|_, serialized_node| AnimationGraphNode {
clip: serialized_node.clip.as_ref().map(|clip| match clip {
SerializedAnimationClip::AssetId(asset_id) => Handle::Weak(*asset_id),
SerializedAnimationClip::AssetPath(asset_path) => {
load_context.load(asset_path)
}
}),
weight: serialized_node.weight,
},
|_, _| (),
),
root: serialized_animation_graph.root,
})
load_context: &'a mut LoadContext<'_>,
) -> Result<Self::Asset, Self::Error> {
let mut bytes = Vec::new();
reader.read_to_end(&mut bytes).await?;

// Deserialize a `SerializedAnimationGraph` directly, so that we can
// get the list of the animation clips it refers to and load them.
let mut deserializer = ron::de::Deserializer::from_bytes(&bytes)?;
let serialized_animation_graph = SerializedAnimationGraph::deserialize(&mut deserializer)
.map_err(|err| deserializer.span_error(err))?;

// Load all `AssetPath`s to convert from a
// `SerializedAnimationGraph` to a real `AnimationGraph`.
Ok(AnimationGraph {
graph: serialized_animation_graph.graph.map(
|_, serialized_node| AnimationGraphNode {
clip: serialized_node.clip.as_ref().map(|clip| match clip {
SerializedAnimationClip::AssetId(asset_id) => Handle::Weak(*asset_id),
SerializedAnimationClip::AssetPath(asset_path) => {
load_context.load(asset_path)
}
}),
weight: serialized_node.weight,
},
|_, _| (),
),
root: serialized_animation_graph.root,
})
}

Expand Down
69 changes: 29 additions & 40 deletions crates/bevy_asset/src/io/android.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use crate::io::{
get_meta_path, AssetReader, AssetReaderError, EmptyPathStream, PathStream, Reader, VecReader,
};
use bevy_utils::tracing::error;
use bevy_utils::BoxedFuture;
use std::{ffi::CString, path::Path};

/// [`AssetReader`] implementation for Android devices, built on top of Android's [`AssetManager`].
Expand All @@ -17,57 +16,47 @@ use std::{ffi::CString, path::Path};
pub struct AndroidAssetReader;

impl AssetReader for AndroidAssetReader {
fn read<'a>(
&'a self,
path: &'a Path,
) -> BoxedFuture<'a, Result<Box<Reader<'a>>, AssetReaderError>> {
Box::pin(async move {
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
.asset_manager();
let mut opened_asset = asset_manager
.open(&CString::new(path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(path.to_path_buf()))?;
let bytes = opened_asset.buffer()?;
let reader: Box<Reader> = Box::new(VecReader::new(bytes.to_vec()));
Ok(reader)
})
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
.asset_manager();
let mut opened_asset = asset_manager
.open(&CString::new(path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(path.to_path_buf()))?;
let bytes = opened_asset.buffer()?;
let reader: Box<Reader> = Box::new(VecReader::new(bytes.to_vec()));
Ok(reader)
}

fn read_meta<'a>(
&'a self,
path: &'a Path,
) -> BoxedFuture<'a, Result<Box<Reader<'a>>, AssetReaderError>> {
Box::pin(async move {
let meta_path = get_meta_path(path);
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
.asset_manager();
let mut opened_asset = asset_manager
.open(&CString::new(meta_path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(meta_path))?;
let bytes = opened_asset.buffer()?;
let reader: Box<Reader> = Box::new(VecReader::new(bytes.to_vec()));
Ok(reader)
})
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
let meta_path = get_meta_path(path);
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
.asset_manager();
let mut opened_asset = asset_manager
.open(&CString::new(meta_path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(meta_path))?;
let bytes = opened_asset.buffer()?;
let reader: Box<Reader> = Box::new(VecReader::new(bytes.to_vec()));
Ok(reader)
}

fn read_directory<'a>(
async fn read_directory<'a>(
&'a self,
_path: &'a Path,
) -> BoxedFuture<'a, Result<Box<PathStream>, AssetReaderError>> {
) -> Result<Box<PathStream>, AssetReaderError> {
let stream: Box<PathStream> = Box::new(EmptyPathStream);
error!("Reading directories is not supported with the AndroidAssetReader");
Box::pin(async move { Ok(stream) })
Ok(stream)
}

fn is_directory<'a>(
async fn is_directory<'a>(
&'a self,
_path: &'a Path,
) -> BoxedFuture<'a, std::result::Result<bool, AssetReaderError>> {
) -> std::result::Result<bool, AssetReaderError> {
error!("Reading directories is not supported with the AndroidAssetReader");
Box::pin(async move { Ok(false) })
Ok(false)
}
}
Loading

0 comments on commit ac49dce

Please sign in to comment.