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 attached object colors as is in Rviz plugin #3274

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

ANogin
Copy link
Contributor

@ANogin ANogin commented Jan 27, 2025

Description

Fixes #3273. As explained in #3273, there is some weird code from 2013 that messes up the colors of attached objects in rviz, overriding the colors provided via the planning scene interface with potentially out of range values. This removes the erroneous code and just passes the colors as is.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@sea-bass sea-bass added backport-humble Mergify label that triggers a PR backport to Humble backport-jazzy Mergify label that triggers a PR backport to Jazzy labels Jan 27, 2025
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.

I'm curious under which circumstances this bug manifested itself. I never noticed a fully-white attached body.
Anyway, given that fact, that nobody noticed that issue yet, we can safely use the original color.

rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Jan 27, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 45.58%. Comparing base (524ca4f) to head (3365d14).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...gin_render_tools/src/robot_state_visualization.cpp 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3274      +/-   ##
==========================================
- Coverage   45.98%   45.58%   -0.39%     
==========================================
  Files         716      716              
  Lines       62395    62372      -23     
  Branches     7544     7542       -2     
==========================================
- Hits        28683    28426     -257     
- Misses      33545    33780     +235     
+ Partials      167      166       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sea-bass sea-bass added this pull request to the merge queue Jan 27, 2025
@rr-mark
Copy link

rr-mark commented Jan 27, 2025

I'm curious under which circumstances this bug manifested itself. I never noticed a fully-white attached body. Anyway, given that fact, that nobody noticed that issue yet, we can safely use the original color.

I think we were previously seeing this problem, but had a workaround to avoid it (see e.g. https://robotics.stackexchange.com/questions/113543/how-can-attached-collision-objects-be-recoloured-in-rviz2).

It's relatively hard to change the colour of an attached collision object away from default, so I'm not surprised not many people ran into this behaviour.

@rhaschke
Copy link
Contributor

Now, I'm even more surprised: as MoveIt1 used the same code, I'm wondering why it didn't display the attached object as white.

Merged via the queue into moveit:main with commit 870b23d Jan 27, 2025
9 checks passed
mergify bot pushed a commit that referenced this pull request Jan 27, 2025
Fixes #3273

Co-authored-by: Sebastian Castro <[email protected]>
(cherry picked from commit 870b23d)
mergify bot pushed a commit that referenced this pull request Jan 27, 2025
Fixes #3273

Co-authored-by: Sebastian Castro <[email protected]>
(cherry picked from commit 870b23d)
sea-bass pushed a commit that referenced this pull request Jan 27, 2025
(cherry picked from commit 870b23d)

Co-authored-by: Aleksey Nogin <[email protected]>
@rr-mark
Copy link

rr-mark commented Jan 27, 2025

Now, I'm even more surprised: as MoveIt1 used the same code, I'm wondering why it didn't display the attached object as white.

The behaviour we saw before our workaround was that attached collision objects would be the default colour until recoloured, at which point they would be white for any valid colour input.

Our workaround is to have a separate node listening to moveit's planning scene, recolouring all the objects, and republishing to rviz. Hopefully that won't be needed after we start using this PR.

sea-bass pushed a commit that referenced this pull request Jan 27, 2025
(cherry picked from commit 870b23d)

Co-authored-by: Aleksey Nogin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble backport-jazzy Mergify label that triggers a PR backport to Jazzy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rviz plugin overrides attached object colors incorrectly.
5 participants