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

Premultiplied alpha on materials #8245

Merged
merged 8 commits into from
Mar 1, 2016

Conversation

bhouston
Copy link
Contributor

Using premultiplied alpha on materials leads to better results for areas where there are specular highlights on transparency when using RGBA8 buffers. This change should be perfectly backwards compatible, but allows for easy usage of premultiplied alpha when desired.

This was discussed here: #5810 Clara.io has been running for more than year with premultiplied alpha to great results. I'd like to get this option into ThreeJS proper, at least as an option that people can use.

@@ -491,6 +491,8 @@ THREE.WebGLProgram = ( function () {
parameters.shadowMapEnabled ? '#define ' + shadowMapTypeDefine : '',
parameters.pointLightShadows > 0 ? '#define POINT_LIGHT_SHADOWS' : '',

parameters.premultipliedAlpha ? "#define PREMULTIPLIED_ALPHA" : '',
Copy link
Owner

Choose a reason for hiding this comment

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

We need to add parameters.premultipliedAlpha = material.premultipliedAlpha to WebGLPrograms.js too.

@WestLangley
Copy link
Collaborator

As an alternative API, consider what I said in #5810 (comment):

In fact, we could add a new blending option: THREE.PremultipyAlphaBlending, making the setting of the blending even easier.

That way, all you have to do in javascript is set:

material.blending = THREE.PremultipyAlphaBlending;

The new property material.premultipliedAlpha is not required, and the user does not have to specify a custom blending mode.

The new THREE.PremultipyAlphaBlending blending option does this:

_gl.blendEquation( _gl.FUNC_ADD );
_gl.blendFunc( _gl.ONE, _gl.ONE_MINUS_SRC_ALPHA );

In the shader, you would add this, instead:

#ifdef PREMULTIPLIED_ALPHA_BLENDING

    gl_FragColor.rgb *= gl_FragColor.a;

#endif

@bhouston
Copy link
Contributor Author

I've fixed a few bugs with the implementation, switch from the boolean material.premultipledAlpha to material.blend = THREE.PremultipledAlphaBlending and then I have added an example:

http://ci.threejs.org/api/pullrequests/8245/examples/webgl_materials_transparency.html

@bhouston
Copy link
Contributor Author

Here is an example of the different premultiplied alpha has - it is drastic:

premutlipledalpha

@@ -0,0 +1,6 @@
#ifdef PREMULTIPLIED_ALPHA
Copy link
Collaborator

Choose a reason for hiding this comment

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

#ifdef PREMULTIPLIED_ALPHA_BLENDING

gl_FragColor.rgb *= gl_FragColor.a;

#endif

We do not want this to be confused with renderer.premultipliedAlpha.

@bhouston bhouston force-pushed the premultiplied_alpha_on_materials branch from 48a46a9 to 7009dbf Compare February 29, 2016 18:00
@bhouston
Copy link
Contributor Author

@WestLangley thank you for the valuable feedback. :)

You are right that I should make those changes. One issue though is that I have already built another PR (#8250) on top of this one and it included other refactors. Given that this feedback is about internals, and not about public interfaces, could we merge this and possibly the other PR (inline tonemapping) and then I can make these non-interface refactoring changes?

@WestLangley
Copy link
Collaborator

@bhouston Sure. Merge and fix later.

mrdoob added a commit that referenced this pull request Mar 1, 2016
@mrdoob mrdoob merged commit 5ab0845 into mrdoob:dev Mar 1, 2016
@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2016

Thanks guys!

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2016

One thing though....

Is there a way to make this the default? In the previous approach (material.premultipliedAlpha) I liked that we could just se that to true by default.

With the new material.blending approach (which I like better) seems like the user needs to change the blending by hand, it won't just work by setting transparent: true in the material.

@bhouston
Copy link
Contributor Author

bhouston commented Mar 1, 2016

Well you could set Material.blending = THREE.PremultipliedAlphaBlending in Material.js?

@bhouston
Copy link
Contributor Author

bhouston commented Mar 1, 2016

We could also rename THREE.PremultipliedAlphaBlending to be THREE.PremultipledAlphaNormalBlending, which suggests this is the normal mode. It is a bit long through. We can then have other THREE.PremultipledAlphaAddBlending, etc.

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2016

Well you could set Material.blending = THREE.PremultipliedAlphaBlending in Material.js?

I think that should do. However, what happens if the user wants premultipliedAlpha but with AdditiveBlending?

@WestLangley
Copy link
Collaborator

Discussion continued in #8255

@bhouston bhouston deleted the premultiplied_alpha_on_materials branch March 14, 2016 13:45
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.

3 participants