fponticelli / thx.color

General purpose color library for Haxe
http://thx-lib.org
MIT License
38 stars 4 forks source link

Conversion issue #29

Closed ramsestom closed 8 years ago

ramsestom commented 8 years ago

Running this code:

var t1:Int = Rgba.create(0, 125, 255, 255); 
var t2:Int = Rgba.create(0, 125, 255, 255).toHsla().toRgba();
trace(t1 + " " + t2);

Here is the output I have:

8257535 8257281

So, as you can see, converting an Rgba to Hsla and back to Rgba alter it and completely change its color....

fponticelli commented 8 years ago

That doesn't look right. I will look into it.

fponticelli commented 8 years ago

What is really concerning is that there should not be an automatic cast to Int ... that feels very dangerous.

ramsestom commented 8 years ago

Well even without the cast to Int, there is an issue with the alpha value. Here is the output I get with:

var t1:Rgba = Rgba.create(0, 125, 255, 255); 
var t2:Rgba = Rgba.create(0, 125, 255, 255).toHsla().toRgba();
trace(t1.toString() + "\t" + t2.toString());

output: rgba(0,125,255,1) rgba(0,125,255,0.00392156862745098)

Also, I find it a bit disturbing that the alpha value is divided by 255 in the toString() function. As the Rgba create() function take an Int alpha value, it would make more sense to also output this alpha value as an Int value in the toString() function. No?

ramsestom commented 8 years ago

if you remove the automatic cast to int, it would be great to have two functions for colors encoded with (R G B A) 4 channels:

toIntRGBA() and toIntARGB()

(the toIntARGB() function would be really useful because some haxe frameworks, like lime/openfl, treat colors as ARGB Ints (like in flash) . So on all my openfl projects where I want to use thx.color, I have to write this function. It would be easier if it was directly embeded in thx.color ;) )

If you keep the automatic cast to Int, would be a good idea to add an Argb class too, similar to the Rgba class and with a function in both class that allows to convert from on to another (a toArgb() function in the Rgba class and a toRgba() in the Argb one)

fponticelli commented 8 years ago

Ok, I am working on the following issues:

ramsestom commented 8 years ago

Great.

If you don't want to deprecate fromIntand fromIntsfrom Rgba and Rgbxa. The other solution would be to add an Argb class as I suggested (no need to add an Argbx because an Argbx conversion to Intwould lost float precision and result in an Argb anyway) and allow conversion between Argb and Rgba from both class. And the fromInt function of Rgba would take an Intencoded in RGBA format and the one of Argb would take an Int encoded in ARGB format... It might be a better solution to keep backward compatibility for people using the current version of thx.color...

fponticelli commented 8 years ago

That makes sense.

fponticelli commented 8 years ago

Made a new release that should fix all of the above. Feel free to open a new issue if you find something wrong.

ramsestom commented 8 years ago

OK Thanks.

Well now that you removed the implicit casting of Rgb or Rgba or Argb... to Int, you should add a toInt function to them. Cause for now they only have a fromInt function

fponticelli commented 8 years ago

Ooops :)

fponticelli commented 8 years ago

Sorry, I guess you are using the git version. I made the change, published to haxelib and forgot to push to master. It is now done. You will have to update thx.core (also published and pushed).