Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/add urdf parser #117

Merged

Conversation

Levi-Armstrong
Copy link
Contributor

@Levi-Armstrong Levi-Armstrong commented Aug 23, 2019

This add additional functionality over what urdfdom provides related to additional shapes and support of quaternions for the origin tag.

Additional Shapes

  • Cone
  • Capsule
  • Mesh
  • Convex Mesh with ability to convert to Convex mesh
  • SDF Mesh
  • Octomap with sub shape types (Box, Sphere Inside, Sphere Outside) and an option to prune. This has a custom prune operation to limit the number of sub shapes created. Also provides support for supplying point cloud to define the octomap with resolution.
  • Origin with quaternion support for rotation. You are able to define both to not break comparability with urdfdom but this solver default to quaternion if both are available.

@traversaro
Copy link

traversaro commented Aug 24, 2019

In the proposed parser std::stod is used for converting from string to double, but unfortunately std::stod depends on the currently set global locale, so this would make this parser subject to subtle locale-related bugs such as the one described in ros/urdfdom_headers#41, ros/urdfdom#98 and robotology/idyntree#288 .

@Levi-Armstrong
Copy link
Contributor Author

@traversaro Thank you for pointing out the issue with using std::stod. I have made changes based on one of the links you provided to avoid using std::stod.

@Levi-Armstrong
Copy link
Contributor Author

@mpowelson This should be ready to review. I currently working on adding documentation on the additional feature available in this parser over urdfdom.

Copy link
Contributor

@mpowelson mpowelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Levi-Armstrong Is there a minimum version of tinyxml? On 16.04 (kinetic) I get

error: no matching function for call to 'tinyxml2::XMLElement::DoubleAttribute(const char [17], int) const'
   s->soft_lower_limit = xml_element->DoubleAttribute("soft_lower_limit", 0);
                                                                           ^
In file included from /home/mpowelson/workspaces/temp2/src/tesseract-1/tesseract/tesseract_urdf/include/tesseract_urdf/urdf_parser.h:33:0,
                 from /home/mpowelson/workspaces/temp2/src/tesseract-1/tesseract/tesseract_urdf/src/urdf_parser.cpp:27:
/usr/include/tinyxml2.h:1205:14: note: candidate: double tinyxml2::XMLElement::DoubleAttribute(const char*) const
     double   DoubleAttribute( const char* name ) const {
              ^'
/usr/include/tinyxml2.h:1205:14: note:   candidate expects 1 argument, 2 provided

tesseract/tesseract_urdf/package.xml Outdated Show resolved Hide resolved
tesseract/tesseract_urdf/include/tesseract_urdf/box.h Outdated Show resolved Hide resolved
@mpowelson
Copy link
Contributor

That review was not done. For some reason when I commented, it posted the whole review. If you want, I'll have to look at the rest next week.

Copy link
Contributor

@mpowelson mpowelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry in advance for the sprawling comments.

@mpowelson
Copy link
Contributor

@Levi-Armstrong Is there a minimum version of tinyxml? On 16.04 (kinetic) I get

error: no matching function for call to 'tinyxml2::XMLElement::DoubleAttribute(const char [17], int) const'
   s->soft_lower_limit = xml_element->DoubleAttribute("soft_lower_limit", 0);
                                                                           ^
In file included from /home/mpowelson/workspaces/temp2/src/tesseract-1/tesseract/tesseract_urdf/include/tesseract_urdf/urdf_parser.h:33:0,
                 from /home/mpowelson/workspaces/temp2/src/tesseract-1/tesseract/tesseract_urdf/src/urdf_parser.cpp:27:
/usr/include/tinyxml2.h:1205:14: note: candidate: double tinyxml2::XMLElement::DoubleAttribute(const char*) const
     double   DoubleAttribute( const char* name ) const {
              ^'
/usr/include/tinyxml2.h:1205:14: note:   candidate expects 1 argument, 2 provided

@Levi-Armstrong Ok I was mistaken. I still get this error, and it looks like CI does too.

@mpowelson
Copy link
Contributor

@Levi-Armstrong I accidentally pushed it to your branch again. I can submit a PR if you want from my fork with the same name.

@Levi-Armstrong
Copy link
Contributor Author

@mpowelson It is ready for a second look.

@mpowelson
Copy link
Contributor

@Levi-Armstrong It looks good to me. Because the tests changed files and also were modified in the same commit, I did not look at them again. I assume you fixed the things I pointed out. Everything else looks good once we get the fixes for 16.04 in there.

@Levi-Armstrong
Copy link
Contributor Author

@mpowelson Can you check the latest and make sure it build on kinetic?

@mpowelson
Copy link
Contributor

@Levi-Armstrong It needs Levi-Armstrong#23

@mpowelson
Copy link
Contributor

@Levi-Armstrong I didn't realize how bad the Eigen alignment issue was. Basically all of my examples now no longer build with this PR. Until we fix the those I'm not sure we should merge this. I'm not sure how common this issue will be.

@mpowelson mpowelson mentioned this pull request Sep 20, 2019
@Levi-Armstrong
Copy link
Contributor Author

@mpowelson I have fixed the parsing of available materials and added unit tests to verify that it works and fails when expected.

@@ -30,15 +30,15 @@
<geometry>
<mesh filename="package://tesseract_support/urdf/abb_irb2400/irb2400/visual/base_link.dae"/>
</geometry>
<material name="">
<material name="base_link_material">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Levi-Armstrong Does this change suggest that urdf_dom had a default name, but this one doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will test it with urdfdom and see if I get the same error I was getting from tesseract_urdf. It looks like tesseract_motion_planners tests are only failing on kinetic. Could you run the tests on your machine and see why they are failing?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anonymous material definitions are definitely supported by urdfdom, so the name should not be necessary -- if you want to maintain some level of compatibility with urdfdom that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gavanderhoorn Thank you. I have added support for anonymous material names.

@Levi-Armstrong
Copy link
Contributor Author

@mpowelson It looks like the issue with boost and pcl on xenial is related to pcl not supporting c++11 on xenial. They recommend if this becomes an issue to build pcl from source. To avoid doing this I added a compiler definition to disable the ability to parse point cloud files defined in the urdf for xenial builds. I could not find any workarounds.

@mpowelson
Copy link
Contributor

@Levi-Armstrong The tests run on my PC as well now.

@Levi-Armstrong Levi-Armstrong merged commit d6397ad into tesseract-robotics:master Sep 25, 2019
@Levi-Armstrong Levi-Armstrong deleted the feature/AddURDFParser branch September 25, 2019 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants