Skip to content

Reintroduce clear_texture Metal/Vulkan/DX12#1905

Merged
kvark merged 13 commits intogfx-rs:masterfrom
Wumpf:clear-texture
Sep 7, 2021
Merged

Reintroduce clear_texture Metal/Vulkan/DX12#1905
kvark merged 13 commits intogfx-rs:masterfrom
Wumpf:clear-texture

Conversation

@Wumpf
Copy link
Copy Markdown
Member

@Wumpf Wumpf commented Sep 4, 2021 •
edited
Loading

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
Wumpf
Wumpf commented Sep 4, 2021
View reviewed changes
kvark
kvark requested changes Sep 5, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@Wumpf
Copy link
Copy Markdown
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 :)

Wumpf added 2 commits September 5, 2021 23:36
@Wumpf
comment & namespace fixes
b9dde2f
@Wumpf
Extent3d has now a simple mipmap calculating function
dbb8b1a 
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
Copy Markdown
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
Copy Markdown
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?

kvark
kvark approved these changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Closed
@Wumpf Wumpf mentioned this pull request Oct 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kvark kvark kvark approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Wumpf @kvark @scoopr