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

Shaders: Renamed linearToRelativeLuminance() function #24347

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

WestLangley
Copy link
Collaborator

This PR renames the linearToRelativeLuminance() function.

We are not computing "relative luminance" here since we are are not normalizing the input. The input is unbounded.

Furthermore, the formula is correct only if the RGB-valued input is in a linear color space with sRGB primaries and D65 white point. If the working color space were to change, then different weights would be required.

Consequently, I renamed the method to luminance(), and added an inline comment explaining the assumptions.

Alternatively, the function could be named something more precise, such as linearSRGBtoLuminance(), but personally, I think that is overkill for now. My expectation is the three.js working color space is not going to change any time soon -- certainly not until wide-gamut monitors are commonplace -- so the simpler method name seems acceptable to me.

Other opinions welcome.

/ping @donmccurdy

@WestLangley WestLangley added this to the r143 milestone Jul 13, 2022
Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! It's helpful to have these requirements identified by a comment as you've done here, but I don't expect that changing the working color space will be a priority any time soon.

@Mugen87 Mugen87 merged commit 3ef8abb into mrdoob:dev Jul 18, 2022
@WestLangley WestLangley deleted the dev-luminance branch July 18, 2022 12:01
@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
@takahirox
Copy link
Collaborator

Sorry for the late response in the already merged PR.

If I'm right, this function doesn't seem to be used in the core. Do we really want to keep this function in the core?

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.

5 participants