-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
This PR is based of #18213. Once it's merged, I will rebase onto main. |
…nd return a reference type.
@@ -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>); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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? |
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. |
Sorry for slow reply @andriyDev, it may help, not very familiar with all bevy offers in terms of assets.
Do you envision this use case being covered by asset processing? |
@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. |
Objective
Solution
LoadContext
. Users are only given aNestedAssetRef
/NestedErasedAssetRef
which is merely an immutable reference to the assets.Testing
load_dependencies
test to account for the fact that nested immediate loaded assets now modify the previously loaded assets.Risks
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 aNestedAssetRef
/NestedErasedAssetRef
to access its data. Previously, nested asset loads could look like:Now these nested assets are "registered" for you.