Fix non-const Color::operator[] to return mutable reference

[nunoguedelha]

🦟 Bug fix

Fixes #575

Summary

This fixes the issue by having the operator[] function (non const version) returning a reference to the Color component, of type float&, instead of just the component value, of type float. This PR takes in account all the discussion thread in PR #579 which was previously closed.

The fix consists in two main steps:

• The first commit implements the desired behaviour and unit tests.
• The second commit refactors the operator[] function implementation for avoiding code duplication.

Desired Behaviour

• We need the non-const version of the operator[] function to return a reference to a float. Since functions can not be overloaded if they differ only in the return type, we get the desired behaviour by changing the prototype of the existing function. Although the impact is minor, this breaks API so the change is branched off the mainbranch.
• The implementation is almost identical to the const version, except for the use of a local static variable allowing a NAN_F value to be referenced from the caller scope, in case of a wrong _index input (refer to @azeey comment in Color [] changed to call by reference #579 (comment)_):

  static float invalidParam = NAN_F;
  return invalidParam;
  

  But since this variable can be rewritten while we invoke the operator with the wrong index, we declare it and set it in two steps, otherwise the example below would first output NAN_F, then 0.9.

  math::Color clr(.1f, .2f, .3f, 1.0f);
  cout << clr[4];
  clr[4] = 0.9f;
  cout << clr[4];
  

  a test was added for this behaviour.

Unit Tests

• I split the tests on operator[] and clr.SetFromHSV, so the last check on clr.SetFromHSV(300, 0.5, 1.0) is done using R(), G(), B(), A() accessors, etc.
• I tested the case where we try to mutate a Color component using the wrong index.
• The other added tests are similar to what was already proposed in Color [] changed to call by reference #579 .

Refactoring

We can't have the common behaviour of both mutable and const versions of operator[] implemented:

• in operator[]() const and somehow called by operator[]() since the first returns a value, not a reference;
• neither in operator[]() and called by operator[]() const since the first would then have to be declared const.

The easiest solution is to:

• implement the behaviour in a private function declared as const and returning a const float&;
• use a const_cast<float&> to cast the returned const reference to a non-const.

How to Reproduce

If we try the following snippet in the namespace gz:

math::Color clr(.1f, .2f, .3f, 1.0f);
clr[0] = 0.5f;

The result is changed by the fix…

Before Fix

The snippet won’t compile and instead return the error: “expression is not assignable”, as the left expression is an rvalue.

After Fix

The snippet will compile and the Color object component _r will be changed.
In addition:

math::Color clr(.1f, .2f, .3f, 1.0f);
cout << clr[4];
clr[4] = 0.9f;
cout << clr[4];

will output NAN_F twice (even after overwriting clr[4]).

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers
• Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

[nunoguedelha]

Regarding codecheckand test coverage

This PR doesn't add further codecheck errors nor warnings to the ones existing already in the mainbranch.
Same code coverage tests passed as on the mainbranch ().

[azeey]

Thanks for the contribution! The changes look good to me. Do you mind adding a line in the Migration guide to indicate that float& Color::operator[](const unsigned int _index) now returns a reference?

[nunoguedelha]

Sure @azeey . On it..

[nunoguedelha]

Thanks for the contribution! The changes look good to me. Do you mind adding a line in the Migration guide to indicate that float& Color::operator[](const unsigned int _index) now returns a reference?

@azeey , since this change breaks API it would go into a major version, so 10x right? In such case I need to add a section Gazebo Math 10.X above Gazebo Math 8.X to 9.X... please let me know.

[azeey]

Yes, please add a new section. Thanks!

[nunoguedelha]

@azeey , done!

[nunoguedelha]

@azeey , side note regarding the Ruby and Python bindings...

Currently, it seems to me that:

• there are no Ruby tests for the Color class;
• there are no Ruby bindings for Color::operator[](...), neither const neither non-const;
• Python bindings do not support the mutators float &Color::R(), float &Color::G(), ....

I know that Pybind11 supports functions returning references, but since it is not implemented for float &Color::R() anyway, and I have no idea on how to implement it right away, I didn't even try to wrap float &Color::operator[](...).

I would say this deserves a separate PR. What do you think @azeey ?

[azeey]

You can skip the Ruby tests/bindings. We are more focused on Python these days. But I was under the impression that mutation of the individual colors was implemented via

gz-math/src/python_pybind11/src/Color.cc

Lines 118 to 129 in 77f7c04

	.def("r",
	py::overload_cast<>(&Class::R),
	"Get the red value")
	.def("g",
	py::overload_cast<>(&Class::G),
	"Get the green value")
	.def("b",
	py::overload_cast<>(&Class::B),
	"Get the blue value")
	.def("a",
	py::overload_cast<>(&Class::A),
	"Get the alpha value")

For float &Color::operator[](...), I think you can just follow the patterh from

gz-math/src/python_pybind11/src/Color.cc

Lines 150 to 151 in 77f7c04

	.def("__getitem__",
	py::overload_cast<const unsigned int>(&Class::operator[]));

And provide the non-const version to py::overload_cast

[nunoguedelha]

You can skip the Ruby tests/bindings. We are more focused on Python these days. But I was under the impression that mutation of the individual colors was implemented via

gz-math/src/python_pybind11/src/Color.cc

Lines 118 to 129 in 77f7c04

	.def("r",
	py::overload_cast<>(&Class::R),
	"Get the red value")
	.def("g",
	py::overload_cast<>(&Class::G),
	"Get the green value")
	.def("b",
	py::overload_cast<>(&Class::B),
	"Get the blue value")
	.def("a",
	py::overload_cast<>(&Class::A),
	"Get the alpha value")

Considering the behaviour I've observed, the bindings you mentioned above are for the read-only accessors, right? Indeed the following test example:

clr.set(0.1, 0.2, 0.3, 0.4)
self.assertAlmostEqual(0.1, clr.r())
self.assertAlmostEqual(0.2, clr.g())
self.assertAlmostEqual(0.3, clr.b())
self.assertAlmostEqual(0.4, clr.a())
print("clr.r() =", clr.r(), "clr.g() =", clr.g(), "clr.b() =", clr.b(), "clr.a() =", clr.a())

...will pass and output:

clr.r() = 0.10000000149011612 clr.g() = 0.20000000298023224 clr.b() = 0.30000001192092896 clr.a() = 0.4000000059604645

But then, mutation won't work:

clr.r() = 0.5

...will fail and output:

136:     clr.r() = 0.5
136:     ^^^^^^^
136: SyntaxError: cannot assign to function call here. Maybe you meant '==' instead of '='?
Failed

For float &Color::operator[](...), I think you can just follow the patterh from

gz-math/src/python_pybind11/src/Color.cc

Lines 150 to 151 in 77f7c04

	.def("__getitem__",
	py::overload_cast<const unsigned int>(&Class::operator[]));

And provide the non-const version to py::overload_cast

Actually, I think this is already the non-const version, since the const version should have been defined with the py::const_ argument (std::true_type) as follows:

.def("__getitem__",
     py::overload_cast<const unsigned int>(&Class::operator[], py::const_));

Using the original non-const version to mutate the component will also fail:

136: Traceback (most recent call last):
136:   File "/Users/nunoguedelha/dev/gz-math/src/python_pybind11/test/Color_TEST.py", line 188, in test_color
136:     clr[0] = 0.6
136:     ~~~^^^
136: TypeError: 'gz.math.Color' object does not support item assignment

...probably because we need to define a binding through __setitem__.

I'm looking deeper into __setitem__ now...