Skip to content
This repository has been archived by the owner on Aug 3, 2020. It is now read-only.

urdf: switch from TinyXML to TinyXML2 #205

Closed
wants to merge 1 commit into from
Closed

urdf: switch from TinyXML to TinyXML2 #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions urdf/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ find_package(catkin REQUIRED COMPONENTS
urdf_parser_plugin pluginlib rosconsole_bridge roscpp cmake_modules)


find_package(TinyXML REQUIRED)
find_package(TinyXML2 REQUIRED)

# Find version components
if(NOT urdfdom_headers_VERSION)
Expand All @@ -25,7 +25,7 @@ add_compile_options(-std=c++11)

catkin_package(
LIBRARIES ${PROJECT_NAME}
INCLUDE_DIRS include ${TinyXML_INLCLUDE_DIRS} ${CATKIN_DEVEL_PREFIX}/include
INCLUDE_DIRS include ${TinyXML2_INLCLUDE_DIRS} ${CATKIN_DEVEL_PREFIX}/include
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 we change the APIs to not expose TinyXML2 so its headers don't need to be exported.

CATKIN_DEPENDS rosconsole_bridge roscpp
DEPENDS urdfdom_headers urdfdom Boost
Copy link
Contributor

Choose a reason for hiding this comment

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

I think TinyXML2 would have to be added to the DEPENDS list.

)
Expand All @@ -37,13 +37,13 @@ include_directories(
${catkin_INCLUDE_DIRS}
${urdfdom_INCLUDE_DIRS}
${urdfdom_headers_INCLUDE_DIRS}
${TinyXML_INCLUDE_DIRS}
${TinyXML2_INCLUDE_DIRS}
)

link_directories(${Boost_LIBRARY_DIRS} ${catkin_LIBRARY_DIRS})

add_library(${PROJECT_NAME} src/model.cpp src/rosconsole_bridge.cpp)
target_link_libraries(${PROJECT_NAME} ${TinyXML_LIBRARIES} ${catkin_LIBRARIES} ${urdfdom_LIBRARIES})
target_link_libraries(${PROJECT_NAME} ${TinyXML2_LIBRARIES} ${catkin_LIBRARIES} ${urdfdom_LIBRARIES})

if(APPLE)
set_target_properties(${PROJECT_NAME} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
Expand Down
10 changes: 5 additions & 5 deletions urdf/include/urdf/model.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
#include <map>
#include <urdf_model/model.h>
#include <urdf/urdfdom_compatibility.h>
#include <tinyxml.h>
#include <tinyxml2.h>
#include <boost/shared_ptr.hpp>
#include <boost/weak_ptr.hpp>
#include <ros/ros.h>
Expand All @@ -51,10 +51,10 @@ namespace urdf{
class Model: public ModelInterface
{
public:
/// \brief Load Model from TiXMLElement
bool initXml(TiXmlElement *xml);
/// \brief Load Model from TiXMLDocument
bool initXml(TiXmlDocument *xml);
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing these constructors in kinetic would break API/ABI compatibility. This change would have to wait until the next Ros release (ROS M).

/// \brief Load Model from XMLElement
bool initXml(tinyxml2::XMLElement *xml);
/// \brief Load Model from XMLDocument
bool initXml(tinyxml2::XMLDocument *xml);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can retarget this pull request to ROS M, I'd be in favor of tinyxml2 not being visible in the APIs at all.

/// \brief Load Model given a filename
bool initFile(const std::string& filename);
/// \brief Load Model given the name of a parameter on the parameter server
Expand Down
17 changes: 11 additions & 6 deletions urdf/src/model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
#include <fstream>
#include <iostream>

using namespace tinyxml2;

namespace urdf{

static bool IsColladaData(const std::string& data)
Expand Down Expand Up @@ -108,21 +110,22 @@ bool Model::initParamWithNodeHandle(const std::string& param, const ros::NodeHan
return Model::initString(xml_string);
}

bool Model::initXml(TiXmlDocument *xml_doc)
bool Model::initXml(XMLDocument *xml_doc)
{
if (!xml_doc)
{
ROS_ERROR("Could not parse the xml document");
return false;
}

std::stringstream ss;
ss << *xml_doc;
XMLPrinter printer;
xml_doc->Print(&printer);
std::string str(printer.CStr());

return Model::initString(ss.str());
return Model::initString(str);
}

bool Model::initXml(TiXmlElement *robot_xml)
bool Model::initXml(XMLElement *robot_xml)
{
if (!robot_xml)
{
Expand All @@ -131,7 +134,9 @@ bool Model::initXml(TiXmlElement *robot_xml)
}

std::stringstream ss;
ss << (*robot_xml);
XMLPrinter printer;
robot_xml->Accept(&printer);
ss << printer.CStr();

return Model::initString(ss.str());
}
Expand Down