Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color [] changed to call by reference #579

Open
wants to merge 8 commits into
base: gz-math7
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .vscode/c_cpp_properties.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file.

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"configurations": [
{
"name": "Linux",
"includePath": [
"${workspaceFolder}/**"
],
"defines": [],
"compilerPath": "/usr/bin/gcc",
"cStandard": "c17",
"cppStandard": "gnu++17",
"intelliSenseMode": "linux-gcc-x64"
}
],
"version": 4
}
7 changes: 7 additions & 0 deletions .vscode/settings.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"files.associations": {
"ostream": "cpp",
"stdexcept": "cpp",
"cmath": "cpp"
}
}
4 changes: 2 additions & 2 deletions include/gz/math/Color.hh
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,14 @@ namespace gz
/// 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,
/// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// \brief Get as uint32 RGBA packed value
/// \return the color
Expand Down
22 changes: 19 additions & 3 deletions src/Color.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
#include <cmath>
#include <algorithm>
#include<iostream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to include this ?


#include "gz/math/Color.hh"

Expand Down Expand Up @@ -187,13 +188,28 @@ 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<const Color *>(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;
}

std::cerr << "Trying to read index " << _index << " of Color" << std::endl;
throw std::runtime_error("Index Error: Color index out of range");
Avisheet marked this conversation as resolved.
Show resolved Hide resolved
}

//////////////////////////////////////////////////
float Color::operator[](const unsigned int _index) const
const float& Color::operator[](const unsigned int _index) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this per my comment in the header.

{
switch (_index)
{
Expand Down
31 changes: 30 additions & 1 deletion src/Color_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ TEST(Color, Color)
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_TRUE(std::isnan(clr[4]));

clr.R() = 0.1f;
clr.G() = 0.2f;
Expand Down Expand Up @@ -433,3 +433,32 @@ TEST(Color, HSV)
EXPECT_NEAR(clr.B(), 0.3f, 1e-3);
EXPECT_NEAR(clr.A(), 1.0, 1e-3);
}

TEST(Color, OperatorIndex){
math::Color clr;
clr[0] = 0.1f;
clr[1] = 0.2f;
clr[2] = 0.3f;
clr[3] = 0.4f;

EXPECT_FLOAT_EQ(clr[0], 0.1f);
EXPECT_FLOAT_EQ(clr[1], 0.2f);
EXPECT_FLOAT_EQ(clr[2], 0.3f);
EXPECT_FLOAT_EQ(clr[3], 0.4f);

// const math::Color constClr = clr;

// // 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
16 changes: 16 additions & 0 deletions src/Vector3_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -576,3 +576,19 @@ TEST(Vector3dTest, OperatorStreamOut)
stream << std::setprecision(1) << std::fixed << v;
EXPECT_EQ(stream.str(), "0.1 1.2 2.3");
}

TEST(Vector3dTest, OperatorLessThan) {
math::Vector3d vec1(1.0, 2.0, 3.0);
math::Vector3d vec2(1.0, 2.0, 4.0);

// Test the less than operator
EXPECT_TRUE(vec1 < vec2);
EXPECT_FALSE(vec2 < vec1);
EXPECT_FALSE(vec1 < vec1);

// Additional test for asymmetric behavior
math::Vector3d vec3(1.0, 2.0, 2.0);
math::Vector3d vec4(1.0, 2.0, 4.0);
EXPECT_TRUE(vec3 < vec4);
EXPECT_FALSE(vec4 < vec3);
}
2 changes: 1 addition & 1 deletion src/python_pybind11/test/Color_TEST.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert


clr.set(0.1, 0.2, 0.3, 0.4)
clr = clr + 0.2
Expand Down