[BREAKING] Use linear space for vertex colors in gLTF

[LeXXik]

Fixes #7473

[BREAKING]

Previously the engine was converting vertex colors from gamma space to linear (always assuming they were stored in gamma space). It was now decided to follow gLTF spec, which stores vertex colors in linear space. Both FBX and gLTF meshes now assume vertex colors to be in linear space. This PR introduces new API to allow specifying if the vertex colors need to be converted to gamma space or use as is (keep them in linear).

When set to true, the vertex shader converts vertex colors from gamma to linear space to ensure correct interpolation in the fragment shader. This flag is provided for backwards compatibility, allowing users to mark their materials to handle vertex colors in gamma space. Defaults to false, which indicates that vertex colors are stored in linear space.

New API:

standardMaterial.vertexColorGamma = false;

Change:
Left: FBX (ok) < - > Right: GLB (not ok)

Before:

After:

Checklist

• I have read the contributing guidelines
• My code follows the project's coding standards
• This PR focuses on a single change

[LeXXik]

@mvaligursky here is an attempt to fix the vertex colors. The bottom cubes are now identical, where solid colors are used for cube sides in both FBX and GLB. The top ones differ a bit in interpolation, though. I think it is because FBX transforms the colors from gamma to linear, while in GLB we use the colors directly. Not sure, so marking this as draft. Please, check. You can run the repro project against this PR to see the result.

[mvaligursky]

Yep this is definitely a solution I'd be happy with. We should document that new flag on StandardMaterial.

The way vertex colors work is that vertex shader gets them and outputs them as varyings to fragment shader - they get interpolated for each fragment. And then fragment shader handles gamma correction. This is not correct, as the interpolation can take place in linear or gamma space currently, but it should always be in linear space. This is why you see that interpolation difference I suspect.

So instead of handling gamma correction in fragment shader, this needs to be move to vertex shader. And fragment shader would simply consume vertex colors, always in linear space.

Risk - people that override diffuse / emissive chunks would need to remove gamma correction from there, so slight breaking change. We could update _validateMapChunk to detect gammaCorrectInputVec3 in those chunks and log a warning perhaps.

[Maksims]

We use color sometimes to contain our data in it, so gamma conversion while can be applied in some cases, in others in shader we would not want to apply it.
Especially if RGB contains mixed values, some to be gamma corrected (like emissive intensity), while others might have other meaning.
So as it is per texture (per slot) we would want to have it per chunk as well, so user can ensure data is not modified and gets to the shader as is, but then it is gamma corrected in relevant chunks when required only.

[mvaligursky]

Your case should work well @Maksims - you'd use the flag on StandardMaterial to mark them as linear and no conversion would take place anywhere.

[LeXXik]

@Maksims right, but I think this PR addresses an issue, which would prevent your use-case today. Currently imported GLB models always convert vertex colors to gamma space, so it would mangle your data today.

@mvaligursky you were right, moving the gamma correction to vertex shader helped with interpolation (I've updated the screenshot). At the moment gammaPS is still added to the fragment shader, albeit not used. Do we want to remove it? I am not sure where it is added. Could be in separate PR.

I moved gamma correction to litmain. Let me know if you want it to be somewhere else.
Added docs to new material property and debug check for gamma correction in fragment shader.

[mvaligursky]

gammaPS is still added to the fragment shader

leave it, I think this is added more or less globally, as multiple possible chunks use it.

[mvaligursky]

Alright, another change request here, sorry and thanks.

Lets align with GLB spec:

1. By default, assume vertex colors on the mesh are stored on linear space.
2. StandardMaterial will have vertexColorGamma flag default to false. This flag is only used by users to manually mark they have vertices in gamma space.
3. glb parser does not need to touch this flag.

The rest of the code stays. JSDocs on that flag should change to something like:

When set to true, the vertex shader converts vertex colors from gamma to linear space to ensure correct interpolation in the fragment shader. This flag is provided for backwards compatibility, allowing users to mark their materials to handle vertex colors in gamma space. Defaults to false, which indicates that vertex colors are stored in linear space.

[mvaligursky]

Also, the validation should be perhaps specific to diffuse and emissive chunk only
and simply search for gammaCorrectInputVec3(saturate3(vVertexColor. or gammaCorrectInput(saturate(vVertexColor. strings and warn if in any of those.

I just want to avoid any false positives if possible.

[LeXXik]

When set to true, the vertex shader converts vertex colors from gamma to linear space to ensure...

You probably mean from linear to gamma, if linear is the new default.

By default, assume vertex colors on the mesh are stored on linear space.

Hmm, but that would break FBX vertex colors, which need to be gamma corrected.

[mvaligursky]

You probably mean from linear to gamma, if linear is the new default.

I mean from gamma to linear.
Linear is default, so nothing to do. But when the user sets vertexColorGamma, which indicates mesh has vertex colors in gamma space, we need to convert them to linear space in VS, before interpolation.

[mvaligursky]

Hmm, but that would break FBX vertex colors, which need to be gamma corrected.

Yep, there is no solution that handles everything. We've discussed this and decided this change is worth sorting out, and this is the most correct way. There will be some impact to existing projects, but a flag will fix it. We'll need to mark this PR with [BREAKING]

[LeXXik]

Alright, I see, thanks. I've updated the comments and PR description. This PR will require an Editor change as well.

[mvaligursky]

Great stuff, one small change and we're done here!

[mvaligursky]

Approving but not merging yet.
I need to sync with @kpal81xd regarding this, which possibly will be released before the engine to get the data set up: playcanvas/editor#1539