-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Refactor shared specialize params into a SystemParam
#18762
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
base: main
Are you sure you want to change the base?
Conversation
I had something similar like this in the past but one of the frustrations here is that 2d is left out because it doesn't share a material trait. Doesn't mean we shouldn't do this, though. |
This includes refactoring wireframes to reuse `EntitySpecializationTicks` instead of its own type. This meant changing the bounds on `SpecializeMeshParams` since `WireframeMaterial` isn't a `Material`.
I've updated the PR to include 3D wireframes. Note that this meant a bit of refactoring - I think I can also support the 2D pipeline. Maybe that's too much for one PR, but I'll try it and see. |
…_render`. Also made `RenderMeshInstances` a generic parameter. This is all prep for supporting 2D.
This meant some light refactoring to replace `WireframeEntitySpecializationTicks` with `EntitySpecializationTicks<Wireframe2dMaterial>`.
Updated with 2D support and made ready for review. The description has been updated to note that 2D support might have been better as a follow up PR. |
@@ -724,32 +704,35 @@ pub fn specialize_material2d_meshes<M: Material2d>( | |||
let Some(material_asset_id) = render_material_instances.get(visible_entity) else { | |||
continue; | |||
}; | |||
let Some(mesh_instance) = render_mesh_instances.get_mut(visible_entity) else { | |||
let Some(mesh_instance) = params.render_mesh_instances.get(visible_entity) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this from get_mut
to get
. Which seems safe but raising it just in case.
Objective
Refactor parameters shared between various specialize systems into one
SystemParam
. This is mainly to stop systems exceeding the maximum number of parameters, but is a bit cleaner too and include some minor 2D/3D unification.Background
As part of #18074 I added a parameter to
specialize_material_meshes
. This exceeded the maximum number of parameters allowed in a system. Initially I moved parameters into tuples, but it was suggested that aSystemParam
might be a cleaner fix.Solution
Add a struct for parameters shared between all specialize systems:
This meant some refactoring:
EntitySpecializationTicks
structs inbevy_pbr
andbevy_sprite
and moved them tobevy_render
.EntitySpecializationTicks
instead of its own type.Concerns
Maybe this PR does too much. Updating just the 3D parts would make it simpler, then the 2D parts could be a follow up PR.
Testing
Ran
testbed_3d/2d
,wireframe_3d/2d
,deferred_rendering
,motion_blur
,lighting
, and a few other examples. Win10/Nvidia across Vulkan, WebGL/Chrome, WebGPU/Chrome.