Use async-fn-in-trait (RPIT) for various Asset traits. #11362
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
Note for reviewers: Much easier to review if you hide the whitespace diff. |
JMS55
added
A-Assets
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
|
Ah didn't realise Bevy MSRV isn't 1.75 yet, that makes this a bit pointless. Also WASM doesn't build - trait fns need to be optionally Send, only on non-wasm platforms. That's... tricky. Maybe too early for async-fn-in-trait after all, but, will do some reading on what the state of things is there currently. |
Shouldn't be a blocker, I think we can bump it. |
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
1 similar comment
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
Oh ok great! Would that be a seperate PR or included in this? Given the insistent bot I've added it in here. That leaves the WASM problem. The recap, mainly for myself:
The potential solutions as far as I can see:
So yeah, not sure, might have hit a dead end for wasm! |
|
This PR is fine for bumping MSRV. I don't have time to think about WASM atm, will let someone else respond. Feel free to join the -dev channels in discord (engine-dev/asset-dev) and brainstorm solutions with other people though :) |
|
No issues to update the MSRV, this is usually done in the first PR that needs the new version of Rust. I would love to have this, but I also think this is blocked for Wasm for now 🙁 |
|
So it seems there is a way to make conditional Send bounds, and more people do it this way. Have made an attempt with a "FutureSend" (could be MaybeSend? ConditionalSend?) that seems to work for wasm! Also tried to understand where this problem really comes from, and it basically comes down to: wasm_bindgen uses JS promises which aren't Send for futures for web APIs, including Fetch, which is why the HttpWasmAssetReader can't have Send futures. That really seems to be the only reason wasm needs this special treatment, though of course that propogates all the way through. Let me know if this seems workable, still happy to say this is getting too weird and it should wait for rust to stabilise more async features, or idk use #[async_trait] instead and call it a day haha. Thanks! |
There was a problem hiding this comment.
I'm on board for these changes! Love the simplified AssetLoader interface.
The FutureSend pattern doesn't bother me at all. It feels like a natural extension of the BoxedFuture approach we're using. It is also just an "internal" change to the trait declaration. I like that users don't need to think about it.
The only place this feels like a potential downgrade to me is ErasedAssetReader/Writer, and the need for them to show up in a few public apis in place of AssetReader. The trait split allows for async syntax when implementing AssetReaders/Writers. This is an ergonomics win, but implementing the traits is largely something that we do internally. It is a very uncommon thing for a user to do. Given that the split comes at the cost of additional complexity for consumers of the traits (which feels like it could be more common), that does feel like a high price to pay. However in practice, these traits are largely abstracted out from consumers. In general I feel neutral on this change, so I'm happy to accept it as it is.
| basis_universal::ColorSpace::Linear | ||
|
|
||
| let compressed_basis_data = { | ||
| // PERF: this should live inside the future, but CompressorParams and Compressor are not Send / can't be owned by the BoxedFuture (which _is_ Send) |
There was a problem hiding this comment.
We can remove this as this does live "inside" the future now. Intersting that this pattern works!
There was a problem hiding this comment.
Opps nice catch! And yeah annoying that the extra scope is needed (known problem appearantly) but is cool that it works at all! Maybe future borrow checker one day :)
|
Thanks a lot for that! Really nice to hear your design thoughts :) Great that the Wasm solution sounds workable! I've renamed FutureSend to WasmNotSend as per giooschi's suggestion on Discord to match WGPU. And yeah ErasedX is the main actual visible usability downside... Good point that that's maybe more what is exposed to the user even :/ My hope was that since a bunch of the other Asset traits already have these it's not a whole other concep to lear. Really annoying that #[async-trait] CAN do without this but 'native' RPITIT can't! From yosh & boats blog seems the async-WG thinks dyn + async traits might happen in 2024, and could get rid of all these wrappers. |
Not a huge deal, but I'd prefer it if we made the naming a bit more generic to avoid having "wasm" in the The module name seems like a good choice: |
|
Oh neat! Yeah prefer that too, made it |
|
Have updated to latest main in case this is still the way to go :) Some weird CI failures atm that I think are unrelated, other PRs seem to have the same. |
# Objective - The exported hashtypes are just re-exports from hashbrown, we want to drop that dependency and (in the future) let the user import their own choice. - Fixes bevyengine#11717 ## Solution - Adding a deprecated tag on the re-exports, so in future releases these can be safely removed.
# Objective Split up from bevyengine#11007, fixing most of the remaining work for bevyengine#10569. Implement `Meshable` for `Cuboid`, `Sphere`, `Cylinder`, `Capsule`, `Torus`, and `Plane3d`. This covers all shapes that Bevy has mesh structs for in `bevy_render::mesh::shapes`. `Cone` and `ConicalFrustum` are new shapes, so I can add them in a follow-up, or I could just add them here directly if that's preferrable. ## Solution Implement `Meshable` for `Cuboid`, `Sphere`, `Cylinder`, `Capsule`, `Torus`, and `Plane3d`. The logic is mostly just a copy of the the existing `bevy_render` shapes, but `Plane3d` has a configurable surface normal that affects the orientation. Some property names have also been changed to be more consistent. The default values differ from the old shapes to make them a bit more logical: - Spheres now have a radius of 0.5 instead of 1.0. The default capsule is equivalent to the default cylinder with the sphere's halves glued on. - The inner and outer radius of the torus are now 0.5 and 1.0 instead of 0.5 and 1.5 (i.e. the new minor and major radii are 0.25 and 0.75). It's double the width of the default cuboid, half of its height, and the default sphere matches the size of the hole. - `Cuboid` is 1x1x1 by default unlike the dreaded `Box` which is 2x1x1. Before, with "old" shapes:  Now, with primitive meshing:  I only changed the `3d_shapes` example to use primitives for now. I can change them all in this PR or a follow-up though, whichever way is preferrable. ### Sphere API Spheres have had separate `Icosphere` and `UVSphere` structs, but with primitives we only have one `Sphere`. We need to handle this with builders: ```rust // Existing structs let ico = Mesh::try_from(Icophere::default()).unwrap(); let uv = Mesh::from(UVSphere::default()); // Primitives let ico = Sphere::default().mesh().ico(5).unwrap(); let uv = Sphere::default().mesh().uv(32, 18); ``` We could add methods on `Sphere` directly to skip calling `.mesh()`. I also added a `SphereKind` enum that can be used with the `kind` method: ```rust let ico = Sphere::default() .mesh() .kind(SphereKind::Ico { subdivisions: 8 }) .build(); ``` The default mesh for a `Sphere` is an icosphere with 5 subdivisions (like the default `Icosphere`). --- ## Changelog - Implement `Meshable` and `Default` for `Cuboid`, `Sphere`, `Cylinder`, `Capsule`, `Torus`, and `Plane3d` - Use primitives in `3d_shapes` example --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
Bevy could benefit from *irradiance volumes*, also known as *voxel
global illumination* or simply as light probes (though this term is not
preferred, as multiple techniques can be called light probes).
Irradiance volumes are a form of baked global illumination; they work by
sampling the light at the centers of each voxel within a cuboid. At
runtime, the voxels surrounding the fragment center are sampled and
interpolated to produce indirect diffuse illumination.
## Solution
This is divided into two sections. The first is copied and pasted from
the irradiance volume module documentation and describes the technique.
The second part consists of notes on the implementation.
### Overview
An *irradiance volume* is a cuboid voxel region consisting of
regularly-spaced precomputed samples of diffuse indirect light. They're
ideal if you have a dynamic object such as a character that can move
about
static non-moving geometry such as a level in a game, and you want that
dynamic object to be affected by the light bouncing off that static
geometry.
To use irradiance volumes, you need to precompute, or *bake*, the
indirect
light in your scene. Bevy doesn't currently come with a way to do this.
Fortunately, [Blender] provides a [baking tool] as part of the Eevee
renderer, and its irradiance volumes are compatible with those used by
Bevy.
The [`bevy-baked-gi`] project provides a tool, `export-blender-gi`, that
can
extract the baked irradiance volumes from the Blender `.blend` file and
package them up into a `.ktx2` texture for use by the engine. See the
documentation in the `bevy-baked-gi` project for more details as to this
workflow.
Like all light probes in Bevy, irradiance volumes are 1×1×1 cubes that
can
be arbitrarily scaled, rotated, and positioned in a scene with the
[`bevy_transform::components::Transform`] component. The 3D voxel grid
will
be stretched to fill the interior of the cube, and the illumination from
the
irradiance volume will apply to all fragments within that bounding
region.
Bevy's irradiance volumes are based on Valve's [*ambient cubes*] as used
in
*Half-Life 2* ([Mitchell 2006], slide 27). These encode a single color
of
light from the six 3D cardinal directions and blend the sides together
according to the surface normal.
The primary reason for choosing ambient cubes is to match Blender, so
that
its Eevee renderer can be used for baking. However, they also have some
advantages over the common second-order spherical harmonics approach:
ambient cubes don't suffer from ringing artifacts, they are smaller (6
colors for ambient cubes as opposed to 9 for spherical harmonics), and
evaluation is faster. A smaller basis allows for a denser grid of voxels
with the same storage requirements.
If you wish to use a tool other than `export-blender-gi` to produce the
irradiance volumes, you'll need to pack the irradiance volumes in the
following format. The irradiance volume of resolution *(Rx, Ry, Rz)* is
expected to be a 3D texture of dimensions *(Rx, 2Ry, 3Rz)*. The
unnormalized
texture coordinate *(s, t, p)* of the voxel at coordinate *(x, y, z)*
with
side *S* ∈ *{-X, +X, -Y, +Y, -Z, +Z}* is as follows:
```text
s = x
t = y + ⎰ 0 if S ∈ {-X, -Y, -Z}
⎱ Ry if S ∈ {+X, +Y, +Z}
⎧ 0 if S ∈ {-X, +X}
p = z + ⎨ Rz if S ∈ {-Y, +Y}
⎩ 2Rz if S ∈ {-Z, +Z}
```
Visually, in a left-handed coordinate system with Y up, viewed from the
right, the 3D texture looks like a stacked series of voxel grids, one
for
each cube side, in this order:
| **+X** | **+Y** | **+Z** |
| ------ | ------ | ------ |
| **-X** | **-Y** | **-Z** |
A terminology note: Other engines may refer to irradiance volumes as
*voxel
global illumination*, *VXGI*, or simply as *light probes*. Sometimes
*light
probe* refers to what Bevy calls a reflection probe. In Bevy, *light
probe*
is a generic term that encompasses all cuboid bounding regions that
capture
indirect illumination, whether based on voxels or not.
Note that, if binding arrays aren't supported (e.g. on WebGPU or WebGL
2),
then only the closest irradiance volume to the view will be taken into
account during rendering.
[*ambient cubes*]:
https://advances.realtimerendering.com/s2006/Mitchell-ShadingInValvesSourceEngine.pdf
[Mitchell 2006]:
https://advances.realtimerendering.com/s2006/Mitchell-ShadingInValvesSourceEngine.pdf
[Blender]: http://blender.org/
[baking tool]:
https://docs.blender.org/manual/en/latest/render/eevee/render_settings/indirect_lighting.html
[`bevy-baked-gi`]: https://github.com/pcwalton/bevy-baked-gi
### Implementation notes
This patch generalizes light probes so as to reuse as much code as
possible between irradiance volumes and the existing reflection probes.
This approach was chosen because both techniques share numerous
similarities:
1. Both irradiance volumes and reflection probes are cuboid bounding
regions.
2. Both are responsible for providing baked indirect light.
3. Both techniques involve presenting a variable number of textures to
the shader from which indirect light is sampled. (In the current
implementation, this uses binding arrays.)
4. Both irradiance volumes and reflection probes require gathering and
sorting probes by distance on CPU.
5. Both techniques require the GPU to search through a list of bounding
regions.
6. Both will eventually want to have falloff so that we can smoothly
blend as objects enter and exit the probes' influence ranges. (This is
not implemented yet to keep this patch relatively small and reviewable.)
To do this, we generalize most of the methods in the reflection probes
patch bevyengine#11366 to be generic over a trait, `LightProbeComponent`. This
trait is implemented by both `EnvironmentMapLight` (for reflection
probes) and `IrradianceVolume` (for irradiance volumes). Using a trait
will allow us to add more types of light probes in the future. In
particular, I highly suspect we will want real-time reflection planes
for mirrors in the future, which can be easily slotted into this
framework.
## Changelog
> This section is optional. If this was a trivial fix, or has no
externally-visible impact, you can delete this section.
### Added
* A new `IrradianceVolume` asset type is available for baked voxelized
light probes. You can bake the global illumination using Blender or
another tool of your choice and use it in Bevy to apply indirect
illumination to dynamic objects.
# Objective - System `create_surfaces` needs to happen before `prepare_windows` or we lose one frame at startup ## Solution - Specify the ordering, remove the set as it doesn't mean anything there
# Objective - Fixes bevyengine#11740 ## Solution - Turned `Mesh::set_indices` into `Mesh::insert_indices` and added related methods for completeness. --- ## Changelog - Replaced `Mesh::set_indices(indices: Option<Indices>)` with `Mesh::insert_indices(indices: Indices)` - Replaced `Mesh::with_indices(indices: Option<Indices>)` with `Mesh::with_inserted_indices(indices: Indices)` and `Mesh::with_removed_indices()` mirroring the API for inserting / removing attributes. - Updated the examples and internal uses of the APIs described above. ## Migration Guide - Use `Mesh::insert_indices` or `Mesh::with_inserted_indices` instead of `Mesh::set_indices` / `Mesh::with_indices`. - If you have passed `None` to `Mesh::set_indices` or `Mesh::with_indices` you should use `Mesh::remove_indices` or `Mesh::with_removed_indices` instead. --------- Co-authored-by: François <mockersf@gmail.com>
# Objective Fix bevyengine#11657 ## Solution Add a `ReflectKind` enum, add `Reflect::reflect_kind` which returns a `ReflectKind`, and add `kind` method implementions to `ReflectRef`, `ReflectMut`, and `ReflectOwned`, which returns a `ReflectKind`. I also changed `AccessError` to use this new struct instead of it's own `TypeKind` struct. --- ## Changelog - Added `ReflectKind`, an enumeration over the kinds of a reflected type without its data. - Added `Reflect::reflect_kind` (with default implementation) - Added implementation for the `kind` method on `ReflectRef`, `ReflectMut`, and `ReflectOwned` which gives their kind without any information, as a `ReflectKind`
… views. (bevyengine#11751) Don't try to create a uniform buffer for light probes if there are no views. Fixes the panic on examples that have no views, such as `touch_input_events`.
…1758) # Objective - This aims to fix bevyengine#11755 - After bevyengine#10812 some pipeline compilation can take more time than before and all call to `get_render_pipeline` should check the result. ## Solution - Check `get_render_pipeline` call result for msaa_writeback - I checked that no other call to `get_render_pipeline` in bevy code base is missng the checking on the result.
[`ScheduleLabel`] derive macro uses "ScheduleName" as the trait name by mistake. This only affects the error message when a user tries to use the derive macro on a union type. No other code is affected.
… message)" This reverts commit d710987. Github CI doesn't agree with my local outputs? No clue, quite strange.
Remove comment about CompressorParams needing to be in the future (it is now!)
and bad merges woops.
… into async-post-RPIT
|
Wellll seems like I fucked the merge pretty bad :') I'll redo this PR at head, it was getting a bit messy anyway. |
# Objective
Simplify implementing some asset traits without Box::pin(async move{})
shenanigans.
Fixes (in part) #11308
## Solution
Use async-fn in traits when possible in all traits. Traits with return
position impl trait are not object safe however, and as AssetReader and
AssetWriter are both used with dynamic dispatch, you need a Boxed
version of these futures anyway.
In the future, Rust is [adding
](https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html)proc
macros to generate these traits automatically, and at some point in the
future dyn traits should 'just work'. Until then.... this seemed liked
the right approach given more ErasedXXX already exist, but, no clue if
there's plans here! Especially since these are public now, it's a bit of
an unfortunate API, and means this is a breaking change.
In theory this saves some performance when these traits are used with
static dispatch, but, seems like most code paths go through dynamic
dispatch, which boxes anyway.
I also suspect a bunch of the lifetime annotations on these function
could be simplified now as the BoxedFuture was often the only thing
returned which needed a lifetime annotation, but I'm not touching that
for now as traits + lifetimes can be so tricky.
This is a revival of
[pull/11362](#11362) after a
spectacular merge f*ckup, with updates to the latest Bevy. Just to recap
some discussion:
- Overall this seems like a win for code quality, especially when
implementing these traits, but a loss for having to deal with ErasedXXX
variants.
- `ConditionalSend` was the preferred name for the trait that might be
Send, to deal with wasm platforms.
- When reviewing be sure to disable whitespace difference, as that's 95%
of the PR.
## Changelog
- AssetReader, AssetWriter, AssetLoader, AssetSaver and Process now use
async-fn in traits rather than boxed futures.
## Migration Guide
- Custom implementations of AssetReader, AssetWriter, AssetLoader,
AssetSaver and Process should switch to async fn rather than returning a
bevy_utils::BoxedFuture.
- Simultaniously, to use dynamic dispatch on these traits you should
instead use dyn ErasedXXX.
# Objective
Simplify implementing some asset traits without Box::pin(async move{})
shenanigans.
Fixes (in part) bevyengine/bevy#11308
## Solution
Use async-fn in traits when possible in all traits. Traits with return
position impl trait are not object safe however, and as AssetReader and
AssetWriter are both used with dynamic dispatch, you need a Boxed
version of these futures anyway.
In the future, Rust is [adding
](https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html)proc
macros to generate these traits automatically, and at some point in the
future dyn traits should 'just work'. Until then.... this seemed liked
the right approach given more ErasedXXX already exist, but, no clue if
there's plans here! Especially since these are public now, it's a bit of
an unfortunate API, and means this is a breaking change.
In theory this saves some performance when these traits are used with
static dispatch, but, seems like most code paths go through dynamic
dispatch, which boxes anyway.
I also suspect a bunch of the lifetime annotations on these function
could be simplified now as the BoxedFuture was often the only thing
returned which needed a lifetime annotation, but I'm not touching that
for now as traits + lifetimes can be so tricky.
This is a revival of
[pull/11362](bevyengine/bevy#11362) after a
spectacular merge f*ckup, with updates to the latest Bevy. Just to recap
some discussion:
- Overall this seems like a win for code quality, especially when
implementing these traits, but a loss for having to deal with ErasedXXX
variants.
- `ConditionalSend` was the preferred name for the trait that might be
Send, to deal with wasm platforms.
- When reviewing be sure to disable whitespace difference, as that's 95%
of the PR.
## Changelog
- AssetReader, AssetWriter, AssetLoader, AssetSaver and Process now use
async-fn in traits rather than boxed futures.
## Migration Guide
- Custom implementations of AssetReader, AssetWriter, AssetLoader,
AssetSaver and Process should switch to async fn rather than returning a
bevy_utils::BoxedFuture.
- Simultaniously, to use dynamic dispatch on these traits you should
instead use dyn ErasedXXX.
(First OSS contribution, so apologies if I mess up the format, roll-ups, or err the rest! Picked a random issue to see what it's like)
Objective
Simplify implementing some asset traits without Box::pin(async move{}) shenanigans.
Fixes (in part) #11308
Solution
Use async-fn in traits when possible in all traits. Traits with return position impl trait are not object safe however, and as AssetReader and AssetWriter are both used with dynamic dispatch, you need a Boxed version of these futures anyway.
In the future, Rust is adding proc macros to generate these traits automatically, and at some point in the future
dyn traitsshould 'just work'. Until then.... this seemed liked the right approach given more ErasedXXX already exist, but, no clue if there's plans here! Especially since these are public now, it's a bit of an unfortunate API, and means this is a breaking change.In theory this saves some performance when these traits are used with static dispatch, but, seems like most code paths go through dynamic dispatch which boxes anyway.
Changelog
Migration Guide
async fnrather than returningbevy_utils::BoxedFuture. Simultaniously, to use dynamic dispatch on these traits you should instead usedyn ErasedXXX.