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

WebGLRenderer: Fix incorrect background color space when setting scene.background to color #28434

Merged
merged 4 commits into from
May 20, 2024

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented May 20, 2024

Related issue: #28423

Description

Previously the transmission buffer was rendering the background with an incorrect color space causing it to be lighter. I'm not entirely clear where the background color was being rendered for the transmission pass previously. Here's a demo showing the incorrect colors through a transmissive object.

  • Ensures the transmission buffer is assigned a color space.
  • Calls background.render when rendering the transmission pass which converts the color apropriately.
Before After
image image
image image

Edit: Possibly related to #28118, as well?

@gkjohnson gkjohnson requested a review from donmccurdy May 20, 2024 02:00
Copy link

github-actions bot commented May 20, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
678.9 kB (168.2 kB) 679 kB (168.3 kB) +80 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
456.9 kB (110.3 kB) 457 kB (110.3 kB) +57 B

@@ -1430,7 +1430,8 @@ class WebGLRenderer {
samples: 4,
stencilBuffer: stencil,
resolveDepthBuffer: false,
resolveStencilBuffer: false
resolveStencilBuffer: false,
colorSpace: ColorManagement.workingColorSpace,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, it hadn't occurred to me that the default color space of a render target is THREE.NoColorSpace! Good catch, and I expect that there my be some other places we may need to fix similar issues.

Comment on lines 1465 to 1466
const renderBackground = xr.enabled === false || xr.isPresenting === false || xr.hasDepthSensing() === false;
if ( renderBackground ) background.render( scene );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the motivation for this part of the change, why would XR presentation and depth sensing affect whether the background is drawn?

Copy link
Collaborator Author

@gkjohnson gkjohnson May 20, 2024

Choose a reason for hiding this comment

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

I don't know the history of some of these decisions, unfortunately - just that it's already done elsewhere in WebGLRenderer to determine whether to draw the background. There's probably some clean up that should happen in the render function in this respect.

See https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1156

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok! I don't know much about that part but will approve the PR in any case, thanks again!

@gkjohnson gkjohnson requested a review from Mugen87 May 20, 2024 03:27
@gkjohnson gkjohnson added this to the r165 milestone May 20, 2024
@@ -1461,6 +1462,9 @@ class WebGLRenderer {

_this.clear();

const renderBackground = xr.enabled === false || xr.isPresenting === false || xr.hasDepthSensing() === false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind passing the existing renderBackground flag from the render() method into renderTransmissionPass(). In this way, it's not necessary to make the same evaluation twice.

Copy link
Collaborator

@Mugen87 Mugen87 May 20, 2024

Choose a reason for hiding this comment

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

You can also consider to rename it to _renderBackground and store it in the same scope like _clippingEnabled. Then it is not necessary to change the signature of renderTransmissionPass().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@Mugen87
Copy link
Collaborator

Mugen87 commented May 20, 2024

I'm not entirely clear where the background color was being rendered for the transmission pass previously.

I think it didn't at all. Before #28118, the background was renderer to the default framebuffer (or current bound render target) before any render pass were executed. This approach introduces performance issue though as explain in #28118.

Now, the background is rendered after the transmission pass. However, before and after #28118 the background was never rendered with the transmission render target bound. This should be corrected with this PR.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented May 20, 2024

Sounds good - as I mentioned previously I'm a bit confused by some of the logic relating to the transmission pass and I don't know enough about the reasoning to make changes to it. This code block, for example, is really odd to me:

			_this.getClearColor( _currentClearColor );
			_currentClearAlpha = _this.getClearAlpha();
			if ( _currentClearAlpha < 1 ) _this.setClearColor( 0xffffff, 0.5 );

			_this.clear();

Why are we forcing a 50% transparent white background if the background is at all partially transparent of any color, for example?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 20, 2024

The reasoning behind this is explained in #25819.

@gkjohnson gkjohnson merged commit 6208714 into mrdoob:dev May 20, 2024
12 checks passed
@gkjohnson gkjohnson deleted the transmission-bg-fix branch May 20, 2024 10:10
@@ -1461,6 +1464,8 @@ class WebGLRenderer {

_this.clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

After reading the code again I think _this.clear(); can be removed now. Otherwise clear() is effectively called twice.

Copy link
Collaborator

@Mugen87 Mugen87 May 20, 2024

Choose a reason for hiding this comment

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

The previous approach only honored the clear color set via setClearColor(). But if you trigger the clear indirectly via background.render( scene ), then both clear color and Scene.background = new Color() are honored.

Copy link
Collaborator Author

@gkjohnson gkjohnson May 21, 2024

Choose a reason for hiding this comment

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

I don't think it's necessarily the case? Again the rationale behind the current transmission clearing logic isn't super clear to me and may deserve some more comments (especially for the white clear color, for example). If renderBackground is set to false then this call to "clear" will still be run, for example. I guess it can at least go in an "else" branch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If renderBackground is set to false then this call to "clear" will still be run, for example. I guess it can at least go in an "else" branch?

That makes sense. This particular clear should be retained in XR.

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