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

use specified color_ for EndEffector marker #96

Merged

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented Aug 11, 2021

Related to moveit/moveit#2792 .

Addresses this clang warning

<<<
moveit_visual_tools/include/moveit_visual_tools/imarker_end_effector.h:150:29: error: private field 'color_' is not used [-Werror,-Wunused-private-field]
rviz_visual_tools::colors color_ = rviz_visual_tools::PURPLE;
^
1 error generated.

We could have removed the member just as well (it barely matters...),
but that would entail an unnecessary API change in the constructor.

I added the default value for the constructor parameter as it is always overwritten until now...

Addresses this clang warning

<<<
moveit_visual_tools/include/moveit_visual_tools/imarker_end_effector.h:150:29: error: private field 'color_' is not used [-Werror,-Wunused-private-field]
  rviz_visual_tools::colors color_ = rviz_visual_tools::PURPLE;
                            ^
1 error generated.
>>>

We could have removed the member just as well (it barely matters...),
but that would entail an unnecessary API change in the constructor.

I added the default value for the constructor parameter as it is always overwritten until now...
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

To begin with, an issue external to the original code base shouldn't result in a failure of CI 😉

Comment on lines -315 to -317
marker.color.r = 0.5;
marker.color.g = 0.5;
marker.color.b = 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't purple but grey...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't see an issue with this (tiny) change in behavior. If you disagree please change it before merging 🐈

@v4hn
Copy link
Contributor Author

v4hn commented Aug 11, 2021

To begin with, an issue external to the original code base shouldn't result in a failure of CI 😉

Yes and no. I believe the best way to go forward upon finding an issue is by fixing it. 😄
Also, I know what you mean, but these are not really altogether external.

Feel free to patch the MoveIt CI further to be less strict on downstream dependencies if you like.
As they are all under our control, I prefer to keep -Werror etc there to get notified if downstream shows new warnings due to MoveIt-related changes.

@tylerjw
Copy link
Member

tylerjw commented Aug 12, 2021

The specific color could be change in a follow on PR. I am going to merge these PRs so we can get CI working again for moveit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants