Skip to content

Avoid performance pitfalls in update_previous_global_transform system #18618

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.

Sign up for GitHub

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 6 commits into
base: main
Choose a base branch
from
+22 −18
Open

Avoid performance pitfalls in update_previous_global_transform system #18618

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
589fc7d
Don't use commands for mutation!
alice-i-cecile Mar 30, 2025
f9e1e67
Move should_run into an actual run condition
alice-i-cecile Mar 30, 2025
b0bf2d7
Statically avoid branching by stealing @aevyrie's homework
alice-i-cecile Mar 30, 2025
34b1e5f
Make docs more correct: filter is not a component
alice-i-cecile Mar 30, 2025
6a7bd7b
Required components and slap some par_iter on that thang
alice-i-cecile Mar 30, 2025
36c6ea9
Stray comment
alice-i-cecile Mar 30, 2025
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
40 changes: 22 additions & 18 deletions crates/bevy_pbr/src/prepass/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ where
.add_systems(
PreUpdate,
(
update_mesh_previous_global_transforms,
update_mesh_previous_global_transforms.run_if(any_camera_active),
update_previous_view_data,
),
)
Expand All @@ -187,7 +187,12 @@ where
BinnedRenderPhasePlugin::<AlphaMask3dPrepass, MeshPipeline>::new(
self.debug_flags,
),
));
))
// This can't be an attribute on the mesh types due to the shape of our dependency graph
.register_required_components::<Mesh3d, PreviousGlobalTransform>();
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 sure that this is something we want.

PreviousGlobalTransform is intentionally not required by Mesh3d, and it's not due to dependencies (I've talked about wanting to move PreviousGlobalTransform into bevy_transform before).

If you're making a game without temporal effects (TAA/DLSS/RT/motion blur/etc), then there's no need for PreviousGlobalTransform. It's only required for Mesh3d if at least one camera has MotionVectorPrepass.

I think we're better off:

  1. Moving PreviousGlobalTransform to bevy_transform
  2. Having a system in bevy_transform to update PreviousGlobalTransform, no filters based on mesh or whatever
  3. Have a new system in bevy_pbr/bevy_core_pipeline that when there's at least one camera with a MotionVectorPrepass, query for Mesh3d entities without a PreviousGlobalTransform component and insert a dummy one. Order this before the system from 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also do something like add a cargo feature for this, and say using MotionVectorPrepass requires turning the flag on, and then use cfg_attr to require PreviousGlobalTransform.


#[cfg(feature = "meshlet")]
app.register_required_components::<MeshletMesh3d, PreviousGlobalTransform>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed, MeshletMesh3d already requires PreviousGlobalTransform.

}

let Some(render_app) = app.get_sub_app_mut(RenderApp) else {
Expand Down Expand Up @@ -257,6 +262,11 @@ pub fn update_previous_view_data(
}
}

/// Tracks the previous [`GlobalTransform`] of a mesh,
/// assuming that the mesh's transform is affine.
///
/// This is added as a required component for [`Mesh3d`] and the meshlet equivalent in
/// the [`PrepassPlugin`].
#[derive(Component, PartialEq, Default)]
pub struct PreviousGlobalTransform(pub Affine3A);

Expand All @@ -265,24 +275,18 @@ type PreviousMeshFilter = With<Mesh3d>;
#[cfg(feature = "meshlet")]
type PreviousMeshFilter = Or<(With<Mesh3d>, With<MeshletMesh3d>)>;

/// A run condition for [`update_mesh_previous_global_transforms`] that checks if any camera is active.
pub fn any_camera_active(views: Query<&Camera, Or<(With<Camera3d>, With<ShadowView>)>>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have somewhere to have common run conditions? I feel like this could be useful for more than just the prepass module

views.iter().any(|camera| camera.is_active)
}

/// Updates the [`PreviousGlobalTransform`] of all meshes that match the [`PreviousMeshFilter`].
pub fn update_mesh_previous_global_transforms(
mut commands: Commands,
views: Query<&Camera, Or<(With<Camera3d>, With<ShadowView>)>>,
meshes: Query<(Entity, &GlobalTransform, Option<&PreviousGlobalTransform>), PreviousMeshFilter>,
mut meshes: Query<(&GlobalTransform, &mut PreviousGlobalTransform), PreviousMeshFilter>,
) {
let should_run = views.iter().any(|camera| camera.is_active);

if should_run {
for (entity, transform, old_previous_transform) in &meshes {
let new_previous_transform = PreviousGlobalTransform(transform.affine());
// Make sure not to trigger change detection on
// `PreviousGlobalTransform` if the previous transform hasn't
// changed.
if old_previous_transform != Some(&new_previous_transform) {
commands.entity(entity).try_insert(new_previous_transform);
}
}
}
meshes.par_iter_mut().for_each(|(transform, mut previous)| {
previous.set_if_neq(PreviousGlobalTransform(transform.affine()));
});
}

#[derive(Resource)]
Expand Down
Loading