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

Renaming CommandEncoder::clearBuffer to CommandEncoder::fillBuffer #2174

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

UpsettingBoy
Copy link
Contributor

@UpsettingBoy UpsettingBoy commented Nov 12, 2021

Connections
#2170

Description
This PR adds fill_buffer to CommandEncoder for Vulkan and D3D12. Metal and GLES yet to be done (I cannot implement them 😢).

There are probably some changes to be made, mainly in the d3d12 implementation. e.g There are no deallocations over the CPU only descriptor heap. Also, I think the changes I made for d3d12 FixedSizeHeap might be problematic on multi-threading
scenarios.

EDIT: For now is just a rename to keep up with the spec.

Testing
I only tested this PR with a small change to the hello-compute example. No other testing is done :(

@kvark
Copy link
Member

kvark commented Nov 13, 2021

Thank you for taking a stab at this!
We actually don't need this new fill_buffer method in wgpu-hal, there is already clear_buffer for exactly the same purpose. The API difference between them is the value: u8, but if you look at WebGPU spec, there isn't a value there either (because it would be more difficult to implement) - https://gpuweb.github.io/gpuweb/

So all that needs to be done here is exposing the method to wgpu-rs

@UpsettingBoy
Copy link
Contributor Author

Ah, alright, I misread part of the discussions over gpuweb haha.
So this would be more of a renaming from clear_buffer to fill_buffer, wouldn't it?

@kvark
Copy link
Member

kvark commented Nov 13, 2021

We had it called fill_buffer at some point, then renamed to clear_buffer. I guess it's time to switch it back now :)
cc @Wumpf who did most of this work.

@kvark
Copy link
Member

kvark commented Nov 13, 2021

Another thing is - this API should now be available without any features, previously it was behind a feature.

@Wumpf
Copy link
Member

Wumpf commented Nov 13, 2021

oh cool didn't know that this is now in the spec. Nice!
Wanted to change the meaning of the extension a little bit towards "this is what zero init does when it hits", but that would change that again then (:
Wumpf@127cbab

@UpsettingBoy
Copy link
Contributor Author

Alright, but does clear_texture remain under CLEAR_COMMANDS?

Also, by gpuweb/gpuweb#2208 comment, value may be added in the future (is on the workings I think: gpuweb/gpuweb#2290) so, do you think is better to stall this PR until then and create another one to keep up with the spec in the meantime?

@Wumpf
Copy link
Member

Wumpf commented Nov 14, 2021

Alright, but does clear_texture remain under CLEAR_COMMANDS?

yes, and we should rename it to CLEAR_TEXTURES, but I can do that later; going to make some changes to it anyways.
I'd say let's add value when it enters the spec. With zero we can implement it on all backends today, but adding a value means that we need to solve this with shaders on some backends

* fill_buffer does not require CLEAR_BUFFER feature
@UpsettingBoy UpsettingBoy reopened this Nov 14, 2021
@UpsettingBoy UpsettingBoy changed the title Implementation of CommandEncoder::fillBuffer Renaming CommandEncoder::clearBuffer to CommandEncoder::fillBuffer Nov 14, 2021
@UpsettingBoy
Copy link
Contributor Author

Okay, so I removed all the previous commits from this branch. They will still remain on my branch (https://github.com/UpsettingBoy/wgpu/tree/cmd-encoder-value) so when the spec adds value there'll be some work done :)

@Wumpf I left CLEAR_COMMAND untouched for you to change.

@UpsettingBoy UpsettingBoy marked this pull request as ready for review November 14, 2021 13:02
wgpu-core/src/command/clear.rs Show resolved Hide resolved
@kvark kvark merged commit 052cb3d into gfx-rs:master Nov 15, 2021
@UpsettingBoy UpsettingBoy deleted the cmd-encoder-fill-buffer branch November 15, 2021 08:39
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.

3 participants