Skip to content

GPUBGLBinding: rename multisample to multisampled.#367

Merged
Kangz merged 2 commits intogpuweb:masterfrom
Kangz:multisampled
Jul 15, 2019
Merged

GPUBGLBinding: rename multisample to multisampled.#367
Kangz merged 2 commits intogpuweb:masterfrom
Kangz:multisampled

Conversation

@Kangz
Copy link
Contributor

@Kangz Kangz commented Jul 12, 2019

This is because reading binding.multisampled = false seems like better
grammar than binding.multisample.


Preview | Diff

@damyanp
Copy link

damyanp commented Jul 12, 2019

Possible out of scope for this review, but I'm a bit confused about when an app should specify "true" for this field. Is this a promise about what the properties of the sampler that the shader might use to read from the texture?

(Otherwise the actual change itself looks good to me.)

@kvark
Copy link
Contributor

kvark commented Jul 12, 2019

@damyanp this is a promise that the shader sees this texture as an MSAA texture, and we are only able to create bind groups that have a multisampled texture filling this binding.

@damyanp
Copy link

damyanp commented Jul 12, 2019

@kvark - ah, I think I'm catching up. Presumably this would be validated (in all implementations) so that if you try and bind a multisampled texture to a binding where the layout doesn't have this set then it would fail?

@kvark
Copy link
Contributor

kvark commented Jul 12, 2019

Yes, it should be validated along all the other fields of the bind group layout (by all implementations).

This is because reading binding.multisampled = false seems like better
grammar than binding.multisample.
@kainino0x
Copy link
Contributor

Rebased over #365

@Kangz
Copy link
Contributor Author

Kangz commented Jul 15, 2019

Merging this as it is quite trivial.

@Kangz Kangz merged commit 8252e82 into gpuweb:master Jul 15, 2019
@Kangz Kangz deleted the multisampled branch July 15, 2019 12:35
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.

5 participants