wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.05k stars 611 forks source link

Colors are not exact RGB values #6096

Closed scarmain closed 8 months ago

scarmain commented 8 months ago

Describe the bug The RGB values that are rounded are not rounding properly. For example, Color.kRed returns red = 1, green = 2.44140625E-4, blue = 2.44140625E-4.

I think this has to do with the Color.roundAndClamp(), this should probably only be run with the equals() function, not all the colors.

To Reproduce (Using java test) assertEquals(1, Color.kRed.red, 1e-4); assertEquals(0, Color.kRed.green, 1e-4); //fails, 2.44140625E-4 assertEquals(0, Color.kRed.blue, 1e-4); //fails, 2.44140625E-4

Expected behavior The values should match 1,0,0 exact

Desktop (please complete the following information):

Additional context I was trying to make a Color.getHSV function, and it was throwing off results (red saturation would be 254.9377, calculated max(rgb)-min(rgb)/max * 255)

calcmogul commented 8 months ago

From https://github.wpilib.org/allwpilib/docs/development/java/edu/wpi/first/wpilibj/util/Color.html, the rounding is to limit colors to 12 bits of precision.

2^-12 works out to 0.000244140625. Maybe it's an off-by-one error in the rounding?

scarmain commented 8 months ago

Also related, the fromHSV has a ton of rounding also, which doesn't need it, as Color accepts doubles... (although you would have to divide by 255 for all of them)

Color.fromHSV(115, 23, 15).toHexString() = #0E0F0F From Website https://www.rapidtables.com/convert/color/hsv-to-rgb.html (to use this, the hue must be doubled from 180 to 360, and the sat/value should x*100/255, as we use 255 as max) hsv(230,9.02,5.88) = #0E0E0F

Edit: I know it's knit-picky, but it was really throwing off my inverse HSV formula... Again using that site: RBG(14, 15, 15) = HSV (180, 6.7, 5.9) (their units, not WPILIB) RGB(14, 14, 15) = HSV (240, 6.7, 5.9) Maybe I need to lower my test sensitivity, but it's crazy to have a 30* (in WpiLib units) error.

calcmogul commented 8 months ago

We'd accept a PR that fixes the HSV conversion accuracy. The test tolerances are like 1e-2, which is massive for the (0, 1) scale. I'm not personally familiar with the color conversion code, or color conversion in general, so I'm not the right person to mess with that code.