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

[ros2] gazebo_ros_api_plugin split #779

Open
chapulina opened this issue Jul 25, 2018 · 6 comments

Comments

@chapulina
Copy link
Contributor

commented Jul 25, 2018

The gazebo_ros_api_plugin is quite large and offers a lot of functionality all at once. While porting it to ROS 2, it would be nice to take the opportunity to also break that plugin into smaller ones so that:

  • Users don't need to "pay for what they don't use"
  • It is easier to remove functionality which shouldn't be present (see for example #704 where a param was added to allow disabling all API at once)

See the table below for a proposal of a split based on what kind of entity (model / link / joint / etc) an API targets.

I also considered a split which takes into account whether an API provides access to ground truth that wouldn't be available in the real world, or whether it allows altering simulation with a god's hand, i.e. in a way a robot wouldn't be able to in the real world. But after marking the API below with these 2 characteristics (to the best of my knowledge), it became clear to me that they really go hand-in-hand, and maybe the most useful approach would make each API disable-able within their plugins.

Another question I have is whether the new plugins should be in gazebo_plugins instead of gazebo_ros.

API Ground truth God's hand Proposed category Comments
apply_joint_effort   gazebo_ros_force_system ✔️ #941 
clear_joint_forces   gazebo_ros_force_system ✔️ #941 
get_joint_properties     gazebo_ros_properties ✔️ #868 #972
set_joint_properties   gazebo_ros_properties  ✔️ #868 #972
         
clear_body_wrenches   gazebo_ros_force_system  ✔️ #941
apply_body_wrench   gazebo_ros_force_system ✔️ #941
link_states   gazebo_ros_state  ✔️ #969
get_link_properties     gazebo_ros_properties  ✔️ #868 #972
set_link_properties   gazebo_ros_properties  ✔️ #868 #972
get_link_state   gazebo_ros_state ✔️ #839
set_link_state   gazebo_ros_state ✔️ #839
         
model_states   gazebo_ros_state ✔️ #968
get_model_state   gazebo_ros_state ✔️ #839
set_model_state   gazebo_ros_state ✔️ #839
set_model_configuration   ?  Maybe this belongs to joint?
get_model_properties   gazebo_ros_properties  ✔️ #868 #972
         
get_world_properties     gazebo_ros_properties  ⚠️ Deprecated, use get_model_list
get_physics_properties     gazebo_ros_properties 🚧 #973
set_physics_properties   gazebo_ros_properties 🚧 #973
         
get_light_properties gazebo_ros_properties ✔️ #868 #972
set_light_properties gazebo_ros_properties ✔️ #868 #972
spawn_sdf_model   gazebo_ros_factory ✔️ #808
spawn_urdf_model   gazebo_ros_factory ✔️ #808
delete_light   gazebo_ros_factory ✔️ #808
delete_model   gazebo_ros_factory ✔️ #808
         
clock     gazebo_ros_init ✔️ #794
pub_clock_frequency gazebo_ros_init ✔️ #794
unpause_physics   gazebo_ros_init ✔️ #866
pause_physics   gazebo_ros_init ✔️ #866
reset_simulation   gazebo_ros_init ✔️ #866
reset_world   gazebo_ros_init ✔️ #866
enable_ros_network ⚠️ Deprecated, just remove the undesired plugins
@kev-the-dev

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2018

A couple ideas I have for the port:

  • Remove the get_physics_properties and set_physics_properties services, which duplicate the functionality of the physics dynamic reconfigure in the current plugin. Replace with parameters for physics properties
  • Remove get_world_properties. Replace with get_model_list (perhaps under factory?). The other attributes are sim_time (will come from clock) and rendering_enabled (currently unused)
  • Have clock as a standalone plugin (perhaps the only function of the "time" category), move the unpause/pause and reset services to the "world" category. I think many users only use the clock functionality and implement everything else with their own case-specific plugins
  • Combine spawn_sdf_model and spawn_urdf_model into spawn_model. The plugin already checks the type internally, so might as well keep the interface agnostic for the user
  • Rename clear_body_wrench/apply_body_wrench to clear_link_wrench/apply_link_wrench to be more explicit with what this does
  • Rename set_model_configuration to set_model_joint_positions

Otherwise this splitting looks good to me! It makes sense to me to just put all the plugins (including the sensors, etc) into gazebo_ros and not reintroduce gazebo_plugins.

@chapulina

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

Started a migration page: ROS 2 Migration: gazebo_ros_api_plugin

@chapulina

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

Migration page for spawn / delete: ROS 2 Migration: Spawn and delete

@chapulina

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

I was thinking of dividing plugins into state and properties instead of by entity type (link / joint / model / world). I'm also inclined to deprecate apply/clear_body_wrench, since they duplicate gazebo_ros_force.

Thoughts, @j-rivero ?

@j-rivero

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

I was thinking of dividing plugins into state and properties instead of by entity type (link / joint / model / world).

I can find more use cases for diving them into state and properties than by having them divided by type, so +1.

I'm also inclined to deprecate apply/clear_body_wrench, since they duplicate gazebo_ros_force.

I have no experience with both so can not comment anything against the removal.

@nzlz

This comment has been minimized.

Copy link

commented Jan 8, 2019

I would be interested in having properties migrated to ros2, in a gazebo_ros_properties plugin I guess. Please let me know if anyone is already doing this work, otherwise I can try to do it myself.
Update: followed at PR #868.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.