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

Add option to change material color from ROS. #486

Merged
merged 21 commits into from
Feb 1, 2024

Conversation

bperseghetti
Copy link
Member

@bperseghetti bperseghetti commented Jan 14, 2024

🎉 New feature

Summary

Allows for setting a GZ entities visual material color from ROS. This is highly valuable for creating noticeable LED status lighting. And depends on associated gz-msgs.

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

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

This allows bridging MaterialColor from ROS to GZ and is
important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

can you add a test?

ros_gz_interfaces/msg/MaterialColor.msg Outdated Show resolved Hide resolved
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti
Copy link
Member Author

bperseghetti commented Jan 15, 2024

can you add a test?

Yes, I was planning to wait and make sure this change was desired in upstream before spending any extra time to add a test.
Also to make testing work properly with CI will need to get gazebosim/gz-msgs#414 in first anyhow. This was more just to show the interdependent changes.

@bperseghetti
Copy link
Member Author

@ahcorde Test added.

Test for the MaterialColor message ROS<->GZ.

Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti bperseghetti requested a review from ahcorde January 16, 2024 05:32
@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Jan 16, 2024
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

you included some changes related with renaming ign to gz. Do you mind to remove them?

@bperseghetti
Copy link
Member Author

you included some changes related with renaming ign to gz. Do you mind to remove them?

So that readme is in the ros_gz_bridge instead of the ros_ign_bridge, should it not be changed to match the naming?

@azeey
Copy link
Contributor

azeey commented Jan 17, 2024

So that readme is in the ros_gz_bridge instead of the ros_ign_bridge, should it not be changed to match the naming?

I think we should stick with ign since Humble is officially paired with Fortress. It's confusing for users since trying to use gz doesn't work in Humble/Fortress.

@bperseghetti
Copy link
Member Author

So that readme is in the ros_gz_bridge instead of the ros_ign_bridge, should it not be changed to match the naming?

I think we should stick with ign since Humble is officially paired with Fortress. It's confusing for users since trying to use gz doesn't work in Humble/Fortress.

But to be clear this is not the main README but this is the one inside ros_gz_bridge. The one inside ros_ign_bridge should just say it's a shim on the other and have ign listed in it, right?

I guess I should just swap them.

Signed-off-by: Benjamin Perseghetti <[email protected]>
@azeey
Copy link
Contributor

azeey commented Jan 17, 2024

But to be clear this is not the main README but this is the one inside ros_gz_bridge. The one inside ros_ign_bridge should just say it's a shim on the other and have ign listed in it, right?

https://github.com/gazebosim/ros_gz/tree/humble/ros_ign_bridge does say it's a shim

@bperseghetti
Copy link
Member Author

But to be clear this is not the main README but this is the one inside ros_gz_bridge. The one inside ros_ign_bridge should just say it's a shim on the other and have ign listed in it, right?

https://github.com/gazebosim/ros_gz/tree/humble/ros_ign_bridge does say it's a shim

Well I did also just change that so you know it's a shim but it has different interface and that gives you the interface of it.

azeey added a commit to gazebosim/gz-sim that referenced this pull request Jan 25, 2024
This change allows for a simpler mapping from `MaterialColor` msg in gz to the much more cumbersome Visual msg. By doing this and adding a topic listener it allows users to rapidly change the colors of a entities visual material color components, including from the [ROS side using the gz bridge](gazebosim/ros_gz#486). This is especially useful for simulating status lighting.


---------

Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
@bperseghetti bperseghetti requested a review from ahcorde January 26, 2024 16:24
@bperseghetti
Copy link
Member Author

@osrf-jenkins run tests please

@azeey
Copy link
Contributor

azeey commented Jan 26, 2024

This PR is targeting humble which needs to work with Fortress, but the gz-msgs and gz-sim that landed recently all targeted Harmonic. I recommend retargeting this to rolling.

@azeey
Copy link
Contributor

azeey commented Jan 26, 2024

Alternatively, you can do something like

// Dataframe is available from versions 8.4.0 (fortress) forward
// This can be removed when the minimum supported version passes 8.4.0
#if (IGNITION_MSGS_MAJOR_VERSION > 8) || \
((IGNITION_MSGS_MAJOR_VERSION == 8) && (IGNITION_MSGS_MINOR_VERSION >= 4))
#define HAVE_DATAFRAME true
#endif
#if (GZ_MSGS_MAJOR_VERSION > 8) || \
((GZ_MSGS_MAJOR_VERSION == 8) && (GZ_MSGS_MINOR_VERSION >= 4))
#define HAVE_DATAFRAME true
#endif

Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@azeey azeey removed the needs upstream release Blocked by a release of an upstream library label Feb 1, 2024
Signed-off-by: Benjamin Perseghetti <[email protected]>
azeey and others added 3 commits January 31, 2024 20:28
Signed-off-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

linters

"ros_gz_bridge" "--output-file" "/github/home/ws/build/ros_gz_bridge/ament_flake8/flake8.txt" "--command" "/opt/ros/rolling/bin/ament_flake8" "--xunit-file" "/github/home/ws/build/ros_gz_bridge/test_results/ros_gz_bridge/flake8.xunit.xml"
2024-02-01T03:46:02.0622501Z 10: Test timeout computed to be: 60
2024-02-01T03:46:02.1111404Z 10: -- run_test.py: invoking following command in '/github/home/ws/src/ros_gz/ros_gz_bridge':
2024-02-01T03:46:02.1112566Z 10:  - /opt/ros/rolling/bin/ament_flake8 --xunit-file /github/home/ws/build/ros_gz_bridge/test_results/ros_gz_bridge/flake8.xunit.xml
2024-02-01T03:46:02.4909897Z 10: from ros_gz_bridge.mappings import MAPPINGS, MAPPINGS_8_4_0, MAPPINGS_10_1_0
2024-02-01T03:46:02.4911413Z 10: ^
2024-02-01T03:46:02.4913079Z 10: 1     I101 Imported names are in the wrong order. Should be MAPPINGS, MAPPINGS_10_1_0, MAPPINGS_8_4_0
2024-02-01T03:46:02.4914637Z 10: 
2024-02-01T03:46:02.4916618Z 10: ./ros_gz_bridge/__init__.py:19:1: I101 Imported names are in the wrong order. Should be MAPPINGS, MAPPINGS_10_1_0, MAPPINGS_8_4_0
2024-02-01T03:46:02.4918587Z 10: 
2024-02-01T03:46:02.4919517Z 10: 
2024-02-01T03:46:02.4921177Z 10: 5 files checked
2024-02-01T03:46:02.4922615Z 10: 1 errors
2024-02-01T03:46:02.4924329Z 10: 
2024-02-01T03:46:02.4926578Z 10: 'I'-type errors: 1
2024-02-01T03:46:02.4928273Z 10: 
2024-02-01T03:46:02.4929977Z 10: Checked files:

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti bperseghetti requested a review from ahcorde February 1, 2024 14:40
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

@azeey any other comment?

@azeey azeey merged commit b5dffde into gazebosim:humble Feb 1, 2024
9 checks passed
@azeey
Copy link
Contributor

azeey commented Feb 1, 2024

@bperseghetti could you please cherry-pick this into iron and ros2 branches?

@bperseghetti
Copy link
Member Author

@bperseghetti could you please cherry-pick this into iron and ros2 branches?

Yep, I can do it later today!

@ahcorde
Copy link
Collaborator

ahcorde commented Feb 19, 2024

friendly ping @bperseghetti do you have any plans to forward port these changes?

@bperseghetti
Copy link
Member Author

Sorry, my bad, I let this one slip through the cracks on my side, will create the PRs after my meetings today.

bperseghetti added a commit to rudislabs/ros_gz that referenced this pull request Mar 27, 2024
Forward port of gazebosim#486

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
bperseghetti added a commit to rudislabs/ros_gz that referenced this pull request Mar 27, 2024
Forward port of gazebosim#486

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
bperseghetti added a commit to rudislabs/ros_gz that referenced this pull request Mar 27, 2024
Forward port of gazebosim#486

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
bperseghetti added a commit to rudislabs/ros_gz that referenced this pull request Mar 27, 2024
Forward port of gazebosim#486.

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti
Copy link
Member Author

friendly ping @bperseghetti do you have any plans to forward port these changes?

Randomly remembered to do this, sorry about the dealy, please see associated "forward feature ports":
#520
#521

ahcorde pushed a commit that referenced this pull request Mar 27, 2024
Forward port of #486

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
azeey pushed a commit that referenced this pull request May 6, 2024
Forward port of #486.

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
mergify bot pushed a commit that referenced this pull request Jun 25, 2024
Forward port of #486.

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
(cherry picked from commit 78dc482)
azeey pushed a commit that referenced this pull request Jun 25, 2024
Forward port of #486.

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
(cherry picked from commit 78dc482)
Amronos pushed a commit to Amronos/ros_gz that referenced this pull request Sep 18, 2024
Forward port of gazebosim#486.

    * Message and bridge for MaterialColor.

    This allows bridging MaterialColor from ROS to GZ and is
    important for allowing simulation users to create status lights.

Signed-off-by: Benjamin Perseghetti <[email protected]>
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