gazebosim / ros_gz

Integration between ROS (1 and 2) and Gazebo simulation
https://gazebosim.org
Apache License 2.0
211 stars 125 forks source link

Add option to change material color from ROS. #486

Closed bperseghetti closed 5 months ago

bperseghetti commented 5 months ago

🎉 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

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.

bperseghetti commented 5 months ago

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 https://github.com/gazebosim/gz-msgs/pull/414 in first anyhow. This was more just to show the interdependent changes.

bperseghetti commented 5 months ago

@ahcorde Test added.

bperseghetti commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

azeey commented 5 months ago

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 commented 5 months ago

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.

bperseghetti commented 5 months ago

@osrf-jenkins run tests please

azeey commented 5 months ago

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 commented 5 months ago

Alternatively, you can do something like https://github.com/gazebosim/ros_gz/blob/84d5d27e6f730db506e6b229c40ef3fdc7042cf6/ros_gz_bridge/include/ros_gz_bridge/ros_gz_bridge.hpp#L26-L38

bperseghetti commented 5 months ago

Alternatively, you can do something like

https://github.com/gazebosim/ros_gz/blob/84d5d27e6f730db506e6b229c40ef3fdc7042cf6/ros_gz_bridge/include/ros_gz_bridge/ros_gz_bridge.hpp#L26-L38

Yep, can't believe I didn't think of/see that. Thanks!

bperseghetti commented 5 months ago

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.

I do want this to land in humble first and foremost as that is the "LTS", I can also bump it to rolling in another eventual PR and at that point remove the if/def conditionals from the code.

azeey commented 5 months ago

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

bperseghetti commented 5 months ago

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

Yep, I can do it later today!

ahcorde commented 4 months ago

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

bperseghetti commented 4 months ago

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

bperseghetti commented 3 months ago

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": https://github.com/gazebosim/ros_gz/pull/520 https://github.com/gazebosim/ros_gz/pull/521