Herbathon: add makeBodyFromUrdf convenience function #388
Conversation
include/aikido/io/util.hpp
Outdated
| namespace io { | ||
|
|
||
| const SkeletonPtr makeBodyFromURDF( | ||
| const std::shared_ptr<aikido::io::CatkinResourceRetriever> |
There was a problem hiding this comment.
Maybe this can be a dart::common::ResourceRetrieverPtr, since that's what the DART URDF loader is expecting.
include/aikido/io/util.hpp
Outdated
| namespace aikido { | ||
| namespace io { | ||
|
|
||
| const SkeletonPtr makeBodyFromURDF( |
There was a problem hiding this comment.
Let's rename this to loadSkeletonFromURDF.
include/aikido/io/util.hpp
Outdated
| const std::shared_ptr<aikido::io::CatkinResourceRetriever> | ||
| resourceRetriever, | ||
| const std::string& uri, | ||
| const Eigen::Isometry3d& transform); |
There was a problem hiding this comment.
Maybe we can make this an optional argument by setting its default value to identity.
include/aikido/io/util.hpp
Outdated
| namespace aikido { | ||
| namespace io { | ||
|
|
||
| const SkeletonPtr makeBodyFromURDF( |
There was a problem hiding this comment.
Nit: Returning by const value is generally unnecessary. Let's just return SkeletonPtr.
include/aikido/io/util.hpp
Outdated
| #include <dart/dart.hpp> | ||
| #include <dart/utils/urdf/DartLoader.hpp> | ||
|
|
||
| using dart::dynamics::SkeletonPtr; |
There was a problem hiding this comment.
We should avoid using in headers because it can pollute namespaces by including this header. Let's remove and use explicitly dart::dynamics::SkeletonPtr in headers. You could use using in sources.
include/aikido/io/util.hpp
Outdated
| #ifndef AIKIDO_IO_UTIL_HPP_ | ||
| #define AIKIDO_IO_UTIL_HPP_ | ||
|
|
||
| #include <aikido/io/CatkinResourceRetriever.hpp> |
There was a problem hiding this comment.
Nit: Our convention is to:
(1) use "..." for including AIKIDO headers
(2) include headers in order (in header file): (a) system headers (i.e., STL), (2) 3rd party libraries, (3) AIKIDO headers.
So this should be:
#include <dart/dart.hpp>
#include <dart/utils/urdf/DartLoader.hpp>
#include "aikido/io/CatkinResourceRetriever.hpp"There was a problem hiding this comment.
I'll change it but could you tell me if there is a specific reason for the header include order? Does it have an advantage to do it this way?
There was a problem hiding this comment.
This header ordering generally follows the Google C++ style convention:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
src/io/util.cpp
Outdated
| throw std::runtime_error("unable to load '" + uri + "'"); | ||
|
|
||
| dynamic_cast<dart::dynamics::FreeJoint*>(skeleton->getJoint(0)) | ||
| ->setTransform(transform); |
There was a problem hiding this comment.
Need nullity check after dynamic casting.
src/io/util.cpp
Outdated
| if (!skeleton) | ||
| throw std::runtime_error("unable to load '" + uri + "'"); | ||
|
|
||
| dynamic_cast<dart::dynamics::FreeJoint*>(skeleton->getJoint(0)) |
There was a problem hiding this comment.
I think we can also make this skeleton->getRootJoint() instead of skeleton->getJoint(0). @jslee02 can you confirm?
There was a problem hiding this comment.
Correct!
Also, we should check whether skeleton is not empty.
Codecov Report
@@ Coverage Diff @@
## master #388 +/- ##
==========================================
- Coverage 81.11% 80.96% -0.16%
==========================================
Files 200 201 +1
Lines 5693 5705 +12
==========================================
+ Hits 4618 4619 +1
- Misses 1075 1086 +11
|
include/aikido/io/util.hpp
Outdated
| namespace aikido { | ||
| namespace io { | ||
|
|
||
| dart::dynamics::SkeletonPtr loadSkeletonFromURDF( |
src/io/util.cpp
Outdated
| namespace aikido { | ||
| namespace io { | ||
|
|
||
| dart::dynamics::SkeletonPtr loadSkeletonFromURDF( |
There was a problem hiding this comment.
Nit: We typically put "//==================================" at the beginning of a new method. Please check other cpp files.
src/io/util.cpp
Outdated
| dart::dynamics::SkeletonPtr loadSkeletonFromURDF( | ||
| const dart::common::ResourceRetrieverPtr resourceRetriever, | ||
| const std::string& uri, | ||
| const Eigen::Isometry3d& transform = Eigen::Isometry3d::Identity()) |
There was a problem hiding this comment.
I would prefer making the default param Eigen::Isometry3d::identity() be specified at the header file to make it more explicit.
src/io/util.cpp
Outdated
| = urdfLoader.parseSkeleton(uri, resourceRetriever); | ||
|
|
||
| if (!skeleton) | ||
| throw std::runtime_error("unable to load '" + uri + "'"); |
src/io/util.cpp
Outdated
| if (freeJoint == nullptr) | ||
| { | ||
| throw std::runtime_error( | ||
| "unable to cast Skeleton's root joint to FreeJoint."); |
* added makeBodyFromURDF convenience function (see herbathon trello) * added cpp, configured CMakeLists * integrated PR feedback about loadSkeletonFromUrdf * formatting * documentation * Minor cleanup.
Implemented for the Herbathon. I thought a small util file would be appropriate for stuff like this. What do you think?
I tested the function with the food manipulation demo.