search

scpeters-test / sdformat

Simulation Description Format (SDF) parser and description files.
http://sdformat.org
Other
1 stars 0 forks source link

Change the SDF spec to make <plugin> less gazebo-specific #93

Open scpeters-test opened 9 years ago

scpeters-test commented 9 years ago

Original report (archived issue) by sylvainjoyeux (Bitbucket: sylvainjoyeux).

I am sorry if this is not the right place. I asked on answers.gazebo.org about where to create a proposal to change the SDF spec and someone suggested here. Since it seemed to be as good a suggestion as any, here I am.

The current tag requires that a library name is given, and that library name must be a gazebo plugin. Plugin tags without a valid gazebo plugin generate an error.

I'm in a situation where we use SDF as an overall scene/world/robot description format in a robot framework (Rock, http://rock-robotics.org). We naturally started creating gazebo plugins for things that were missing (e.g. underwater thursters) but also wanted to add "plugins" for dealing externally with some things (for reasons that are beyond this proposal, we for instance want to simulate cameras outside of gazebo). We even had cases (the thrusters) where some parts of the infrastructure need to load plugins of their own, and need to know about the parameters given to the gazebo plugin.

In any case, to me, this is a matter of making SDF less gazebo-specific and more a general-purpose format, something it is already pretty good at. What I want to propose is to remove the 'file' attribute and instead define reserved tags within the element:

<plugin name='thrusters'>
   <gazebo>libgazebo_thrusters.so</gazebo>
   <!-- name of the Rock task that can export this plugin to the outside world -->
   <rock>gazebo_thrusters::Task</rock>
</plugin>

A software environment would simply ignore a plugin tag which does not have the required inside tag (i.e. gazebo would ignore plugin elements that do not have a element and so on).

scpeters-test commented 9 years ago

Original comment by sylvainjoyeux (Bitbucket: sylvainjoyeux).

Ping !

scpeters-test commented 9 years ago

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

Thanks for the proposal. This is a very good idea. How about the following?

#!xml

<plugin name="plugin_name" type="gazebo" file="somelibrary.so">
<!-- Insert any content here -->
</plugin>

Where the file element is optional.

scpeters-test commented 9 years ago

Original comment by sylvainjoyeux (Bitbucket: sylvainjoyeux).

Not sure. Let's try to describe my use case and see if that makes sense.

My use case is that the same "thrusters SDF extension" is being interpreted by different elements in the system. There's a gazebo plugin for the physics, then there is a component that exports the plugin to the rest of the framework. That's what led me to propose having a single tag with various sub-tags. This way, different software parts can share the common parameters but have the software-specific parts isolated.

Does that make sense ?

scpeters-test commented 9 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters).

Can you expand your example to include some of the common parameters that you mentioned?

scpeters-test commented 9 years ago

Original comment by sylvainjoyeux (Bitbucket: sylvainjoyeux).

The plugin expects a list of links and thruster parameters. The thruster limits are used both in the physics and in the gazebo-to-rock interface.

By the way, why is a plugin name even needed ?

<plugin name='thrusters'>
   <gazebo>libgazebo_thrusters.so</gazebo>
   <!-- name of the Rock task that can export this plugin to the outside world -->
   <rock>gazebo_thrusters::Task</rock>

   <thruster name='thruster::x::right'>
        <max_thrust>200</max_thrust>
        <min_thrust>-200</min_thrust>
   </thruster>
   <thruster name='thruster::x::left'>
        <max_thrust>200</max_thrust>
        <min_thrust>-200</min_thrust>
   </thruster>
</plugin>
scpeters-test commented 9 years ago

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

The plugin name is there so that it be referenced by a program. Gazebo used to have the ability to unload a plugin based on a name.

The contents of a <plugin> element are left undefined by SDF, and are directly copied. So, in your example you can do the following:

#!xml

<plugin name='thrusters' file="libgazebo_thrusters.so">
   <!-- name of the Rock task that can export this plugin to the outside world -->
   <rock>gazebo_thrusters::Task</rock>

   <thruster name='thruster::x::right'>
        <max_thrust>200</max_thrust>
        <min_thrust>-200</min_thrust>
   </thruster>
   <thruster name='thruster::x::left'>
        <max_thrust>200</max_thrust>
        <min_thrust>-200</min_thrust>
   </thruster>
</plugin>

Gazebo will be happy with this, and your program should just ignore the file attribute.

scpeters-test commented 9 years ago

Original comment by sylvainjoyeux (Bitbucket: sylvainjoyeux).

Yes. That is indeed a very obvious solution. With the extension of the type= attribute, and file= being optional, it would definitely cover all my use-cases.

scpeters-test commented 6 years ago

Original comment by sylvainjoyeux (Bitbucket: sylvainjoyeux).

I'd like to go back to this.

What would it take to get the type and optional file parameter implemented ? I don't mind doing the work, but I'm not entirely sure about the way forward ...

How I see it, I would need to:

Am I missing anything ?

scpeters-test commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

I'd suggest we make type optional with no default value, and for Gazebo, we can start using gazebo plugin types like gazebo::ModelPlugin, gazebo::WorldPlugin, etc but still hadle the case where type is not provided. We're also starting to use plugins on the Ignition libraries, and in that case we can start using types like ignition::gui::plugins::Publisher.

If we make filename optional, we also have to double check on Gazebo's side whether that's being properly handled.

scpeters-test commented 6 years ago

Original comment by sylvainjoyeux (Bitbucket: sylvainjoyeux).

Does it make a lot of sense to use the plugin class as the 'type' attribute ? I would think that the plugin type is constrained by where the plugin tag is (i.e. a plugin inside a can only be a ModelTask), in which case just gazebo would be enough.

scpeters-test commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

Fair point, maybe my example should have been a derived class like gazebo::BuoyancyPlugin, which is derived from gazebo::ModelPlugin.

scpeters-test commented 6 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters).

cc @mxgrey since he has been improving plugin support in ignition-common

scpeters-test commented 6 years ago

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

I think there are a lot of great ideas here.

In the hopefully-not-too-distant future, we're going to have the ability to load arbitrary plugins in a completely generic way, so I'm not sure that we even need to worry much about a "type" specifier, except maybe to be used as an extra sanity check somewhere.

Basically, pending the ignition-common pull request 59, your plugin can be loaded into a generic PluginPtr container, and you can use that container to access any interfaces that your plugin instance provides, without the plugin loader or parser needing to know anything about the type. The plugin itself will know what interfaces it provides upon being loaded.

That being said, it's conceivable that you might have your own plugin loading framework which you'd like to utilize instead of the existing Gazebo plugin loading framework or the upcoming ignition plugin framework. If that's the case, then I would propose that the type attribute would indicate what kind of loader is expected to perform the loading operation. This would help with backwards compatibility, as we could specify gazebo, ignition, or a user's custom plugin loader. The Gazebo plugin loader would only pay attention if gazebo is specified, and ignition would only pay attention to it if ignition is specified.

scpeters-test commented 6 years ago

Original comment by Shane Loretz (Bitbucket: Shane Loretz).

I agree with @mxgrey's last paragraph and think the type attribute is necessary: it allows other projects to use their own plugin loader, and it allows ignition projects to only load plugins relevant to them (related). If you recognize the type then use your plugin loader.

<plugin name="thrusters" type="ignition::gui::Plugin" filename="libgazebo_thrusters_gui.so">
  <max_thrust>200</max_thrust>
  <min_thrust>-200</min_thrust>
</plugin>

Backwards compatibility options:

  1. @chapulina's idea: add type as an optional attribute. Gazebo will attempt to load plugins with no type, printing a warning that type should be more specific.

  2. Make type required and bump the SDF version. The conversion script from 1.6 will automatically add gazebo types to plugin blocks. Gazebo will only load plugins with its types.

  3. ???

I mention bumping the SDF version because I think the filename attribute should be renamed to location if type is added. Another plugin loader may not use filenames. Maybe it expects a URI and will download the plugin?

scpeters-test commented 5 years ago

Original comment by Lorenzo Rapetti (Bitbucket: lorrapetti).

Discussion in gazebo-yarp-plugins points out another possible use of SDF <plugin> when detached from a gazebo plugin.

The aim here is to add a custom SDF Element to a <model> (that will be then used to override an element of a plugin), but a custom element can be add only trough a <plugin>.
A possible solution is to create a dummy plugin (e.g. gazebo_yarp_dummy.so) just to be able to define the custom elements inside the plugin:

<model name="myModelOverriden">
  <plugin name="plugin_override" filename="gazebo_yarp_dummy.so">
    <yarpPluginConfigurationOverride plugin_name='overriden_plugin'> </yarpPluginConfigurationOverride>
  </plugin>
  <include>
    <uri>model://myModel</uri>
  </include>
</model>

Making the attribute filename optional, it would avoid the use of a dummy plugin.

scpeters-test commented 4 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).

Another aspect that is Gazebo-specific is the attribute type of the element physics, see a relevant discussion in https://bitbucket.org/ignitionrobotics/ign-gazebo/pull-requests/541/find-custom-physics-engines/diff#comment-137379501 .