Implementation of the builder pattern by Andful · Pull Request #856 · gfx-rs/wgpu

Andful · 2020-07-31T11:39:47Z

Connections
This pull request addresses the issue #851

Description
Add a builder for wgpu types.

Testing
TODO

monocodus

This is an autogenerated code review.New suggestions: 2

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

monocodus · 2020-07-31T11:45:57Z

wgpu-types/src/lib.rs

+    pub fn new() -> Self {
+        Self {
+            index_format: IndexFormat::default(),
+            vertex_buffers: Cow::Borrowed(&[]),
+        }
+    }


error: you should consider adding a Default implementation for VertexStateDescriptor<'a>

| 1201 | / pub fn new() -> Self { 1202 | | Self { 1203 | | index_format: IndexFormat::default(), 1204 | | vertex_buffers: Cow::Borrowed(&[]), 1205 | | } 1206 | | } | |_____^ | note: `-D clippy::new-without-default` implied by `-D warnings` help: [refer to the official clippy documentation](https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default) help: try this | 1200 | impl Default for VertexStateDescriptor<'a> { 1201 | fn default() -> Self { 1202 | Self::new() 1203 | } 1204 | } |

generated by rust-clippy

monocodus · 2020-07-31T11:45:57Z

wgpu-types/src/lib.rs

+    pub fn new() -> Self {
+        Self {
+            usage: TextureUsage::OUTPUT_ATTACHMENT,
+            format: TextureFormat::Bgra8UnormSrgb,
+            width: 0,
+            height: 0,
+            present_mode: PresentMode::Fifo,
+        }
+    }


error: you should consider adding a Default implementation for SwapChainDescriptor

| 1514 | / pub fn new() -> Self { 1515 | | Self { 1516 | | usage: TextureUsage::OUTPUT_ATTACHMENT, 1517 | | format: TextureFormat::Bgra8UnormSrgb, ... | 1521 | | } 1522 | | } | |_____^ | help: [refer to the official clippy documentation](https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default) help: try this | 1513 | impl Default for SwapChainDescriptor { 1514 | fn default() -> Self { 1515 | Self::new() 1516 | } 1517 | } |

generated by rust-clippy

wgpu-types/src/lib.rs

monocodus

This is an autogenerated code review.

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

kvark

Thank you for this grand work! I understand it was a bit tedious to write this up, but this looks great to me as way forward. I don't think we want to auto-derive this, since we need more flexibility on how we construct the types, and we want compile times to be fast. In particular, there are places where we know fields are to be set together, and we can take advantage of that.

kvark · 2020-07-31T13:05:12Z

wgpu-types/src/lib.rs

+impl DeviceDescriptor {
+    pub fn new() -> Self {
+        Self {
+            features: Features::default(),


for things that derive Default we should just do Self::default() here

kvark · 2020-07-31T13:21:42Z

wgpu-types/src/lib.rs

+        self
+    }
+
+    pub fn shader_validation(&mut self, validation: bool) -> &mut Self {


let's have no arguments and just assign this to true, to be in line with compatible_surface

Should the function name be enable_shader_validation in your opinion?

uh, that's a hard one... this choice affects all the boolean entry points.
My feeling is that just shader_validation() without prefixes or postfixes is fine. @cwfitzgerald @grovesNL please share your thoughts if you feel strongly

I don't have particular feelings on the positive side, either thing() or enable_thing() work fine. Going to negative (which we don't right now) it should be disable_thing().

kvark · 2020-07-31T13:25:23Z

wgpu-types/src/lib.rs

+        Self::default()
+    }
+
+    pub fn src_factor(&mut self, factor: BlendFactor) -> &mut Self {


let's take advantage of the manual builder pattern here and combine the src and dst factors: pub fn factors(&mut self, src: BlendFactor, dst: BlendFactor) -> &mut Self. I don't think it's useful to only set one of them ever.

I agree that it's probably not useful to only set one of them, but this breaks consistency and expectations. When looking for these functions you generally expect the function name to match the field 1:1. I'd argue against using factors(..) simply to keep everything consistent.

wgpu-types/src/lib.rs

kvark · 2020-07-31T13:26:51Z

wgpu-types/src/lib.rs

+impl<S> RequestAdapterOptions<S> {
+    pub fn new() -> Self {
+        Self {
+            power_preference: PowerPreference::default(),


let's call Self::Default

S: Default is required. I will implement default for RequestAdapterOptions

kvark · 2020-07-31T13:44:57Z

wgpu-types/src/lib.rs

+        self
+    }
+
+    pub fn alpha_to_coverage_enabled(&mut self, enabled: bool) -> &mut Self {


let's remove the argument, and strip the "_enabled" suffix from the method

kvark · 2020-07-31T13:45:29Z

wgpu-types/src/lib.rs

 }

+impl CommandBufferDescriptor {
+    pub fn new(todo: u32) -> Self {


doesn't need the argument here, should just call Self::default() in the impl

kvark · 2020-07-31T13:47:56Z

wgpu-types/src/lib.rs


+impl<'a> RenderBundleDescriptor<Option<Cow<'a, str>>> {
+    pub fn new() -> Self {
+        Self { label: None }


similarly, let's call Self::default() for consistency

kvark · 2020-07-31T13:48:30Z

wgpu-types/src/lib.rs

 }

+impl TextureDataLayout {
+    pub fn new(offset: BufferAddress, bytes_per_row: u32, rows_per_image: u32) -> Self {


let's remove the offset and rows_per_image from here (since they are optional) and have separate methods for them

kvark · 2020-07-31T13:49:12Z

wgpu-types/src/lib.rs

 }
+
+impl<T> TextureCopyView<T> {
+    pub fn new(texture: T, origin: Origin3d) -> Self {


let's remove the origin from here (defaulting to ZERO), and have a method for it

Andful · 2020-08-01T11:23:03Z

wgpu-types/src/lib.rs

+            depth_write_enabled: false,
+            depth_compare: CompareFunction::Undefined,


Should depth_compare be initialized as CompareFunction::Undefined? My worry is that this might fail silently.

Sorry, I should have said. We need to accept it as a new() parameter instead of having a separate method.

wgpu-types/src/lib.rs

kvark

This looks great, thank you!
Just one last fix to make.

kvark · 2020-08-02T00:21:59Z

wgpu-types/src/lib.rs

+            depth_write_enabled: false,
+            depth_compare: CompareFunction::Undefined,


Sorry, I should have said. We need to accept it as a new() parameter instead of having a separate method.

wgpu-types/src/lib.rs

Andful · 2020-08-02T01:01:43Z

wgpu-types/src/lib.rs

+    pub fn new(width: u32, height: u32) -> Self {
+        Self {
+            usage: TextureUsage::OUTPUT_ATTACHMENT,
+            format: TextureFormat::Bgra8UnormSrgb,


This default value is ok for texture format for the swap chain? I noticed that for web it usually take value TextureFormat::Bgra8Unorm.

At least here it uses TextureFormat::Bgra8Unorm but it also states // TODO: Allow srgb unconditionally

I think it's ok defaulting to Bgra8UnormSrgb here. WASM users can always work around by using non-default, and we want to fix this in WASM too

Andful · 2020-08-02T01:07:55Z

Made the (hopefully) last change. Could you check my last comment?

wgpu-types/src/lib.rs

Andful · 2020-08-02T03:29:35Z

wgpu-types/src/lib.rs

+impl<'a> TextureViewDescriptor<Option<Cow<'a, str>>> {
+    pub fn new(format: TextureFormat, dimension: TextureViewDimension) -> Self {
+        Self {
+            label: None,
+            format,
+            dimension,
+            aspect: TextureAspect::default(),
+            base_mip_level: 0,
+            level_count: None,
+            base_array_layer: 0,
+            array_layer_count: None,
+        }
+    }
+
+    pub fn label(&mut self, label: impl IntoCow<'a, str>) -> &mut Self {
+        self.label = Some(label.into_cow());
+        self
+    }
+}
+
 impl<L> TextureViewDescriptor<L> {
+    pub fn mip_levels(
+        &mut self,
+        base_mip_level: u32,
+        level_count: Option<u32>,
+    ) -> &mut Self {
+        self.base_mip_level = base_mip_level;
+        self.level_count = level_count;
+        self
+    }
+
+    pub fn array_layers(
+        &mut self,
+        base_array_layer: u32,
+        array_layer_count: Option<u32>,
+    ) -> &mut Self {
+        self.base_array_layer = base_array_layer;
+        self.array_layer_count = array_layer_count;
+        self
+    }
+


@kvark Could you double check this? I accidentally missed its implementation previously. Also, should level_count and array_layer_count checked to be different from 0?

sorry, I should have spotted that. Please separate these both into methods that change base vs count.
thank you for the heads up!

kvark · 2020-08-02T03:32:46Z

wgpu-types/src/lib.rs

+impl<'a> TextureViewDescriptor<Option<Cow<'a, str>>> {
+    pub fn new(format: TextureFormat, dimension: TextureViewDimension) -> Self {
+        Self {
+            label: None,
+            format,
+            dimension,
+            aspect: TextureAspect::default(),
+            base_mip_level: 0,
+            level_count: None,
+            base_array_layer: 0,
+            array_layer_count: None,
+        }
+    }
+
+    pub fn label(&mut self, label: impl IntoCow<'a, str>) -> &mut Self {
+        self.label = Some(label.into_cow());
+        self
+    }
+}
+
 impl<L> TextureViewDescriptor<L> {
+    pub fn mip_levels(
+        &mut self,
+        base_mip_level: u32,
+        level_count: Option<u32>,
+    ) -> &mut Self {
+        self.base_mip_level = base_mip_level;
+        self.level_count = level_count;
+        self
+    }
+
+    pub fn array_layers(
+        &mut self,
+        base_array_layer: u32,
+        array_layer_count: Option<u32>,
+    ) -> &mut Self {
+        self.base_array_layer = base_array_layer;
+        self.array_layer_count = array_layer_count;
+        self
+    }
+


sorry, I should have spotted that. Please separate these both into methods that change base vs count.
thank you for the heads up!

kvark · 2020-08-02T03:33:57Z

wgpu-types/src/lib.rs

+    pub fn new(width: u32, height: u32) -> Self {
+        Self {
+            usage: TextureUsage::OUTPUT_ATTACHMENT,
+            format: TextureFormat::Bgra8UnormSrgb,


I think it's ok defaulting to Bgra8UnormSrgb here. WASM users can always work around by using non-default, and we want to fix this in WASM too

Signed-off-by: Andrea Nardi <buongiorno19972@gmail.com>

kvark

Great stuff, thank you for doing this!
I think we may discover some parts needing changes, but that's ok. Most of the content here is good, and we need it ASAP.
bors r+

kvark · 2020-08-02T03:52:04Z

@Andful thank you! Would you be interested to update wgpu-rs based on this PR?

Andful · 2020-08-02T03:58:12Z

Yes, I would be. It is only the example code, is it not?

bors · 2020-08-02T04:01:40Z

Build succeeded:

kvark · 2020-08-02T04:03:12Z

Yes, I would be. It is only the example code, is it not?

likely, yes

monocodus bot suggested changes Jul 31, 2020

View reviewed changes

aloucks reviewed Jul 31, 2020

View reviewed changes

wgpu-types/src/lib.rs Show resolved Hide resolved

Andful changed the title ~~first draft implementation~~ Implementation of the builder pattern Jul 31, 2020

monocodus bot suggested changes Jul 31, 2020

View reviewed changes

kvark requested changes Jul 31, 2020

View reviewed changes

Andful force-pushed the add-builder-pattern branch 3 times, most recently from cae8a65 to d8c6822 Compare August 1, 2020 11:20

Andful commented Aug 1, 2020

View reviewed changes

kvark requested changes Aug 2, 2020

View reviewed changes

Andful force-pushed the add-builder-pattern branch from d8c6822 to 8a2fb4b Compare August 2, 2020 00:53

Andful commented Aug 2, 2020

View reviewed changes

Andful force-pushed the add-builder-pattern branch 2 times, most recently from 3d1048e to 24cef68 Compare August 2, 2020 02:54

Andful commented Aug 2, 2020

View reviewed changes

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved

Andful force-pushed the add-builder-pattern branch 3 times, most recently from 68c4ca0 to 42f3b93 Compare August 2, 2020 03:22

Andful commented Aug 2, 2020

View reviewed changes

kvark requested changes Aug 2, 2020

View reviewed changes

Implementation of the builder pattern

acfbbb4

Signed-off-by: Andrea Nardi <buongiorno19972@gmail.com>

Andful force-pushed the add-builder-pattern branch from 42f3b93 to acfbbb4 Compare August 2, 2020 03:49

kvark approved these changes Aug 2, 2020

View reviewed changes

bors bot merged commit 5d35936 into gfx-rs:master Aug 2, 2020

kvark mentioned this pull request Aug 5, 2020

Builder patterns for descriptors #851

Closed

Andful mentioned this pull request Aug 5, 2020

fixed lifetime inconsistency when assigning string literal as label #863

Closed

		depth_write_enabled: false,
		depth_compare: CompareFunction::Undefined,

Conversation

Andful commented Jul 31, 2020

Uh oh!

monocodus bot left a comment

Choose a reason for hiding this comment

Uh oh!

monocodus bot Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

monocodus bot Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

monocodus bot left a comment

Choose a reason for hiding this comment

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andful Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andful commented Aug 2, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark left a comment

Andful Aug 2, 2020 •

edited

Loading