Fix whitespace preservation behavior with TinyXML2

src/parser.cc

@@ -78,11 +78,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 <typename TPtr>
 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["
@@ -97,7 +113,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);
 }
@@ -108,7 +124,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);
   }
@@ -121,7 +137,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);
   }
@@ -131,7 +147,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';
@@ -390,7 +406,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())
@@ -426,7 +442,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))
     {
@@ -473,7 +489,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())
   {
@@ -487,7 +503,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))
@@ -521,7 +537,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())
   {
@@ -770,7 +786,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 ["
@@ -1375,7 +1391,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
@@ -1420,7 +1436,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())

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
 )
 

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 <iostream>
+#include <string>
+#include <gtest/gtest.h>
+
+#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());
+}

test/sdf/whitespace.sdf

@@ -0,0 +1,40 @@
+<?xml version="1.0" ?>
+<sdf version="1.6">
+  <world name="default">
+
+    <include>
+      <uri>test_model</uri>
+    </include>
+
+    <include>
+      <uri>
+          test_model
+      </uri>
+      <name>override_model_name</name>
+    </include>
+
+    <include>
+      <uri>test_light</uri>
+    </include>
+
+    <include>
+      <uri>
+          test_light
+      </uri>
+      <name>override_light_name</name>
+      <pose>4 5 6 0 0 0</pose>
+    </include>
+
+    <include>
+      <uri>test_actor</uri>
+    </include>
+
+    <include>
+      <uri>
+          test_actor
+      </uri>
+      <name>override_actor_name</name>
+    </include>
+
+  </world>
+</sdf>