urdf: switch from TinyXML to TinyXML2

[rojkov]

The library TinyXML is considered to be unmaintained and
since all future development is focused on TinyXML2 this
patch updates urdf to use TinyXML2.

depends on ros/urdfdom#99

[v4hn]

tinyxml 1 is horribly obsolete and was always a pain to package for distribution maintainers, so I fully agree that we should finally move to tinyxml2 in ROS.

CI failed to find the tinyxml2 package in this request.

Is there a serious problem with using the old tinyxml in kinetic?
This patch changes public API, so I believe it would be better to target ROS lunar.
(Although personally I wouldn't mind if this would be changed in kinetic).

[sloretz]

04:59:14 CMake Error at /usr/share/cmake-3.5/Modules/FindPackageHandleStandardArgs.cmake:148 (message):
04:59:14   Could NOT find TinyXML2 (missing: TinyXML2_LIBRARY TinyXML2_INCLUDE_DIR)
04:59:14 Call Stack (most recent call first):
04:59:14   /usr/share/cmake-3.5/Modules/FindPackageHandleStandardArgs.cmake:388 (_FPHSA_FAILURE_MESSAGE)
04:59:14   /opt/ros/kinetic/share/cmake_modules/cmake/Modules/FindTinyXML2.cmake:65 (find_package_handle_standard_args)
04:59:14   CMakeLists.txt:11 (find_package)

I believe the build failed because tinyxml2 was not added as a dependency in the package.xml

[sloretz]

Thanks for the pull request. The switch to TinyXML2 will have to wait until ROS M because the APIs use TinyXML objects.

I intend to move urdf and urdf_parser_plugin to a new repo. Once I move it and create a ROS M branch, would you be willing to open a pull request there?

[sloretz]

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

[sloretz]

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.

[sloretz]

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

[sloretz]

I would rather we change the APIs to not expose TinyXML2 so its headers don't need to be exported.

[rojkov]

@sloretz Yes, I can resubmit the PR to a new repo. And I agree it's not wise to expose TinyXML in public API especially if the TinyXML values get converted to strings anyway. Are these methods needed at all by the way? Since there's already Model::initString() existing.

If the methods can be dropped there's no need to depend on TinyXML(2).

[sbarthelemy]

And I agree it's not wise to expose TinyXML in public API

such lower level API were probably meant to enable users to parse URDF extensions. But maybe they shall be supported only in urdf_parser.h

especially if the TinyXML values get converted to strings anyway. Are these methods needed at all by the way? Since there's already Model::initString() existing.

By the way, the API

bool initFile(const std::string& filename);

is quite poor since it won't work on on Windows if the filename contains non-ascii chars.
If tinyxml2 supports it, it would be better practice (more orthogonal design) to have

bool initFromStream(std::istream& is);

And let the user pass a std::ifstream, boost::filesystem::ifstream, std::stringstream or wathever stream fits her need.

bool initString(const std::string& xmlstring);

would be redundant then (but could be maintained),

[sloretz]

@rojkov The urdf packages have been moved to their own repo as part of #195. Would you mind moving this to https://github.com/ros/urdf?

[rojkov]

Resubmitted as ros/urdf#4