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

Standalone kdl_parser library #13

Merged
merged 10 commits into from
Mar 19, 2019
Merged

Standalone kdl_parser library #13

merged 10 commits into from
Mar 19, 2019

Conversation

christian-rauch
Copy link
Contributor

As proposed in #12, this PR makes catkin and rosconsole optional. I.e. plain CMake macros are used if catkin is not available and logging is done via std io streams if rosconsole is not available.

Since urdf itself depends on ROS for similar reasons, it has been replaced by urdfdom. This disables some of the functions like treeFromParam and treeFromXml, which use methods that are not present in urdfdom. In the log term it would be beneficial, if urdf also would be disentangled from ROS.

I copied FindTinyXML.cmake and FindTinyXML2.cmake from https://github.com/ros/cmake_modules into this repo, to not introduce another dependency.

@christian-rauch
Copy link
Contributor Author

@sloretz Can you comment on this PR?

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and your patience @christian-rauch . Comments are below.

kdl_parser/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,74 @@
##################################################################################################
#
# CMake script for finding TinyXML.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to say it looks like this was copied from urdfdom, in which case this shouldn't be needed since find_package(urdfdom REQUIRED) is used in the CMakeLists.txt, but it looks like urdfdom is not find_packageing TinyXML in its <project>-config.cmake like it should

Copy link
Contributor

Choose a reason for hiding this comment

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

kdl_parser/cmake/FindTinyXML2.cmake Show resolved Hide resolved
@@ -77,8 +79,10 @@ bool treeFromString(const std::string & xml, KDL::Tree & tree);
* \param[out] tree The resulting KDL Tree
* \return true on success, false on failure
*/
#ifdef HAS_URDF
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather keep the signature, but return False if the library was not built with urdf. That way a downstream package including kdl_parser.hpp doesn't have to define HAS_URDF themselves.

@clalancette thoughts?

kdl_parser/src/kdl_parser.cpp Outdated Show resolved Hide resolved
kdl_parser/src/kdl_parser.cpp Show resolved Hide resolved
kdl_parser/src/kdl_parser.cpp Show resolved Hide resolved
kdl_parser/src/kdl_parser.cpp Outdated Show resolved Hide resolved
@christian-rauch
Copy link
Contributor Author

Thanks for coming back to this PR.

I discovered that the non-catkin version doesn't build any more.
The urdf lib version in ROS melodic (1.13) uses TinyXML2, i.e. initXml (const tinyxml2::XMLDocument *xml), while the Ubuntu repo (version 1.12) still uses TinyXML(1), i.e. initXml (TiXmlElement *xml), like in ROS kinetic.
I might have intended to base this PR on kinetic-devel instead of melodic-devel.

Anyway, should we maybe use urdf::parseURDF from (urdf_parser / liburdfdom-dev) instead of the manual XML loading and parsing?

@christian-rauch
Copy link
Contributor Author

I went ahead and replaced most of the urdf library with urdfdom. The urdf library is basically only required if you want to read a URDF model from a ROS parameter. Maybe this could also replaced by ROS methods to remove the urdf library dependency completely.

kdl_parser/include/kdl_parser/kdl_parser.hpp Outdated Show resolved Hide resolved
kdl_parser/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM! @clalancette Mind taking a look at this one too?

I ran check_kdl_parser on test/pr2_desc.xml building with ROS and without. The only difference I see is how ROS_WARN/ROS_ERROR are output, which is fine.

When building with ROS ROS_WARN is output to stdout with formatting that includes a timestamp and console colors. When building without ROS the warning is sent to stderr and is just the text of the warning itself.

@christian-rauch
Copy link
Contributor Author

@sloretz @clalancette Any news/feedback on this?

@sloretz
Copy link
Contributor

sloretz commented Mar 19, 2019

ping @clalancette , ok to merge this one?

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One minor change, but I'll still approve.

kdl_parser/src/kdl_parser.cpp Outdated Show resolved Hide resolved
@sloretz
Copy link
Contributor

sloretz commented Mar 19, 2019

Thanks for the PR!

@sloretz sloretz merged commit 4454445 into ros:melodic-devel Mar 19, 2019
@christian-rauch christian-rauch deleted the standalone branch March 19, 2019 14:31
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.

3 participants