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: Improved lighting in LDrawLoader example #22188

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Examples: Improved lighting in LDrawLoader example #22188

merged 1 commit into from
Jul 26, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jul 26, 2021

Related issue: #22181

Description

Enabled ACESFilmicToneMapping and increased IBL brightness.

before after
Screenshot 2021-07-26 at 15 03 18 Screenshot 2021-07-26 at 15 03 31
Screenshot 2021-07-26 at 15 02 50 Screenshot 2021-07-26 at 15 03 02

/cc @gkjohnson

@mrdoob mrdoob added this to the r131 milestone Jul 26, 2021
@mrdoob mrdoob merged commit dded9d5 into dev Jul 26, 2021
@mrdoob mrdoob deleted the ldraw branch July 26, 2021 14:23
@WestLangley
Copy link
Collaborator

WestLangley commented Jul 26, 2021

It looks over-bright, and there is no way to control the environment intensity of "room environment".

Also, AcesFilmicTonemapping hue-shifts. The rendered colors should look like the CSS Lego colors. If it looks washed out, then you need to find another solution.

https://www.ldraw.org/article/547

@WestLangley
Copy link
Collaborator

With this PR.

Screen Shot 2021-07-26 at 12 19 31 PM

LDrawLoader hacked to use MeshBasicMaterial. No tone mapping. Linear output Encoding.

Screen Shot 2021-07-26 at 12 18 49 PM

@gkjohnson
Copy link
Collaborator

If we continue to use outputEncoding = sRGBEncoding I think we need to convert the LDraw colors to Linear color space.

To @mrdoob's comment (#22181 (comment)):

Shouldn't the loader produce linear colors? What if someone wanted to mix Lego models with GLTF models? 🤔

I wouldn't be against it but I think a number of model loaders I think load material and vertex colors in sRGB because that's how they're stored (PLYLoader, ColladaLoader, etc). But having a consistent color space that models are loaded in to would simplify things. If we change the loaders so colors are consistently loaded in Linear color space then I think the default value for WebGLRenderer.outputEncoding would need to changed to sRGBEncoding.

@WestLangley
Copy link
Collaborator

And the loader is setting envMapIntensity to 0.3 for some materials; 1.0 for others. Why?

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jul 26, 2021

And the loader is setting envMapIntensity to 0.3 for some materials; 1.0 for others. Why?

Oh yes sorry -- I don't believe there's a good reason. It should be removed so it defaults to 1.0. I think it was a misunderstood / misguided addition on my part. The PEARLESCENT surface finish should also probably be converted to use MeshStandardMaterial, as well.

@gkjohnson gkjohnson mentioned this pull request Jul 27, 2021
@mrdoob
Copy link
Owner Author

mrdoob commented Jul 27, 2021

@gkjohnson

I think the default value for WebGLRenderer.outputEncoding would need to changed to sRGBEncoding.

Yes! I want to change that soon. We can try next month?

@gkjohnson
Copy link
Collaborator

@mrdoob @WestLangley

Yes! I want to change that soon. We can try next month?

Would that entail converting all the model loaders to produce linear colors rather than sRGB? And I'm not sure if this has been discussed before but have you considered ferrying around the color space in the Color class so the renderer is aware of what the correct conversion should be?

const color = new THREE.Color();
color.set( 0xff0000, sRGBEncoding );
console.log( color.encoding ); // sRGBEncoding

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 27, 2021

Related #19169.

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.

None yet

4 participants