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: Make background result independent of premultipliedAlpha setting #21034

Merged
merged 5 commits into from
Jan 21, 2021

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jan 9, 2021

Fix #20819

Follow on from #20983

Description

Fixes the discrepancy in solid background color described in #20819 (comment) when premultipledAlpha is set to true and false. As far as I can tell when the premultipliedAlpha setting is set to true the browser seems to implicitly multiply the renderer clear color by the clear alpha to ensure it's multiplied (can anyone confirm? The documentation on the setting is pretty thin). This is an issue because RGBE encoding stores E in alpha. This PR uses a box to render the background color so renderer clear color is not used and gives consistent results regardless of the setting.

Here's a before and after achieved by adjusting the webgl_loader_gltf example with the following settings and tone mapping disabled:

renderer.setClearColor( 0xff0000 );
const envMap = pmremGenerator.fromScene( scene ).texture;
premultipliedAlpha : true premultipliedAlpha : false
Before image image
After image image

cc @elalish

@gkjohnson
Copy link
Collaborator Author

Here's the same set of tests from #20983 (comment) with tone mapping disabled and premultiplied alpha set to true:

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

@gkjohnson
Copy link
Collaborator Author

The webgl_furnace_test e2e screenshot test is failing but I'm actually not sure why. It does use PRMREMGenerator.fromScene but here's the difference in the screenshots when I run in locally:

in dev newly generated screenshot
image webgl_furnace_test

As far as I can tell from the code the spheres should only be blue when the user mouses over the page.

@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2021

/ping @elalish

@mrdoob mrdoob added this to the r125 milestone Jan 12, 2021
@elalish
Copy link
Contributor

elalish commented Jan 12, 2021

Seems good to me. We should figure out what's up with the furnace test. I'm hoping it's just a weird artifact of how the screen-shot was done rather than an incorrect render. That test should still be valid.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Jan 12, 2021

Seems good to me. We should figure out what's up with the furnace test. I'm hoping it's just a weird artifact of how the screen-shot was done rather than an incorrect render. That test should still be valid.

I merged in latest dev and it looks like the screenshot was fixed (at least on my local machine). I'm not sure how the PREM change could have affected the sphere color but perhaps something else was causing an issue.

EDIT: I spoke too soon...

@gkjohnson
Copy link
Collaborator Author

Okay here are the new results of the screenshot test:

in dev newly generated screenshot
image webgl_furnace_test

The background is now lighter which should be expected given that the furnace test uses fromScene and default premultipliedAlpha setting. Would you like me to update screenshot?

@elalish
Copy link
Contributor

elalish commented Jan 12, 2021

Yeah, go ahead and update the screenshot. That looks great, thanks!

@gkjohnson
Copy link
Collaborator Author

Looks like webgl_materials_envmaps_hdr_nodes has an issue, now? But I've made the screenshot locally and see no differences. Is there a way to see the generated screenshot from CI?

@gkjohnson
Copy link
Collaborator Author

@mrdoob I merged master in and it looks like all tests are passing now though I wouldn't have expected that to change anything. I was under the impression that the e2e tests should be deterministic, right? But I see from the GH actions setup that we run the same e2e test 4 times with no changes and it seems it can fail in only one of the runs. Are there still some quirks in the setup that cause it to incorrectly fail sometimes?

@gkjohnson
Copy link
Collaborator Author

I see from the GH actions setup that we run the same e2e test 4 times with no changes and it seems it can fail in only one of the runs

Nevermind I see now that the four E2E runs check different screenshots between them so they're evaluated in parallel so it makes sense that only one of the E2E CI runs would fail. Either way it looks like merging master fixed the failure so this should be good.

@mrdoob mrdoob merged commit 57876fc into mrdoob:dev Jan 21, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 21, 2021

Thanks!

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.

PMREMGenerator: FromScene behavior differs with renderer clear color vs scene.background
3 participants