From 50458d5e547d932f279eb5cd8a3327d6689cef53 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 10 Sep 2020 03:09:49 -0500 Subject: [PATCH] Fix whitespace preservation behavior with TinyXML2 (#359) This addresses a change in how whitespace is preserved/collapsed between tinyxml and tinyxml2. The change primarily impacts SDF files that have newlines, which is frequently done for aesthetics with include uris. In this case, we use an alternate constructor for TinyXML2 that will collapse whitespace by default. Note that there is a performance impact of this behavior, which causes the parsing to run essentially twice. More information on the behavior may be found here: https://leethomason.github.io/tinyxml2/ Closes #322 Signed-off-by: Michael Carroll Co-authored-by: Steve Peters --- src/parser.cc | 42 ++++++++++++------ src/parser_urdf_TEST.cc | 76 +++++++++++++++++++++++++++++++++ test/integration/CMakeLists.txt | 1 + test/integration/whitespace.cc | 64 +++++++++++++++++++++++++++ test/sdf/whitespace.sdf | 40 +++++++++++++++++ 5 files changed, 210 insertions(+), 13 deletions(-) create mode 100644 test/integration/whitespace.cc create mode 100644 test/sdf/whitespace.sdf diff --git a/src/parser.cc b/src/parser.cc index 35a9bb468..3c812b183 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -81,11 +81,27 @@ bool readStringInternal( const bool _convert, Errors &_errors); +////////////////////////////////////////////////// +/// \brief Internal helper for creating XMLDocuments +/// +/// This creates an XMLDocument with whitespace collapse +/// on, which is not default behavior in tinyxml2. +/// This function is to consolidate locations it is used. +/// +/// There is a performance impact associated with collapsing whitespace. +/// +/// For more information on the behavior and performance implications, +/// consult the TinyXML2 documentation: https://leethomason.github.io/tinyxml2/ +inline auto makeSdfDoc() +{ + return tinyxml2::XMLDocument(true, tinyxml2::COLLAPSE_WHITESPACE); +} + ////////////////////////////////////////////////// template static inline bool _initFile(const std::string &_filename, TPtr _sdf) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); if (tinyxml2::XML_SUCCESS != xmlDoc.LoadFile(_filename.c_str())) { sdferr << "Unable to load file[" @@ -100,7 +116,7 @@ static inline bool _initFile(const std::string &_filename, TPtr _sdf) bool init(SDFPtr _sdf) { std::string xmldata = SDF::EmbeddedSpec("root.sdf", false); - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); xmlDoc.Parse(xmldata.c_str()); return initDoc(&xmlDoc, _sdf); } @@ -111,7 +127,7 @@ bool initFile(const std::string &_filename, SDFPtr _sdf) std::string xmldata = SDF::EmbeddedSpec(_filename, true); if (!xmldata.empty()) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); xmlDoc.Parse(xmldata.c_str()); return initDoc(&xmlDoc, _sdf); } @@ -124,7 +140,7 @@ bool initFile(const std::string &_filename, ElementPtr _sdf) std::string xmldata = SDF::EmbeddedSpec(_filename, true); if (!xmldata.empty()) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); xmlDoc.Parse(xmldata.c_str()); return initDoc(&xmlDoc, _sdf); } @@ -134,7 +150,7 @@ bool initFile(const std::string &_filename, ElementPtr _sdf) ////////////////////////////////////////////////// bool initString(const std::string &_xmlString, SDFPtr _sdf) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); if (xmlDoc.Parse(_xmlString.c_str())) { sdferr << "Failed to parse string as XML: " << xmlDoc.ErrorStr() << '\n'; @@ -393,7 +409,7 @@ bool readFileWithoutConversion( bool readFileInternal(const std::string &_filename, SDFPtr _sdf, const bool _convert, Errors &_errors) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); std::string filename = sdf::findFile(_filename, true, true); if (filename.empty()) @@ -429,7 +445,7 @@ bool readFileInternal(const std::string &_filename, SDFPtr _sdf, else if (URDF2SDF::IsURDF(filename)) { URDF2SDF u2g; - tinyxml2::XMLDocument doc; + auto doc = makeSdfDoc(); u2g.InitModelFile(filename, &doc); if (sdf::readDoc(&doc, _sdf, "urdf file", _convert, _errors)) { @@ -476,7 +492,7 @@ bool readStringWithoutConversion( bool readStringInternal(const std::string &_xmlString, SDFPtr _sdf, const bool _convert, Errors &_errors) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); xmlDoc.Parse(_xmlString.c_str()); if (xmlDoc.Error()) { @@ -490,7 +506,7 @@ bool readStringInternal(const std::string &_xmlString, SDFPtr _sdf, else { URDF2SDF u2g; - tinyxml2::XMLDocument doc; + auto doc = makeSdfDoc(); u2g.InitModelString(_xmlString, &doc); if (sdf::readDoc(&doc, _sdf, "urdf string", _convert, _errors)) @@ -524,7 +540,7 @@ bool readString(const std::string &_xmlString, ElementPtr _sdf) ////////////////////////////////////////////////// bool readString(const std::string &_xmlString, ElementPtr _sdf, Errors &_errors) { - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); xmlDoc.Parse(_xmlString.c_str()); if (xmlDoc.Error()) { @@ -773,7 +789,7 @@ std::string getModelFilePath(const std::string &_modelDirPath) } } - tinyxml2::XMLDocument configFileDoc; + auto configFileDoc = makeSdfDoc(); if (tinyxml2::XML_SUCCESS != configFileDoc.LoadFile(configFilePath.c_str())) { sdferr << "Error parsing XML in file [" @@ -1264,7 +1280,7 @@ bool convertFile(const std::string &_filename, const std::string &_version, return false; } - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); if (!xmlDoc.LoadFile(filename.c_str())) { // read initial sdf version @@ -1309,7 +1325,7 @@ bool convertString(const std::string &_sdfString, const std::string &_version, return false; } - tinyxml2::XMLDocument xmlDoc; + auto xmlDoc = makeSdfDoc(); xmlDoc.Parse(_sdfString.c_str()); if (!xmlDoc.Error()) diff --git a/src/parser_urdf_TEST.cc b/src/parser_urdf_TEST.cc index c9d5a6ada..cf65e1845 100644 --- a/src/parser_urdf_TEST.cc +++ b/src/parser_urdf_TEST.cc @@ -852,6 +852,82 @@ TEST(URDFParser, OutputPrecision) EXPECT_EQ("0", poseValues[5]); } +///////////////////////////////////////////////// +TEST(URDFParser, ParseWhitespace) +{ + std::string str = R"( + + + + + + + + + + + + + + + + + + + + Gazebo/Orange + + + 100 + + + + 1000 + + + +)"; + tinyxml2::XMLDocument doc; + doc.Parse(str.c_str()); + + sdf::URDF2SDF parser; + tinyxml2::XMLDocument sdfXml; + parser.InitModelDoc(&doc, &sdfXml); + + auto root = sdfXml.RootElement(); + ASSERT_NE(nullptr, root); + auto modelElem = root->FirstChildElement("model"); + ASSERT_NE(nullptr, modelElem); + auto linkElem = modelElem->FirstChildElement("link"); + ASSERT_NE(nullptr, linkElem); + auto visualElem = linkElem->FirstChildElement("visual"); + ASSERT_NE(nullptr, visualElem); + auto collisionElem = linkElem->FirstChildElement("collision"); + ASSERT_NE(nullptr, collisionElem); + + auto materialElem = visualElem->FirstChildElement("material"); + ASSERT_NE(nullptr, materialElem); + auto scriptElem = materialElem->FirstChildElement("script"); + ASSERT_NE(nullptr, scriptElem); + auto nameElem = scriptElem->FirstChildElement("name"); + ASSERT_NE(nullptr, nameElem); + EXPECT_EQ("Gazebo/Orange", std::string(nameElem->GetText())); + + auto surfaceElem = collisionElem->FirstChildElement("surface"); + ASSERT_NE(nullptr, surfaceElem); + auto frictionElem = surfaceElem->FirstChildElement("friction"); + ASSERT_NE(nullptr, frictionElem); + auto odeElem = frictionElem->FirstChildElement("ode"); + ASSERT_NE(nullptr, odeElem); + auto muElem = odeElem->FirstChildElement("mu"); + ASSERT_NE(nullptr, muElem); + auto mu2Elem = odeElem->FirstChildElement("mu2"); + ASSERT_NE(nullptr, mu2Elem); + + EXPECT_EQ("100", std::string(muElem->GetText())); + EXPECT_EQ("1000", std::string(mu2Elem->GetText())); +} + ///////////////////////////////////////////////// /// Main int main(int argc, char **argv) diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index d039ba030..42fd7d6e8 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -40,6 +40,7 @@ set(tests urdf_gazebo_extensions.cc urdf_joint_parameters.cc visual_dom.cc + whitespace.cc world_dom.cc ) diff --git a/test/integration/whitespace.cc b/test/integration/whitespace.cc new file mode 100644 index 000000000..8389e7296 --- /dev/null +++ b/test/integration/whitespace.cc @@ -0,0 +1,64 @@ +/* + * Copyright 2019 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include +#include +#include + +#include "sdf/Actor.hh" +#include "sdf/Collision.hh" +#include "sdf/Filesystem.hh" +#include "sdf/Geometry.hh" +#include "sdf/Light.hh" +#include "sdf/Link.hh" +#include "sdf/Mesh.hh" +#include "sdf/Model.hh" +#include "sdf/parser.hh" +#include "sdf/Root.hh" +#include "sdf/SDFImpl.hh" +#include "sdf/Visual.hh" +#include "sdf/World.hh" +#include "test_config.h" + +const auto g_testPath = sdf::filesystem::append(PROJECT_SOURCE_PATH, "test"); +const auto g_modelsPath = + sdf::filesystem::append(g_testPath, "integration", "model"); + +///////////////////////////////////////////////// +std::string findFileCb(const std::string &_input) +{ + return sdf::filesystem::append(g_testPath, "integration", "model", _input); +} + +////////////////////////////////////////////////// +TEST(WhitespaceTest, Whitespace) +{ + sdf::setFindCallback(findFileCb); + + const auto worldFile = + sdf::filesystem::append(g_testPath, "sdf", "whitespace.sdf"); + + sdf::Root root; + sdf::Errors errors = root.Load(worldFile); + for (auto e : errors) + std::cout << e.Message() << std::endl; + EXPECT_TRUE(errors.empty()); + + ASSERT_NE(nullptr, root.Element()); + EXPECT_EQ(worldFile, root.Element()->FilePath()); + EXPECT_EQ("1.6", root.Element()->OriginalVersion()); +} diff --git a/test/sdf/whitespace.sdf b/test/sdf/whitespace.sdf new file mode 100644 index 000000000..fc7cf95c9 --- /dev/null +++ b/test/sdf/whitespace.sdf @@ -0,0 +1,40 @@ + + + + + + test_model + + + + + test_model + + override_model_name + + + + test_light + + + + + test_light + + override_light_name + 4 5 6 0 0 0 + + + + test_actor + + + + + test_actor + + override_actor_name + + + +