Different error checks for repeated sibling elements names using SDF6 spec with the 6.2.0 version of the library and 9.0.0 or greater

[j-rivero]

Environment

• OS Version: Ubuntu 18.04
• Source build of sdf6 branch (since packages are affected by sdformat6 packages does not ship command line utilities #585 to use ign)
• Binaries for sdformat9 9.2.0

Description

With the following example:

<?xml version="1.0" encoding="UTF-8"?>
<sdf version='1.6'>
  <world name='default'>

    <model name='wall'>
      <static>1</static>
      <pose frame=''>0 0 1 0 0 1</pose>
      <link name='wall'>
        <collision name='repeated_name'>
          <pose frame=''>-2.5 0 0 0 0 0</pose>
          <geometry>
            <box>
              <size>5 0.2 2</size>
            </box>
          </geometry>
        </collision>
         <collision name='collision_1'>
          <pose frame=''>-2.5 0 0 0 0 0</pose>
          <geometry>
            <box>
              <size>5 0.2 2</size>
            </box>
          </geometry>
        </collision>
         <collision name='collision_2'>
          <pose frame=''>-2.5 0 0 0 0 0</pose>
          <geometry>
            <box>
              <size>5 0.2 2</size>
            </box>
          </geometry>
        </collision>
        <visual name='repeated_name'>
          <pose frame=''>-2.5 0 0 0 0 0</pose>
          <geometry>
            <box>
              <size>5 0.2 2</size>
            </box>
          </geometry>
        </visual>
      </link>
    </model>
  </world>
</sdf>

• Expected behavior: same behavior when using the SDF 1.6 spec independently of the libsdformat implementation version
• Actual behavior: when using libsdformat6 the check says the example is valid. When using libsdformat9 the check says the example is not valid.

Steps to reproduce

1. Install libsdformat9-dev packages
2. Install a from source copy of sdf6 branch
3. Install ignition-tools
4. Download the example in this description
5. Run ign as detailed in the output section

Output

~ ❯ ign sdf --check -d 1.6 --force-version 6.2.0  --check /home/jrivero/code/gazebo/test/worlds/test_sdf16_err_sibling_different_type.world
Valid.
~ ❯ ign sdf --check -d 1.6 --force-version 9.5.0  --check /home/jrivero/code/gazebo/test/worlds/test_sdf16_err_sibling_different_type.world
Error: Non-unique names detected in <link name='wall'>
  <collision name='repeated_name'>
    <pose>-2.5 0 0 0 -0 0</pose>
    <geometry>
      <box>
        <size>5 0.2 2</size>
      </box>
    </geometry>
  </collision>
  <collision name='collision_1'>
    <pose>-2.5 0 0 0 -0 0</pose>
    <geometry>
      <box>
        <size>5 0.2 2</size>
      </box>
    </geometry>
  </collision>
  <collision name='collision_2'>
    <pose>-2.5 0 0 0 -0 0</pose>
    <geometry>
      <box>
        <size>5 0.2 2</size>
      </box>
    </geometry>
  </collision>
  <visual name='repeated_name'>
    <pose>-2.5 0 0 0 -0 0</pose>
    <geometry>
      <box>
        <size>5 0.2 2</size>
      </box>
    </geometry>
  </visual>
</link>

Looking into the specification: naming rules for 1.4-1.6, it says:

Sibling elements of different types are not mandated to have unique names, so the following is valid, though it is confusing and not recommended.

So I believe that the issue could be a bug in sdformat9 (probably above) when forcing to use the 1.6 spec.

[jennuine]

This may not be a bug, looking at Migration.md under SDFormat specification 1.6 to 1.7 > Modifications#2 it says regarding unique names:

Some existing SDFormat models may not comply with this requirement. The ign sdf --check command can be used to identify models that violate this requirement.

Should confirm with @scpeters or @azeey though

[j-rivero]

Looking into the API for the recursiveSiblingUniqueNames method it is unclear to me that we want to introduce SDF specs if the method is designed just to provide the check and it's on its clients to use it to implement whatever logic they want:

  /// \brief Check that all sibling elements of the any type have unique names.
  /// This checks recursively and should check the files exhaustively
  /// rather than terminating early when the first duplicate name is found.

I used this approach in gazebo11 . The downside is that all the clients should implement a logic defined in the standard, which seems inefficient to me for multiple reasons.

[azeey]

libsdformat has the limitation that all files are converted to the latest Spec version known by the library before parsing. This means that if you parse a 1.6 file in libsdformat9, it will be converted to 1.7, and if you parse the same file in libsdformat11, it will be converted to 1.8. To work around this limitation, we have sdf::Element::OriginalVersion and there are places in the code that issue warnings based on that. However, name uniqueness for sibling elements is required to provide Frame Semantics features that were introduced in SDFormat 1.7. And there is no way for libsdformat9 to function with that feature disabled, so as a work around and not to break a lot of existing SDF files, if a name collision is found, we print a warning and rename the element as you can see in: https://github.com/osrf/sdformat/blob/9876a6089c88ac39408e658b6b4b2928abab8377/src/Model.cc#L294-L297 and https://github.com/osrf/sdformat/blob/9876a6089c88ac39408e658b6b4b2928abab8377/src/Model.cc#L331-L334)

However, we did not put in these workarounds for the validation checks because we want to encourage users to update their models.