Do texture init via clear passes when possible #2307
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kvark
reviewed
Dec 19, 2021
kvark
reviewed
Dec 19, 2021
kvark
reviewed
Dec 19, 2021
|
there's apparently some DX12 test failures during tests that I need to look into |
kvark
reviewed
Dec 19, 2021
This was referenced Dec 23, 2021
this has mostly implications on the constraints, but also allows a more leaky documentation which makes sense for this non-standard function as there is no other place to look it up
pending inits need to store Stored textures now though, causing more ref count bumping
…_via_buffer_copies
kvark
approved these changes
Dec 27, 2021
|
Running this on https://github.com/niklaskorz/linon/tree/wgpu-0.12 gives me the following error instead of the current "Texture needs to have the COPY_DST flag" (#2309):
full stack trace |
|
@niklaskorz looks like this will be addressed by the very PR we are commenting on |
That's what I was hoping :) The error I posted occurs when using this PR as wgpu crate, which is why I thought it relevant for the discussion here. |
|
@niklaskorz reproed it and looking for a solution, I think I know what to do but looking for a nice way to add a test for this in wgpu. Meanwhile, you can fix/workaround this in your application by passing a clear color to egui in your application: @kvark please hold on with merging until I have this done as well. Meanwhile, you may want to complain about |
|
all done in f80a13d . Not pretty: For testing I needed to hijack an existing example so I didn't need to go through all the window creation stuff. Destruction of the new clear target every surface has is in two places and I'm actually not sure why this is ok to do it there (expecting it to be still in use, but neither DX12/Vulkan complain about this even under resize which causes discarded surfaces) |
kvark
approved these changes
Dec 28, 2021
kvark
added
the
PR: needs back-porting
Wumpf
added a commit
to Wumpf/wgpu
that referenced
this pull request
Dec 29, 2021
* CLEAR_COMMANDS extension is now more of a window into wgpu zero-init this has mostly implications on the constraints, but also allows a more leaky documentation which makes sense for this non-standard function as there is no other place to look it up * clear_texture via renderpasses wip * 3D depth textures are no longer allowed, volumes are always cleared via CPY_DST * cleanup texture's clear_views * rename CLEAR_COMMANDS to CLEAR_TEXTURE * separate clear_texture into reusable & more descriptive parts * texture clear views are now created ahead of time * discarded surface fixup goes through new clear_texture method now * onq ueue texture initialization now goes threw clear_texture pending inits need to store Stored textures now though, causing more ref count bumping * texture init on queue_write_texture now also goes through new clear_texture * transfer functions on commandbuffer use now new texture init route * merge collect_zero_buffer_copies_for_clear_texture into clear_texture_via_buffer_copies * clear functions now take TextureInitRange * Fix clippy lints * command_encoder_clear_texture no longer takes write lock on texture * TextureClearMode encodes now is_color * code cleanup, mostly about `use` * Handle volume textures in clear_texture_via_render_passes properly * texture clear no longer requires id::Stored * init tracking fixes for volumes and init on partial subresource writes * texture creation enforces COPY_DST only if absolutely necessary * unrolled functional chain, reduce unsafe scope size * fix clippy lints * clear_texture test no longer creates 1D textures see gfx-rs#2323 * 3D textures are no longer cleared as render target since this isn't supported on Metal * fix deno building issue, fix formatting * TextureInner::Surface can now be zero initialized
kvark
pushed a commit
that referenced
this pull request
Dec 29, 2021
* CLEAR_COMMANDS extension is now more of a window into wgpu zero-init this has mostly implications on the constraints, but also allows a more leaky documentation which makes sense for this non-standard function as there is no other place to look it up * clear_texture via renderpasses wip * 3D depth textures are no longer allowed, volumes are always cleared via CPY_DST * cleanup texture's clear_views * rename CLEAR_COMMANDS to CLEAR_TEXTURE * separate clear_texture into reusable & more descriptive parts * texture clear views are now created ahead of time * discarded surface fixup goes through new clear_texture method now * onq ueue texture initialization now goes threw clear_texture pending inits need to store Stored textures now though, causing more ref count bumping * texture init on queue_write_texture now also goes through new clear_texture * transfer functions on commandbuffer use now new texture init route * merge collect_zero_buffer_copies_for_clear_texture into clear_texture_via_buffer_copies * clear functions now take TextureInitRange * Fix clippy lints * command_encoder_clear_texture no longer takes write lock on texture * TextureClearMode encodes now is_color * code cleanup, mostly about `use` * Handle volume textures in clear_texture_via_render_passes properly * texture clear no longer requires id::Stored * init tracking fixes for volumes and init on partial subresource writes * texture creation enforces COPY_DST only if absolutely necessary * unrolled functional chain, reduce unsafe scope size * fix clippy lints * clear_texture test no longer creates 1D textures see #2323 * 3D textures are no longer cleared as render target since this isn't supported on Metal * fix deno building issue, fix formatting * TextureInner::Surface can now be zero initialized
|
#2327 is published in wgpu-core-0.12.1 |
kvark
removed
the
PR: needs back-porting
bors bot
pushed a commit
to bevyengine/bevy
that referenced
this pull request
Dec 29, 2021
# Objective - `Msaa` was disabled in webgl due to a bug in wgpu - Bug has been fixed (gfx-rs/wgpu#2307) and backported (gfx-rs/wgpu#2327), and updates for [`wgpu-core`](https://crates.io/crates/wgpu-core/0.12.1) and [`wgpu-hal`](https://crates.io/crates/wgpu-hal/0.12.1) have been released ## Solution - Remove custom config for `Msaa` in webgl - I also changed two options that were using the arch instead of the `webgl` feature. it shouldn't change much for webgl, but could help if someone wants to target wasm but not webgl2 Co-authored-by: François <[email protected]>
mockersf
added a commit
to mockersf/bevy
that referenced
this pull request
Jan 1, 2022
# Objective - `Msaa` was disabled in webgl due to a bug in wgpu - Bug has been fixed (gfx-rs/wgpu#2307) and backported (gfx-rs/wgpu#2327), and updates for [`wgpu-core`](https://crates.io/crates/wgpu-core/0.12.1) and [`wgpu-hal`](https://crates.io/crates/wgpu-hal/0.12.1) have been released ## Solution - Remove custom config for `Msaa` in webgl - I also changed two options that were using the arch instead of the `webgl` feature. it shouldn't change much for webgl, but could help if someone wants to target wasm but not webgl2 Co-authored-by: François <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Should fix #2149 - haven't confirmed since I don't a repro at hand.
Changes clear extension semantics a bit as discussed on #2174
Description
Previously, render target/depth textures couldn't be initialized at all if they not also were COPY_DST, violating the spec.
Now, we always will use render passes when possible since it's expected to be faster.
All this comes with an overhaul of the texture clearing mechanisms as we tried to optimize things based on the assumption of texture-init == copy zero_buffer:
TextureClearModewhich may bring with it pre-created render views for clearingOther smaller but important things in this PR:
Testing
Tests in tests/clear_texture.rs clear now depth/stencil formats as well (this was not possible previously)
Ran
cargo testlocally (win10)