From f8ca427954cf335ead7915108238548414861f4f Mon Sep 17 00:00:00 2001 From: Guthrie Adams <82461473+gadams3@users.noreply.github.com> Date: Tue, 14 Dec 2021 10:52:01 -0600 Subject: [PATCH] Fixed several material editor bugs related to file paths and hot reload prompts (#6374) * draft Signed-off-by: Guthrie Adams * Removes automatic generation of relative paths for external references for materials Updated material editor functions for creating new materials, creating or saving model or lighting presets, to save to the project asset folder instead of the material folder which is not included in the new templates Changed function for getting saved file names to handle case where Qt save file dialog adds double extensions if the extension contains a dot Signed-off-by: Guthrie Adams * Fixed problems with material editor hot reloading after documents or dependencies changed. Triggering message boxes within the tick function, which is executed from the main application timer, caused the tick function to be called a second time recursively. Switched from using the tick bus to a timer so that the documents re opening and dialogs are triggered outside of the main tick. Signed-off-by: Guthrie Adams --- .../Util/MaterialPropertyUtil.h | 8 +++--- .../AtomToolsDocumentSystemComponent.cpp | 25 ++++++++++-------- .../AtomToolsDocumentSystemComponent.h | 9 +++---- .../Code/Source/Util/MaterialPropertyUtil.cpp | 26 +++++-------------- .../Code/Source/Util/Util.cpp | 6 ++++- .../Code/Tests/AtomToolsFrameworkTest.cpp | 18 ++++++------- .../Code/Source/Document/MaterialDocument.cpp | 2 +- .../CreateMaterialDialog.cpp | 20 ++++++-------- .../MaterialEditorBrowserInteractions.cpp | 9 ++++--- .../ViewportSettingsInspector.cpp | 14 +++++----- 10 files changed, 60 insertions(+), 77 deletions(-) diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Include/AtomToolsFramework/Util/MaterialPropertyUtil.h b/Gems/Atom/Tools/AtomToolsFramework/Code/Include/AtomToolsFramework/Util/MaterialPropertyUtil.h index d27f2403a1..961febf0f4 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Include/AtomToolsFramework/Util/MaterialPropertyUtil.h +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Include/AtomToolsFramework/Util/MaterialPropertyUtil.h @@ -53,14 +53,12 @@ namespace AtomToolsFramework const AZ::RPI::MaterialTypeSourceData::PropertyDefinition& propertyDefinition, AZ::RPI::MaterialPropertyValue& propertyValue); - //! Generate a file path from the exported file to the external reference. - //! This function returns a relative path from the export file to the reference file. - //! If the relative path is too different or distant from the export path then we return the asset folder relative path. + //! Generate a file path that is relative to either the source asset root or the export path //! @param exportPath absolute path of the file being saved //! @param referencePath absolute path of a file that will be treated as an external reference - //! @param maxPathDepth the maximum relative depth or number of parent or child folders between the export path and the reference path + //! @param relativeToExportPath specifies if the path is relative to the source asset root or the export path AZStd::string GetExteralReferencePath( - const AZStd::string& exportPath, const AZStd::string& referencePath, const uint32_t maxPathDepth = 2); + const AZStd::string& exportPath, const AZStd::string& referencePath, const bool relativeToExportPath = false); //! Traverse up the instance data node hierarchy to find the containing dynamic property object const AtomToolsFramework::DynamicProperty* FindDynamicPropertyForInstanceDataNode(const AzToolsFramework::InstanceDataNode* pNode); diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp index 3a392c1413..3b27c9cd0f 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.cpp @@ -24,6 +24,7 @@ AZ_PUSH_DISABLE_WARNING(4251 4800, "-Wunknown-warning-option") // disable warnin #include #include #include +#include AZ_POP_DISABLE_WARNING namespace AtomToolsFramework @@ -121,7 +122,6 @@ namespace AtomToolsFramework void AtomToolsDocumentSystemComponent::Deactivate() { - AZ::TickBus::Handler::BusDisconnect(); AtomToolsDocumentNotificationBus::Handler::BusDisconnect(); AtomToolsDocumentSystemRequestBus::Handler::BusDisconnect(); m_documentMap.clear(); @@ -160,25 +160,30 @@ namespace AtomToolsFramework void AtomToolsDocumentSystemComponent::OnDocumentExternallyModified(const AZ::Uuid& documentId) { m_documentIdsWithExternalChanges.insert(documentId); - if (!AZ::TickBus::Handler::BusIsConnected()) - { - AZ::TickBus::Handler::BusConnect(); - } + QueueReopenDocuments(); } void AtomToolsDocumentSystemComponent::OnDocumentDependencyModified(const AZ::Uuid& documentId) { m_documentIdsWithDependencyChanges.insert(documentId); - if (!AZ::TickBus::Handler::BusIsConnected()) + QueueReopenDocuments(); + } + + void AtomToolsDocumentSystemComponent::QueueReopenDocuments() + { + if (!m_queueReopenDocuments) { - AZ::TickBus::Handler::BusConnect(); + m_queueReopenDocuments = true; + QTimer::singleShot(0, [this] { ReopenDocuments(); }); } } - void AtomToolsDocumentSystemComponent::OnTick([[maybe_unused]] float deltaTime, [[maybe_unused]] AZ::ScriptTimePoint time) + void AtomToolsDocumentSystemComponent::ReopenDocuments() { for (const AZ::Uuid& documentId : m_documentIdsWithExternalChanges) { + m_documentIdsWithDependencyChanges.erase(documentId); + AZStd::string documentPath; AtomToolsDocumentRequestBus::EventResult(documentPath, documentId, &AtomToolsDocumentRequestBus::Events::GetAbsolutePath); @@ -191,8 +196,6 @@ namespace AtomToolsFramework continue; } - m_documentIdsWithDependencyChanges.erase(documentId); - AtomToolsFramework::TraceRecorder traceRecorder(m_maxMessageBoxLineCount); bool openResult = false; @@ -235,7 +238,7 @@ namespace AtomToolsFramework m_documentIdsWithDependencyChanges.clear(); m_documentIdsWithExternalChanges.clear(); - AZ::TickBus::Handler::BusDisconnect(); + m_queueReopenDocuments = false; } AZ::Uuid AtomToolsDocumentSystemComponent::OpenDocument(AZStd::string_view sourcePath) diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h index a0f5eb085d..532271974c 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Document/AtomToolsDocumentSystemComponent.h @@ -9,7 +9,6 @@ #pragma once #include -#include #include #include @@ -28,7 +27,6 @@ namespace AtomToolsFramework //! AtomToolsDocumentSystemComponent is the central component of the Material Editor Core gem class AtomToolsDocumentSystemComponent : public AZ::Component - , private AZ::TickBus::Handler , private AtomToolsDocumentNotificationBus::Handler , private AtomToolsDocumentSystemRequestBus::Handler { @@ -59,10 +57,8 @@ namespace AtomToolsFramework void OnDocumentExternallyModified(const AZ::Uuid& documentId) override; ////////////////////////////////////////////////////////////////////////// - //////////////////////////////////////////////////////////////////////// - // AZ::TickBus::Handler overrides... - void OnTick(float deltaTime, AZ::ScriptTimePoint time) override; - //////////////////////////////////////////////////////////////////////// + void QueueReopenDocuments(); + void ReopenDocuments(); //////////////////////////////////////////////////////////////////////// // AtomToolsDocumentSystemRequestBus::Handler overrides... @@ -87,6 +83,7 @@ namespace AtomToolsFramework AZStd::unordered_map> m_documentMap; AZStd::unordered_set m_documentIdsWithExternalChanges; AZStd::unordered_set m_documentIdsWithDependencyChanges; + bool m_queueReopenDocuments = false; const size_t m_maxMessageBoxLineCount = 15; }; } // namespace AtomToolsFramework diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/MaterialPropertyUtil.cpp b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/MaterialPropertyUtil.cpp index 3ffd8efa6e..ab72214881 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/MaterialPropertyUtil.cpp +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/MaterialPropertyUtil.cpp @@ -222,31 +222,15 @@ namespace AtomToolsFramework return true; } - AZStd::string GetExteralReferencePath(const AZStd::string& exportPath, const AZStd::string& referencePath, const uint32_t maxPathDepth) + AZStd::string GetExteralReferencePath( + const AZStd::string& exportPath, const AZStd::string& referencePath, const bool relativeToExportPath) { if (referencePath.empty()) { return {}; } - AZ::IO::BasicPath exportFolder(exportPath); - exportFolder.RemoveFilename(); - - const AZStd::string relativePath = AZ::IO::PathView(referencePath).LexicallyRelative(exportFolder).StringAsPosix(); - - // Count the difference in depth between the export file path and the referenced file path. - uint32_t parentFolderCount = 0; - AZStd::string::size_type pos = 0; - const AZStd::string parentFolderToken = ".."; - while ((pos = relativePath.find(parentFolderToken, pos)) != AZStd::string::npos) - { - parentFolderCount++; - pos += parentFolderToken.length(); - } - - // If the difference in depth is too great then revert to using the asset folder relative path. - // We could change this to only use relative paths for references in subfolders. - if (parentFolderCount > maxPathDepth) + if (!relativeToExportPath) { AZStd::string watchFolder; AZ::Data::AssetInfo assetInfo; @@ -260,7 +244,9 @@ namespace AtomToolsFramework } } - return relativePath; + AZ::IO::BasicPath exportFolder(exportPath); + exportFolder.RemoveFilename(); + return AZ::IO::PathView(referencePath).LexicallyRelative(exportFolder).StringAsPosix(); } const AtomToolsFramework::DynamicProperty* FindDynamicPropertyForInstanceDataNode(const AzToolsFramework::InstanceDataNode* pNode) diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/Util.cpp b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/Util.cpp index cd50c10e23..b45ff3c12f 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/Util.cpp +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Source/Util/Util.cpp @@ -62,6 +62,8 @@ namespace AtomToolsFramework const QFileInfo initialFileInfo(initialPath); const QString initialExt(initialFileInfo.completeSuffix()); + // Instead of just passing in the absolute file path, we pass in the absolute folder path and the base name to prevent the file + // dialog from displaying multiple extensions when the extension contains a "." const QFileInfo selectedFileInfo(AzQtComponents::FileDialog::GetSaveFileName( QApplication::activeWindow(), "Save File", @@ -82,7 +84,9 @@ namespace AtomToolsFramework return QFileInfo(); } - return selectedFileInfo; + // Reconstructing the file info from the absolute path and expected extension to compensate for an issue with the save file + // dialog adding the extension multiple times if it contains "." like *.lightingpreset.azasset + return QFileInfo(selectedFileInfo.absolutePath() + AZ_CORRECT_FILESYSTEM_SEPARATOR_STRING + selectedFileInfo.baseName() + "." + initialExt); } QFileInfo GetOpenFileInfo(const AZStd::vector& assetTypes) diff --git a/Gems/Atom/Tools/AtomToolsFramework/Code/Tests/AtomToolsFrameworkTest.cpp b/Gems/Atom/Tools/AtomToolsFramework/Code/Tests/AtomToolsFrameworkTest.cpp index 3124372bd2..bdd166192f 100644 --- a/Gems/Atom/Tools/AtomToolsFramework/Code/Tests/AtomToolsFrameworkTest.cpp +++ b/Gems/Atom/Tools/AtomToolsFramework/Code/Tests/AtomToolsFrameworkTest.cpp @@ -66,15 +66,15 @@ namespace UnitTest TEST_F(AtomToolsFrameworkTest, GetExteralReferencePath_Succeeds) { - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("", "", 2), ""); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/condor.material", "", 2), ""); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/talisman.material", "", 2), ""); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/talisman.material", "d:/project/assets/textures/gold.png", 2), "../textures/gold.png"); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/talisman.material", "d:/project/assets/textures/gold.png", 0), "textures/gold.png"); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", 3), "../../../materials/condor.material"); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", 2), "materials/condor.material"); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", 1), "materials/condor.material"); - ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", 0), "materials/condor.material"); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("", "", true), ""); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/condor.material", "", true), ""); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/talisman.material", "", false), ""); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/talisman.material", "d:/project/assets/textures/gold.png", true), "../textures/gold.png"); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/materials/talisman.material", "d:/project/assets/textures/gold.png", false), "textures/gold.png"); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", true), "../../../materials/condor.material"); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", false), "materials/condor.material"); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", false), "materials/condor.material"); + ASSERT_EQ(AtomToolsFramework::GetExteralReferencePath("d:/project/assets/objects/upgrades/materials/supercondor.material", "d:/project/assets/materials/condor.material", false), "materials/condor.material"); } AZ_UNIT_TEST_HOOK(new AtomToolsFrameworkTestEnvironment); diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 2d7768feec..97d8d5e354 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -555,7 +555,7 @@ namespace MaterialEditor void MaterialDocument::SourceFileChanged(AZStd::string relativePath, AZStd::string scanFolder, [[maybe_unused]] AZ::Uuid sourceUUID) { - auto sourcePath = AZ::RPI::AssetUtils::ResolvePathReference(scanFolder, relativePath); + const auto sourcePath = AZ::RPI::AssetUtils::ResolvePathReference(scanFolder, relativePath); if (m_absolutePath == sourcePath) { diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/CreateMaterialDialog/CreateMaterialDialog.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/CreateMaterialDialog/CreateMaterialDialog.cpp index fd20716570..31ca873c48 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/CreateMaterialDialog/CreateMaterialDialog.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/CreateMaterialDialog/CreateMaterialDialog.cpp @@ -6,25 +6,21 @@ * */ -#include - -#include -#include - -#include - -#include - +#include #include #include #include - -#include +#include +#include +#include +#include +#include +#include namespace MaterialEditor { CreateMaterialDialog::CreateMaterialDialog(QWidget* parent) - : CreateMaterialDialog(QString(AZ::IO::FileIOBase::GetInstance()->GetAlias("@projectroot@")) + AZ_CORRECT_FILESYSTEM_SEPARATOR + "Materials", parent) + : CreateMaterialDialog(QString(AZ::Utils::GetProjectPath().c_str()) + AZ_CORRECT_FILESYSTEM_SEPARATOR + "Assets", parent) { } diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialEditorBrowserInteractions.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialEditorBrowserInteractions.cpp index d73737a2dc..435d1f6f12 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialEditorBrowserInteractions.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialEditorBrowserInteractions.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -106,8 +107,8 @@ namespace MaterialEditor menu->addAction("Create Material...", [entry]() { const QString defaultPath = AtomToolsFramework::GetUniqueFileInfo( - QString(AZ::IO::FileIOBase::GetInstance()->GetAlias("@projectroot@")) + - AZ_CORRECT_FILESYSTEM_SEPARATOR + "Materials" + + QString(AZ::Utils::GetProjectPath().c_str()) + + AZ_CORRECT_FILESYSTEM_SEPARATOR + "Assets" + AZ_CORRECT_FILESYSTEM_SEPARATOR + "untitled." + AZ::RPI::MaterialSourceData::Extension).absoluteFilePath(); @@ -182,8 +183,8 @@ namespace MaterialEditor menu->addAction("Create Child Material...", [entry]() { const QString defaultPath = AtomToolsFramework::GetUniqueFileInfo( - QString(AZ::IO::FileIOBase::GetInstance()->GetAlias("@projectroot@")) + - AZ_CORRECT_FILESYSTEM_SEPARATOR + "Materials" + + QString(AZ::Utils::GetProjectPath().c_str()) + + AZ_CORRECT_FILESYSTEM_SEPARATOR + "Assets" + AZ_CORRECT_FILESYSTEM_SEPARATOR + "untitled." + AZ::RPI::MaterialSourceData::Extension).absoluteFilePath(); diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/ViewportSettingsInspector/ViewportSettingsInspector.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/ViewportSettingsInspector/ViewportSettingsInspector.cpp index 5721dfcc72..613762c10a 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/ViewportSettingsInspector/ViewportSettingsInspector.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/ViewportSettingsInspector/ViewportSettingsInspector.cpp @@ -6,10 +6,11 @@ * */ +#include #include #include #include -#include +#include #include #include #include @@ -346,13 +347,10 @@ namespace MaterialEditor AZStd::string ViewportSettingsInspector::GetDefaultUniqueSaveFilePath(const AZStd::string& baseName) const { - AZStd::string savePath = AZ::IO::FileIOBase::GetInstance()->GetAlias("@projectroot@"); - savePath += AZ_CORRECT_FILESYSTEM_SEPARATOR; - savePath += "Materials"; - savePath += AZ_CORRECT_FILESYSTEM_SEPARATOR; - savePath += baseName; - savePath = AtomToolsFramework::GetUniqueFileInfo(savePath.c_str()).absoluteFilePath().toUtf8().constData(); - return savePath; + return AtomToolsFramework::GetUniqueFileInfo( + QString(AZ::Utils::GetProjectPath().c_str()) + + AZ_CORRECT_FILESYSTEM_SEPARATOR + "Assets" + + AZ_CORRECT_FILESYSTEM_SEPARATOR + baseName.c_str()).absoluteFilePath().toUtf8().constData(); } AZ::Crc32 ViewportSettingsInspector::GetGroupSaveStateKey(const AZStd::string& groupName) const