Implement motion vectors and TAA for skinned meshes and meshes with morph targets.#13572
Conversation
morph targets. This is a revamped version of bevyengine#9902. Instead of adding more bind group layouts as that patch did, which created a combinatorial explosion of layouts, this patch unconditionally adds `prev_joint_matrices` and `prev_morph_weights` bindings to the shader bind groups. These add no significant overhead if unused because we simply bind dummy buffers, and the driver strips them out if unused. We already do this extensively with the various `StandardMaterial` bindings as well as the mesh view bindings, so this approach isn't anything new. The overall technique consists of double-buffering the joint matrix and morph weights buffers, as most of the previous attempts to solve this problem did. The process is generally straightforward. Note that, to avoid regressing the ability of mesh extraction, skin extraction, and morph target extraction to run in parallel, I had to add a new system to rendering, `set_mesh_motion_vector_flags`. The comment there explains the details; it generally runs very quickly. I've tested this with modified versions of the `animated_fox`, `morph_targets`, and `many_foxes` examples that add TAA, and the patch works. To avoid bloating those examples, I didn't add switches for TAA to them. Addresses points (1) and (2) of bevyengine#8423.
c75eecd to
83d376c
Compare
aevyrie
left a comment
There was a problem hiding this comment.
Closes an important gap in bevy, very little added code, this is largely documentation - the code quality here is great. The simplifying assumptions here w.r.t. the buffers seems well justified. All-in-all this should be pretty uncontroversial.
| // Compute the motion vector, for TAA. For this we need to know where the | ||
| // vertex was last frame. | ||
| #ifdef MOTION_VECTOR_PREPASS | ||
| // Use vertex_no_morph.instance_index instead of vertex.instance_index to work around a wgpu dx12 bug. |
There was a problem hiding this comment.
Is the vertex_no_morph.instance_index workaround no longer valid?
There was a problem hiding this comment.
I just tried animated_fox with TAA enabled on the Direct3D 12 backend and it works fine. I guess whatever the issue was is now fixed. Or my changes to the shaders perturbed the bug out of existence.
| let skin = skins_uniform.current_buffer.buffer(); | ||
| if let Some(skin) = skin { | ||
| groups.skinned = Some(layouts.skinned(&render_device, &model, skin)); | ||
| let prev_skin = skins_uniform.prev_buffer.buffer().unwrap_or(skin); |
There was a problem hiding this comment.
We can bind the same buffer twice? I would've thought you needed a dummy buffer. Maybe that rule is only for storage resources, and does not apply to uniform buffers?
There was a problem hiding this comment.
I didn't think there was any kind of rule against binding a read-only buffer twice. I can see problems with read-write buffers, but read-only seems always fine. It's extremely common for textures, so I don't see why it wouldn't be for buffers...
| // Borrow check workaround. | ||
| let (morph_indices, uniform) = (morph_indices.into_inner(), uniform.into_inner()); | ||
|
|
||
| // Swap buffers. We need to keep the previous frame's buffer around for the |
There was a problem hiding this comment.
I think your way is a bit cleaner, but this is how I did it in my PR https://github.com/JMS55/bevy/blob/3334d0bd5ff643f54a605672a70d05b6b271b455/crates/bevy_pbr/src/render/morph.rs#L93-L110. Can't remember why they look different off the top of my head.
|
This is approximately a 2%-4% regression in CPU time of CPU 13th Gen Intel(R) Core(TM) i9-13900H Mac mini, M2 I'd argue that this small regression is worth it. Rationale: This is a worst-case stress test, and real-world games will typically not have that many skinned meshes. There should be no regressions for non-skinned meshes. We can also fix the regressions in the future in a variety of ways. TAA for skinned meshes is such a commonly-requested feature that it seems worthwhile to have it. |
|
The regression is somewhat worse--13% or so for Frame time is a 5.73% regression: I still think we should take this regression as this is still not very large and this is a stress test. Most games won't see this kind of regression. |
|
This version of the patch should eliminate the regression by only including the bindings for views with motion vectors. |
JMS55
left a comment
There was a problem hiding this comment.
I think we're technically doing a double pipeline compile here, one the first frame you add a skinned entity when it doesn't yet have a previous skin buffer entry, and then a second pipeline the next frame when it does.
I'm ok with that though, we can always improve it later.
|
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1326 if you'd like to help out. |
…storage buffers (#20076) ## Objective Fixes #20058 ## Solution Fix the `dynamic_offsets` array being too small if a mesh has morphs and skins and motion blur, and the renderer isn't using storage buffers (i.e. WebGL2). The bug was introduced in #13572. ## Testing - Minimal repro: https://github.com/M4tsuri/bevy_reproduce. - Also examples `animated_mesh`, `morph_targets`, `test_invalid_skinned_meshes`. - As far as I can tell Bevy doesn't have any examples or tests that can repro the problem combination. Tested with WebGL and native, Win10/Chrome/Nvidia.
This is a revamped equivalent to #9902, though it shares none of the code. It handles all special cases that I've tested correctly.
The overall technique consists of double-buffering the joint matrix and morph weights buffers, as most of the previous attempts to solve this problem did. The process is generally straightforward. Note that, to avoid regressing the ability of mesh extraction, skin extraction, and morph target extraction to run in parallel, I had to add a new system to rendering,
set_mesh_motion_vector_flags. The comment there explains the details; it generally runs very quickly.I've tested this with modified versions of the
animated_fox,morph_targets, andmany_foxesexamples that add TAA, and the patch works. To avoid bloating those examples, I didn't add switches for TAA to them.Addresses points (1) and (2) of #8423.
Changelog
Fixed