[ros2] new package ros_ign_interfaces, provide some Ignition-specific ROS messages. #152

gezp · 2021-05-04T08:01:18Z

This is migration of gazebo_ros_pkgs/gazebo_msgs for Ignition to provide some Ignition-specific ROS messages.

This package is necessary to implement some functions like this #70 .

ContactState.msg and ContactStates.msg are migrated from gazebo_ros_pkgs/gazebo_msgs
DeleteEntity.srv and SpawnEntity.srv are migrated from gazebo_ros_pkgs/gazebo_msgs and modified.
ControlWorld.srv refer to ign-msgs/proto/ignition/msgs/world_control.proto
SetEntityPose.srv is new srv file.

… .msg and .srv files Signed-off-by: gezp <[email protected]>

chapulina

Thank you for the PR! I think this is an opportunity to improve on the message definitions from gazebo_msgs, otherwise we could just depend on that package.

Some thoughts:

The messages should match ign-msgs as much as possible, for seamless integration. This includes matching message names and fields.
We should remove textual debug fields like info and status_message, which aren't a standard practice on ROS and just make the messages larger. This kind of information can go into console logging instead. It's also difficult to standardize textual status messages so the receiver knows what to expect.

chapulina · 2021-05-04T16:45:27Z

ros_ign_interfaces/package.xml

+  <description>Message and service data structures for interacting with Ignition from ROS2.</description>
+  <license>Apache 2.0</license>
+  <maintainer email="[email protected]">Louise Poubel</maintainer>
+


Feel free to add yourself as author 😉

chapulina · 2021-05-04T16:58:11Z

ros_ign_interfaces/msg/ContactState.msg

@@ -0,0 +1,8 @@
+string info                                   # text info on this contact


I know the message on gazebo_msgs has the info field, which is filled with debug information, but I believe this isn't a good practice. It makes the message larger and doesn't add any new information.

How about removing the field?

chapulina · 2021-05-04T17:01:13Z

ros_ign_interfaces/srv/ControlWorld.srv

@@ -0,0 +1,11 @@
+bool pause                        


Do you mind documenting all the fields in the message? Let's try to do better than ign-msgs 😬

chapulina · 2021-05-04T17:07:29Z

ros_ign_interfaces/srv/ControlWorld.srv

@@ -0,0 +1,11 @@
+bool pause                        


Do you mind calling this message WorldControl to match ignition::msgs::WorldControl?

chapulina · 2021-05-04T17:32:48Z

ros_ign_interfaces/srv/DeleteEntity.srv

@@ -0,0 +1,5 @@
+string name                       # Name of the Ignition Gazebo entity to be deleted. 
+uint8 type                        # Type: Model or Light


How about adding an entity message like ignition::msgs::Entity and using it here? Using the same pattern as Ignition should make it easier to maintain. It's also easier to only update one messsage if we add new entity types.

chapulina · 2021-05-04T17:40:09Z

ros_ign_interfaces/msg/ContactState.msg

+string collision1_name                        # name of contact collision1
+string collision2_name                        # name of contact collision2
+geometry_msgs/Wrench[] wrenches               # list of forces/torques
+geometry_msgs/Wrench total_wrench             # sum of forces/torques in every DOF


I'd also suggest removing the total_wrench field, because it doesn't add new information. The receiver can calculate it if they need it.

chapulina · 2021-05-04T17:40:25Z

ros_ign_interfaces/msg/ContactState.msg

+geometry_msgs/Wrench[] wrenches               # list of forces/torques
+geometry_msgs/Wrench total_wrench             # sum of forces/torques in every DOF
+geometry_msgs/Vector3[] contact_positions     # list of contact position
+geometry_msgs/Vector3[] contact_normals       # list of contact normals


Prefixing these fields with contact_ feels redundant.

chapulina · 2021-05-04T17:41:01Z

ros_ign_interfaces/msg/ContactState.msg

+geometry_msgs/Wrench total_wrench             # sum of forces/torques in every DOF
+geometry_msgs/Vector3[] contact_positions     # list of contact position
+geometry_msgs/Vector3[] contact_normals       # list of contact normals
+float64[] depths                              # list of penetration depths


I'd suggest changing the name of this message to Contact so it matches ignition::msgs::Contact.

chapulina · 2021-05-04T17:49:31Z

ros_ign_interfaces/srv/SpawnEntity.srv

@@ -0,0 +1,12 @@
+string name                             # New name for the entity, overrides the name on the SDF


How about renaming this message to EntityFactory so it matches ign-msgs?

Signed-off-by: gezp <[email protected]>

gezp · 2021-05-05T01:41:55Z

According to above advice, I modified and added the definition of message to make them match ign-msgs .

Signed-off-by: gezp <[email protected]>

gezp · 2021-05-10T07:44:33Z

@chapulina
hi, i have modified my PR, could you review again? after this PR completed, I could try to implement service bridge.

ahcorde

Some style nits

ahcorde · 2021-05-10T07:58:08Z

ros_ign_interfaces/CMakeLists.txt

+find_package(geometry_msgs REQUIRED)
+find_package(rosidl_default_generators REQUIRED)
+
+


nit:

Suggested change

ahcorde · 2021-05-10T07:58:47Z

ros_ign_interfaces/README.md

+
+## Messages (.msg)
+
+* [Contact](msg/Contact.msg) : related to [ignition::msgs::Contact](https://github.com/ignitionrobotics/ign-msgs/blob/ign-msgs7/proto/ignition/msgs/contact.proto). Contant info bewteen collisions in Ignition Gazebo.


Same below

Suggested change

* [Contact](msg/Contact.msg) : related to [ignition::msgs::Contact](https://github.com/ignitionrobotics/ign-msgs/blob/ign-msgs7/proto/ignition/msgs/contact.proto). Contant info bewteen collisions in Ignition Gazebo.

* [Contact](msg/Contact.msg): related to [ignition::msgs::Contact](https://github.com/ignitionrobotics/ign-msgs/blob/ign-msgs7/proto/ignition/msgs/contact.proto). Contant info bewteen collisions in Ignition Gazebo.

ahcorde · 2021-05-10T08:00:30Z

ros_ign_interfaces/msg/Entity.msg

@@ -0,0 +1,13 @@
+#Entity type: constant definition
+uint8 NONE = 0


can you indent the default values at the same level ?

ahcorde · 2021-05-10T08:00:43Z

ros_ign_interfaces/msg/Entity.msg

+uint64 id      #Entity unique identifier accross all types. Defaults to 0
+string name    #Entity name, which is not guaranteed to be unique.
+uint8 type     #Entity type.


Suggested change

uint64 id #Entity unique identifier accross all types. Defaults to 0

string name #Entity name, which is not guaranteed to be unique.

uint8 type #Entity type.

uint64 id # Entity unique identifier accross all types. Defaults to 0

string name # Entity name, which is not guaranteed to be unique.

uint8 type # Entity type.

ahcorde · 2021-05-10T08:01:28Z

ros_ign_interfaces/msg/WorldControl.msg

@@ -0,0 +1,8 @@
+bool pause                                  # paused state.      


Start comment with a Upper case ( in all msgs )

Suggested change

bool pause # paused state.

bool pause # Paused state.

Signed-off-by: gezp <[email protected]>

ros_ign_interfaces/package.xml

… ROS messages. (#152) * add new package ros_ign_interfaces,provide some Ignition-specific ros .msg and .srv files Signed-off-by: gezp <[email protected]> * modify to match ign-msgs Signed-off-by: gezp <[email protected]> * add author info Signed-off-by: gezp <[email protected]> * modify comments Signed-off-by: gezp <[email protected]> * update code and doc style Signed-off-by: gezp <[email protected]>

gezp requested a review from chapulina as a code owner May 4, 2021 08:01

add new package ros_ign_interfaces,provide some Ignition-specific ros…

9af3ed6

… .msg and .srv files Signed-off-by: gezp <[email protected]>

chapulina added the ROS 2 ROS 2 label May 4, 2021

chapulina reviewed May 4, 2021

View reviewed changes

gezp added 2 commits May 5, 2021 09:21

modify to match ign-msgs

a59852a

Signed-off-by: gezp <[email protected]>

add author info

22c2ad4

Signed-off-by: gezp <[email protected]>

modify comments

d75b177

Signed-off-by: gezp <[email protected]>

ahcorde requested changes May 10, 2021

View reviewed changes

update code and doc style

55344d9

Signed-off-by: gezp <[email protected]>

ahcorde requested changes May 17, 2021

View reviewed changes

ros_ign_interfaces/package.xml Show resolved Hide resolved

ahcorde approved these changes May 18, 2021

View reviewed changes

ahcorde merged commit fff2dbf into gazebosim:ros2 May 18, 2021

chapulina mentioned this pull request Jul 20, 2021

⬅️ Backport ros2 to foxy #169

Merged

chapulina mentioned this pull request Aug 13, 2021

Expose ignition::msgs::Contact through ROS 2 msgs #174

Closed

gezp mentioned this pull request Jan 6, 2022

update maintainer #204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ros2] new package ros_ign_interfaces, provide some Ignition-specific ROS messages. #152

[ros2] new package ros_ign_interfaces, provide some Ignition-specific ROS messages. #152

gezp commented May 4, 2021 •

edited

Loading

chapulina left a comment

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

chapulina May 4, 2021

gezp commented May 5, 2021

gezp commented May 10, 2021

ahcorde left a comment

ahcorde May 10, 2021

ahcorde May 10, 2021

ahcorde May 10, 2021

ahcorde May 10, 2021

ahcorde May 10, 2021

		@@ -0,0 +1,5 @@
		string name # Name of the Ignition Gazebo entity to be deleted.
		uint8 type # Type: Model or Light

		@@ -0,0 +1,12 @@
		string name # New name for the entity, overrides the name on the SDF

		find_package(geometry_msgs REQUIRED)
		find_package(rosidl_default_generators REQUIRED)


		## Messages (.msg)

		* [Contact](msg/Contact.msg) : related to [ignition::msgs::Contact](https://github.com/ignitionrobotics/ign-msgs/blob/ign-msgs7/proto/ignition/msgs/contact.proto). Contant info bewteen collisions in Ignition Gazebo.

	* [Contact](msg/Contact.msg) : related to [ignition::msgs::Contact](https://github.com/ignitionrobotics/ign-msgs/blob/ign-msgs7/proto/ignition/msgs/contact.proto). Contant info bewteen collisions in Ignition Gazebo.
	* [Contact](msg/Contact.msg): related to [ignition::msgs::Contact](https://github.com/ignitionrobotics/ign-msgs/blob/ign-msgs7/proto/ignition/msgs/contact.proto). Contant info bewteen collisions in Ignition Gazebo.

		@@ -0,0 +1,13 @@
		#Entity type: constant definition
		uint8 NONE = 0

[ros2] new package ros_ign_interfaces, provide some Ignition-specific ROS messages. #152

[ros2] new package ros_ign_interfaces, provide some Ignition-specific ROS messages. #152

Conversation

gezp commented May 4, 2021 • edited Loading

chapulina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gezp commented May 5, 2021

gezp commented May 10, 2021

ahcorde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gezp commented May 4, 2021 •

edited

Loading