Update the prepass shaders and fix the batching logic for bindless and multidraw. #16755
+82
−43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
multidraw. This commit resolves most of the failures seen in bevyengine#16670. It contains two major fixes: 1. The prepass shaders weren't updated for bindless mode, so they were accessing `material` as a single element instead of as an array. I added the needed `BINDLESS` check. 2. If the mesh didn't support batch set keys (i.e. `get_batch_set_key()` returns `None`), and multidraw was enabled, the batching logic would try to multidraw all the meshes in a bin together instead of disabling multidraw. This is because we checked whether the `Option<BatchSetKey>` for the previous batch was equal to the `Option<BatchSetKey>` for the next batch to determine whether objects could be multidrawn together, which would return true if batch set keys were absent, causing an entire bin to be multidrawn together. This patch fixes the logic so that multidraw is only enabled if the batch set keys match *and are `Some`*. Additionally, this commit adds batch key support for bins that use `Opaque3dNoLightmapBinKey`, which in practice means prepasses. Consequently, this patch enables multidraw for the prepass when GPU culling is enabled. When testing this patch, try adding `GpuCulling` to the camera in the `deferred_rendering` and `ssr` examples. You can see that these examples break without this patch and work properly with it.
pcwalton
requested review from
JMS55,
Elabajaba,
IceSentry and
bushrat011899
pcwalton
added
A-Rendering
pcwalton
added a commit
to pcwalton/bevy
that referenced
this pull request
Dec 10, 2024
default. This patch replaces the undocumented `NoGpuCulling` component with a new component, `NoIndirectDrawing`, effectively turning indirect drawing on by default. Indirect mode is needed for the recently-landed multidraw feature (bevyengine#16427). Since multidraw is such a win for performance, when that feature is supported the small performance tax that indirect mode incurs is virtually always worth paying. To ensure that custom drawing code such as that in the `custom_shader_instancing` example continues to function, this commit additionally makes GPU culling take the `NoFrustumCulling` component into account. This PR is an alternative to bevyengine#16670 that doesn't break the `custom_shader_instancing` example. **PR bevyengine#16755 should land first in order to avoid breaking deferred rendering, as multidraw currently breaks it**.
This was referenced Dec 10, 2024
JMS55
approved these changes
Dec 11, 2024
kristoff3r
approved these changes
Dec 11, 2024
|
I played around with the deferred rendering example, and this happens if you first go to "Forward + Prepass", back to just "Forward" and then resize the window. I don't know if it's related to this PR or not, it didn't happen on main but main was broken in other ways.
|
|
Probably unrelated, but I can verify for sure. |
|
@kristoff3r That bug is still there in 0.15, so it's unrelated. |
pcwalton
added
S-Ready-For-Final-Review
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 13, 2024
…y default. (#16757) This patch replaces the undocumented `NoGpuCulling` component with a new component, `NoIndirectDrawing`, effectively turning indirect drawing on by default. Indirect mode is needed for the recently-landed multidraw feature (#16427). Since multidraw is such a win for performance, when that feature is supported the small performance tax that indirect mode incurs is virtually always worth paying. To ensure that custom drawing code such as that in the `custom_shader_instancing` example continues to function, this commit additionally makes GPU culling take the `NoFrustumCulling` component into account. This PR is an alternative to #16670 that doesn't break the `custom_shader_instancing` example. **PR #16755 should land first in order to avoid breaking deferred rendering, as multidraw currently breaks it**. ## Migration Guide * Indirect drawing (GPU culling) is now enabled by default, so the `GpuCulling` component is no longer available. To disable indirect mode, which may be useful with custom render nodes, add the new `NoIndirectDrawing` component to your camera.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit resolves most of the failures seen in #16670. It contains two major fixes:
The prepass shaders weren't updated for bindless mode, so they were accessing
materialas a single element instead of as an array. I added the neededBINDLESScheck.If the mesh didn't support batch set keys (i.e.
get_batch_set_key()returnsNone), and multidraw was enabled, the batching logic would try to multidraw all the meshes in a bin together instead of disabling multidraw. This is because we checked whether theOption<BatchSetKey>for the previous batch was equal to theOption<BatchSetKey>for the next batch to determine whether objects could be multidrawn together, which would return true if batch set keys were absent, causing an entire bin to be multidrawn together. This patch fixes the logic so that multidraw is only enabled if the batch set keys match and areSome.Additionally, this commit adds batch key support for bins that use
Opaque3dNoLightmapBinKey, which in practice means prepasses. Consequently, this patch enables multidraw for the prepass when GPU culling is enabled.When testing this patch, try adding
GpuCullingto the camera in thedeferred_renderingandssrexamples. You can see that these examples break without this patch and work properly with it.