Skip to content

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

greeble-dev
Copy link
Contributor

@greeble-dev greeble-dev commented Apr 8, 2025

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 a SystemParam might be a cleaner fix.

Solution

Add a struct for parameters shared between all specialize systems:

#[derive(SystemParam)]
pub struct SpecializeMeshParams<'w, M: Send + Sync + 'static, RenderMeshInstances: Resource> {
    pub pipeline_cache: Res<'w, PipelineCache>,
    pub entity_specialization_ticks: Res<'w, EntitySpecializationTicks<M>>,
    pub render_mesh_instances: Res<'w, RenderMeshInstances>,
    pub render_meshes: Res<'w, RenderAssets<RenderMesh>>,
    pub ticks: SystemChangeTick,
}

This meant some refactoring:

  • Merged duplicate EntitySpecializationTicks structs in bevy_pbr and bevy_sprite and moved them to bevy_render.
  • Changed wireframe pipeline to use 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.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 8, 2025
@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 8, 2025
@tychedelia
Copy link
Contributor

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`.
@greeble-dev
Copy link
Contributor Author

greeble-dev commented Apr 10, 2025

I've updated the PR to include 3D wireframes. Note that this meant a bit of refactoring - WireframeEntitySpecializationTicks is replaced by EntitySpecializationTicks<WireframeMaterial>.

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>`.
@greeble-dev
Copy link
Contributor Author

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.

@greeble-dev greeble-dev marked this pull request as ready for review April 10, 2025 11:02
@@ -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 {
Copy link
Contributor Author

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.

@ItsDoot ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants