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

PMREMGenerator: Improve handling of background and clear color #20983

Merged
merged 3 commits into from
Jan 8, 2021

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #20819

Description

This improves the handling of scene.background and renderer.clearColor in PMREMGenerator so that the resulting env map is the same regardless of whether or not a color is set to the scene background or the renderer clear color. Both scenes will now look like the second image in #20819 instead of being different.

I also fixed the fact that PMREMGenerator was modifying the scene.background color in place not resetting the scene.background color after finishing rendering.

I've also been looking at the background tinting color issue I brought up in #20819, as well, and I think I understand what's happening now and will have a fix in the next few days.

Happy new year!

cc @elalish

@mrdoob mrdoob added this to the r125 milestone Jan 2, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 7, 2021

@elalish looks good?

@elalish
Copy link
Contributor

elalish commented Jan 7, 2021

Yeah, LGTM. I haven't tested it in depth or anything since I'm not too worried about it breaking me (my scenes don't use the background).

@gkjohnson
Copy link
Collaborator Author

I've tested this by modifying the webgl_loader_gltf example, adjusting this line to use fromScene, and testing with different colors and values set for scene.background and renderer.clearColor:

Case 1

// showing scene background overrides background clear color
renderer.setClearColor( 0xff0000 );
scene.background = new THREE.Color( 0xffffff );
const envMap = pmremGenerator.fromScene( scene ).texture;

image

Case 2

// showing renderer clear color is used when scene.background is null
renderer.setClearColor( 0xff0000 );
scene.background = null;
const envMap = pmremGenerator.fromScene( scene ).texture;

image

Case 3

// showing renderer clear color yields the same result as scene.background color
renderer.setClearColor( 0xffffff );
scene.background = null;
const envMap = pmremGenerator.fromScene( scene ).texture;

image

Case 4

// showing the environment map is used if set as the scene.background
renderer.setClearColor( 0xffffff );
scene.background = texture;
const envMap = pmremGenerator.fromScene( scene ).texture;

image

@mrdoob
Copy link
Owner

mrdoob commented Jan 8, 2021

image

Unrelated, but... that looks so good 🤩

@mrdoob mrdoob merged commit 5fe52c1 into mrdoob:dev Jan 8, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 8, 2021

Thanks!


if ( background.isColor ) {

_backgroundColor.copy( background ).convertSRGBToLinear();
Copy link
Owner

Choose a reason for hiding this comment

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

One note about this...

It's pretty clear to me now that THREE.Color should always be linear.

We have to find all the places where it's not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you elaborate? My impression was that scene.background and renderer.clearColor were the only places that SRGB colors were expected (#20819 (comment)). Are you suggesting those should be changed to Linear, as well? Are there other places SRGB is required?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know about renderer.clearColor, but scene.background should definitely be linear.
I think color spaces / encodings is a hard enough concept. Having exceptions make things ever harder.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm all for updating the background colors to expect linear colors but I do think it would be very confusing if renderer.clearColor and scene.background yield different results with the same color.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, renderer.clearColor should be linear too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but I do think it would be very confusing if renderer.clearColor and scene.background yield different results with the same color.

Indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Linear color sounds great and all, but isn't a common use case of background/clear color to match the CSS of the page around it? If tone-mapping is applied, that's going to get pretty tricky.

Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it be better to make the background transparent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's a simple solution. Some perf cost, but probably not enough to worry about.

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