Skip to content

Grand cleanup and refactor of the descriptors in the API#873

Merged
bors[bot] merged 4 commits intogfx-rs:masterfrom
kvark:grand-cleanup
Aug 13, 2020
Merged

Grand cleanup and refactor of the descriptors in the API#873
bors[bot] merged 4 commits intogfx-rs:masterfrom
kvark:grand-cleanup

Conversation

@kvark
Copy link
Copy Markdown
Member

@kvark kvark commented Aug 12, 2020

Connections
Cleanup follows gfx-rs/wgpu-rs#501
Fixes #871
Fixes #848

Description
There is a lot of small and big things crumbled in here.

The major one is where descriptors live, and how they are parametrized. Logic is the following:

  • if something is useful by wgpu-rs's API leave it in wgpu-types
  • if in addition it's useful to wgpu-native(i.e. has repr(C)), we possibly parametrize it (currently, only label is).
  • otherwise, the type is moved to wgpu-core and stripped of generics
  • remove all the builders

Some medium-sized things:

Some smaller things:

  • using NonZeroU8 for anisotropy
  • using NonZeroU32 for descriptor count
  • putting sampler addressing modes into an array
  • add labels to command buffers and pipeline layouts
  • improves errors for exceeding binding limits

Testing
Tested on gfx-rs/wgpu-rs#503

@kvark kvark requested review from cwfitzgerald and grovesNL August 12, 2020 18:58
Copy link
Copy Markdown
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
Copy Markdown
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.

context: &'a super::RenderPassContext,
) -> crate::command::RenderBundleEncoderDescriptor<'a> {
crate::command::RenderBundleEncoderDescriptor {
label: label.map(Cow::Borrowed),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be more consistent to import Borrowed directly everywhere.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think wgpu crate imports Borrowed directly anywhere

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah sorry, could have sworn I did the same as in wgpu-rs there.

Copy link
Copy Markdown
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 great!

One minor concern I have is that we currently have the documentation duplicated between wgpu-core and wgpu-rs for non-shared descriptors, which means we need to make sure they are always kept in sync. This could pose some issues if we let wgpu-core's stagnate due to us not actively seeing it.

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

@grovesNL grovesNL 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 👍

@kvark
Copy link
Copy Markdown
Member Author

kvark commented Aug 13, 2020

bors r=cwfitzgerald,grovesNL

@bors
Copy link
Copy Markdown
Contributor

bors bot commented Aug 13, 2020

@bors bors bot merged commit 6e3e88d into gfx-rs:master Aug 13, 2020
@kvark kvark deleted the grand-cleanup branch August 13, 2020 03:33
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Aug 13, 2020
503: Update for the wgpu grand refactor r=kvark a=kvark

Depends on gfx-rs/wgpu#873

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Aug 13, 2020
503: Update for the wgpu grand refactor r=kvark a=kvark

Depends on gfx-rs/wgpu#873

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Aug 13, 2020
503: Update for the wgpu grand refactor r=kvark a=kvark

Depends on gfx-rs/wgpu#873

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors bot added a commit that referenced this pull request Aug 31, 2020
912: Fix write-only stencil state descriptors from not working - fixes #911 r=kvark a=Dinnerbone

**Connections**
This fixes [#911 - Stencil testing broken between v0.5 and v0.6](#911).

**Description**
Write-only stencil states (read 0, write >0) are being treated as if they are disabled, which causes pipelines to act as though they don't have any stencil state set at all. This worked prior to commit 2473c25 (introduced in PR #873). As far as I can tell, this works fine in Vulkan, Metal, DX12 and DX11 as we've been using this approach over at Ruffle for a while now.

**Testing**
You can view the reproduction case in #911 for manual testing. I have confirmed that this fix makes that case work as expected.

I couldn't find any automated tests for wgpu-types to copy and add for this case. If that's wanted then please let me know what the best approach is.

Co-authored-by: Nathan Adams <dinnerbone@dinnerbone.com>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
503: Update for the wgpu grand refactor r=kvark a=kvark

Depends on gfx-rs#873

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
kvark pushed a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
873: Boids example: cleaner rand code r=kvark a=dhardy

... for a certain definition of cleaner. Is this clearer or just more confusing?

Co-authored-by: Diggory Hardy <git@dhardy.name>
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.

Move subscriber logic out into a separate crate Refactor TextureViewDescriptor to match upstream spec better

4 participants