Stride: Color math should ideally always be in floats

[aaronfranke]

Version: All, master

Platform(s): All, but mostly affects newer devices

Color math should ideally be done with float to support arbitrary precision. It's somewhat common to have 10-bpc and 12-bpc displays, and HDR color math often needs 16 bits per channel. float aka Single precision floats have 23 bits of significant binary digits which is plenty for colors.

Floats also avoid the problem of clipping, as colors above 1 still work fine. Even if they clip when displayed, if the brightness is reduced for the whole image, the brightness differences in bright areas is preserved when using floats.

Floats are also more friendly to the user. It's easier to understand that "0.5" is 50% strength than "127".

Xenko already has Color3 and Color4 classes which already work with floats. However, there's not much point to Color3 in my opinion. You can just have constructors and methods that take the alpha value as an optional parameter for use cases that don't need it.

If it were up to me, I'd do the following:

1. Modify Color4 to make the alpha value optional. Consider deprecating Color3. This isn't a breaking change.

2. Rename Color to Color8 and consider deprecating it. This is a breaking change.

3. Rename Color4 to Color. This is a breaking change.

These don't all necessarily have to be done at once. In fact it may be preferable to leave a time gap between 2 and 3 to avoid people using "Color" in their code and having the implementation change without them knowing.

Obviously it's the Xenko team's decision of what to do, but I do think that something should be done, since color math being done with bytes is not ideal. Having a "Color" class with nothing after the name implies that it's the recommended / default choice.

[tebjan]

hi @aaronfranke the low bit/less channel color types match the pixel format for different texture formats. so if you want to create packed memory (colors are structs) that you can copy into a texture, you need those color types. also many operations on bytes are much faster than with floats. so the color types make sense and i would keep them... but i totally agree that you should not do math with 8 bit color types, also a rename might help beginners to chose Color4 instead of Color.

[Qibbi]

An issue would also be that the suggested Color8 uses the number to denote the bits per channel, while Color4 uses the number to denote the number of channels. This would result in an inconsistent naming scheme.
Additionally the current naming is what people who come from other engines expect them to be.

[Kryptos-FR]

Modify Color4 to make the alpha value optional. Consider deprecating Color3. This isn't a breaking change.

This is a breaking change. Color4 and Color3 are structs. The only way to make the alpha "optional" would be to change that field from float to float?. And then you would need to test everywhere whether the value is defined or not. Plus there might be some serialization/packing issue. And performance would definitely be impacted, which is a feature in a realtime game engine.

As @tebjan noted, the reason those different types exists is because there are required for the different texture format/mapping that exists.

[aaronfranke]

@Kryptos-FR No, using optional parameters, it would be done like this:

public Color4(float red, float green, float blue, float alpha = 1.0f)
{
        R = red;
        G = green;
        B = blue;
        A = alpha;
}

Then you can write code in either of these two ways:

new Color4(0.1f, 0.2f, 0.3f); // Alpha is 1.0
new Color4(0.1f, 0.2f, 0.3f, 0.4f); // Alpha is 0.4

[Kryptos-FR]

The semantic of optional parameter is not "cases that don't need it" but more for default values. How do you then differentiate those non-needed cases to the one that explicitly set the value to 1? And nothing prevents from setting the alpha later in a method that takes a ref Color4.

On the other hand, Color3 has a very clear semantic that the alpha will never change. And that semantic is preserved after serialization/deserialization.

[aaronfranke]

How do you then differentiate those non-needed cases to the one that explicitly set the value to 1?

Why would you need to do that?

And nothing prevents from setting the alpha later in a method that takes a ref Color4.

What's wrong with being able to set alpha?

[Kryptos-FR]

I think you did not understand the point of Color3.

[phr00t]

I agree with @aaronfranke here. 3 "Color" structs are a bit silly -- it should be standardized to one struct and Unity is a great example: Color is like Xenko's Color4 with an optional alpha value. Color3 and Color should be deprecated.

A great example of why 3 color structs is bad: https://github.com/phr00t/xenko/commit/dd82594294a073f4dbe5a722d63c22afc73b834f

VertexPositionNormalColor was using "Color", causing a bunch of bugs down the line because (byte)1 (very dark) was being considered (float)1 (very bright) somewhere else. Changing to Color4 fixes this problem.

I see Color referenced in many places -- some with hardcoded 0-255 values -- so getting rid of it is a big undertaking, unfortunately. Might be better just to tell everyone to use Color4. Might be able to get rid of Color3, at least.

[phr00t]

Making Color4 alpha optional and suited for general use: https://github.com/phr00t/xenko/commit/91a1867e7900dd8104b3af5f06952b088365b309

[tebjan]

having multiple color structs is important to have the right types for creating continuous memory that you upload to the GPU. so they should all stay as they are needed. the optional alpha argument is fine by me. see also my comment from above.

[phr00t]

OK @tebjan, was unaware of internal purposes. Would be nice to hide them from the user, so they didn't accidentally use wrong color structures, but relatively minor.

I'm perplexed @Kryptos-FR believes users are required to distinguish "ones with alpha being changed" and ones without when it is so easy to make it an optional parameter. Unity has been doing it that way without issue for thousands of games & developers. Anyway, I suppose that is the beauty of open source software -- we can each have our branches to meet our needs.

[Kryptos-FR]

@phr00t Sorry I should have commented why I down voted your comment. Bad behavior on my side.
I'm actually not against a default value in the constructor. What I was against is the way you change method arguments checking. Especially the check from exactly 4 to a "greater than" check. While it does work with an array of more than 4 elements, I prefer to notify the caller that they might be doing something wrong since it doesn't make sense to pass an array ray with more that 4 values and could be an indication of a bug (e.g. passing the wrong array). The name of the class is Color4 after all.

[phr00t]

@Kryptos-FR thank you for the explanation. I have updated the pull request to complain if a different float[] array size is given. With that done, my thought process was (and may still be) -- if the function has enough data to work, let it work. Using float[] to set a Color4 seems like a niche use case already, and developers might find it more useful if they can store more data in the same float array while letting the first 4 be color values. Who knows -- hard to say it is always a bug. I'll likely be allowing it on my branch.

The name is Color4, but that is only because "Color" is already taken, unfortunately.

[aaronfranke]

I'll close this since @xen2 seems to have his mind made up. Even though he hasn't commented in this thread, there was a discussion in this PR.

[xen2]

I mostly closed the PR because for the GPU-side types (Vertex*), the byte version is preferred.

Note that I am not excluding a possible renaming on the API side at some point (i.e. Color => ColorB and Color4 => Color?). But of course, such a breaking change for such a common type is quite some work. It might be worth waiting to combine with some other breaking changes and reevaluate then.