-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
WebGLRenderer: Add emissiveIntensity uniform #23010
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
Conversation
A I do not think it is appropriate to consider restricting |
Actually, we should add
But the light that is emitted from a material has units of radiance (or, if you prefer, luminance). Hence, we should add
Actually, it would be consistent with how intensity units for lights are defined. |
|
We do already have
It certainly isn't my intention to limit THREE.Color to [0,1] components. I also don't intend to limit it to the sRGB Color Space – see "LegacyWorkflow" in #22346 (comment) (the comment, not the PR itself). But I think the consensus of that thread is that we do want the option of having the renderer treat a THREE.Color as sRGB without requiring users to explicitly do color-by-color conversion, and that the option should probably be the default eventually. Where an intensity parameter exists that intensity should be linear, multiplied by the color after sRGB→Linear conversion. Because I expect conversion to eventually happen in The code in #22346 is an alternative approach, in which THREE.Color instances accept sRGB inputs but internally store linear components. I'm fine with either of these but discussion in the thread seemed to be leaning toward doing conversion in the renderer instead, such that THREE.Color values are (in general) sRGB. |
|
I'm not ready to open a PR for this part, but the idea is to eventually include sRGB→Linear conversion (behind a renderer option) here: three.js/src/renderers/webgl/WebGLUniforms.js Lines 215 to 227 in 5b8b9c6
|
|
Regarding the discussion in #22346 this PR makes totally sense and I'm happy to see that the following line is change:
The multiplication should happen in the shader where linear color space is ensured. |
Oh, of course. I added it in #7918. :-) |
|
Guys, you can only convert from one color space to another color space. So you need to know the color space the color is in, and you need to know the working colorspace to convert to. three.js does not handle color space yet, but as I have said elsewhere, implicitly the three.js working color space is linear-sRGB. That could change in the future to a wider-gamut color space such as ACEScg. CSS colors are in sRGB colorspace currently, but in the near future, other color spaces will be allowed. And for the record, what three.js calls |
Doing that in the shader would be per-pixel. Currently, we multiply once in the engine. |
If #22346 is implement, the emissive color is not necessarily in linear color space. Theoretically it could be in any color space as long as it is uploaded to the shader where a linear workflow is in place. |
The emissive color must be in the scene-referred three.js working color space, which is currently linear-sRGB. Colors passed to the shader must be in the working color space. |
I think the term "linear-sRGB" will create confusion for most readers here, so I'm just going to refer to this as "linear space" if that's alright. :) I'm also going to define "working color space" to mean, "the color space that users are primarily interacting with" in our APIs.
Based on the definition above, I would have said that colors passed to the shader must be in linear color space. In order to ensure that, conversion from the working color space to linear space needs to happen somewhere — and preferably not the shader. The goal of #22346 is to provide at least one alternative, sRGB, for the working color space. Allowing users to provide colors in sRGB Color Space appears to be the more common default (with the notable exception of Filament?) and more consistent with prevailing CSS usage and texture authoring workflows. If I set a material's color in Blender, it is considered sRGB. In three.js, it is considered linear. This is a source of user confusion. There are basically two ways we can provide that:
In #22346 I attempted to implement (A), but now the conversation in that thread has pushed toward (B) instead. I think this PR is a pre-requisite for (B). I am still fine with either (A) or (B), but would like to narrow it down to one. |
|
After some further reading and discussion (thanks @WestLangley! 🙏), my reasons for this PR are really incorrect. The code itself is probably fine, and possibly still worthwhile. But I'd like to close the issue for now and reopen a new thread (coming soon) with the right conceptual basis. |
Related issue:
Scaling a
THREE.Colorby linear intensity is more complicated once we accept sRGB inputs. One prominent example was RoomEnvironment. This PR changes RoomEnvironment to use emissive and emissiveIntensity instead ofcolor.setScalar( intensity ), and adds a uniform to ensure that emissive and emissiveIntensity are not pre-multiplied.If we don't want to ever consider Color values to be sRGB, then this change is not necessary.
This PR can be safely merged with or without the addition of a linear workflow, and is more consistent with how we handle
specularColorandspecularIntensityalready.