From c0acbe7bd4cc17f1519efa9dee5a117c091d6be1 Mon Sep 17 00:00:00 2001 From: Guthrie Adams Date: Thu, 4 Nov 2021 12:27:18 -0500 Subject: [PATCH] =?UTF-8?q?Changed=20how=20asset=20creator=20generates=20t?= =?UTF-8?q?he=20asset=20instance.=20Instead=20of=20finding=20or=20creating?= =?UTF-8?q?=20the=20asset=20in=20the=20asset=20manager,=20one=20is=20direc?= =?UTF-8?q?tly=20instantiated=20and=20only=20added=20to=20the=20asset=20ma?= =?UTF-8?q?nager=20after=20creation=20is=20complete.=20This=20allows=20for?= =?UTF-8?q?=20reuse=20of=20previously=20loaded=20asset=20ids=20and=20will?= =?UTF-8?q?=20replace=20or=20=E2=80=9Creload=E2=80=9D=20a=20pre-existing?= =?UTF-8?q?=20asset=20with=20the=20newly=20created=20one.=20This=20also=20?= =?UTF-8?q?sends=20although=20correct=20notifications.=20Changed=20materia?= =?UTF-8?q?l=20document=20to=20load=20a=20source=20data=20are=20for=20the?= =?UTF-8?q?=20parent=20material=20as=20well.=20=20It=20was=20also=20a=20pr?= =?UTF-8?q?eviously=20loading=20the=20parent=20material=20products=20asset?= =?UTF-8?q?=20which=20would=20be=20out=20of=20date=20compared=20to=20the?= =?UTF-8?q?=20source=20data.=20Changed=20material=20document=20to=20track?= =?UTF-8?q?=20source=20file=20dependency=20changes=20instead=20of=20produc?= =?UTF-8?q?t=20asset=20changes.=20Fixed=20a=20bug=20or=20copy=20paste=20er?= =?UTF-8?q?ror=20in=20the=20document=20manager=20that=20was=20using=20the?= =?UTF-8?q?=20same=20container=20to=20track=20documents=20the=20modified?= =?UTF-8?q?=20externally=20and=20from=20other=20dependency=20changes.=20Re?= =?UTF-8?q?turning=20source=20data=20dependencies=20when=20creating=20a=20?= =?UTF-8?q?material=20asset=20from=20source.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Guthrie Adams --- .../RPI.Edit/Material/MaterialSourceData.h | 3 +- .../Include/Atom/RPI.Reflect/AssetCreator.h | 3 +- .../RPI.Edit/Material/MaterialSourceData.cpp | 61 +++++++++++------- .../Material/MaterialTypeSourceData.cpp | 3 - .../AtomToolsDocumentSystemComponent.cpp | 14 +++-- .../AtomToolsDocumentSystemComponent.h | 4 +- .../Code/Source/Document/MaterialDocument.cpp | 63 ++++++++++--------- .../Code/Source/Document/MaterialDocument.h | 11 +--- 8 files changed, 86 insertions(+), 76 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 17dc4556fb..77bf45023d 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 @@ -99,7 +99,8 @@ namespace AZ Data::AssetId assetId, AZStd::string_view materialSourceFilePath = "", bool elevateWarnings = true, - bool includeMaterialPropertyNames = true) const; + bool includeMaterialPropertyNames = true, + AZStd::unordered_set* sourceDependencies = nullptr) const; private: void ApplyPropertiesToAssetCreator( diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h index abdbe9cdce..79d43d0b8d 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h @@ -118,7 +118,7 @@ namespace AZ ResetIssueCounts(); // Because the asset creator can be used multiple times - m_asset = Data::AssetManager::Instance().CreateAsset(assetId, AZ::Data::AssetLoadBehavior::PreLoad); + m_asset = Data::Asset(assetId, aznew AssetDataT, AZ::Data::AssetLoadBehavior::PreLoad); m_beginCalled = true; if (!m_asset) @@ -138,6 +138,7 @@ namespace AZ } else { + Data::AssetManager::Instance().AssignAssetData(m_asset); result = AZStd::move(m_asset); success = true; } 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 7ff33b2d04..c912826026 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -188,14 +188,20 @@ namespace AZ } Outcome> MaterialSourceData::CreateMaterialAssetFromSourceData( - Data::AssetId assetId, AZStd::string_view materialSourceFilePath, bool elevateWarnings, bool includeMaterialPropertyNames) const + Data::AssetId assetId, + AZStd::string_view materialSourceFilePath, + bool elevateWarnings, + bool includeMaterialPropertyNames, + AZStd::unordered_set* sourceDependencies) const { - MaterialAssetCreator materialAssetCreator; - materialAssetCreator.SetElevateWarnings(elevateWarnings); + const auto materialTypeSourcePath = AssetUtils::ResolvePathReference(materialSourceFilePath, m_materialType); + const auto materialTypeAssetId = AssetUtils::MakeAssetId(materialTypeSourcePath, 0); + if (!materialTypeAssetId.IsSuccess()) + { + return Failure(); + } -#if 1 MaterialTypeSourceData materialTypeSourceData; - AZStd::string materialTypeSourcePath = AssetUtils::ResolvePathReference(materialSourceFilePath, m_materialType); if (!AZ::RPI::JsonUtils::LoadObjectFromFile(materialTypeSourcePath, materialTypeSourceData)) { return Failure(); @@ -203,21 +209,16 @@ namespace AZ materialTypeSourceData.ResolveUvEnums(); - auto materialTypeAsset = - materialTypeSourceData.CreateMaterialTypeAsset(AZ::Uuid::CreateRandom(), materialTypeSourcePath, elevateWarnings); - if (!materialTypeAsset.IsSuccess()) - { - return Failure(); - } -#else - auto materialTypeAsset = AssetUtils::LoadAsset(materialSourceFilePath, m_materialType); + const auto materialTypeAsset = + materialTypeSourceData.CreateMaterialTypeAsset(materialTypeAssetId.GetValue(), materialTypeSourcePath, elevateWarnings); if (!materialTypeAsset.IsSuccess()) { return Failure(); } -#endif - materialAssetCreator.Begin(assetId, *materialTypeAsset.GetValue().Get(), includeMaterialPropertyNames); + AZStd::unordered_set dependencies; + dependencies.insert(materialSourceFilePath); + dependencies.insert(materialTypeSourcePath); AZStd::vector parentSourceDataStack; @@ -225,6 +226,13 @@ namespace AZ AZStd::string parentSourceAbsPath = AssetUtils::ResolvePathReference(materialSourceFilePath, parentSourceRelPath); while (!parentSourceRelPath.empty()) { + if (dependencies.find(parentSourceAbsPath) != dependencies.end()) + { + return Failure(); + } + + dependencies.insert(parentSourceAbsPath); + MaterialSourceData parentSourceData; if (!AZ::RPI::JsonUtils::LoadObjectFromFile(parentSourceAbsPath, parentSourceData)) { @@ -232,20 +240,22 @@ namespace AZ } // Make sure the parent material has the same material type - auto materialTypeIdOutcome1 = AssetUtils::MakeAssetId(materialSourceFilePath, m_materialType, 0); - auto materialTypeIdOutcome2 = AssetUtils::MakeAssetId(parentSourceAbsPath, parentSourceData.m_materialType, 0); - if (!materialTypeIdOutcome1.IsSuccess() || !materialTypeIdOutcome2.IsSuccess() || - materialTypeIdOutcome1.GetValue() != materialTypeIdOutcome2.GetValue()) + const auto parentTypeAssetId = AssetUtils::MakeAssetId(parentSourceAbsPath, parentSourceData.m_materialType, 0); + if (!parentTypeAssetId || parentTypeAssetId.GetValue() != materialTypeAssetId.GetValue()) { AZ_Error("MaterialSourceData", false, "This material and its parent material do not share the same material type."); return Failure(); } - parentSourceDataStack.push_back(parentSourceData); parentSourceRelPath = parentSourceData.m_parentMaterial; parentSourceAbsPath = AssetUtils::ResolvePathReference(parentSourceAbsPath, parentSourceRelPath); + parentSourceDataStack.emplace_back(AZStd::move(parentSourceData)); } + MaterialAssetCreator materialAssetCreator; + materialAssetCreator.SetElevateWarnings(elevateWarnings); + materialAssetCreator.Begin(assetId, *materialTypeAsset.GetValue().Get(), includeMaterialPropertyNames); + while (!parentSourceDataStack.empty()) { parentSourceDataStack.back().ApplyPropertiesToAssetCreator(materialAssetCreator, materialSourceFilePath); @@ -257,12 +267,15 @@ namespace AZ Data::Asset material; if (materialAssetCreator.End(material)) { + if (sourceDependencies) + { + sourceDependencies->insert(dependencies.begin(), dependencies.end()); + } + return Success(material); } - else - { - return Failure(); - } + + return Failure(); } void MaterialSourceData::ApplyPropertiesToAssetCreator( diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp index b20873141b..396ba71e14 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -398,8 +398,6 @@ namespace AZ if (shaderAssetResult) { auto shaderAsset = shaderAssetResult.GetValue(); - shaderAsset.SetAutoLoadBehavior(Data::AssetLoadBehavior::PreLoad); - auto optionsLayout = shaderAsset->GetShaderOptionGroupLayout(); ShaderOptionGroup options{ optionsLayout }; for (auto& iter : shaderRef.m_shaderOptionValues) @@ -501,7 +499,6 @@ namespace AZ if (imageAssetResult) { auto imageAsset = imageAssetResult.GetValue(); - imageAsset.SetAutoLoadBehavior(Data::AssetLoadBehavior::PreLoad); materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), imageAsset); } else diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp index 5652e9fe23..00de2a7c4c 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp @@ -159,7 +159,7 @@ namespace AtomToolsFramework void AtomToolsDocumentSystemComponent::OnDocumentExternallyModified(const AZ::Uuid& documentId) { - m_documentIdsToReopen.insert(documentId); + m_documentIdsWithExternalChanges.insert(documentId); if (!AZ::TickBus::Handler::BusIsConnected()) { AZ::TickBus::Handler::BusConnect(); @@ -168,7 +168,7 @@ namespace AtomToolsFramework void AtomToolsDocumentSystemComponent::OnDocumentDependencyModified(const AZ::Uuid& documentId) { - m_documentIdsToReopen.insert(documentId); + m_documentIdsWithDependencyChanges.insert(documentId); if (!AZ::TickBus::Handler::BusIsConnected()) { AZ::TickBus::Handler::BusConnect(); @@ -177,7 +177,7 @@ namespace AtomToolsFramework void AtomToolsDocumentSystemComponent::OnTick([[maybe_unused]] float deltaTime, [[maybe_unused]] AZ::ScriptTimePoint time) { - for (const AZ::Uuid& documentId : m_documentIdsToReopen) + for (const AZ::Uuid& documentId : m_documentIdsWithExternalChanges) { AZStd::string documentPath; AtomToolsDocumentRequestBus::EventResult(documentPath, documentId, &AtomToolsDocumentRequestBus::Events::GetAbsolutePath); @@ -191,6 +191,8 @@ namespace AtomToolsFramework continue; } + m_documentIdsWithDependencyChanges.erase(documentId); + AtomToolsFramework::TraceRecorder traceRecorder(m_maxMessageBoxLineCount); bool openResult = false; @@ -204,7 +206,7 @@ namespace AtomToolsFramework } } - for (const AZ::Uuid& documentId : m_documentIdsToReopen) + for (const AZ::Uuid& documentId : m_documentIdsWithDependencyChanges) { AZStd::string documentPath; AtomToolsDocumentRequestBus::EventResult(documentPath, documentId, &AtomToolsDocumentRequestBus::Events::GetAbsolutePath); @@ -231,8 +233,8 @@ namespace AtomToolsFramework } } - m_documentIdsToReopen.clear(); - m_documentIdsToReopen.clear(); + m_documentIdsWithDependencyChanges.clear(); + m_documentIdsWithExternalChanges.clear(); AZ::TickBus::Handler::BusDisconnect(); } diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h index 9c556a07e7..a0f5eb085d 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h @@ -85,8 +85,8 @@ namespace AtomToolsFramework AZStd::intrusive_ptr m_settings; AZStd::function m_documentCreator; AZStd::unordered_map> m_documentMap; - AZStd::unordered_set m_documentIdsToRebuild; - AZStd::unordered_set m_documentIdsToReopen; + AZStd::unordered_set m_documentIdsWithExternalChanges; + AZStd::unordered_set m_documentIdsWithDependencyChanges; const size_t m_maxMessageBoxLineCount = 15; }; } // namespace AtomToolsFramework diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 1dcef43577..049d4c47ff 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -567,26 +567,26 @@ namespace MaterialEditor } } - void MaterialDocument::SourceFileChanged(AZStd::string relativePath, AZStd::string scanFolder, AZ::Uuid sourceUUID) + void MaterialDocument::SourceFileChanged(AZStd::string relativePath, AZStd::string scanFolder, [[maybe_unused]] AZ::Uuid sourceUUID) { - if (m_sourceAssetId.m_guid == sourceUUID) + auto sourcePath = AZ::RPI::AssetUtils::ResolvePathReference(scanFolder, relativePath); + + if (m_absolutePath == sourcePath) { // ignore notifications caused by saving the open document if (!m_saveTriggeredInternally) { AZ_TracePrintf("MaterialDocument", "Material document changed externally: '%s'.\n", m_absolutePath.c_str()); - AtomToolsFramework::AtomToolsDocumentNotificationBus::Broadcast(&AtomToolsFramework::AtomToolsDocumentNotificationBus::Events::OnDocumentExternallyModified, m_id); + AtomToolsFramework::AtomToolsDocumentNotificationBus::Broadcast( + &AtomToolsFramework::AtomToolsDocumentNotificationBus::Events::OnDocumentExternallyModified, m_id); } m_saveTriggeredInternally = false; } - } - - void MaterialDocument::OnAssetReloaded(AZ::Data::Asset asset) - { - if (m_dependentAssetIds.find(asset->GetId()) != m_dependentAssetIds.end()) + else if (m_sourceDependencies.find(sourcePath) != m_sourceDependencies.end()) { AZ_TracePrintf("MaterialDocument", "Material document dependency changed: '%s'.\n", m_absolutePath.c_str()); - AtomToolsFramework::AtomToolsDocumentNotificationBus::Broadcast(&AtomToolsFramework::AtomToolsDocumentNotificationBus::Events::OnDocumentDependencyModified, m_id); + AtomToolsFramework::AtomToolsDocumentNotificationBus::Broadcast( + &AtomToolsFramework::AtomToolsDocumentNotificationBus::Events::OnDocumentDependencyModified, m_id); } } @@ -655,7 +655,6 @@ namespace MaterialEditor return false; } - m_sourceAssetId = sourceAssetInfo.m_assetId; m_relativePath = sourceAssetInfo.m_relativePath; if (!AzFramework::StringFunc::Path::Normalize(m_relativePath)) { @@ -722,14 +721,15 @@ namespace MaterialEditor // we can create the asset dynamically from the source data. // Long term, the material document should not be concerned with assets at all. The viewport window should be the // only thing concerned with assets or instances. - auto createResult = m_materialSourceData.CreateMaterialAssetFromSourceData(Uuid::CreateRandom(), m_absolutePath, true); - if (!createResult) + auto materialAssetResult = + m_materialSourceData.CreateMaterialAssetFromSourceData(Uuid::CreateRandom(), m_absolutePath, true, true, &m_sourceDependencies); + if (!materialAssetResult) { AZ_Error("MaterialDocument", false, "Material asset could not be created from source data: '%s'.", m_absolutePath.c_str()); return false; } - m_materialAsset = createResult.GetValue(); + m_materialAsset = materialAssetResult.GetValue(); if (!m_materialAsset.IsReady()) { AZ_Error("MaterialDocument", false, "Material asset is not ready: '%s'.", m_absolutePath.c_str()); @@ -743,28 +743,35 @@ namespace MaterialEditor return false; } - // track material type asset to notify when dependencies change - m_dependentAssetIds.insert(materialTypeAsset->GetId()); - AZ::Data::AssetBus::MultiHandler::BusConnect(materialTypeAsset->GetId()); - AZStd::array_view parentPropertyValues = materialTypeAsset->GetDefaultPropertyValues(); AZ::Data::Asset parentMaterialAsset; if (!m_materialSourceData.m_parentMaterial.empty()) { - // There is a parent for this material - auto parentMaterialResult = AssetUtils::LoadAsset(m_absolutePath, m_materialSourceData.m_parentMaterial); - if (!parentMaterialResult) + AZ::RPI::MaterialSourceData parentMaterialSourceData; + const auto parentMaterialFilePath = AssetUtils::ResolvePathReference(m_absolutePath, m_materialSourceData.m_parentMaterial); + if (!AZ::RPI::JsonUtils::LoadObjectFromFile(parentMaterialFilePath, parentMaterialSourceData)) { - AZ_Error("MaterialDocument", false, "Parent material asset could not be loaded: '%s'.", m_materialSourceData.m_parentMaterial.c_str()); + AZ_Error("MaterialDocument", false, "Material parent source data could not be loaded for: '%s'.", parentMaterialFilePath.c_str()); return false; } - parentMaterialAsset = parentMaterialResult.GetValue(); - parentPropertyValues = parentMaterialAsset->GetPropertyValues(); + const auto parentMaterialAssetIdResult = AssetUtils::MakeAssetId(parentMaterialFilePath, 0); + if (!parentMaterialAssetIdResult) + { + AZ_Error("MaterialDocument", false, "Material parent asset ID could not be created: '%s'.", parentMaterialFilePath.c_str()); + return false; + } - // track parent material asset to notify when dependencies change - m_dependentAssetIds.insert(parentMaterialAsset->GetId()); - AZ::Data::AssetBus::MultiHandler::BusConnect(parentMaterialAsset->GetId()); + auto parentMaterialAssetResult = m_materialSourceData.CreateMaterialAssetFromSourceData( + parentMaterialAssetIdResult.GetValue(), parentMaterialFilePath, true, true, &m_sourceDependencies); + if (!parentMaterialAssetResult) + { + AZ_Error("MaterialDocument", false, "Material parent asset could not be created from source data: '%s'.", parentMaterialFilePath.c_str()); + return false; + } + + parentMaterialAsset = parentMaterialAssetResult.GetValue(); + parentPropertyValues = parentMaterialAsset->GetPropertyValues(); } // Creating a material from a material asset will fail if a texture is referenced but not loaded @@ -913,15 +920,13 @@ namespace MaterialEditor void MaterialDocument::Clear() { AZ::TickBus::Handler::BusDisconnect(); - AZ::Data::AssetBus::MultiHandler::BusDisconnect(); AzToolsFramework::AssetSystemBus::Handler::BusDisconnect(); m_materialAsset = {}; m_materialInstance = {}; m_absolutePath.clear(); m_relativePath.clear(); - m_sourceAssetId = {}; - m_dependentAssetIds.clear(); + m_sourceDependencies.clear(); m_saveTriggeredInternally = {}; m_compilePending = {}; m_properties.clear(); diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h index 03997a2a91..452111f99a 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h @@ -29,7 +29,6 @@ namespace MaterialEditor : public AtomToolsFramework::AtomToolsDocument , public MaterialDocumentRequestBus::Handler , private AZ::TickBus::Handler - , private AZ::Data::AssetBus::MultiHandler , private AzToolsFramework::AssetSystemBus::Handler { public: @@ -105,11 +104,6 @@ namespace MaterialEditor void SourceFileChanged(AZStd::string relativePath, AZStd::string scanFolder, AZ::Uuid sourceUUID) override; ////////////////////////////////////////////////////////////////////////// - ////////////////////////////////////////////////////////////////////////// - // AZ::Data::AssetBus::Router overrides... - void OnAssetReloaded(AZ::Data::Asset asset) override; - ////////////////////////////////////////////////////////////////////////// - bool SavePropertiesToSourceData(AZ::RPI::MaterialSourceData& sourceData, PropertyFilterFunction propertyFilter) const; bool OpenInternal(AZStd::string_view loadPath); @@ -137,11 +131,8 @@ namespace MaterialEditor // Material instance being edited AZ::Data::Instance m_materialInstance; - // Asset used to open document - AZ::Data::AssetId m_sourceAssetId; - // Set of assets that can trigger a document reload - AZStd::unordered_set m_dependentAssetIds; + AZStd::unordered_set m_sourceDependencies; // Track if document saved itself last to skip external modification notification bool m_saveTriggeredInternally = false;