Implement Anisotropic Filtering Extension and Expose Extension Interface #690

cwfitzgerald · 2020-06-01T05:41:19Z

Connections

Resolves #568.

Description

This PR solves two tightly related problems:

Adds the ability to create a sampler with anisotropic filtering
Adds the webgpu interface for getting the limits/extensions on adapters and devies.

Testing

I have tested this against my codebase which uses it. Adding an example in wgpu-rs on how to use extensions/limits would be good, especially as we have native/webgpu extensions, but can come as a future enhancement, once we feel the extension situation has stabilized a bit.

TODO

Allow wgpu-rs to call limit/extension interface (Add Extension/Limit Interface wgpu-rs#338)
Determine how wgpu-native is going to adopt this functionality.
After discussing Proposal for Handling Native/Webgpu Extensions and Expanding Limits #691, what changes need to be made.

scoopr · 2020-06-01T10:01:20Z

wgpu-types/src/lib.rs

+/// Before an extension is used, one must check the actual supported
+/// extensions by ...
+///
+/// TODO: Allow user to check extensions available.
 #[repr(C)]
 #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "trace", derive(Serialize))]
 #[cfg_attr(feature = "replay", derive(Deserialize))]
 pub struct Extensions {


Should Extensions/Limits be marked as #[non_exhaustive]?

I would imagine so. I should add that note to #691. I anticipate discussion on that issue changing this PR before it's merged.

I realize now that another benefit to this is that adding extensions that don't modify structures become a non breaking change.

Actually, we may need to add extensions that do change structures in a non-breaking way, at some point, probably soon. I.e. we don't want to break wgpu-core API after WebGPU is out and we are just adding more extensions :/

We could do this by adding a private PhantomData field to structures, which implement Default.

If we're okay with the MSRV bump, we can just use #[non_exhaustive] on basically every struct.

kvark

Thank you!
It looks great overall, just a few small things to fix before we go.

wgpu-core/src/device/mod.rs

wgpu-core/src/instance.rs

cwfitzgerald · 2020-06-01T14:06:34Z

Changes should be addressed

kvark

Thank you! A few more notes...

wgpu-types/src/lib.rs

kvark · 2020-06-01T14:12:29Z

wgpu-types/src/lib.rs

@@ -88,6 +88,10 @@ impl From<Backend> for BackendBit {
 #[cfg_attr(feature = "trace", derive(Serialize))]
 #[cfg_attr(feature = "replay", derive(Deserialize))]
 pub struct Extensions {
+    /// This is a native only extension. Support is planned to be added to webgpu,


I don't think this is even required to be specified, actually. If you are running on the Web, the adapter will just not expose this extension. There is no extra benefit of knowing that it's native-only.

wgpu-core/src/instance.rs

cwfitzgerald · 2020-06-01T14:41:05Z

Addressed as discussed

kvark

Thank you!
bors r+

690: Implement Anisotropic Filtering Extension and Expose Extension Interface r=kvark a=cwfitzgerald **Connections** Resolves #568. **Description** This PR solves two tightly related problems: - Adds the ability to create a sampler with anisotropic filtering - Adds the webgpu interface for getting the limits/extensions on adapters and devies. **Testing** I have tested this against my codebase which uses it. Adding an example in wgpu-rs on how to use extensions/limits would be good, especially as we have native/webgpu extensions, but can come as a future enhancement, once we feel the extension situation has stabilized a bit. **TODO** - [x] Allow wgpu-rs to call limit/extension interface (gfx-rs/wgpu-rs#338) - [ ] Determine how wgpu-native is going to adopt this functionality. - [ ] After discussing #691, what changes need to be made. Co-authored-by: Connor Fitzgerald <[email protected]>

kvark · 2020-06-01T16:04:44Z

bors retry

bors · 2020-06-01T16:09:46Z

Build succeeded:

338: Add Extension/Limit Interface r=kvark a=cwfitzgerald Follow up to gfx-rs/wgpu#690. This forwards the extension/limit access interface into wgpu-rs. It appears that the web-sys doesn't actually implement any of the methods we need, so I had them return the defaults. `Device::limits` exists, but it returns `Object` not `GpuLimits`, so doesn't appear usable. Co-authored-by: Connor Fitzgerald <[email protected]>

696: Rustification of Extensions and SamplerDescriptor r=kvark a=cwfitzgerald **Connections** This begins the rustificaiton of `wgpu-types` as discussed in #689, starting with `Extensions` and `SamplerDescriptor`. #691 and the resulting discussion was used to advise the shape of the extension struct. **Description** The PR should be fairly straight forward. As discussed, I have left extensions as a bitflag until the webgpu-native issue can be opened and discussed regarding allocation of extensions. The most controversial part of this will be the `AnisotropyLevel` enum. I created it for two reasons, one that matters, one that doesn't: - It means that, with the exception of enabling the aniso extension (and lod_bias), the sampler is correct by construction, which is helpful to both beginners and experts. It also better exposes the range of valid values and means less panics in user code. - It saves a byte in the `Option<u8>` 😄 I definitely think that, if at all possible, we should have explicitly typed enums for "numbers" which have a limited amount of valid values (so _not_ lod bias, though that may be better expressed, idk) to make it very explicit which values can be accepted. This also feels more "rusty" and reduces the amount of "magicness" in the interface. Ofc, I'm not married to the idea. **Testing** Follow up PR into wgpu-rs will include conversion of the examples. **TODO** - [x] wgpu-native pr rolling up this PR and #690 (gfx-rs/wgpu-native#34) - [x] wgpu-rs pr updating examples with the new structures (gfx-rs/wgpu-rs#345) Co-authored-by: Connor Fitzgerald <[email protected]>

338: Add Extension/Limit Interface r=kvark a=cwfitzgerald Follow up to gfx-rs#690. This forwards the extension/limit access interface into wgpu-rs. It appears that the web-sys doesn't actually implement any of the methods we need, so I had them return the defaults. `Device::limits` exists, but it returns `Object` not `GpuLimits`, so doesn't appear usable. Co-authored-by: Connor Fitzgerald <[email protected]>

690: Add `RenderEncoder` r=kvark a=0x182d4454fb211940 It seems like `RenderPass` and `RenderBundleEncoder` share many methods in common (`set_pipeline`, `draw_indexed`, etc). By creating a shared trait, it allows more reusable code. For instance: ```rust impl RedCircle { fn new(...) -> Self { .. } fn draw<'a>(&'a self, encoder: &mut impl RenderEncoder<'a>) { .. } } ``` This code would then work for both structs. Moving the previous methods from their `impl`s to the trait's would break previous code, as to use these methods code would now have to import `RenderEncoder`. To avoid this, it is implemented by calling the direct methods. It may be worth considering moving this to `wgpu::util`. Co-authored-by: 0x182d4454fb211940 <[email protected]>