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: Remove gammaOutput, introduce outputEncoding. #18127

Merged
merged 3 commits into from Dec 13, 2019

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 11, 2019

As proposed in #11337.

This PR removes gammaOutput and introduces outputEncoding as a replacement. gammaOutput can still be used, however a warning will be generated. The renderer ensures to update materials if outputEncoding is changed during runtime.

There is in subtle change that needs some highlight: getTextureEncodingFromMap() does only depend from the given map now. That means the previous behavior of automatically setting the encoding of a render target's textures to GammaEncoding if gammaOutput is true does not exist anymore. This is also the correct behavior from my point of view. Meaning, when using post-processing via EffectComposer, WebGLRenderer.outputEncoding should not be changed and gamma correction should be done via ShaderPass and GammaCorrectionShader. Check out how webgl_materials_normalmap is written now to see this idea in action.

@WestLangley
Copy link
Collaborator

It is not appropriate to use both of these patterns in the same example:

renderer.outputEncoding = THREE.GammaEncoding;

texture.encoding = THREE.sRGBEncoding;

Doing so will cause color distortion.

In the examples, use THREE.sRGBEncoding for both texture encoding and renderer output encoding.

Some users prefer the flexibility of GammaEncoding in combination with renderer.gammaFactor. I think it would be appropriate to have one example use that case.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 11, 2019

Doing so will cause color distortion.

I've just tried to retain the current visuals as good as possible. If things can be improved, I'm glad to do this with this PR.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 11, 2019

I think it would be appropriate to have one example use that case.

Do you have an example already in mind?

@WestLangley
Copy link
Collaborator

It would have to be one without textures, I guess...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 11, 2019

How about webgl_buffergeometry?

@mrdoob mrdoob added this to the r112 milestone Dec 11, 2019
@WestLangley
Copy link
Collaborator

How about webgl_buffergeometry?

Or none. I don't have a strong opinion on that.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 12, 2019

Then I would leave it out for now since the API is proper documented. If necessary, we can still update one of the examples in the future.

@mrdoob mrdoob merged commit 48072f9 into mrdoob:dev Dec 13, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 13, 2019

Many thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2019

Yay! It's great to get rid of gammeInput and gammaOutput in a single release^^

@mrdoob
Copy link
Owner

mrdoob commented Dec 14, 2019

So many things are being solved in this release 😍

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2019

I would say it's the best release of 2019 😁

@mrdoob
Copy link
Owner

mrdoob commented Dec 14, 2019

Getting it ready for the next decade 😀

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

3 participants