Skip to content
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

fix shadow batching #11645

Merged
merged 1 commit into from
Feb 14, 2024
Merged

fix shadow batching #11645

merged 1 commit into from
Feb 14, 2024

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Feb 1, 2024

Objective

RenderMeshInstance::material_bind_group_id is only set from queue_material_meshes::<M>. this field is used (only) for determining batch groups, so some items may be batched incorrectly if they have never been in the camera's view or if they don't use the Material abstraction.

in particular, shadow views render more meshes than the main camera, and currently batch some meshes where the object has never entered the camera view together. this is quite hard to trigger, but should occur in a scene with out-of-view alpha-mask materials (so that the material instance actually affects the shadow) in the path of a light.

this is also a footgun for custom pipelines: failing to set the material_bind_group_id will result in all meshes being batched together and all using the closest/furthest material to the camera (depending on sort order).

Solution

  • queue_shadows now sets the material_bind_group_id correctly
  • MeshPipeline doesn't attempt to batch meshes if the material_bind_group_id has not been set. custom pipelines still need to set this field to take advantage of batching, but will at least render correctly if it is not set

@robtfm robtfm added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Feb 1, 2024
@robtfm robtfm mentioned this pull request Feb 1, 2024
@JMS55 JMS55 added this to the 0.13 milestone Feb 2, 2024
@JMS55 JMS55 self-requested a review February 5, 2024 05:57
@NthTensor
Copy link
Contributor

Is there a shadow stress tests I should benchmark this against?

@robtfm
Copy link
Contributor Author

robtfm commented Feb 9, 2024

This is about correctness rather than performance. The other pr mentioned above is about perf and includes a shadow feature in the many_cubes benchmark.

@rparrett rparrett added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 13, 2024
@cart cart added this pull request to the merge queue Feb 14, 2024
Merged via the queue into bevyengine:main with commit 73bf730 Feb 14, 2024
27 checks passed
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants