From c07a7c3766061870624856998aa0f321c07a286f Mon Sep 17 00:00:00 2001 From: daimini Date: Tue, 27 Apr 2021 17:20:22 -0700 Subject: [PATCH 1/5] Detect and block instantiations that would generate circular dependencies in the instance hierarchy. --- .../Prefab/PrefabPublicHandler.cpp | 33 +++++++++++++++++-- .../Prefab/PrefabPublicHandler.h | 8 +++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 1e9cc35230..fdcef9bd4b 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -184,6 +184,16 @@ namespace AzToolsFramework instanceToParentUnder = prefabEditorEntityOwnershipInterface->GetRootPrefabInstance(); parent = instanceToParentUnder->get().GetContainerEntityId(); } + + //Detect whether this instantiation would produce a cyclical dependency + 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())) + { + return AZ::Failure(AZStd::string("Instantiate Prefab operation aborted - Instantiation would have introduced a cyclical dependency.")); + } { // Initialize Undo Batch object @@ -194,7 +204,7 @@ namespace AzToolsFramework instanceToParentUnderDomBeforeCreate, instanceToParentUnder->get()); // Instantiate the Prefab - auto instanceToCreate = prefabEditorEntityOwnershipInterface->InstantiatePrefab(filePath, instanceToParentUnder); + auto instanceToCreate = prefabEditorEntityOwnershipInterface->InstantiatePrefab(relativePath, instanceToParentUnder); if (!instanceToCreate) { @@ -242,13 +252,30 @@ namespace AzToolsFramework { AZ_Assert( false, - "Failed to create prefab : Couldn't get a valid owning instance for the common root entity of the enities provided"); + "Failed to create prefab : Couldn't get a valid owning instance for the common root entity of the entities provided"); return AZ::Failure(AZStd::string( - "Failed to create prefab : Couldn't get a valid owning instance for the common root entity of the enities provided")); + "Failed to create prefab : Couldn't get a valid owning instance for the common root entity of the entities provided")); } return AZ::Success(); } + bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, const Instance& instance) + { + const Instance* currentInstance = &instance; + + while (currentInstance != nullptr) + { + if (currentInstance->GetTemplateId() == prefabTemplateId) + { + return true; + } + + currentInstance = ¤tInstance->GetParentInstance()->get(); + } + + return false; + } + void PrefabPublicHandler::CreateLink( const EntityList& topLevelEntities, Instance& sourceInstance, TemplateId targetTemplateId, UndoSystem::URSequencePoint* undoBatch, AZ::EntityId commonRootEntityId) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index e83513dbff..351ffcb71d 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -96,6 +96,14 @@ 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. + * + * \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. + */ + bool IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, const Instance& instance); + static Instance* GetParentInstance(Instance* instance); static Instance* GetAncestorOfInstanceThatIsChildOfRoot(const Instance* ancestor, Instance* descendant); static void GenerateContainerEntityTransform(const EntityList& topLevelEntities, AZ::Vector3& translation, AZ::Quaternion& rotation); From b758a1553f36aa6c84174bc5ee77106086de5f90 Mon Sep 17 00:00:00 2001 From: daimini Date: Tue, 27 Apr 2021 17:21:33 -0700 Subject: [PATCH 2/5] GetTemplateIdFromFilePath now asserts if it's passed an absolute path. This is helpful for debugging, since the function would silently fail even if the file was actually loaded. --- .../AzToolsFramework/Prefab/PrefabSystemComponent.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp index 40c8b3bc6a..e838497548 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp @@ -720,6 +720,8 @@ namespace AzToolsFramework TemplateId PrefabSystemComponent::GetTemplateIdFromFilePath(AZ::IO::PathView filePath) const { + AZ_Assert(!filePath.IsAbsolute(), "Prefab - GetTemplateIdFromFilePath was passed an absolute path. Prefabs use paths relative to the project folder."); + auto found = m_templateFilePathToIdMap.find(filePath); if (found != m_templateFilePathToIdMap.end()) { From bc3f2856013236716ee77793028099eee96499a1 Mon Sep 17 00:00:00 2001 From: daimini Date: Thu, 29 Apr 2021 18:45:42 -0700 Subject: [PATCH 3/5] Refactored IsPrefabInInstanceAncestorHierarchy to use Instance Optional References. Added more information to the error message. --- .../Prefab/PrefabPublicHandler.cpp | 18 +++++++++++------- .../Prefab/PrefabPublicHandler.h | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index fdcef9bd4b..f62a045640 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -192,7 +192,13 @@ namespace AzToolsFramework // 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())) { - return AZ::Failure(AZStd::string("Instantiate Prefab operation aborted - Instantiation would have introduced a cyclical dependency.")); + 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() + ) + ); } { @@ -259,18 +265,16 @@ namespace AzToolsFramework return AZ::Success(); } - bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, const Instance& instance) + bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalReference instance) { - const Instance* currentInstance = &instance; - - while (currentInstance != nullptr) + while (instance.has_value()) { - if (currentInstance->GetTemplateId() == prefabTemplateId) + if (instance->get().GetTemplateId() == prefabTemplateId) { return true; } - currentInstance = ¤tInstance->GetParentInstance()->get(); + instance = instance->get().GetParentInstance(); } return false; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index 351ffcb71d..aef416cdd6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -102,7 +102,7 @@ namespace AzToolsFramework * \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. */ - bool IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, const Instance& instance); + bool IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalReference instance); static Instance* GetParentInstance(Instance* instance); static Instance* GetAncestorOfInstanceThatIsChildOfRoot(const Instance* ancestor, Instance* descendant); From 4662290d92315721e7e3dc4bfe28c998f0e5ff7e Mon Sep 17 00:00:00 2001 From: daimini Date: Fri, 7 May 2021 16:50:41 -0700 Subject: [PATCH 4/5] Switching IsPrefabInInstanceAncestorHierarchy to use const refs --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 2 +- .../AzToolsFramework/Prefab/PrefabPublicHandler.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 2ecde20861..29c7f1c554 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -258,7 +258,7 @@ namespace AzToolsFramework return AZ::Success(); } - bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalReference instance) + bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalConstReference instance) { while (instance.has_value()) { diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index 421c7c0052..03b3827328 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -112,7 +112,7 @@ namespace AzToolsFramework * \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. */ - bool IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalReference instance); + bool IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalConstReference instance); static Instance* GetParentInstance(Instance* instance); static Instance* GetAncestorOfInstanceThatIsChildOfRoot(const Instance* ancestor, Instance* descendant); From e24827efb3d053ece7e7d277ef3e3b2c1530d5a3 Mon Sep 17 00:00:00 2001 From: daimini Date: Fri, 7 May 2021 17:08:35 -0700 Subject: [PATCH 5/5] Fix direct editing of a reference --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 29c7f1c554..d4485f8e99 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -260,14 +260,16 @@ namespace AzToolsFramework bool PrefabPublicHandler::IsPrefabInInstanceAncestorHierarchy(TemplateId prefabTemplateId, InstanceOptionalConstReference instance) { - while (instance.has_value()) + InstanceOptionalConstReference currentInstance = instance; + + while (currentInstance.has_value()) { - if (instance->get().GetTemplateId() == prefabTemplateId) + if (currentInstance->get().GetTemplateId() == prefabTemplateId) { return true; } - instance = instance->get().GetParentInstance(); + currentInstance = currentInstance->get().GetParentInstance(); } return false;