Sitelet https://github.com/gpuweb/gpuweb/pull/1256
Skip to content

Rename binding types - #1256

Merged
kvark merged 2 commits into
gpuweb:mainfrom
kvark:binding
Nov 30, 2020
Merged

kvark merged 2 commits into
gpuweb:mainfrom
kvark:binding

Conversation

@kvark

@kvark kvark commented Nov 25, 2020 •

Copy link
Copy Markdown
Contributor

Follow-up to #1223

I think GPUBufferType is confusing since it sounds like something that users would specify at buffer creation, and it doesn't have things like "it's a vertex buffer". This PR renames it to GPUBufferBindingType and does the same for samplers and textures for consistency.
The storage texture bindings don't really have a type, they have access flags instead. So the field is renamed to access.


Preview | Diff

@kvark
kvark requested review from kainino0x and toji November 25, 2020 04:38
@Kangz

Kangz commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

I'm not sure about renaming GPUTextureSampleType that describes clearly what's in it with GPUTextureBindingType though. It's ok if there isn't perfect symmetry between the different binding types (storage textures are different already).

@kvark

kvark commented Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

If we keep the GPUTextureSampleType name for the type, we should probably rename the field from type to sample_type. Would you prefer that?

@Kangz

Kangz commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

sampleType, yeah that would work.

@toji toji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was mildly fond of having a 'type' field across each binding layout variant, but this is fine if you want more descriptive names.

@kainino0x kainino0x left a comment

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.

lgtm. Only concern is that there's no longer an obvious "primary" field in each of these binding types ('type' -> 'access'|'sampleType'|'type'). But really these fields aren't really any more important than the other fields in the dict, so it should be fine.

@kvark

kvark commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

Ok, I generally agree. We can chat about it more on the editors call.
I'd love to have us settle on naming, so that we stop breaking all the user code.

@kainino0x

Copy link
Copy Markdown
Contributor

Fortunately we don't break any users code until we actually implement it, so as long as we get it settled soon it doesn't matter how many times we change it :)

@kvark

kvark commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

There are users following wgpu-rs master, and I have a PR blocked for landing on this binding type change. I don't want them to port their code on this API, only if we are about to change it right afterwards.

@kainino0x

Copy link
Copy Markdown
Contributor

@kvark
kvark merged commit 4d2d3dc into gpuweb:main Nov 30, 2020
@kvark
kvark deleted the binding branch November 30, 2020 22:21
bors Bot added a commit to gfx-rs/wgpu that referenced this pull request Nov 30, 2020
1047: Update bind group layout API to match upstream r=cwfitzgerald a=kvark

**Connections**
Follows gpuweb/gpuweb#1076, gpuweb/gpuweb#1223 (gpuweb/gpuweb#1164), gpuweb/gpuweb#1255, and gpuweb/gpuweb#1256

**Description**
Aligns our API closer to the latest changes in WebGPU upstream. We technically don't have to do this, but I believe in the end it would be best if our API gets close to upstream.

Note: this is a sensitive change for the users, everybody will get their code broken. So please take a look at the API and see if something is missing or needs improvement, so that we don't have to go through the changes again afterwards.

**Testing**
Doesn't really need testing. Partially covered by the existing playtest.

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
This CL adds unimplemented stubs for the `fwidthCoarse` tests.

Issue: gpuweb#1256
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.

4 participants