Skip to content

Fix nested immediate asset loading. #18295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • When performing a nested immediate asset load, the asset is stored in the LoadContext. Users are only given a NestedAssetRef/NestedErasedAssetRef which is merely an immutable reference to the assets.
  • Once the "outer" asset finishes loading, any nested immediate assets it had loaded also get sent to the asset system (along with the outer asset). This will either refresh the asset if it was already loaded, or it will now be loaded (unless the handle gets dropped).
  • Users no longer need to (and can't) add nested immediate assets as subassets.

Testing

  • Updated the existing load_dependencies test to account for the fact that nested immediate loaded assets now modify the previously loaded assets.

Risks

  • Nested, immediate asset loads that use a custom reader will not give you ownership of the value. These must be accessed through a handle (or cloned). The asset_decompression example is relevant here: it needed to be refactored to no longer need to store the decompressed asset.

Migration Guide

Nested, immediate asset loads no longer return a CompleteLoadedAsset/CompleteErasedLoadedAsset. Instead they return a handle to the asset and a NestedAssetRef/NestedErasedAssetRef to access its data. Previously, nested asset loads could look like:

let loaded_asset = load_context.loader().immediate().load<Image>("nested_asset.png").await?;
let image = loaded_asset.get_asset();
// Access the image here, or even call `.take()` to take inner asset!

let handle = load_context.add_loaded_labeled_asset("this_is_nested", image);
// Store this handle somewhere.

Now these nested assets are "registered" for you.

let (handle, loaded_asset) = load_context.loader().immediate().load<Image>("nested_asset.png").await?;
let image = loaded_asset.get_asset();
// Access the image here, but we can't `.take()` it anymore...
// Store this handle somewhere.

@andriyDev andriyDev requested a review from pcwalton March 13, 2025 06:55
@andriyDev andriyDev added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 13, 2025
@andriyDev
Copy link
Contributor Author

This PR is based of #18213. Once it's merged, I will rebase onto main.

@andriyDev andriyDev added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 13, 2025
@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 14, 2025
@andriyDev andriyDev added this to the 0.16 milestone Mar 14, 2025
@andriyDev andriyDev added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 17, 2025
@@ -1781,6 +1811,10 @@ impl RecursiveDependencyLoadState {
}
}

/// A wrapper for storing nested directly-loaded assets.
#[derive(Deref, DerefMut, Default)]
pub(crate) struct NestedAssets(HashMap<AssetPath<'static>, CompleteErasedLoadedAsset>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty confused by the current state of things (I'm also just now getting caught up on #15481) and what we're trying to accomplish.

It seems like the problem we're trying to solve is "subassets of subassets", which is something I'm in support of.

In my head the way we do this is to construct a "tree" of sub assets, where each item in the tree has a label, and we devise a nesting scheme into labels (ex: something asset.gltf/Scene0/Image0). Only the root of the tree should have a real "asset path". Every "sub asset" should just have a label. And sub-assets should be able to have their own sub-assets (which have labels).

If we are loading an asset "immediately", we are not loading it to feed it into "final" asset system storage, but as an input to the loader (ex: perhaps because we want to take "ownership" of it and consider it "nested", in which case, it should not retain a conception of its original identity because it is not that asset anymore).

If I'm missing something (ex: my conception of the system we should be building should change, or we are actually aligned on this), can you let me know? What is your "vision" for this part of the asset system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking a look!

To start, my vision here is that nested asset loads should (try to) act like regular asset loads. Obviously it can't be identical since it needs to work in async, but making it look similar would be ideal. I don't understand the motivation for wanting to own the nested asset (with the exception of nested assets that use a reader). It seems to me like if you want to manipulate the nested asset, you should just clone it. This has all sorts of benefits, like now your regular asset loading for the nested asset can be in sync with your nested asset loading (though we're not fully there with this PR).

The tree of subassets is mostly fine - I don't really have a problem with it, but setting up the API to do that is kind of a pain. We need to know the label you're gonna give the asset before we build the subasset, since otherwise the handles to the sub-subassets will be wrong. This is fine, but it's not clear to me this is worth it. For one it locks us into a specific "structure" for our asset labels. For example, we now have to ban slashes from subasset labels (else they could alias with other subassets). It also opens the door for "parsing the label" which I really want to avoid. I'd much rather have users decide on what structure makes sense for their asset type. Hence #15481 - just preventing users from making duplicates is safe enough, preserves the existing behavior (e.g., the GLTF loader can remain the same and ensures the labels are unique "by construction").

Trying to do nested asset loads by trying to follow the tree is actually quite tricky. We need to load the path for the nested asset, but all its subassets need to be registered against the root asset. In addition, any relative asset paths need to be relative to the nested asset. Oh, and the subasset labels for the nested asset need to have the correct prefix now with its slashes as well. This may be possible, but it sounds like a recipe for disaster where we accidentally pass the wrong path. Before #15481, we registered the nested asset as a subasset of the root, but the subassets of the nested asset were still being registered as subassets of the nested asset.

I think just making nested asset loads act more like regular asset loads results in a more clear API: if you want access to an asset, you just load it like you normally load an asset and then you can access it. Maybe even with #15813, we can avoid loading at all here, and you just send a normal request to the asset system to load, await the load using #14431, take the Arc to the nested asset, and pass it back to the async stuff as a reference. But that's very much besides the point, making nested asset loads look like regular asset loads seems simpler to me.

@mcobzarenco
Copy link
Contributor

Do I understand correctly that with this PR, it won't be possible to create a custom asset which loads some other asset (say an image) and applies a tranformation to it without cloning the data? This is a pattern I use frequently, is there still a way to retain ownership of the subasset you load? Automatic labelling doesn't seem worth the cost to me

@andriyDev
Copy link
Contributor Author

andriyDev commented Mar 22, 2025

Do I understand correctly that with this PR, it won't be possible to create a custom asset which loads some other asset (say an image) and applies a tranformation to it without cloning the data? This is a pattern I use frequently, is there still a way to retain ownership of the subasset you load? Automatic labelling doesn't seem worth the cost to me

This is totally a limitation. There is no way to retain ownership with this PR. With some more expansion of the NestedLoader API, we could support this for a custom Reader, but there's still weirdness with any subassets, so I'd probably like to avoid this. This isn't really about automatic labelling, it's more just fixing the completely broken state after #15481 (which itself was fixing the insanity of subassets). The automatic labelling idea is an alternative to this PR essentially.

@mcobzarenco This seems like a case where asset processing could help, is that something you could adopt, or does it not fill your needs in some way?

@andriyDev andriyDev requested a review from cart March 22, 2025 22:51
@andriyDev
Copy link
Contributor Author

andriyDev commented Mar 26, 2025

We've decided just to rollback #15481 instead. Here is the full discussion.

I believe we are likely to make moves in this direction in the future, but as we are already in the RC process, this isn't the time to be making such moves.

I will close this for now.

@cart cart moved this to Responded in @cart's attention Mar 27, 2025
@mcobzarenco
Copy link
Contributor

@mcobzarenco This seems like a case where asset processing could help, is that something you could adopt, or does it not fill your needs in some way?

Sorry for slow reply @andriyDev, it may help, not very familiar with all bevy offers in terms of assets.

  • Conceptually, in my mind, it would be preferable to opt out of registering "sub-assets" of a custom asset.
  • To make this concrete, say I want to generate a mesh from a heightmap image. I wouldn't add the image to Assets<Image>, I just load the image, process it to generate a mesh and only add the mesh to Assets<Mesh>.

Do you envision this use case being covered by asset processing?

@andriyDev
Copy link
Contributor Author

@mcobzarenco No worries! Thank you for following up!

Asset processing would work for this, but I don't think it's the best fit. This seems like a case to me where this PR would (have been) desirable. You don't actually need to own the image data at all, you want to generate a new asset based on the data of another. Note that once the image data gets passed to the asset system it will just get dropped because nothing holds a handle to it.

On the flip side, imagine your setup, but now we also want to render the heightmap image in the UI. Without this PR, you need to do two loads of the image data. However with this PR (closed but not forgotten), the mesh could load the image data, and then the UI could get the handle and have the mesh loader populate it. Now there's a single load of the image data!

This PR was definitely not fully formed (so closing was correct), but I believe we can and should recreate its goal in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: Responded
Development

Successfully merging this pull request may close these issues.

Need some way to nest labeled assets
3 participants