Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep the value in its storage after destroy #4678

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Conversation

nical
Copy link
Contributor

@nical nical commented Nov 13, 2023

Checklist

  • Run cargo clippy.

Connections

Fixes #4668

Description

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 buffer tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter does not need to be in storage anymore. To work around that, this commit adds a Destroyed variant in storage which keeps the value so that the previous 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.

It's a fair bit unsavory to have the two ways to do the same thing in principle, but a better solution will take more time to bake and would conflict unnecessarily with the arcanization work.

Testing

An existing test was altered to cover the regression.

@nical nical requested a review from a team as a code owner November 13, 2023 16:59
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.
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

wgpu-core/src/identity.rs Outdated Show resolved Hide resolved
wgpu-core/src/identity.rs Outdated Show resolved Hide resolved
nical and others added 2 commits November 14, 2023 15:07
Co-authored-by: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com>
Co-authored-by: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com>
@nical nical enabled auto-merge (squash) November 14, 2023 14:08
@nical nical merged commit a697e43 into gfx-rs:trunk Nov 14, 2023
27 checks passed
@nical nical deleted the drop-destroy branch November 14, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commit #4657 appears to introduce a race condition
2 participants