Commit 5f7e43c
Retain asset without data for RENDER_WORLD-only assets (#21732)
# 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 `clone`s 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 #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.
---------
Co-authored-by: Jasmine S <[email protected]>
Co-authored-by: Greeble <[email protected]>1 parent 2e9ef69 commit 5f7e43c
File tree
11 files changed
+1155
-128
lines changed- crates
- bevy_asset/src
- bevy_camera/src
- bevy_mesh/src
- bevy_pbr/src/render
- bevy_render/src
- mesh
- texture
11 files changed
+1155
-128
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
8 | | - | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
9 | 10 | | |
10 | | - | |
11 | | - | |
12 | | - | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
21 | | - | |
22 | | - | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
23 | 28 | | |
24 | 29 | | |
25 | 30 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| 9 | + | |
| 10 | + | |
9 | 11 | | |
10 | 12 | | |
11 | 13 | | |
| |||
56 | 58 | | |
57 | 59 | | |
58 | 60 | | |
| 61 | + | |
| 62 | + | |
59 | 63 | | |
60 | 64 | | |
61 | 65 | | |
| |||
64 | 68 | | |
65 | 69 | | |
66 | 70 | | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | 71 | | |
71 | 72 | | |
72 | 73 | | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | 74 | | |
77 | 75 | | |
| 76 | + | |
| 77 | + | |
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| |||
0 commit comments