Skip to content

Commit

Permalink
bevy_asset: Improve NestedLoader API (#15509)
Browse files Browse the repository at this point in the history
# Objective

The `NestedLoader` API as it stands right now is somewhat lacking:

- It consists of several types `NestedLoader`, `UntypedNestedLoader`,
`DirectNestedLoader`, and `UntypedDirectNestedLoader`, where a typestate
pattern on `NestedLoader` would be make it more obvious what it does,
and allow centralising the documentation
- The term "untyped" in the asset loader code is overloaded. It can mean
either:
- we have literally no idea what the type of this asset will be when we
load it (I dub this "unknown type")
- we know what type of asset it will be, but we don't know it statically
- we only have a TypeId (I dub this "dynamic type" / "erased")
- There is no way to get an `UntypedHandle` (erased) given a `TypeId`

## Solution

Changes `NestedLoader` into a type-state pattern, adding two type
params:
- `T` determines the typing
- `StaticTyped`, the default, where you pass in `A` statically into `fn
load<A>() -> ..`
- `DynamicTyped`, where you give a `TypeId`, giving you a
`UntypedHandle`
- `UnknownTyped`, where you have literally no idea what type of asset
you're loading, giving you a `Handle<LoadedUntypedAsset>`
- `M` determines the "mode" (bikeshedding TBD, I couldn't come up with a
better name)
- `Deferred`, the default, won't load the asset when you call `load`,
but it does give you a `Handle` to it (this is nice since it can be a
sync fn)
- `Immediate` will load the asset as soon as you call it, and give you
access to it, but you must be in an async context to call it

Changes some naming of internals in `AssetServer` to fit the new
definitions of "dynamic type" and "unknown type". Note that I didn't do
a full pass over this code to keep the diff small. That can probably be
done in a new PR - I think the definiton I laid out of unknown type vs.
erased makes it pretty clear where each one applies.

<details>
<summary>Old issue</summary>

The only real problem I have with this PR is the requirement to pass in
`type_name` (from `core::any::type_name`) into Erased. Users might not
have that type name, only the ID, and it just seems sort of weird to
*have* to give an asset type name. However, the reason we need it is
because of this:
```rs
    pub(crate) fn get_or_create_path_handle_erased(
        &mut self,
        path: AssetPath<'static>,
        type_id: TypeId,
        type_name: &str,
        loading_mode: HandleLoadingMode,
        meta_transform: Option<MetaTransform>,
    ) -> (UntypedHandle, bool) {
        let result = self.get_or_create_path_handle_internal(
            path,
            Some(type_id),
            loading_mode,
            meta_transform,
        );
        // it is ok to unwrap because TypeId was specified above
        unwrap_with_context(result, type_name).unwrap()
    }

pub(crate) fn unwrap_with_context<T>(
    result: Result<T, GetOrCreateHandleInternalError>,
    type_name: &str,
) -> Option<T> {
    match result {
        Ok(value) => Some(value),
        Err(GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified) => None,
        Err(GetOrCreateHandleInternalError::MissingHandleProviderError(_)) => {
            panic!("Cannot allocate an Asset Handle of type '{type_name}' because the asset type has not been initialized. \
                    Make sure you have called app.init_asset::<{type_name}>()")
        }
    }
}
```
This `unwrap_with_context` is literally the only reason we need the
`type_name`. Potentially, this can be turned into an `impl
Into<Option<&str>>`, and output a different error message if the type
name is missing. Since if we are loading an asset where we only know the
type ID, by definition we can't output that error message, since we
don't have the type name. I'm open to suggestions on this.

</details>

## Testing

Not sure how to test this, since I kept most of the actual NestedLoader
logic the same. The only new API is loading an `UntypedHandle` when in
the `DynamicTyped, Immediate` state.

## Migration Guide

Code which uses `bevy_asset`'s `LoadContext::loader` / `NestedLoader`
will see some naming changes:
- `untyped` is replaced by `with_unknown_type`
- `with_asset_type` is replaced by `with_static_type`
- `with_asset_type_id` is replaced by `with_dynamic_type`
- `direct` is replaced by `immediate` (the opposite of "immediate" is
"deferred")
  • Loading branch information
aecsocket authored Oct 1, 2024
1 parent c323db0 commit 1df8238
Show file tree
Hide file tree
Showing 8 changed files with 425 additions and 156 deletions.
1 change: 1 addition & 0 deletions crates/bevy_asset/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ async-lock = "3.0"
crossbeam-channel = "0.5"
downcast-rs = "1.2"
disqualified = "1.0"
either = "1.13"
futures-io = "0.3"
futures-lite = "2.0.1"
blake3 = "1.5"
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ pub use handle::*;
pub use id::*;
pub use loader::*;
pub use loader_builders::{
DirectNestedLoader, NestedLoader, UntypedDirectNestedLoader, UntypedNestedLoader,
Deferred, DynamicTyped, Immediate, NestedLoader, StaticTyped, UnknownTyped,
};
pub use path::*;
pub use reflect::*;
Expand Down Expand Up @@ -689,7 +689,7 @@ mod tests {
for dep in ron.embedded_dependencies {
let loaded = load_context
.loader()
.direct()
.immediate()
.load::<CoolText>(&dep)
.await
.map_err(|_| Self::Error::CannotLoadDependency {
Expand Down
10 changes: 7 additions & 3 deletions crates/bevy_asset/src/loader.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
io::{AssetReaderError, MissingAssetSourceError, MissingProcessedAssetReaderError, Reader},
loader_builders::NestedLoader,
loader_builders::{Deferred, NestedLoader, StaticTyped},
meta::{AssetHash, AssetMeta, AssetMetaDyn, ProcessedInfoMinimal, Settings},
path::AssetPath,
Asset, AssetLoadError, AssetServer, AssetServerMode, Assets, Handle, UntypedAssetId,
Expand Down Expand Up @@ -290,7 +290,11 @@ impl<A: Asset> AssetContainer for A {
}
}

/// An error that occurs when attempting to call [`DirectNestedLoader::load`](crate::DirectNestedLoader::load)
/// An error that occurs when attempting to call [`NestedLoader::load`] which
/// is configured to work [immediately].
///
/// [`NestedLoader::load`]: crate::NestedLoader::load
/// [immediately]: crate::Immediate
#[derive(Error, Debug)]
#[error("Failed to load dependency {dependency:?} {error}")]
pub struct LoadDirectError {
Expand Down Expand Up @@ -550,7 +554,7 @@ impl<'a> LoadContext<'a> {

/// Create a builder for loading nested assets in this context.
#[must_use]
pub fn loader(&mut self) -> NestedLoader<'a, '_> {
pub fn loader(&mut self) -> NestedLoader<'a, '_, StaticTyped, Deferred> {
NestedLoader::new(self)
}

Expand Down
Loading

0 comments on commit 1df8238

Please sign in to comment.