Add support for exporting additional frames in ModelExporter #557

traversaro · 2019-08-19T16:08:15Z

~~This PR builds on the top of #554, so it will not be ready to be merged until that PR is merged before.~~

Relevant commits:

5fab702 : Add Model::getLinkAdditionalFrames method
b77faa5 : Add support for exporting additional frames in ModelExporter

All addition come with the respective unit tests and documentation.

traversaro · 2019-08-23T12:58:11Z

@prashanthr05 this PR is now ready for review, thanks!

prashanthr05 · 2019-08-24T14:18:41Z

src/model/src/Model.cpp

+    }
+
+    frameIndices.resize(0);
+    for (FrameIndex frameIndex=this->getNrOfLinks(); frameIndex < this->getNrOfFrames(); frameIndex++) {


For quicker understanding and readabilityof code, it would be great to document the relationship between the link serialization and the additional frames serialization, also here.

I see it is documented here,

idyntree/src/model/include/iDynTree/Model/Model.h

Line 63 in ed3e020

* The frame indices between 0 and getNrOfLinks()-1 are always assigned to the

In that case, we can give a pointer to this documentation here.

Fixed in 81e7f7a .

prashanthr05 · 2019-08-24T14:19:39Z

src/model/include/iDynTree/Model/Model.h

+         * @param[out] frames a vector of FrameIndex of the frame indeces attached to the specified link index,
+         * @return true if the specified link is a valid link, false otherwise.
+         */
+        bool getLinkAdditionalFrames(const LinkIndex lnkIndex, std::vector<FrameIndex>& frameIndeces) const;


Does it also make sense to have an overload with linkName as input argument?

Not specifically for this scenario. but in general nice-to-have feature in the Model API.

Similar overload for frameNames as output?

It could make sense, but I would leave this to a separate PR. Probably we can have a guideline to have for all methods in iDynTree::Model both the index and the names one, and then have a PR for add names-variant for all methods missing them. Can you open a new issue on this?

prashanthr05 · 2019-08-24T14:23:53Z

src/model_io/urdf/include/iDynTree/ModelIO/ModelExporter.h

@@ -63,11 +63,30 @@ class ModelExporterOptions
 *
 * Furthermore, currently the model exporter only exports a subset of the features
 * supported in iDynTree::Model. In particular, the following features are not exported:
- * * Additional frames
+ *


I like the way how Additional Frames disappeared ;D

prashanthr05 · 2019-08-24T14:27:37Z

src/model_io/urdf/include/iDynTree/ModelIO/ModelExporter.h

+ * due to workaround a bug in official %KDL parser used in ROS (see https://github.com/ros/kdl_parser/issues/27 for more info). For this reason,
+ * if the selected base_link has at least one additional frame, the first additional frame of the base link is added as a **parent** fake URDF link,
+ * instead as a **child** fake URDF link as done with the rest of %iDynTree's additional frames. If no additional frame is available for the base link,
+ * the base link of the URDF will have a mass, and will generate a warning then used with the ROS's [`kdl_parser`](https://github.com/ros/kdl_parser) .


Wow, is this the reason why Travis is failing in this pull request robotology/icub-models-generator#113 related to icub-model-generator?

I do not think so, I think that is an error is always there even if non-failing PRs.

What I thought was, due to the parent-child direction inverting workaround, the root_tlink-base_link gets inverted parent reference and the base link might have a mass throwing an error?

Because, the segfaults was due to the fact that root_link has a described and the exact words from the failure read...

The root link root_link has an inertia specified in the URDF, but KDL does not support a root link with an inertia. As a workaround, you can add an extra dummy link to your URDF. No link found with name base_link

Would that be possible?

Because, the segfaults was due to the fact that root_link has a described and the exact words from the failure read...

Are you sure that the same error were not present in the successful builds?

Are you sure that the same error were not present in the successful builds?

Uhmm.. of this, I am not sure.

prashanthr05 · 2019-08-24T14:33:36Z

src/model_io/urdf/src/URDFModelExport.cpp

+ * <parent_element>
+ *   <link name="<frame_name>"/>
+ *   <joint name="<frame_name>_fixed_joint" type="fixed">
+ *      <origin xyz="0.1 0.2 0.3" rpy="0 0 3.1416"/>


The numbers here are misleading. Can we change them to placeholders?

UPDATE: Ok, given that this is just an example of it might look on the generated URDF, please ignore my comment.

prashanthr05 · 2019-08-24T14:41:50Z

src/model_io/urdf/src/URDFModelExport.cpp

+
+    // Export fake link
+    xmlNodePtr link_xml = xmlNewChild(parent_element, NULL, BAD_CAST "link", NULL);
+    xmlNewProp(link_xml, BAD_CAST "name", BAD_CAST frame_name.c_str());


If any future developer asks why there is BAD_CAST all across the code, we could possibly document this https://stackoverflow.com/questions/45058878/why-libxml2-uses-bad-cast-everywhere-in-a-c-c-code somewhere in the code ?

Fixed in ee923f4 .

prashanthr05 · 2019-08-24T14:44:39Z

src/model_io/urdf/src/URDFModelExport.cpp

+    // child
+    xmlNodePtr child_xml = xmlNewChild(joint_xml, NULL, BAD_CAST "child", NULL);
+    xmlNewProp(child_xml, BAD_CAST "link", BAD_CAST child_of_fake_joint.c_str());
+


I see a common trend of using the method xmlNewChild and xmlNewProp sequentially. Can we not make a function that does the composition of these two steps?

I see this also depends on how many properties we add in sequence. So the complexity of having a single function to handle all properties might be big. But can still be handled by a vector of key-value pairs.

Yes, this could make sense, but at least initially I preferred to avoid start writing a C++ wrapper on the top of the libxml2 API, without having a clear idea about our need from the libxml2's API. Furthermore, I am a bit afraid that adding a layer between the libxml2's API and the parser could impact readability. However, a C++ wrapper could make sense, but I would prefer to complete the URDF exporter implementation to understand our needs, and eventually do an libxml2 abstraction as a refactoring. What do you think?

traversaro · 2019-08-26T09:28:22Z

I should have addressed all your comments @prashanthr05 .

prashanthr05 · 2019-08-27T08:59:30Z

src/model_io/urdf/tests/ModelExporterUnitTest.cpp

-
-    // TODO(traversaro) : uncomment the following line when frames are correctly handled by the exporter
-    // ASSERT_EQUAL_DOUBLE(model.getNrOfFrames(), modelReloaded.getNrOfLinks());
-


Thank you for remembering to remove this. Sorry I missed this in the review!

To return all the additional frames connected to a given link.

…DF support

traversaro · 2019-08-27T09:05:35Z

Thanks @prashanthr05 !

traversaro force-pushed the feature/URDFexportAdditionalFrames branch 2 times, most recently from 2173398 to 726c917 Compare August 23, 2019 12:52

prashanthr05 reviewed Aug 24, 2019

View reviewed changes

traversaro mentioned this pull request Aug 26, 2019

Add support in ModelExporter to export visual and collision geometries to URDF files #565

Merged

prashanthr05 reviewed Aug 27, 2019

View reviewed changes

prashanthr05 approved these changes Aug 27, 2019

View reviewed changes

traversaro added 2 commits August 27, 2019 11:04

Add Model::getLinkAdditionalFrames method

62a0bbf

To return all the additional frames connected to a given link.

Add support for exporting additional frames in the ModelExporter's UR…

358beda

…DF support

traversaro force-pushed the feature/URDFexportAdditionalFrames branch from 805d1fe to 358beda Compare August 27, 2019 09:05

traversaro merged commit af0effb into robotology:devel Aug 27, 2019

traversaro mentioned this pull request Sep 2, 2019

Support URDF export #536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for exporting additional frames in ModelExporter #557

Add support for exporting additional frames in ModelExporter #557

traversaro commented Aug 19, 2019 •

edited

Loading

traversaro commented Aug 23, 2019

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019

traversaro Aug 26, 2019

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019

traversaro Aug 26, 2019

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019

traversaro Aug 24, 2019

prashanthr05 Aug 27, 2019 •

edited

Loading

traversaro Aug 27, 2019

prashanthr05 Aug 27, 2019

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019

traversaro Aug 26, 2019

prashanthr05 Aug 24, 2019

prashanthr05 Aug 24, 2019 •

edited

Loading

traversaro Aug 26, 2019

prashanthr05 Aug 27, 2019

traversaro commented Aug 26, 2019

prashanthr05 Aug 27, 2019

traversaro commented Aug 27, 2019


		// TODO(traversaro) : uncomment the following line when frames are correctly handled by the exporter
		// ASSERT_EQUAL_DOUBLE(model.getNrOfFrames(), modelReloaded.getNrOfLinks());

Add support for exporting additional frames in ModelExporter #557

Add support for exporting additional frames in ModelExporter #557

Conversation

traversaro commented Aug 19, 2019 • edited Loading

traversaro commented Aug 23, 2019

prashanthr05 Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanthr05 Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanthr05 Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanthr05 Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanthr05 Aug 24, 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

prashanthr05 Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Aug 26, 2019

Choose a reason for hiding this comment

traversaro commented Aug 27, 2019

traversaro commented Aug 19, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 27, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019 •

edited

Loading

prashanthr05 Aug 24, 2019 •

edited

Loading