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: Let transmissionRenderTarget size match relevant viewport size #28088

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Apr 8, 2024

Related issue: #28073 (split off from #28078)

Description

This PR ensures that the transmissionRenderTarget dimensions match the current viewport size (or would-be viewport in case of WebXR). As a result the render target size matches that of each eye, instead of the full WebXR framebuffer size. Similarly when rendering to a render target, it will match its size instead of the full drawing buffer size.

There is one additional fix for WebXR with nested render calls (e.g. when using a Reflector). The Reflector tries to "restore" the viewport when in a WebXR session (Reflector.js#L164-L172), but when rendering the transmission pass, the viewport should be left unchanged (matching the transmission render target)

This contribution is funded by Fern Solutions

Copy link

github-actions bot commented Apr 8, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
672.7 kB (166.7 kB) 672.7 kB (166.7 kB) +41 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
452.4 kB (109.2 kB) 452.5 kB (109.2 kB) +64 B

@Mugen87 Mugen87 added this to the r164 milestone Apr 8, 2024
@@ -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

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

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!

@Mugen87 Mugen87 merged commit 144e95b into mrdoob:dev Apr 9, 2024
12 checks passed
@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 10, 2024

Tested the PR on a Quest to with dev version of webxr_vr_sandbox. The change alone doubles the FPS from 8 to 16. The example is very taxing on purpose so the low frame rate does not surprise but granted there is still room for improvements^^.

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.

2 participants