From 44dcf9a3ca8c975e9bb7579a3e418216d7e89092 Mon Sep 17 00:00:00 2001 From: David Vo Date: Thu, 9 Apr 2026 01:18:12 +1000 Subject: [PATCH] [wpilibc] Fix HSV to RGB conversion off-by-one error (#8722) `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: https://github.com/wpilibsuite/allwpilib/blob/7ca35e5678cf32caec6a1a866ca51d0136c4c398/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 :) --- .../src/test/native/cpp/LEDPatternTest.cpp | 55 +++++++++++++++++++ .../main/native/include/wpi/util/Color.hpp | 2 +- wpiutil/src/test/native/cpp/ColorTest.cpp | 32 +++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/wpilibc/src/test/native/cpp/LEDPatternTest.cpp b/wpilibc/src/test/native/cpp/LEDPatternTest.cpp index 09b0118807..9d6c3b11de 100644 --- a/wpilibc/src/test/native/cpp/LEDPatternTest.cpp +++ b/wpilibc/src/test/native/cpp/LEDPatternTest.cpp @@ -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 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 buffer; int saturation = 42; diff --git a/wpiutil/src/main/native/include/wpi/util/Color.hpp b/wpiutil/src/main/native/include/wpi/util/Color.hpp index 5ae15b723a..6cc180248d 100644 --- a/wpiutil/src/main/native/include/wpi/util/Color.hpp +++ b/wpiutil/src/main/native/include/wpi/util/Color.hpp @@ -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; diff --git a/wpiutil/src/test/native/cpp/ColorTest.cpp b/wpiutil/src/test/native/cpp/ColorTest.cpp index 77a34a9d1e..f95d5bbb90 100644 --- a/wpiutil/src/test/native/cpp/ColorTest.cpp +++ b/wpiutil/src/test/native/cpp/ColorTest.cpp @@ -8,6 +8,8 @@ #include +#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());