Skip to content

Remove unnecessary Option wrapper from PreviousGlobalTransform in meshlet extraction #20306

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
Show file tree
Hide file tree
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
6 changes: 3 additions & 3 deletions crates/bevy_pbr/src/meshlet/instance_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ impl InstanceManager {
aabb: MeshletAabb,
bvh_depth: u32,
transform: &GlobalTransform,
previous_transform: Option<&PreviousGlobalTransform>,
previous_transform: &PreviousGlobalTransform,
render_layers: Option<&RenderLayers>,
mesh_material_ids: &RenderMaterialInstances,
render_material_bindings: &RenderMaterialBindings,
Expand All @@ -101,7 +101,7 @@ impl InstanceManager {
) {
// Build a MeshUniform for the instance
let transform = transform.affine();
let previous_transform = previous_transform.map(|t| t.0).unwrap_or(transform);
let previous_transform = previous_transform.0;
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
let previous_transform = previous_transform.0;

Imo remove this line, just use previous_transform.0 later on directly.

let mut flags = if not_shadow_receiver {
MeshFlags::empty()
} else {
Expand Down Expand Up @@ -201,7 +201,7 @@ pub fn extract_meshlet_mesh_entities(
Entity,
&MeshletMesh3d,
&GlobalTransform,
Option<&PreviousGlobalTransform>,
&PreviousGlobalTransform,
Option<&RenderLayers>,
Has<NotShadowReceiver>,
Has<NotShadowCaster>,
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_pbr/src/prepass/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,12 @@ pub fn update_mesh_previous_global_transforms(
let should_run = views.iter().any(|camera| camera.is_active);

if should_run {
// Initialize PreviousGlobalTransform for new mesh entities to current GlobalTransform
for (entity, transform) in &new_meshes {
let new_previous_transform = PreviousGlobalTransform(transform.affine());
commands.entity(entity).try_insert(new_previous_transform);
}
// Update previous transforms to current transform values for next frame
meshes.par_iter_mut().for_each(|(transform, mut previous)| {
previous.set_if_neq(PreviousGlobalTransform(transform.affine()));
});
Expand Down
84 changes: 84 additions & 0 deletions examples/test_previous_transform.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
//! Test example that verifies PreviousGlobalTransform is correctly initialized
//! to match GlobalTransform on the first frame for new entities.

use bevy::pbr::PreviousGlobalTransform;
use bevy::prelude::*;

fn main() {
App::new()
.add_plugins(DefaultPlugins)
.add_systems(Startup, setup)
.add_systems(Update, check_transforms)
.run();
}

fn setup(
mut commands: Commands,
mut meshes: ResMut<Assets<Mesh>>,
mut materials: ResMut<Assets<StandardMaterial>>,
) {
// Camera
commands.spawn((
Camera3d::default(),
Transform::from_xyz(3.0, 3.0, 3.0).looking_at(Vec3::ZERO, Vec3::Y),
));

// Light
commands.spawn((
DirectionalLight::default(),
Transform::from_rotation(Quat::from_euler(EulerRot::XYZ, -0.5, -0.5, 0.0)),
));

// Spawn a mesh entity - this should trigger PreviousGlobalTransform initialization
commands.spawn((
Mesh3d(meshes.add(Sphere::new(1.0))),
MeshMaterial3d(materials.add(StandardMaterial::default())),
Transform::from_xyz(1.0, 2.0, 3.0),
TestEntity,
));
}

#[derive(Component)]
struct TestEntity;

fn check_transforms(
query: Query<(&GlobalTransform, Option<&PreviousGlobalTransform>), With<TestEntity>>,
mut frame_count: Local<u32>,
) {
*frame_count += 1;

for (global_transform, maybe_previous) in &query {
println!(
"Frame {}: GlobalTransform: {:?}",
*frame_count,
global_transform.translation()
);
if let Some(previous) = maybe_previous {
println!(
"Frame {}: PreviousGlobalTransform exists: {:?}",
*frame_count, previous.0.translation
);

// On the first frame, previous should equal current
if *frame_count == 1 {
let current_affine = global_transform.affine();
if (previous.0.translation - current_affine.translation).length() < 0.001 {
println!(
"SUCCESS: PreviousGlobalTransform correctly initialized to GlobalTransform"
);
} else {
println!("FAIL: PreviousGlobalTransform not initialized correctly");
println!("Current: {:?}", current_affine.translation);
println!("Previous: {:?}", previous.0.translation);
}
}
} else {
println!("Frame {}: PreviousGlobalTransform missing", *frame_count);
}
}

// Exit after a few frames
if *frame_count >= 3 {
std::process::exit(0);
}
}
Loading