From c7c96d8bafa63c80f43aeb16a1ad425e27b900be Mon Sep 17 00:00:00 2001 From: lsemp3d <58790905+lsemp3d@users.noreply.github.com> Date: Thu, 16 Dec 2021 17:20:37 -0800 Subject: [PATCH 1/3] Code maintenance, improved OnCatalogLoaded logic and track additions and removals Signed-off-by: lsemp3d <58790905+lsemp3d@users.noreply.github.com> --- Gems/GraphCanvas/Code/Source/GraphCanvas.cpp | 56 +++++++++++--------- Gems/GraphCanvas/Code/Source/GraphCanvas.h | 7 +-- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp b/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp index ebc15023cc..75306969f2 100644 --- a/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp +++ b/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp @@ -227,7 +227,6 @@ namespace GraphCanvas AzFramework::AssetCatalogEventBus::Handler::BusDisconnect(); Styling::PseudoElementFactoryRequestBus::Handler::BusDisconnect(); GraphCanvasRequestBus::Handler::BusDisconnect(); - AZ::Data::AssetBus::MultiHandler::BusDisconnect(); m_translationAssetWorker.Deactivate(); UnregisterAssetHandler(); @@ -369,26 +368,46 @@ namespace GraphCanvas void GraphCanvasSystemComponent::OnCatalogLoaded(const char* /*catalogFile*/) { - auto postEnumerateCb = [this]() + AZStd::vector translationAssets; + auto postEnumerateCb = [&translationAssets]() { - PopulateTranslationDatabase(); - }; - - // Find any TranslationAsset files that may have translation database key/values - AZ::Data::AssetCatalogRequests::AssetEnumerationCB collectAssetsCb = [this](const AZ::Data::AssetId assetId, const AZ::Data::AssetInfo& assetInfo) - { - if (AZ::StringFunc::EndsWith(assetInfo.m_relativePath, ".names", false)) + for (const AZ::Data::AssetId& assetId : translationAssets) { - m_translationAssets.push_back(assetId); + AZ::Data::AssetManager::Instance().GetAsset(assetId, AZ::Data::AssetLoadBehavior::Default); } }; - m_translationAssets.clear(); + // Find any TranslationAsset files that may have translation database key/values + AZ::Data::AssetCatalogRequests::AssetEnumerationCB collectAssetsCb = [&translationAssets](const AZ::Data::AssetId assetId, const AZ::Data::AssetInfo& assetInfo) + { + if (AZ::StringFunc::EndsWith(assetInfo.m_relativePath, ".names", false)) + { + translationAssets.push_back(assetId); + } + }; AZ::Data::AssetCatalogRequestBus::Broadcast(&AZ::Data::AssetCatalogRequestBus::Events::EnumerateAssets, nullptr, collectAssetsCb, postEnumerateCb); } + void GraphCanvasSystemComponent::OnCatalogAssetRemoved(const AZ::Data::AssetId& /*assetId*/, const AZ::Data::AssetInfo& assetInfo) + { + if (assetInfo.m_assetType == azrtti_typeid()) + { + GraphCanvas::TranslationRequestBus::Broadcast(&GraphCanvas::TranslationRequests::Restore); + } + } + + void GraphCanvasSystemComponent::OnCatalogAssetAdded(const AZ::Data::AssetId& assetId) + { + ReloadDatabase(assetId); + } + void GraphCanvasSystemComponent::OnCatalogAssetChanged(const AZ::Data::AssetId& assetId) + { + ReloadDatabase(assetId); + } + + void GraphCanvasSystemComponent::ReloadDatabase(const AZ::Data::AssetId& assetId) { AZ::Data::AssetInfo assetInfo; AZ::Data::AssetCatalogRequestBus::BroadcastResult(assetInfo, &AZ::Data::AssetCatalogRequestBus::Events::GetAssetInfoById, assetId); @@ -405,20 +424,5 @@ namespace GraphCanvas AZ::Data::AssetManager::Instance().UnregisterHandler(m_assetHandler.get()); m_assetHandler.reset(); } - - for (const AZ::Data::AssetId& assetId : m_translationAssets) - { - AZ::Data::AssetBus::MultiHandler::BusDisconnect(assetId); - } - m_translationAssets.clear(); - } - - void GraphCanvasSystemComponent::PopulateTranslationDatabase() - { - for (const AZ::Data::AssetId& assetId : m_translationAssets) - { - AZ::Data::AssetBus::MultiHandler::BusConnect(assetId); - AZ::Data::AssetManager::Instance().GetAsset(assetId, AZ::Data::AssetLoadBehavior::Default); - } } } diff --git a/Gems/GraphCanvas/Code/Source/GraphCanvas.h b/Gems/GraphCanvas/Code/Source/GraphCanvas.h index 0b47e9010a..37bf064203 100644 --- a/Gems/GraphCanvas/Code/Source/GraphCanvas.h +++ b/Gems/GraphCanvas/Code/Source/GraphCanvas.h @@ -25,7 +25,6 @@ namespace GraphCanvas , private GraphCanvasRequestBus::Handler , protected Styling::PseudoElementFactoryRequestBus::Handler , protected AzFramework::AssetCatalogEventBus::Handler - , protected AZ::Data::AssetBus::MultiHandler { public: @@ -80,15 +79,17 @@ namespace GraphCanvas // AzFramework::AssetCatalogEventBus::Handler void OnCatalogLoaded(const char* /*catalogFile*/) override; void OnCatalogAssetChanged(const AZ::Data::AssetId&) override; + void OnCatalogAssetAdded(const AZ::Data::AssetId&) override; + void OnCatalogAssetRemoved(const AZ::Data::AssetId& /*assetId*/, const AZ::Data::AssetInfo& /*assetInfo*/) override; //// + void ReloadDatabase(const AZ::Data::AssetId&); + AZStd::unique_ptr m_assetHandler; void RegisterTranslationBuilder(); void UnregisterAssetHandler(); TranslationAssetWorker m_translationAssetWorker; - AZStd::vector m_translationAssets; - void PopulateTranslationDatabase(); TranslationDatabase m_translationDatabase; }; From 20d55c62670ce0a6986bd93bc8f2fa2e7616e477 Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Fri, 17 Dec 2021 16:31:39 -0600 Subject: [PATCH 2/3] Fixed the translation error messages for GraphCanvas It was unconditionally logging an AZ_Warning and AZ_Error if a translation entry with the same key was added to the database. This didn't account for the same key and value being added to the database, so loading the same translation file twice would result in error messages for each entry. Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> --- .../Code/Source/Translation/TranslationDatabase.cpp | 8 +++++--- .../Code/Source/Translation/TranslationSerializer.cpp | 7 ++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Gems/GraphCanvas/Code/Source/Translation/TranslationDatabase.cpp b/Gems/GraphCanvas/Code/Source/Translation/TranslationDatabase.cpp index 0b5029de63..448f69adfa 100644 --- a/Gems/GraphCanvas/Code/Source/Translation/TranslationDatabase.cpp +++ b/Gems/GraphCanvas/Code/Source/Translation/TranslationDatabase.cpp @@ -163,9 +163,11 @@ namespace GraphCanvas } else { - AZStd::string warning = AZStd::string::format("Unable to store key: %s with value: %s because that key already exists with value: %s", entry.first.c_str(), entry.second.c_str(), m_database[entry.first].c_str()); - AZ_Warning("TranslationSerializer", false, warning.c_str()); - warnings = true; + const bool valueMatches = entry.second == m_database[entry.first]; + AZ_Warning("TranslationDatabase", valueMatches, + R"(Unable to store key: "%s" with value: "%s" because that key already exists with value: "%s")", + entry.first.c_str(), entry.second.c_str(), m_database[entry.first].c_str()); + warnings = !valueMatches; } } diff --git a/Gems/GraphCanvas/Code/Source/Translation/TranslationSerializer.cpp b/Gems/GraphCanvas/Code/Source/Translation/TranslationSerializer.cpp index 754267b27d..72cbfbb9e8 100644 --- a/Gems/GraphCanvas/Code/Source/Translation/TranslationSerializer.cpp +++ b/Gems/GraphCanvas/Code/Source/Translation/TranslationSerializer.cpp @@ -24,11 +24,12 @@ namespace GraphCanvas } else { - const AZStd::string& existingValue = translationDbItr->second; + [[maybe_unused]] const AZStd::string& existingValue = translationDbItr->second; // There is a name collision - const AZStd::string error = AZStd::string::format("Unable to store key: %s with value: %s because that key already exists with value: %s (proposed: %s)", baseKey.c_str(), it.GetString(), existingValue.c_str(), it.GetString()); - AZ_Error("TranslationSerializer", false, error.c_str()); + AZ_Error("TranslationSerializer", existingValue == it.GetString(), + R"(Unable to store key: "%s" with value: "%s" because that key already exists with value: "%s" (proposed: "%s"))", + baseKey.c_str(), it.GetString(), existingValue.c_str(), it.GetString()); } } else if (it.IsObject()) From 86d22c8bb38ead46a237bcee497ed8e528eb751b Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Fri, 17 Dec 2021 18:26:36 -0600 Subject: [PATCH 3/3] Updated GraphCanvasSystemComponent::OnCatalogLoaded to use TranslationRequests::Restore The TranslationDatabase::Restore function will clear out the database before merging in all the translation .names files making it idempotent. Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> --- Gems/GraphCanvas/Code/Source/GraphCanvas.cpp | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp b/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp index 75306969f2..de7966e53c 100644 --- a/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp +++ b/Gems/GraphCanvas/Code/Source/GraphCanvas.cpp @@ -368,25 +368,7 @@ namespace GraphCanvas void GraphCanvasSystemComponent::OnCatalogLoaded(const char* /*catalogFile*/) { - AZStd::vector translationAssets; - auto postEnumerateCb = [&translationAssets]() - { - for (const AZ::Data::AssetId& assetId : translationAssets) - { - AZ::Data::AssetManager::Instance().GetAsset(assetId, AZ::Data::AssetLoadBehavior::Default); - } - }; - - // Find any TranslationAsset files that may have translation database key/values - AZ::Data::AssetCatalogRequests::AssetEnumerationCB collectAssetsCb = [&translationAssets](const AZ::Data::AssetId assetId, const AZ::Data::AssetInfo& assetInfo) - { - if (AZ::StringFunc::EndsWith(assetInfo.m_relativePath, ".names", false)) - { - translationAssets.push_back(assetId); - } - }; - - AZ::Data::AssetCatalogRequestBus::Broadcast(&AZ::Data::AssetCatalogRequestBus::Events::EnumerateAssets, nullptr, collectAssetsCb, postEnumerateCb); + GraphCanvas::TranslationRequestBus::Broadcast(&GraphCanvas::TranslationRequests::Restore); } void GraphCanvasSystemComponent::OnCatalogAssetRemoved(const AZ::Data::AssetId& /*assetId*/, const AZ::Data::AssetInfo& assetInfo)