From 7bdf44a0996b93bea061c5146e5627e7abbb8b21 Mon Sep 17 00:00:00 2001 From: srikappa Date: Wed, 12 May 2021 13:38:44 -0700 Subject: [PATCH 1/2] Detect cyclical dependencies in the nested prefabs of the prefab being instantiated --- .../Prefab/PrefabDomUtils.cpp | 37 ++++++++++++++++ .../AzToolsFramework/Prefab/PrefabDomUtils.h | 14 +++++++ .../Prefab/PrefabPublicHandler.cpp | 42 ++++++++++--------- .../Prefab/PrefabPublicHandler.h | 18 ++++---- 4 files changed, 84 insertions(+), 27 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.cpp index 0bffd26be0..145a293ab8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.cpp @@ -199,6 +199,43 @@ namespace AzToolsFramework return true; } + void GetTemplateSourcePaths(const PrefabDomValue& prefabDom, AZStd::unordered_set& templateSourcePaths) + { + PrefabDomValueConstReference findSourceResult = PrefabDomUtils::FindPrefabDomValue(prefabDom, PrefabDomUtils::SourceName); + if (!findSourceResult.has_value() || !(findSourceResult->get().IsString()) || + findSourceResult->get().GetStringLength() == 0) + { + AZ_Assert( + false, + "PrefabDomUtils::GetDependentTemplatePath - Source value of prefab in the provided DOM is not a valid string."); + return; + } + + templateSourcePaths.emplace(findSourceResult->get().GetString()); + PrefabDomValueConstReference instancesReference = GetInstancesValue(prefabDom); + if (instancesReference.has_value()) + { + const PrefabDomValue& instances = instancesReference->get(); + + for (PrefabDomValue::ConstMemberIterator instanceIterator = instances.MemberBegin(); + instanceIterator != instances.MemberEnd(); ++instanceIterator) + { + GetTemplateSourcePaths(instanceIterator->value, templateSourcePaths); + } + } + } + + PrefabDomValueConstReference GetInstancesValue(const PrefabDomValue& prefabDom) + { + PrefabDomValueConstReference findInstancesResult = FindPrefabDomValue(prefabDom, PrefabDomUtils::InstancesName); + if (!findInstancesResult.has_value() || !(findInstancesResult->get().IsObject())) + { + return AZStd::nullopt; + } + + return findInstancesResult->get(); + } + void PrintPrefabDomValue( [[maybe_unused]] const AZStd::string_view printMessage, [[maybe_unused]] const PrefabDomValue& prefabDomValue) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h index c7c2827770..6f7d2fe9b1 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h @@ -100,6 +100,20 @@ namespace AzToolsFramework .Append(instanceName); }; + /** + * Gets a set of all the template source paths in the given dom. + * @param prefabDom The DOM to get the template source paths from. + * @param templateSourcePaths The set of template source paths to populate. + */ + void GetTemplateSourcePaths(const PrefabDomValue& prefabDom, AZStd::unordered_set& templateSourcePaths); + + /** + * Gets the instances DOM value from the given prefab DOM. + * + * @return the instances DOM value or AZStd::nullopt if it instances can't be found. + */ + PrefabDomValueConstReference GetInstancesValue(const PrefabDomValue& prefabDom); + /** * Prints the contents of the given prefab DOM value to the debug output console in a readable format. * @param printMessage The message that will be printed before printing the PrefabDomValue diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 14537683ef..e2c616d5d8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -191,25 +191,30 @@ namespace AzToolsFramework auto relativePath = m_prefabLoaderInterface->GetRelativePathToProject(filePath); Prefab::TemplateId templateId = m_prefabSystemComponentInterface->GetTemplateIdFromFilePath(relativePath); - // If the template isn't currently loaded, there's no way for it to be in the hierarchy so we just skip the check. - if (templateId != Prefab::InvalidTemplateId && IsPrefabInInstanceAncestorHierarchy(templateId, instanceToParentUnder->get())) + if (templateId == InvalidTemplateId) { - return AZ::Failure( - AZStd::string::format( - "Instantiate Prefab operation aborted - Cyclical dependency detected\n(%s depends on %s).", - relativePath.Native().c_str(), - instanceToParentUnder->get().GetTemplateSourcePath().Native().c_str() - ) - ); + // Load the template from the file + templateId = m_prefabLoaderInterface->LoadTemplateFromFile(filePath); + AZ_Assert(templateId != InvalidTemplateId, "Template with source path %s couldn't be loaded correctly.", filePath); } - + + const PrefabDom& templateDom = m_prefabSystemComponentInterface->FindTemplateDom(templateId); + AZStd::unordered_set templatePaths; + PrefabDomUtils::GetTemplateSourcePaths(templateDom, templatePaths); + + if (IsCyclicalDependencyFound(instanceToParentUnder->get(), templatePaths)) + { + return AZ::Failure(AZStd::string::format( + "Instantiate Prefab operation aborted - Cyclical dependency detected\n(%s depends on %s).", + relativePath.Native().c_str(), instanceToParentUnder->get().GetTemplateSourcePath().Native().c_str())); + } + { // Initialize Undo Batch object ScopedUndoBatch undoBatch("Instantiate Prefab"); PrefabDom instanceToParentUnderDomBeforeCreate; - m_instanceToTemplateInterface->GenerateDomForInstance( - instanceToParentUnderDomBeforeCreate, instanceToParentUnder->get()); + m_instanceToTemplateInterface->GenerateDomForInstance(instanceToParentUnderDomBeforeCreate, instanceToParentUnder->get()); // Instantiate the Prefab auto instanceToCreate = prefabEditorEntityOwnershipInterface->InstantiatePrefab(relativePath, instanceToParentUnder); @@ -223,8 +228,7 @@ namespace AzToolsFramework PrefabUndoHelpers::UpdatePrefabInstance( instanceToParentUnder->get(), "Update prefab instance", instanceToParentUnderDomBeforeCreate, undoBatch.GetUndoBatch()); - CreateLink({}, instanceToCreate->get(), instanceToParentUnder->get().GetTemplateId(), - undoBatch.GetUndoBatch(), parent); + CreateLink({}, instanceToCreate->get(), instanceToParentUnder->get().GetTemplateId(), undoBatch.GetUndoBatch(), parent); AZ::EntityId containerEntityId = instanceToCreate->get().GetContainerEntityId(); // Apply position @@ -277,17 +281,17 @@ namespace AzToolsFramework return AZ::Success(); } - bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalConstReference instance) + bool PrefabPublicHandler::IsCyclicalDependencyFound( + InstanceOptionalConstReference instance, AZStd::unordered_set& templateSourcePaths) { InstanceOptionalConstReference currentInstance = instance; while (currentInstance.has_value()) { - if (currentInstance->get().GetTemplateId() == prefabTemplateId) + if (templateSourcePaths.contains(currentInstance->get().GetTemplateSourcePath())) { return true; } - currentInstance = currentInstance->get().GetParentInstance(); } @@ -966,5 +970,5 @@ namespace AzToolsFramework return true; } - } -} + } // namespace Prefab +} // namespace AzToolsFramework diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index 03b3827328..519c7ca53f 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -12,8 +12,8 @@ #pragma once -#include #include +#include #include #include @@ -106,13 +106,15 @@ namespace AzToolsFramework const AZStd::vector& entityIds, EntityList& inputEntityList, EntityList& topLevelEntities, AZ::EntityId& commonRootEntityId, InstanceOptionalReference& commonRootEntityOwningInstance); - /* Detects whether an instance of prefabTemplateId is present in the hierarchy of ancestors of instance. + /* Checks whether the template source path of any of the ancestors in the instance hierarchy matches with one of the + * paths provided in a set. * - * \param prefabTemplateId The template id to test for - * \param instance The instance whose ancestor hierarchy prefabTemplateId will be tested against. - * \return true if an instance of the template of id prefabTemplateId could be found in the ancestor hierarchy of instance, false otherwise. + * \param instance The instance whose ancestor hierarchy the provided set of template source paths will be tested against. + * \param templateSourcePaths The template source paths provided to be checked against the instance ancestor hierarchy. + * \return true if any of the template source paths could be found in the ancestor hierarchy of instance, false otherwise. */ - bool IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalConstReference instance); + bool IsCyclicalDependencyFound( + InstanceOptionalConstReference instance, AZStd::unordered_set& templateSourcePaths); static Instance* GetParentInstance(Instance* instance); static Instance* GetAncestorOfInstanceThatIsChildOfRoot(const Instance* ancestor, Instance* descendant); @@ -128,5 +130,5 @@ namespace AzToolsFramework uint64_t m_newEntityCounter = 1; }; - } -} + } // namespace Prefab +} // namespace AzToolsFramework From 1a456cc9b9545455c2ee04316b46b8f0478f34ca Mon Sep 17 00:00:00 2001 From: srikappa Date: Fri, 14 May 2021 11:48:31 -0700 Subject: [PATCH 2/2] Marked an our parameter correctly and changed a ref to const ref --- .../AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h | 2 +- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 2 +- .../AzToolsFramework/Prefab/PrefabPublicHandler.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h index 6f7d2fe9b1..5ee91c85ae 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h @@ -103,7 +103,7 @@ namespace AzToolsFramework /** * Gets a set of all the template source paths in the given dom. * @param prefabDom The DOM to get the template source paths from. - * @param templateSourcePaths The set of template source paths to populate. + * @param[out] templateSourcePaths The set of template source paths to populate. */ void GetTemplateSourcePaths(const PrefabDomValue& prefabDom, AZStd::unordered_set& templateSourcePaths); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index e2c616d5d8..c4fe97bf11 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -282,7 +282,7 @@ namespace AzToolsFramework } bool PrefabPublicHandler::IsCyclicalDependencyFound( - InstanceOptionalConstReference instance, AZStd::unordered_set& templateSourcePaths) + InstanceOptionalConstReference instance, const AZStd::unordered_set& templateSourcePaths) { InstanceOptionalConstReference currentInstance = instance; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index 519c7ca53f..fb3c3462d8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -114,7 +114,7 @@ namespace AzToolsFramework * \return true if any of the template source paths could be found in the ancestor hierarchy of instance, false otherwise. */ bool IsCyclicalDependencyFound( - InstanceOptionalConstReference instance, AZStd::unordered_set& templateSourcePaths); + InstanceOptionalConstReference instance, const AZStd::unordered_set& templateSourcePaths); static Instance* GetParentInstance(Instance* instance); static Instance* GetAncestorOfInstanceThatIsChildOfRoot(const Instance* ancestor, Instance* descendant);