Retain asset metadata in RetainedAssets and check if extracted when accessing Assets#23871
Retain asset metadata in RetainedAssets and check if extracted when accessing Assets#23871beicause wants to merge 23 commits intobevyengine:mainfrom
RetainedAssets and check if extracted when accessing Assets#23871Conversation
This reverts commit 23cdfc4.
andriyDev
left a comment
There was a problem hiding this comment.
I haven't done a full review (yet), but just from a quick skim, this is leaking a lot of rendering details into the asset system, which I think is just wrong. Now calling Assets::::get returns a "maybe extracted", which may not be relevant at all to music stuff. The fact that rendering uses a render world and needs to extract data should not leak into literally every other asset type.
Personally I also don't like this "all or nothing" approach. For example, do we really need to remove all the data about an Image? For example, we could keep the size of the image on the Assets, which could be useful information even without having the actual image data (e.g. picking or something?), and it's tiny anyway. So making that data completely inaccessible just because the image data is extracted feels... not ideal.
There is |
Oh then I strongly disagree with this PR. This just brings us back to the previous state of "users just see their Mesh asset as suddenly missing when using RENDER_WORLD". This is exactly why we stopped just removing from the Assets resource - it yields unintuitive behavior and bad error messages. |
e8d6f1e to
a950d20
Compare
a950d20 to
c3875fc
Compare
|
Update: I added an associated type and |
…derAsset` Remove generics argument of EmptyRetainedAsset Fix build
|
I don't think I'm qualified to review this PR - it involves too many big design decisions on how the asset system and renderer work together. I think it needs SME review, and there probably should be a formal goal - something that can organise consensus on the long-term path to optimizing GPU upload and supporting streaming. Gonna throw out some quick thoughts anyway: I agree with this PR's objective of avoiding so many There's also a few variations on this. Maybe the mesh could be split into two assets, one of which is the GPU data to be extracted. Or maybe the full-fat assets-as-entities-with-multiple-components happens, and the GPU data can be a separate component on the mesh asset entity. Either way, there's a big design space, so it's best discussed in a working group. |
|
I think this is less about whether the mesh representation is GPU-optimal but more about how to deal with render world only usage and their metadata needed in the main world. |
|
Closing. I agree that this could be resolved if we have non-monolithic assets as entities, where it could be easier to split off and unload gpu data. |
Objective
Follow up to #21732
Currently we retain render-world-only assets in
Assets, and during extraction asset's gpu data is taken, which causes a lack of integrity guarantees when accessing assets and Mesh interface to provide manytry_*methods to avoid panics.Also
take_gpu_dataonly runs for render-world-only assets, which means for the other asset usages,Mesh::final_aabbisn't precomputed.Solution
The main design changes are in
bevy_assets/assets.rs,bevy_assets/render_asset.rs,bevy_render/{erased_}render_asset.rsAdd
ErasedRetainedAssetsresource andRenderAssets::retain_main_world_asset. When extracting, we create the metadata version of asset and add it toErasedRetainedAssets. This always runs for any asset usage. To opt out this, useEmptyRetainedAsset. UseRetainedAssets<A>system parameter to accessErasedRetainedAssets.Allow storing
Extractable<A>instead ofAinternally inAssetsso that we can extract data within it (though only the heap data withinAcan be deallocated). For Mesh, Image, ShaderBuffer,Assets::getwill return aExtractable<A>to check if it is extracted.All
try_*methods of Mesh are removed since it's impossible to panic if you can get a Mesh. And systems that need Meshfinal_aabborskinned_mesh_boundsnow useRetainedMesh.Whether Image is extracted is checked everywhere as well.
Testing
I tested
test_skinned_mesh_bounds,alter_sprite,mesh_picking