Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make blending state explicit #1134

Merged
merged 1 commit into from Nov 23, 2020
Merged

Make blending state explicit #1134

merged 1 commit into from Nov 23, 2020

Conversation

kvark
Copy link
Contributor

@kvark kvark commented Oct 5, 2020

Writing 251649e I realized how awkward our "default" values for blending are:

|descriptor|.{{GPUColorStateDescriptor/alphaBlend}} and |descriptor|.{{GPUColorStateDescriptor/colorBlend}}
are either both defaults, or |descriptor|.{{GPUColorStateDescriptor/format}} has
{{GPUTextureComponentType/"float"}} component type.

I believe the spec would be better if instead of comparing to some "default" this was an explicit "provided" versus "undefined".


Preview | Diff

@kainino0x
Copy link
Contributor

kainino0x commented Oct 6, 2020

We chose this because the default values semantically represent what happens when blending is disabled. This would create a difference between unspecified/undefined and {} and { srcFactor: "one", dstFactor: "zero", operation: "add" }.

How about instead having a dfn dfn which says something like

<dfn dfn>blending is enabled</dfn> in a {{GPUBlendDescriptor}} if it is not equal to
`{ srcFactor: "one", dstFactor: "zero", operation: "add" }`

and using that?

@kvark
Copy link
Contributor Author

kvark commented Oct 6, 2020

We chose this because the default values semantically represent what happens when blending is disabled.

Right. Except that if your render target is non-blendable, then enabling blending is technically a UB in native APIs.

@kainino0x
Copy link
Contributor

You want to reintroduce the distinction, then? (between blend-disabled and blend-enabled-but-noop)

I have no real feelings either way.

@kvark
Copy link
Contributor Author

kvark commented Oct 6, 2020

You want to reintroduce the distinction, then?

Yes. That's how it's done in all the native APIs. E.g. in Metal blending is an explicit flag.
I don't think it poses any harm or confusion.

@kainino0x kainino0x added this to Needs Discussion in Main Oct 6, 2020
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

Sounds good to me. Let's take it to the meeting then.

Copy link
Contributor

@kdashg kdashg left a comment

Choose a reason for hiding this comment

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

Needs to spec what happens if only one but not both are undefined. (is blending enabled then?)

@kainino0x
Copy link
Contributor

We could put one extra layer of dictionary in between so there's only one nullable thing.

{ blend?: { alpha: GPUBlendDescriptor, color: GPUBlendDescriptor }, ... }

@kainino0x
Copy link
Contributor

Editors meeting: Agreed to add that extra layer of dictionary.

@kainino0x kainino0x moved this from Needs Discussion to Needs Investigation/Proposal or Revision in Main Nov 20, 2020
@kvark
Copy link
Contributor Author

kvark commented Nov 20, 2020

This is now updated with a separate struct. Please take another look!

@kainino0x kainino0x merged commit 37b409f into main Nov 23, 2020
@kainino0x kainino0x deleted the kvark-blend branch November 23, 2020 19:25
bors bot added a commit to gfx-rs/wgpu that referenced this pull request Feb 19, 2021
1224: Update the blend API to upstream r=kvark a=kvark

**Connections**
Matches gpuweb/gpuweb#1134

**Description**
Makes blending state ON/OFF explicit.

**Testing**
Simple enough!

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@kainino0x kainino0x moved this from Needs Investigation/Proposal or Revision to Specification Done in Main Jan 19, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
…web#1134)

The ASTC texture compression format has a lot of different block sizes
so using 16x16 for the test might not work. Instead use the LCM of the
block size.

(and add GCD and LCD util functions)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Main
Specification Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants