Clamping color channel values to within [0, 1] (#613)

Enforces color channel values to be within the range [0, 1] as indicated by the documentation of the input parameters. --------- Signed-off-by: Addisu Z. Taddese <[email protected]> Co-authored-by: Ian Chen <[email protected]>
gazebosim · Aug 16, 2024 · 98608b6 · 98608b6
1 parent 00deb01
commit 98608b6
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 29 deletions.
diff --git a/include/gz/math/Color.hh b/include/gz/math/Color.hh
@@ -100,13 +100,36 @@ namespace gz::math
     /// \param[in] _g Green value (range 0 to 1)
     /// \param[in] _b Blue value (range 0 to 1)
     /// \param[in] _a Alpha value (0=transparent, 1=opaque)
-    public: constexpr Color(const float _r, const float _g, const float _b,
-                            const float _a = 1.0)
+    /// \note If there are values outside the range [0, 1], they will be
+    /// clamped and an error message will be printed
+    public: Color(const float _r, const float _g, const float _b,
+                  const float _a = 1.0)
     : r(_r), g(_g), b(_b), a(_a)
     {
       this->Clamp();
     }
 
+    /// \brief Static function to create colors that are not clamped.
+    /// \param[in] _r Red value (range 0 to 1)
+    /// \param[in] _g Green value (range 0 to 1)
+    /// \param[in] _b Blue value (range 0 to 1)
+    /// \param[in] _a Alpha value (0=transparent, 1=opaque)
+    /// \note This is mainly intended to initialize constexpr values
+    /// such as Color::gRed. The Color::Color constructor is recommended
+    /// for regular use.
+    public: static constexpr Color UnclampedColor(const float _r,
+                                                  const float _g,
+                                                  const float _b,
+                                                  const float _a)
+    {
+      auto clr = Color();
+      clr.r = _r;
+      clr.g = _g;
+      clr.b = _b;
+      clr.a = _a;
+      return clr;
+    }
+
     /// \brief Reset the color to default values to red=0, green=0,
     /// blue=0, alpha=1.
     public: void Reset();
@@ -257,18 +280,7 @@ namespace gz::math
     public: bool operator!=(const Color &_pt) const;
 
     /// \brief Clamp the color values to valid ranges
-    private: constexpr void Clamp()
-    {
-      // These comparisons are carefully written to handle NaNs correctly.
-      if (!(this->r >= 0)) { this->r = 0; }
-      if (!(this->g >= 0)) { this->g = 0; }
-      if (!(this->b >= 0)) { this->b = 0; }
-      if (!(this->a >= 0)) { this->a = 0; }
-      if (this->r > 1) { this->r = this->r/255.0f; }
-      if (this->g > 1) { this->g = this->g/255.0f; }
-      if (this->b > 1) { this->b = this->b/255.0f; }
-      if (this->a > 1) { this->a = 1; }
-    }
+    private: void Clamp();
 
     /// \brief Stream insertion operator
     /// \param[in] _out the output stream

diff --git a/src/Color.cc b/src/Color.cc
@@ -16,6 +16,7 @@
  */
 #include <cmath>
 #include <algorithm>
+#include <iostream>
 
 #include "gz/math/Color.hh"
 
@@ -26,14 +27,14 @@ namespace {
 
 // Use constexpr storage for the Color constants, to avoid the C++ static
 // initialization order fiasco.
-constexpr Color gWhite = Color(1, 1, 1, 1);
-constexpr Color gBlack = Color(0, 0, 0, 1);
-constexpr Color gRed = Color(1, 0, 0, 1);
-constexpr Color gGreen = Color(0, 1, 0, 1);
-constexpr Color gBlue = Color(0, 0, 1, 1);
-constexpr Color gYellow = Color(1, 1, 0, 1);
-constexpr Color gMagenta = Color(1, 0, 1, 1);
-constexpr Color gCyan = Color(0, 1, 1, 1);
+constexpr Color gWhite = Color::UnclampedColor(1, 1, 1, 1);
+constexpr Color gBlack = Color::UnclampedColor(0, 0, 0, 1);
+constexpr Color gRed = Color::UnclampedColor(1, 0, 0, 1);
+constexpr Color gGreen = Color::UnclampedColor(0, 1, 0, 1);
+constexpr Color gBlue = Color::UnclampedColor(0, 0, 1, 1);
+constexpr Color gYellow = Color::UnclampedColor(1, 1, 0, 1);
+constexpr Color gMagenta = Color::UnclampedColor(1, 0, 1, 1);
+constexpr Color gCyan = Color::UnclampedColor(0, 1, 1, 1);
 
 }  // namespace
 
@@ -183,6 +184,9 @@ void Color::SetFromYUV(const float _y, const float _u, const float _v)
   this->r = _y + 1.140f*_v;
   this->g = _y - 0.395f*_u - 0.581f*_v;
   this->b = _y + 2.032f*_u;
+  this->r = this->r > 1 ? this->r / 255.0 : this->r;
+  this->g = this->g > 1 ? this->g / 255.0 : this->g;
+  this->b = this->b > 1 ? this->b / 255.0 : this->b;
   this->Clamp();
 }
 
@@ -552,3 +556,24 @@ void Color::A(const float _a)
 {
   this->a = _a;
 }
+
+//////////////////////////////////////////////////
+void Color::Clamp()
+{
+  bool clamped = false;
+  // These comparisons are carefully written to handle NaNs correctly.
+  if (!(this->r >= 0)) { this->r = 0; clamped=true;}
+  if (!(this->g >= 0)) { this->g = 0; clamped=true;}
+  if (!(this->b >= 0)) { this->b = 0; clamped=true;}
+  if (!(this->a >= 0)) { this->a = 0; clamped=true;}
+  if (this->r > 1) { this->r = 1.0; clamped=true;}
+  if (this->g > 1) { this->g = 1.0; clamped=true;}
+  if (this->b > 1) { this->b = 1.0; clamped=true;}
+  if (this->a > 1) { this->a = 1.0; clamped=true;}
+
+  if (clamped)
+  {
+    // TODO(azeey) Use spdlog when we have it's available.
+    std::cerr << "Color values were clamped\n";
+  }
+}
diff --git a/src/Color_TEST.cc b/src/Color_TEST.cc
@@ -247,7 +247,7 @@ TEST(Color, Color)
 
   clr = math::Color(1.0f, 0.0f, 0.5f, 1.0f) +
     math::Color(0.1f, 0.3f, 0.4f, 1.0f);
-  EXPECT_TRUE(math::equal(0.00431373f, clr.R()));
+  EXPECT_TRUE(math::equal(1.0f, clr.R())) << clr;
   EXPECT_TRUE(math::equal(0.3f, clr.G()));
   EXPECT_TRUE(math::equal(0.9f, clr.B()));
   EXPECT_TRUE(math::equal(1.0f, clr.A()));
@@ -422,8 +422,8 @@ TEST(Color, HSV)
   EXPECT_NEAR(hsv.Z(), 0.3f, 1e-3);
 
   clr.SetFromHSV(60, 10, 5);
-  EXPECT_NEAR(clr.R(), 0.0196078f, 1e-3);
-  EXPECT_NEAR(clr.G(), 0.0196078f, 1e-3);
+  EXPECT_NEAR(clr.R(), 1.0, 1e-3);
+  EXPECT_NEAR(clr.G(), 1.0, 1e-3);
   EXPECT_NEAR(clr.B(), 0.0f, 1e-3);
   EXPECT_NEAR(clr.A(), 1.0, 1e-3);
 

diff --git a/src/python_pybind11/test/Color_TEST.py b/src/python_pybind11/test/Color_TEST.py
@@ -216,10 +216,10 @@ def test_color(self):
         self.assertAlmostEqual(0.9064, clr.b(), delta=1e-3)
         self.assertAlmostEqual(0.04, clr.a())
 
-        self.assertTrue(clr.yuv() == Vector3f(0.104985, 0.95227, 0.429305))
+        self.assertTrue(clr.yuv() == Vector3f(0.104985, 0.95227, 0.429305), msg=f"{clr}")
 
         clr = Color(1.0, 0.0, 0.5, 1.0) + Color(0.1, 0.3, 0.4, 1.0)
-        self.assertAlmostEqual(0.00431373, clr.r(), delta=1e-4)
+        self.assertAlmostEqual(1.0, clr.r(), delta=1e-4)
         self.assertAlmostEqual(0.3, clr.g(), delta=1e-4)
         self.assertAlmostEqual(0.9, clr.b(), delta=1e-4)
         self.assertAlmostEqual(1.0, clr.a(), delta=1e-4)
@@ -318,8 +318,8 @@ def test_HSV(self):
         self.assertAlmostEqual(hsv.z(), 0.3, delta=1e-3)
 
         clr.set_from_hsv(60, 10, 5)
-        self.assertAlmostEqual(clr.r(), 0.0196078, delta=1e-3)
-        self.assertAlmostEqual(clr.g(), 0.0196078, delta=1e-3)
+        self.assertAlmostEqual(clr.r(), 1.0, delta=1e-3)
+        self.assertAlmostEqual(clr.g(), 1.0, delta=1e-3)
         self.assertAlmostEqual(clr.b(), 0.0, delta=1e-3)
         self.assertAlmostEqual(clr.a(), 1.0, delta=1e-3)