Team2890/allwpilib
4
0
Fork 0
mirror of https://github.com/wpilibsuite/allwpilib synced 2026-06-19 00:41:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

[wpilibc] Fix HSV to RGB conversion off-by-one error (#8722)

Browse Source 
`Color::FromHSV` didn't match the Java `Color.fromHSV` in some saturated
edge cases, introducing an off-by-one error when the HSV color should
correspond complete saturation of one or two of the primary colors.

Example:

- Java: `Color.fromHSV(0, 255, 255) -> (255, 0, 0)`
- C++: `Color::FromHSV(0, 255, 255) -> (255, 1, 1)`

This also means the following methods are also transitively affected:

- `AddressableLED::LEDData::SetHSV`
- `LEDPattern::Rainbow`

This off-by-one error is introduced by a rounding error from the chroma
calculation, which was dividing by 256 rather than the appropriate
maximum value of 255 like in Java:


7ca35e5678/wpilibj/src/main/java/edu/wpi/first/wpilibj/util/Color.java (L176-L177)

Also port appropriate tests from Java to C++ to catch this bug.

I found this bug when I tried to port `AddressableLEDBuffer` to RobotPy.
Codex found the root cause :)
This commit is contained in:
David Vo
2026-04-09 01:18:12 +10:00
committed by GitHub
parent 5b4769ea0a
commit 44dcf9a3ca
3 changed files with 88 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

55
wpilibc/src/test/native/cpp/LEDPatternTest.cpp
View File

@@ -385,6 +385,61 @@ TEST(LEDPatternTest, RainbowFullSize) {
}
}
TEST(LEDPatternTest, LEDDataSetHSVExactRgbValues) {
struct TestCase {
int h;
int s;
int v;
int r;
int g;
int b;
};
constexpr TestCase kCases[] = {
{0, 0, 0, 0, 0, 0}, {0, 0, 255, 255, 255, 255},
{0, 255, 255, 255, 0, 0}, {60, 255, 255, 0, 255, 0},
{120, 255, 255, 0, 0, 255}, {30, 255, 255, 255, 255, 0},
{90, 255, 255, 0, 255, 255}, {150, 255, 255, 255, 0, 255},
{0, 255, 128, 128, 0, 0}, {60, 255, 128, 0, 128, 0},
{120, 255, 128, 0, 0, 128},
};
for (const auto& test : kCases) {
SCOPED_TRACE(::testing::Message() << "SetHSV(" << test.h << ", " << test.s
<< ", " << test.v << ")");
AddressableLED::LEDData data;
data.SetHSV(test.h, test.s, test.v);
EXPECT_EQ(test.r, data.r & 0xFF);
EXPECT_EQ(test.g, data.g & 0xFF);
EXPECT_EQ(test.b, data.b & 0xFF);
}
}
TEST(LEDPatternTest, RainbowFullSizeExactRgbValues) {
std::array<AddressableLED::LEDData, 180> buffer;
LEDPattern::Rainbow(255, 255).ApplyTo(buffer);
struct TestCase {
int index;
int r;
int g;
int b;
};
constexpr TestCase kCases[] = {
{0, 255, 0, 0}, {30, 255, 255, 0}, {60, 0, 255, 0},
{90, 0, 255, 255}, {120, 0, 0, 255}, {150, 255, 0, 255},
};
for (const auto& test : kCases) {
SCOPED_TRACE(::testing::Message() << "LED " << test.index);
EXPECT_EQ(test.r, buffer[test.index].r & 0xFF);
EXPECT_EQ(test.g, buffer[test.index].g & 0xFF);
EXPECT_EQ(test.b, buffer[test.index].b & 0xFF);
}
}
TEST(LEDPatternTest, RainbowHalfSize) {
std::array<AddressableLED::LEDData, 90> buffer;
int saturation = 42;

2
wpiutil/src/main/native/include/wpi/util/Color.hpp
View File

@@ -865,7 +865,7 @@ class Color {
// that changes (X) from low to high (X+m) or high to low (v-X)
// Difference between highest and lowest value of any rgb component
int chroma = (s * v) >> 8;
int chroma = (s * v) / 255;
// Because hue is 0-180 rather than 0-360 use 30 not 60
int region = (h / 30) % 6;

32
wpiutil/src/test/native/cpp/ColorTest.cpp
View File

@@ -8,6 +8,8 @@
#include <gtest/gtest.h>
#include "wpi/util/Color8Bit.hpp"
TEST(ColorTest, ConstructDefault) {
constexpr wpi::util::Color color;
@@ -94,6 +96,36 @@ TEST(ColorTest, FromHSV) {
EXPECT_DOUBLE_EQ(0.251220703125, color.blue);
}
TEST(ColorTest, FromHSVExactRgbValues) {
struct TestCase {
int h;
int s;
int v;
int r;
int g;
int b;
};
constexpr TestCase kCases[] = {
{0, 0, 0, 0, 0, 0}, {0, 0, 255, 255, 255, 255},
{0, 255, 255, 255, 0, 0}, {60, 255, 255, 0, 255, 0},
{120, 255, 255, 0, 0, 255}, {30, 255, 255, 255, 255, 0},
{90, 255, 255, 0, 255, 255}, {150, 255, 255, 255, 0, 255},
{0, 255, 128, 128, 0, 0}, {60, 255, 128, 0, 128, 0},
{120, 255, 128, 0, 0, 128},
};
for (const auto& test : kCases) {
SCOPED_TRACE(::testing::Message() << "FromHSV(" << test.h << ", " << test.s
<< ", " << test.v << ")");
wpi::util::Color8Bit color{
wpi::util::Color::FromHSV(test.h, test.s, test.v)};
EXPECT_EQ(test.r, color.red);
EXPECT_EQ(test.g, color.green);
EXPECT_EQ(test.b, color.blue);
}
}
TEST(ColorTest, ToHexString) {
constexpr wpi::util::Color color1{255, 128, 64};
EXPECT_EQ("#FF8040", color1.HexString());