Implement motion vectors and TAA for skinned meshes and meshes with morph targets. #13572
Conversation
pcwalton
added
A-Rendering
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.
pcwalton
force-pushed
the
skinned-motion-vectors
branch
from
c75eecd
to
83d376c
Compare
| #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.
| 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. |
alice-i-cecile
added
D-Complex
|
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. |
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.
alice-i-cecile
added
S-Ready-For-Final-Review
IceSentry
added
the
M-Needs-Release-Note
|
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. |
alice-i-cecile
removed
the
M-Needs-Release-Note
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