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

Add AddressMode::ClampToBorder behind a feature #891

Merged
merged 1 commit into from
Aug 22, 2020

Conversation

jshrake
Copy link
Contributor

@jshrake jshrake commented Aug 22, 2020

Connections
Closes #890
Linked to gfx-rs/wgpu-rs#526

Description

  • Adds support for border sampler addressing behind a new feature Features::ADDRESS_MODE_CLAMP_TO_BORDER.
  • Adds a new enum SamplerBorderColor so that users can optionally specify a supported border color on the SamplerDescriptor.

Testing
Tested against the Metal (MacOS 10.15) and Vulkan (Ubuntu 18.04) backends.

Ran the wgpu-rs/cube example and switched the address modes to AddressMode::ClampToBorder, without enabling the feature, and confirmed I received an error message. Ran the same example with the new feature specified in the optional_features return and confirmed that the example ran and looks reasonable.

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.

Checker summary (by rust_clippy):
The tool has found 0 warnings, 1 errors.

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.

Looks good! Some changes/questions

wgpu-core/src/resource.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
/// Supported platforms:
/// - DX12
/// - Vulkan
/// - Metal (macOS 10.12+ only)
Copy link
Member

Choose a reason for hiding this comment

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

If 10.12 is required, as of right now the check only checks !iOS, so this will report being available on the older versions of mac. Not sure if you classify that as an issue @kvark.

wgpu-types/src/lib.rs Show resolved Hide resolved
@jshrake
Copy link
Contributor Author

jshrake commented Aug 22, 2020

Setting the border_color to an arbitrary color (like wgpu::Color::Red) on MacOS results in the follow error case https://github.com/gfx-rs/gfx/blob/375f1dc078affdb610a0b1a45d218b788a83179e/src/backend/metal/src/device.rs#L843-L853

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.

This is great, thank you for tackling this issue so quickly!
Just a few minor things to correct, and we are good to go

wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
wgpu-core/src/instance.rs Show resolved Hide resolved
@@ -397,6 +400,8 @@ pub enum CreateSamplerError {
InvalidClamp(u8),
#[error("cannot create any more samplers")]
TooManyObjects,
#[error("AddressMode::ClampToBorder requires feature ADDRESS_MODE_CLAMP_TO_BORDER")]
MissingSamplerFeature,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to make this consistent with the other variants. So this should be MissingFeature(wgt::Feature), and the doc comment should talk specifically about the clamp-to-border, so that we can use this for other features later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Let me know if the current message + doc comment is acceptable.

@kvark
Copy link
Member

kvark commented Aug 22, 2020

Setting the border_color to an arbitrary color (like wgpu::Color::Red) on MacOS results in the follow error case

Oh that's fun! I think we should just make the extension to support only a limited set of colors that Metal supports, then.
Otherwise, it will not be available on any Metal.

@jshrake
Copy link
Contributor Author

jshrake commented Aug 22, 2020

Oh that's fun! I think we should just make the extension to support only a limited set of colors that Metal supports, then.
Otherwise, it will not be available on any Metal.

I introduced a new enum SamplerBorderColor to represent the common subset of colors supported across these platforms:

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.

Looks great, thank you!
bors r+

@bors
Copy link
Contributor

bors bot commented Aug 22, 2020

@bors bors bot merged commit 0bb6bb8 into gfx-rs:master Aug 22, 2020
@jshrake jshrake deleted the clamp-to-border branch August 22, 2020 23:41
bors bot added a commit to gfx-rs/gfx that referenced this pull request Aug 23, 2020
3347: Refactor border color, add feature for comparison mutable samplers r=kvark a=kvark

Contributes towards #3346
PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [ ] tested examples with the following backends:

cc @jshrake (related to gfx-rs/wgpu#891)

Co-authored-by: Dzmitry Malyshau <[email protected]>
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Aug 23, 2020
526: Add optional border_color to SamplerDescriptor r=kvark a=jshrake

Depends on gfx-rs/wgpu#891

Co-authored-by: Justin Shrake <[email protected]>
@kvark kvark mentioned this pull request May 17, 2021
bors bot added a commit that referenced this pull request May 17, 2021
1391: Simplify features r=cwfitzgerald a=kvark

**Connections**
Fixes #1390
Makes vertex writable storage optional (native-only for now)
Fixes the follow-up to #891 

**Description**
Refactors the code so that adding new features and maintaining current ones is much easier.
Refactors the way missing features are checked and reported, DRY way.

**Testing**
Not really tested

Co-authored-by: Dzmitry Malyshau <[email protected]>
kvark pushed a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
526: Add optional border_color to SamplerDescriptor r=kvark a=jshrake

Depends on gfx-rs#891

Co-authored-by: Justin Shrake <[email protected]>
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.

Extension for "border" sampler addressing mode
3 participants