Skip to content

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 will unblock a crash fix (#18074). It's also a bit cleaner and might help with future refactoring.

Background

As part of #18074 I needed to add a parameter to specialize_material_meshes, but this would have pushed it over the maximum number of parameters allowed in a system. I could have dodged the limit by moving parameters into tuples, but it was suggested that a SystemParam might be a cleaner fix.

Solution

Take five parameters common to all specialize systems and put them in a single SpecializeMeshParams struct.

 pub fn specialize_material_meshes(
+    params: SpecializeMeshParams<EntitySpecializationTicks, RenderMeshInstances>,
-    render_meshes: Res<RenderAssets<RenderMesh>>,
     params: SpecializeMeshParams<EntitySpecializationTicks, RenderMeshInstances>,
     render_materials: Res<ErasedRenderAssets<PreparedMaterial>>,
-    render_mesh_instances: Res<RenderMeshInstances>,
     render_material_instances: Res<RenderMaterialInstances>,
     render_lightmaps: Res<RenderLightmaps>,
     render_visibility_ranges: Res<RenderVisibilityRanges>,
     views: Query<(&ExtractedView, &RenderVisibleEntities)>,
     view_key_cache: Res<ViewKeyCache>,
-    entity_specialization_ticks: Res<EntitySpecializationTicks>,
     view_specialization_ticks: Res<ViewSpecializationTicks>,
     mut specialized_material_pipeline_cache: ResMut<SpecializedMaterialPipelineCache>,
     mut pipelines: ResMut<SpecializedMeshPipelines<MaterialPipelineSpecializer>>,
     pipeline: Res<MaterialPipeline>,
-    pipeline_cache: Res<PipelineCache>,
-    ticks: SystemChangeTick,
)

Testing

Ran testbed_3d, testbed_2d, wireframe, wireframe_2d, deferred_rendering, motion_blur, lighting, and a few other examples. Win10/Nvidia across Vulkan, WebGL/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
Member

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
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this is sometimes done intentionally to trigger change detection, but if its not documented as such i think its fine to assume it was unintentional.

@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
@alice-i-cecile alice-i-cecile requested a review from atlv24 July 7, 2025 19:29
@alice-i-cecile alice-i-cecile requested a review from tychedelia July 7, 2025 19:29
@greeble-dev
Copy link
Contributor Author

Fixed merge conflicts but reverted EntitySpecializationTicks unification - got too complicated for this PR after the type erased material changes.

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.

5 participants