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

Examples: Linearize sRGB material color in Matcap example #20279

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

WestLangley
Copy link
Collaborator

Color pickers return CSS colors in sRGB colorspace, while material.color is a scene-referred linear value.

@WestLangley WestLangley added this to the r121 milestone Sep 7, 2020
@WestLangley
Copy link
Collaborator Author

@donmccurdy This is something we have not been careful about. There are a lot of examples that need to be changed if we want to handle this consistently...

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

There are a lot of examples that need to be changed if we want to handle this consistently...

I'm not sure we want to go down this path since the respective code is confusing for most users. Since the color picking is in some sense a subjective choice, I'm not sure color spaces do matter much in these cases.

@WestLangley
Copy link
Collaborator Author

I'm not sure we want to go down this path since the respective code is confusing for most users.

Most users are not coding.

Since the color picking is in some sense a subjective choice, I'm not sure color spaces do matter much in these cases.

Color spaces do not matter much?

We have to handle color space correctly -- and consistently. Otherwise, for example, MeshBasicMaterial will not render in the selected color. Likewise, the other materials will not render as expected.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

Most user do code (three.js is JavaScript library, no?) and most of them have problems understanding color spaces.

The engine should try to hide this complexity as good as possible. It's the same motivation like integrating PMREM. Delegating these tasks to the user is just a proof that the engine is not capable of handling it internally.

@WestLangley
Copy link
Collaborator Author

@Mugen87 said

I'm not sure color spaces do matter much in these cases.

With all due respect, we should not have to be debating whether color space is important at this point.

//

The engine should try to hide this complexity as good as possible.

How is the engine going to know if the color was selected by a color-picker? It is the developer's responsibility to linearize it.

Screen Shot 2020-09-07 at 11 25 07 AM

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

With all due respect, we should not have to be debating whether color space is important at this point.

Color spaces are important but the engine should try to not confront the user with it whenever possible. The proposed solution (asking users to work with convertSRGBToLinear()) is not a user-friendly approach from my point of view.

I just wanted to highlight with my statement that users often "visually" pick a color. The user does not care in what color space it is. The engine should somehow find a way so that the selected color is actually rendered at screen without asking for further configurations from the user. If necessary, with a custom color-picker.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

Or maybe something like THREE.CSSColor?

@WestLangley
Copy link
Collaborator Author

I just wanted to highlight with my statement that users often "visually" pick a color. The user does not care in what color space it is.

The user of the app is not the same as the developer of the app.

The proposed solution (asking users to work with convertSRGBToLinear()) is not a user-friendly approach from my point of view.

The developer must care.

glTF, for example, has a spec, and adheres to the same conventions that three.js does. The developer must understand that spec.

Just as developers must set texture.encoding, colors must be linear. It is the same concept.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

When you load a glTF asset, GLTFLoader takes care about setting the right encodings for the textures. The user of the library does not have to care about this. Ideally, the engine can provide similar automatisms when defining colors.

@WestLangley
Copy link
Collaborator Author

You are confusing user with developer.

Ideally, the engine can provide similar automatisms when defining colors.

You are blocking my PR based on some ideal in your head which has neither a spec nor a solution. Please avoid doing that.

Do not let the perfect be the enemy of the good.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

Sorry but project members sometimes have different opinions on certain topics. Please try to accept this instead of becoming defensive.

@WestLangley
Copy link
Collaborator Author

Sorry but project members sometimes have different opinions on certain topics. Please try to accept this instead of becoming defensive.

Sorry, that is a deflection tactic. Please stay on-topic.

//

I would like the demo I am maintaining to work correctly. This 2-line PR makes it work correctly.

This PR also demonstrates to @donmccurdy an issue I am concerned about so we can address it in the future.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 7, 2020

See #20287 (comment) for my thoughts, but I'm afraid I don't really like either of the setSRGB or CSSColor APIs. There are many examples of software that provide correct color workflows without requiring the user to explicitly state the colorspace each time they interact with a color, and that should be our goal.

If the user has done [TBD], these color conversions should happen automatically. I'm just not sure what the "TBD" should be yet, or how to implement it. Perhaps something like:

import { Color } from 'three';

const color = new Color();

color.setHex( 0xCCCCCC ); // no conversion

Color.setLinearWorkflow( true );

color.setHex( 0xCCCCCC ); // automatic sRGB -> linear

Eventually, setLinearWorkflow( true ) and renderer.outputEncoding = sRGBEncoding may become the default.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

I'm fine with that goal. For me, it's important to not spread the code base with color conversion related logic (and thus not confront users with it). Providing a proper default and enable color space configuration at a single place sounds like a good solution.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Sep 7, 2020

If the user has done [TBD], these color conversions should happen automatically. I'm just not sure what the "TBD" should be yet, or how to implement it.

Exactly. No one does.

In the mean time, I would like the demo I am maintaining to be correct.

With #20287 I could do

color.setSRGB( value );

@donmccurdy
Copy link
Collaborator

I'm fine with this PR. @Mugen87 I do see your concern about complexity, but I agree with @WestLangley that the current usage is badly incorrect. Until we have an agreed-upon API, we should try to show correct usage, whether it's ugly or not...

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2020

Okay, for this demo I'm fine with the usage of convertSRGBToLinear(). Hope we find a better solution in the future.

@WestLangley
Copy link
Collaborator Author

I'm fine with this PR. @Mugen87 I do see your concern about complexity, but I agree with @WestLangley that the current usage is badly incorrect. Until we have an agreed-upon API, we should try to show correct usage, whether it's ugly or not...

Since we are in agreement, merging for now. I will not correct other examples until alternative APIs have been discussed.

At least everyone is aware of the issue now. :-)

@WestLangley WestLangley merged commit 74ec0bc into mrdoob:dev Sep 7, 2020
@WestLangley WestLangley deleted the dev_examples_matcap branch September 7, 2020 22:02
@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2020

What do you guys think about adding THREE.LinearColor (extend THREE.Color) which would make THREE.Color sRGB?
Or would it make more sense to add THREE.sRGBColor (extend THREE.Color) which would make THREE.Color linear?

@WestLangley
Copy link
Collaborator Author

I am not sure it is a good idea to assign a particular colorspace to THREE.Color...

@donmccurdy
Copy link
Collaborator

I share @WestLangley's concern ... and requiring users to replace .color and .emissive properties on their materials and lights sounds error-prone and verbose, for whichever workflow is not the default.

Continuing from #20287:

I'd prefer that setHex()/getHex() and CSS string equivalents would assume sRGB input/output ... Methods operating on r/g/b components could remain linear

Personally, I think that is too confusing. The different color formats should be equivalent, IMO.

I'm okay with @WestLangley's suggestion here, too, that setHex() and set() and color.r = .5 should have equivalent behavior. We could do that in the Color class, as #20279 (comment), suggests. Or in the renderer, for any Color-typed uniform, when some setting (inputEncoding?) is enabled. I realize that sounds a bit like gammaInput, removed in #18108, but I don't believe that ever affected Color uniforms before?

@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2020

I was thinking we could add an additional parameter to the relevant methods to specify the color space/encoding, but I think convertSRGBToLinear() is simpler and educational.

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