Three.js: Problem with Reflector affecting lighting when its texture is rendered.

[ChrisDenham]

Description of the problem

I have a problem with changes to lighting when Reflector object is used.

I have distilled the problem down here:
https://jsfiddle.net/cmd4/9zLhq9Lf/
You should see that the lighting on the walls changes whenever you rotate the view, as the mirror changes between facing towards and away from the camera.

Note that to show the problem, it seems to require adding the Reflector object before the first render, and the rest of the scene after the first render. i.e. If I create the whole scene before the first render, then I don't seem to get any issues.

My guess is that the problem may be related to setting up GL state for the point light when rendered for both the main camera and the "virtual" camera created by the Reflector class.

Maybe in this function below, but here I am out of my depth:
(from https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLLights.js)

setup( lights, shadows, camera )
 {
...
    var viewMatrix = camera.matrixWorldInverse;
...
    } else if ( light.isSpotLight ) {
                     var uniforms = cache.get( light );
                     uniforms.position.setFromMatrixPosition( light.matrixWorld );
                     uniforms.position.applyMatrix4( viewMatrix ); <<<<HERE???
...

Three.js version

• [x] Master
• [x] r88

Browser

• [x] Chrome
• [x] Firefox
• [x] Edge

OS

• [x] Windows
• [ ] macOS
• [ ] Linux
• [x] Android
• [x] iOS

[WestLangley]

It is not clear to me what you are referring to...

Are you aware that the highlights on the walls are specular highlights, and have nothing to do with the mirror?

Change the wall material to MeshLambertMaterial and/or reduce the intensity of the spotlight.

[ChrisDenham]

I am referring to the sudden transition in the lighting on the walls as you rotate the view. Do you not see change as you rotate at the point where you are looking at the back of the mirror (and thus has no need to render the mirror texture)?

[ChrisDenham]

btw. yes, i agree the lighting on the walls should not be affected by the mirror.... but in this case it does. That’s the problem... the lighting on the walls looks incorrect when camera is facing the mirror.

[WestLangley]

OK, I see that.

Here is the pattern you are implementing:

scene.add( mirror );
renderer.render( scene, camera );
createMirrorTest(); // this function adds lights

You are changing the number of lights after the first render, so you need to recompile the mirror's shader.

mirror.material.needsUpdate = true; // you need to add this line

updated fiddle: https://jsfiddle.net/9zLhq9Lf/2/

[ChrisDenham]

Ah ha. Thanks very much. I didn't know that I had to force update of materials when i add lights. In general, do i need to force update of all the materials used in a scene if I add a light after a material is first rendered?

[WestLangley]

Yes. See the Materials section in How to Update Things in the docs.

[ChrisDenham]

Thanks again. Apologies for posting what turned out to be a newbie question into the issue tracker.

[ChrisDenham]

@WestLangley I'm still having trouble with this. When I change the starting camera view so that it is facing the back of the mirror, your call to mirror.material.needsUpdate = true; seems to no longer fix the problem. I updated the starting view from your fiddle here: https://jsfiddle.net/cmd4/9zLhq9Lf/3/
As you can see, the problem persists in this fiddle, but I can't see how to fix it.

[ChrisDenham]

In case it helps diagnose what's going on, I noticed that I can work around the problem if I comment out the line in Reflector.js that prevents it rendering to texture when the plane is facing away from the camera:
https://github.com/mrdoob/three.js/blob/6d9c1f752f3d2e371cfd05dc637dd990e035d9e2/examples/js/objects/Reflector.js#L90

[WestLangley]

OK, then this is a more fundamental issue. Thank you for pursuing this.

Lighting problems occur if the mirror is not in the camera view when the mirror is added to the scene.

This bug can be reproduced in webgl_mirror.html by initializing the camera position like so

camera.position.set( 0, 75, - 160 );

and then rotating the camera until all mirrors are in view.

/ping @Mugen87 @mrdoob

[Mugen87]

Um, i wonder how the reflector's material can influence the lighting of the entire scene 🤔?

For some reasons, the lights seem to be reflected along the reflection plane. It's easy to see this when you set the camera in webgl_mirror.html to this value.

camera.position.set( 0, - 75 , 160 );

Besides, comment out the addition of the vertical mirror.

// scene.add( verticalMirror );

[ChrisDenham]

Could the issue be related to light position uniform setup in the code I referred to in my first comment? If lights are positioned in camera space, it made me wonder if for some reason the light position was using the position for the wrong camera?
https://github.com/mrdoob/three.js/blob/14ed22d5b5422f11738016cef85d70927117851f/src/renderers/webgl/WebGLLights.js#L262
This is just a guess based on how the light position seeming to shift when the mirror texture is rendered.

[Mugen87]

Oh dear, i think i've found the issue :unamused:

The problem is the setup of light uniforms in WebGLRenderer:

https://github.com/mrdoob/three.js/blob/b50726ac9ddc361111e636c009bc32019c225b1f/src/renderers/WebGLRenderer.js#L1141

The setup of lights is done once per frame. But when a reflector is rendered, it overwrites the lights setup because it executes a rendering of the scene in onBeforeRender(). The problem is now that this setup is not reverted. So depending on the order of the reflector in the render list, subsequent objects are rendered with the wrong light setup. Or to be more precise, with the wrong view matrix (the one of the reflector's virtual camera).

I think we have to find a way to restore the light setup after possible changes in onBeforeRender().

[Mugen87]

One possible workaround: Set the .renderOrder of a reflector to a high value.

https://jsfiddle.net/9zLhq9Lf/4/

But we definitely need a conceptual solution for this bug.

[ChrisDenham]

ooo.. interesting. so presumably the problem could affect anything where onbeforerender is used to render from a different camera position?
Thanks for the renderOrder workaround for the meantime.

[Mugen87]

so presumably the problem could affect anything where onbeforerender is used to render from a different camera position?

Yes, unfortunately. This makes the problem even more critical...

[Mugen87]

I've created a basic PR that solves the problem. Not the best solution but at least straightforward.

An other solution: Set renderOrder in Reflector to something like Infinity so it's always at the end of the render list. But this seems very hacky...

[ChrisDenham]

Cool.. will try the PR change monday. Btw. just wondered if adding a call to a new function in WebGLRenderer risked breaking things for other renderer types that don’t implement that function? Does it need to check if the function exists before calling? Not an issue for me... I just thought it worth mentioning as it popped into my head when looking at your PR.

[Mugen87]

Btw. just wondered if adding a call to a new function in WebGLRenderer risked breaking things for other renderer types that don’t implement that function?

That should be no problem. The PR introduces a new method that won't affect existing code. But one thing is important:

The version (or release) of Reflector and three.js itself (so the core) should always match. So using a current Reflector but an old three.js version is no good approach. This is also true for the loaders and all other entities of the examples directory.

[ChrisDenham]

@Mugen87 I just tried your PR. Looks good to me. Thanks. Another supplementary question though: Presumably the PR only fixes Reflector, but couldn't the issue potentially affect anything that uses onbeforerender to render another camera position? i.e. do we need a more general fix?

[Mugen87]

but couldn't the issue potentially affect anything that uses onbeforerender to render another camera position? i.e. do we need a more general fix?

Yes, right now it is necessary to call .updateLights() after each call of .render(). A general solution requires some refactoring so nested calls of .render() don't overwrite state information. But let's wait how @mrdoob evaluates this issue. Maybe there is an obvious solution that i've missed so far.

[mrdoob]

https://jsfiddle.net/9zLhq9Lf/6/

[ChrisDenham]

Just for info: I can confirm that updating to r90 fixed the problems I was having with Reflector in my application. Thank you! :-)