Skip to content

Commit

Permalink
Fix motion vector computation after #17688. (#17717)
Browse files Browse the repository at this point in the history
PR #17688 broke motion vector computation, and therefore motion blur,
because it enabled retention of `MeshInputUniform`s, and
`MeshInputUniform`s contain the indices of the previous frame's
transform and the previous frame's skinned mesh joint matrices. On frame
N, if a `MeshInputUniform` is retained on GPU from the previous frame,
the `previous_input_index` and `previous_skin_index` would refer to the
indices for frame N - 2, not the index for frame N - 1.

This patch fixes the problems. It solves these issues in two different
ways, one for transforms and one for skins:

1. To fix transforms, this patch supplies the *frame index* to the
shader as part of the view uniforms, and specifies which frame index
each mesh's previous transform refers to. So, in the situation described
above, the frame index would be N, the previous frame index would be N -
1, and the `previous_input_frame_number` would be N - 2. The shader can
now detect this situation and infer that the mesh has been retained, and
can therefore conclude that the mesh's transform hasn't changed.

2. To fix skins, this patch replaces the explicit `previous_skin_index`
with an invariant that the index of the joints for the current frame and
the index of the joints for the previous frame are the same. This means
that the `MeshInputUniform` never has to be updated even if the skin is
animated. The downside is that we have to copy joint matrices from the
previous frame's buffer to the current frame's buffer in
`extract_skins`.

The rationale behind (2) is that we currently have no mechanism to
detect when joints that affect a skin have been updated, short of
comparing all the transforms and setting a flag for
`extract_meshes_for_gpu_building` to consume, which would regress
performance as we want `extract_skins` and
`extract_meshes_for_gpu_building` to be able to run in parallel.

To test this change, use `cargo run --example motion_blur`.
  • Loading branch information
pcwalton authored Feb 18, 2025
1 parent 5e569af commit 0517b96
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 23 deletions.
1 change: 1 addition & 0 deletions crates/bevy_pbr/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ bevy_asset = { path = "../bevy_asset", version = "0.16.0-dev" }
bevy_color = { path = "../bevy_color", version = "0.16.0-dev" }
bevy_core_pipeline = { path = "../bevy_core_pipeline", version = "0.16.0-dev" }
bevy_derive = { path = "../bevy_derive", version = "0.16.0-dev" }
bevy_diagnostic = { path = "../bevy_diagnostic", version = "0.16.0-dev" }
bevy_ecs = { path = "../bevy_ecs", version = "0.16.0-dev" }
bevy_image = { path = "../bevy_image", version = "0.16.0-dev" }
bevy_math = { path = "../bevy_math", version = "0.16.0-dev" }
Expand Down
21 changes: 10 additions & 11 deletions crates/bevy_pbr/src/render/gpu_preprocess.rs
Original file line number Diff line number Diff line change
Expand Up @@ -704,12 +704,10 @@ impl Node for EarlyGpuPreprocessNode {
continue;
};

// If we're drawing indirectly, make sure the mesh preprocessing
// shader has access to the view info it needs to do culling.
let mut dynamic_offsets: SmallVec<[u32; 1]> = smallvec![];
if !no_indirect_drawing {
dynamic_offsets.push(view_uniform_offset.offset);
}
// Make sure the mesh preprocessing shader has access to the
// view info it needs to do culling and motion vector
// computation.
let dynamic_offsets = [view_uniform_offset.offset];

// Are we drawing directly or indirectly?
match *phase_bind_groups {
Expand Down Expand Up @@ -1416,6 +1414,11 @@ fn preprocess_direct_bind_group_layout_entries() -> DynamicBindGroupLayoutEntrie
DynamicBindGroupLayoutEntries::new_with_indices(
ShaderStages::COMPUTE,
(
// `view`
(
0,
uniform_buffer::<ViewUniform>(/* has_dynamic_offset= */ true),
),
// `current_input`
(3, storage_buffer_read_only::<MeshInputUniform>(false)),
// `previous_input`
Expand Down Expand Up @@ -1471,11 +1474,6 @@ fn gpu_culling_bind_group_layout_entries() -> DynamicBindGroupLayoutEntries {
9,
storage_buffer_read_only::<MeshCullingData>(/* has_dynamic_offset= */ false),
),
// `view`
(
0,
uniform_buffer::<ViewUniform>(/* has_dynamic_offset= */ true),
),
))
}

Expand Down Expand Up @@ -1930,6 +1928,7 @@ impl<'a> PreprocessBindGroupBuilder<'a> {
"preprocess_direct_bind_group",
&self.pipelines.direct_preprocess.bind_group_layout,
&BindGroupEntries::with_indices((
(0, self.view_uniforms.uniforms.binding()?),
(3, self.current_input_buffer.as_entire_binding()),
(4, self.previous_input_buffer.as_entire_binding()),
(
Expand Down
21 changes: 16 additions & 5 deletions crates/bevy_pbr/src/render/mesh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use bevy_core_pipeline::{
prepass::MotionVectorPrepass,
};
use bevy_derive::{Deref, DerefMut};
use bevy_diagnostic::FrameCount;
use bevy_ecs::{
prelude::*,
query::ROQueryItem,
Expand Down Expand Up @@ -551,12 +552,18 @@ pub struct MeshInputUniform {
/// Low 16 bits: index of the material inside the bind group data.
/// High 16 bits: index of the lightmap in the binding array.
pub material_and_lightmap_bind_group_slot: u32,
/// The number of the frame on which this [`MeshInputUniform`] was built.
///
/// This is used to validate the previous transform and skin. If this
/// [`MeshInputUniform`] wasn't updated on this frame, then we know that
/// neither this mesh's transform nor that of its joints have been updated
/// on this frame, and therefore the transforms of both this mesh and its
/// joints must be identical to those for the previous frame.
pub timestamp: u32,
/// User supplied tag to identify this mesh instance.
pub tag: u32,
/// Padding.
pub pad_a: u32,
/// Padding.
pub pad_b: u32,
pub pad: u32,
}

/// Information about each mesh instance needed to cull it on GPU.
Expand Down Expand Up @@ -1117,6 +1124,7 @@ impl RenderMeshInstanceGpuBuilder {
render_material_bindings: &RenderMaterialBindings,
render_lightmaps: &RenderLightmaps,
skin_uniforms: &SkinUniforms,
timestamp: FrameCount,
) -> u32 {
let (first_vertex_index, vertex_count) =
match mesh_allocator.mesh_vertex_slice(&self.shared.mesh_asset_id) {
Expand Down Expand Up @@ -1164,6 +1172,7 @@ impl RenderMeshInstanceGpuBuilder {
lightmap_uv_rect: self.lightmap_uv_rect,
flags: self.mesh_flags.bits(),
previous_input_index: u32::MAX,
timestamp: timestamp.0,
first_vertex_index,
first_index_index,
index_count: if mesh_is_indexed {
Expand All @@ -1176,8 +1185,7 @@ impl RenderMeshInstanceGpuBuilder {
self.shared.material_bindings_index.slot,
) | ((lightmap_slot as u32) << 16),
tag: self.shared.tag,
pad_a: 0,
pad_b: 0,
pad: 0,
};

// Did the last frame contain this entity as well?
Expand Down Expand Up @@ -1607,6 +1615,7 @@ pub fn collect_meshes_for_gpu_building(
render_material_bindings: Res<RenderMaterialBindings>,
render_lightmaps: Res<RenderLightmaps>,
skin_uniforms: Res<SkinUniforms>,
frame_count: Res<FrameCount>,
) {
let RenderMeshInstances::GpuBuilding(ref mut render_mesh_instances) =
render_mesh_instances.into_inner()
Expand Down Expand Up @@ -1646,6 +1655,7 @@ pub fn collect_meshes_for_gpu_building(
&render_material_bindings,
&render_lightmaps,
&skin_uniforms,
*frame_count,
);
}

Expand Down Expand Up @@ -1673,6 +1683,7 @@ pub fn collect_meshes_for_gpu_building(
&render_material_bindings,
&render_lightmaps,
&skin_uniforms,
*frame_count,
);
mesh_culling_builder
.update(&mut mesh_culling_data_buffer, instance_data_index as usize);
Expand Down
16 changes: 12 additions & 4 deletions crates/bevy_pbr/src/render/mesh_preprocess.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -209,14 +209,22 @@ fn main(@builtin(global_invocation_id) global_invocation_id: vec3<u32>) {
}
#endif

// Look up the previous model matrix.
// See whether the `MeshInputUniform` was updated on this frame. If it
// wasn't, then we know the transforms of this mesh must be identical to
// those on the previous frame, and therefore we don't need to access the
// `previous_input_index` (in fact, we can't; that index are only valid for
// one frame and will be invalid).
let timestamp = current_input[input_index].timestamp;
let mesh_changed_this_frame = timestamp == view.frame_count;

// Look up the previous model matrix, if it could have been.
let previous_input_index = current_input[input_index].previous_input_index;
var previous_world_from_local_affine_transpose: mat3x4<f32>;
if (previous_input_index == 0xffffffff) {
previous_world_from_local_affine_transpose = world_from_local_affine_transpose;
} else {
if (mesh_changed_this_frame && previous_input_index != 0xffffffffu) {
previous_world_from_local_affine_transpose =
previous_input[previous_input_index].world_from_local;
} else {
previous_world_from_local_affine_transpose = world_from_local_affine_transpose;
}
let previous_world_from_local =
maths::affine3_to_square(previous_world_from_local_affine_transpose);
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_pbr/src/render/skinning.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ fn skin_model(
+ weights.z * joint_matrices.data[indexes.z]
+ weights.w * joint_matrices.data[indexes.w];
#else // SKINS_USE_UNIFORM_BUFFERS
let skin_index = mesh[instance_index].current_skin_index;
var skin_index = mesh[instance_index].current_skin_index;
return weights.x * joint_matrices[skin_index + indexes.x]
+ weights.y * joint_matrices[skin_index + indexes.y]
+ weights.z * joint_matrices[skin_index + indexes.z]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ struct MeshInput {
// Low 16 bits: index of the material inside the bind group data.
// High 16 bits: index of the lightmap in the binding array.
material_and_lightmap_bind_group_slot: u32,
timestamp: u32,
// User supplied index to identify the mesh instance
tag: u32,
pad_a: u32,
pad_b: u32,
pad: u32,
}

// The `wgpu` indirect parameters structure. This is a union of two structures.
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_render/src/render_resource/buffer_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ impl<T: NoUninit> RawBufferVec<T> {
self.values.append(&mut other.values);
}

/// Returns the value at the given index.
pub fn get(&self, index: u32) -> Option<&T> {
self.values.get(index as usize)
}

/// Sets the value at the given index.
///
/// The index must be less than [`RawBufferVec::len`].
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_render/src/view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ pub mod visibility;
pub mod window;

use bevy_asset::{load_internal_asset, weak_handle, Handle};
use bevy_diagnostic::FrameCount;
pub use visibility::*;
pub use window::*;

Expand Down Expand Up @@ -568,6 +569,7 @@ pub struct ViewUniform {
pub frustum: [Vec4; 6],
pub color_grading: ColorGradingUniform,
pub mip_bias: f32,
pub frame_count: u32,
}

#[derive(Resource)]
Expand Down Expand Up @@ -889,6 +891,7 @@ pub fn prepare_view_uniforms(
Option<&TemporalJitter>,
Option<&MipBias>,
)>,
frame_count: Res<FrameCount>,
) {
let view_iter = views.iter();
let view_count = view_iter.len();
Expand Down Expand Up @@ -942,6 +945,7 @@ pub fn prepare_view_uniforms(
frustum,
color_grading: extracted_view.color_grading.clone().into(),
mip_bias: mip_bias.unwrap_or(&MipBias(0.0)).0,
frame_count: frame_count.0,
}),
};

Expand Down
1 change: 1 addition & 0 deletions crates/bevy_render/src/view/view.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,5 @@ struct View {
frustum: array<vec4<f32>, 6>,
color_grading: ColorGrading,
mip_bias: f32,
frame_count: u32,
};

0 comments on commit 0517b96

Please sign in to comment.