Unbreak shadows by retaining work item buffers corresponding to ExtractedViews, not ViewTargets.#17039
Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom Dec 30, 2024
Merged
Conversation
`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.
pcwalton
added
P-Regression
BenjaminBrienen
approved these changes
Dec 30, 2024
MalekiRe
approved these changes
Dec 30, 2024
Contributor
MalekiRe
left a comment
MalekiRe
left a comment
There was a problem hiding this comment.
A rendering PR I actually understand!
LGTM
Contributor
I know, right?! 😆 |
BenjaminBrienen
added
C-Bug
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.
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.
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.
OK, so this is tricky. Every frame,
delete_old_work_item_buffersdeletes the mesh preprocessing index buffers (a.k.a. work item buffers) for views that don't haveViewTargets. This was always wrong for shadow map views, as shadow maps only haveExtractedViewcomponents, notViewTargets. However, before #16836, the problem was masked, because uploading the mesh preprocessing index buffers for shadow views had already completed by the timedelete_old_work_item_buffersran. But PR #16836 moveddelete_old_work_item_buffersfrom theManageViewsphase toPrepareResources, which runs beforewrite_batched_instance_buffersuploads 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 inbatch_and_prepare_binned_render_phase, which runs duringPrepareResourcesas well, and under that system ordering, there will be no problem other than a little inefficiency arising from recreating the buffers every frame. But, ifdelete_old_work_item_buffersruns afterbatch_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_bufferslook atExtractedViews, notViewTargets, preventing work item buffers corresponding to live shadow map views from being deleted.