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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow converting from/to TwistWithCovarianceStamped #374

Merged
merged 4 commits into from
May 22, 2023

Conversation

ejalaa12
Copy link
Contributor

馃帀 New feature

Allows converting from/to TwistWithCovarianceStamped

Summary

ros_gz_bridge allows converting gz.msgs.TwistWithCovariance <-> geometry_msgs.msg.TwistWithCovariance.

However the gz.msgs.TwistWithCovariance also contains a header and stamp. So it could be converted to a geometry_msgs.msg.TwistWithCovariance**Stamped**.

This PR implements that conversion. It is now possible to convert either to/from:

  • geometry_msgs.msg.TwistWithCovariance
  • geometry_msgs.msg.TwistWithCovarianceStamped.

Note: I also have a backport of that commit to the humble branch, should I open it after this PR is done ?

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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.

Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
@osrf-triage osrf-triage added this to Inbox in Core development Mar 15, 2023
@azeey azeey moved this from Inbox to In review in Core development Mar 20, 2023
Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Thanks for adding this !
I can see we can add PoseWithCovarianceStamped in a similar fashion. Though not required in this PR, could you add a ticket for this ?

Comment on lines 30 to 47
| geometry_msgs/msg/TransformStamped | ignition::msgs::Pose |
| geometry_msgs/msg/Twist | ignition::msgs::Twist |
| geometry_msgs/msg/TwistWithCovariance | ignition::msgs::TwistWithCovariance |
| geometry_msgs/msg/TwistWithCovarianceStamped | ignition::msgs::TwistWithCovariance |
| nav_msgs/msg/Odometry | ignition::msgs::Odometry |
| nav_msgs/msg/Odometry | ignition::msgs::OdometryWithCovariance |
| rcl_interfaces/msg/ParameterValue | ignition::msgs::Any |
| ros_gz_interfaces/msg/Contact | ignition::msgs::Contact |
| ros_gz_interfaces/msg/Contacts | ignition::msgs::Contacts |
| ros_gz_interfaces/msg/Dataframe | ignition::msgs::Dataframe |
| ros_gz_interfaces/msg/Entity | ignition::msgs::Entity |
| ros_gz_interfaces/msg/Float32Array | ignition::msgs::Float_V |
| ros_gz_interfaces/msg/GuiCamera | ignition::msgs::GUICamera |
| ros_gz_interfaces/msg/JointWrench | ignition::msgs::JointWrench |
| ros_gz_interfaces/msg/Light | ignition::msgs::Light |
| ros_gz_interfaces/msg/StringVec | ignition::msgs::StringMsg_V |
| ros_gz_interfaces/msg/TrackVisual | ignition::msgs::TrackVisual |
| ros_gz_interfaces/msg/VideoRecord | ignition::msgs::VideoRecord |
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the vertical table alignment and keep the namespace as "gz::" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I was probably working on a wrong branch with ignition instead of gz. I'll a an amend commit to clean this up

Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
@ejalaa12
Copy link
Contributor Author

sorry for the delay in answer @adityapande-1995
I just pushed the changes for the readme.
I also opened up a PR for the PoseWithCovarianceStamped as well.

@ejalaa12
Copy link
Contributor Author

@adityapande-1995 Did you have time to review my changes ?

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.

conflicts

@ejalaa12
Copy link
Contributor Author

@ahcorde just fixed the conflicts

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.

@adityapande-1995 do you mind to take another look ?

Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Looks good with conflicts resolved in the README !

Signed-off-by: Alejandro Hern谩ndez Cordero <ahcorde@gmail.com>
@adityapande-1995 adityapande-1995 merged commit e84dda6 into gazebosim:ros2 May 22, 2023
3 checks passed
Core development automation moved this from In review to Done May 22, 2023
@ejalaa12
Copy link
Contributor Author

Thanks for the review and the merge !

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

Successfully merging this pull request may close these issues.

None yet

3 participants