InspectSDFElement should allow duplicate plugin names

[M1chaelM]

Environment

• OS Version: Ubuntu 20.04
• Source or binary build?
  Binary: 9.6.1

Description

• Expected behavior: Have multiple child elements with the same name should not produce an error if the element type is "plugin"
• Actual behavior:
  If an element in the SDF contains multiple child plugin elements with the same name:
• The recursiveSameTypeUniqueNames function and the recursiveSiblingUniqueNames function in parser.cc (called by Gazebo) output an error message containing the entire element followed by the message

[Err] [Server.cc:98] SDF is not valid, see errors above. This can lead to an unexpected behaviour.

Model.cc also produces a warning:

Warning [Model.cc:212] Non-unique names detected in XML children of model with name[element_name]

Steps to reproduce

1. Define a model with a plugin in the definition. For example we have a buoy model (https://github.com/osrf/vrx/blob/master/vrx_gazebo/models/mb_marker_buoy_red/model.sdf) that uses an attached bouyancy plugin.
2. Define a nested model that uses several instances of the model you defined the previous step. For example, we have a nested model that arranges several of these buoys into a navigation course.
3. Launch Gazebo using a world that has the nested model in it.

Output

Using a version of the example above with only two buoys, we see:

Error: Non-unique names detected in type plugin in
<model name='short_navigation_course_0'>
  <frame name='red_bound_0::__model__' attached_to='red_bound_0::link'>
    <pose relative_to='__model__'>0 0 0 0 -0 0</pose>
  </frame>
  <link name='red_bound_0::link'>
    <inertial>
      <pose>0 0 -2 0 -0 0</pose>
      <mass>20</mass>
      <inertia>
        <ixx>1</ixx>
        <ixy>0</ixy>
        <ixz>0</ixz>
        <iyy>1</iyy>
        <iyz>0</iyz>
        <izz>0.1</izz>
      </inertia>
    </inertial>
    <collision name='collision_inner'>
      <pose>0 -0.02 0.02 0 -0 0</pose>
      <geometry>
        <cylinder>
          <radius>0.15</radius>
          <length>1.15</length>
        </cylinder>
      </geometry>
    </collision>
    <collision name='collision_outer'>
      <pose>0 0 -0.3 0 -0 0</pose>
      <geometry>
        <cylinder>
          <radius>0.325</radius>
          <length>0.1</length>
        </cylinder>
      </geometry>
    </collision>
    <visual name='visual'>
      <pose>0 0 0 1.57079 -0 0</pose>
      <geometry>
        <mesh>
          <uri>model://mb_marker_buoy_red/meshes/mb_marker_buoy.dae</uri>
        </mesh>
      </geometry>
      <material>
        <script>
          <uri>model://mb_marker_buoy_red/materials/scripts/mb_marker_buoy.material</uri>
          <name>mb_marker_buoy/Red</name>
        </script>
      </material>
    </visual>
    <pose relative_to='red_bound_0::__model__'>0 0 0 0 -0 0</pose>
  </link>
  <plugin name='BuoyancyPlugin' filename='libbuoyancy_gazebo_plugin.so'>
    <wave_model>ocean_waves</wave_model>
    <fluid_density>1000</fluid_density>
    <fluid_level>0.0</fluid_level>
    <linear_drag>25.0</linear_drag>
    <angular_drag>2.0</angular_drag>
    <buoyancy name='buoyancy_sphere'>
      <link_name>red_bound_0::link</link_name>
      <pose>0 0 -0.12 0 0 0</pose>
      <geometry>
        <sphere>
          <radius>0.325</radius>
        </sphere>
      </geometry>
    </buoyancy>
  </plugin>
  <frame name='green_bound_0::__model__' attached_to='green_bound_0::link'>
    <pose relative_to='__model__'>0 -20 0.2 0 -0 0</pose>
  </frame>
  <link name='green_bound_0::link'>
    <inertial>
      <pose>0 0 -2 0 -0 0</pose>
      <mass>20</mass>
      <inertia>
        <ixx>1</ixx>
        <ixy>0</ixy>
        <ixz>0</ixz>
        <iyy>1</iyy>
        <iyz>0</iyz>
        <izz>0.1</izz>
      </inertia>
    </inertial>
    <collision name='collision_inner'>
      <pose>0 -0.02 0.02 0 -0 0</pose>
      <geometry>
        <cylinder>
          <radius>0.15</radius>
          <length>1.15</length>
        </cylinder>
      </geometry>
    </collision>
    <collision name='collision_outer'>
      <pose>0 0 -0.3 0 -0 0</pose>
      <geometry>
        <cylinder>
          <radius>0.325</radius>
          <length>0.1</length>
        </cylinder>
      </geometry>
    </collision>
    <visual name='visual'>
      <pose>0 0 0 1.57079 -0 0</pose>
      <geometry>
        <mesh>
          <uri>model://mb_marker_buoy_red/meshes/mb_marker_buoy.dae</uri>
        </mesh>
      </geometry>
      <material>
        <script>
          <uri>file://media/materials/scripts/gazebo.material</uri>
          <name>Gazebo/White</name>
        </script>
      </material>
    </visual>
    <pose relative_to='green_bound_0::__model__'>0 0 0 0 -0 0</pose>
  </link>
  <plugin name='BuoyancyPlugin' filename='libbuoyancy_gazebo_plugin.so'>
    <wave_model>ocean_waves</wave_model>
    <fluid_density>1000</fluid_density>
    <fluid_level>0.0</fluid_level>
    <linear_drag>25.0</linear_drag>
    <angular_drag>2.0</angular_drag>
    <buoyancy name='buoyancy_sphere'>
      <link_name>green_bound_0::link</link_name>
      <pose>0 0 -0.12 0 0 0</pose>
      <geometry>
        <sphere>
          <radius>0.325</radius>
        </sphere>
      </geometry>
    </buoyancy>
  </plugin>
  <pose>-475 185 0 0 0 -2.14</pose>
</model>

[Err] [Server.cc:98] SDF is not valid, see errors above. This can lead to an unexpected behaviour.
Warning [Model.cc:212] Non-unique names detected in XML children of model with name[short_navigation_course_0].

Additional Notes

I haven't yet found documentation confirming that duplicate plugin names are allowed, but the comment on line 1476 of parser.cc in sdformat suggests this is the case.

Additionally, the sdformat implementation of the <include> tag does not rename plugins to avoid name conflicts, as it does with other elements, and plugin names also can't be overridden when a model is invoked. As illustrated in the above example, this means that whenever a model definition includes a plugin and that model is used more than once in a nested model, Gazebo will report that the SDF is invalid.

[caguero]

Here's the helper function that we use to detect if we have unique child names:

https://github.com/ignitionrobotics/sdformat/blob/sdf11/src/Element.cc#L825

So potentially, we could modify this function for ignoring plugins.

[scpeters]

As of version 1.7 of SDFormat, all sibling elements must have unique names. It is absolutely necessary for frame semantics that sibling //model, //link, //joint, and //frame elements have unique names to avoid duplicate names in the frame graph, and we decided that the rule would be most clear if it applied to all sibling elements in a file. To comply with the specification, you may apply a different name to each plugin, perhaps incorporating the link name to which they correspond, such as redBuoyancyPlugin and greenBuoyancyPlugin.

If you think the specification is too restrictive, feel free to retitle this issue accordingly.

[M1chaelM]

Thank you for this response. I don't feel the specification is too restrictive (I don't have an opinion either way), but I don't think it's possible to fix the problem by re-titling the plugin names as you suggest. The reason is that if I have a nested model that includes two red buoys (which is the case), I will still get the error unless there is some way to override the plugin name of the child model when including it into the parent model.

Put another way, if the policy is as you describe, it seems that the implementation of the "include" tag should rewrite the plugin names in the same way that it rewrites the link names. That would also resolve the issue.

[azeey]

Related: #294

[chapulina]

Fixed in Fortress, needs backport to Citadel

[chapulina]

Backported to Citadel in

• Backport #721 to sdf9: Make exception for plugins when checking for name uniqueness #733