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

WebGLRenderer: Create WebGL context with alpha: true. #23230

Merged
merged 7 commits into from
Jan 14, 2022
Merged

WebGLRenderer: Create WebGL context with alpha: true. #23230

merged 7 commits into from
Jan 14, 2022

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jan 14, 2022

Related issue: #22418

@mrdoob mrdoob added this to the r137 milestone Jan 14, 2022
@mrdoob mrdoob force-pushed the alpha-true branch 2 times, most recently from a9a82a7 to 23c34ae Compare January 14, 2022 02:29
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

So there are two ways of doing it...

  1. Let the browser do the compositing by doing canvas.style.background = '#000 (first commit) which results in a bunch of E2E errors specially with lines with additive blending:
before after
image image
  1. Or do it in WebGLBackground (second commit) which only breaks:
webgl_materials_blending_custom webgl_shadow_contact
Screen Shot 2022-01-13 at 9 40 58 PM Screen Shot 2022-01-13 at 9 52 17 PM

I can look into fixing webgl_shadow_contact but I don't think we can do much about webgl_materials_blending_custom....

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

On the bright side... many of the examples that used to run at 40-50fps on my laptop, now run at 60fps 🥲

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

The PR has the following line:

state.buffers.color.setClear( color.r, color.g, color.b, useAlpha ? alpha : 1.0, premultipliedAlpha );

I'm afraid this could introduce side effects when doing post processing. When alpha is set to false it is not possible to clear the alpha value in render targets anymore which might be necessary for certain use cases. Do you think we can figure out a solution that does affect the number of cleared channels?

How about using something like this: Mugen87@db825d2

The idea is to change the default of alpha to true like in this PR. However, the alpha parameter is not passed to WebGLBackground. Instead, the default clearAlpha in WebGLBackground is changed from 0 to 1.

So if users want to blend the canvas with the HTML background, they need this line:

renderer.setClearAlpha( 0 );

Granted, the approach requires a migration task compared to this PR since users have to add the above line.

but I don't think we can do much about webgl_materials_blending_custom....

Yeah, it seems no solution can support all blending scenarios demonstrated in the webgl_materials_blending* examples. We probably have to update/remove some of the blending options.

BTW: Both blending examples fail in the PR. Not sure why the E2E test did not catch the failure in webgl_materials_blending 🤔 .

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

Some fiddles for testing subtractive blending:

(1) alpha: false: https://jsfiddle.net/pgueybwc/
(2) alpha: true: https://jsfiddle.net/pgueybwc/1/
(3) alpha: true and opacity 0.5: https://jsfiddle.net/pgueybwc/2/
(4) alpha: false and opacity 0.5: https://jsfiddle.net/pgueybwc/3/

I think I understand what happens in (2) and (3) but can somebody explain why test case 1 and 4 look identical?

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

Hmm...

We could also set alpha: true and scene.background = new Color( 0, 0, 0 ) by default.

I think that should solve post processing and, migration wise, people would just have to set scene.background to null if they want transparent background.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

We could also set alpha: true and scene.background = new Color( 0, 0, 0 ) by default.

Sounds good! Working with Scene.background seems more intuitive than setting a different clear alpha value with setClearAlpha().

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

Seems like it also breaks code that used renderer.setClearColor(). Which, I'm okay with 😇

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

Hmm... It also breaks renderer.autoClear = false 😔

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

When alpha is always true, I think the renderer should use a clear alpha value of 1 by default. WebGPURenderer works like that and it turned out to be a good setup so far.

Does this sound okay for you?

What are your plans for setClearAlpha() and setClearColor()? Just want to know if these methods could be part of a potential solution.

The engine has to provide a way to define the clear alpha value. If this should not happen via setClearAlpha(), we have to think about a new API.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

@Mugen87 Yeah, I think I'm going to give Mugen87@db825d2 a try. Seems easier indeed 🤞

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

One more try... 😅

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

One "solution" would be to remove THREE.CustomBlending, material.blendSrc, material.blendDest and material.blendEquation and see how many people complain... 🙈

Does WebGPU have these things too?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

Yes^^. WebGPURenderer already supports the same set of blending configurations like WebGLRenderer.

// blending
let alphaBlend = {};
let colorBlend = {};
if ( material.transparent === true && material.blending !== NoBlending ) {
alphaBlend = this._getAlphaBlend( material );
colorBlend = this._getColorBlend( material );
}

The blending is part of the render pipeline and thus of the respective descriptor (see https://gpuweb.github.io/gpuweb/#blend-state for details).

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

I'm not sure it's necessary to remove any blending setups. I'm sure we can update webgl_materials_blending and webgl_materials_blending_custom such that both examples makes sense with alpha: true.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 14, 2022

In the long term, I would recommend to remove the alpha parameter completely.

Maybe we can introduce at some point a solution over Scene.background (e.g. by assigning a four component color instance). In this way, users could perform all background configurations via Scene.background and don't need setClearAlpha() and setClearColor() anymore. However, this requires users always pass a scene to WebGLRenderer.render() and not just a mesh.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 14, 2022

Alright. I'll merge this for now then.

I don't think I'll be able to investigate THREE. CustomBlending during this cycle though.

@mrdoob mrdoob merged commit c7976e0 into dev Jan 14, 2022
@mrdoob mrdoob deleted the alpha-true branch January 14, 2022 20:53
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.

None yet

2 participants