Avoid performance pitfalls in update_previous_global_transform system #18618
Conversation
alice-i-cecile
added
A-Rendering
| )); | ||
| )) | ||
| // This can't be an attribute on the mesh types due to the shape of our dependency graph | ||
| .register_required_components::<Mesh3d, PreviousGlobalTransform>(); |
There was a problem hiding this comment.
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:
- Moving PreviousGlobalTransform to bevy_transform
- Having a system in bevy_transform to update PreviousGlobalTransform, no filters based on mesh or whatever
- 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.
There was a problem hiding this comment.
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>(); |
There was a problem hiding this comment.
This is not needed, MeshletMesh3d already requires PreviousGlobalTransform.
Co-authored-by: François Mockers <francois.mockers@vleue.com>
| @@ -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 { | |||
There was a problem hiding this comment.
Do we have somewhere to have common run conditions? I feel like this could be useful for more than just the prepass module
|
This is an alternative/sibling PR that only addresses perf within functions, so might be less controversial. #18632 |
alice-i-cecile
added
S-Waiting-on-Author
|
Moving this out of the milestone in favor of #18632. I'll clean this up once that's merged: there's some smaller quality issues to address still. |
Objective
@aevyrie saw this system showing up in his
big_spacetraces.Upon investigation, this system is using commands to unconditionally mutate a component. That's slow!
Apparently this fixes #14681.
Solution
I'm going to avoid parallelizing for now; let's take the free wins here first.Casual 38x perf improvement.
Requests for reviewers
2. @aevyrie I'd be interested in a fresh bench on this, but this is so clearly better that I don't care if we skip it.3. Should I swap to using required components here and avoid ever inserting?