Skip to content
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

Xenko.Core.Mathematics library #251

Closed
Qibbi opened this issue Nov 2, 2018 · 4 comments
Closed

Xenko.Core.Mathematics library #251

Qibbi opened this issue Nov 2, 2018 · 4 comments

Comments

@Qibbi
Copy link

@Qibbi Qibbi commented Nov 2, 2018

Color:
Color uses byte for its channels. But in functions like public static Color Add(Color left, Color right) just add them together which returns an integer. These integers are used in the construction of the result, but because of overloads the constructor that is actually called is public Color(float red, float green, float blue, float alpha).
This constructor uses R = ToByte(red); which results in bad values.
So for every method casts to byte should be used.

A lot of other structs:
With the addition of the in keyword a lot of the methods of this library might be faster. Additionally a lot of ref keywords which are used for that can be changed to in to have the const contract.

@SleepyMode

This comment has been minimized.

Copy link
Collaborator

@SleepyMode SleepyMode commented Nov 8, 2018

Let me give this a shot.

@SleepyMode

This comment has been minimized.

Copy link
Collaborator

@SleepyMode SleepyMode commented Nov 9, 2018

Okay, so, quick update.
The in keyword cannot be used due to the fact we're using C# 7.0 and the in argument modifier has only been introduced in C# 7.2.

As for the byte casts, I'm just going over it again and then I'll throw in a pull request.

@aaronfranke

This comment has been minimized.

Copy link
Contributor

@aaronfranke aaronfranke commented Nov 9, 2018

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 at least 16 bits per channel.

Xenko's Color3 and Color4 classes appear to already work this way. Perhaps it would be best to deprecate Color or perhaps rename it to Color8 or Color32?

Side note, what's the point of Color3? 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 rename Color to Color8 and consider deprecating it, and merge Color3 and Color4 into one Color class using optional parameters for the alpha value. That last part (making Color4 alpha optional), isn't a breaking change, but the rest of this very much is.

@SleepyMode

This comment has been minimized.

Copy link
Collaborator

@SleepyMode SleepyMode commented Nov 11, 2018

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 at least 16 bits per channel.

Xenko's Color3 and Color4 classes appear to already work this way. Perhaps it would be best to deprecate Color or perhaps rename it to Color8 or Color32?

Side note, what's the point of Color3? 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 rename Color to Color8 and consider deprecating it, and merge Color3 and Color4 into one Color class using optional parameters for the alpha value. That last part (making Color4 alpha optional), isn't a breaking change, but the rest of this very much is.

Does that really belong here though? I'd recommend making a new issue with the suggestion so it doesn't disappear once this one is dealt with.

@xen2 xen2 closed this in 8133146 Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.