Changed URDF parser to use Gnome libxml2 instead of TinyXML

[francesco-romano]

Even if TinyXML is one of the most widespread XML libraries around, it is functionally very limited, and no more maintained, not even for security/bug fixes.

This PR changes the URDF parser of iDynTree to use Gnome libxml2, a C-based XML parser.

The old library has been backed-up in idyntree-modelio-urdf-legacy in case we want to keep it around for some time. Given that the new library should be a drop-in replacement of the old one, hopefully (modulo bugs) nobody should see any difference.

A new library, idyntree-modelio-xml has been created (that depends on libxml2). This library supports generic XML parsing (of course the output is a tree of XML elements) and XSD 1.0 validation support. The latter can be useful for configuration files (even if I still think that we should strive to have XSD as much as possible) or other simple XML files.

The updated idyntree-modelio-urdf depends on idyntree-modelio-xml and adapts the generated tree to create a Model and SensorsList objects.

[traversaro]

I think we should take the occasion to generate a Windows installer for libxml2 and dependencies using vcpkg , to see how it is working and provide an easy installation of the dependencies for Windows users.

[francesco-romano]

@traversaro focus first on the design.
While developing the library I did a couple of "changes", i.e. in particular I started with the idea that the parent element does not know when the children has been parsed. At the end, I added also this possibility as it is useful. There can be some inconsistency in this regard.

Feel free to comment.

There are a couple of pending todos, but they are more for future development (as even without those, we should have feature-parity with the previous version).

Lastly, given how important the model parsing is for the robot controllers, this PR should be thoroughly tested on the robots (as the unit tests do not cover enough the code)

[traversaro]

@francesco-romano is not highlighting the best feature of this new parser.

If we take one of our models from icub-models, and we insert a small xml syntax error, such as :

diff --git a/iCub/robots/iCubGenova04/model.urdf b/iCub/robots/iCubGenova04/model.urdf
index 3973b4c..09a2110 100644
--- a/iCub/robots/iCubGenova04/model.urdf
+++ b/iCub/robots/iCubGenova04/model.urdf
@@ -1202,7 +1202,7 @@
     <child link="r_forearm_skin_19"/>
     <dynamics damping="0.1"/>
   </joint>
-  <link name="r_forearm_skin_22"/>
+  <link name="r_forearm_skin_22">
   <joint name="r_forearm_skin_22_fixed_joint" type="fixed">
     <origin xyz="-0.0544173950421 -0.0287223 0.0306037516609" rpy="-1.2658242728 -0.234328151379 3.04788317971"/>
     <parent link="r_forearm"/>

This is the output of the old TinyXML-based parser:

straversaro@iiticublap103:~/src/icub-models/iCub/robots/iCubGenova04$ ~/src/robotology-superbuild/build/install/bin/idyntree-model-info --total-mass -m model.urdf 
[ERROR]  :: modelFromURDFString : Invalid XML
[ERROR] error in loading robot model
Impossible to read model at file model.urdf

No indication of the kind of error, beside "Invalid XML" (that is quite useless if you deal with a 3K lines XML file such as https://github.com/robotology/icub-models/blob/master/iCub/robots/iCubGenova04/model.urdf).
This instead is the output with the new libxml2-based parser:

straversaro@iiticublap103:~/src/icub-models/iCub/robots/iCubGenova04$ ~/src/robotology-superbuild/build/install/bin/idyntree-model-info --total-mass -m model.urdf 
[ERROR] XMLParser :: [Parsing] : Opening and ending tag mismatch: link line 1205 and robot

[ERROR] XMLParser :: [Parsing] : Premature end of data in tag robot line 1

[ERROR] XMLParser :: parserCallbackEndDocument : Unbalanced tags in the document
[ERROR] ModelLoader :: loadModelFromFile : Error in parsing model from URDF.
Impossible to read model at file model.urdf

There is an exact indication of the wrong line, so it is trivial to fix the problem.
This kind of feedback is actual better than the one provided by any tool based on urdfdom, as their parser is based on TinyXML as well. See for example the output of the urdf_to_graphiz ROS command:

straversaro@iiticublap103:~/src/icub-models/iCub/robots/iCubGenova04$ urdf_to_graphiz model.urdf 
Error:   Error reading end tag.
         at line 71 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/model.cpp
ERROR: Model Parsing the xml failed

[diegoferigo]

This. is. amazing. Thanks @francesco-romano! 🎉

I couldn't review all the details, but I like a lot the entire architecture, no major concerns about it. Not a big fan on indented cpps with namespaces tho, especially on new code.

When I dug into libxml2 some while ago, I found its usage a bit messy. However, it is one of the only xml feature-complete parser with a really open license. Well done!

Approving despite the minor comments.

cc @lucaTagliapietra This is exactly the pattern I was telling you about element callbacks for our qt-based parser.

[diegoferigo]

ForName sounds a bit odd, maybe FromName would be better?

[diegoferigo]

Same as above.

[traversaro]

Not a big fan on indented cpps with namespaces tho, especially on new code.

My fault, I want to do a general clang_format cleanup (using https://github.com/robotology-playground/clang-format) of iDynTree codebase as soon as we merge the pending PRs .

[traversaro]

Meta-comments about the CMake structure:

• Even if probably it is not like this in the current structure (there are some exception to this in the Qt stuff due to components that are available only in recent versions of the library), it would be ideal to keep all the find_package calls in cmake/iDynTreeDependencies.cmake and all the options in cmake/iDynTreeOptions.cmake .

• I don't get the logistics of the IDYNTREE_LEGACY_URDF_PARSER CMake option. If this option is enable, the iDynTree_LIBRARIES CMake variable contains two libraries that expose the same symbols? Furthermore, also the headers have the same name and the same #ifndef MACROS , so they would clash if both are installed.

[francesco-romano]

I will answers here to the comments:

@diegoferigo:

• indent: that is the default Xcode indentation. I could have reformat it after, but as @traversaro mentioned there is some work on clang-format. I thought that it will be reformatted anyway.
• name: the inspiration comes from methods such as tableView:cellForRowAtIndexPath:. The point is that from seems that there is a translation process behind. For instead means that it expects an element corresponding to the specified name. Indeed, for can be seen as the shorter version of corresponding.
  Thus, no. I do not like from 😄 .

@traversaro

• CMake: makes sense. Indeed, I added libxml2 in FindDependencies, but I forgot about the option.
• legacy: you are totally right. While the targets are different, the symbols are the same. I think this is your call: do you want to completely remove the old library? I do not have any objection on it (of course).

PS: @traversaro try the validation agains an XSD. That is much better than the "normal" parser error :D

[traversaro]

legacy: you are totally right. While the targets are different, the symbols are the same. I think this is your call: do you want to completely remove the old library? I do not have any objection on it (of course).

I am pushing some properly deprecated headers for all the public symbols of the old class. Once we done that, I think it is safe to just delete the old library.

[francesco-romano]

Travis (on my branch) seems happy now.
Thanks @traversaro

[traversaro]

Things missing:

• Documentation and AppVeyor (@traversaro)
• Test on the iCub robot with something serious (either YOGA++ or Walking) (@ someone? )

[ahmadgazar]

@traversaro Can you give me access rights for the repo? For the time being I don't have hands on the robot, so I will try to test the pull request on Gazebo first (also because safety first :)) as suggested by @francesco-romano .

[fjandrad]

I'll help @ahmadgazar test this.

[ahmadgazar]

Me and @fjandrad tested the pull request locally on my pc in Gazebo for yoga++, and everything seem to be working just fine. We will be testing it on a physical iCub for a final confirmation.

[ahmadgazar]

We have tested the PR on a physical iCub doing yoga++, and it has done all the yoga completely without problems. However, by adding external perturbations on the arms the robot was unable to get back to it's home position. We suspect though that this behavior has nothing to do with the changes done on the PR (someone can correct me if I am wrong).

[fjandrad]

I performed an extra check using a simpler urdf ( the one from iCalibrate.
For this I needed to merge the pr with master ( so I did locally ).
The aim of that model is to easily excite an FT sensor.
So I tested with the same motions I already do in that model and I get a different behavior.
Usual behavior of the force torque estimator looks like this:

But with the new parser I get:

Since it actually changes the behavior of the FT estimation I would hold this PR until we understand why or what it changes.

Update: I reverted the merge and tested again to see if the merge somehow messed it up but I got the same result.
cc @traversaro @francesco-romano

[francesco-romano]

Thanks guys for the work.
I do not have access to the loc2 repository (and the urdf file).
Can you upload here the urdf so that I can check where the problem might be?

[traversaro]

It may be worth to add the problematic model to https://github.com/robotology/idyntree/blob/master/src/tests/data/testModels.h.in#L28 .

[francesco-romano]

I'll do it. Send me the urdf first so that I can run some tests on it.

[fjandrad]

This is the model I used to get those plots.

icalibrate.txt

Had to change to txt to upload but is urdf.

[francesco-romano]

Thanks @fjandrad
I will take a look and I will let you know when you can run the test again!

[fjandrad]

After @traversaro pointed out that the joint order might be different for the parser and that I needed to either set a desired order through model loader or make the mapping in the code, I ran a test with and without the ordering to check.
Indeed the joint order is different, with new parser I get:

Joints: 
         [0] weight_passive_joint (dofs: 1) : weight_interface<-->weight
         [1] shoulder_pitch (dofs: 1) : base<-->shoulder_1
         [2] shoulder_roll (dofs: 1) : shoulder_1<-->shoulder_2
         [3] FT_sensor_joint (dofs: 0) : sensor_support<-->weight_interface
         [4] FT_support_fixed_joint (dofs: 0) : shoulder_2<-->sensor_support

The original order was :

Joints: 
    [0] shoulder_pitch (dofs: 1) : base<-->shoulder_1
    [1] shoulder_roll (dofs: 1) : shoulder_1<-->shoulder_2
    [2] weight_passive_joint (dofs: 1) : weight_interface<-->weight
    [3] FT_support_fixed_joint (dofs: 0) : shoulder_2<-->sensor_support
    [4] FT_sensor_joint (dofs: 0) : sensor_support<-->weight_interface

Using the model loader solved the issue correctly. So the problem was in my code and not the use of the new parser itself. Sorry for the trouble.

[francesco-romano]

Thanks @fjandrad
Yes, the order is non deterministic, and I think (but not totally sure) that it was a sort of "undefined behaviour" even on the previous parser, in the sense that the order was never documented.

For a correct workflow there are two options:

• Once the model is loaded, you get the order of the joints and use that. This is guaranteed to not change.
• You use loadReducedModel passing the desired joints ordering. This is guaranteed to change the internal order to the specified order. There is a test for this feature, so we are quite sure we won't break it unintentionally in the future.

[traversaro]

Yes, the order is non deterministic, and I think (but not totally sure) that it was a sort of "undefined behaviour" even on the previous parser, in the sense that the order was never documented.

Yes, this has always been the intended behavior at least.

[traversaro]

Thanks @francesco-romano !

[ahoarau]

Hello again iDynTree devs,
Sorry in advance for the long post and winning, but I'm having some difficulties understanding the choice of libxml2 in this PR. The PR is nice, just not libxml2 :

Context
I'm using iDynTree as an embedded external dependency for some projects. The sources are copied in a external subfolder and shipped with the apps. Example : https://github.com/syroco/orca

Now that you changed the embedded tinyxml to libxml2, I need to integrate libxml2 also so my project does not depend on more libs.

So far the negative points against libxml2 regarding integration/usage :

• Adds an external dependency that users have to install : idyntree used to have only eigen which is very easy to integrate/install/copy/runseverywhere.
• It uses autotools (2018!): if we want to embedded it ,+1 dependency (autoconf, not present in ubuntu 16 by default for example)
• It is hard to translate it to cmake : a lot of code is generated via autoconf, creating tons of c_flags to activate/include part of the code --> integration nightmare
• Stable releases are heavy ! 38.4Mb for the 2.8.9, so you have to take only the 34 files that interest your project --> integration nightmare
• It is written in C (!) so the interface is questionable in a c++14 library (that is just winning)

Positive points about libxml2: it supports XSD validation... (!)

Now Tinyxml2 : https://github.com/leethomason/tinyxml2
c++, 1 header, 1 c++ file, native cmake support, well supported, no code generated, interface is nice, integration is trivial. Done !
But no XSD validation.

So here's my question :

• is XSD validation really worth all the trouble ?

I saw in the idyntree code that libxml2 is only present in one private file (very nice), but I'm afraid the idyntree::XMLParser design was created with the libxml2 interface in mind. @francesco-romano , do you think it would be possible to use tinyxml2 in the new parser ?

Thanks for your help, idyntree is great !

[traversaro]

Hi @ahoarau, sorry for the short response, hopefully I will reply more in depth during the weekend.
In a nutshell the XSD validation and better error messages was considered to be extremely important, as in the lab we have a lot of new students/members every year, and unfortunately the tooling for automatic editing of URDF is basically non-existent: with the "old" generic error messages of TinyXML2, we always had problems for new users to identify the problem in the URDF that they manually modified. The solution in that case was typically "ask @traversaro" but that is not really scalable. I know that an expert user can easily discover automatically the source of problems in the XML file, but that is typically out of a reach of a newly graduated engineering student.

Clearly the downside is the additional external dependency, but I think there are proper solution to most of the point you raise, I will answer on that in the weekend. We tried to preemptive address some of those issues as discussed in robotology/community#316 .

I saw in the idyntree code that libxml2 is only present in one private file (very nice), but I'm afraid the idyntree::XMLParser design was created with the libxml2 interface in mind. @francesco-romano , do you think it would be possible to use tinyxml2 in the new parser ?

I am a bit afraid that having a double backed for idyntree::XMLParser will be a maintenance nightmare, especially as we want to expand the functionality of the model handling in iDynTree (mainly URDF export
and in-place update).

[ahoarau]

I understand the need to have a tool to check the XML correctness.

According to tinyxml2, it only reports line numbers :

Error Reporting
TinyXML-2 reports the line number of any errors in an XML document that cannot be parsed correctly. In addition, all nodes (elements, declarations, text, comments etc.) and attributes have a line number recorded as they are parsed. This allows an application that performs additional validation of the parsed XML document (e.g. application-implemented DTD validation) to report line number information for error messages.

I would still use this very lightweight/c++/embeddable library in the code, but print a message in case of errors saying :

XML could not be parsed, please check XSD validation against this schema : https://robotology/idyntree/some_xsd_example.xsd
Possible tools :
http://www.utilities-online.info/xsdvalidation/
https://www.freeformatter.com/xml-validator-xsd.html
https://www.corefiling.com/opensource/schemaValidate/

That would be my last point about this change. Sorry for the winning.

Meanwhile, I integrated libxml2 as a sub_dir thanks to the cmake example you showed me:
https://github.com/syroco/orca/tree/master/external/libxml2

This is a very ugly integration, as I had to trim the library, generate files via autoconf on linux/macOS, and copy them. Not very scalable but it seems to work.