From 76e8a0ef4763a712b6a8cc5ba0a96a90651efeec Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 14:11:47 -0700 Subject: [PATCH 1/8] Fixed bug with delete prefab where the prefabs were not being deleted from the correct instances --- .../Prefab/PrefabPublicHandler.cpp | 32 ++++++++++++------- .../Prefab/PrefabPublicInterface.h | 6 ++-- .../UI/Prefab/PrefabIntegrationManager.cpp | 9 ++++-- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 9649818ed9..d1a3e189b1 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -636,15 +636,23 @@ namespace AzToolsFramework if (!EntitiesBelongToSameInstance(entityIds)) { - return AZ::Failure(AZStd::string("DeleteEntitiesAndAllDescendantsInInstance - Deletion Error. Cannot delete multiple " - "entities belonging to different instances with one operation.")); + return AZ::Failure(AZStd::string("Cannot delete multiple entities belonging to different instances with one operation.")); } - InstanceOptionalReference instance = GetOwnerInstanceByEntityId(entityIds[0]); - // Retrieve entityList from entityIds EntityList inputEntityList = EntityIdListToEntityList(entityIds); + EntityList topLevelEntities; + AZ::EntityId commonRootEntityId; + InstanceOptionalReference commonRootEntityOwningInstance; + PrefabOperationResult findCommonRootOutcome = FindCommonRootOwningInstance( + entityIds, inputEntityList, topLevelEntities, commonRootEntityId, commonRootEntityOwningInstance); + + if (!findCommonRootOutcome.IsSuccess()) + { + return findCommonRootOutcome; + } + AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzToolsFramework); UndoSystem::URSequencePoint* currentUndoBatch = nullptr; @@ -680,14 +688,15 @@ namespace AzToolsFramework AZ_PROFILE_SCOPE(AZ::Debug::ProfileCategory::AzToolsFramework, "Internal::DeleteEntities:UndoCaptureAndPurgeEntities"); Prefab::PrefabDom instanceDomBefore; - m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBefore, instance->get()); + m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBefore, commonRootEntityOwningInstance->get()); if (deleteDescendants) { AZStd::vector entities; AZStd::vector> instances; - bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, instance->get(), entities, instances); + bool success = RetrieveAndSortPrefabEntitiesAndInstances( + inputEntityList, commonRootEntityOwningInstance->get(), entities, instances); if (!success) { @@ -712,22 +721,23 @@ namespace AzToolsFramework // If this is the container entity, it actually represents the instance so get its owner if (owningInstance->get().GetContainerEntityId() == entityId) { - auto instancePtr = instance->get().DetachNestedInstance(owningInstance->get().GetInstanceAlias()); + auto instancePtr = + commonRootEntityOwningInstance->get().DetachNestedInstance(owningInstance->get().GetInstanceAlias()); instancePtr.reset(); } else { - instance->get().DetachEntity(entityId); + commonRootEntityOwningInstance->get().DetachEntity(entityId); AZ::ComponentApplicationBus::Broadcast(&AZ::ComponentApplicationRequests::DeleteEntity, entityId); } } } Prefab::PrefabDom instanceDomAfter; - m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomAfter, instance->get()); - + m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomAfter, commonRootEntityOwningInstance->get()); + PrefabUndoInstance* command = aznew PrefabUndoInstance("Instance deletion"); - command->Capture(instanceDomBefore, instanceDomAfter, instance->get().GetTemplateId()); + command->Capture(instanceDomBefore, instanceDomAfter, commonRootEntityOwningInstance->get().GetTemplateId()); command->SetParent(selCommand); } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h index 1a8da0dfe0..1abd183ce8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h @@ -28,9 +28,9 @@ namespace AzToolsFramework namespace Prefab { - typedef AZ::Outcome PrefabOperationResult; - typedef AZ::Outcome PrefabRequestResult; - typedef AZ::Outcome PrefabEntityResult; + using PrefabOperationResult = AZ::Outcome; + using PrefabRequestResult = AZ::Outcome; + using PrefabEntityResult = AZ::Outcome; /*! * PrefabPublicInterface diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Prefab/PrefabIntegrationManager.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Prefab/PrefabIntegrationManager.cpp index 7f28080e9e..bc7afbf085 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Prefab/PrefabIntegrationManager.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Prefab/PrefabIntegrationManager.cpp @@ -403,9 +403,12 @@ namespace AzToolsFramework AzToolsFramework::EntityIdList selectedEntityIds; AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult( selectedEntityIds, &AzToolsFramework::ToolsApplicationRequests::GetSelectedEntities); - - AzToolsFramework::ToolsApplicationRequestBus::Broadcast( - &AzToolsFramework::ToolsApplicationRequests::DeleteEntitiesAndAllDescendants, selectedEntityIds); + PrefabOperationResult deleteSelectedResult = + s_prefabPublicInterface->DeleteEntitiesAndAllDescendantsInInstance(selectedEntityIds); + if (!deleteSelectedResult.IsSuccess()) + { + WarnUserOfError("Delete selected entities error", deleteSelectedResult.GetError()); + } } void PrefabIntegrationManager::GenerateSuggestedFilenameFromEntities(const EntityIdList& entityIds, AZStd::string& outName) From cd4be3a708b6645fbcc6cc2f711e6cd9ae851048 Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 14:37:22 -0700 Subject: [PATCH 2/8] Found a better way to find common owning instance --- .../Prefab/PrefabPublicHandler.cpp | 35 +++++++++---------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index d1a3e189b1..eec0128419 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -639,20 +639,19 @@ namespace AzToolsFramework return AZ::Failure(AZStd::string("Cannot delete multiple entities belonging to different instances with one operation.")); } + AZ::EntityId firstEntityIdToDelete = entityIds[0]; + InstanceOptionalReference commonOwningInstance = GetOwnerInstanceByEntityId(firstEntityIdToDelete); + + // If the first entity id is that of an instance, we need to delete that instance from it's parent. + if (commonOwningInstance->get().GetContainerEntityId() == firstEntityIdToDelete && + !IsLevelInstanceContainerEntity(firstEntityIdToDelete)) + { + commonOwningInstance = commonOwningInstance->get().GetParentInstance(); + } + // Retrieve entityList from entityIds EntityList inputEntityList = EntityIdListToEntityList(entityIds); - EntityList topLevelEntities; - AZ::EntityId commonRootEntityId; - InstanceOptionalReference commonRootEntityOwningInstance; - PrefabOperationResult findCommonRootOutcome = FindCommonRootOwningInstance( - entityIds, inputEntityList, topLevelEntities, commonRootEntityId, commonRootEntityOwningInstance); - - if (!findCommonRootOutcome.IsSuccess()) - { - return findCommonRootOutcome; - } - AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzToolsFramework); UndoSystem::URSequencePoint* currentUndoBatch = nullptr; @@ -688,15 +687,14 @@ namespace AzToolsFramework AZ_PROFILE_SCOPE(AZ::Debug::ProfileCategory::AzToolsFramework, "Internal::DeleteEntities:UndoCaptureAndPurgeEntities"); Prefab::PrefabDom instanceDomBefore; - m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBefore, commonRootEntityOwningInstance->get()); + m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBefore, commonOwningInstance->get()); if (deleteDescendants) { AZStd::vector entities; AZStd::vector> instances; - bool success = RetrieveAndSortPrefabEntitiesAndInstances( - inputEntityList, commonRootEntityOwningInstance->get(), entities, instances); + bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonOwningInstance->get(), entities, instances); if (!success) { @@ -721,23 +719,22 @@ namespace AzToolsFramework // If this is the container entity, it actually represents the instance so get its owner if (owningInstance->get().GetContainerEntityId() == entityId) { - auto instancePtr = - commonRootEntityOwningInstance->get().DetachNestedInstance(owningInstance->get().GetInstanceAlias()); + auto instancePtr = commonOwningInstance->get().DetachNestedInstance(owningInstance->get().GetInstanceAlias()); instancePtr.reset(); } else { - commonRootEntityOwningInstance->get().DetachEntity(entityId); + commonOwningInstance->get().DetachEntity(entityId); AZ::ComponentApplicationBus::Broadcast(&AZ::ComponentApplicationRequests::DeleteEntity, entityId); } } } Prefab::PrefabDom instanceDomAfter; - m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomAfter, commonRootEntityOwningInstance->get()); + m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomAfter, commonOwningInstance->get()); PrefabUndoInstance* command = aznew PrefabUndoInstance("Instance deletion"); - command->Capture(instanceDomBefore, instanceDomAfter, commonRootEntityOwningInstance->get().GetTemplateId()); + command->Capture(instanceDomBefore, instanceDomAfter, commonOwningInstance->get().GetTemplateId()); command->SetParent(selCommand); } From b92a68f5a43d09ee1446b1277231adf4c7cb346d Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 14:45:06 -0700 Subject: [PATCH 3/8] Improved a comment --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 3 ++- .../AzToolsFramework/Prefab/PrefabPublicInterface.h | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index eec0128419..bbc964d9a4 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -642,7 +642,8 @@ namespace AzToolsFramework AZ::EntityId firstEntityIdToDelete = entityIds[0]; InstanceOptionalReference commonOwningInstance = GetOwnerInstanceByEntityId(firstEntityIdToDelete); - // If the first entity id is that of an instance, we need to delete that instance from it's parent. + // If the first entity id is a container entity id, then we need to mark its parent as the common owning instance because you + // cannot detete an instance from itself. if (commonOwningInstance->get().GetContainerEntityId() == firstEntityIdToDelete && !IsLevelInstanceContainerEntity(firstEntityIdToDelete)) { diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h index 1abd183ce8..72bc5df3f6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h @@ -28,9 +28,9 @@ namespace AzToolsFramework namespace Prefab { - using PrefabOperationResult = AZ::Outcome; - using PrefabRequestResult = AZ::Outcome; - using PrefabEntityResult = AZ::Outcome; + typedef AZ::Outcome PrefabOperationResult; + typedef AZ::Outcome PrefabOperationResult; + typedef AZ::Outcome PrefabEntityResult; /*! * PrefabPublicInterface From 7d8180a3bbc325bd2b99d055b400cd81f95d7f13 Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 14:46:30 -0700 Subject: [PATCH 4/8] Fixed a typo --- .../AzToolsFramework/Prefab/PrefabPublicInterface.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h index 72bc5df3f6..1a8da0dfe0 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h @@ -29,7 +29,7 @@ namespace AzToolsFramework namespace Prefab { typedef AZ::Outcome PrefabOperationResult; - typedef AZ::Outcome PrefabOperationResult; + typedef AZ::Outcome PrefabRequestResult; typedef AZ::Outcome PrefabEntityResult; /*! From 9328c944b25c62e7d01d51ff8cf905f81f56d295 Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 14:49:06 -0700 Subject: [PATCH 5/8] Remove an unnecessary check in the if statement --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index bbc964d9a4..3ac3da11f0 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -644,8 +644,7 @@ namespace AzToolsFramework // If the first entity id is a container entity id, then we need to mark its parent as the common owning instance because you // cannot detete an instance from itself. - if (commonOwningInstance->get().GetContainerEntityId() == firstEntityIdToDelete && - !IsLevelInstanceContainerEntity(firstEntityIdToDelete)) + if (commonOwningInstance->get().GetContainerEntityId() == firstEntityIdToDelete) { commonOwningInstance = commonOwningInstance->get().GetParentInstance(); } From 16b2dabde2bf0635b54bd113ea73811b819acdd8 Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 14:51:27 -0700 Subject: [PATCH 6/8] Removed additional spacing --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 3ac3da11f0..8699d60440 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -732,7 +732,7 @@ namespace AzToolsFramework Prefab::PrefabDom instanceDomAfter; m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomAfter, commonOwningInstance->get()); - + PrefabUndoInstance* command = aznew PrefabUndoInstance("Instance deletion"); command->Capture(instanceDomBefore, instanceDomAfter, commonOwningInstance->get().GetTemplateId()); command->SetParent(selCommand); From d8865a8f0d3f8ff458a94d49e61ceb2e24248a94 Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 17:41:45 -0700 Subject: [PATCH 7/8] Remove links when deleting prefabs --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 8699d60440..6f217f46f4 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -708,6 +708,7 @@ namespace AzToolsFramework for (auto& nestedInstance : instances) { + RemoveLink(nestedInstance, commonOwningInstance->get().GetTemplateId(), currentUndoBatch); nestedInstance.reset(); } } @@ -720,7 +721,7 @@ namespace AzToolsFramework if (owningInstance->get().GetContainerEntityId() == entityId) { auto instancePtr = commonOwningInstance->get().DetachNestedInstance(owningInstance->get().GetInstanceAlias()); - instancePtr.reset(); + RemoveLink(instancePtr, commonOwningInstance->get().GetTemplateId(), currentUndoBatch); } else { From b72836fa8f31a6aced58d22a973634022ad586df Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 13 May 2021 18:03:49 -0700 Subject: [PATCH 8/8] Fixed a typo --- .../AzToolsFramework/Prefab/PrefabPublicHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 6f217f46f4..5ecff637a1 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -643,7 +643,7 @@ namespace AzToolsFramework InstanceOptionalReference commonOwningInstance = GetOwnerInstanceByEntityId(firstEntityIdToDelete); // If the first entity id is a container entity id, then we need to mark its parent as the common owning instance because you - // cannot detete an instance from itself. + // cannot delete an instance from itself. if (commonOwningInstance->get().GetContainerEntityId() == firstEntityIdToDelete) { commonOwningInstance = commonOwningInstance->get().GetParentInstance();