Rustification of Extensions and SamplerDescriptor #696

cwfitzgerald · 2020-06-02T22:27:49Z

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

wgpu-native pr rolling up this PR and Implement Anisotropic Filtering Extension and Expose Extension Interface #690 (Add Device/Limit Accessors and Anisotropy Extension wgpu-native#34)
wgpu-rs pr updating examples with the new structures (Rustify the example's use of Extensions and SamplerDescriptor wgpu-rs#345)

kvark

Thank you! Expressed some thoughts below

kvark · 2020-06-02T23:11:04Z

wgpu-types/src/lib.rs

+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "trace", derive(Serialize))]
+#[cfg_attr(feature = "replay", derive(Deserialize))]
+pub enum AnisotropyLevel {


The advantages here are obvious (i.e. being correct by construction), the disadvantages, however, less so.
We had a lot of iteration on similar aspects of the API in gfx-rs during the years... and quite often the original desire to make the types pretty back-fired in the end. The morale of gfx-rs experience is that we need to strive for simplicity.

In this case, for example, writing this down as anisotropy_clamp: Some(wgpu:AnisotropyLevel::X2) isn't any cleaner than anisotropy_clamp: Some(2). And the "correct by construction" aspect is not helping much in practice(!) since the users gets the error instantly upon creating a sampler.

In another case, say you are writing an engine and you don't have that value predefined. You are loading from disk/configuration. Now you have 2 choices:

make your configuration fully use AnisotropyLevel enum here, which makes it intrusive. Not always applicable, i.e. if you don't control the format

do a manual match aniso_level { 0 | 1 => None, 2 => Some(wgpu::AnisotropyLevel::X2), .. }, which is very annoying.

So in both of these cases the strong enum, despite it being elegant and all that, doesn't bring much to the table in practice.

Also we need to think about what the other parameters are going to be. For example, do you see MSAA level also enumerized in the same way? Unlike unisotropy, MSAA levels have different support on platforms. That means, we don't know at compile time if the value is good anyway. We might as well accept it as an integer without fanciness.

That's fair enough. The point about the intrusiveness is an interesting angle that I didn't think about as I'm mainly used to working on end products (where I can see the whole picture). I'll switch it back.

kvark

Thank you! just one last thing

kvark · 2020-06-03T00:49:23Z

wgpu-types/src/lib.rs

@@ -955,10 +956,10 @@ pub struct SamplerDescriptor<L> {
    pub lod_max_clamp: f32,
    pub compare: CompareFunction,


can we also switch this to be an Option since we are at it?

kvark

Thank you!
bors r+

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 <connorwadefitzgerald@gmail.com>

cwfitzgerald · 2020-06-03T01:28:37Z

bors retry

bors · 2020-06-03T01:28:38Z

Already running a review

bors · 2020-06-03T01:33:28Z

Build failed:

MacOS Stable

cwfitzgerald · 2020-06-03T01:34:02Z

bors retry

kvark · 2020-06-03T01:34:04Z

bors retry

bors · 2020-06-03T01:34:08Z

Already running a review

bors · 2020-06-03T01:38:42Z

Build succeeded:

345: Rustify the example's use of Extensions and SamplerDescriptor r=kvark a=cwfitzgerald Implements gfx-rs/wgpu#696. This didn't change any content from #338, just interface, but I tested a couple examples and it seems to work okay. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>

345: Rustify the example's use of Extensions and SamplerDescriptor r=kvark a=cwfitzgerald Implements gfx-rs#696. This didn't change any content from gfx-rs#338, just interface, but I tested a couple examples and it seems to work okay. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>

monocodus bot approved these changes Jun 2, 2020

View reviewed changes

cwfitzgerald requested a review from kvark June 2, 2020 22:30

monocodus bot approved these changes Jun 2, 2020

View reviewed changes

cwfitzgerald force-pushed the rustification branch from 3d30ac3 to 33afc6b Compare June 2, 2020 23:10

monocodus bot approved these changes Jun 2, 2020

View reviewed changes

kvark reviewed Jun 2, 2020

View reviewed changes

cwfitzgerald force-pushed the rustification branch from 33afc6b to fae6b89 Compare June 2, 2020 23:20

monocodus bot approved these changes Jun 2, 2020

View reviewed changes

cwfitzgerald mentioned this pull request Jun 3, 2020

Rustify the example's use of Extensions and SamplerDescriptor gfx-rs/wgpu-rs#345

Merged

monocodus bot approved these changes Jun 3, 2020

View reviewed changes

kvark reviewed Jun 3, 2020

View reviewed changes

Rustification of Extensions and SamplerDescriptor

ade7ce1

cwfitzgerald force-pushed the rustification branch from fae6b89 to ade7ce1 Compare June 3, 2020 01:04

monocodus bot approved these changes Jun 3, 2020

View reviewed changes

kvark approved these changes Jun 3, 2020

View reviewed changes

bors bot merged commit ac9587e into gfx-rs:master Jun 3, 2020

cwfitzgerald deleted the rustification branch June 3, 2020 01:53

kvark mentioned this pull request Jul 6, 2020

General direction on the API of exposed structures #689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustification of Extensions and SamplerDescriptor #696

Rustification of Extensions and SamplerDescriptor #696

cwfitzgerald commented Jun 2, 2020 •

edited

Loading

kvark left a comment

kvark Jun 2, 2020

cwfitzgerald Jun 2, 2020

kvark left a comment

kvark Jun 3, 2020

kvark left a comment

cwfitzgerald commented Jun 3, 2020

bors bot commented Jun 3, 2020

bors bot commented Jun 3, 2020

cwfitzgerald commented Jun 3, 2020

kvark commented Jun 3, 2020

bors bot commented Jun 3, 2020

bors bot commented Jun 3, 2020

		@@ -955,10 +956,10 @@ pub struct SamplerDescriptor<L> {
		pub lod_max_clamp: f32,
		pub compare: CompareFunction,

Rustification of Extensions and SamplerDescriptor #696

Rustification of Extensions and SamplerDescriptor #696

Conversation

cwfitzgerald commented Jun 2, 2020 • edited Loading

kvark left a comment

Choose a reason for hiding this comment

kvark Jun 2, 2020

Choose a reason for hiding this comment

cwfitzgerald Jun 2, 2020

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

kvark Jun 3, 2020

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

cwfitzgerald commented Jun 3, 2020

bors bot commented Jun 3, 2020

bors bot commented Jun 3, 2020

cwfitzgerald commented Jun 3, 2020

kvark commented Jun 3, 2020

bors bot commented Jun 3, 2020

bors bot commented Jun 3, 2020

cwfitzgerald commented Jun 2, 2020 •

edited

Loading