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

Make urdf plugable and revive urdf_parser_plugin #13

Merged
merged 36 commits into from
Sep 18, 2020
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c105a5d
Make urdf plugable
sloretz Jul 15, 2020
4608277
Restore dependency on urdfdom
sloretz Jul 27, 2020
683c3ff
Style
sloretz Jul 28, 2020
2058537
Stops crash; not sure why TODO
sloretz Jul 28, 2020
535c275
Add benchmark showing plugin overhead
sloretz Jul 28, 2020
e310409
Whitespace
sloretz Jul 28, 2020
7250733
CMake 3.5
sloretz Aug 5, 2020
7efedb4
Include <string>
sloretz Aug 5, 2020
288413b
Remove buildtool_export on ament_cmake
sloretz Aug 5, 2020
f9b2176
exec_depend -> build_export_depend urdfdom headers
sloretz Aug 5, 2020
e09b0e1
Remove commented code
sloretz Aug 5, 2020
3adaaba
Document urdf_parser_plugin usage
sloretz Aug 5, 2020
d079336
Document PIMPL forward declaration
sloretz Aug 5, 2020
41fa3f7
Alphabetize dependencies
sloretz Aug 5, 2020
cb348db
Use tinyxml2 to reduce false positive might_handle()
sloretz Aug 5, 2020
4ba3e05
Update urdf/src/urdf_plugin.cpp
sloretz Aug 5, 2020
33ad016
Handle pluginlib exceptions
sloretz Aug 5, 2020
7055891
Merge branch 'ros2-make_urdf_plugable' of github.com:ros2/urdf into r…
sloretz Aug 5, 2020
8bb27d1
Return early on failure
sloretz Aug 5, 2020
c9f629d
Document size_t max is no confidence score
sloretz Aug 6, 2020
0076d76
Remove debut print
sloretz Aug 6, 2020
6df48c9
Move urdfdom_headers comment one line below
sloretz Aug 6, 2020
3841ea4
Avoid using nullptr in release mode
sloretz Aug 6, 2020
572534d
nonvirtual dtor final class
sloretz Aug 18, 2020
4f43841
Use ROS 2 urdfdom_headers
sloretz Aug 18, 2020
9494566
Remove unused exec variable
sloretz Aug 18, 2020
aac7fd3
Skip if xml fails to parse
sloretz Aug 19, 2020
67df5dd
Make sure test can find pluginlib plugin
sloretz Aug 19, 2020
deabb1b
Use SHARED instead of module
sloretz Aug 19, 2020
70a81cc
picked -> chosen
sloretz Aug 19, 2020
3341988
Use pluginlib_enable_plugin_testing()
sloretz Aug 25, 2020
69b3d49
Define might_handle() return to length of data
sloretz Aug 25, 2020
025f816
Return data.size() when not confident
sloretz Aug 25, 2020
31014c0
Try urdf when no plugin is confident
sloretz Aug 25, 2020
06474e8
ModelImplementation final
sloretz Sep 14, 2020
f9fe6f9
Initialize best_plugin to nullptr
sloretz Sep 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions urdf/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ cmake_minimum_required(VERSION 3.5)
project(urdf)

find_package(ament_cmake_ros REQUIRED)
find_package(pluginlib REQUIRED)
find_package(urdf_parser_plugin REQUIRED)
find_package(urdfdom REQUIRED)
find_package(urdfdom_headers REQUIRED)
find_package(tinyxml_vendor REQUIRED)
Expand Down Expand Up @@ -33,8 +35,10 @@ target_include_directories(${PROJECT_NAME}
"$<INSTALL_INTERFACE:include>"
)
ament_target_dependencies(${PROJECT_NAME}
urdf_parser_plugin
urdfdom
urdfdom_headers
pluginlib
TinyXML)

if(WIN32)
Expand All @@ -45,6 +49,22 @@ if(APPLE)
set_target_properties(${PROJECT_NAME} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
endif()

add_library(urdf_xml_parser SHARED
src/urdf_plugin.cpp
)
target_link_libraries(urdf_xml_parser
${PROJECT_NAME}
)
ament_target_dependencies(urdf_xml_parser
"pluginlib"
"urdf_parser_plugin"
)

install(TARGETS urdf_xml_parser
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin)

install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
Expand All @@ -56,6 +76,7 @@ install(DIRECTORY include/${PROJECT_NAME}/
if(BUILD_TESTING)
find_package(ament_cmake_cppcheck REQUIRED)
find_package(ament_cmake_cpplint REQUIRED)
find_package(ament_cmake_google_benchmark REQUIRED)
find_package(ament_cmake_lint_cmake REQUIRED)
find_package(ament_cmake_uncrustify REQUIRED)
# This forces cppcheck to consider all files in this project to be C++,
Expand All @@ -65,13 +86,51 @@ if(BUILD_TESTING)
ament_cpplint()
ament_lint_cmake()
ament_uncrustify(LANGUAGE "C++")

# Write files to enable pluginlib to find urdf_xml_parser in build folder
set(fake_urdf_prefix "${CMAKE_CURRENT_BINARY_DIR}/fake_urdf_install")
# Copy urdf_xml_parser to fake prefix lib/ and bin/
file(MAKE_DIRECTORY "${fake_urdf_prefix}/lib")
file(MAKE_DIRECTORY "${fake_urdf_prefix}/bin")
add_custom_command(TARGET urdf_xml_parser
POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:urdf_xml_parser>
"${fake_urdf_prefix}/lib/"
)
add_custom_command(TARGET urdf_xml_parser
POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:urdf_xml_parser>
"${fake_urdf_prefix}/bin/"
)
# Fake install plugin description file
file(COPY "urdf_parser_description.xml" DESTINATION "${fake_urdf_prefix}/share/urdf/")
# Fake install package.xml
configure_file("package.xml" "${fake_urdf_prefix}/share/urdf/package.xml" COPYONLY)
# Fake install entry in ament_index telling pluginlib where to find plugin description
file(WRITE "${fake_urdf_prefix}/share/ament_index/resource_index/urdf_parser_plugin__pluginlib__plugin/urdf"
"share/urdf/urdf_parser_description.xml\n")
# Fake install entry in ament_index declaring this a package
file(WRITE "${fake_urdf_prefix}/share/ament_index/resource_index/packages/urdf" "")

Choose a reason for hiding this comment

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

Well, yuck. If I'm understanding this properly, this is more-or-less recreating the install directory structure under build. I guess its OK, but it seems pretty fragile to me; if we change the ament structure for any reason, this is going to break.

How valuable do you think this performance test is? I'm wondering if it would be more straightforward to not include it at all.

Copy link
Author

Choose a reason for hiding this comment

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

Well, yuck. If I'm understanding this properly, this is more-or-less recreating the install directory structure under build. I guess its OK, but it seems pretty fragile to me; if we change the ament structure for any reason, this is going to break.

Yeah. It will break if how pluginlib uses the ament index is changed. I copied this logic from pluginlib's own tests. A cmake function provided by pluginlib that implemented this would eliminate the duplication if something does change

https://github.com/ros/pluginlib/blob/7c3e7d5b7af6d776079e2e9cba2ac9a5581777b0/pluginlib/CMakeLists.txt#L86-L94

How valuable do you think this performance test is? I'm wondering if it would be more straightforward to not include it at all.

It's straight forward to delete it, though it does give valuable info about the overhead of the plugin infrastructure, and it does prove the plugin infrastructure works. I would rather robustify it than delete it.

Choose a reason for hiding this comment

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

That's a valid point @clalancette, these performance tests we just a guide to make sure that the new plugin-based file type check weren't introducing large amounts of overhead. While no one should be parsing URDF/SDF inside their control loop, we don't want this plugin structure to have a substantial impact on existing code execution times.

I think it would be fine to refrain from including this install deployment. This code was more-or-less intended as a one-off verification that the plugin checking times were reasonable. @sloretz can you recap the results of the tests here? Would they be impacted at all by the size of the URDF/SDFormat file being parsed?

Copy link
Author

Choose a reason for hiding this comment

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

I think it would be fine to refrain from including this install deployment. This code was more-or-less intended as a one-off verification that the plugin checking times were reasonable.

I would recommend keeping it as it's the only test that makes sure the urdf plugin is loadable.

@sloretz can you recap the results of the tests here?

Linux amd64

  • direct - 0.015 ms
  • plugin - 0.743 ms

Linux aarch64

  • direct - 0.045 ms
  • plugin - 3.009 ms

OSX

  • direct - 0.012 ms
  • plugin - 2.371 ms

Windows

  • direct - 0.017 ms
  • plugin - 1.970 ms

Would they be impacted at all by the size of the URDF/SDFormat file being parsed?

Yes since cb348db asks tinyxml2 to parse the whole document to decide the confidence. See discussion on #13 (comment) . The size dependent overhead would apply times every plugin with a similar score plugin - ros/sdformat_urdf#1 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

3341988 uses a new pluginlib CMake function that enables testing plugins in the same package that built them: ros/pluginlib#201


ament_add_google_benchmark(plugin_overhead
test/benchmark_plugin_overhead.cpp
APPEND_ENV AMENT_PREFIX_PATH="${fake_urdf_prefix}"
)
target_link_libraries(plugin_overhead
urdf
)
endif()

ament_export_libraries(${PROJECT_NAME})
ament_export_targets(${PROJECT_NAME})
ament_export_include_directories(include)
ament_export_dependencies(pluginlib)
ament_export_dependencies(tinyxml_vendor)
ament_export_dependencies(TinyXML)
ament_export_dependencies(urdf_parser_plugin)
ament_export_dependencies(urdfdom)
ament_export_dependencies(urdfdom_headers)

pluginlib_export_plugin_description_file(urdf_parser_plugin "urdf_parser_description.xml")

ament_package()
26 changes: 21 additions & 5 deletions urdf/include/urdf/model.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#ifndef URDF__MODEL_H_
#define URDF__MODEL_H_

#include <memory>
#include <string>

#include "tinyxml.h" // NOLINT
Expand All @@ -48,24 +49,39 @@
namespace urdf
{

// PIMPL Forward Declaration
class ModelImplementation;

/// \brief Populates itself based on a robot descripton
///
/// This class uses `urdf_parser_plugin` to parse the given robot description.
/// The picked plugin is the one that reports the most confident score.
sloretz marked this conversation as resolved.
Show resolved Hide resolved
/// There is no way to override this choice except by uninstalling undesirable
/// parser plugins.
class Model : public ModelInterface
{
public:
URDF_EXPORT
Model();

URDF_EXPORT
~Model();

/// \brief Load Model from TiXMLElement
[[deprecated("use initString instead")]]
URDF_EXPORT bool initXml(TiXmlElement * xml);
/// \brief Load Model from TiXMLDocument
[[deprecated("use initString instead")]]
URDF_EXPORT bool initXml(TiXmlDocument * xml);

/// \brief Load Model given a filename
URDF_EXPORT bool initFile(const std::string & filename);
/// \brief Load Model given the name of a parameter on the parameter server
// URDF_EXPORT bool initParam(const std::string & param);
/// \brief Load Model given the name of parameter on parameter server using provided nodehandle
// URDF_EXPORT bool initParamWithNodeHandle(const std::string & param,
// const ros::NodeHandle & nh = ros::NodeHandle());

/// \brief Load Model from a XML-string
URDF_EXPORT bool initString(const std::string & xmlstring);

private:
std::unique_ptr<ModelImplementation> impl_;
};

// shared_ptr declarations moved to urdf/urdfdom_compatibility.h to allow for
Expand Down
7 changes: 7 additions & 0 deletions urdf/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,29 @@

<buildtool_depend>ament_cmake_ros</buildtool_depend>

<build_depend>pluginlib</build_depend>
<build_depend>tinyxml</build_depend>
<build_depend>tinyxml_vendor</build_depend>
<build_depend>urdfdom</build_depend>
<build_depend>urdf_parser_plugin</build_depend>
<!-- use ROS 2 package urdfdom_headers until upstream provides 1.0.0.-->
<build_depend>urdfdom_headers</build_depend>

<exec_depend>pluginlib</exec_depend>
<exec_depend>tinyxml</exec_depend>
<exec_depend>tinyxml_vendor</exec_depend>
<exec_depend>urdfdom</exec_depend>
<!-- use ROS 2 package urdfdom_headers until upstream provides 1.0.0.-->
<exec_depend>urdfdom_headers</exec_depend>

<build_export_depend>pluginlib</build_export_depend>
<build_export_depend>tinyxml</build_export_depend>
<build_export_depend>urdf_parser_plugin</build_export_depend>
<build_export_depend>urdfdom</build_export_depend>
<!-- use ROS 2 package urdfdom_headers until upstream provides 1.0.0.-->
<build_export_depend>urdfdom_headers</build_export_depend>

<test_depend>ament_cmake_google_benchmark</test_depend>
<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>

Expand Down
124 changes: 71 additions & 53 deletions urdf/src/model.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,45 @@

/* Author: Wim Meeussen */

#include "urdf/model.h"
#include <urdf_parser_plugin/parser.h>
#include <pluginlib/class_loader.hpp>

#include <cassert>
#include <fstream>
#include <iostream>
#include <limits>
#include <string>
#include <utility>
#include <vector>

#include "urdf/model.h"

/* we include the default parser for plain URDF files;
other parsers are loaded via plugins (if available) */
#include "urdf_parser/urdf_parser.h"

namespace urdf
{
class ModelImplementation

Choose a reason for hiding this comment

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

Nit: this can be a final class, since nothing will be deriving from it.

Copy link
Author

Choose a reason for hiding this comment

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

Final in 06474e8

{
public:
ModelImplementation()
: loader_("urdf_parser_plugin", "urdf::URDFParser")
{
}

~ModelImplementation() = default;

// Loader used to get plugins
pluginlib::ClassLoader<urdf::URDFParser> loader_;
};


Model::Model()
: impl_(new ModelImplementation)
{
}

static bool IsColladaData(const std::string & data)
Model::~Model()
{
return data.find("<COLLADA") != std::string::npos;
clear();
impl_.reset();
}

bool Model::initFile(const std::string & filename)
Expand Down Expand Up @@ -124,59 +146,55 @@ bool Model::initXml(TiXmlElement * robot_xml)
return Model::initString(ss.str());
}

bool Model::initString(const std::string & xml_string)
bool Model::initString(const std::string & data)
{
urdf::ModelInterfaceSharedPtr model;

// necessary for COLLADA compatibility
if (IsColladaData(xml_string)) {
fprintf(stderr, "Parsing robot collada xml string is not yet supported.\n");
return false;
/*
ROS_DEBUG("Parsing robot collada xml string");

static boost::mutex PARSER_PLUGIN_LOCK;
static boost::scoped_ptr<pluginlib::ClassLoader<urdf::URDFParser> > PARSER_PLUGIN_LOADER;
boost::mutex::scoped_lock _(PARSER_PLUGIN_LOCK);

try
{
if (!PARSER_PLUGIN_LOADER)
PARSER_PLUGIN_LOADER.reset(new pluginlib::ClassLoader<urdf::URDFParser>("urdf_parser_plugin", "urdf::URDFParser"));
const std::vector<std::string> &classes = PARSER_PLUGIN_LOADER->getDeclaredClasses();
bool found = false;
for (std::size_t i = 0 ; i < classes.size() ; ++i)
if (classes[i].find("urdf/ColladaURDFParser") != std::string::npos)
{
boost::shared_ptr<urdf::URDFParser> instance = PARSER_PLUGIN_LOADER->createInstance(classes[i]);
if (instance)
model = instance->parse(xml_string);
found = true;
break;
}
if (!found)
ROS_ERROR_STREAM("No URDF parser plugin found for Collada files. Did you install the corresponding package?");
}
catch(pluginlib::PluginlibException& ex)
{
ROS_ERROR_STREAM("Exception while creating planning plugin loader " << ex.what() << ". Will not parse Collada file.");
}
size_t best_score = std::numeric_limits<size_t>::max();
clalancette marked this conversation as resolved.
Show resolved Hide resolved
pluginlib::UniquePtr<urdf::URDFParser> best_plugin;

Choose a reason for hiding this comment

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

There is some possibility that no plugin matches, in which case we wouldn't assign anything to best_plugin until after the for loop. I think the "no confidence" logic below that currently prevents us from accessing anything bad, but I'd still feel better if we explicitly assigned this to nullptr here.

Copy link
Author

Choose a reason for hiding this comment

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

Explicit initialize to nullptr in f9fe6f9

std::string best_plugin_name;

// Figure out what plugins might handle this format
for (const std::string & plugin_name : impl_->loader_.getDeclaredClasses()) {
pluginlib::UniquePtr<urdf::URDFParser> plugin_instance;
try {
plugin_instance = impl_->loader_.createUniqueInstance(plugin_name);
} catch (const pluginlib::CreateClassException &) {
fprintf(stderr, "Failed to load urdf_parser_plugin [%s]\n", plugin_name.c_str());
sloretz marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
if (!plugin_instance) {
// Debug mode
assert(plugin_instance);
// Release mode
sloretz marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
size_t score = plugin_instance->might_handle(data);
sloretz marked this conversation as resolved.
Show resolved Hide resolved
if (score < best_score) {
best_score = score;
best_plugin = std::move(plugin_instance);
best_plugin_name = plugin_name;
}
clalancette marked this conversation as resolved.
Show resolved Hide resolved
}
*/
} else {
fprintf(stderr, "Parsing robot urdf xml string.\n");
model = parseURDF(xml_string);

if (!best_plugin) {
fprintf(stderr, "No plugin found for given robot description.\n");
return false;
}

model = best_plugin->parse(data);

// copy data from model into this object
if (model) {
this->links_ = model->links_;
this->joints_ = model->joints_;
this->materials_ = model->materials_;
this->name_ = model->name_;
this->root_link_ = model->root_link_;
return true;
if (!model) {
fprintf(stderr, "Failed to parse robot description using: %s\n", best_plugin_name.c_str());
return false;
}
return false;

this->links_ = model->links_;
this->joints_ = model->joints_;
this->materials_ = model->materials_;
this->name_ = model->name_;
this->root_link_ = model->root_link_;
return true;
}
} // namespace urdf
Loading