All Color in Godot is assumed to be encoded using the nonlinear sRGB transfer function. Any cases where this is not the case must be clearly documented. (As in, any time a Color is expected to use linear encoding, it should be clearly documented.)

Would you mind going through the chain of functions and clearly documenting (with comments or otherwise) when functions accept linear-encoded values instead of the expected nonlinear sRGB-encoded values?

When writing this, you can simply say that “color must be linear-encoded” or “color must use linear encoding”.

As a side note, I see some multiplications against an energy multiplier that are happening on the colour channels: indeed, this multiplication must occur on linear-encoded values otherwise it will produce incorrect results.

@allenwp I updated the pr with a better description of what is happening. Where I removed the srgb_to_linear call, clear_color is in fact encoded using the nonlinear sRGB transfer function. The issue is that converting this to linear now is incorrect because it will be converted a second time later when it is put into the uniform buffer for the sky shader.

Not sure if the comments should still be added given that the value is not being passed in linear encoding.

@allenwp I updated the pr with a better description of what is happening. Where I removed the srgb_to_linear call, clear_color is in fact encoded using the nonlinear sRGB transfer function. The issue is that converting this to linear now is incorrect because it will be converted a second time later when it is put into the uniform buffer for the sky shader.

Not sure if the comments should still be added given that the value is not being passed in linear encoding.

Ah, so there is another problem that colour/lighting maths are being performed on these nonlinear sRGB-encoded values (when they are multiplied by the energy multiplier). This is likely incorrect and should be addressed.

To put it simply, no maths of any sort should be performed on nonlinear sRGB-encoded values. The only exception is if you are using the math as a layer of "encoding" that will be directly reversed before doing any further operations, like with a "luminance multiplier".

@allenwp I've updated the pr to correct this. If fog is enabled, bg_energy_multiplier is already applied to linear colours in the shader via sky_energy_multiplier, so it doesn't need to be multiplied into the colour before the shader. For the cases where the sky is not drawn (clear colour / background colour with no fog), the colour is converted to linear encoding, multiplied, and converted back.

I've added a similar change to compatibility, which applies the brightness multiplier to linear-encoded colours.

Thanks! It will take me a while to review this, and I'm not familiar with this part of the code base, but at a glance, I believe the intent of this updated PR is correct.

A couple of things that I can mention before doing a thorough review: dithering must always be applied as the final step before quantization. The dithering in the sky shaders is implemented in quite a weird way, and probably can be improved, but for now it should be left as-is, as the final step in the shader.

Another thing to mention is that conversions to and from nonlinear sRGB encoding are expensive and should be minimized. So if there's a way to keep things in linear (and document it with comments) the whole way through to limit conversions back and forth, that is preferred for performance reasons.

Finally, the Compatibility renderer intentionally performs some colour maths incorrectly for performance reasons, so avoiding decoding and re-encoding in this renderer may actually be the intended design, even though it produces incorrect results. You can see this comment for more details about this. Conversely, this sort of optimization to perform colour maths on nonlinear sRGB-encoded values should never happen in the Mobile/Forward+ renderers.

Anyway, at some point I'll take a deeper look into this PR to verify it.

And thanks for prompting me to write all of this out. Our discussion here is a good template for some colour documentation for contributors...

@allenwp I can probably optimize the RD renderers and remove the extra conversions, as after what I added they are only accessed in linear anyways.

As for Compatibility, I wasn't originally going to add the conversions but I saw that other environment colours (e.g. ambient light) are converted to linear before energy calculations, and it's also done on the cpu and not the gpu so I decided to add it anyways. In terms of the gpu work, I only rearranged the order of calculations to multiply with the energy while the colour is in linear and did not add anything. I did move the dithering before the conversion back to srgb because it uses the energy multiplier and I wasn't sure if it mattered.

If parts / all of Compatibility changes need to be removed / reworked I can do that. I will start with moving the dithering back to where it was.

and it's also done on the cpu and not the gpu so I decided to add it anyways.

Great; this sounds entirely reasonable and does not go against the spirit of the Compatibility renderer!

If parts / all of Compatibility changes need to be removed / reworked I can do that. I will start with moving the dithering back to where it was.

Based on what you said, the dithering change is the only part that should be reverted. All the rest sounds good and doesn't need to be reworked!

In the future, we can improve the dithering to not depend on the energy multiplier or revisit this to come to some other sort of solution...

@allenwp I just finished making all of the changes and also updated the pr description.

The only thing I've changed in my most recent push is that I've done some deduplication of the code, so it should still work the same.

I'm not sure what difference it makes but I believe the energy multiplier should always be multiplied into the clear colour after sending it to the shader so that the framebuffer is cleared with the correct color so I've changed that.

Needs rebase