-
Notifications
You must be signed in to change notification settings - Fork 928
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
Introduce ExternalStagingBuffer
for staging buffer uses outside of wgpu-core
#6090
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current changes LGTM so far, but the unresolved TODO
means I'm not sure if this should merge yet.
@@ -483,6 +482,10 @@ impl Global { | |||
.and_then(Arc::into_inner) | |||
.ok_or_else(|| QueueWriteError::Transfer(TransferError::InvalidBufferId(buffer_id)))?; | |||
|
|||
// TODO: bubble up safety requirements, they currently hold but we should make functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(blocking): Should this be resolved before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't intend to resolve this in this PR. After degenerification and removal of registries we might not even need to resolve it since we can make the API safe by not giving users the pointer at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, shall we file an issue, then, and remove this comment? If we don't want an issue to track it, then I wonder if we should even include the comment.
Will downgrade to non-blocking with a response to the above, trusting that you've thought this through.
@teoxoy: We gonna merge this? 👀 |
It's not a priority right now, I was thinking I might get to remove the registries first which would make the situation clearer with regards to the safety discussion. |
Demoting to draft due to plans to remove registries first. |
Connections
-
Description
I find that this refactor makes things clearer since most of the time we use staging buffers only internally.
Testing
Existing tests.