Skip to content

Commit

Permalink
Upgrade to tinyxml2 for rviz (#418)
Browse files Browse the repository at this point in the history
* Remove TinyXML dependency in default plugins

It was only used to parse a string for urdf parsing.
The API however already supports passing in the string.

* Add test for display factory

- This will be upgraded to tinyxml2
- Make sure that it works correctly

* Remove tinyxml in favour of tinyxml2 in rviz_common

* Refactoring of display_factory for better readability
  • Loading branch information
Martin-Idel committed Sep 3, 2020
1 parent d590721 commit 40e3b9a
Show file tree
Hide file tree
Showing 8 changed files with 242 additions and 68 deletions.
18 changes: 14 additions & 4 deletions rviz_common/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ find_package(rviz_assimp_vendor REQUIRED)
find_package(rviz_rendering REQUIRED)
find_package(sensor_msgs REQUIRED)
find_package(std_msgs REQUIRED)
find_package(tinyxml_vendor REQUIRED)
find_package(tinyxml2_vendor REQUIRED)
find_package(tf2 REQUIRED)
find_package(tf2_geometry_msgs REQUIRED)
find_package(tf2_ros REQUIRED)
find_package(message_filters REQUIRED)
find_package(urdf REQUIRED)
find_package(yaml_cpp_vendor REQUIRED)

find_package(TinyXML REQUIRED) # provided by tinyxml_vendor
find_package(TinyXML2 REQUIRED) # provided by tinyxml_vendor

# Copy env_config.hpp so that env_config.cpp can find it
# TODO(jsquare): Get rid of copy hpp file
Expand Down Expand Up @@ -227,7 +227,6 @@ target_include_directories(rviz_common
PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>
${TinyXML_INCLUDE_DIRS}
)

target_link_libraries(rviz_common
Expand All @@ -236,7 +235,6 @@ target_link_libraries(rviz_common
rviz_ogre_vendor::OgreOverlay
rviz_rendering::rviz_rendering
Qt5::Widgets
${TinyXML_LIBRARIES}
)

ament_target_dependencies(rviz_common
Expand All @@ -252,6 +250,7 @@ ament_target_dependencies(rviz_common
tf2_geometry_msgs
tf2_ros
message_filters
tinyxml2_vendor
urdf
yaml_cpp_vendor
)
Expand Down Expand Up @@ -330,6 +329,17 @@ if(BUILD_TESTING)
qt5_wrap_cpp(rviz_common_test_moc_files test/mock_property_change_receiver.hpp)

qt5_wrap_cpp(rviz_common_test_frame_manager_moc src/rviz_common/frame_manager.hpp)

ament_add_gmock(display_factory_test
test/display_factory_test.cpp
src/rviz_common/display_factory.cpp
${SKIP_DISPLAY_TESTS})
if(TARGET display_factory_test)
target_compile_definitions(display_factory_test PUBLIC
-D_TEST_PLUGIN_DESCRIPTIONS="${CMAKE_CURRENT_SOURCE_DIR}")
target_link_libraries(display_factory_test rviz_common Qt5::Widgets)
endif()

ament_add_gmock(frame_manager_test
test/frame_manager_test.cpp
src/rviz_common/frame_manager.cpp
Expand Down
2 changes: 1 addition & 1 deletion rviz_common/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
<depend>tf2_geometry_msgs</depend>
<depend>tf2_ros</depend>
<depend>message_filters</depend>
<depend>tinyxml_vendor</depend>
<depend>tinyxml2_vendor</depend>
<depend>urdf</depend>
<depend>yaml_cpp_vendor</depend>

Expand Down
149 changes: 90 additions & 59 deletions rviz_common/src/rviz_common/display_factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@

#include <string>

// TODO(wjwwood): replace with tinyxml2? implicit dependency?
#include <tinyxml.h> // NOLINT: cpplint is unable to handle the include order here
#include <tinyxml2.h> // NOLINT: cpplint is unable to handle the include order here

#include "rviz_common/display_group.hpp"
#include "rviz_common/logging.hpp"
Expand Down Expand Up @@ -77,71 +76,22 @@ QSet<QString> DisplayFactory::getMessageTypes(const QString & class_id)

if (!xml_file.isEmpty()) {
RVIZ_COMMON_LOG_DEBUG_STREAM("Parsing " << xml_file.toStdString());
TiXmlDocument document;
document.LoadFile(xml_file.toStdString());
TiXmlElement * config = document.RootElement();
if (config == nullptr) {
RVIZ_COMMON_LOG_ERROR_STREAM(
"Skipping XML Document \"" << xml_file.toStdString() << "\" which had no Root Element. "
"This likely means the XML is malformed or missing.");
return QSet<QString>();
}
if (config->ValueStr() != "library" &&
config->ValueStr() != "class_libraries")
tinyxml2::XMLDocument document;
document.LoadFile(xml_file.toUtf8().constData());
tinyxml2::XMLElement * config = document.RootElement();
if (!hasRootNode(config, xml_file.toStdString()) ||
!hasLibraryRoot(config, xml_file.toStdString()))
{
RVIZ_COMMON_LOG_ERROR_STREAM(
"The XML document \"" << xml_file.toStdString() <<
"\" given to add must have either \"library\" or "
"\"class_libraries\" as the root tag");
return QSet<QString>();
}
// Step into the filter list if necessary
if (config->ValueStr() == "class_libraries") {
if (config->Value() == std::string("class_libraries")) {
config = config->FirstChildElement("library");
}

TiXmlElement * library = config;
tinyxml2::XMLElement * library = config;
while (library) {
TiXmlElement * class_element = library->FirstChildElement("class");
while (class_element) {
std::string derived_class;
if (class_element->Attribute("type")) {
derived_class = class_element->Attribute("type");
}
std::string current_class_id;
if (class_element->Attribute("name")) {
current_class_id = class_element->Attribute("name");
RVIZ_COMMON_LOG_DEBUG_STREAM(
"XML file specifies lookup name (i.e. magic name) = " << current_class_id);
} else {
RVIZ_COMMON_LOG_DEBUG_STREAM(
"XML file has no lookup name (i.e. magic name) for class " << derived_class <<
", assuming class_id == real class name.");
current_class_id = derived_class;
}

QSet<QString> message_types;
TiXmlElement * message_type = class_element->FirstChildElement("message_type");

while (message_type) {
if (message_type->GetText()) {
const char * message_type_str = message_type->GetText();
RVIZ_COMMON_LOG_DEBUG_STREAM(
current_class_id << " supports message type " << message_type_str);
#if QT_VERSION < QT_VERSION_CHECK(5, 0, 0)
message_types.insert(QString::fromAscii(message_type_str));
#else
message_types.insert(QString(message_type_str));
#endif
}
message_type = message_type->NextSiblingElement("message_type");
}

message_type_cache_[QString::fromStdString(current_class_id)] = message_types;

// step to next class_element
class_element = class_element->NextSiblingElement("class");
}
fillCacheForAllClassElements(library);
library = library->NextSiblingElement("library");
}
}
Expand All @@ -154,4 +104,85 @@ QSet<QString> DisplayFactory::getMessageTypes(const QString & class_id)
return QSet<QString>();
}

bool DisplayFactory::hasRootNode(tinyxml2::XMLElement * root_element, const std::string & xml_file)
{
if (root_element == nullptr) {
RVIZ_COMMON_LOG_ERROR_STREAM(
"Skipping XML Document \"" << xml_file << "\" which had no Root Element. "
"This likely means the XML is malformed or missing.");
return false;
}
return true;
}

bool
DisplayFactory::hasLibraryRoot(tinyxml2::XMLElement * root_element, const std::string & xml_file)
{
if (root_element->Value() != std::string("library") &&
root_element->Value() != std::string("class_libraries"))
{
RVIZ_COMMON_LOG_ERROR_STREAM(
"The XML document \"" << xml_file <<
"\" given to add must have either \"library\" or "
"\"class_libraries\" as the root tag");
return false;
}
return true;
}

void DisplayFactory::fillCacheForAllClassElements(tinyxml2::XMLElement * library)
{
tinyxml2::XMLElement * class_element = library->FirstChildElement("class");
while (class_element) {
const std::string derived_class = lookupDerivedClass(class_element);
const std::string current_class_id = lookupClassId(class_element, derived_class);
QSet<QString> message_types = parseMessageTypes(class_element, current_class_id);

message_type_cache_[QString::fromStdString(current_class_id)] = message_types;

class_element = class_element->NextSiblingElement("class");
}
}

QSet<QString> DisplayFactory::parseMessageTypes(
tinyxml2::XMLElement * class_element, const std::string & current_class_id) const
{
QSet<QString> message_types;

const tinyxml2::XMLElement * message_type = class_element->FirstChildElement("message_type");
while (message_type) {
if (message_type->GetText()) {
const char * message_type_str = message_type->GetText();
RVIZ_COMMON_LOG_DEBUG_STREAM(
current_class_id << " supports message type " << message_type_str);
message_types.insert(QString(message_type_str));
}
message_type = message_type->NextSiblingElement("message_type");
}
return message_types;
}

std::string DisplayFactory::lookupDerivedClass(const tinyxml2::XMLElement * class_element) const
{
if (class_element->Attribute("type")) {
return class_element->Attribute("type");
}
return "";
}

std::string DisplayFactory::lookupClassId(
const tinyxml2::XMLElement * class_element, const std::string & derived_class) const
{
if (class_element->Attribute("name")) {
RVIZ_COMMON_LOG_DEBUG_STREAM(
"XML file specifies lookup name (i.e. magic name) = " << class_element->Attribute("name"));
return class_element->Attribute("name");
} else {
RVIZ_COMMON_LOG_DEBUG_STREAM(
"XML file has no lookup name (i.e. magic name) for class " << derived_class <<
", assuming class_id == real class name.");
return derived_class;
}
}

} // namespace rviz_common
18 changes: 15 additions & 3 deletions rviz_common/src/rviz_common/display_factory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@
#ifndef RVIZ_COMMON__DISPLAY_FACTORY_HPP_
#define RVIZ_COMMON__DISPLAY_FACTORY_HPP_

#include <QMap>
#include <QSet>
#include <QString>
#include <string>

#include <QMap> // NOLINT: cpplint cannot handle include order here
#include <QSet> // NOLINT: cpplint cannot handle include order here
#include <QString> // NOLINT: cpplint cannot handle include order here

#include "rviz_common/factory/pluginlib_factory.hpp"
#include "rviz_common/display.hpp"
Expand All @@ -54,6 +56,16 @@ class DisplayFactory : public PluginlibFactory<Display>
Display * makeRaw(const QString & class_id, QString * error_return = nullptr) override;

QMap<QString, QSet<QString>> message_type_cache_;

private:
bool hasRootNode(tinyxml2::XMLElement * root_element, const std::string & xml_file);
bool hasLibraryRoot(tinyxml2::XMLElement * root_element, const std::string & xml_file);
void fillCacheForAllClassElements(tinyxml2::XMLElement * library);
QSet<QString> parseMessageTypes(
tinyxml2::XMLElement * class_element, const std::string & current_class_id) const;
std::string lookupClassId(
const tinyxml2::XMLElement * class_element, const std::string & derived_class) const;
std::string lookupDerivedClass(const tinyxml2::XMLElement * class_element) const;
};

} // namespace rviz_common
Expand Down
86 changes: 86 additions & 0 deletions rviz_common/test/display_factory_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright (c) 2019, Martin Idel
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
* * Neither the name of the Willow Garage, Inc. nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/

#include <gmock/gmock.h>

#include <memory>
#include <utility>

#include <QString> // NOLINT: cpplint cannot handle include order here

#include "../src/rviz_common/display_factory.hpp"

using namespace ::testing; // NOLINT use namespace testing to not clutter tests

class TestDisplayFactory : public rviz_common::DisplayFactory
{
public:
explicit TestDisplayFactory(QString path_suffix = "/test/resources/plugins_description.xml")
: rviz_common::DisplayFactory(), path_suffix_(std::move(path_suffix))
{}

// overwrite getPluginManifestPath to get our own test resource. This method is called in the
// real method we want to test which uses tinyxml to parse the plugin manifest.
QString getPluginManifestPath(const QString & class_id) const override
{
(void) class_id;
const auto path_prefix = QString(_TEST_PLUGIN_DESCRIPTIONS);
return path_prefix + path_suffix_;
}

private:
QString path_suffix_;
};

TEST(TestDisplayFactory, getMessageTypes_finds_sensor_msgs_for_fake_camera_display) {
auto display_factory = std::make_unique<TestDisplayFactory>();
const QSet<QString> message_types = display_factory->getMessageTypes("rviz_common_test/Camera");
ASSERT_THAT(message_types, Contains("sensor_msgs/msg/Image"));
ASSERT_THAT(message_types, Contains("sensor_msgs/msg/CompressedImage"));
}

TEST(TestDisplayFactory, getMessageTypes_finds_no_messages_for_fake_grid_display) {
auto display_factory = std::make_unique<TestDisplayFactory>();
const QSet<QString> message_types = display_factory->getMessageTypes("rviz_common_test/Grid");
ASSERT_THAT(message_types, IsEmpty());
}

TEST(TestDisplayFactory, getMessageTypes_finds_no_messages_for_missing_display) {
auto display_factory = std::make_unique<TestDisplayFactory>();
const QSet<QString> message_types = display_factory->getMessageTypes(
"rviz_common_test/MissingDisplay");
ASSERT_THAT(message_types, IsEmpty());
}

TEST(TestDisplayFactory, getMessageTypes_finds_no_messages_for_xml_with_wrong_root) {
auto display_factory =
std::make_unique<TestDisplayFactory>("/test/resources/broken_plugins.xml");
const QSet<QString> message_types = display_factory->getMessageTypes("rviz_common_test/Camera");
ASSERT_THAT(message_types, IsEmpty());
}
13 changes: 13 additions & 0 deletions rviz_common/test/resources/broken_plugins.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<no-library path="rviz_common_test">
<class
name="rviz_common_test/Camera"
type="rviz_common_test::displays::CameraDisplay"
base_class_type="rviz_common::Display"
>
<description>
Displays an image from a camera, with the visualized world rendered behind it. &lt;a href="http://www.ros.org/wiki/rviz/DisplayTypes/Camera"&gt;More Information&lt;/a&gt;.
</description>
<message_type>sensor_msgs/msg/Image</message_type>
<message_type>sensor_msgs/msg/CompressedImage</message_type>
</class>
</no-library>
Loading

0 comments on commit 40e3b9a

Please sign in to comment.