Reflector Color Space Issues

[WestLangley]

I am opening this as a separate issue in the hope we can keep the discussion focused.

Note that "color space" and "encoding" have still not been decoupled in the shader, so I am using legacy terminology here. Hopefully that will end soon.

1. Reflector should render in scene-referred linear color space -- to a float or half-float render target. (Or to an RGBM-encoded UInt8 render target.) This is true, regardless of the renderer's output encoding. Setting the Reflector's renderTarget.texture.encoding to renderer.outputEncoding is not the right thing to do.

2. Consequently, it is only correct to include encodings_fragment in the reflector's fragment shader if it is needed for something like RGBM encoding. But three.js no longer supports such encodings.

3. When encoding to sRGB color space, tone mapping must proceed encoding. This is to ensure the inputs to the encoding step are in LDR. Reflector includes the encoding fragment, but not tone mapping fragment. Ideally, it should not include either of them.

Granted, encoding to sRGB and then decoding to linear-sRGB when reading back should not make a vast visual difference, except for rounding due to the Uint8 conversion. But still, it is unnecessary.

Feedback welcome.

three.js version: [r.142]

[Mugen87]

Ideally, it should not include either of them.

Reflector renders the scene from a virtual camera to a render target and then projects the respective texture onto its surface. The reflector's shader (which performs the projection) is equivalent to built-in materials, meaning it's fragment shader has to output the color in the same space defined in renderer.outputEncoding.

If encodings_fragment is going to be removed, how would be the color conversion be implemented when using the reflector within a sRGB workflow?

[WestLangley]

The reflector's shader (which performs the projection) is equivalent to built-in materials

@Mugen87 You are correct. That is a very good way of thinking about it.

The reflector should include the tone mapping fragment.

[Mugen87]

The reflector should include the tone mapping fragment, however.

If we just include tonemapping_fragment and do nothing else, the code is not right. The problem is that the beauty pass of the reflector (rendered with the virtual camera) is already tone mapped. If tonemapping_fragment is added to the reflectors shader, tone mapping would be applied twice.

This can only be fixed if tone mapping is disabled when the beauty pass is performed (and then enabled back again). The code for this needs to be placed around this line of code:

three.js/examples/jsm/objects/Reflector.js

Line 158 in 7ad7827

renderer.render( scene, virtualCamera );

I would say it's indeed more correct to add the tone mapping fragment and update Reflector.onBeforeRender() as described above. The same should be done for Refractor.

[WestLangley]

I am inclined to agree with what I intended to say in my original post, but did not say very precisely.

1. When rendering with the virtual camera to the render target texture, rendering should be in scene-referred linear colorspace: no tone mapping, and no encoding (if floating point textures are supported). The render target texture is an HDR texture with linear encoding.

2. When rendering with the real camera, both tone mapping and encoding should be included in the Reflector's shader.