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

Clean up WebXR manager by calling setRenderTarget #22558

Merged
merged 14 commits into from
Nov 25, 2021
Merged

Clean up WebXR manager by calling setRenderTarget #22558

merged 14 commits into from
Nov 25, 2021

Conversation

cabanier
Copy link
Contributor

This change changes WebXR so it calls setRenderTarget instead of binding to the framebuffer directly.
This will fix a couple of issues.

In addition, it introduces the ext_multisampled_render_to_texture extension and uses that instead of using renderbuffers.

This contribution is funded by Oculus.

@cabanier
Copy link
Contributor Author

cabanier commented Sep 22, 2021

This change ended up being much larger than I anticipated.
It adds the following to:

  • Add support for multisampled-render-to-texture. This extension will be used if the render target is multisampled and the browser supports it. If WebXR provides an depth texture, this extension will be disabled for the WebXR render targer and will fall back to regular multisample.
  • Enhanced setRenderTarget so WebXR can pass in its textures or framebuffer. Passing in a texture pr framebuffer will signal to three.js that it doesn't to allocate them and use the provided ones instead.
  • More efficient processing when blitting multisampled data. Previously the depth data was always blitted, even if it wasn't used. We now also discard the msaa data after copying.
  • Removed the old WebXR workarounds in the state.
  • Ensured that the correct depth buffer types are requested. (There were some mismatches that triggered internal copies)
  • Set foveation to the maximum value.

I tried out all the vr (and some non-vr) examples with:

  • vanilla WebXR
  • WebXR Layers without AA
  • WebXR Layers with multisampled buffers and a provided WebXR depth texture (the default)
  • WebXR Layers with multisampled buffers and an implied WebXR depth buffer
  • WebXR Layers with multisampled-render-to-texture and a provided WebXR depth texture
  • WebXR Layers with multisampled-render-to-texture and an implied WebXR depth buffer (upcoming default for oculus)

Performance of WebXR Layers with multisampled-render-to-texture and an implied WebXR depth buffer is on par or slightly better than the origin vanilla WebXR implementation.


// The multisample_render_to_texture extension doesn't work properly if there
// are midframe flushes and an external depth buffer. Disable use of the extension.
if ( renderTarget.useMultisampleRenderToTexture ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth adding a log/warning here. How likely is it that you would end up in this state without user error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not likely, but the author can force a texture if they wanted to.
I'll add a warning.

@cabanier
Copy link
Contributor Author

@Mugen87 can you also take a look?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 24, 2021

Yes! I'll try to review the changes in the next days 👍 .

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 24, 2021

Is EXT_multisampled_render_to_texture a WebGL 2 only extension? If so, the following line should be moved to the above if branch:

getExtension( 'EXT_multisampled_render_to_texture' );

@cabanier
Copy link
Contributor Author

@Mugen87 I think I addressed your comments. Do you have any suggestions on how to best test this?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 25, 2021

Um, it's probably best to merge PRs which apply deep changes to the renderer at the beginning of a month. In this way, we have the complete dev cycle for more detailed testing.

It's definitely a good sign that the E2E tests all pass (although they do no cover all use cases). I've also tested selected examples manually today by downloading your branch. Everything seems to work fine. However, having more time for verification is of course preferable.

@cabanier
Copy link
Contributor Author

Um, it's probably best to merge PRs which apply deep changes to the renderer at the beginning of a month. In this way, we have the complete dev cycle for more detailed testing.

That's OK :-)
There's a known rendering issue with the current state of the code in three. Should I provide a temporary workaround for it?

It's definitely a good sign that the E2E tests all pass (although they do no cover all use cases). I've also tested selected examples manually today by downloading your branch. Everything seems to work fine. However, having more time for verification is of course preferable.

Thanks! If you know of more examples, I would love for one of us to try them out.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 26, 2021

There's a known rendering issue with the current state of the code in three.

Do you mind explaining in more detail the rendering issue? I've not seen something unusual when testing the branch.

@cabanier
Copy link
Contributor Author

There's a known rendering issue with the current state of the code in three.

Do you mind explaining in more detail the rendering issue? I've not seen something unusual when testing the branch.

The version that is currently checked in uses the ext_multiview_to_texture extension. It allows efficient multisampled code but it suffers from the drawback that a certain order of graphics calls cause the depth buffer to be discarded.
This problem will only show up if this extension is supported which is just the oculus browser for now.

We can revert the PR that introduced this extension or integrate this PR which works around that issue.

@mrdoob mrdoob added this to the r134 milestone Oct 4, 2021
@cabanier
Copy link
Contributor Author

I'm still worried that this is changing colors from the old non-layers based WebXR.
Can we verify that this doesn't affect the rendering of non-quest VR browsers?

@elalish
Copy link
Contributor

elalish commented Dec 10, 2021

I think my fix is pretty safe; it's just making the layers and non-layers code paths have the same behavior: #22997

@arpu
Copy link

arpu commented Dec 10, 2021

Okay, I can see that indeed the problem is that when entering WebXR mode the shader is recompiled because the output encoding changes from Gamma to Linear. The reason it changes is that the WebXR render target is set to sRGB, but this line sets the texture encoding to Linear: https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLPrograms.js#L95

Then this value is picked up by the renderer as the output encoding, but this is apparently incorrect, as the WebXR renderbuffer is exactly like the default renderbuffer and input values need to be converted into sRGB; it's not done for you. I think Renderbuffers need to use different logic for their sRGB encoding than textures use. What do you think @Mugen87?

tested your Patch on oculus quest2 browser with disabled webxr Layers, initial Loaded Textures looks normal now, but new created Textures in webxr session still darker looks like this needs some redefine?

@elalish
Copy link
Contributor

elalish commented Dec 10, 2021

@arpu I assume that's a separate (but probably related) issue. I can't repro it, so I'll let you explore.

@arpu
Copy link

arpu commented Dec 10, 2021

if i remove this line https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLPrograms.js#L95 it works in webxr view

maybe this needs be sRGB in a webxrSession

@elalish
Copy link
Contributor

elalish commented Dec 10, 2021

It might have to do with how the textures are being created. I thought that line was my problem too originally, but it turned out the fix was elsewhere. Do you have a simple repro URL? And does it also repro on mobile AR?

@arpu
Copy link

arpu commented Dec 10, 2021

will test again on next dev build update with your Patch on https://raw.githack.com/mrdoob/three.js/dev/examples/?q=webxr#webxr_vr_ballshooter

@arpu
Copy link

arpu commented Dec 17, 2021

@elalish have reproducible simple example at https://raw.githack.com/vrlandio/three.js/vrlandio/0.136.0.vr2/examples/?q=webxr#webxr_vr_ballshooter ( on Controller select it create a new Geometry and new Material for spawn the Ball)

Bildschirmfoto von 2021-12-17 18-36-55

i can reproduce this with chrome and the WebXR API Emulator and on Oculus Quest 2 OculusBrowser with enabled/disabled Layers

@cabanier
Copy link
Contributor Author

Okay, I can see that indeed the problem is that when entering WebXR mode the shader is recompiled because the output encoding changes from Gamma to Linear.

As I mentioned in the PR, if we change the texture format to be sRGB, we also need to communicate that to the browser when we create the projection layer. Otherwise, the multisampled path that uses renderbuffers is broken.

I also think we should make this lighter rendering an option as it produces output that doesn't match the 2D representation; at least on quest.

@mrdoob
Copy link
Owner

mrdoob commented Dec 17, 2021

The render inside VR should be identical to the one outside VR (as it always has been).

@arpu
Copy link

arpu commented Dec 30, 2021

i can still reproduce this with https://raw.githack.com/vrlandio/three.js/vrlandio/0.136.0.vr2/examples/?q=webxr#webxr_vr_ballshooter ( all created balls in the webxr session are darker)

and some side note, if i enable webxr navigation in OculusBrowser and transform in webxr to a new Scene all texture are darker

@mrdoob
Copy link
Owner

mrdoob commented Dec 30, 2021

@arpu Can't repro. Do you mind recording a video?

@cabanier
Copy link
Contributor Author

cabanier commented Jan 2, 2022

@arpu This user reports that r136 fixed their color issue. Does it still happen for you?

@arpu
Copy link

arpu commented Jan 3, 2022

@cabanier yes the example use three.js r136, with r136 all initial loaded bevore enter the webxr session looks good but not the created in the webxr Session, but looks like @Mugen87 refactor the decoding #23129 (comment), better wait to investigate after this

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 3, 2022

I can't imagine how the removal of the sRGB inline decode should affect WebXR scenes. I think your issue needs to be investigated/solved separately.

@hybridherbst
Copy link
Contributor

hybridherbst commented Jan 3, 2022

I can confirm that we're also seeing dark/wrong colorspace WebXR rendering with r136, both on Desktop (via Quest + Link) and Oculus Browser on Quest.

@mrdoob
Copy link
Owner

mrdoob commented Jan 3, 2022

@hybridherbst Can you share a screen recording from inside the Quest?

@hybridherbst
Copy link
Contributor

@mrdoob

In Browser (no XR):
chrome_PV4DPUHI09

In XR (Quest via Link):
OculusMirror_N1ZURGjrY5

It seems to be related to using render textures (doesn't happen when not rendering into any); will investigate further and open a new issue if it's not just a mistake with using RTs on our end.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 4, 2022

Do you mind checking if the latest dev build solves the issue?

https://github.com/mrdoob/three.js/tree/dev/build

@hybridherbst
Copy link
Contributor

hybridherbst commented Jan 4, 2022

Still happens on latest builds.
Seems the issue happens when rendering into a MultiSampleRenderTarget by doing

// initialize RT
this.rtTexture = new THREE.WebGLMultisampleRenderTarget(256, 128);
...
// at any point in time, e.g. update loop
const xr = rend.xr.enabled;
rend.xr.enabled = false;
const rt = rend.getRenderTarget();
rend.setRenderTarget(this.rtTexture);
rend.render(this.rtScene, currentCamera);
rend.setRenderTarget(rt);
rend.xr.enabled = xr;

Before, this wasn't causing any darkening and both the RT and the XR result were correct;
now it seems that the linear encoding of the RT "spoils" the renderer.

Creating the RT with sRGBEncoding fixes the issue – I'm not sure but I think the rendering result shouldn't change when rendering into a RT and then restoring state.

// no darkening anymore
this.rtTexture = new THREE.WebGLMultisampleRenderTarget(256, 128, { encoding: THREE.sRGBEncoding });

@cabanier
Copy link
Contributor Author

cabanier commented Jan 4, 2022

@cabanier yes the example use three.js r136, with r136 all initial loaded bevore enter the webxr session looks good but not the created in the webxr Session, but looks like @Mugen87 refactor the decoding #23129 (comment), better wait to investigate after this

I applied your change to the latest version of three.js and the balls look identical.
Can you try the same?

@arpu
Copy link

arpu commented Jan 4, 2022

@cabanier latest master threejs version? 91e4d29

@cabanier
Copy link
Contributor Author

cabanier commented Jan 4, 2022

@cabanier latest master threejs version? 91e4d29

Latest dev version. My top commit is 9daf001

@arpu
Copy link

arpu commented Jan 4, 2022

@cabanier Yes this fixed the Problem with loaded Materials looks darker! Awesome

@cabanier
Copy link
Contributor Author

cabanier commented Jan 4, 2022

@cabanier Yes this fixed the Problem with loaded Materials looks darker! Awesome

When you say "this fixed the problem", what change are you referring to?

@arpu
Copy link

arpu commented Jan 4, 2022

not sure tested latest dev version 0311f8f

@gonnavis
Copy link
Contributor

gonnavis commented Jan 19, 2022

I couldn't think of a way to avoid the new (technical) properties of WebGLMultisampleRenderTarget. Maybe we can figure this out at a later point (e.g. with a new subclass of WebGLMultisampleRenderTarget).

I'm really facing severe glitch effect ( after upgrade from r134 to r135/136 ) when using WebGLMultisampleRenderTarget with EffectComposer.
If delete this line of code this.useRenderbuffer = this.useRenderToTexture === false; in three.module.js ( r135/136 ), or using default WebGLRenderTarget will ok.
And feel it's strange that put these logics in and only in WebGLMultisampleRenderTarget.js:

    this.ignoreDepthForMultisampleCopy = options.ignoreDepth !== undefined ? options.ignoreDepth : true;
    this.useRenderToTexture = ( options.useRenderToTexture !== undefined ) ? options.useRenderToTexture : false;
    this.useRenderbuffer = this.useRenderToTexture === false;

I'll try to make a small example.

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.

8 participants