-
-
Notifications
You must be signed in to change notification settings - Fork 948
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
Color math should ideally always be in floats #259
Comments
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 |
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. |
This is a breaking change. As @tebjan noted, the reason those different types exists is because there are required for the different texture format/mapping that exists. |
@Kryptos-FR No, using optional parameters, it would be done like this:
Then you can write code in either of these two ways:
|
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 On the other hand, |
Why would you need to do that?
What's wrong with being able to set alpha? |
I think you did not understand the point of |
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: phr00t@dd82594 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. |
Making Color4 alpha optional and suited for general use: phr00t@91a1867 |
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. |
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. |
@phr00t Sorry I should have commented why I down voted your comment. Bad behavior on my side. |
@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. |
I mostly closed the PR because for the GPU-side types ( Note that I am not excluding a possible renaming on the API side at some point (i.e. |
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
andColor4
classes which already work with floats. However, there's not much point toColor3
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:
Modify
Color4
to make the alpha value optional. Consider deprecatingColor3
. This isn't a breaking change.Rename
Color
toColor8
and consider deprecating it. This is a breaking change.Rename
Color4
toColor
. 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.
The text was updated successfully, but these errors were encountered: