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

Added new UNRESTRICTED_INDEX_BUFFER downlevel flag. #3157

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 1, 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
n/a

Description

WebGL doesn't support buffers that are are used both as vertex and index buffer. This introduces a new downlevel feature to check for this.

Testing
Tested same application in browser and desktop (Metal). Browser now correctly logs an error:

panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_buffer
      note: label = `rerun logo`
    Downlevel flags BUFFER_USAGE_COMBINE_VERTEX_INDEX are required but not supported on the device.
This is not an invalid use of WebGPU: the underlying API or device does not support enough features to be a fully compliant implementation. A subset of the features can still be used. If you are running this program on native and not in a browser and wish to work around this issue, call Adapter::downlevel_properties or Device::downlevel_properties to get a listing of the features the current platform supports.

not super expressive but good enough I reckon

Wumpf added 2 commits November 1, 2022 16:46
WebGL doesn't support buffers that are are used both as vertex and index buffer.
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.

The requirement is actually that index usage can only share with COPY_DST or COPY_SRC. INDEX | UNIFORM is also invalid. See https://registry.khronos.org/webgl/specs/latest/2.0/#BUFFER_OBJECT_BINDING.

There's also complexity where some buffer copies are invalid in webgl but this PR doesn't have to try to express that.

@cwfitzgerald
Copy link
Member

Maybe MIXED_USAGE_INDEX_BUFFER or something.

@Wumpf
Copy link
Member Author

Wumpf commented Nov 1, 2022

Thanks for digging that up! Went ahead and looked into doing transfer restrictions as well which is why I called it UNRESTRICTED_INDEX_BUFFER

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2022

Codecov Report

Merging #3157 (484725f) into master (32febc5) will decrease coverage by 0.00%.
The diff coverage is 54.05%.

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
- Coverage   64.73%   64.73%   -0.01%     
==========================================
  Files          81       81              
  Lines       38737    38782      +45     
==========================================
+ Hits        25076    25105      +29     
- Misses      13661    13677      +16     
Impacted Files Coverage Δ
wgpu-core/src/resource.rs 25.13% <ø> (ø)
wgpu-hal/src/dx11/adapter.rs 0.00% <0.00%> (ø)
wgpu-types/src/lib.rs 88.04% <0.00%> (-0.01%) ⬇️
wgpu-core/src/command/transfer.rs 69.92% <35.00%> (-0.85%) ⬇️
wgpu-core/src/device/mod.rs 66.37% <100.00%> (+0.06%) ⬆️
wgpu-hal/src/gles/adapter.rs 83.64% <100.00%> (+0.11%) ⬆️
wgpu/src/backend/direct.rs 55.25% <0.00%> (-0.65%) ⬇️
wgpu-hal/src/vulkan/adapter.rs 77.68% <0.00%> (-0.19%) ⬇️
wgpu-hal/src/dx12/adapter.rs 80.65% <0.00%> (-0.05%) ⬇️
wgpu/src/lib.rs 52.81% <0.00%> (ø)
... and 7 more

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

@Wumpf Wumpf requested a review from cwfitzgerald November 1, 2022 20:18
@Wumpf Wumpf changed the title Added new BUFFER_USAGE_COMBINE_VERTEX_INDEX downlevel flag. Added new UNRESTRICTED_INDEX_BUFFER downlevel flag. Nov 1, 2022
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/transfer.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/transfer.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf requested a review from cwfitzgerald November 3, 2022 16:39
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
@cwfitzgerald cwfitzgerald merged commit b838b08 into gfx-rs:master Nov 7, 2022
@Wumpf Wumpf deleted the mix-vertex-index-buffer-usage-downlevel-feature branch November 7, 2022 21:41
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