Retain asset without data for RENDER_WORLD-only assets

[robtfm]

Objective

when RenderAssets with RenderAssetUsages::RENDER_WORLD and without RenderAssetUsages::MAIN_WORLD are extracted, the asset is removed from the assets collection. this causes some issues:

• systems which rely on the asset, like picking with meshes, fail with "asset not found" errors which are unintuitive.
• loading the asset by path a second time results in the asset being reloaded from storage, re-extracted and re-transferred to gpu, replacing the existing asset
• knowledge about the asset state is lost, we cannot tell if an asset is already loaded with AssetServer::get_handle
• metadata (image size, e.g.) is no longer available for the asset

Solution

extraction:

• add take_gpu_data to the RenderAsset trait. use it to pull the data out of the asset for transfer, and leave the empty asset in the collection. default implementation just clones the asset.
• if the data has already been taken, panic. this follows from modifying an asset after extraction, which is always a code error, so i think panic here makes sense log an error

Mesh/RenderMesh:

• make Mesh::attributes and Mesh::indices options
• take them on extraction
• expect operations which access or modify the vertex data or indices if it has been extracted. accessing the vertex data after extraction is always a code error. fixes Somehow prevent confusion caused by Assets being removed due to not having RenderAssetUsages::MAIN_WORLD #19737 by resulting in the error Mesh has been extracted to RenderWorld. To access vertex attributes, the mesh must have RenderAssetUsages::MAIN_WORLD
• provide try_xxx operations which allow users to handle the access error gracefully if required (no usages as part of this pr, but provided for future)
• compute the mesh Aabb when gpu data is taken and store the result. this allows extracted meshes to still use frustum culling (otherwise using multiple copies of an extracted mesh now panics as compute_aabb relied on vertex positions). there's a bit of a tradeoff here: users may not need the Aabb and we needlessly compute it. but i think users almost always do want them, and computing once (for extracted meshes) is cheaper than the alternative, keeping position data and computing a fresh Aabb every time the mesh is used on a new entity.

Image/GpuImage:

images are a little more complex because the data can be deliberately None for render-targets / GPU-written textures where we only want an uninitialized gpu-side texture.

• take Image::data on extraction
• record on the resulting GpuImage whether any data was found initially
• on subsequent modifications with no data, panic if there was data previously

corner case / issue: when used with RenderAssetBytesPerFrameLimiter there may be no previous gpu asset if it is still queued pending upload due to the bandwidth limit. this can result in a modified image with initial data skipping the had_data check, resulting in a blank texture. i think this is sufficiently rare that it's not a real problem, users would still hit the panic if the asset is transferred in time and the problem/solution should be clear when they do hit it.

ShaderStorageBuffer/GpuShaderStorageBuffer

follows the same pattern as Image/GpuImage:

• take ShaderStorageBuffer::data on extraction
• record on the resulting GpuShaderStorageBuffer whether any data was found initially
• on modifications with no data, panic if there was data previously

we don't have the queue issue here because GpuShaderStorageBuffer doesn't implement byte_len so we can't end up queueing them.

other RenderAssets

i didn't modify the other RenderAsset types (GpuAutoExposureCompensationCurve, GpuLineGizmo, RenderWireframeMaterial, PreparedMaterial, PreparedMaterial2d, PreparedUiMaterial) on the assumption that cloning these is cheap enough anyway the asset usages are not exposed so we should never call take_gpu_data. the default implementation panics with a message directing users to implement the method if required

Testing

only really tested within my work project. i can add some explicit tests if required.

[hukasu]

I wonder if it would be better having an enum like this just to have some more information

enum RenderAssetData {
    Available(Vec<u8>),
    Unavailable,
    SentToRenderWorld
}

[robtfm]

maybe ... i tried to avoid changing the api of the main-world types (particularly Image::data which is pub) but maybe it's worthwhile

[alice-i-cecile]

Could we swap these methods to return Result and avoid the expects here?

[robtfm]

We could … it is a usage error though, rather than “something that can happen and you should deal with”, like result usually implies.

I don’t feel strongly, will change if you prefer it.

[andriyDev]

I don't think the intent is for a user to "deal with it", but rather "let's not crash the program because one asset has the wrong flags set". So I am in favour of making these Results.

[robtfm]

making these return results will add literally hundreds of unwraps to the code base, e.g. there are 104 from primitive mesh builder functions calling with_inserted_attribute which are all guaranteed to succeed since they operate on meshes they've just constructed themselves.

perhaps i can add try_xxx variants, so that if we find situations where there are panics that we don't want we have a way to manage them in future.

[andriyDev]

I don't think the intent is for a user to "deal with it", but rather "let's not crash the program because one asset has the wrong flags set". So I am in favour of making these Results.

[andriyDev]

Doc comment please! Also linking to RenderAsset::take_gpu_data would be good.

[andriyDev]

Should we even provide a default impl here? Cloning generally seems like the wrong behaviour to me, since I'd expect pretty much every impl to want to transfer the data rather than doing a possibly expensive clone.

[robtfm]

hm. i'm not sure the defaul impl gets called anywhere currently (i think the remaining RenderAsset types don't allow the user to set the usages), but then, what should we do here? panic in the trait impl or implement it explicitly for all the RenderAsset types with a panic or a clone, and require users to implement it explicitly as well even if they won't use it?

that doesn't seem helpful to me.

[robtfm]

i changed it so the default returns AssetExtractionError::NoImplementation

[andriyDev]

I much prefer making users implement this explicitly - pretty much every render asset should want to handle this correctly I think, especially since the default is just breaking.

[andriyDev]

All these mutation methods essentially stop functioning when the asset is extracted. What about runtime generated (and mutated) assets? Is the intent that users will like fully replace the Mesh with like *mesh_mut = Mesh::new()?

If so, this probably needs a migration guide.

[robtfm]

the pattern would be either to fully initialize in one step, or to initialize with RenderAssetUsages::MAIN_WORLD, make any modifications, then when it's finished change the usage to RenderAssetUsages::RENDER_WORLD. after that no more modifications are possible since the data is only on GPU and we can't get it back.

if users want to make continuous runtime modifications, then they should use MAIN_WORLD || RENDER_WORLD, or fully replacing will also work.

this PR doesn't change anything in those scenarios, modifying an asset with RENDER_WORLD and not MAIN_WORLD is not possible currently either since the asset gets removed from the Assets collection. i can write something if you like, but the usage doesn't change.

[andriyDev]

Why can't we just reupload here? We're losing a capability here I think, since previously hot-reloading an image would just send the data to the GPU, which would unconditionally upload. Am I missing something here?

Could you also test that hot reloading still works? I believe the alter_sprite example is probably sufficient with the file_watcher feature (and then messing with the loaded sprite).

[robtfm]

so the purpose of RenderAssetUsages::RENDER_WORLD is to reduce CPU memory by not keeping data on the CPU that is only required on the GPU. we clearly can't re-upload data from the CPU that we have chosen to purge.

as i described in the top comment, this check is specifically to catch an error which was not previously possible: where a user modifies some metadata on a RENDER_WORLD-only image after it has been transferred. that's not possible currently because currently the image gets removed from the CPU-side Assets collection so cannot be accidentally modified.

keeping access to it CPU side has the benefits i listed right at the top, but introduces this new way to break things, so i added this check to help anyone who does so accidentally. without this check, the image would be treated as an uninitialized texture -- for images with data: None we create an uninitialized GPU-texture which is much faster -- and the GPU pixel data would be zeros, which will never be what the user wants if the image had data when first transferred.

we're not losing any capabilities. default is MAIN_WORLD | RENDER_WORLD, in which case you can modify and (re)upload as much as you like because we keep the pixel data CPU side.

hot reloading still works for MAIN_WORLD | RENDER_WORLD and for RENDER_WORLD-only as reloading an asset reloads the data too.

[robtfm]

• added MeshExtractableData enum to avoid the nasty nested options / make the intent clearer.
• added try_xxx functions to avoid 100s of unwraps in the cases where we know the access is valid (and reduce the breaking changes to the public api).
• changed the extract functions to return an error, and the default impl to return AssetExtractionError::NoExtractionImplementation.

i still need to do a pass through to fix up docs for all the copy/pasted functions, but the logic should be complete now i think.

[greeble-dev]

Code seems reasonable - just got a couple of requests to avoid some corner cases. I also tested a variety of examples, and hacked in a few extra tests (including glTFs with RENDER_WORLD only, and making some examples deliberately trigger errors).

I don't like how the Mesh interface has ended up, but I'm not sure that's a problem with this PR. My hot take is that it's a problem with Mesh trying to be three things at once:

1. A finished, immutable asset that can be efficiently shared and rendered.
2. A way to incrementally build a mesh by adding/modifying attributes and suchlike.
3. A way to animate meshes by mutating an asset in-place and re-extracting.

I think the solution could be:

• Add a new MeshBuilder struct and move all the Mesh mutation methods to MeshBuilder.
  • This struct wouldn't have to deal with MeshAccess errors since it's entirely CPU side, so a load of try_xxx methods disappear.
  • A finish function would consume the builder and return a Mesh.
• Change Mesh to be immutable, except by take_gpu_data.
  • Animation that wants to mutate the mesh now has to create a fresh Mesh and overwrite the entire asset.

That gives as clear separation between 1) and 2), but makes 3) more awkward and inefficient. I'd argue that's ok in the short-term since it still basically works and mutating the asset is always going to be somewhat inefficient. In the long-term there could be a separate way of efficiently sending fine-grained mesh updates to the GPU.

The Mesh/MeshBuilder separation might also make compressed and derived data (like AABBs) more robust and efficient. So MeshBuilder::finish would automatically calculate AABBs and optionally apply compression.

EDIT: I should have said that I don't think this PR needs to explore stuff like MeshBuilder. What the PR does right now - make the Mesh interface more awkward in the short-term - seems like a reasonable trade-off for solving RENDER_WORLD issues now.

[greeble-dev]

Suggested change

	}) = attributes.as_ref()?.get(&Self::ATTRIBUTE_POSITION.id)
	{
	}) = attributes.as_ref()?.get(&Self::ATTRIBUTE_POSITION.id)
	&& !position_values.is_empty()
	{

Not 100% sure it's actually possible to make and extract a mesh with an empty position attribute, but seems reasonable to avoid a panic here just in case.

[robtfm]

i'll change this to not bubble the error here, just fail to calc the aabb. then the only "?" error will be AlreadyExtracted. that should solve this and the below issue.

it's legitimate to extract a mesh with no positions, i guess, if you are doing some custom processing that doesn't use positions? maybe?

[robtfm]

sorry i misunderstood - i see now why this is required and added it

[greeble-dev]

If Mesh::take_gpu_data returns MeshAccessError::NotFound (e.g. because the mesh has no attributes), then this logic will wrongly report AlreadyExtracted. I repro'd this by changing the generate_custom_mesh example to make a mesh with no attributes.

I'm not sure what the solution is - maybe AssetExtractionError should also have a discriminant for missing data.

[robtfm]

it would be nice to have a MeshBuilder with infallible access methods. then all the Mesh accessors could be result-based without scattering 100s of unwraps about.

i don't think i want to do it as part of this PR though.

[greeble-dev]

Ah, my bad, I should have been clearer that I think this PR is fine as it is. It would have been simpler if MeshBuilder already existed, but c'est la vie.

[greeble-dev]

I'm clicking approve because the PR solves an immediate problem that I've seen a few people trip over. It will also unblock #21837. The Mesh interface ends up a bit awkward right now, but there's room to improve it in future (see #21986). I think the PR still needs a migration guide though?

EDIT: Minor correction - maybe this PR can skip the migration guide, assuming #21986 lands in the same release and has a migration that covers both? Would keep things simpler.

[beicause]

Code seems reasonable - just got a couple of requests to avoid some corner cases. I also tested a variety of examples, and hacked in a few extra tests (including glTFs with RENDER_WORLD only, and making some examples deliberately trigger errors).

I don't like how the Mesh interface has ended up, but I'm not sure that's a problem with this PR. My hot take is that it's a problem with Mesh trying to be three things at once:

1. A finished, immutable asset that can be efficiently shared and rendered.
2. A way to incrementally build a mesh by adding/modifying attributes and suchlike.
3. A way to animate meshes by mutating an asset in-place and re-extracting.

I think the solution could be:

• Add a new MeshBuilder struct and move all the Mesh mutation methods to MeshBuilder.

  • This struct wouldn't have to deal with MeshAccess errors since it's entirely CPU side, so a load of try_xxx methods disappear.
  • A finish function would consume the builder and return a Mesh.

• Change Mesh to be immutable, except by take_gpu_data.

  • Animation that wants to mutate the mesh now has to create a fresh Mesh and overwrite the entire asset.

That gives as clear separation between 1) and 2), but makes 3) more awkward and inefficient. I'd argue that's ok in the short-term since it still basically works and mutating the asset is always going to be somewhat inefficient. In the long-term there could be a separate way of efficiently sending fine-grained mesh updates to the GPU.

The Mesh/MeshBuilder separation might also make compressed and derived data (like AABBs) more robust and efficient. So MeshBuilder::finish would automatically calculate AABBs and optionally apply compression.

I totally agree with @greeble-dev.
Related to #17000. My quick thoughts are moving all the current Mesh methods to something like InfallibleMesh, too and making Mesh only contains packed vertex+indices data for GPU and necessary info about attributes layout, morph targets, aabb, etc.

So copying vertex and indices to MeshAllocator is fast and no longer needs repacking data. This opens a door to direct constructing Mesh and mutating Mesh in-place if you have the raw vertex and indices data and info and want to avoid repacking.

I think the current approach of extracting vertex attributes is suboptimal. And Mesh and InfallibleMesh have many identical methods, making things confusing.

Edit: Oh, think again, I can see the reason to store attributes on the Mesh since mesh_picking or physics engine may need to read the Mesh’s vertex positions so we can’t just store interleaved vertex attributes. In that case, the above is less relevant and I think the current PR is fine.
See discussion in #21986

[beicause]

Regarding the Mesh interface, I‘d prefer to remove these try_xxx methods and let them panic. Adding a is_extracted_to_render_world method for checking is sufficient IMO, these try_xxx methods are rarely useful.

[robtfm]

Regarding the Mesh interface, I‘d prefer to remove these try_xxx methods and let them panic. Adding a is_extracted_to_render_world method for checking is sufficient IMO, these try_xxx methods are rarely useful.

since the change to results was at request of @andriyDev (and @alice-i-cecile) i'll defer to them. i agree fwiw

[beicause]

These extractable data can be consolidated. This ensures you always extract them together and that they are either all with data or without data simultaneously. Like this:

struct MeshExtractableData {
    attributes: BTreeMap<MeshVertexAttributeId, MeshAttributeData>,
    indices: Option<Indices>,
    #[cfg(feature = "morph")]
    morph_targets: Option<Handle<Image>>,
    #[cfg(feature = "morph")]
    morph_target_names: Option<Vec<String>>,
}

struct Mesh {
    ...
    extractable_data: Option<MeshExtractableData>,
    ...
}

[robtfm]

could be. i don't think it improves anything but i can change it if others disagree.

[IceSentry]

The new _option function feel a bit weird but I don't really have an alternative solution. I mean it could either always return the option or always map the option to a result by it's not a blocker.

LGTM

[IceSentry]

And yeah, I prefer keeping the try_ function that return results. We should generally try to avoid panics as much as possible. I've hit way too many unexpected panics in rendering code. Arguably, we shouldn't even have a panicky variant but that's a completely different discussion.

[andriyDev]

We should make these all doc comments.

[robtfm]

do we typically use doc comments for private methods on private structs? i will change if so but i didn't think it was standard.

[alice-i-cecile]

We typically do, as we have documentation built for private items on dev-docs.bevy.org.

[andriyDev]

I much prefer making users implement this explicitly - pretty much every render asset should want to handle this correctly I think, especially since the default is just breaking.

[andriyDev]

I'm a little worried that we're silently mutating the asset. Is this cause for concern?

[IceSentry]

I would argue it's kinda part of the contract of using the render world asset usage but I can see why some people wouldn't like it. I'm not sure what the alternative is unless we can communicate more details about why an asset changed. If a user could react to this change right now there would be no way to know if it was a user event or the render world extraction. I guess if they look at the asset and see missing data they would know that's what hapenned.

[robtfm]

i used get_mut initially, but it ended up in a broken feedback loop since re-extraction is triggered by asset modifications (which is required behaviour for intentional user modifications), including this one.

the alternative would be to use a get_mut variant that fires a different event i guess. but the behaviour of extraction is predictable enough -- the data will be removed in the extract stage -- that i don't think it's warranted.

[robtfm]

I much prefer making users implement this explicitly - pretty much every render asset should want to handle this correctly I think, especially since the default is just breaking.

there are several RenderAssets that don't implement it and don't need to because their RenderAssetUsages are not exposed. it's not useful to make authors add it to types that don't support it, as i mentioned last time.

[beicause]

And yeah, I prefer keeping the try_ function that return results. We should generally try to avoid panics as much as possible. I've hit way too many unexpected panics in rendering code. Arguably, we shouldn't even have a panicky variant but that's a completely different discussion.

My concern is that if we add more extractable resources (like RawMesh, etc.), we will also follow a similar pattern of providing try_xxx functions, which feels really bad. Also, If the extractable data isn’t consolidated, we have to check each field individually to determine if a resource data is complete, it's inconvenient.

[robtfm]

we will also follow a similar pattern of providing try_xxx functions

you've seen the InfallibleMesh pr i made on top of this, right? there it would still be an extra unwrap() on the base function, but wouldn't have any duplicated try_ functions.

[beicause]

Another possible approach I consider is to consolidate the extractable data and access it through something similar to entry API, thereby avoiding multiple unwrap operations.

[beicause]

we will also follow a similar pattern of providing try_xxx functions

you've seen the InfallibleMesh pr i made on top of this, right? there it would still be an extra unwrap() on the base function, but wouldn't have any duplicated try_ functions.

That is unrelated. I mean if more extractable assets are added in the future, or if crate authors want to add their extractable assets, should the interfaces also follow the try_xxx+xxx pattern? I think this is an area that could be improved.