Reflector/Refractor: Set .encoding to match renderer.outputEncoding

[WestLangley]

This PR is based on an idea suggested by @PlumCantaloupe in #19665.

After some experimentation, I think this implementation works well-enough for now, and is also an improvement since renderer.outputEncoding is no longer required to be linear when using reflectors.

There is no performance issue.

[WestLangley]

It looks a bit different than master because renderer.outputEncoding has changed and so has tone mapping.

https://raw.githack.com/westlangley/three.js/dev_reflector_encoding/examples/webgl_mirror.html

[mrdoob]

Something I tried while messing with this was adding this line here:

renderTarget.texture.encoding = renderer.outputEncoding;

I think this worked for Reflector and Refractor, but not for Water.

[Mugen87]

I think this worked for Reflector and Refractor, but not for Water.

I've added your supposed fix for Reflector, Refractor and Water.

Both Water and Water2 (which relies on Reflector, Refractor) actually seem to work without showing performance issues.

[PlumCantaloupe]

Just FYI, noticed something a bit strange. When using the line:

material.onBeforeCompile = function ( shader, renderer ) {
    this.uniforms[ "tDiffuse" ].value.encoding = renderer.outputEncoding;
}

in a non-build A-Frame use-case (just using within basic script tags) there are a bunch of errors and it doesn't render properly (see image below). West's solution works for me in my own build system - just not here where I am trying to debug a camera bug on the Quest :) Seems as if the shaders don't exist yet ... If I comment out that code it works fine though (I did add the old change from #19665 just in case).

See example here ( Glitch.com code ), and note West's code at lines 79-82 on index.html

[WestLangley]

Another option to consider is to add encoding as a valid option to the Reflector constructor. Users would be advised to set it to match renderer.outputEncoding for now.

But the current requirement of forcing the renderer to render to linear space is not acceptable.

[PlumCantaloupe]

Yeah, that is a good point. No need to pass the entire renderer object.

EDIT: though one nice thing about having the renderer is having a more explicit"foolproof" way to make sure the correct encoding is set.

[Mugen87]

Another option to consider is to add encoding as a valid option to the Reflector constructor.

This was the previous approach introduced in #18709.

@mrdoob I think it's better to revert #19618 as suggested and start looking for another solution based from the previous code.

[WestLangley]

EDIT: though one nice thing about having the renderer is having a more explicit"foolproof" way to make sure the correct encoding is set.

What would be correct is for the reflector to render to a floating point render target in linear space - without applying tone mapping. But because of a design limitation and related performance issues, we can't do that.

I am reluctant to refer to what we are doing now as "correct". Instead, we are trying to make the best of a difficult situation.

[Mugen87]

I am reluctant to refer to what we are doing now as "correct". Instead, we are trying to make the best of a difficult situation.

Yeah, this is a really tricky topic.

[mrdoob]

Okay... "Fixed" Reflector and Refractor: 34fe461

These are "easy" because there is no shading, the pixels the reflection camera see are ready to present.

Water, however, needs to render in linear color space because its shader has light support...

[Mugen87]

Water, however, needs to render in linear color space because its shader has light support...

Noted this in the migration guide: https://github.com/mrdoob/three.js/wiki/Migration-Guide#r117--r118

[mrdoob]

Noted this in the migration guide: https://github.com/mrdoob/three.js/wiki/Migration-Guide#r117--r118

Thank you! This is a temporary situation, hopefully...

[mrdoob]

Closing this for now.

[PlumCantaloupe]

Hello again, sorry to resurrect this. I wasn't sure it should be a new issue ...

I started exploring this encoding issue when this mirror doesn't work on the Oculus Quest in A-Frame (Oculus Browser and Firefox Reality- aframevr/aframe#4629 ), but have found some additional issues. I have recreated the cubes WebXR r118.dev example and added a mirror here: https://glitch.com/~pure-threejs-mirrortest which leads to following strange behaviours (only after entering VR mode interestingly):

• object parented to the camera does not move in the mirror.
• object sorting appears to be broken on the mirror.
• I haven't tested this example on the Oculus Quest using the link and Firefox Desktop, but as mentioned in the A-Frame issue linked above, I imagine it is fine as those mirror examples were.

Not sure if there is a workaround here, or if it is a standalone browser performance optimization recently introduced that breaks this?

[mrdoob]

object sorting appears to be broken on the mirror.

Fascinating. I can reproduce this in the Oculus Browser, but I can't reproduce when using the WebXR Emulator 🤔

It's unrelated to encoding though. Do you mind creating a new issue?

[PlumCantaloupe]

Just added. Thank you for taking a peek :)