Separate linear and sRGB uniform buffers in RD rendering backends

[clayjohn]

Fixes: #84989

We had a separate uniform set for sRGB vs linear versions of the uniform buffer. When turning on HDR_2D we would use the linear uniform set. Since the same material can be used in both a regular viewport and one with HDR_2D, we have to generate both sets.

Previously, we had one data array and one uniform buffer for both sets. That resulted in the sRGB version of the uniform set overwriting the values in the linear set and the shader effectively always using sRGB values.

This separates out the buffer so the two versions can co-exist.

Before: Note the difference between the version on the right and the one on the left

After: Note how the color is consistent now

[BastiaanOlij]

Looks good to me, seems to be a sensible change.

[BastiaanOlij]

I would use an enum for the buffer, its just clearer why we have two. Might be over kill.

[clayjohn]

Could you clarify where the Enum would go?

Do you mean you would create an enum just to establish the size of the Array?

[Dowsley]

I believe what he means is making it clearer that 0 is Linear and 1 is sRGB through an Enum. Declared at the top of material.h like this:

enum BufferType {
	LINEAR = 0,
	SRGB = 1
};

Used as such (without the need of comments):

Vector<uint8_t> ubo_data[BufferType.LINEAR];
RID uniform_buffer[BufferType.SRGB];

I don't find it overkill, might save some headache for someone trying to guess which one is which in a later feature. But I think this ship has sailed since it has already been merged.

@clayjohn if this is welcome, let me know if I should make another PR with this minor change. It might be a good start for me to get used with contributions.

[clayjohn]

I don't think it would be beneficial. Using enums hides what is actually happening. For example, your code will fail to compile (an array of size 0 doesn't work for the UBO array, the array needs to have 2 elements). Also, the uniform buffer RID array is also wrong since it also needs two elements.

It's really easy to make this kind of mistake when you hide the important values behind an enum.

That being said, enums can be really handy when you use them both to initialize the array and to index into the array. Doing something like:

LINEAR = 0 // use to index into the array
SRGB = 1 // use to index into the array
MAX = 2 // use to initialize the array

But in this case we index into the array with a boolean. So enums won't help.

IMO using enums here adds more complexity for no gain

[akien-mga]

Thanks!

[Dowsley]

Tysm. This was quite a headache to figure out when enabling HDR. I can only imagine how many users couldn't find a workaround and simply decided not to use it.

[Dowsley]

@clayjohn I know the feature works because you reproduced so I'm just trying to learn here.

If ubo_data[0] is linear and ubo_data[1] is sRGB, why is the boolean p_use_linear_color used like this? When it's true, it evaluates to 1 and accesses the sRGB data. Which seems counter-intuitive given the name of the variable.

[JohnPandich]

Hey folks, I am running into a tricky edge-case that impacts usability. When using a viewport with "transparent_bg" set to true, the glow is removed with the background. Please let me know if you would like me to upload a project with the issue reproduced.

update: On second thought this is a completely separate issue and is accounted for here: #28141