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: Improve offscreen rendering in WebXR. #26160

Merged
merged 15 commits into from
Oct 6, 2023

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented May 28, 2023

Fixed #21188, #18846
Related issue: #8146

Description

Binds renderer.setRenderTarget(null) to renderer.setRenderTarget(_layerRenderTarget) in WebXR. This fixes scenes which render offscreen and composite like post-processing, GPGPU (e.g. LUTs), and volumes. This can be worked around by calling renderer.getRenderTarget at the start of each frame and binding to that instead which is effectively what this PR does.

I've also updated fixes from #18846 for the new layers API so renderer.getDrawingBufferSize and renderer.getSize will report correctly in WebXR, resolving #21188 (cc @gkjohnson). Note that renderer.getPixelRatio will always report 1 regardless of native scaling factor (as implemented).

This demo works around the above issues, rendering a grid to a render target and blitting it: https://codesandbox.io/s/mdhg6s

Before, that would be:
const handleResize = () => {
  renderer.setSize(window.innerWidth, window.innerHeight)
  renderTarget.setSize(window.innerWidth, window.innerHeight)
  camera.aspect = window.innerWidth / window.innerHeight
  camera.updateProjectionMatrix()
}
handleResize()
window.addEventListener('resize', handleResize)
renderer.xr.addEventListener('sessionend', handleResize)

// https://github.com/mrdoob/three.js/issues/21188
const resizeWithBaseLayer = () => {
  const layer = renderer.xr.getBaseLayer()
  const width = layer.textureWidth ?? layer.framebufferWidth
  const height = layer.textureHeight ?? layer.framebufferHeight
  renderTarget.setSize(width, height)
}
renderer.xr.addEventListener('sessionstart', resizeWithBaseLayer)

renderer.setAnimationLoop(() => {
  controls.update()

  // In the DOM, gl.bindFramebuffer(gl.FRAMEBUFFER, null) binds
  // to the canvas, but in WebXR we render straight to a layer.
  //
  // Three.js creates an internal render target, so calling
  // renderer.setRenderTarget(null) will not render to screen
  // in WebXR.
  //
  // Layers will also have a different size, so mirror the
  // render target's dimensions which correspond to the layer.
  const _surface = renderer.getRenderTarget()

  renderer.setRenderTarget(renderTarget)
  renderer.render(scene, camera)

  // Disable XR projection for fullscreen effects
  const xrEnabled = renderer.xr.enabled
  renderer.xr.enabled = false
  renderer.setRenderTarget(_surface)
  renderer.render(effect, camera)
  renderer.xr.enabled = xrEnabled
})
Now, that would be:
const size = new THREE.Vector2()

const handleResize = () => {
  renderer.setSize(window.innerWidth, window.innerHeight)
  renderer.getDrawingBufferSize(size)
  renderTarget.setSize(size.width, size.height)
  camera.aspect = window.innerWidth / window.innerHeight
  camera.updateProjectionMatrix()
}
handleResize()
window.addEventListener('resize', handleResize)
renderer.xr.addEventListener('sessionstart', handleResize)
renderer.xr.addEventListener('sessionend', handleResize)

renderer.setAnimationLoop(() => {
  controls.update()

  renderer.setRenderTarget(renderTarget)
  renderer.render(scene, camera)

  // Disable XR projection for fullscreen effects
  const xrEnabled = renderer.xr.enabled
  renderer.xr.enabled = false
  renderer.setRenderTarget(null)
  renderer.render(effect, camera)
  renderer.xr.enabled = xrEnabled
})

Regarding examples and EffectComposer, I've since mirrored the above demo -- resizing with the base layer and disabling XR projection for fullscreen effects via FullScreenQuad.

I have a branch based on this one which enables WebXR for all of the postprocessing examples (diff) which you can try at https://rawcdn.githack.com/CodyJasonBennett/three.js/c13f9091360c8eda4c198f164d2c63eb1b3986a0/examples/?q=postprocessing#webgl_postprocessing. Notably, (S)SAO/Background don't work because of CustomBlending and unexpected clearing with WebGLBackground from the XR environment blend mode (fixed since #26428).

Show examples:

Below are taken with an emulator, but this PR ensures it also works on-device.

Example Preview
webgl_postprocessing webgl_postprocessing
webgl_postprocessing_3dlut webgl_postprocessing_3dlut
webgl_postprocessing_afterimage webgl_postprocessing_afterimage
webgl_postprocessing_dof webgl_postprocessing_dof
webgl_postprocessing_fxaa webgl_postprocessing_fxaa
webgl_postprocessing_glitch webgl_postprocessing_glitch
webgl_postprocessing_rgb_halftone webgl_postprocessing_rgb_halftone
webgl_postprocessing_sobel webgl_postprocessing_sobel
webgl_postprocessing_unreal_bloom webgl_postprocessing_unreal_bloom
webgl_postprocessing_unreal_bloom_selective webgl_postprocessing_unreal_bloom_selective

WRT previous concerns for performance, I haven't opted for adding any external examples for pmndrs/postprocessing since #18846 (comment), but I have tested against this PR's builds with the library against a few R3F examples that use it (e.g. https://codesandbox.io/s/7c35ym). I'd recommend this library since it has very performant effects that I've been able to run on low end mobile hardware, and they're modular enough to mix with three's if you really want to. Notable user-land integrations include 0beqz/realism-effects and N8python/n8ao.

@github-actions
Copy link

github-actions bot commented May 28, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
649.2 kB (160.9 kB) 649.5 kB (161 kB) +351 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
442.5 kB (107.1 kB) 442.8 kB (107.1 kB) +351 B

@CodyJasonBennett CodyJasonBennett changed the title WebGLRenderer: bind to base layer instead of canvas in WebXR WebGLRenderer: improve offscreen rendering in WebXR May 29, 2023
Copy link
Contributor

@cabanier cabanier left a comment

Choose a reason for hiding this comment

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

This looks like a good change but it would be nice to see how it performs on mobile hardware.
Can you also add a vr example that invokes these new codepaths? (or is there one already?)

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented May 29, 2023

This PR shouldn't have any performance implications since it doesn't touch any hot paths. Are you concerned with post-postprocessing end-to-end or something more implementation specific with this PR?

I tested the unreal bloom example which you can try at https://raw.githack.com/CodyJasonBennett/three.js/test/xr-bloom/examples/webgl_postprocessing_unreal_bloom.html. Not all effects work like the pixel pass, but I don't plan on addressing them here but separately since this PR contains fixes I'd like to not block.

In user-land, I'd recommend pmndrs/postprocessing since it has very performant effects, but I haven't been able to get a stable enough implementation over WebXR in three.js prior.

@cabanier
Copy link
Contributor

This PR shouldn't have any performance implications since it doesn't touch any hot paths. Are you concerned with post-postprocessing end-to-end or something more implementation specific with this PR?

I am worried about introducing flushes because they will impact performance and introduce visual glitches (ie "snow"). If postprocessing could discard depth or even not create a resolve, that would be better.

I tested the unreal bloom example which you can try at https://raw.githack.com/CodyJasonBennett/three.js/test/xr-bloom/examples/webgl_postprocessing_unreal_bloom.html. Not all effects work like the pixel pass, but I don't plan on addressing them here but separately since this PR contains fixes I'd like to not block.

Could you create an example based on that?

@CodyJasonBennett
Copy link
Contributor Author

If you're referring to internal disposal from resizing effects/render targets in EffectComposer, I can split that into a separate PR. I don't want that to block this PR behind further enhancements.

@cabanier
Copy link
Contributor

If you're referring to internal disposal from resizing effects/render targets in EffectComposer, I can split that into a separate PR. I don't want that to block this PR behind further enhancements.

No, just a simple example that uses these new code paths. That way I can make sure that nothing is broken in our PRs for multiview and our work for spacewarp and timewarp layers.

@CodyJasonBennett
Copy link
Contributor Author

I've added a minimal demo webxr_vr_postprocessing that uses EffectComposer, UnrealBloom, and the new OutputPass. My PR description has a demo that performs a simple blit via workarounds if you want something smaller and without this PR.

@PhilipZhu
Copy link

PhilipZhu commented May 30, 2023

I tried to change parameters from the control panels in https://raw.githack.com/CodyJasonBennett/three.js/test/xr-bloom/examples/webgl_postprocessing_unreal_bloom.html, and regular rendering from the browser window and XR rendering doesn't match. This is true for both the chrome emulator extension and Quest2
Regular broswer window:
image
VR render:
image

@marcofugaro
Copy link
Contributor

really useful PR! would love to see it merged

@cabanier
Copy link
Contributor

cabanier commented Oct 6, 2023

I don't understand why this should be addressed anywhere else than Quest Browser.

This is unfortunately nothing that the browser can fix. It's a side effect of how mobile/tile based GPUs are designed.

Is gl.bindFramebuffer expected to be destructive?

Yes :-\

The number of calls doesn't change with this PR, only the target. Maybe this can be reverted and instead done by exposing what's currently WebXRManager._getRenderTarget()? It could be instead bound by EffectComposer or OutputPass when rendering to screen if that gives any behavioral benefit.

There are definitely ways to solve post-processing with careful coding and using layers (especially on Quest 3). They require deeper changes to three though.

@CodyJasonBennett
Copy link
Contributor Author

I just noticed there's a depth/stencil attachment configured in WebXRManager. That would explain your comments. Thanks.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 6, 2023

Should we still merge #26902 or revert for now?

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Oct 6, 2023

#26902 reverts the changes to WebGLRenderer with an alternative solution for postprocessing. I believe that addresses this issue, but would appreciate device testing to confirm. I'm also happy to pick in any lost changes to examples and such if you'd prefer to simply revert.

@cabanier
Copy link
Contributor

cabanier commented Oct 6, 2023

#26902 reverts the changes to WebGLRenderer with an alternative solution for postprocessing. I believe that addresses this issue, but would appreciate device testing to confirm.

I applied your change locally. It didn't seem to make a difference.

Mugen87 added a commit to Mugen87/three.js that referenced this pull request Oct 6, 2023
@CodyJasonBennett
Copy link
Contributor Author

I'm at a loss of what's actually happening then. I've implemented many post-processing systems that worked well as low as Pixel 3a, notably via mipmaps in a similar fashion such as MipmapBlur out of pmndrs/postprocessing. This has included bloom, SSAO, and full volumetric effects. @RenaudRohlinger may be inclined to demo such examples, but I have no direction as for how to continue.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 6, 2023

Thanks for testing! Okay then let's revert for now.

@CodyJasonBennett CodyJasonBennett deleted the fix/offscreen-webxr branch October 6, 2023 17:28
Mugen87 added a commit that referenced this pull request Oct 6, 2023
* Revert "Examples: Clean up. (#26901)"

This reverts commit 306fb93.

* Revert "Updated builds."

This reverts commit 7aba3d2.

* Revert "WebGLRenderer: Improve offscreen rendering in WebXR. (#26160)"

This reverts commit afdfa33.
@cabanier
Copy link
Contributor

cabanier commented Oct 6, 2023

I'm at a loss of what's actually happening then. I've implemented many post-processing systems that worked well as low as Pixel 3a, notably via mipmaps in a similar fashion such as MipmapBlur out of pmndrs/postprocessing. This has included bloom, SSAO, and full volumetric effects. @RenaudRohlinger may be inclined to demo such examples, but I have no direction as for how to continue.

I'm sorry to be the bearer of bad news. :-
Stereo foveated headsets require more careful coding than mono screens that are not as sensitive to performance.
It IS definitely possible by re-arranging how things are rendered so you only bind once to the main framebuffer, by discarding depth when needed and by doing postprocessing that doesn't require a resolve.

@CodyJasonBennett
Copy link
Contributor Author

I thought #26902 would help in that regard, but I'm happy to help if there are other means you find more appropriate.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Nov 3, 2023

I'd like to revisit this. I don't think that pinning performance issues with tiled GPUs and multiple passes is fair to this PR nor the issues it addresses outside of postprocessing and incorrect render state. That's not inherent to this PR, but surrounding usage and implementation. I've also demonstrated how performant effects for mobile exist in the ecosystem and not strictly three.js examples which both rely on changes to core. This PR doesn't affect existing apps, but ensures a valid drawing target is bound which has regressed since the introduction of layers. Performance characteristics do not change this way. This should be considered a fix, and I'm happy to remove changes to EffectComposer if that's the only way for this to be considered as such, but I believe that would be largely an oversight.

@cabanier
Copy link
Contributor

cabanier commented Nov 7, 2023

This PR doesn't affect existing apps, but ensures a valid drawing target is bound which has regressed since the introduction of layers.

I don't understand why the introduction of layers affects this. If anything, it makes it easier to work around the rebinding. Since almost all of VR uses tiled GPUs, I don't think we should introduce postprocessing unless it can be made performant on mobile platforms.

@CodyJasonBennett
Copy link
Contributor Author

I don't understand why the introduction of layers affects this. If anything, it makes it easier to work around the rebinding.

I apologize if I'm completely wrong here. I'm going off of memory from https://twitter.com/Cody_J_Bennett/status/1482585611781480448 which I recall working in the old emulator but not on-device. Same observation prompted this PR when I noticed rendering was to the canvas instead of a layer.

Since almost all of VR uses tiled GPUs, I don't think we should introduce postprocessing unless it can be made performant on mobile platforms.

I'm confused as to why we don't give people the agency to even prove otherwise, let alone allow broken rendering state as-is for other cases. I've already demonstrated numerous times this can be done with pmndrs/postprocessing and other linked related work. I also stress that this PR isn't strictly an enhancement to enable postprocessing, but to ensure that renderer.setRenderTarget (at any frequency) does not break further rendering. I've tried to reduce the changes here to entertain this case only, but I believe that to be a disservice to the ecosystem.

@cabanier
Copy link
Contributor

cabanier commented Nov 7, 2023

sorry that I sounded a bit blunt.
I too want to have post-processing support in WebXR. However, the hard part is how to make it performant and I hope that we can make changes to three to make it so.
We likely have to be smart about when we render and introduce multiview. We can also wait for WebGPU which will give us more control over the render pipeline so we don't have to be as careful.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Nov 7, 2023

I have to repeat my last comment if you insist on performance, but I will not let this PR be a casualty of multiview. If the order of operations matters for its implementation, then it alone should wait for WebGPU.

@cabanier
Copy link
Contributor

cabanier commented Nov 7, 2023

You don't need multiview; it would just make implementing usable postprocessing easier...

@CodyJasonBennett
Copy link
Contributor Author

Unless this introduces a regression for new or existing code, I still don't see a reason to shoot this down. I'm not going to entertain the dialogue around post-processing + mobile since it's both provably untrue (see any documentation or real-world usage attached to this PR) and besides the scope of this PR which I have to stress is wider than strictly that use-case. I'm not happy I have to bend over backwards to reiterate that point specifically, and it's not been particularly regarded at any point despite me mentioning it in at every point.

@BryanChrisBrown
Copy link

I'd like to chime in here, this PR would fix a handful of rendering issues we've seen in three.js when used with the Looking Glass WebXR library, is it possible that the oculus multiview requirements could be split into a separate PR or worked on separately from this PR?

My understanding is that post processing doesn't work in WebXR at all right now, and I think making progress towards more powerful devices, desktopVR, Looking Glass etc. would be helpful.

@BryanChrisBrown
Copy link

Any chance we can get this PR revisited @cabanier?

Do you think it’s possible to merge ignoring the performance on meta quest systems?

@cabanier
Copy link
Contributor

cabanier commented Jun 16, 2024

Any chance we can get this PR revisited @cabanier?

Do you think it’s possible to merge ignoring the performance on meta quest systems?

It’s not just quest. It’s any xr device that is based on a mobile gpu.
This code can barely run a single triangle at acceptable performance. As I mentioned before, this will only be possible if this patch is rewritten to avoid flushes. Some work recently landed to enable this for reflective surfaces so it’s definitely doable.

@CodyJasonBennett
Copy link
Contributor Author

Just a reminder this is what we're talking about. Issues with sizing I split into #26905 after this was reverted, and this is more impactful than just post-processing, but preventing setRenderTarget from creating a bad state (which only user-land code calls).

I've outlined workarounds in the description which aren't as exotic since upstreaming other fixes for those adventurous.

this.setRenderTarget = function ( renderTarget, activeCubeFace = 0, activeMipmapLevel = 0 ) {

+    // Render to base layer instead of canvas in WebXR
+    if ( renderTarget === null && this.xr.isPresenting ) {
+
+        renderTarget = this.xr._getRenderTarget();
+
+    }

I'd like to put in a $1000 bounty to implement this proper without stepping on other work from Meta, since I'm assuming that's part of the concern here. If this is a multi-step piece of work, then I'm happy to continue in an issue and compensate accordingly.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Jun 16, 2024

Also, reminder we can implement this today which doesn't affect existing code whatsoever, and later implement something more applicable to Quest and future improvements there. IMO that's the correct way of doing this, since we're gatekeeping WebXR as a whole, and much more than just post-processing. Hence the bounty since we have to brute force through this otherwise.

@RektTillNoon
Copy link

Is there anyone actively working on this? Where does this PR stand in terms of priority for the team?

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.

WebXR: Provide way to get XR viewport resolution when presenting
9 participants