gazebosim / sdformat

Simulation Description Format (SDFormat) parser and description files.
http://sdformat.org
Apache License 2.0
169 stars 97 forks source link

URDF to SDF conversion silently ignores <gazebo> tag with non-existing reference #1372

Open traversaro opened 9 months ago

traversaro commented 9 months ago

Desired behavior

If I try to convert the following URDF model, saved in a file called oneLink.urdf:

<?xml version='1.0' encoding='UTF-8'?>
<robot name="oneLink">
    <link name="link1">
        <inertial>
            <mass value="1" />
            <inertia ixx="0.01" ixy="0.0" ixz="0.0" iyy="0.01" iyz="0.0" izz="0.01" />
        </inertial>
    </link>
    <gazebo reference="lіnk1">
        <sensor name="link1_imu" type="imu">
            <always_on>1</always_on>
            <update_rate>100</update_rate>
            <pose>0.13525 0 -0.07019999999999993 0.0 -0.0 -2.0943952105869315</pose>
            <plugin name="sensor_plugin" filename="example_plugin.so" />
        </sensor>
    </gazebo>
</robot>

to SDF with sdformat 14.0.0 (i.e. via gz sdf -p ./oneLink.urdf > oneLink.sdf), I obtaine the following result, without any additional output:

(gzsdf) traversaro@IITBMP014LW012:~/test34$ gz sdf -p ./oneLink.urdf
<sdf version='1.11'>
  <model name='oneLink'>
    <link name='link1'>
      <inertial>
        <pose>0 0 0 0 0 0</pose>
        <mass>1</mass>
        <inertia>
          <ixx>0.01</ixx>
          <ixy>0</ixy>
          <ixz>0</ixz>
          <iyy>0.01</iyy>
          <iyz>0</iyz>
          <izz>0.01</izz>
        </inertia>
      </inertial>
    </link>
  </model>
</sdf>

Note how the imu sensors is completely ignored, without any warning message or error. Why is this happening? Because, despite the apperance, the link1 string in <link name='link1'> is not the same string as lіnk1 in <gazebo reference="lіnk1">. The second letter of the string in the first case is i , i.e. unicode character U+0069 (Latin Small Letter I), while in the second case is unicode character U+0456 (Cyrillic Small Letter Byelorussian-Ukrainian I).

Clearly I crafted this example to make it explicitly tricky to spot (see https://gist.github.com/StevenACoffman/a5f6f682d94e38ed804182dc2693ed4b if you want to have fun), but in general it would be convenient if the URDF --> SDF parser detected <gazebo reference=".."> that referenced to non-existing elements, and printed at least a warning, to help in case of typos.

Alternatives considered

Continue not to print any warning if a <gazebo> tag refers to a non existing element.

Implementation suggestion

I checked the code, and indeed first the parser parsers all the <gazebo> tags in https://github.com/gazebosim/sdformat/blob/f3607761d3846dba0b660880484ff1e5eb1e9425/src/parser_urdf.cc#L3372, and then only later when it parses links, collisions, visuals and joints, it uses the related sdf extensions if any is found (see for example https://github.com/gazebosim/sdformat/blob/f3607761d3846dba0b660880484ff1e5eb1e9425/src/parser_urdf.cc#L2870C3-L2870C12 or https://github.com/gazebosim/sdformat/blob/f3607761d3846dba0b660880484ff1e5eb1e9425/src/parser_urdf.cc#L3182).

Probably we should add a boolean flag or similar to each SDF extension, that is set to true once a SDF extension is consumed. If an SDF extension is not used at the end of the parsing, a warning should be printed.

Additional context

We experienced this with @Gio-DS .

azeey commented 9 months ago

Ouch! That's a tough one to debug. +1 for a warning message if an extension references an unknown link.

aagrawal05 commented 7 months ago

Hi, I have attempted an implementation on my personal fork aagrawal05/sdformat on the parser_ref_warning_message branch.

Currently it only checks in the InsertSDFExtensionJoint method, but similar functionality can be easily implemented for the other functions (namely InsertSDFExtensionRobot, InsertSDFExtensionCollision, InsertSDFExtensionVisual, and InsertSDFExtensionJoint).

My solution currently works by adding a boolean check in the linear search of the extensions in the insertion function and adding the warning if not found.

I've also written a test ParseGazeboRefDoesntExistWarningMessage that ensures the functionality of this check.

Please let me know if this approach is suitable and if any changes would need to be made. If not, I'd be happy to open the pull request.

azeey commented 7 months ago

Thanks for looking into it @aagrawal05. I think it would be easier to check your approach if you just open a draft PR.