From 13a1125b5b0b2738a96bdb7354a4e1d69365af5d Mon Sep 17 00:00:00 2001 From: Nuno Guedelha Date: Thu, 25 Sep 2025 19:33:08 +0100 Subject: [PATCH 1/5] WIP: Fix non-const Color::operator[] to return mutable reference Applied non optimal Changes to operator[] for having the desired behaviour. Added respective unit tests Signed-off-by: Nuno Guedelha --- include/gz/math/Color.hh | 2 +- src/Color.cc | 24 ++++++++++++++-- src/Color_TEST.cc | 40 ++++++++++++++++++++------ src/python_pybind11/test/Color_TEST.py | 13 ++++++--- 4 files changed, 63 insertions(+), 16 deletions(-) diff --git a/include/gz/math/Color.hh b/include/gz/math/Color.hh index b38beb8db..9aa53e232 100644 --- a/include/gz/math/Color.hh +++ b/include/gz/math/Color.hh @@ -168,7 +168,7 @@ namespace gz::math /// 3=alpha) /// \return r, g, b, or a when _index is 0, 1, 2 or 3. A NAN_F value is /// returned if the _index is invalid - public: float operator[](const unsigned int _index); + public: float &operator[](const unsigned int _index); /// \brief Array index operator, const version /// \param[in] _index Color component index(0=red, 1=green, 2=blue, diff --git a/src/Color.cc b/src/Color.cc index 0759baa5c..21c8dff89 100644 --- a/src/Color.cc +++ b/src/Color.cc @@ -191,9 +191,29 @@ void Color::SetFromYUV(const float _y, const float _u, const float _v) } ////////////////////////////////////////////////// -float Color::operator[](const unsigned int _index) +float &Color::operator[](const unsigned int _index) { - return (*static_cast(this))[_index]; + switch (_index) + { + case 0: + return this->r; + case 1: + return this->g; + case 2: + return this->b; + case 3: + return this->a; + default: + break; + } + + // This allows referencing a NAN_F value. + static float invalidParam; + // Prevents overwriting of invalidParam with non NAN_F + // value through wrong index. + invalidParam = NAN_F; + + return invalidParam; } ////////////////////////////////////////////////// diff --git a/src/Color_TEST.cc b/src/Color_TEST.cc index 348f8a60a..17c7e81c6 100644 --- a/src/Color_TEST.cc +++ b/src/Color_TEST.cc @@ -186,20 +186,36 @@ TEST(Color, Color) EXPECT_FLOAT_EQ(1.0f, clr.A()); clr.SetFromHSV(300, 0.5, 1.0); - EXPECT_FLOAT_EQ(1.0f, clr[0]); - EXPECT_FLOAT_EQ(0.5f, clr[1]); - EXPECT_FLOAT_EQ(1.0f, clr[2]); - EXPECT_FLOAT_EQ(1.0f, clr[3]); - EXPECT_TRUE(std::isnan(clr[4])); + EXPECT_FLOAT_EQ(1.0f, clr.R()); + EXPECT_FLOAT_EQ(0.5f, clr.G()); + EXPECT_FLOAT_EQ(1.0f, clr.B()); + EXPECT_FLOAT_EQ(1.0f, clr.A()); clr.R() = 0.1f; clr.G() = 0.2f; clr.B() = 0.3f; clr.A() = 0.4f; - EXPECT_FLOAT_EQ(0.1f, clr[0]); - EXPECT_FLOAT_EQ(0.2f, clr[1]); - EXPECT_FLOAT_EQ(0.3f, clr[2]); - EXPECT_FLOAT_EQ(0.4f, clr[3]); + EXPECT_FLOAT_EQ(0.1f, clr.R()); + EXPECT_FLOAT_EQ(0.2f, clr.G()); + EXPECT_FLOAT_EQ(0.3f, clr.B()); + EXPECT_FLOAT_EQ(0.4f, clr.A()); + + EXPECT_FLOAT_EQ(clr.R(), clr[0]); + EXPECT_FLOAT_EQ(clr.G(), clr[1]); + EXPECT_FLOAT_EQ(clr.B(), clr[2]); + EXPECT_FLOAT_EQ(clr.A(), clr[3]); + EXPECT_TRUE(std::isnan(clr[4])); + + clr[0] = 0.5f; + clr[1] = 0.6f; + clr[2] = 0.7f; + clr[3] = 0.8f; + clr[4] = 0.9f; + EXPECT_FLOAT_EQ(0.5f, clr[0]); + EXPECT_FLOAT_EQ(0.6f, clr[1]); + EXPECT_FLOAT_EQ(0.7f, clr[2]); + EXPECT_FLOAT_EQ(0.8f, clr[3]); + EXPECT_TRUE(std::isnan(clr[4])); clr.Set(0.1f, 0.2f, 0.3f, 0.4f); clr = clr + 0.2f; @@ -328,6 +344,12 @@ TEST(Color, ConstAndSet) EXPECT_FLOAT_EQ(clr.B(), 0.3f); EXPECT_FLOAT_EQ(clr.A(), 0.4f); + EXPECT_FLOAT_EQ(clr.R(), clr[0]); + EXPECT_FLOAT_EQ(clr.G(), clr[1]); + EXPECT_FLOAT_EQ(clr.B(), clr[2]); + EXPECT_FLOAT_EQ(clr.A(), clr[3]); + EXPECT_TRUE(std::isnan(clr[4])); + math::Color clr2; clr2.R(0.4f); clr2.G(0.3f); diff --git a/src/python_pybind11/test/Color_TEST.py b/src/python_pybind11/test/Color_TEST.py index 0b2f09d0f..cc680de5a 100644 --- a/src/python_pybind11/test/Color_TEST.py +++ b/src/python_pybind11/test/Color_TEST.py @@ -172,10 +172,15 @@ def test_color(self): self.assertAlmostEqual(1.0, clr.a()) clr.set_from_hsv(300, 0.5, 1.0) - self.assertAlmostEqual(1.0, clr[0]) - self.assertAlmostEqual(0.5, clr[1]) - self.assertAlmostEqual(1.0, clr[2]) - self.assertAlmostEqual(1.0, clr[3]) + self.assertAlmostEqual(1.0, clr.r()) + self.assertAlmostEqual(0.5, clr.g()) + self.assertAlmostEqual(1.0, clr.b()) + self.assertAlmostEqual(1.0, clr.a()) + + self.assertAlmostEqual(clr.r(), clr[0]) + self.assertAlmostEqual(clr.g(), clr[1]) + self.assertAlmostEqual(clr.b(), clr[2]) + self.assertAlmostEqual(clr.a(), clr[3]) self.assertTrue(math.isnan(clr[4])) clr.set(0.1, 0.2, 0.3, 0.4) From 25b501858aab40011319544e3d20335e3c958e9a Mon Sep 17 00:00:00 2001 From: Nuno Guedelha Date: Thu, 9 Oct 2025 22:07:15 +0200 Subject: [PATCH 2/5] WIP: Refactor Color::operator[] Refactored Color::operator[] implementation (const version too) using a common private function, made possible by the use of const_cast<> for returning a non constant float&. Signed-off-by: Nuno Guedelha --- include/gz/math/Color.hh | 7 +++++++ src/Color.cc | 38 ++++++++++++++++---------------------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/include/gz/math/Color.hh b/include/gz/math/Color.hh index 9aa53e232..8cfdac93c 100644 --- a/include/gz/math/Color.hh +++ b/include/gz/math/Color.hh @@ -177,6 +177,13 @@ namespace gz::math /// returned if the _index is invalid public: float operator[](const unsigned int _index) const; + /// \brief Returns the Color component indexed by _index (accessible also through const object) + /// \param[in] _index Color component index(0=red, 1=green, 2=blue, + /// 3=alpha) + /// \return r, g, b, or a when _index is 0, 1, 2 or 3. A NAN_F value is + /// returned if the _index is invalid + private: const float &GetMutableRGBAfromIndex(const unsigned int _index) const; + /// \brief Get as uint32 RGBA packed value /// \return the color public: RGBA AsRGBA() const; diff --git a/src/Color.cc b/src/Color.cc index 21c8dff89..37998e85b 100644 --- a/src/Color.cc +++ b/src/Color.cc @@ -193,31 +193,19 @@ void Color::SetFromYUV(const float _y, const float _u, const float _v) ////////////////////////////////////////////////// float &Color::operator[](const unsigned int _index) { - switch (_index) - { - case 0: - return this->r; - case 1: - return this->g; - case 2: - return this->b; - case 3: - return this->a; - default: - break; - } - - // This allows referencing a NAN_F value. - static float invalidParam; - // Prevents overwriting of invalidParam with non NAN_F - // value through wrong index. - invalidParam = NAN_F; - - return invalidParam; + return const_cast( + static_cast(this)->GetMutableRGBAfromIndex(_index) + ); } ////////////////////////////////////////////////// float Color::operator[](const unsigned int _index) const +{ + return this->GetMutableRGBAfromIndex(_index); +} + +////////////////////////////////////////////////// +const float &Color::GetMutableRGBAfromIndex(const unsigned int _index) const { switch (_index) { @@ -233,7 +221,13 @@ float Color::operator[](const unsigned int _index) const break; } - return NAN_F; + // This allows referencing a NAN_F value. + static float invalidParam; + // Prevents overwriting of invalidParam with non NAN_F + // value through wrong index. + invalidParam = NAN_F; + + return invalidParam; } ////////////////////////////////////////////////// From 3011083e24f3ee32dcf6bbfc7b5be7eb9f78b4b0 Mon Sep 17 00:00:00 2001 From: Nuno Guedelha Date: Sat, 11 Oct 2025 14:46:49 +0200 Subject: [PATCH 3/5] Fixes after codecheck run - renamed GetMutableRGBAfromIndex - Fixed line length > 80 Signed-off-by: Nuno Guedelha --- include/gz/math/Color.hh | 5 +++-- src/Color.cc | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/include/gz/math/Color.hh b/include/gz/math/Color.hh index 8cfdac93c..1bdd1b86b 100644 --- a/include/gz/math/Color.hh +++ b/include/gz/math/Color.hh @@ -177,12 +177,13 @@ namespace gz::math /// returned if the _index is invalid public: float operator[](const unsigned int _index) const; - /// \brief Returns the Color component indexed by _index (accessible also through const object) + /// \brief Returns the Color component indexed by _index (accessible also + /// through const object) /// \param[in] _index Color component index(0=red, 1=green, 2=blue, /// 3=alpha) /// \return r, g, b, or a when _index is 0, 1, 2 or 3. A NAN_F value is /// returned if the _index is invalid - private: const float &GetMutableRGBAfromIndex(const unsigned int _index) const; + private: const float &GetRGBAfromIndex(const unsigned int _index) const; /// \brief Get as uint32 RGBA packed value /// \return the color diff --git a/src/Color.cc b/src/Color.cc index 37998e85b..8da83db2e 100644 --- a/src/Color.cc +++ b/src/Color.cc @@ -194,18 +194,18 @@ void Color::SetFromYUV(const float _y, const float _u, const float _v) float &Color::operator[](const unsigned int _index) { return const_cast( - static_cast(this)->GetMutableRGBAfromIndex(_index) + static_cast(this)->GetRGBAfromIndex(_index) ); } ////////////////////////////////////////////////// float Color::operator[](const unsigned int _index) const { - return this->GetMutableRGBAfromIndex(_index); + return this->GetRGBAfromIndex(_index); } ////////////////////////////////////////////////// -const float &Color::GetMutableRGBAfromIndex(const unsigned int _index) const +const float &Color::GetRGBAfromIndex(const unsigned int _index) const { switch (_index) { From 9085124c85a3ee5e8e463114b36222293129d1af Mon Sep 17 00:00:00 2001 From: Nuno Guedelha Date: Tue, 14 Oct 2025 12:20:57 +0200 Subject: [PATCH 4/5] Update migration guide Signed-off-by: Nuno Guedelha --- Migration.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Migration.md b/Migration.md index 305c099a1..3d73e92b6 100644 --- a/Migration.md +++ b/Migration.md @@ -5,6 +5,13 @@ Deprecated code produces compile-time warnings. These warning serve as notification to users that their code should be upgraded. The next major release will remove the deprecated code. +## Gazebo Math 10.X + +### Breaking Changes + +1. **Color.hh** + + Fix: return type and behaviour change of member function `Color::operator[](const unsigned int _index)`. Now behaves like a mutator function, returning a mutable reference to the `Color` component (`float&`) instead of just the component value (`float`). In case of wrong index input, function returns a reference to `NAN_F`, and assigning any value to it has no effect. Refer to [#701](https://github.com/gazebosim/gz-math/pull/701) for further details. + ## Gazebo Math 8.X to 9.X 1. **SphericalCoordinates.hh** From 2ddbb96a5e23848fd846cf359b2c2a6c46172806 Mon Sep 17 00:00:00 2001 From: Nuno Guedelha Date: Fri, 17 Oct 2025 16:10:18 +0200 Subject: [PATCH 5/5] Add Python bindings allowing assignment through operator [] Signed-off-by: Nuno Guedelha --- src/python_pybind11/src/Color.cc | 38 +++++++++++++++++--------- src/python_pybind11/test/Color_TEST.py | 11 ++++++++ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/python_pybind11/src/Color.cc b/src/python_pybind11/src/Color.cc index 9428fa26a..2dee0fd33 100644 --- a/src/python_pybind11/src/Color.cc +++ b/src/python_pybind11/src/Color.cc @@ -116,29 +116,33 @@ void defineMathColor(py::module &m, const std::string &typestr) &Class::SetFromABGR, "Set from uint32 ABGR packed value") .def("r", - py::overload_cast<>(&Class::R), + py::overload_cast<>(&Class::R, py::const_), "Get the red value") .def("g", - py::overload_cast<>(&Class::G), + py::overload_cast<>(&Class::G, py::const_), "Get the green value") .def("b", - py::overload_cast<>(&Class::B), + py::overload_cast<>(&Class::B, py::const_), "Get the blue value") .def("a", - py::overload_cast<>(&Class::A), + py::overload_cast<>(&Class::A, py::const_), "Get the alpha value") .def("r", - py::overload_cast(&Class::R), - "Get the red value") + py::overload_cast(&Class::R), + "Set the red value", + "_r"_a) .def("g", - py::overload_cast(&Class::G), - "Get the green value") + py::overload_cast(&Class::G), + "Set the green value", + "_g"_a) .def("b", - py::overload_cast(&Class::B), - "Get the blue value") + py::overload_cast(&Class::B), + "Set the blue value", + "_b"_a) .def("a", - py::overload_cast(&Class::A), - "Get the alpha value") + py::overload_cast(&Class::A), + "Set the alpha value", + "_a"_a) .def("__str__", toString) .def("__repr__", toString) .def("__copy__", [](const Class &self) { @@ -148,7 +152,15 @@ void defineMathColor(py::module &m, const std::string &typestr) return Class(self); }, "memo"_a) .def("__getitem__", - py::overload_cast(&Class::operator[])); + py::overload_cast(&Class::operator[], py::const_), + "Array index operator, const version. Get the indexed component value", + "_index"_a) + .def("__setitem__", + [](Class &self, const unsigned int _index, const float value) { + self[_index] = value; + }, + "Array index operator. Set the indexed component value", + "_index"_a, "value"_a); } } // namespace python } // namespace math diff --git a/src/python_pybind11/test/Color_TEST.py b/src/python_pybind11/test/Color_TEST.py index cc680de5a..3afb3afd8 100644 --- a/src/python_pybind11/test/Color_TEST.py +++ b/src/python_pybind11/test/Color_TEST.py @@ -183,6 +183,17 @@ def test_color(self): self.assertAlmostEqual(clr.a(), clr[3]) self.assertTrue(math.isnan(clr[4])) + clr[0] = 0.5 + clr[1] = 0.6 + clr[2] = 0.7 + clr[3] = 0.8 + clr[4] = 0.9 + self.assertAlmostEqual(0.5, clr[0]) + self.assertAlmostEqual(0.6, clr[1]) + self.assertAlmostEqual(0.7, clr[2]) + self.assertAlmostEqual(0.8, clr[3]) + self.assertTrue(math.isnan(clr[4])) + clr.set(0.1, 0.2, 0.3, 0.4) clr = clr + 0.2 self.assertTrue(clr == Color(0.3, 0.4, 0.5, 0.6))