bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.5k stars 3.6k forks source link

Subassets of subassets do not work. #15417

Open andriyDev opened 2 months ago

andriyDev commented 2 months ago

Problem

Today, ErasedLoadedAsset holds the asset as Box<dyn AssetContainer>, and holds subassets as HashMap<String, ErasedLoadedAsset> (not really, but for simplicity, this is true). This means that subassets can themselves hold subassets, and so on, and so on. This makes it very confusing when you find out that these "nested subassets" get "flattened" when they get inserted by the AssetServer. For example, consider the following loader:

async fn load(load_context: &mut LoadContext) -> Result<Self::Asset, Self::Error> {
    load_context.add_labeled_asset("A".into(), Self::Asset(1));
    load_context.labeled_asset_scope("B".into(), |load_context| {
        // This asset replaces the "root" subasset labeled "A"
        load_context.add_labeled_asset("A".into(), Self::Asset(2));
        Self::Asset(3)
    });
    Ok(Self::Asset(4))
}

The ErasedLoadedAsset for this looks like:

{
    value: SomeAsset(4),
    labeled_assets: {
        "A": ErasedLoadedAsset {
            value: SomeAsset(1),
        },
        "B": ErasedLoadedAsset {
            value: SomeAsset(3),
            labeled_assets: {
                "A": ErasedLoadedAsset {
                    value: SomeAsset(2)
                }
            }
        }
    }
}

The resulting Assets would be:

{
    "my_asset.blah": SomeAsset(4),
    "my_asset.blah#A": SomeAsset(2),
    "my_asset.blah#B": SomeAsset(3),
}

What solution would you like?

  1. Remove labeled_assets from ErasedLoadedAsset/LoadedAssetand propagate it separately. The hashmap of labeled assets should continue to just use ErasedLoadedAsset.

  2. Make the LoadContext::labeled_asset_scope (and related) merge the current and the new set of subassets.

    1. We could consider disallowing subassets to themselves load subassets (though this is likely not desirable)
    2. Overwriting a subasset should probably be a warning or maybe even a panic.
  3. Make AssetServer::send_loaded_asset no longer recursive.

What alternative(s) have you considered?

Do nothing. Keep the confusing behavior. This is only an issue if, in an asset loader, in a LoadContext::labeled_asset_scope (or equivalent), a subasset shares a name with a previously added subasset.

Another alternative: Make them actually work! We could create some notation to support nested subassets in AssetPath (for example, "my_asset.blah#A#B#C" or maybe "my_asset.blah#A/B/C"). It's not clear that this is something anyone wants or needs, so the complexity seems unwarranted.

andriyDev commented 2 months ago

I suspect also the meta field also doesn't need to exist for subassets. This might point to splitting the LoadContext into a SubassetLoadContext or something.