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

Remove DEPTH24PLUS_STENCIL8 feature #3151

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

HexyWitch
Copy link
Contributor

@HexyWitch HexyWitch commented Oct 30, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
#3112

Description
Removes the DEPTH24PLUS_STENCIL8 feature. The feature is not present in the WebGPU spec and was blocking the use of the Depth24PlusStencil8 texture format on several platforms that support it. Tests have been adjusted to include Depth24PlusStencil8 in clear_texture_2d_uncompressed instead of having its own test.

Testing
The change passes tests, and has been briefly tested locally on a Windows machine with an RTX 2080, using the Vulkan and WebGL backends.

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2022

Codecov Report

Merging #3151 (fe5b48c) into master (51b34c5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3151      +/-   ##
==========================================
+ Coverage   64.76%   64.80%   +0.04%     
==========================================
  Files          81       81              
  Lines       38764    38751      -13     
==========================================
+ Hits        25106    25114       +8     
+ Misses      13658    13637      -21     
Impacted Files Coverage Δ
wgpu-hal/src/dx12/adapter.rs 80.65% <ø> (-0.05%) ⬇️
wgpu-hal/src/vulkan/adapter.rs 77.68% <ø> (-0.19%) ⬇️
wgpu-types/src/lib.rs 88.04% <100.00%> (-0.01%) ⬇️
wgpu-core/src/hub.rs 60.67% <0.00%> (-0.16%) ⬇️
wgpu-hal/src/gles/egl.rs 37.95% <0.00%> (+0.12%) ⬆️
wgpu-hal/src/gles/queue.rs 61.65% <0.00%> (+0.27%) ⬆️
wgpu-hal/src/gles/device.rs 80.06% <0.00%> (+0.31%) ⬆️
wgpu-hal/src/gles/command.rs 70.08% <0.00%> (+1.16%) ⬆️
wgpu-hal/src/gles/conv.rs 74.35% <0.00%> (+1.44%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@maxammann
Copy link
Contributor

maxammann commented Oct 31, 2022

I just tested this on my WebGL build of maplibre-rs and it still errors with:

ERROR /home/max/.cargo/git/checkouts/wgpu-746059844e0eb641/fe5b48c/wgpu/src/backend/direct.rs:908 
Error in Adapter::request_device: unsupported features were requested: DEPTH32FLOAT_STENCIL8

Tested on Firefox and Chrome with WebGL.

EDIT: Depth24PlusStencil8 works on Firefox with WebGL. So DEPTH32FLOAT_STENCIL8 is probably just unsupported in browsers.

@maxammann
Copy link
Contributor

The feature is not present in the WebGPU spec and was blocking the use of the Depth24PlusStencil8 texture format on several platforms that support it.

I think you meant here Depth32FloatStencil8 right?

maxammann added a commit to maxammann/maplibre-rs that referenced this pull request Oct 31, 2022
@HexyWitch
Copy link
Contributor Author

I think you meant here Depth32FloatStencil8 right?

No, this change addresses the DEPTH24PLUS_STENCIL8 feature, which wasn't marked as supported on some backends, making the Depth24PlusStencil8 texture format unavailable even though it was actually supported.

Depth32FloatStencil8 is gated by the DEPTH32FLOAT_STENCIL8 feature, which is in the WebGPU spec and so should probably stay? I don't know what support looks like for Depth32FloatStencil8 on the different backends but it's unrelated to this change.

@cwfitzgerald cwfitzgerald self-assigned this Nov 1, 2022
maxammann added a commit to maplibre/maplibre-rs that referenced this pull request Nov 2, 2022
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Thanks! Will backport shortly

@cwfitzgerald cwfitzgerald merged commit a31cd2e into gfx-rs:master Nov 2, 2022
cwfitzgerald pushed a commit to cwfitzgerald/wgpu that referenced this pull request Nov 2, 2022
cwfitzgerald pushed a commit to cwfitzgerald/wgpu that referenced this pull request Nov 2, 2022
cwfitzgerald added a commit that referenced this pull request Nov 2, 2022
Co-authored-by: Lilith <lilith@triplehex.dev>
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.

4 participants