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::ClampToZero #2364

Merged
merged 12 commits into from
Jan 15, 2022
Merged

add AddressMode::ClampToZero #2364

merged 12 commits into from
Jan 15, 2022

Conversation

laptou
Copy link
Contributor

@laptou laptou commented Jan 6, 2022

Description

On iOS, the "clamp to border" sampler address mode is not supported, however, the "clamp to zero" address mode does seem to be supported, so I would like to use that instead. I have added a new corresponding sampler address mode wgpu::AddressMode::ClampToZero. On the Metal backend I implement this using the native "clamp to zero" address mode, while I implement it using the "clamp to border" address mode on every other backend.

Testing
Test this change by verifying that:

  • wgpu::AddressMode::ClampToZero works as expected on all platforms

@laptou
Copy link
Contributor Author

laptou commented Jan 6, 2022

I could also have implemented this by trying to use ClampToBorder on the Metal backend, and then falling back to ClampToZero if it is not supported and if the border colour is transparent black. However, if the border colour is not transparent black, then it seems like we would have to either ignore it or panic, so I'm not sure that that design would be better.

@kvark
Copy link
Member

kvark commented Jan 7, 2022

Thank you for filing this! I didn't know about clamp-to-zero being a separate mode on Metal.
Changes like this should go through https://github.com/gpuweb/gpuweb first, unless you are adding a native-only extension.

There is a problem with your proposal though. Clamp-to-zero returns something that depends on the texture format:

Out-of-range texture coordinates return transparent zero (0,0,0,0) for images with an alpha channel and return opaque zero (0,0,0,1) for images without an alpha channel.

This is different from the clamp-to-border semantics that other backends expose. So it doesn't look like we can implement this portably.

@laptou
Copy link
Contributor Author

laptou commented Jan 7, 2022

What should I do? I would love to be able to use a clamping address mode on all platforms that I'm targeting (which includes iOS), and if I can't then my shaders will have to become more complicated (and probably slower) because I would have to introduce bounds checks for my texture coordinates.

Would it work to simply document the differences in behaviour (i.e., "On the Metal backend, the sampler behaviour depends on whether the texture has an alpha channel, while on other backends this is equivalent to ClampToBorder with a transparent black border colour"), or is there a workaround/hack that I can use to make this work in my project without merging a change like this into wgpu?

Changes like this should go through gpuweb/gpuweb first, unless you are adding a native-only extension.

Since I am not targeting the web with my application, I'm going to say that this is a native-only extension 😅 someone else can deal with getting this past the W3C if they need it on the web

@kvark
Copy link
Member

kvark commented Jan 7, 2022

Ok, let's make it a native-only feature then. We'll have to document the behavior guarantees of Metal, in a similar way you suggested.
It's supported 100% on native, but it has to be a feature because the Web doesn't expose this, and we want applications on wgpu-rs to be targeting the Web seamlessly.

@laptou
Copy link
Contributor Author

laptou commented Jan 7, 2022

it has to be a feature

What do you mean by this? Do you mean that I should put AddressMode::ClampToZero behind a Cargo feature flag? Or what do I need to do for this change to move forward?

@kvark
Copy link
Member

kvark commented Jan 7, 2022

I mean a feature in WebGPU sense, it needs to be a flag in wgt::Features, similar to how clamp-to-border is exposed

@laptou
Copy link
Contributor Author

laptou commented Jan 7, 2022

Oh, I already implemented that (see here). The feature is available for any adapter where clamp-to-border is available, except for the Metal adapter where it is always available.

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.

Unfortunately, we are hitting another problem here.
I think a solution would require this new behavior to be a part of enum SamplerBorderColor enum (e.g. AutoBlack) instead of the sampler addressing mode enum. This would ensure there is never a conflict between clamping to zero and clamping to other borders.

wgpu-hal/src/dx12/device.rs Outdated Show resolved Hide resolved
@laptou laptou requested a review from kvark January 11, 2022 22:39
@laptou
Copy link
Contributor Author

laptou commented Jan 12, 2022

Okay, I implemented it the way you suggested, but I just ran into a problem with this design when trying to test it on iOS: AddressMode::ClampToBorder is not allowed on iOS with the current settings, so we would need to put in a special case where it checks the border color and panics if we're on iOS and the border colour is not AutoBlack.

@kvark
Copy link
Member

kvark commented Jan 12, 2022

Okay, I implemented it the way you suggested, but I just ran into a problem with this design when trying to test it on iOS: AddressMode::ClampToBorder is not allowed on iOS with the current settings, so we would need to put in a special case where it checks the border color and panics if we're on iOS and the border colour is not AutoBlack.

The way validation would work then is the following:

  • if ADDRESS_MODE_CLAMP_TO_ZERO is enabled (your new feature), then it's allowed to use border_color = Some(Zero) (or AutoBlack, but I think Zero is closer to the feature name)
  • if ADDRESS_MODE_CLAMP_TO_BORDER is enabled, then it's allowed to use border_color = Some(other border colors).
  • otherwise, border_color has to be None.

@laptou
Copy link
Contributor Author

laptou commented Jan 14, 2022

done @kvark

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.

Very nice!

wgpu-hal/src/metal/conv.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
@laptou
Copy link
Contributor Author

laptou commented Jan 14, 2022

done

@kvark kvark enabled auto-merge (squash) January 14, 2022 19:50
auto-merge was automatically disabled January 14, 2022 20:17

Head branch was pushed to by a user without write access

@kvark kvark enabled auto-merge (squash) January 15, 2022 05:04
@kvark kvark merged commit 0183e7d into gfx-rs:master Jan 15, 2022
@laptou laptou deleted the add-address-mode branch January 15, 2022 20:40
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.

2 participants