Add ModelExporter class and add preliminary support for exporting URDF from iDynTree::Model #554

traversaro · 2019-08-14T08:51:45Z

Add iDynTree::ModelExporter class and related tests.

Currently the only format supported for export is the URDF format.

Currently the model exporter only exports a subset of the features supported in iDynTree::Model. In particular, the following features are not supported:

Additional frames
Visual and Collision Geometries
Sensors
Joint limits, friction parameters
Automatically change link frames location for link whose origin does not lay on the rotation/translation axis of the parent joint, as requested by URDF
Wrapping the ModelExporter in bindings

The PR is composed by the following two commits:

5982124 : Add fpconv as a MIT-licensed vendored dependency, to support double - shortest string exact conversion, that is currently not supported in the STL. This will be removed as soon as we can depend on C++17's std::to_chars
047ef63 : Add ModelExporter class, support for exporting URDF, and tests.

My idea is to implement most of the missing features before 0.12 release, but as the limitations are currently documented, I think it make sense to first review this initial PR, and then the other features can be added in future PRs.

This PR is based on the old closed PR #382 .

This dependency is necessary to convert a double to its shortest string exact representation. It can be probably removed as soon as std::to_chars is widely available.

prashanthr05 · 2019-08-14T09:35:47Z

src/model_io/urdf/include/private/URDFModelExport.h

+ *
+ * @warning This function does not support exporting sensor or solid shapes at the moment.
+ *
+ * @param[in] options the URDFParserOptions struct of options passed to the parser


It'd be great to document also the output and default parameters here. Also, if we should specify a relative path or an absolute path of the urdf file. These details are not clear.

For the default options, the user is referred to the iDynTree::ModelExportOptions, that contains docs related to the available and the default options. I will make it clear in the commit to address reviews, let me know if it is enough.

src/model_io/urdf/src/URDFModelExport.cpp

prashanthr05 · 2019-08-14T09:49:11Z

src/model_io/urdf/src/URDFModelExport.cpp

+    xmlNodePtr child_xml = xmlNewChild(joint_xml, NULL, BAD_CAST "child", NULL);
+    xmlNewProp(child_xml, BAD_CAST "link", BAD_CAST model.getLinkName(childLink->getIndex()).c_str());
+
+    // TODO(traversaro) : handle joint limits and friction


This should be moved also in the documentation.

I agree this is not mentioned in the description of the pr

I think this is still pending

src/model_io/urdf/src/URDFModelExport.cpp

prashanthr05 · 2019-08-14T09:51:10Z

src/model_io/urdf/src/URDFModelExport.cpp

+    xmlNewProp(robot, BAD_CAST "name", BAD_CAST "iDynTreeURDFModelExportModelName");
+    xmlDocSetRootElement(urdf_xml, robot);
+
+    // TODO(traversaro) : We are assuming that the model has no loops,


This must be mentioned in the documentation, explicitly. Since I notice there are users of iDynTree in the cable robots or parallel mechanisms community as well.

I think this should be in the main readme of idyntree not only in this pr
I thought it was there but I could not find a mention to it now.

Related issue: #555 .

fjandrad

Small doubt about the unit test not checking for the sensors

src/model_io/urdf/tests/ModelExporterUnitTest.cpp

prashanthr05 · 2019-08-14T14:01:00Z

I think it will be helpful if we add an explicit print to console statement in the URDFStringFromModel or URDFFromModel functions about the elements that the user should not expect in the generated urdf file. Like the ones described in the PR. One step forward in avoiding any possible errors while using the generated urdf.

traversaro · 2019-08-14T14:19:40Z

I think it will be helpful if we add an explicit print to console statement in the URDFStringFromModel or URDFFromModel functions about the elements that the user should not expect in the generated urdf file. Like the ones described in the PR. One step forward in avoiding any possible errors while using the generated urdf.

I totally agree with this suggestion, but I expect to implement most of those features before the 0.12 release, so I would prefer to avoid overthinking how to report this problems to the user, as eventually we will need to remove them. If for the 0.12 release we still miss some features, then I would agree to add those error messages.

traversaro · 2019-08-17T17:51:42Z

Reviews addressed in 15f4848 .

fjandrad

One small variable name to check and some documentation concerns

fjandrad · 2019-08-18T13:19:12Z

src/model_io/urdf/src/URDFModelExport.cpp

+    ok = ok && exportTransform(Transform(Rotation::Identity(), inertia.getCenterOfMass()), inertial);
+
+    xmlNodePtr inertia_xml = xmlNewChild(inertial, NULL, BAD_CAST "inertia", NULL);
+    RotationalInertiaRaw rotInertia = i.getRotationalInertiaWrtCenterOfMass();


I think this one was also meant to be inertia and not i

That happens when you modify the code and you do not try to recompile it. : )
Thanks for the check.

fjandrad · 2019-08-18T13:24:03Z

src/model_io/urdf/src/URDFModelExport.cpp

+    xmlNewProp(robot, BAD_CAST "name", BAD_CAST "iDynTreeURDFModelExportModelName");
+    xmlDocSetRootElement(urdf_xml, robot);
+
+    // TODO(traversaro) : We are assuming that the model has no loops,


I think this should be in the main readme of idyntree not only in this pr
I thought it was there but I could not find a mention to it now.

fjandrad · 2019-08-18T13:24:19Z

src/model_io/urdf/src/URDFModelExport.cpp

+    xmlNodePtr child_xml = xmlNewChild(joint_xml, NULL, BAD_CAST "child", NULL);
+    xmlNewProp(child_xml, BAD_CAST "link", BAD_CAST model.getLinkName(childLink->getIndex()).c_str());
+
+    // TODO(traversaro) : handle joint limits and friction


I think this is still pending

traversaro · 2019-08-19T07:16:00Z

@fjandrad I am not sure why I cannot directly respond to your comments, anyhow I reply to them here.

I think this should be in the main readme of idyntree not only in this pr
I thought it was there but I could not find a mention to it now.

It make sense, I opened a new issue for this in #555 .

I think this is still pending

I already added it in 15f4848#diff-05b37777f3462332b9cce4040d3f16deR69 for the main user facing documentation, and I added it in the specific function private docs in ba04aaa .

As far as I know all your comments should be addressed.

traversaro · 2019-08-20T08:54:08Z

Friendly ping @prashanthr05 @fjandrad .

traversaro · 2019-08-20T08:59:10Z

A legit problem in the build system on macOS was detected by Travis in https://travis-ci.org/robotology/idyntree/jobs/573683257, and it should be fixed by aaf1dec .

traversaro · 2019-08-21T12:12:21Z

Friend ping @fjandrad

fixup

traversaro · 2019-08-23T12:19:04Z

Thannks for the reviews @fjandrad and @prashanthr05 !

traversaro force-pushed the feature/exportURDF branch from b68e41a to 047ef63 Compare August 14, 2019 08:53

Add external vendored dependency on the fpconv library

219916a

This dependency is necessary to convert a double to its shortest string exact representation. It can be probably removed as soon as std::to_chars is widely available.

traversaro force-pushed the feature/exportURDF branch from 047ef63 to 710961b Compare August 14, 2019 08:57

prashanthr05 reviewed Aug 14, 2019

View reviewed changes

src/model_io/urdf/src/URDFModelExport.cpp Show resolved Hide resolved

prashanthr05 reviewed Aug 14, 2019

View reviewed changes

src/model_io/urdf/src/URDFModelExport.cpp Show resolved Hide resolved

prashanthr05 reviewed Aug 14, 2019

View reviewed changes

src/model_io/urdf/src/URDFModelExport.cpp Show resolved Hide resolved

prashanthr05 reviewed Aug 14, 2019

View reviewed changes

fjandrad reviewed Aug 14, 2019

View reviewed changes

src/model_io/urdf/tests/ModelExporterUnitTest.cpp Show resolved Hide resolved

fjandrad reviewed Aug 18, 2019

View reviewed changes

traversaro mentioned this pull request Aug 19, 2019

Main README/doc entry point should document that iDynTree currently support only multibody models without any loops #555

Open

traversaro mentioned this pull request Aug 19, 2019

Add support for exporting additional frames in ModelExporter #557

Merged

prashanthr05 approved these changes Aug 20, 2019

View reviewed changes

fjandrad approved these changes Aug 22, 2019

View reviewed changes

Add ModelExporter class, initial support for URDF export and tests

a25f21e

fixup

traversaro force-pushed the feature/exportURDF branch from aaf1dec to a25f21e Compare August 22, 2019 13:03

traversaro merged commit e326aa6 into robotology:devel Aug 23, 2019

traversaro deleted the feature/exportURDF branch August 23, 2019 12:19

traversaro mentioned this pull request Sep 2, 2019

Support URDF export #536

Closed

traversaro mentioned this pull request Sep 13, 2019

Add ModelCalibrationHelper class #576

Merged

traversaro mentioned this pull request Sep 26, 2020

Impossible to set timestep lower then 1e-7 robotology-legacy/gym-ignition#245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ModelExporter class and add preliminary support for exporting URDF from iDynTree::Model #554

Add ModelExporter class and add preliminary support for exporting URDF from iDynTree::Model #554

traversaro commented Aug 14, 2019 •

edited

Loading

prashanthr05 Aug 14, 2019 •

edited

Loading

traversaro Aug 17, 2019

prashanthr05 Aug 14, 2019

fjandrad Aug 14, 2019

fjandrad Aug 18, 2019

prashanthr05 Aug 14, 2019

fjandrad Aug 18, 2019

traversaro Aug 20, 2019

fjandrad left a comment

prashanthr05 commented Aug 14, 2019

traversaro commented Aug 14, 2019

traversaro commented Aug 17, 2019

fjandrad left a comment

fjandrad Aug 18, 2019

traversaro Aug 19, 2019

fjandrad Aug 18, 2019

fjandrad Aug 18, 2019

traversaro commented Aug 19, 2019 •

edited

Loading

traversaro commented Aug 20, 2019

traversaro commented Aug 20, 2019

traversaro commented Aug 21, 2019

traversaro commented Aug 23, 2019

Add ModelExporter class and add preliminary support for exporting URDF from iDynTree::Model #554

Add ModelExporter class and add preliminary support for exporting URDF from iDynTree::Model #554

Conversation

traversaro commented Aug 14, 2019 • edited Loading

prashanthr05 Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjandrad left a comment

Choose a reason for hiding this comment

prashanthr05 commented Aug 14, 2019

traversaro commented Aug 14, 2019

traversaro commented Aug 17, 2019

fjandrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Aug 19, 2019 • edited Loading

traversaro commented Aug 20, 2019

traversaro commented Aug 20, 2019

traversaro commented Aug 21, 2019

traversaro commented Aug 23, 2019

traversaro commented Aug 14, 2019 •

edited

Loading

prashanthr05 Aug 14, 2019 •

edited

Loading

traversaro commented Aug 19, 2019 •

edited

Loading