Refactor batch_and_prepare_binned_render_phase in preparation for bin retention.#16922
Merged
alice-i-cecile merged 6 commits intobevyengine:mainfrom Dec 30, 2024
Merged
Conversation
retention. This commit makes the following changes: * `IndirectParametersBuffer` has been changed from a `BufferVec` to a `RawBufferVec`. This won about 20us or so on Bistro by avoiding `encase` overhead. * The methods on the `GetFullBatchData` trait no longer have the `entity` parameter, as it was unused. * `PreprocessWorkItem`, which specifies a transform-and-cull operation, now supplies the mesh instance uniform output index directly instead of having the shader look it up from the indirect draw parameters. Accordingly, the responsibility of writing the output index to the indirect draw parameters has been moved from the CPU to the GPU. This is in preparation for retained indirect instance draw commands, where the mesh instance uniform output index may change from frame to frame, while the indirect instance draw commands will be cached. We won't want the CPU to have to upload the same indirect draw parameters again and again if a batch didn't change from frame to frame. * `batch_and_prepare_binned_render_phase` and `batch_and_prepare_sorted_render_phase` now allocate indirect draw commands for an entire batch set at a time when possible, instead of one batch at a time. This change will allow us to retain the indirect draw commands for whole batch sets. * `GetFullBatchData::get_batch_indirect_parameters_index` has been replaced with `GetFullBatchData::write_batch_indirect_parameters`, which takes an offset and writes into it instead of allocating. This is necessary in order to use the optimization mentioned in the previous point. * At the WGSL level, `IndirectParameters` has been factored out into `mesh_preprocess_types.wgsl`. This is because we'll need a new compute shader that zeroes out the instance counts in preparation for a new frame. That shader will need to access `IndirectParameters`, so it was moved to a separate file. * Bins are no longer raw vectors but are instances of a separate type, `RenderBin`. This is so that the bin can eventually contain its retained batches.
pcwalton
added
C-Code-Quality
IceSentry
approved these changes
Dec 22, 2024
kristoff3r
approved these changes
Dec 22, 2024
pcwalton
added
S-Ready-For-Final-Review
Member
|
Running an example check to spot unexpected regressions: bevyengine/bevy-example-runner#74 |
Contributor
Author
|
Looks like this broke shadows. I'll look. |
alice-i-cecile
added
S-Waiting-on-Author
Member
|
this fixes shadows, #16836 broke them |
alice-i-cecile
added
S-Ready-For-Final-Review
Contributor
Author
|
Shadows still seem broken even with this PR. I wonder if the issue is intermittent. |
pcwalton
added a commit
to pcwalton/bevy
that referenced
this pull request
Dec 30, 2024
`ExtractedView`s, not `ViewTarget`s. OK, so this is tricky. Every frame, `delete_old_work_item_buffers` deletes the mesh preprocessing index buffers (a.k.a. work item buffers) for views that don't have `ViewTarget`s. This was always wrong for shadow map views, as shadow maps only have `ExtractedView` components, not `ViewTarget`s. However, before bevyengine#16836, the problem was masked, because uploading the mesh preprocessing index buffers for shadow views had already completed by the time `delete_old_work_item_buffers` ran. But PR bevyengine#16836 moved `delete_old_work_item_buffers` from the `ManageViews` phase to `PrepareResources`, which runs before `write_batched_instance_buffers` uploads the work item buffers to the GPU. This itself isn't wrong, but it exposed the bug, because now it's possible for work item buffers to get deleted before they're uploaded in `write_batched_instance_buffers`. This is actually intermittent! It's possible for the old work item buffers to get deleted, and then *recreated* in `batch_and_prepare_binned_render_phase`, which runs during `PrepareResources` as well, and under that system ordering, there will be no problem other than a little inefficiency arising from recreating the buffers every frame. But, if `delete_old_work_item_buffers` runs *after* `batch_and_prepare_render_phase`, then the work item buffers corresponding to shadow views will get deleted, and then the shadows will disappear. The fact that this is racy is what made it look like bevyengine#16922 solved the issue. In fact, it didn't solve the issue: it just perturbed the ordering on the build bots enough that the issue stopped appearing. However, on my system, the shadows still don't appear with bevyengine#16922. This commit solves the problem by making `delete_old_work_item_buffers` look at `ExtractedView`s, not `ViewTarget`s, preventing work item buffers corresponding to live shadow map views from being deleted.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 30, 2024
…actedView`s, not `ViewTarget`s. (#17039) OK, so this is tricky. Every frame, `delete_old_work_item_buffers` deletes the mesh preprocessing index buffers (a.k.a. work item buffers) for views that don't have `ViewTarget`s. This was always wrong for shadow map views, as shadow maps only have `ExtractedView` components, not `ViewTarget`s. However, before #16836, the problem was masked, because uploading the mesh preprocessing index buffers for shadow views had already completed by the time `delete_old_work_item_buffers` ran. But PR #16836 moved `delete_old_work_item_buffers` from the `ManageViews` phase to `PrepareResources`, which runs before `write_batched_instance_buffers` uploads the work item buffers to the GPU. This itself isn't wrong, but it exposed the bug, because now it's possible for work item buffers to get deleted before they're uploaded in `write_batched_instance_buffers`. This is actually intermittent! It's possible for the old work item buffers to get deleted, and then *recreated* in `batch_and_prepare_binned_render_phase`, which runs during `PrepareResources` as well, and under that system ordering, there will be no problem other than a little inefficiency arising from recreating the buffers every frame. But, if `delete_old_work_item_buffers` runs *after* `batch_and_prepare_render_phase`, then the work item buffers corresponding to shadow views will get deleted, and then the shadows will disappear. The fact that this is racy is what made it look like #16922 solved the issue. In fact, it didn't: it just perturbed the ordering on the build bots enough that the issue stopped appearing. However, on my system, the shadows still don't appear with #16922. This commit solves the problem by making `delete_old_work_item_buffers` look at `ExtractedView`s, not `ViewTarget`s, preventing work item buffers corresponding to live shadow map views from being deleted.
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this pull request
Jan 6, 2025
…actedView`s, not `ViewTarget`s. (bevyengine#17039) OK, so this is tricky. Every frame, `delete_old_work_item_buffers` deletes the mesh preprocessing index buffers (a.k.a. work item buffers) for views that don't have `ViewTarget`s. This was always wrong for shadow map views, as shadow maps only have `ExtractedView` components, not `ViewTarget`s. However, before bevyengine#16836, the problem was masked, because uploading the mesh preprocessing index buffers for shadow views had already completed by the time `delete_old_work_item_buffers` ran. But PR bevyengine#16836 moved `delete_old_work_item_buffers` from the `ManageViews` phase to `PrepareResources`, which runs before `write_batched_instance_buffers` uploads the work item buffers to the GPU. This itself isn't wrong, but it exposed the bug, because now it's possible for work item buffers to get deleted before they're uploaded in `write_batched_instance_buffers`. This is actually intermittent! It's possible for the old work item buffers to get deleted, and then *recreated* in `batch_and_prepare_binned_render_phase`, which runs during `PrepareResources` as well, and under that system ordering, there will be no problem other than a little inefficiency arising from recreating the buffers every frame. But, if `delete_old_work_item_buffers` runs *after* `batch_and_prepare_render_phase`, then the work item buffers corresponding to shadow views will get deleted, and then the shadows will disappear. The fact that this is racy is what made it look like bevyengine#16922 solved the issue. In fact, it didn't: it just perturbed the ordering on the build bots enough that the issue stopped appearing. However, on my system, the shadows still don't appear with bevyengine#16922. This commit solves the problem by making `delete_old_work_item_buffers` look at `ExtractedView`s, not `ViewTarget`s, preventing work item buffers corresponding to live shadow map views from being deleted.
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this pull request
Jan 6, 2025
…in retention. (bevyengine#16922) This commit makes the following changes: * `IndirectParametersBuffer` has been changed from a `BufferVec` to a `RawBufferVec`. This won about 20us or so on Bistro by avoiding `encase` overhead. * The methods on the `GetFullBatchData` trait no longer have the `entity` parameter, as it was unused. * `PreprocessWorkItem`, which specifies a transform-and-cull operation, now supplies the mesh instance uniform output index directly instead of having the shader look it up from the indirect draw parameters. Accordingly, the responsibility of writing the output index to the indirect draw parameters has been moved from the CPU to the GPU. This is in preparation for retained indirect instance draw commands, where the mesh instance uniform output index may change from frame to frame, while the indirect instance draw commands will be cached. We won't want the CPU to have to upload the same indirect draw parameters again and again if a batch didn't change from frame to frame. * `batch_and_prepare_binned_render_phase` and `batch_and_prepare_sorted_render_phase` now allocate indirect draw commands for an entire batch set at a time when possible, instead of one batch at a time. This change will allow us to retain the indirect draw commands for whole batch sets. * `GetFullBatchData::get_batch_indirect_parameters_index` has been replaced with `GetFullBatchData::write_batch_indirect_parameters`, which takes an offset and writes into it instead of allocating. This is necessary in order to use the optimization mentioned in the previous point. * At the WGSL level, `IndirectParameters` has been factored out into `mesh_preprocess_types.wgsl`. This is because we'll need a new compute shader that zeroes out the instance counts in preparation for a new frame. That shader will need to access `IndirectParameters`, so it was moved to a separate file. * Bins are no longer raw vectors but are instances of a separate type, `RenderBin`. This is so that the bin can eventually contain its retained batches.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
…actedView`s, not `ViewTarget`s. (bevyengine#17039) OK, so this is tricky. Every frame, `delete_old_work_item_buffers` deletes the mesh preprocessing index buffers (a.k.a. work item buffers) for views that don't have `ViewTarget`s. This was always wrong for shadow map views, as shadow maps only have `ExtractedView` components, not `ViewTarget`s. However, before bevyengine#16836, the problem was masked, because uploading the mesh preprocessing index buffers for shadow views had already completed by the time `delete_old_work_item_buffers` ran. But PR bevyengine#16836 moved `delete_old_work_item_buffers` from the `ManageViews` phase to `PrepareResources`, which runs before `write_batched_instance_buffers` uploads the work item buffers to the GPU. This itself isn't wrong, but it exposed the bug, because now it's possible for work item buffers to get deleted before they're uploaded in `write_batched_instance_buffers`. This is actually intermittent! It's possible for the old work item buffers to get deleted, and then *recreated* in `batch_and_prepare_binned_render_phase`, which runs during `PrepareResources` as well, and under that system ordering, there will be no problem other than a little inefficiency arising from recreating the buffers every frame. But, if `delete_old_work_item_buffers` runs *after* `batch_and_prepare_render_phase`, then the work item buffers corresponding to shadow views will get deleted, and then the shadows will disappear. The fact that this is racy is what made it look like bevyengine#16922 solved the issue. In fact, it didn't: it just perturbed the ordering on the build bots enough that the issue stopped appearing. However, on my system, the shadows still don't appear with bevyengine#16922. This commit solves the problem by making `delete_old_work_item_buffers` look at `ExtractedView`s, not `ViewTarget`s, preventing work item buffers corresponding to live shadow map views from being deleted.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
…in retention. (bevyengine#16922) This commit makes the following changes: * `IndirectParametersBuffer` has been changed from a `BufferVec` to a `RawBufferVec`. This won about 20us or so on Bistro by avoiding `encase` overhead. * The methods on the `GetFullBatchData` trait no longer have the `entity` parameter, as it was unused. * `PreprocessWorkItem`, which specifies a transform-and-cull operation, now supplies the mesh instance uniform output index directly instead of having the shader look it up from the indirect draw parameters. Accordingly, the responsibility of writing the output index to the indirect draw parameters has been moved from the CPU to the GPU. This is in preparation for retained indirect instance draw commands, where the mesh instance uniform output index may change from frame to frame, while the indirect instance draw commands will be cached. We won't want the CPU to have to upload the same indirect draw parameters again and again if a batch didn't change from frame to frame. * `batch_and_prepare_binned_render_phase` and `batch_and_prepare_sorted_render_phase` now allocate indirect draw commands for an entire batch set at a time when possible, instead of one batch at a time. This change will allow us to retain the indirect draw commands for whole batch sets. * `GetFullBatchData::get_batch_indirect_parameters_index` has been replaced with `GetFullBatchData::write_batch_indirect_parameters`, which takes an offset and writes into it instead of allocating. This is necessary in order to use the optimization mentioned in the previous point. * At the WGSL level, `IndirectParameters` has been factored out into `mesh_preprocess_types.wgsl`. This is because we'll need a new compute shader that zeroes out the instance counts in preparation for a new frame. That shader will need to access `IndirectParameters`, so it was moved to a separate file. * Bins are no longer raw vectors but are instances of a separate type, `RenderBin`. This is so that the bin can eventually contain its retained batches.
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
5 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 makes the following changes:
IndirectParametersBufferhas been changed from aBufferVecto aRawBufferVec. This won about 20us or so on Bistro by avoidingencaseoverhead.The methods on the
GetFullBatchDatatrait no longer have theentityparameter, as it was unused.PreprocessWorkItem, which specifies a transform-and-cull operation, now supplies the mesh instance uniform output index directly instead of having the shader look it up from the indirect draw parameters. Accordingly, the responsibility of writing the output index to the indirect draw parameters has been moved from the CPU to the GPU. This is in preparation for retained indirect instance draw commands, where the mesh instance uniform output index may change from frame to frame, while the indirect instance draw commands will be cached. We won't want the CPU to have to upload the same indirect draw parameters again and again if a batch didn't change from frame to frame.batch_and_prepare_binned_render_phaseandbatch_and_prepare_sorted_render_phasenow allocate indirect draw commands for an entire batch set at a time when possible, instead of one batch at a time. This change will allow us to retain the indirect draw commands for whole batch sets.GetFullBatchData::get_batch_indirect_parameters_indexhas been replaced withGetFullBatchData::write_batch_indirect_parameters, which takes an offset and writes into it instead of allocating. This is necessary in order to use the optimization mentioned in the previous point.At the WGSL level,
IndirectParametershas been factored out intomesh_preprocess_types.wgsl. This is because we'll need a new compute shader that zeroes out the instance counts in preparation for a new frame. That shader will need to accessIndirectParameters, so it was moved to a separate file.Bins are no longer raw vectors but are instances of a separate type,
RenderBin. This is so that the bin can eventually contain its retained batches.