Skip to content

Fix: Provide CPU mesh processing with MaterialBindingId #19083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 49 additions & 5 deletions crates/bevy_pbr/src/render/mesh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,12 +837,43 @@ pub struct RenderMeshInstanceGpuQueues(Parallel<RenderMeshInstanceGpuQueue>);
pub struct MeshesToReextractNextFrame(MainEntityHashSet);

impl RenderMeshInstanceShared {
fn from_components(
/// A gpu builder will provide the mesh instance id
/// during `RenderMeshGpuBuilder::update`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// during `RenderMeshGpuBuilder::update`.
/// during `RenderMeshInstanceGpuBuilder::update`.

///
/// Therefore, this does not need to be queried.
#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn for_gpu_building(
previous_transform: Option<&PreviousGlobalTransform>,
mesh: &Mesh3d,
tag: Option<&MeshTag>,
not_shadow_caster: bool,
no_automatic_batching: bool,
) -> Self {
Self::for_cpu_building(
previous_transform,
mesh,
tag,
default(),
not_shadow_caster,
no_automatic_batching,
)
}

/// Build a struct with common properties between gpu and cpu builders for cpu builders.
///
/// Unlike the gpu builder, the cpu builder does not have an equivalent
/// [`RenderMeshGpuBuilder::update`]. Therefore, we need to have
/// the [`MaterialBindingId`] ahead of time.
///
/// Otherwise, a mesh with a handle to a base color texture will all appear identical
/// based on the most recently inserted mesh.
Comment on lines +864 to +869
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a verification that this comment is true. That there really isn't an equivalent...and if there isn't, well...I need to find out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think we need to document what will happen if this code doesn't work. In general, I think these comments might be saying a bit too much. It's important to document on the gpu side that this will be updated later but its's clear from the usage at the call site that we retrieve the binding id ahead of time.

fn for_cpu_building(
previous_transform: Option<&PreviousGlobalTransform>,
mesh: &Mesh3d,
tag: Option<&MeshTag>,
material_bindings_index: MaterialBindingId,
not_shadow_caster: bool,
no_automatic_batching: bool,
) -> Self {
let mut mesh_instance_flags = RenderMeshInstanceFlags::empty();
mesh_instance_flags.set(RenderMeshInstanceFlags::SHADOW_CASTER, !not_shadow_caster);
Expand All @@ -858,8 +889,7 @@ impl RenderMeshInstanceShared {
RenderMeshInstanceShared {
mesh_asset_id: mesh.id(),
flags: mesh_instance_flags,
// This gets filled in later, during `RenderMeshGpuBuilder::update`.
material_bindings_index: default(),
material_bindings_index,
lightmap_slab_index: None,
tag: tag.map_or(0, |i| **i),
}
Expand Down Expand Up @@ -1305,6 +1335,8 @@ pub struct ExtractMeshesSet;
/// [`MeshUniform`] building.
pub fn extract_meshes_for_cpu_building(
mut render_mesh_instances: ResMut<RenderMeshInstances>,
mesh_material_ids: Res<RenderMaterialInstances>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: these appear to be correctly ordered via the constraints on late_sweep_material_instances.

render_material_bindings: Res<RenderMaterialBindings>,
render_visibility_ranges: Res<RenderVisibilityRanges>,
mut render_mesh_instance_queues: Local<Parallel<Vec<(Entity, RenderMeshInstanceCpu)>>>,
meshes_query: Extract<
Expand Down Expand Up @@ -1358,10 +1390,22 @@ pub fn extract_meshes_for_cpu_building(
transmitted_receiver,
);

let shared = RenderMeshInstanceShared::from_components(
let mesh_material = mesh_material_ids.mesh_material(MainEntity::from(entity));

let material_bindings_index = (mesh_material != DUMMY_MESH_MATERIAL.untyped())
.then(|| {
render_material_bindings
.get(&mesh_material)
.copied()
.unwrap_or_default()
})
.unwrap_or_default();
Comment on lines +1395 to +1402
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be unwrap_or_defaulting here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what the check for DUMMY_MESH_MATERIAL is doing here, is it's presumably already effectively covered by the unwrap_or_default when you call get on render_material_bindings -- if a dummy asset is being used we'll default there, which is the right call.


let shared = RenderMeshInstanceShared::for_cpu_building(
previous_transform,
mesh,
tag,
material_bindings_index,
not_shadow_caster,
no_automatic_batching,
);
Expand Down Expand Up @@ -1566,7 +1610,7 @@ fn extract_mesh_for_gpu_building(
transmitted_receiver,
);

let shared = RenderMeshInstanceShared::from_components(
let shared = RenderMeshInstanceShared::for_gpu_building(
previous_transform,
mesh,
tag,
Expand Down
Loading