From 7130c1fe863fcf0aacf6ecee0754385247ddd737 Mon Sep 17 00:00:00 2001 From: Ashton Larkin <42042756+adlarkin@users.noreply.github.com> Date: Fri, 25 Feb 2022 16:28:44 -0700 Subject: [PATCH] initial review feedback Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com> --- .../sdf/usd/{sdf_parser => }/Conversions.hh | 34 ++++++++++--------- usd/src/CMakeLists.txt | 6 ++-- usd/src/{sdf_parser => }/Conversions.cc | 32 ++++++++--------- usd/src/{sdf_parser => }/Conversions_TEST.cc | 28 ++++++++------- usd/src/sdf_parser/Geometry.cc | 2 +- 5 files changed, 53 insertions(+), 49 deletions(-) rename usd/include/sdf/usd/{sdf_parser => }/Conversions.hh (55%) rename usd/src/{sdf_parser => }/Conversions.cc (89%) rename usd/src/{sdf_parser => }/Conversions_TEST.cc (90%) diff --git a/usd/include/sdf/usd/sdf_parser/Conversions.hh b/usd/include/sdf/usd/Conversions.hh similarity index 55% rename from usd/include/sdf/usd/sdf_parser/Conversions.hh rename to usd/include/sdf/usd/Conversions.hh index bc18aacd0..ea11c24bb 100644 --- a/usd/include/sdf/usd/sdf_parser/Conversions.hh +++ b/usd/include/sdf/usd/Conversions.hh @@ -16,8 +16,8 @@ */ -#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 @@ -25,6 +25,7 @@ #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 &_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 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 &_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 convert( + const sdf::Material &_in); } } } diff --git a/usd/src/CMakeLists.txt b/usd/src/CMakeLists.txt index bdf326c63..29b4ef4b5 100644 --- a/usd/src/CMakeLists.txt +++ b/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 ) diff --git a/usd/src/sdf_parser/Conversions.cc b/usd/src/Conversions.cc similarity index 89% rename from usd/src/sdf_parser/Conversions.cc rename to usd/src/Conversions.cc index 03b76d135..1099b3026 100644 --- a/usd/src/sdf_parser/Conversions.cc +++ b/usd/src/Conversions.cc @@ -15,7 +15,7 @@ * */ -#include "sdf/usd/sdf_parser/Conversions.hh" +#include "sdf/usd/Conversions.hh" #include @@ -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,16 +76,13 @@ 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; @@ -94,8 +90,7 @@ namespace sdf std::shared_ptr convert(const sdf::Material &_in) { - std::shared_ptr out = - std::make_shared(); + auto out = std::make_shared(); 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(), diff --git a/usd/src/sdf_parser/Conversions_TEST.cc b/usd/src/Conversions_TEST.cc similarity index 90% rename from usd/src/sdf_parser/Conversions_TEST.cc rename to usd/src/Conversions_TEST.cc index 450a150cb..5e7d78f80 100644 --- a/usd/src/sdf_parser/Conversions_TEST.cc +++ b/usd/src/Conversions_TEST.cc @@ -16,20 +16,19 @@ */ #include + #include #include #include - #include -#include -#include - -#include +#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 materialCommon = + const std::shared_ptr 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 materialCommon = std::make_shared(); @@ -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()); diff --git a/usd/src/sdf_parser/Geometry.cc b/usd/src/sdf_parser/Geometry.cc index c3c4d4d4e..abffbd283 100644 --- a/usd/src/sdf_parser/Geometry.cc +++ b/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"