Skip to content

Make blending state explicit - #1134

Merged
kainino0x merged 1 commit into
mainfrom
kvark-blend
Nov 23, 2020
Merged

Make blending state explicit#1134
kainino0x merged 1 commit into
mainfrom
kvark-blend

Conversation

@kvark

@kvark kvark commented Oct 5, 2020

Copy link
Copy Markdown
Contributor

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

kainino0x commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

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

kvark commented Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

I have no real feelings either way.

@kvark

kvark commented Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

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 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.

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

@kdashg kdashg 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.

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

@kainino0x

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

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

@kvark

kvark commented Nov 20, 2020

Copy link
Copy Markdown
Contributor Author

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>
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

None yet

Development

Successfully merging this pull request may close these issues.

4 participants