Merge branch 'sdf13' into mjcarroll/garden_bazel

.github/workflows/triage.yml

@@ -10,10 +10,8 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: Add ticket to inbox
-        uses: technote-space/create-project-card-action@v1
+        uses: actions/[email protected]
         with:
-          PROJECT: Core development
-          COLUMN: Inbox
-          GITHUB_TOKEN: ${{ secrets.TRIAGE_TOKEN }}
-          CHECK_ORG_PROJECT: true
+          project-url: https://github.com/orgs/gazebosim/projects/7
+          github-token: ${{ secrets.TRIAGE_TOKEN }}
 

Migration.md

@@ -24,6 +24,7 @@ but with improved human-readability..
 
 1. ParserConfig defaults to WARN instead of LOG when parsing unrecognized
    elements.
+2. Updated search order for `sdf::findFile()` making local path (current directory) the first to be searched. 
 
 ### Deprecations
 

include/sdf/SDFImpl.hh

@@ -59,13 +59,13 @@ namespace sdf
   /// The search order in the function is as follows:
   /// 1. Using the global URI path map, search in paths associated with the URI
   ///    scheme of the input.
-  /// 2. Seach in the path defined by the macro `SDF_SHARE_PATH`.
-  /// 3. Search in the the libsdformat install path. The path is formed by
-  ///    has the pattern `SDF_SHARE_PATH/sdformat<major version>/<version>/`
-  /// 4. Directly check if the input path exists in the filesystem.
-  /// 5. Seach in the path defined by the environment variable `SDF_PATH`.
-  /// 6. If enabled via _searchLocalPath, prepend the input with the current
+  /// 2. If enabled via _searchLocalPath, prepend the input with the current
   ///    working directory and check if the result path exists.
+  /// 3. Seach in the path defined by the macro `SDF_SHARE_PATH`.
+  /// 4. Search in the the libsdformat install path. The path is formed by
+  ///    has the pattern `SDF_SHARE_PATH/sdformat<major version>/<version>/`
+  /// 5. Directly check if the input path exists in the filesystem.
+  /// 6. Seach in the path defined by the environment variable `SDF_PATH`.
   /// 7. If enabled via _useCallback and the global callback function is set,
   ///    invoke the function and return its result.
   ///

src/SDF.cc

@@ -106,6 +106,17 @@ std::string findFile(const std::string &_filename, bool _searchLocalPath,
     filename = filename.substr(idx + sep.length());
   }
 
+  // First check the local path, if the flag is set.
+  if (_searchLocalPath)
+  {
+    std::string path = sdf::filesystem::append(sdf::filesystem::current_path(),
+                                               filename);
+    if (sdf::filesystem::exists(path))
+    {
+      return path;
+    }
+  }
+
   // Next check the install path.
   std::string path = sdf::filesystem::append(sdfSharePath(), filename);
   if (sdf::filesystem::exists(path))
@@ -122,7 +133,7 @@ std::string findFile(const std::string &_filename, bool _searchLocalPath,
     return path;
   }
 
-  // Next check to see if the given file exists.
+  // Finally check to see if the given file exists.
   path = filename;
   if (sdf::filesystem::exists(path))
   {
@@ -144,16 +155,6 @@ std::string findFile(const std::string &_filename, bool _searchLocalPath,
     }
   }
 
-  // Finally check the local path, if the flag is set.
-  if (_searchLocalPath)
-  {
-    path = sdf::filesystem::append(sdf::filesystem::current_path(), filename);
-    if (sdf::filesystem::exists(path))
-    {
-      return path;
-    }
-  }
-
   // If we still haven't found the file, use the registered callback if the
   // flag has been set
   if (_useCallback)

src/SDF_TEST.cc

@@ -788,4 +788,32 @@ TEST(SDF, WriteURIPath)
   ASSERT_EQ(std::remove(tempFile.c_str()), 0);
   ASSERT_EQ(rmdir(tempDir.c_str()), 0);
 }
+
+/////////////////////////////////////////////////
+TEST(SDF, FindFileModelSDFCurrDir)
+{
+  std::string currDir;
+
+  // Get current directory path from $PWD env variable
+  currDir = sdf::filesystem::current_path();
+
+  // A file named model.sdf in current directory
+  auto tempFile = currDir + "/model.sdf";
+
+  sdf::SDF tempSDF;
+  tempSDF.Write(tempFile);
+
+  // Check file was created
+  auto fp = fopen(tempFile.c_str(), "r");
+  ASSERT_NE(nullptr, fp);
+
+  // Get path of the file returned from findFile()
+  std::string foundFile = sdf::findFile("model.sdf", true, false);
+
+  // Check that the returned file is model.sdf from curr directory
+  ASSERT_EQ(tempFile, foundFile);
+
+  // Cleanup
+  ASSERT_EQ(std::remove(tempFile.c_str()), 0);
+}
 #endif  // _WIN32

src/parser.cc

@@ -768,25 +768,36 @@ bool readFileInternal(const std::string &_filename, const bool _convert,
     return false;
   }
 
-  // Suppress deprecation for sdf::URDF2SDF
-  if (readDoc(&xmlDoc, _sdf, filename, _convert, _config, _errors))
+  tinyxml2::XMLElement *sdfXml = xmlDoc.FirstChildElement("sdf");
+  if (sdfXml)
   {
-    return true;
+    // Suppress deprecation for sdf::URDF2SDF
+    return readDoc(&xmlDoc, _sdf, filename, _convert, _config, _errors);
   }
-  else if (URDF2SDF::IsURDF(filename))
+  else
   {
-    URDF2SDF u2g;
-    auto doc = makeSdfDoc();
-    u2g.InitModelFile(filename, _config, &doc);
-    if (sdf::readDoc(&doc, _sdf, filename, _convert, _config, _errors))
+    tinyxml2::XMLElement *robotXml = xmlDoc.FirstChildElement("robot");
+    if (robotXml)
     {
-      sdfdbg << "parse from urdf file [" << _filename << "].\n";
-      return true;
+      URDF2SDF u2g;
+      auto doc = makeSdfDoc();
+      u2g.InitModelFile(filename, _config, &doc);
+      if (sdf::readDoc(&doc, _sdf, filename, _convert, _config, _errors))
+      {
+        sdfdbg << "Converting URDF file [" << _filename << "] to SDFormat"
+               << " and parsing it.\n";
+        return true;
+      }
+      else
+      {
+        sdferr << "Failed to parse the URDF file after converting to"
+               << " SDFormat.\n";
+        return false;
+      }
     }
     else
     {
-      sdferr << "parse as old deprecated model file failed.\n";
-      return false;
+      sdferr << "XML does not seem to be an SDFormat or an URDF file.\n";
     }
   }
 
@@ -845,30 +856,40 @@ bool readStringInternal(const std::string &_xmlString, const bool _convert,
     sdferr << "Error parsing XML from string: " << xmlDoc.ErrorStr() << '\n';
     return false;
   }
-  if (readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource), _convert, _config,
-              _errors))
+  tinyxml2::XMLElement *sdfXml = xmlDoc.FirstChildElement("sdf");
+  if (sdfXml)
   {
-    return true;
+    return readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource), _convert,
+                   _config, _errors);
   }
   else
   {
-    URDF2SDF u2g;
-    auto doc = makeSdfDoc();
-    u2g.InitModelString(_xmlString, _config, &doc);
-
-    if (sdf::readDoc(&doc, _sdf, std::string(kUrdfStringSource), _convert,
-                    _config, _errors))
+    tinyxml2::XMLElement *robotXml = xmlDoc.FirstChildElement("robot");
+    if (robotXml)
     {
-      sdfdbg << "Parsing from urdf.\n";
-      return true;
+      URDF2SDF u2g;
+      auto doc = makeSdfDoc();
+      u2g.InitModelString(_xmlString, _config, &doc);
+
+      if (sdf::readDoc(&doc, _sdf, std::string(kUrdfStringSource), _convert,
+                      _config, _errors))
+      {
+        sdfdbg << "Converting URDF to SDFormat and parsing it.\n";
+        return true;
+      }
+      else
+      {
+        sdferr << "Failed to parse the URDF file after converting to"
+               << " SDFormat\n";
+        return false;
+      }
     }
     else
     {
-      sdferr << "parse as old deprecated model file failed.\n";
+      sdferr << "XML does not seem to be an SDFormat or an URDF string.\n";
       return false;
     }
   }
-
   return false;
 }
 

src/parser_TEST.cc

@@ -836,6 +836,89 @@ TEST(Parser, ElementRemovedAfterDeprecation)
   EXPECT_EQ(errors[0].LineNumber().value(), 10);
 }
 
+/////////////////////////////////////////////////
+/// Check for non SDF non URDF error on a string
+TEST(Parser, ReadStringErrorNotSDForURDF)
+{
+  std::stringstream buffer;
+  sdf::testing::RedirectConsoleStream redir(
+      sdf::Console::Instance()->GetMsgStream(), &buffer);
+
+  #ifdef _WIN32
+    sdf::Console::Instance()->SetQuiet(false);
+    sdf::testing::ScopeExit revertSetQuiet(
+      []
+      {
+        sdf::Console::Instance()->SetQuiet(true);
+      });
+  #endif
+
+  sdf::SDFPtr sdf = InitSDF();
+  const std::string testString = R"(<?xml version="1.0" ?>
+    <foo>
+    </foo>)";
+  sdf::Errors errors;
+  EXPECT_FALSE(sdf::readString(testString, sdf, errors));
+  ASSERT_EQ(errors.size(), 0u);
+  EXPECT_NE(std::string::npos, buffer.str().find(
+      "XML does not seem to be an SDFormat or an URDF string."));
+}
+
+/////////////////////////////////////////////////
+/// Check for non SDF non URDF error on a file
+TEST(Parser, ReadFileErrorNotSDForURDF)
+{
+  std::stringstream buffer;
+  sdf::testing::RedirectConsoleStream redir(
+      sdf::Console::Instance()->GetMsgStream(), &buffer);
+
+  #ifdef _WIN32
+    sdf::Console::Instance()->SetQuiet(false);
+    sdf::testing::ScopeExit revertSetQuiet(
+      []
+      {
+        sdf::Console::Instance()->SetQuiet(true);
+      });
+  #endif
+
+  const auto path =
+      sdf::testing::TestFile("sdf", "box.dae");
+
+  sdf::Errors errors;
+  sdf::SDFPtr sdf = sdf::readFile(path, errors);
+  ASSERT_EQ(errors.size(), 0u);
+  EXPECT_NE(std::string::npos, buffer.str().find(
+      "XML does not seem to be an SDFormat or an URDF file."));
+}
+
+/////////////////////////////////////////////////
+/// Check that malformed SDF files do not throw confusing error
+TEST(Parser, ReadFileMalformedSDFNoRobotError)
+{
+  // Capture sdferr output
+  std::stringstream buffer;
+  auto old = std::cerr.rdbuf(buffer.rdbuf());
+
+#ifdef _WIN32
+  sdf::Console::Instance()->SetQuiet(false);
+#endif
+
+  const auto path =
+      sdf::testing::TestFile("sdf", "bad_syntax_pose.sdf");
+
+  sdf::Errors errors;
+  sdf::SDFPtr sdf = sdf::readFile(path, errors);
+  // Check the old error is not printed anymore
+  EXPECT_EQ(std::string::npos, buffer.str().find(
+      "Could not find the 'robot' element in the xml file"));
+
+  // Revert cerr rdbug so as to not interfere with other tests
+  std::cerr.rdbuf(old);
+#ifdef _WIN32
+  sdf::Console::Instance()->SetQuiet(true);
+#endif
+}
+
 /////////////////////////////////////////////////
 /// Main
 int main(int argc, char **argv)

src/parser_urdf.cc

@@ -3212,9 +3212,9 @@ void CreateCollision(tinyxml2::XMLElement* _elem,
   AddKeyValue(sdfCollision, "pose", Values2str(6, pose));
 
   // add geometry block
-  if (!_collision || !_collision->geometry)
+  if (!_collision->geometry)
   {
-    sdfdbg << "urdf2sdf: collision of link [" << _link->name
+    sdfmsg << "urdf2sdf: collision of link [" << _link->name
            << "] has no <geometry>.\n";
   }
   else
@@ -3258,9 +3258,9 @@ void CreateVisual(tinyxml2::XMLElement *_elem, urdf::LinkConstSharedPtr _link,
   AddKeyValue(sdfVisual, "pose", Values2str(6, pose));
 
   // insert geometry
-  if (!_visual || !_visual->geometry)
+  if (!_visual->geometry)
   {
-    sdfdbg << "urdf2sdf: visual of link [" << _link->name
+    sdfmsg << "urdf2sdf: visual of link [" << _link->name
            << "] has no <geometry>.\n";
   }
   else