Skip to content

Conversation

@donmccurdy
Copy link
Collaborator

Related issue:

Scaling a THREE.Color by 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 of color.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 specularColor and specularIntensity already.

@WestLangley
Copy link
Collaborator

If we don't want to ever consider Color values to be sRGB...

A THREE.Color represents a color using the RGB Color Model.

I do not think it is appropriate to consider restricting THREE.Color to the sRGB Color Space.

@WestLangley
Copy link
Collaborator

WestLangley commented Dec 13, 2021

...then this change is not necessary.

Actually, we should add .emissiveIntensity as a material property (not just as a uniform, as is proposed here).

material.emissive is an RGB color, and like all colors, it is unit-less; it is akin to a mask -- or a tint.

But the light that is emitted from a material has units of radiance (or, if you prefer, luminance).

Hence, we should add material.emissiveIntensity, and assign to it units of radiance.

...and is more consistent with how we handle specularColor and specularIntensity already.

Actually, it would be consistent with how intensity units for lights are defined.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 13, 2021

We do already have material.emissiveIntensity – this PR adds a uniform to match that, rather than multiplying it into the emissive color uniform in WebGLMaterials.js.

I do not think it is appropriate to consider restricting THREE.Color to the sRGB Color Space.

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 WebGLUniforms.js, this PR is removing a case that would prematurely multiply sRGB components by a linear intensity.

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.

@donmccurdy
Copy link
Collaborator Author

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:

} else if ( v.r !== undefined ) {
if ( cache[ 0 ] !== v.r || cache[ 1 ] !== v.g || cache[ 2 ] !== v.b ) {
gl.uniform3f( this.addr, v.r, v.g, v.b );
cache[ 0 ] = v.r;
cache[ 1 ] = v.g;
cache[ 2 ] = v.b;
}
} else {

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 13, 2021

Regarding the discussion in #22346 this PR makes totally sense and I'm happy to see that the following line is change:

uniforms.emissive.value.copy( material.emissive ).multiplyScalar( material.emissiveIntensity );

The multiplication should happen in the shader where linear color space is ensured.

@WestLangley
Copy link
Collaborator

We do already have material.emissiveIntensity

Oh, of course. I added it in #7918. :-)

@WestLangley
Copy link
Collaborator

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 renderer.outputEncoding will have to be generalized to support a variety of output devices.

@WestLangley
Copy link
Collaborator

The multiplication should happen in the shader where linear color space is ensured.

Doing that in the shader would be per-pixel. Currently, we multiply once in the engine.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 13, 2021

The multiplication should happen in the shader where linear color space is ensured.

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.

@WestLangley
Copy link
Collaborator

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.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 13, 2021

... implicitly the three.js working color space is linear-sRGB.

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.

Colors passed to the shader must be in the working color space.

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:

  • (A) When a user constructs a THREE.Color with an argument (e.g. #EE88CC) we immediately convert that from the working color space to linear space, setting .r .g .b to linear RGB components. The renderer would do no conversions.
  • (B) When a user constructs a THREE.Color we consider the entire color to remain in the working color space. The renderer would then convert from the working color space to linear space, if necessary.

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.

@donmccurdy donmccurdy marked this pull request as draft December 14, 2021 06:48
@donmccurdy
Copy link
Collaborator Author

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.

@donmccurdy donmccurdy closed this Dec 16, 2021
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