Better handle destroying textures and buffers #4657
Merged
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
Before this commit, explicitly destroying a texture or a buffer (without dropping it) schedules the asynchronous destruction of its raw resources but does not actually mark it as destroyed. This can cause some incorrect behavior, for example mapping a buffer after destroying it does not cause a validation error (and later panics due to the map callback being dropped without being called). This Commit adds `Storage::take_and_mark_destroyed` for use in `destroy` methods. Since it puts the resource in the error state, other methods properly catch that the resource is no longer usable when attempting to access it and raise validation errors. There are other resource types that require similar treatment and will be addressed in followup work.
ErichDonGubler
approved these changes
Nov 9, 2023
nical
added a commit
to nical/wgpu
that referenced
this pull request
Nov 13, 2023
in gfx-rs#4657 the destroy implementation was made to remove the value from the storage and leave an error variant in its place. Unfortunately this causes some issues with the tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter is not in storage anymore. To work around that, this commit adds a `Destroyed` variant in storage which keeps the value so that the tracking behavior is preserved while still making sure that most accesses to the destroyed resource lead to validation errors. ... Except for submitted command buffers that need to be consumed right away. These are replaced with `Element::Error` like before this commit.
nical
added a commit
to nical/wgpu
that referenced
this pull request
Nov 13, 2023
in gfx-rs#4657 the destroy implementation was made to remove the value from the storage and leave an error variant in its place. Unfortunately this causes some issues with the tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter is not in storage anymore. To work around that, this commit adds a `Destroyed` variant in storage which keeps the value so that the tracking behavior is preserved while still making sure that most accesses to the destroyed resource lead to validation errors. ... Except for submitted command buffers that need to be consumed right away. These are replaced with `Element::Error` like before this commit.
1 task
nical
added a commit
to nical/wgpu
that referenced
this pull request
Nov 13, 2023
in gfx-rs#4657 the destroy implementation was made to remove the value from the storage and leave an error variant in its place. Unfortunately this causes some issues with the tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter is not in storage anymore. To work around that, this commit adds a `Destroyed` variant in storage which keeps the value so that the tracking behavior is preserved while still making sure that most accesses to the destroyed resource lead to validation errors. ... Except for submitted command buffers that need to be consumed right away. These are replaced with `Element::Error` like before this commit.
nical
added a commit
to nical/wgpu
that referenced
this pull request
Nov 13, 2023
in gfx-rs#4657 the destroy implementation was made to remove the value from the storage and leave an error variant in its place. Unfortunately this causes some issues with the tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter is not in storage anymore. To work around that, this commit adds a `Destroyed` variant in storage which keeps the value so that the tracking behavior is preserved while still making sure that most accesses to the destroyed resource lead to validation errors. ... Except for submitted command buffers that need to be consumed right away. These are replaced with `Element::Error` like before this commit.
nical
added a commit
that referenced
this pull request
Nov 14, 2023
* Keep the value in its storage after destroy in #4657 the destroy implementation was made to remove the value from the storage and leave an error variant in its place. Unfortunately this causes some issues with the tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter is not in storage anymore. To work around that, this commit adds a `Destroyed` variant in storage which keeps the value so that the tracking behavior is preserved while still making sure that most accesses to the destroyed resource lead to validation errors. ... Except for submitted command buffers that need to be consumed right away. These are replaced with `Element::Error` like before this commit. Co-authored-by: Teodor Tanasoaia <[email protected]>
This was referenced Nov 15, 2023
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.
Checklist
cargo clippy.Description
Before this commit, explicitly destroying a texture or a buffer (without dropping it) schedules the asynchronous destruction of its raw resources but does not actually mark it as destroyed. This can cause some incorrect behavior, for example mapping a buffer after destroying it does not cause a validation error (and later panics due to the map callback being dropped without being called).
This Commit adds
Storage::take_and_mark_destroyedfor use indestroymethods. Since it puts the resource in the error state, other methods properly catch that the resource is no longer usable when attempting to access it and raise validation errors.There are other resource types that require similar treatment and will be addressed in followup work.
Testing
Added a couple of tests. This is also covered by various bits of the CTS in Firefox.