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.

Sign up for GitHub

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

Jump to bottom

WebGLRenderer: Let transmissionRenderTarget size match relevant viewport size #28088

Merged
merged 1 commit into from
Apr 9, 2024
Merged

WebGLRenderer: Let transmissionRenderTarget size match relevant viewport size #28088

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/renderers/WebGLRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
import { Color } from '../math/Color.js';
import { Frustum } from '../math/Frustum.js';
import { Matrix4 } from '../math/Matrix4.js';
import { Vector2 } from '../math/Vector2.js';
import { Vector3 } from '../math/Vector3.js';
import { Vector4 } from '../math/Vector4.js';
import { WebGLAnimation } from './webgl/WebGLAnimation.js';
Expand Down Expand Up @@ -205,7 +204,6 @@ class WebGLRenderer {

const _projScreenMatrix = new Matrix4();

const _vector2 = new Vector2();
const _vector3 = new Vector3();

const _emptyScene = { background: null, fog: null, environment: null, overrideMaterial: null, isScene: true };
Expand Down Expand Up @@ -1428,8 +1426,8 @@ class WebGLRenderer {

const transmissionRenderTarget = currentRenderState.state.transmissionRenderTarget;

_this.getDrawingBufferSize( _vector2 );
transmissionRenderTarget.setSize( _vector2.x, _vector2.y );
const activeViewport = camera.viewport || _currentViewport;
transmissionRenderTarget.setSize( activeViewport.z, activeViewport.w );

//

Expand All @@ -1447,6 +1445,11 @@ class WebGLRenderer {
const currentToneMapping = _this.toneMapping;
_this.toneMapping = NoToneMapping;

// Remove viewport from camera to avoid nested render calls resetting viewport to it (e.g Reflector).
// Transmission render pass requires viewport to match the transmissionRenderTarget.
const currentCameraViewport = camera.viewport;
Copy link
Collaborator

@Mugen87 Mugen87 Apr 8, 2024 •
edited

Choose a reason for hiding this comment

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

It's good that you added a comment that explains this bit 👍 .

Maybe we can find a different XR viewport handling in the future that avoids the viewport reset in Reflector in the first place.

Copy link
Collaborator

@Mugen87 Mugen87 Apr 11, 2024 •
edited

Choose a reason for hiding this comment

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

@mrxz I understand it makes sense to use the viewport to define the size of the render target. However, is the temporary removal really necessary?

What can be irritating is that the viewport can be defined via render targets and camera.viewport. When a reflector is used in a XR scene without transmissive objects, the viewport isn't correct after restoring the XR render target. camera.viewport holds the correct values. So after renderer.setRenderTarget( currentRenderTarget ); at the end of onBeforeRender(), the reflector restores the viewport of the sub camera.

In context of transmission, the transmission render target should now have the same size as camera.viewport. So is the temporary removal of camera.viewport really necessary? When debugging the latest changes today, it seems to me these lines are redundant. If not, can you please explain what bit I am missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In context of transmission, the transmission render target should now have the same size as camera.viewport. So is the temporary removal of camera.viewport really necessary?

@Mugen87 The camera.viewport holds both the size and offset. It's the offset that trips up the transmission rendering. The Reflector first restores the transmission render target, at which point the viewport is also (temporarily) correct. Directly afterwards it notices the camera.viewport and "restores" the viewport. For the left eye this should still all line up, but for the right eye the offset basically lies entirely outside of the bounds of the transmission render target.

In the webxr_vr_sandbox example it's quite difficult to see the difference. Since the viewport is only incorrect after the Reflector is rendered in the right eye, the transmission render target for the right eye will already have the background rendered into it, so it doesn't look completely off.

Hope this clarifies it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! That helped quite a lot!

if ( camera.viewport !== undefined ) camera.viewport = undefined;

renderObjects( opaqueObjects, scene, camera );

textures.updateMultisampleRenderTarget( transmissionRenderTarget );
Expand Down Expand Up @@ -1492,6 +1495,8 @@ class WebGLRenderer {

_this.setClearColor( _currentClearColor, _currentClearAlpha );

if ( currentCameraViewport !== undefined ) camera.viewport = currentCameraViewport;

_this.toneMapping = currentToneMapping;

}
Expand Down