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

LDrawLoader: Convert to linear colors when parsing models #23272

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jan 19, 2022

Related issue: --

https://raw.githack.com/gkjohnson/three.js/ldraw-linear-color/examples/webgl_loader_ldraw.html

Description

Semi-related to @donmccurdy's color space consistency efforts (#22346) -- all loaders should appropriately label texture color spaces, convert colors to Linear color space, and set the renderer output color space to sRGB. This PR does so for LDrawLoader.

From what I can tell these are the loader examples that still are not setting renderer.outputEncoding = sRGBEncoding on the page meaning they don't appropriately set the model color spaces for linear lighting calculations on load where I think the color space should be known at parse time:

  • ColladaLoader examples
  • FBXLoader examples
  • IFCjs example
  • OBJLoader examples
  • SVGLoader (example logic only)
  • Maybe: Vox, VRML, VTK, PCD, GCode

Here are some before and after screenshots. It seems to get rid of the blue specular look on the black pieces. The AT-ST line colors significantly darker now, as well, because tone mapping is being applied to Linear rather than sRGB values. I'm kind of on the fence about the change because while converting the colors to linear is more correct I kind of stylistically prefer some of the "before" screenshots better. Or maybe I'm just used to them. The spaceman blue definitely looks more like the real model now, though:

BEFORE AFTER AFTER w/o Tone Mapping
image image image
image image image
image image image

cc @WestLangley

@gkjohnson gkjohnson requested a review from yomboprime January 19, 2022 06:36
Copy link
Collaborator

@yomboprime yomboprime left a comment

Choose a reason for hiding this comment

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

I guess this PR is correct. Sorry but I don't know much about color spaces.

About the SVGLoader, I would say in that case it does not apply since the loader only parses the file but does not create Mesh or Material objects. Instead they are created afterwards by the specific application, based on path points and .userData, like styles. I think colors should be remain untouched by the SVGLoader.

BTW, I like more the new tones :-)

@mrdoob mrdoob added this to the r137 milestone Jan 19, 2022
@gkjohnson
Copy link
Collaborator Author

About the SVGLoader, I would say in that case it does not apply since the loader only parses the file but does not create Mesh or Material objects. Instead they are created afterwards by the specific application, based on path points and .userData, like styles. I think colors should be remain untouched by the SVGLoader.

Oh yes I see now -- you're right. In that case then maybe the conversion should be happening in the example code itself.

BTW, I like more the new tones :-)

I think most of them look better -- the only one that looked odd to me were the particularly dark lines in the AT-ST example from the tone mapping.

@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2022

From what I can tell these are the loader examples that still are not setting renderer.outputEncoding = sRGBEncoding on the page meaning they don't appropriately set the model color spaces for linear lighting calculations on load where I think the color space should be known at parse time:

  • ColladaLoader examples
  • FBXLoader examples
  • IFCjs example
  • OBJLoader examples
  • SVGLoader (example logic only)
  • Maybe: Vox, VRML, VTK, PCD, GCode

Should we create a new issue to keep track of this work?

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