playcanvas / engine

Powerful web graphics runtime built on WebGL, WebGPU, WebXR and glTF
https://playcanvas.com
MIT License
9.73k stars 1.36k forks source link

It seems reflectionSource do not respect gamma ? #6897

Open scarletsky opened 3 months ago

scarletsky commented 3 months ago

I am sure my standard materials do not need useGammaTonemap, so I set it to false. All textures are sampled correctly except for material.cubeMap, which appears darker than it should. Here is what I found:

In standard.js, we have the following logic:

subCode = subCode.replace(/\$DECODE/g, ChunkUtils.decodeFunc((!options.litOptions.gamma && encoding === 'srgb')
    ? 'linear'
    : encoding)
);

It will check litOptions.gamma to pick linear or encoding.

But in lit-shader.js, the reflectionSource will not check gamma:

if (options.reflectionSource === 'envAtlasHQ') {
    func.append(options.fixSeams ? chunks.fixCubemapSeamsStretchPS : chunks.fixCubemapSeamsNonePS);
    func.append(chunks.envAtlasPS);
    func.append(chunks.reflectionEnvHQPS
        .replace(/\$DECODE_CUBEMAP/g, ChunkUtils.decodeFunc(options.reflectionCubemapEncoding))
        .replace(/\$DECODE/g, ChunkUtils.decodeFunc(options.reflectionEncoding))
    );
} else if (options.reflectionSource === 'envAtlas') {
    func.append(chunks.envAtlasPS);
    func.append(chunks.reflectionEnvPS.replace(/\$DECODE/g, ChunkUtils.decodeFunc(options.reflectionEncoding)));
} else if (options.reflectionSource === 'cubeMap') {
    func.append(options.fixSeams ? chunks.fixCubemapSeamsStretchPS : chunks.fixCubemapSeamsNonePS);
    func.append(chunks.reflectionCubePS.replace(/\$DECODE/g, ChunkUtils.decodeFunc(options.reflectionEncoding)));
} else if (options.reflectionSource === 'sphereMap') {
    func.append(chunks.reflectionSpherePS.replace(/\$DECODE/g, ChunkUtils.decodeFunc(options.reflectionEncoding)));
}

This means all of the $DECODE will be replaced to decodeGamma.

Even though I set app.scene.gammaCorrection = 0, the cubeMap still use decodeGamma, but what I want is decodeLinear... 😢

slimbuck commented 3 months ago

Oh that's interesting. We can investigate updating the reflection decode to respect scene.gammaCorrect, but that may break existing projects. @mvaligursky perhaps we can consider for engine v2?

We probably won't be able to push a change like this out quickly though, so I suggest you patch reflectionCubePS shader chunk to do what you need till we have a solution.

scarletsky commented 3 months ago

@slimbuck I think you should respect material.useGammaTonemap instead of scene.gammaCorrection, because it is a material level setting.

We probably won't be able to push a change like this out quickly though, so I suggest you patch reflectionCubePS shader chunk to do what you need till we have a solution.

Don't worry, I can use a custom build of PlayCanvas engine. 😃

mvaligursky commented 3 months ago

Typically, the decode functions on texture sampling is purely based on the texture format. The lighting part of the shaders needs all data in linear space, and so if the texture is sRGB it needs to be decoded to linear space. If the texture is linear, it should do pass-through decoding.

useGammaTonemap is not related to the way textures are sampled, but only to the way the final pixel is written to the framebuffer.

In engine v2 the useGammaTonemap was removed, and we only have useTonemap. Gamma is automaticaly based on the scene / camera gamma settings.

mvaligursky commented 3 months ago

By the way, from the use of useGammaTonemap it seems you're using engine v1. This has changed considerably in v2. See https://github.com/playcanvas/engine/pull/6702 and https://github.com/playcanvas/engine/pull/6714 and https://github.com/playcanvas/engine/pull/6736

scarletsky commented 3 months ago

Typically, the decode functions on texture sampling is purely based on the texture format. The lighting part of the shaders needs all data in linear space, and so if the texture is sRGB it needs to be decoded to linear space. If the texture is linear, it should do pass-through decoding.

Yes, but the texture.encoding is just a getter:

    get encoding() {
        switch (this.type) {
            case TEXTURETYPE_RGBM:
                return 'rgbm';
            case TEXTURETYPE_RGBE:
                return 'rgbe';
            case TEXTURETYPE_RGBP:
                return 'rgbp';
            default:
                return (this.format === PIXELFORMAT_RGB16F ||
                        this.format === PIXELFORMAT_RGB32F ||
                        this.format === PIXELFORMAT_RGBA16F ||
                        this.format === PIXELFORMAT_RGBA32F ||
                        isIntegerPixelFormat(this.format)) ? 'linear' : 'srgb';
        }
    }

Even though I know the decodeLinear is correct, I can not change it. 😢

Why decodeLinear is correct ? because my scenes are very old. In the old engine, they will be sampled by textureCubeSRGB:

vec4 textureCubeSRGB(samplerCube tex, vec3 uvw) {
    return textureCube(tex, uvw);
}

It is in linear space. In order to keep the same result, I need to replace $DECODE to decodeLinear. But now we have no way to do this.

it seems you're using engine v1.

Yes, I want to upgrade to v2 too, but I still need to support WebGL 1. So I may stick to 1.73.4 until our product drop support iOS 15-...

mvaligursky commented 3 months ago

In case you use engine only, you should be able to change the type of the texture like this. Not sure specifically about cubemaps or what options are supported there, @slimbuck might know more

    helipad: new pc.Asset(
        'helipad-env-atlas',
        'texture',
        { url: rootPath + '/static/assets/cubemaps/helipad-env-atlas.png' },
        { type: pc.TEXTURETYPE_RGBP, mipmaps: false }
    ),