review feedback

Signed-off-by: Ashton Larkin <[email protected]>
gazebosim · Mar 3, 2022 · cdc0257 · cdc0257
1 parent 5f33b20
commit cdc0257
Show file tree

Hide file tree

Showing 7 changed files with 200 additions and 175 deletions.
diff --git a/test/sdf/basic_shapes.sdf b/test/sdf/basic_shapes.sdf
@@ -144,12 +144,12 @@
             <diffuse>0.2 0.5 0.1 1.0</diffuse>
             <specular>0.7 0.3 0.5 0.9</specular>
             <pbr>
-              <specular>
+              <metal>
                 <albedo_map>albedo_map.png</albedo_map>
                 <normal_map>normal_map.png</normal_map>
                 <roughness_map>roughness_map.png</roughness_map>
                 <metalness_map>metalness_map.png</metalness_map>
-              </specular>
+              </metal>
             </pbr>
           </material>
         </visual>

diff --git a/usd/include/sdf/usd/sdf_parser/Material.hh b/usd/include/sdf/usd/sdf_parser/Material.hh
@@ -24,6 +24,7 @@
 // included.
 #pragma push_macro ("__DEPRECATED")
 #undef __DEPRECATED
+#include <pxr/usd/sdf/path.h>
 #include <pxr/usd/usd/stage.h>
 #include <pxr/usd/usdShade/material.h>
 #pragma pop_macro ("__DEPRECATED")
@@ -44,14 +45,14 @@ namespace sdf
     /// \param[in] _materialSdf The SDF material to parse.
     /// \param[in] _stage The stage that should contain the USD representation
     /// of _material.
-    /// \param[out] _materialPath Material usd path
+    /// \param[out] _materialPath USD Material path
     /// \return UsdErrors, which is a list of UsdError objects. This list is
-    /// empty if no errors occurred when parsing _materialSdf to _materialUsd
-    UsdErrors IGNITION_SDFORMAT_USD_VISIBLE
-      ParseSdfMaterial(
+    /// empty if no errors occurred when parsing _materialSdf its USD
+    /// representation
+    UsdErrors IGNITION_SDFORMAT_USD_VISIBLE ParseSdfMaterial(
         const sdf::Material *_materialSdf,
         pxr::UsdStageRefPtr &_stage,
-        std::string &_materialPath);
+        pxr::SdfPath &_materialPath);
   }
   }
 }

diff --git a/usd/src/cmd/sdf2usd.cc b/usd/src/cmd/sdf2usd.cc
@@ -95,13 +95,15 @@ std::string findFileByName(const std::string &_path, const std::string &_name)
 /// \brief Get the full path of a file based on the extension
 /// \param[in] _path Where to begin searching for the file
 /// \param[in] _extension The extension of the file
+/// \param[in] _insertDirectories Whether subdirectories should be inserted as
+/// needed when looking for the file (true) or not (false)
 /// \return The full path to the file with an extension _extension. Empty
 /// string is returned if the file could not be found.
 std::string findFileByExtension(
   const std::string &_path, const std::string &_extension,
-  bool insertDirectories = false)
+  bool _insertDirectories = false)
 {
-  if (insertDirectories)
+  if (_insertDirectories)
   {
     for (ignition::common::DirIter file(_path);
       file != ignition::common::DirIter(); ++file)

diff --git a/usd/src/sdf_parser/Geometry.cc b/usd/src/sdf_parser/Geometry.cc
@@ -383,25 +383,39 @@ namespace usd
         pxr::GfVec3f(meshMax.X(), meshMax.Y(), meshMax.Z()));
       usdMesh.CreateExtentAttr().Set(extentBounds);
 
+      // TODO(adlarkin) update this call in sdf13 to avoid casting the index to
+      // an int:
+      // https://github.com/ignitionrobotics/ign-common/pull/319
       int materialIndex = subMesh->MaterialIndex();
       if (materialIndex != -1)
       {
         auto material = ignMesh->MaterialByIndex(materialIndex);
         const sdf::Material materialSdf = sdf::usd::convert(material);
-        std::string materialPath;
+        pxr::SdfPath materialPath;
         UsdErrors materialErrors = ParseSdfMaterial(
           &materialSdf, _stage, materialPath);
         if (!materialErrors.empty())
         {
           errors.push_back(UsdError(
             sdf::usd::UsdErrorCode::SDF_TO_USD_PARSING_ERROR,
-            "Unable to parse material"));
+            "Unable to convert material [" + std::to_string(materialIndex)
+            + "] of submesh named [" + subMesh->Name()
+            + "] to a USD material."));
           return errors;
         }
 
-        auto materialUSD = pxr::UsdShadeMaterial(_stage->GetPrimAtPath(
-          pxr::SdfPath(materialPath)));
+        auto materialPrim = _stage->GetPrimAtPath(materialPath);
+        if (!materialPrim)
+        {
+          errors.push_back(UsdError(
+                sdf::usd::UsdErrorCode::INVALID_PRIM_PATH,
+                "Unable to get material prim at path ["
+                + materialPath.GetString()
+                + "], but a prim should exist at this path."));
+          return errors;
+        }
 
+        auto materialUSD = pxr::UsdShadeMaterial(materialPrim);
         if (materialUSD &&
             (materialSdf.Emissive() != ignition::math::Color(0, 0, 0, 1)
              || materialSdf.Specular() != ignition::math::Color(0, 0, 0, 1)
@@ -413,9 +427,8 @@ namespace usd
         {
           errors.push_back(UsdError(
               sdf::usd::UsdErrorCode::SDF_TO_USD_PARSING_ERROR,
-              "Unable to convert material [" + std::to_string(materialIndex)
-              + "] of submesh named [" + subMesh->Name()
-              + "] to a USD material."));
+              "The prim at path [" + materialPath.GetString()
+              + "] is not a pxr::UsdShadeMaterial object."));
           return errors;
         }
       }