Color [] changed to call by reference #579

Avisheet · 2024-02-21T19:26:45Z

Summary

The issue you're describing is related to the operator[] overload for the Color class. The problem is that the operator is declared to return a float instead of a reference to a float. As a result, when you use the operator to access a component of the color and attempt to mutate it, the mutation doesn't apply to the actual color instance.

To fix this issue, you should modify the declaration of the operator[] to return a reference to a float. Here's the corrected declaration:

public: float& operator[](const unsigned int _index);

This modification allows you to use the operator to both access and mutate the individual components of the Color class.

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 (optional)

azeey · 2024-02-22T00:49:24Z

Hello @Avisheet! Thanks for the contribution. The change you made in the header looks correct, but it requires a corresponding change in Color.cc. Would you be able to make that change? Also, could you add a unit test?

scpeters · 2024-02-22T03:09:03Z

we would probably need to retarget to main as well since this breaks API, unless we add an operator overload instead of changing the current one

Avisheet · 2024-02-22T03:48:48Z

Hello @azeey .
Thank you for your feedback. I've made the necessary changes in Color.cc to align with the modification in Color.hh. Additionally, I have added a corresponding unit test to ensure the functionality is working as expected.

Please review the changes and let me know if everything looks good to you. If you have any further suggestions or if there's anything else you'd like me to address, please feel free to let me know.

Avisheet · 2024-02-22T03:49:43Z

hello @scpeters , Can you elaborate what you are telling as i am unable to understand it properly

Avisheet · 2024-02-22T08:14:22Z

@azeey I have done some more changes in the Vector3_TEST.cc file to ensure that we get an expected result as "Operators can be used to [strictly order] vectors."Do check them once and tell me the modifications I need to do if any.

azeey · 2024-02-22T15:30:00Z

@Avisheet I've added a checklist to your PR description; it should have been prepopulated when you created the PR unless you removed it. Please make sure to complete all relevant items on the checklist.

Avisheet · 2024-02-22T19:45:01Z

Hey @azeey . The existing behavior is that we return NAN on unknown indices. But if we want to support assignment then it cannot return any such const rvalue . so, we probably have to break that existing functionality . How should we handle that ?
As a result of this behavior we are getting failure in 2 test cases.

Signed-off-by: Avisheet <[email protected]>

Avisheet · 2024-03-01T15:50:30Z

@azeey can you review it and provide me a feedback . So that I can work more in it according to that.

Signed-off-by: Avisheet <[email protected]>

azeey · 2024-03-01T19:41:01Z

.vscode/c_cpp_properties.json

Please remove this file.

azeey · 2024-03-01T19:41:07Z

.vscode/settings.json

Please remove this file.

azeey · 2024-03-01T19:41:46Z

include/gz/math/Color.hh


      /// \brief Array index operator, const version
      /// \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
-      public: float operator[](const unsigned int _index) const;
+      public: const float& operator[](const unsigned int _index) const;


It's better to return a POD type by value, so please revert this change.

src/Color.cc

azeey · 2024-03-01T19:50:00Z

src/Color.cc

 }

 //////////////////////////////////////////////////
-float Color::operator[](const unsigned int _index) const
+const float& Color::operator[](const unsigned int _index) const


Please revert this per my comment in the header.

azeey · 2024-03-01T19:51:45Z

src/python_pybind11/test/Color_TEST.py

@@ -176,7 +176,7 @@ def test_color(self):
        self.assertAlmostEqual(0.5, clr[1])
        self.assertAlmostEqual(1.0, clr[2])
        self.assertAlmostEqual(1.0, clr[3])
-        self.assertTrue(math.isnan(clr[4]))
+        # self.assertTrue(math.isnan(clr[4]))


azeey · 2024-03-01T19:52:16Z

src/Color_TEST.cc

+//     // this tests _that_ the expected exception is thrown
+    EXPECT_THROW({
+        try
+        {
+            clr[4] = 0.1f;
+        }
+        catch( const std::runtime_error& e )
+        {
+            // and this tests that it has the correct message
+            EXPECT_STREQ( "Index Error: Color index out of range", e.what() );
+            throw;
+        }
+    }, std::runtime_error);


I think it would be sufficient to check that clr[4] is a NAN here.

Signed-off-by: Avisheet <[email protected]>

arjo129 · 2024-08-06T04:47:11Z

Hi @Avisheet, was wondering what the status on this was. Do we intend shipping this change with ionic (code freeze 26/8)? If we can't come to a consensus by then I will remove the beta label.

Please address @azeey's feedback on PODs.

adityapande-1995 · 2024-08-06T05:02:22Z

src/Color.cc

@@ -16,6 +16,7 @@
 */
 #include <cmath>
 #include <algorithm>
+#include<iostream>


Do we need to include this ?

arjo129 · 2024-08-19T07:01:25Z

Hi @Avisheet as we haven't heard back from you, I'm removing the beta label. Both @adityapande-1995 and @azeey 's remarks need addressing.

arjo129 · 2024-08-19T07:35:35Z

Hey @Avisheet you don't have to close the PR if you'd like we can merge this in after the code freeze if you can't address it immediately.

Avisheet · 2024-08-19T07:54:55Z

Hey @arjo129 , sorry for the inconvenience, but as I have some of my major projects going on, I won't be able to contribute to it for 1 more month.

arjo129 · 2024-08-19T07:57:23Z

No worries. I can reopen this we will merge it after your commitments! Its just that we will be putting it in the next gazebo release. Let me know if that works for you or if you don;t want to further pursue this that is also fine.

Avisheet · 2024-08-19T08:01:33Z

@arjo129 Yep, it's fine to release it with another gazebo release. I will be working on it for sure, but it's just I was a bit stuck with my major project.

Avisheet · 2024-09-25T09:04:01Z

Hey, I had a doubt that how can I check and run all the test cases on my own device, before giving the PR, so that I can know where the error is, after doing some changes in code.

Avisheet requested review from scpeters and adityapande-1995 as code owners February 21, 2024 19:26

github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Feb 21, 2024

Avisheet force-pushed the gz-math7 branch 2 times, most recently from 608965d to 6f75941 Compare February 22, 2024 15:14

Avisheet force-pushed the gz-math7 branch from 376a192 to 82a2bff Compare February 22, 2024 19:18

Avisheet added 6 commits February 24, 2024 11:33

Changed to call by reference

d5202b1

Signed-off-by: Avisheet <[email protected]>

Added the test case and modified the Color.cc with required changes.

d550a33

Signed-off-by: Avisheet <[email protected]>

Added a test case in Vector3_TEST.cc to get the expected behaviour

59a0f14

Signed-off-by: Avisheet <[email protected]>

changes

8972884

Signed-off-by: Avisheet <[email protected]>

changes applied

719dfe2

Signed-off-by: Avisheet <[email protected]>

errors resolved

a81c769

Signed-off-by: Avisheet <[email protected]>

Avisheet force-pushed the gz-math7 branch from 5b89c03 to a81c769 Compare February 24, 2024 06:03

added whitespace

c7b287d

Signed-off-by: Avisheet <[email protected]>

azeey requested changes Mar 1, 2024

View reviewed changes

Applying changes

39a5c15

Signed-off-by: Avisheet <[email protected]>

Avisheet force-pushed the gz-math7 branch from 3582a56 to 39a5c15 Compare March 11, 2024 03:48

Avisheet mentioned this pull request Apr 4, 2024

non-const Color::operator[] should return mutable reference #575

Open

azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024

adityapande-1995 reviewed Aug 6, 2024

View reviewed changes

scpeters changed the title ~~Changed to call by reference~~ Color [] changed to call by reference Aug 14, 2024

arjo129 removed the beta Targeting beta release of upcoming collection label Aug 19, 2024

Avisheet closed this Aug 19, 2024

arjo129 reopened this Aug 19, 2024

arjo129 removed 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color [] changed to call by reference #579

Color [] changed to call by reference #579

Avisheet commented Feb 21, 2024 •

edited

Loading

azeey commented Feb 22, 2024

scpeters commented Feb 22, 2024

Avisheet commented Feb 22, 2024

Avisheet commented Feb 22, 2024

Avisheet commented Feb 22, 2024

azeey commented Feb 22, 2024

Avisheet commented Feb 22, 2024 •

edited

Loading

Avisheet commented Mar 1, 2024 •

edited

Loading

azeey Mar 1, 2024

azeey Mar 1, 2024

azeey Mar 1, 2024

azeey Mar 1, 2024

azeey Mar 1, 2024

azeey Mar 1, 2024

arjo129 commented Aug 6, 2024

adityapande-1995 Aug 6, 2024

arjo129 commented Aug 19, 2024

arjo129 commented Aug 19, 2024

Avisheet commented Aug 19, 2024

arjo129 commented Aug 19, 2024

Avisheet commented Aug 19, 2024

Avisheet commented Sep 25, 2024

Color [] changed to call by reference #579

Are you sure you want to change the base?

Color [] changed to call by reference #579

Conversation

Avisheet commented Feb 21, 2024 • edited Loading

Summary

Checklist

azeey commented Feb 22, 2024

scpeters commented Feb 22, 2024

Avisheet commented Feb 22, 2024

Avisheet commented Feb 22, 2024

Avisheet commented Feb 22, 2024

azeey commented Feb 22, 2024

Avisheet commented Feb 22, 2024 • edited Loading

Avisheet commented Mar 1, 2024 • edited Loading

azeey Mar 1, 2024

Choose a reason for hiding this comment

azeey Mar 1, 2024

Choose a reason for hiding this comment

azeey Mar 1, 2024

Choose a reason for hiding this comment

azeey Mar 1, 2024

Choose a reason for hiding this comment

azeey Mar 1, 2024

Choose a reason for hiding this comment

azeey Mar 1, 2024

Choose a reason for hiding this comment

arjo129 commented Aug 6, 2024

adityapande-1995 Aug 6, 2024

Choose a reason for hiding this comment

arjo129 commented Aug 19, 2024

arjo129 commented Aug 19, 2024

Avisheet commented Aug 19, 2024

arjo129 commented Aug 19, 2024

Avisheet commented Aug 19, 2024

Avisheet commented Sep 25, 2024

Avisheet commented Feb 21, 2024 •

edited

Loading

Avisheet commented Feb 22, 2024 •

edited

Loading

Avisheet commented Mar 1, 2024 •

edited

Loading