bevy_asset: Improve NestedLoader API

[aecsocket]

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.

Old issue

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:

    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.

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")

[alice-i-cecile]

Fun use of the type state pattern (and Either), clear motivation, and great docs. I like this!

[JMS55]

Sorry, realistically I'm not going to have time to review this.

[bushrat011899]

I think this is a good API change and very well documented. I do have a couple of suggestions but nothing I'd consider blocking.

[alice-i-cecile]

@aecsocket I'll give you a day or two to respond to the review, then merge this in before the release candidate ships :)

[andriyDev]

A couple minor changes, nothing critical!

[andriyDev]

One thing that's a little odd to me, I'm not sure if we care enough: with this impl, you can say:

load_context.loader().with_static_type().with_static_type().with_static_type()...

We could solve this with separate impl blocks, but perhaps that's too much boilerplate.

Feel free to ack!

[BenjaminBrienen]

I think this small improvement would be nice. I bet you someone will notice it and report an issue anyway!

[aecsocket]

To enable this for just the T parameter, I'd need:

impl<M> NestedLoader<StaticTyped, M> {
    fn with_dynamic_type() -> .. {}
    fn with_unknown_type() -> .. {}
}

impl<M> NestedLoader<DynamicTyped, M> {
    fn with_static_type() -> .. {}
    fn with_unknown_type() -> .. {}
}

impl<M> NestedLoader<UnknownType, M> {
    fn with_static_type() -> .. {}
    fn with_dynamic_type -> .. {}
}

I really don't like the duplication here, but what's a deal breaker for me with this approach is I would have to duplicate docs for both of the with_x_type etc. Docs are so much harder to keep up to date than code, and I'd always prioritise having a single source of truth for docs over duplicating docs. Inevitably, if we do this, someone will change how these functions work and change the docs for one of these, then forget the other.

I could also add some sealed traits like TypedNotStatic, TypedNotDynamic, TypedNotUnknown, etc. and impl for those, but IMO this is over-engineering a solution to something that isn't really a problem (keep in mind extra traits and LoC is extra maintenance cost, even if it is trivial). I think if someone writes

my_loader
    .with_static_type()
    .with_static_type()

it's not really harmful/a footgun of any kind, and it's easily spotted in code review.

The same problem exists for M as well, but to a lesser extent.

[aecsocket]

Resolved everything apart from 1 comment. I think my reasoning makes sense for leaving that one open.