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

It seems reflectionSource do not respect gamma ? #6897

Open
scarletsky opened this issue Aug 29, 2024 · 6 comments
Open

It seems reflectionSource do not respect gamma ? #6897

scarletsky opened this issue Aug 29, 2024 · 6 comments
Assignees
Labels
area: graphics Graphics related issue bug

Comments

@scarletsky
Copy link
Contributor

scarletsky commented Aug 29, 2024

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

By the way, from the use of useGammaTonemap it seems you're using engine v1. This has changed considerably in v2. See #6702 and #6714 and #6736

@scarletsky
Copy link
Contributor Author

scarletsky commented Aug 29, 2024

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
Copy link
Contributor

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 }
    ),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue bug
Projects
None yet
Development

No branches or pull requests

3 participants