-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1430,7 +1430,8 @@ class WebGLRenderer { | |
samples: 4, | ||
stencilBuffer: stencil, | ||
resolveDepthBuffer: false, | ||
resolveStencilBuffer: false | ||
resolveStencilBuffer: false, | ||
colorSpace: ColorManagement.workingColorSpace, | ||
} ); | ||
|
||
// debug | ||
|
@@ -1461,6 +1462,9 @@ class WebGLRenderer { | |
|
||
_this.clear(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading the code again I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous approach only honored the clear color set via There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That makes sense. This particular clear should be retained in XR. |
||
|
||
const renderBackground = xr.enabled === false || xr.isPresenting === false || xr.hasDepthSensing() === false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind passing the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can also consider to rename it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
if ( renderBackground ) background.render( scene ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
|
||
// Turn off the features which can affect the frag color for opaque objects pass. | ||
// Otherwise they are applied twice in opaque objects pass and transmission objects pass. | ||
const currentToneMapping = _this.toneMapping; | ||
|
There was a problem hiding this comment.
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.