initial review feedback

Signed-off-by: Ashton Larkin <[email protected]>

usd/include/sdf/usd/sdf_parser/Conversions.hh → usd/include/sdf/usd/Conversions.hh

@@ -16,15 +16,16 @@
  */
 
 
-#ifndef SDF_USD_SDF_PARSER_CONVERSIONS_HH_
-#define SDF_USD_SDF_PARSER_CONVERSIONS_HH_
+#ifndef SDF_USD_CONVERSIONS_HH_
+#define SDF_USD_CONVERSIONS_HH_
 
 #include <memory>
 
 #include <ignition/common/Material.hh>
 
 #include "sdf/Material.hh"
 #include "sdf/sdf_config.h"
+#include "sdf/usd/Export.hh"
 
 namespace sdf
 {
@@ -33,20 +34,21 @@ namespace sdf
   //
   namespace usd
   {
-  //
-  /// \brief Specialized conversion from an Ignition Common Material
-  /// to a SDF material
-  /// \param[in] _in Ignition Common Material.
-  /// \return SDF material.
-  SDFORMAT_VISIBLE
-  sdf::Material convert(const std::shared_ptr<ignition::common::Material> &_in);
-
-  /// \brief Specialized conversion from an SDF material to a Ignition Common
-  /// material.
-  /// \param[in] _in SDF material.
-  /// \return Ignition Common Material.
-  SDFORMAT_VISIBLE
-  std::shared_ptr<ignition::common::Material> convert(const sdf::Material &_in);
+    /// \brief Specialized conversion from an Ignition Common Material
+    /// to a SDF material
+    /// \param[in] _in Ignition Common Material.
+    /// \return SDF material.
+    IGNITION_SDFORMAT_USD_VISIBLE
+    sdf::Material convert(
+        const std::shared_ptr<ignition::common::Material> &_in);
+
+    /// \brief Specialized conversion from an SDF material to a Ignition Common
+    /// material.
+    /// \param[in] _in SDF material.
+    /// \return Ignition Common Material.
+    IGNITION_SDFORMAT_USD_VISIBLE
+    std::shared_ptr<ignition::common::Material> convert(
+        const sdf::Material &_in);
   }
   }
 }

usd/src/CMakeLists.txt

@@ -1,6 +1,6 @@
 set(sources
+  Conversions.cc
   UsdError.cc
-  sdf_parser/Conversions.cc
   sdf_parser/Geometry.cc
   sdf_parser/Joint.cc
   sdf_parser/Light.cc
@@ -28,15 +28,17 @@ target_link_libraries(${usd_target}
 
 set(gtest_sources
   sdf_parser/sdf2usd_TEST.cc
-  sdf_parser/Conversions_TEST.cc
   sdf_parser/Geometry_Sdf2Usd_TEST.cc
   sdf_parser/Joint_Sdf2Usd_TEST.cc
   sdf_parser/Light_Sdf2Usd_TEST.cc
   sdf_parser/Link_Sdf2Usd_TEST.cc
   sdf_parser/Material_Sdf2Usd_TEST.cc
+  # TODO(adlarkin) add a test for SDF -> USD models once model parsing
+  # functionality is complete
   sdf_parser/Sensors_Sdf2Usd_TEST.cc
   sdf_parser/Visual_Sdf2Usd_TEST.cc
   sdf_parser/World_Sdf2Usd_TEST.cc
+  Conversions_TEST.cc
   UsdError_TEST.cc
   UsdUtils_TEST.cc
 )

usd/src/sdf_parser/Conversions.cc → usd/src/Conversions.cc

@@ -15,7 +15,7 @@
  *
  */
 
-#include "sdf/usd/sdf_parser/Conversions.hh"
+#include "sdf/usd/Conversions.hh"
 
 #include <ignition/common/Pbr.hh>
 
@@ -47,10 +47,13 @@ namespace sdf
       pbrWorkflow.SetMetalnessMap(pbr->MetalnessMap());
       pbrWorkflow.SetEmissiveMap(pbr->EmissiveMap());
       pbrWorkflow.SetRoughnessMap(pbr->RoughnessMap());
-
+      pbrWorkflow.SetSpecularMap(pbr->SpecularMap());
       pbrWorkflow.SetEnvironmentMap(pbr->EnvironmentMap());
       pbrWorkflow.SetAmbientOcclusionMap(pbr->AmbientOcclusionMap());
       pbrWorkflow.SetLightMap(pbr->LightMap());
+      pbrWorkflow.SetRoughness(pbr->Roughness());
+      pbrWorkflow.SetGlossiness(pbr->Glossiness());
+      pbrWorkflow.SetMetalness(pbr->Metalness());
 
       if (pbr->NormalMapType() == ignition::common::NormalMapSpace::TANGENT)
       {
@@ -63,10 +66,6 @@ namespace sdf
           pbr->NormalMap(), sdf::NormalMapSpace::OBJECT);
       }
 
-      pbrWorkflow.SetRoughness(pbr->Roughness());
-      pbrWorkflow.SetGlossiness(pbr->Glossiness());
-      pbrWorkflow.SetMetalness(pbr->Metalness());
-
       if (pbr->Type() == ignition::common::PbrType::METAL)
       {
         pbrOut.SetWorkflow(sdf::PbrWorkflowType::METAL, pbrWorkflow);
@@ -77,25 +76,21 @@ namespace sdf
       }
       out.SetPbrMaterial(pbrOut);
     }
-    else
+    else if (!_in->TextureImage().empty())
     {
-      if (!_in->TextureImage().empty())
-      {
-        sdf::Pbr pbrOut;
-        sdf::PbrWorkflow pbrWorkflow;
-        pbrWorkflow.SetAlbedoMap(_in->TextureImage());
-        pbrOut.SetWorkflow(sdf::PbrWorkflowType::SPECULAR, pbrWorkflow);
-        out.SetPbrMaterial(pbrOut);
-      }
+      sdf::Pbr pbrOut;
+      sdf::PbrWorkflow pbrWorkflow;
+      pbrWorkflow.SetAlbedoMap(_in->TextureImage());
+      pbrOut.SetWorkflow(sdf::PbrWorkflowType::SPECULAR, pbrWorkflow);
+      out.SetPbrMaterial(pbrOut);
     }
 
     return out;
   }
 
   std::shared_ptr<ignition::common::Material> convert(const sdf::Material &_in)
   {
-    std::shared_ptr<ignition::common::Material> out =
-      std::make_shared<ignition::common::Material>();
+    auto out = std::make_shared<ignition::common::Material>();
     out->SetEmissive(_in.Emissive());
     out->SetDiffuse(_in.Diffuse());
     out->SetSpecular(_in.Specular());
@@ -129,6 +124,7 @@ namespace sdf
         pbrOut.SetMetalnessMap(pbrWorkflow->MetalnessMap());
         pbrOut.SetEmissiveMap(pbrWorkflow->EmissiveMap());
         pbrOut.SetRoughnessMap(pbrWorkflow->RoughnessMap());
+        pbrOut.SetSpecularMap(pbrWorkflow->SpecularMap());
         pbrOut.SetEnvironmentMap(pbrWorkflow->EnvironmentMap());
         pbrOut.SetAmbientOcclusionMap(pbrWorkflow->AmbientOcclusionMap());
         pbrOut.SetLightMap(pbrWorkflow->LightMap());
@@ -142,7 +138,7 @@ namespace sdf
             pbrWorkflow->NormalMap(),
             ignition::common::NormalMapSpace::TANGENT);
         }
-        else if(pbrWorkflow->NormalMapType() == sdf::NormalMapSpace::OBJECT)
+        else if (pbrWorkflow->NormalMapType() == sdf::NormalMapSpace::OBJECT)
         {
           pbrOut.SetNormalMap(
             pbrWorkflow->NormalMap(),

usd/src/sdf_parser/Conversions_TEST.cc → usd/src/Conversions_TEST.cc

@@ -16,20 +16,19 @@
  */
 
 #include <memory>
+
 #include <gtest/gtest.h>
 
 #include <ignition/common/Material.hh>
 #include <ignition/common/Pbr.hh>
-
 #include <ignition/math/Color.hh>
 
-#include <sdf/Material.hh>
-#include <sdf/Pbr.hh>
-
-#include <sdf/usd/sdf_parser/Conversions.hh>
+#include "sdf/Material.hh"
+#include "sdf/Pbr.hh"
+#include "sdf/usd/Conversions.hh"
 
 /////////////////////////////////////////////////
-TEST(Conversions, Conversions_sdf_to_common)
+TEST(Conversions, SdfToCommonMaterial)
 {
   sdf::Material material;
   material.SetEmissive(ignition::math::Color(1, 0.2, 0.2, 0.7));
@@ -47,6 +46,7 @@ TEST(Conversions, Conversions_sdf_to_common)
   pbrWorkflow.SetMetalnessMap("MetalnessMap");
   pbrWorkflow.SetEmissiveMap("EmissiveMap");
   pbrWorkflow.SetRoughnessMap("RoughnessMap");
+  pbrWorkflow.SetSpecularMap("SpecularMap");
   pbrWorkflow.SetEnvironmentMap("EnvironmentMap");
   pbrWorkflow.SetAmbientOcclusionMap("AmbientOcclusionMap");
   pbrWorkflow.SetLightMap("LightMap");
@@ -59,10 +59,11 @@ TEST(Conversions, Conversions_sdf_to_common)
   pbrSDF.SetWorkflow(sdf::PbrWorkflowType::METAL, pbrWorkflow);
   material.SetPbrMaterial(pbrSDF);
 
-  std::shared_ptr<ignition::common::Material> materialCommon =
+  const std::shared_ptr<ignition::common::Material> materialCommon =
     sdf::usd::convert(material);
+  ASSERT_NE(nullptr, materialCommon);
   const ignition::common::Pbr * pbrCommon = materialCommon->PbrMaterial();
-  EXPECT_TRUE(pbrCommon);
+  ASSERT_NE(nullptr, pbrCommon);
 
   EXPECT_EQ(material.Emissive(), materialCommon->Emissive());
   EXPECT_EQ(material.Diffuse(), materialCommon->Diffuse());
@@ -77,7 +78,7 @@ TEST(Conversions, Conversions_sdf_to_common)
   EXPECT_EQ(pbrWorkflow.MetalnessMap(), pbrCommon->MetalnessMap());
   EXPECT_EQ(pbrWorkflow.EmissiveMap(), pbrCommon->EmissiveMap());
   EXPECT_EQ(pbrWorkflow.RoughnessMap(), pbrCommon->RoughnessMap());
-
+  EXPECT_EQ(pbrWorkflow.SpecularMap(), pbrCommon->SpecularMap());
   EXPECT_EQ(pbrWorkflow.EnvironmentMap(), pbrCommon->EnvironmentMap());
   EXPECT_EQ(pbrWorkflow.AmbientOcclusionMap(),
     pbrCommon->AmbientOcclusionMap());
@@ -93,7 +94,7 @@ TEST(Conversions, Conversions_sdf_to_common)
   EXPECT_DOUBLE_EQ(pbrWorkflow.Metalness(), pbrCommon->Metalness());
 }
 
-TEST(Conversions, Conversions_common_to_sdf)
+TEST(Conversions, CommonToSdfMaterial)
 {
   std::shared_ptr<ignition::common::Material> materialCommon =
     std::make_shared<ignition::common::Material>();
@@ -112,6 +113,7 @@ TEST(Conversions, Conversions_common_to_sdf)
   pbrCommon.SetMetalnessMap("MetalnessMap");
   pbrCommon.SetEmissiveMap("EmissiveMap");
   pbrCommon.SetRoughnessMap("RoughnessMap");
+  pbrCommon.SetSpecularMap("SpecularMap");
   pbrCommon.SetEnvironmentMap("EnvironmentMap");
   pbrCommon.SetAmbientOcclusionMap("AmbientOcclusionMap");
   pbrCommon.SetLightMap("LightMap");
@@ -123,11 +125,13 @@ TEST(Conversions, Conversions_common_to_sdf)
 
   materialCommon->SetPbrMaterial(pbrCommon);
 
-  sdf::Material material = sdf::usd::convert(materialCommon);
+  const sdf::Material material = sdf::usd::convert(materialCommon);
 
   const sdf::Pbr * pbr = material.PbrMaterial();
+  ASSERT_NE(nullptr, pbr);
   const sdf::PbrWorkflow * pbrWorkflow =
     pbr->Workflow(sdf::PbrWorkflowType::METAL);
+  ASSERT_NE(nullptr, pbrWorkflow);
 
   EXPECT_EQ(material.Emissive(), materialCommon->Emissive());
   EXPECT_EQ(material.Diffuse(), materialCommon->Diffuse());
@@ -142,7 +146,7 @@ TEST(Conversions, Conversions_common_to_sdf)
   EXPECT_EQ(pbrWorkflow->MetalnessMap(), pbrCommon.MetalnessMap());
   EXPECT_EQ(pbrWorkflow->EmissiveMap(), pbrCommon.EmissiveMap());
   EXPECT_EQ(pbrWorkflow->RoughnessMap(), pbrCommon.RoughnessMap());
-
+  EXPECT_EQ(pbrWorkflow->SpecularMap(), pbrCommon.SpecularMap());
   EXPECT_EQ(pbrWorkflow->EnvironmentMap(), pbrCommon.EnvironmentMap());
   EXPECT_EQ(pbrWorkflow->AmbientOcclusionMap(),
     pbrCommon.AmbientOcclusionMap());

usd/src/sdf_parser/Geometry.cc

@@ -58,7 +58,7 @@
 #include "sdf/Mesh.hh"
 #include "sdf/Plane.hh"
 #include "sdf/Sphere.hh"
-#include "sdf/usd/sdf_parser/Conversions.hh"
+#include "sdf/usd/Conversions.hh"
 #include "sdf/usd/sdf_parser/Material.hh"
 
 #include "../UsdUtils.hh"