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

Reintroduce clear_texture Metal/Vulkan/DX12 #1905

Merged
merged 13 commits into from
Sep 7, 2021
Merged

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Sep 4, 2021

Connections

Major puzzle piece for getting texture zero init in, see #1688
The idea is to implement the lazy texture inits with render target clears wherever possible, but for everything else we need to be able to perform clears using merely the COPY_DST usage, wgpu_hal's clear_texture is the answer for that.

Description

Main piece here is the reintroduction & implementation of clear_texture, but a few related things made it was well in here:

  • reenable CLEAR_COMMANDS feature accross apis
  • fill_buffer -> clear_buffer (makes things easier and simpler to find)
  • improved test for buffer & texture clearing (previously it would just pass since apis give you zero initialized most or even all of the time)
  • remove unused player test

Note that clear_texture is more restricted than originally:

  • requires COPY_DST usage
  • does not support depth/stencil formats
  • does not support multisampled formats
  • clears to zero only (same for clear_buffer)

For implementing zero init we will internally need to enforce either COPY_DST or RENDER_ATTACHMENT. Conveniently, the formats that clear_texture is not supported almost always come with RENDER_ATTACHMENT anyways.

Did not implemented clear_texture for GLES, I simply ran out of energy to do the tricky row copying again. Left as an exercise for the reader.

Testing

Only tested using the rather simplistic player test. We'll need to set up tests that iterate through more texture types/sizes/formats in the future (not only for clearing, but for all sort of things).
Ran it with Vulkan/DX12 (@ Win10, Nvidia) and Metal (@ MacOS, Intel)

@Wumpf Wumpf changed the title Reintroduce clear texture Metal/Vulkan/DX12 Reintroduce clear_texture Metal/Vulkan/DX12 Sep 4, 2021
wgpu/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

What is the semantics of CLEAR_COMMANDS now? Is it purely for textures? If so, we may need to indicate that in the name.

wgpu-hal/src/dx12/command.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/command.rs Show resolved Hide resolved
wgpu-hal/src/metal/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/metal/command.rs Outdated Show resolved Hide resolved
wgpu-hal/src/metal/mod.rs Outdated Show resolved Hide resolved
@Wumpf
Copy link
Member Author

Wumpf commented Sep 5, 2021

CLEAR_COMMANDS enables the user sided use of clear_texture and clear_buffer. How CLEAR_COMMANDS is now documented:

        /// Enables clear to zero for buffers & textures.
        ///
        /// Supported platforms:
        /// - All
        ///
        /// This is a native only feature.
        const CLEAR_COMMANDS = 1 << 36;

By renaming & simplifying clear_buffer I was hoping that the semantics are now more obvious :)

Arguably now that I explicitly state for the clear_texture that it might be inefficient, the main use is internal testing. Although arguably still convenient for various usecases :)

Fix incorrect use of texture.size in clear_texture for metal/dx12
Fix incorrect mip/layer ranges in clear_texture for metal/dx12
@Wumpf Wumpf requested a review from kvark September 5, 2021 21:36
@kvark
Copy link
Member

kvark commented Sep 6, 2021

By renaming & simplifying clear_buffer I was hoping that the semantics are now more obvious :)

Ok, that's the actual concern.
Before this PR, wgpu-hal was required to implement buffer clears (since it's needed by wgpu-core anyway). And now it's no longer required, and is hidden behind a feature? That seems like a wrong direction.

@Wumpf
Copy link
Member Author

Wumpf commented Sep 6, 2021

Ah okay this is a bit messed up right now, but already was before I reckon. wgpu-hal still is required to implement both clear_buffer and clear_texture because otherwise wgpu won't be able to implement zero-init which it is required to do. However, wgpu won't expose clear_buffer/texture unless asked so because it is not a feature of WebGPU. Allowing that would lead to non-portable applications.

So what I should do in this pr is that it's not wgpu-hal advertising the availability of the feature (it's always there), but wgpu itself?

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Discussed this on matrix. The short story is: clearing textures is a prerequisite for zero-initialization in wgpu-core, so separating it out from buffers wouldn't make much sense.

@kvark kvark merged commit df2a686 into gfx-rs:master Sep 7, 2021
@Wumpf Wumpf deleted the clear-texture branch September 7, 2021 07:07
@scoopr
Copy link
Contributor

scoopr commented Sep 7, 2021

It seems that on macos, all the examples now hang on waiting a mutex in acquire_surface for me, and I hand bisected it to this change.

@scoopr scoopr mentioned this pull request Sep 7, 2021
@Wumpf Wumpf mentioned this pull request Oct 24, 2021
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