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

common.glsl: Introduce linearToRelativeLuminance() #12125

Merged
merged 2 commits into from
Sep 4, 2017
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Sep 4, 2017

see #11986

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 4, 2017

@WestLangley I've just changed LuminosityShader for now. Do you think we need further comments like proposed in #11986 (comment) ?

@WestLangley
Copy link
Collaborator

For now, I would just call the function relativeLuminance() and not differentiate between linear or sRGB inputs. This is a subtlety that most users will neither understand, nor care about.

If @bhouston feels strongly about keeping the name linearToRelativeLuminance, then you should modify your examples to only use the function when you know the input space is linear.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 4, 2017

Okay!

@WestLangley
Copy link
Collaborator

Let's make sure @bhouston finds this acceptable.

@bhouston
Copy link
Contributor

bhouston commented Sep 4, 2017

Let's make sure @bhouston finds this acceptable.

It is for linear RGB color spaces, it isn't a general function. There is a different function for dealing sRGB or gamma corrected spaces, basically you need to decode sRGB or gamma RGB to linear and then apply this.

So I much prefer the original name because it will remind people to be in linear space before using this conversion.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 4, 2017

Only two examples make use of LuminosityShader right now. webgl_postprocessing_sobel and webgl_shaders_tonemapping. In both cases LuminosityShader works on a linear input. So linearToRelativeLuminance() should be consistent with regard to the examples.

I think only ToneMapShader needs a fix.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 4, 2017

Should be okay now!

@mrdoob mrdoob merged commit 07ff147 into mrdoob:dev Sep 4, 2017
@mrdoob
Copy link
Owner

mrdoob commented Sep 4, 2017

Thanks!

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