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

added gamma correction effect #108

Merged
merged 1 commit into from
Dec 22, 2021
Merged

added gamma correction effect #108

merged 1 commit into from
Dec 22, 2021

Conversation

chasedavis
Copy link
Contributor

Per this discussion, adding gamma correction effect as export to react-pp.

I'm still wondering if this is the best solution to the problem encountered, as it seems like some output encoding issue involving render targets, but exporting here does provide a knob for an issue I and @richczhou encountered where color/tonemapping is off in certain custom shader scenes (example).

this is my first contribution so I'm not sure if any other files need adjustments (e.g. postprocessing.d.ts ?)

@drcmda
Copy link
Member

drcmda commented Dec 22, 2021

@chasedavis i didn't know this effect existed, i remember i asked @vanruesc once and he said pp takes care of gammacorrection automatically but in practice pp is always brighter or causes slight banding.

do you know when GammaCorrection should be used? because we know when the canvas is set to autocorrect, so perhaps we can just add it automatically when that's the case?

@drcmda drcmda merged commit ea7d870 into pmndrs:master Dec 22, 2021
@vanruesc
Copy link
Member

The GammaCorrectionEffect is deprecated; don't use it. Setting WebGLRenderer.outputEncoding to sRGBEncoding enables gamma correction for passes that render to screen. Banding is caused by insufficient precision when using UnsignedByteType buffers. It's recommended to use HalfFloatType buffers if the device supports it:

import { HalfFloatType } from "three";

const composer = new EffectComposer(renderer, {
	frameBufferType: HalfFloatType
});

I've read somewhere that Unity works around the banding issue by converting colors to sRGB when writing to frame buffers and decoding back to linear when reading from them. It's more work overall but it doesn't require half float buffers. Three currently doesn't support this in hardware because render targets don't support SRGB8 yet.

There's also an issue with transparency and blending which I'm currently trying to wrap my head around: mrdoob/three.js#23019

@chasedavis
Copy link
Contributor Author

chasedavis commented Dec 22, 2021

You can see in the example that the output encoding is 3001 (SRGBEncoding).

Unfortunately the HalfFloatType doesn't fix the lightness that the effect composer is causing. In this particular example, there weren't issues of banding but the rendered image looked too white. Are there other instances of this issue and workarounds you are aware of @vanruesc ?

@vanruesc
Copy link
Member

vanruesc commented Dec 22, 2021

You can see in the example that the output encoding is 3001 (SRGBEncoding).

In that case, color encoding should be working correctly. I noticed that you set the gamma value of the GammaCorrectionEffect to 0.45 which is unusual. Gamma should be 2.2 to match sRGB, but the effect lets you change that which is why it's deprecated. The banding issues are probably not visible because of the noise which has a similar effect to dithering.

I modified the hat shader to output uColor directly as a sort of sanity test. Since gamma correction doesn't affect primary colors, secondary colors, black and white, I changed the color to #0D800D. This sRGB color is then converted to linear with three's convertSRGBToLinear method because the hat shader treats uColor as a linear color (as-is). Finally, the composer applies the output encoding from linear to sRGB. The resulting color on screen is #0D800D, which means that it's working. (Sandbox)

So the reason why the hat looks too bright or washed out lies somewhere in your custom shader code.

If you want to change the brightness or contrast of the entire scene, consider using the BrightnessContrastEffect instead. You could also integrate some of the brightness-contrast code in your custom shader for local tweaking.

You can find three's color encoding functions over here if you need those for color maps. Custom shaders should also make use of <encodings_fragment> so that they correctly encode their output when rendering to screen (without an effect composer).

@chasedavis
Copy link
Contributor Author

Thanks for diving in here @vanruesc !

It makes sense that you can adjust the shader to achieve a more true color. From the example you shared, it seems that if you delete the effect composer entirely then the backgroundColor (not dictated by the shader?) also changes its perceived brightness..

This is unexpected behavior to me, but I don't doubt that it's caused by the use of the custom shader. I'm not encountering this issue when using normal three/r3f primitives.

In the future, I like the idea of including postprocessing curves directly into custom shaders as necessary, rather than fiddle with the GammaCorrection pass. From a dx perspective, up to you @drcmda on if you keep the GammaCorrection component -- it is much easier for novices like me than hopping into shader code, especially if you're not author of the shader as is the case here. With that said, you can also achieve similar with existing Brightness/Contrast and Hue/Saturation effects as mentioned.

@vanruesc
Copy link
Member

it seems that if you delete the effect composer entirely then the backgroundColor (not dictated by the shader?) also changes its perceived brightness.

Three doesn't apply gamma correction to the scene background because it actually uses clear() for solid background colors. When using effects, the scene colors are stored in a buffer, including the background color. This entire image is then converted to sRGB, hence the difference. The solution is to use convertSRGBToLinear on the background color when using postprocessing.

@iuriiiurevich
Copy link
Contributor

There's also an issue with transparency and blending which I'm currently trying to wrap my head around: mrdoob/three.js#23019

@vanruesc, what do you think about this solution? mrdoob/three.js#23019 (comment)
I use it with react-postprocessing by adding an extra ShaderPass right between the RenderPass and the EffectPass.

@vanruesc
Copy link
Member

@iuriiiurevich I appreciate your input and the time you spent on moving the conversation forward! Unfortunately, I've been too busy to deep dive into the matter and can't really comment on your solution yet.

I'm actually still not sure which result really is the correct one. Three applies the linear-to-sRGB encoding manually inside the shaders when rendering to the canvas (with outputEncoding set to sRGBEncoding). This currently happens before the colors are premultiplied with alpha which in turn happens prior to blending. Other platforms use hardware-sRGB-encoding on the back buffer which would apply after the premultiplication, if I understand correctly. The story is again different when everything has already been blended and we try to convert the entire result from linear to sRGB. It would be nice to know how other engines address this sRGB-transparency-blending issue.

@iuriiiurevich
Copy link
Contributor

Yes, the situation still needs to be clarified.

I'm actually still not sure which result really is the correct one.

One note on this:
Blending the given colors in the given order ((D25578, 0.4), (55D278, 0.4), (34568B, 1)) in Photoshop produces the same result as the normal render (to screen) and the ./right implementation.
test
The top half of this picture is the given colors blended in Photoshop, the bottom half is the ./wrong implementation downloaded with file-saver:

const canvas = document.getElementsByTagName("canvas")[0];
canvas.toBlob((blob) => {
  saveAs(blob, "scene.png");
});

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.

4 participants