From ddab03d6785e66d425fada619dad1d2f9b95fe78 Mon Sep 17 00:00:00 2001
From: santorac <55155825+santorac@users.noreply.github.com>
Date: Tue, 8 Feb 2022 11:18:17 -0800
Subject: [PATCH] Reponse to code review feedback. The main change is making
 GetPropertyValues return by const ref.

Signed-off-by: santorac <55155825+santorac@users.noreply.github.com>
---
 .../Include/Atom/RPI.Edit/Material/MaterialSourceData.h   | 2 +-
 .../Code/Source/RPI.Edit/Material/MaterialSourceData.cpp  | 6 +++---
 .../RPI/Code/Tests/Material/MaterialSourceDataTests.cpp   | 8 ++++----
 .../Code/Source/Document/MaterialDocument.cpp             | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h
index b8c613fd12..8819cbc78b 100644
--- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h
+++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h
@@ -75,7 +75,7 @@ namespace AZ
 
             void SetPropertyValue(const Name& propertyId, const MaterialPropertyValue& value);
             const MaterialPropertyValue& GetPropertyValue(const Name& propertyId) const;
-            PropertyValueMap GetPropertyValues() const;
+            const PropertyValueMap& GetPropertyValues() const;
             bool HasPropertyValue(const Name& propertyId) const;
             void RemovePropertyValue(const Name& propertyId);
 
diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp
index e5092b5480..fe6ec415a3 100644
--- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp
+++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp
@@ -100,7 +100,7 @@ namespace AZ
             m_propertyValues.erase(propertyId);
         }
         
-        MaterialSourceData::PropertyValueMap MaterialSourceData::GetPropertyValues() const
+        const MaterialSourceData::PropertyValueMap& MaterialSourceData::GetPropertyValues() const
         {
             return m_propertyValues;
         }
@@ -112,9 +112,9 @@ namespace AZ
         
         void MaterialSourceData::ConvertToNewDataFormat()
         {
-            for (auto& [groupName, propertyList] : m_propertiesOld)
+            for (const auto& [groupName, propertyList] : m_propertiesOld)
             {
-                for (auto& [propertyName, propertyValue] : propertyList)
+                for (const auto& [propertyName, propertyValue] : propertyList)
                 {
                     SetPropertyValue(MaterialPropertyId{groupName, propertyName}, propertyValue);
                 }
diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp
index 362d7d8b6d..7676772389 100644
--- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp
+++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp
@@ -357,7 +357,7 @@ namespace UnitTest
         return fixupType(a.GetTypeId()) == fixupType(targetTypeId) && fixupType(b.GetTypeId()) == fixupType(targetTypeId);
     }
 
-    void CheckEqual(MaterialSourceData& a, MaterialSourceData& b)
+    void CheckEqual(const MaterialSourceData& a, const MaterialSourceData& b)
     {
         EXPECT_STREQ(a.m_materialType.c_str(), b.m_materialType.c_str());
         EXPECT_STREQ(a.m_description.c_str(), b.m_description.c_str());
@@ -1091,15 +1091,15 @@ namespace UnitTest
         MaterialSourceData sourceDataLevel3 = MaterialUtils::LoadMaterialSourceData("@exefolder@/Temp/m3.material").TakeValue();
         
         auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAssetFromSourceData(Uuid::CreateRandom());
-        EXPECT_TRUE(materialAssetLevel1.IsSuccess());
+        ASSERT_TRUE(materialAssetLevel1.IsSuccess());
         EXPECT_TRUE(materialAssetLevel1.GetValue()->WasPreFinalized());
 
         auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAssetFromSourceData(Uuid::CreateRandom());
-        EXPECT_TRUE(materialAssetLevel2.IsSuccess());
+        ASSERT_TRUE(materialAssetLevel2.IsSuccess());
         EXPECT_TRUE(materialAssetLevel2.GetValue()->WasPreFinalized());
 
         auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAssetFromSourceData(Uuid::CreateRandom());
-        EXPECT_TRUE(materialAssetLevel3.IsSuccess());
+        ASSERT_TRUE(materialAssetLevel3.IsSuccess());
         EXPECT_TRUE(materialAssetLevel3.GetValue()->WasPreFinalized());
 
         auto layout = materialAssetLevel1.GetValue()->GetMaterialPropertiesLayout();
diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp
index 6f576c9e02..8db8ee1eeb 100644
--- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp
+++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp
@@ -677,7 +677,7 @@ namespace MaterialEditor
         if (!loadResult)
         {
             AZ_Error("MaterialDocument", false, "Material source data could not be loaded: '%s'.", m_absolutePath.c_str());
-            return OpenFailed();
+            return false;
         }
 
         m_materialSourceData = loadResult.TakeValue();