Fix several regressions from recent rendering changes.#16890
Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom Dec 22, 2024
Merged
Fix several regressions from recent rendering changes.#16890alice-i-cecile merged 2 commits intobevyengine:mainfrom
alice-i-cecile merged 2 commits intobevyengine:mainfrom
Conversation
This commit fixes the following regressions: 1. Transmission-specific calls to shader lighting functions didn't pass the `enable_diffuse` parameter, breaking the `transmission` example. 2. The combination of bindless `StandardMaterial` and bindless lightmaps caused us to blow past the 128 texture limit on M1/M2 chips in some cases. gfx-rs/wgpu#3334 should fix this, but in the meantime this patch reduces the number of bindless lightmaps from 16 to 4 in order to stay under the limit. 3. The renderer was crashing on startup on Adreno 610 chips. This PR simply disables bindless on Adreno 610 and lower.
pcwalton
added
A-Rendering
rparrett
reviewed
Dec 19, 2024
Member
|
I've started an example-runner test run for this :) We should expect diffs though. |
rparrett
approved these changes
Dec 19, 2024
Contributor
There was a problem hiding this comment.
I am not qualified to review most of the rendering changes, but:
- Confirmed that this fixes
depth_of_fieldon my m1 mac - Confirmed that this fixes
transmission - Changes related to Android make sense to me, but I can't test
- Didn't seem to cause any additional problems in the example run (but mobile screenshots are still not running?)
IceSentry
approved these changes
Dec 22, 2024
| /// If bindless textures aren't in use, then only a single lightmap can be bound | ||
| /// at a time. | ||
| pub const LIGHTMAPS_PER_SLAB: usize = 16; | ||
| pub const LIGHTMAPS_PER_SLAB: usize = 4; |
Contributor
There was a problem hiding this comment.
Is there a reason for the 4 specifically or is it just a nice number that happens to still work because it's small enough??
Contributor
|
I haven't tested anything locally, but the changes LGTM |
IceSentry
added
S-Ready-For-Final-Review
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 6, 2025
Currently, our batchable binned items are stored in a hash table that maps bin key, which includes the batch set key, to a list of entities. Multidraw is handled by sorting the bin keys and accumulating adjacent bins that can be multidrawn together (i.e. have the same batch set key) into multidraw commands during `batch_and_prepare_binned_render_phase`. This is reasonably efficient right now, but it will complicate future work to retain indirect draw parameters from frame to frame. Consider what must happen when we have retained indirect draw parameters and the application adds a bin (i.e. a new mesh) that shares a batch set key with some pre-existing meshes. (That is, the new mesh can be multidrawn with the pre-existing meshes.) To be maximally efficient, our goal in that scenario will be to update *only* the indirect draw parameters for the batch set (i.e. multidraw command) containing the mesh that was added, while leaving the others alone. That means that we have to quickly locate all the bins that belong to the batch set being modified. In the existing code, we would have to sort the list of bin keys so that bins that can be multidrawn together become adjacent to one another in the list. Then we would have to do a binary search through the sorted list to find the location of the bin that was just added. Next, we would have to widen our search to adjacent indexes that contain the same batch set, doing expensive comparisons against the batch set key every time. Finally, we would reallocate the indirect draw parameters and update the stored pointers to the indirect draw parameters that the bins store. By contrast, it'd be dramatically simpler if we simply changed the way bins are stored to first map from batch set key (i.e. multidraw command) to the bins (i.e. meshes) within that batch set key, and then from each individual bin to the mesh instances. That way, the scenario above in which we add a new mesh will be simpler to handle. First, we will look up the batch set key corresponding to that mesh in the outer map to find an inner map corresponding to the single multidraw command that will draw that batch set. We will know how many meshes the multidraw command is going to draw by the size of that inner map. Then we simply need to reallocate the indirect draw parameters and update the pointers to those parameters within the bins as necessary. There will be no need to do any binary search or expensive batch set key comparison: only a single hash lookup and an iteration over the inner map to update the pointers. This patch implements the above technique. Because we don't have retained bins yet, this PR provides no performance benefits. However, it opens the door to maximally efficient updates when only a small number of meshes change from frame to frame. The main churn that this patch causes is that the *batch set key* (which uniquely specifies a multidraw command) and *bin key* (which uniquely specifies a mesh *within* that multidraw command) are now separate, instead of the batch set key being embedded *within* the bin key. In order to isolate potential regressions, I think that at least #16890, #16836, and #16825 should land before this PR does. ## Migration Guide * The *batch set key* is now separate from the *bin key* in `BinnedPhaseItem`. The batch set key is used to collect multidrawable meshes together. If you aren't using the multidraw feature, you can safely set the batch set key to `()`.
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this pull request
Jan 6, 2025
) This commit fixes the following regressions: 1. Transmission-specific calls to shader lighting functions didn't pass the `enable_diffuse` parameter, breaking the `transmission` example. 2. The combination of bindless `StandardMaterial` and bindless lightmaps caused us to blow past the 128 texture limit on M1/M2 chips in some cases, in particular the `depth_of_field` example. gfx-rs/wgpu#3334 should fix this, but in the meantime this patch reduces the number of bindless lightmaps from 16 to 4 in order to stay under the limit. 3. The renderer was crashing on startup on Adreno 610 chips. This PR simply disables bindless on Adreno 610 and lower.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
) This commit fixes the following regressions: 1. Transmission-specific calls to shader lighting functions didn't pass the `enable_diffuse` parameter, breaking the `transmission` example. 2. The combination of bindless `StandardMaterial` and bindless lightmaps caused us to blow past the 128 texture limit on M1/M2 chips in some cases, in particular the `depth_of_field` example. gfx-rs/wgpu#3334 should fix this, but in the meantime this patch reduces the number of bindless lightmaps from 16 to 4 in order to stay under the limit. 3. The renderer was crashing on startup on Adreno 610 chips. This PR simply disables bindless on Adreno 610 and lower.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
Currently, our batchable binned items are stored in a hash table that maps bin key, which includes the batch set key, to a list of entities. Multidraw is handled by sorting the bin keys and accumulating adjacent bins that can be multidrawn together (i.e. have the same batch set key) into multidraw commands during `batch_and_prepare_binned_render_phase`. This is reasonably efficient right now, but it will complicate future work to retain indirect draw parameters from frame to frame. Consider what must happen when we have retained indirect draw parameters and the application adds a bin (i.e. a new mesh) that shares a batch set key with some pre-existing meshes. (That is, the new mesh can be multidrawn with the pre-existing meshes.) To be maximally efficient, our goal in that scenario will be to update *only* the indirect draw parameters for the batch set (i.e. multidraw command) containing the mesh that was added, while leaving the others alone. That means that we have to quickly locate all the bins that belong to the batch set being modified. In the existing code, we would have to sort the list of bin keys so that bins that can be multidrawn together become adjacent to one another in the list. Then we would have to do a binary search through the sorted list to find the location of the bin that was just added. Next, we would have to widen our search to adjacent indexes that contain the same batch set, doing expensive comparisons against the batch set key every time. Finally, we would reallocate the indirect draw parameters and update the stored pointers to the indirect draw parameters that the bins store. By contrast, it'd be dramatically simpler if we simply changed the way bins are stored to first map from batch set key (i.e. multidraw command) to the bins (i.e. meshes) within that batch set key, and then from each individual bin to the mesh instances. That way, the scenario above in which we add a new mesh will be simpler to handle. First, we will look up the batch set key corresponding to that mesh in the outer map to find an inner map corresponding to the single multidraw command that will draw that batch set. We will know how many meshes the multidraw command is going to draw by the size of that inner map. Then we simply need to reallocate the indirect draw parameters and update the pointers to those parameters within the bins as necessary. There will be no need to do any binary search or expensive batch set key comparison: only a single hash lookup and an iteration over the inner map to update the pointers. This patch implements the above technique. Because we don't have retained bins yet, this PR provides no performance benefits. However, it opens the door to maximally efficient updates when only a small number of meshes change from frame to frame. The main churn that this patch causes is that the *batch set key* (which uniquely specifies a multidraw command) and *bin key* (which uniquely specifies a mesh *within* that multidraw command) are now separate, instead of the batch set key being embedded *within* the bin key. In order to isolate potential regressions, I think that at least bevyengine#16890, bevyengine#16836, and bevyengine#16825 should land before this PR does. ## Migration Guide * The *batch set key* is now separate from the *bin key* in `BinnedPhaseItem`. The batch set key is used to collect multidrawable meshes together. If you aren't using the multidraw feature, you can safely set the batch set key to `()`.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 fixes the following regressions:
Transmission-specific calls to shader lighting functions didn't pass the
enable_diffuseparameter, breaking thetransmissionexample.The combination of bindless
StandardMaterialand bindless lightmaps caused us to blow past the 128 texture limit on M1/M2 chips in some cases, in particular thedepth_of_fieldexample. Use Argument Buffers forbinding_arrayon Metal gfx-rs/wgpu#3334 should fix this, but in the meantime this patch reduces the number of bindless lightmaps from 16 to 4 in order to stay under the limit.The renderer was crashing on startup on Adreno 610 chips. This PR simply disables bindless on Adreno 610 and lower.