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

Add blend parameter #2061

Merged
merged 14 commits into from
May 23, 2024
Merged

Add blend parameter #2061

merged 14 commits into from
May 23, 2024

Conversation

Pessimistress
Copy link
Collaborator

@Pessimistress Pessimistress commented Mar 31, 2024

Allow WebGL render pass to explicitly disable blending

Since WebGPU render pass has blending disabled by default this value will be safely ignored?

Change List

  • Add blend: false which disables blending in WebGL

Comment on lines 284 to 290
if (parameters.blendColorOperation === 'none' || parameters.blendAlphaOperation === 'none') {
gl.disable(GL.BLEND);
} else {
gl.enable(GL.BLEND);
Copy link
Collaborator

@donmccurdy donmccurdy Apr 1, 2024

Choose a reason for hiding this comment

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

Because neither WebGL nor WebGPU enables blending for color and alpha separately from the other, the answers to what is the default value of blendAlphaOperation or what happens with blendColorOperation='add' + blendAlphaOperation='none' are getting a bit complicated. I wonder if we should:

  • (A) disallow 'none' for blendAlphaOperation, only use blendColorOperation to enable/disable blending
  • (B) add a separate boolean parameter to enable/disable blending
if (parameters.blend === true) {
  const colorEquation = convertBlendOperationToEquation(
    'blendColorOperation',
    parameters.blendColorOperation || 'add'
  );
  // ...
} else {
  // ...
}

I lean toward (b), but not a strong preference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I initially proposed adding a none option when I was notified of the problem, after reviewing the PR and thinking more about it, I am also thinking that a separate blend enable type parameter seems cleanest.

@Pessimistress Pessimistress changed the title Add BlendOperation: 'none' Add blend parameter May 6, 2024
@Pessimistress Pessimistress marked this pull request as ready for review May 6, 2024 03:56
@@ -10,6 +10,7 @@
},
"references": [
{"path": "../core"},
{"path": "../engine"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anyway we can avoid this?

modules/webgl/src/adapter/converters/device-parameters.ts Outdated Show resolved Hide resolved

if (parameters.blendColorOperation || parameters.blendAlphaOperation) {
gl.enable(GL.BLEND);
if (parameters.blend === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This auto-enabling seems subtle.

  • Is there any reason not to require blend: true to be set and remove the auto-enable?
  • If some good backwards compatibility reason then maybe add a comment explaining that so that when we invariably refactor this next time, we don't break things in deck without knowing?

@ibgreen
Copy link
Collaborator

ibgreen commented May 23, 2024

@Pessimistress merging, but removed the auto-enabling. we can open a separate PR with those changes if you feel they are important enough.

@ibgreen ibgreen merged commit a999dc6 into master May 23, 2024
2 checks passed
@ibgreen ibgreen deleted the x/disable-blend branch May 23, 2024 17:06
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