diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 549830d24e..a52075f4cd 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -87,7 +87,8 @@ namespace AzToolsFramework commonRootInstanceDomBeforeCreate, commonRootEntityOwningInstance->get()); AZStd::vector entities; - AZStd::vector> instances; + AZStd::vector> instancePtrs; + AZStd::vector instances; // Retrieve all entities affected and identify Instances if (!RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonRootEntityOwningInstance->get(), entities, instances)) @@ -96,11 +97,19 @@ namespace AzToolsFramework AZStd::string("Could not create a new prefab out of the entities provided - invalid selection.")); } + // Detach the retrieved entities + for (AZ::Entity* entity : entities) + { + commonRootEntityOwningInstance->get().DetachEntity(entity->GetId()).release(); + } + // When we create a prefab with other prefab instances, we have to remove the existing links between the source and // target templates of the other instances. for (auto& nestedInstance : instances) { - RemoveLink(nestedInstance, commonRootEntityOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch()); + AZStd::unique_ptr outInstance = commonRootEntityOwningInstance->get().DetachNestedInstance(nestedInstance->GetInstanceAlias()); + instancePtrs.emplace_back(AZStd::move(outInstance)); + RemoveLink(outInstance, commonRootEntityOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch()); } PrefabUndoHelpers::UpdatePrefabInstance( @@ -116,7 +125,7 @@ namespace AzToolsFramework // Create the Prefab instanceToCreate = prefabEditorEntityOwnershipInterface->CreatePrefab( - entities, AZStd::move(instances), filePath, commonRootEntityOwningInstance); + entities, AZStd::move(instancePtrs), filePath, commonRootEntityOwningInstance); if (!instanceToCreate) { @@ -286,7 +295,7 @@ namespace AzToolsFramework // Find common root and top level entities bool entitiesHaveCommonRoot = false; - AzToolsFramework::ToolsApplicationRequests::Bus::BroadcastResult( + AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult( entitiesHaveCommonRoot, &AzToolsFramework::ToolsApplicationRequests::FindCommonRootInactive, inputEntityList, commonRootEntityId, &topLevelEntities); @@ -639,18 +648,21 @@ namespace AzToolsFramework { if (entityIds.empty()) { - return AZ::Success(); + return AZ::Failure(AZStd::string("No entities to duplicate.")); } if (!EntitiesBelongToSameInstance(entityIds)) { - return AZ::Failure(AZStd::string("DuplicateEntitiesInInstance - Duplication Error. Cannot duplicate multiple " + return AZ::Failure(AZStd::string("Cannot duplicate multiple " "entities belonging to different instances with one operation.")); } // We've already verified the entities are all owned by the same instance, // so we can just retrieve our instance from the first entity in the list. - InstanceOptionalReference instance = GetOwnerInstanceByEntityId(entityIds[0]); + InstanceOptionalReference commonEntityOwningInstance = GetOwnerInstanceByEntityId(entityIds[0]); + AZ_Assert( + commonEntityOwningInstance.has_value(), + "Failed to duplicate : Couldn't get a valid owning instance for the common root entity of the entities provided"); // This will cull out any entities that have ancestors in the list, since we will end up duplicating // the full nested hierarchy with what is returned from RetrieveAndSortPrefabEntitiesAndInstances @@ -658,48 +670,21 @@ namespace AzToolsFramework AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzToolsFramework); - UndoSystem::URSequencePoint* currentUndoBatch = nullptr; - ToolsApplicationRequests::Bus::BroadcastResult(currentUndoBatch, &ToolsApplicationRequests::Bus::Events::GetCurrentUndoBatch); - - bool createdUndo = false; - if (!currentUndoBatch) - { - createdUndo = true; - ToolsApplicationRequests::Bus::BroadcastResult( - currentUndoBatch, &ToolsApplicationRequests::Bus::Events::BeginUndoBatch, "Duplicate Entities"); - AZ_Assert(currentUndoBatch, "Failed to create new undo batch."); - } - - // In order to undo DuplicateEntitiesInInstance, we have to create a selection command which selects the current selection - // and then add the duplication as children. - // Commands always execute themselves first and then their children (when going forwards) - // and do the opposite when going backwards. - EntityIdList selectedEntities; - ToolsApplicationRequestBus::BroadcastResult(selectedEntities, &ToolsApplicationRequests::GetSelectedEntities); - SelectionCommand* selCommand = aznew SelectionCommand(selectedEntities, "Duplicate Entities"); - - // We insert a "deselect all" command before we duplicate the entities. This ensures the duplicate operations aren't changing - // selection state, which triggers expensive UI updates. By deselecting up front, we are able to do those expensive - // UI updates once at the start instead of once for each entity. - { - EntityIdList deselection; - SelectionCommand* deselectAllCommand = aznew SelectionCommand(deselection, "Deselect Entities"); - deselectAllCommand->SetParent(selCommand); - } + ScopedUndoBatch undoBatch("Duplicate Entities"); { AZ_PROFILE_SCOPE(AZ::Debug::ProfileCategory::AzToolsFramework, "DuplicateEntitiesInInstance::UndoCaptureAndDuplicateEntities"); // Take a snapshot of the instance DOM before we manipulate it Prefab::PrefabDom instanceDomBefore; - m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBefore, instance->get()); + m_instanceToTemplateInterface->GenerateDomForInstance(instanceDomBefore, commonEntityOwningInstance->get()); AZStd::vector entities; - AZStd::vector> instances; + AZStd::vector instances; // Gather all entities/instances in the hierarchy, but don't detach them because we are duplicating not deleting. EntityList inputEntityList = EntityIdSetToEntityList(duplicationSet); - bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, instance->get(), entities, instances, false); + bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonEntityOwningInstance->get(), entities, instances); if (!success) { @@ -715,7 +700,7 @@ namespace AzToolsFramework for (AZ::Entity* entity : entities) { - EntityAliasOptionalReference oldAliasRef = instance->get().GetEntityAlias(entity->GetId()); + EntityAliasOptionalReference oldAliasRef = commonEntityOwningInstance->get().GetEntityAlias(entity->GetId()); AZ_Assert(oldAliasRef.has_value(), "No alias found for Entity in the DOM"); EntityAlias oldAlias = oldAliasRef.value(); @@ -731,14 +716,10 @@ namespace AzToolsFramework // Update the Entity Id in the Entity DOM for the duplicated Entity auto entityIdIter = entityDomBefore.FindMember(PrefabDomUtils::EntityIdName); - if (entityIdIter != entityDomBefore.MemberEnd()) - { - entityIdIter->value.SetString(newEntityAlias.c_str(), newEntityAlias.length(), entityDomBefore.GetAllocator()); - } + AZ_Assert(entityIdIter != entityDomBefore.MemberEnd(), "Entity DOM missing Id."); + entityIdIter->value.SetString(newEntityAlias.c_str(), newEntityAlias.length(), entityDomBefore.GetAllocator()); rapidjson::StringBuffer buffer; - buffer.Clear(); - rapidjson::Writer writer(buffer); entityDomBefore.Accept(writer); @@ -776,20 +757,27 @@ namespace AzToolsFramework entitiesIter->value.AddMember(AZStd::move(aliasName), entityDomAfter, instanceDomAfter.GetAllocator()); } - PrefabUndoInstance* command = aznew PrefabUndoInstance("Instance duplication"); - command->Capture(instanceDomBefore, instanceDomAfter, instance->get().GetTemplateId()); - command->SetParent(selCommand); - } + PrefabUndoInstance* command = aznew PrefabUndoInstance("Entity duplication"); + command->SetParent(undoBatch.GetUndoBatch()); + command->Capture(instanceDomBefore, instanceDomAfter, commonEntityOwningInstance->get().GetTemplateId()); + command->RunRedo(); - selCommand->SetParent(currentUndoBatch); - { - AZ_PROFILE_SCOPE(AZ::Debug::ProfileCategory::AzToolsFramework, "DuplicateEntitiesInInstance:RunRedo"); - selCommand->RunRedo(); - } + EntityIdList duplicatedEntityIds; + for (auto aliasMapIter : oldAliasToNewAliasMap) + { + EntityAlias newEntityAlias = aliasMapIter.second; - if (createdUndo) - { - ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::EndUndoBatch); + AliasPath absoluteEntityPath = commonEntityOwningInstance->get().GetAbsoluteInstanceAliasPath(); + absoluteEntityPath.Append(newEntityAlias); + + AZ::EntityId newEntityId = InstanceEntityIdMapper::GenerateEntityIdForAliasPath(absoluteEntityPath); + duplicatedEntityIds.push_back(newEntityId); + } + + // Select the duplicated entities + auto selectionUndo = aznew SelectionCommand(duplicatedEntityIds, "Select Duplicated Entities"); + selectionUndo->SetParent(undoBatch.GetUndoBatch()); + ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequestBus::Events::RunRedoSeparately, selectionUndo); } return AZ::Success(); @@ -822,17 +810,7 @@ namespace AzToolsFramework AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzToolsFramework); - UndoSystem::URSequencePoint* currentUndoBatch = nullptr; - ToolsApplicationRequests::Bus::BroadcastResult(currentUndoBatch, &ToolsApplicationRequests::Bus::Events::GetCurrentUndoBatch); - - bool createdUndo = false; - if (!currentUndoBatch) - { - createdUndo = true; - ToolsApplicationRequests::Bus::BroadcastResult( - currentUndoBatch, &ToolsApplicationRequests::Bus::Events::BeginUndoBatch, "Delete Selected"); - AZ_Assert(currentUndoBatch, "Failed to create new undo batch."); - } + ScopedUndoBatch undoBatch("Delete Selected"); // In order to undo DeleteSelected, we have to create a selection command which selects the current selection // and then add the deletion as children. @@ -860,7 +838,7 @@ namespace AzToolsFramework if (deleteDescendants) { AZStd::vector entities; - AZStd::vector> instances; + AZStd::vector instances; bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonOwningInstance->get(), entities, instances); @@ -871,13 +849,15 @@ namespace AzToolsFramework for (AZ::Entity* entity : entities) { + commonOwningInstance->get().DetachEntity(entity->GetId()).release(); AZ::ComponentApplicationBus::Broadcast(&AZ::ComponentApplicationRequests::DeleteEntity, entity->GetId()); } for (auto& nestedInstance : instances) { - RemoveLink(nestedInstance, commonOwningInstance->get().GetTemplateId(), currentUndoBatch); - nestedInstance.reset(); + AZStd::unique_ptr outInstance = commonOwningInstance->get().DetachNestedInstance(nestedInstance->GetInstanceAlias()); + RemoveLink(outInstance, commonOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch()); + outInstance.reset(); } } else @@ -889,7 +869,7 @@ namespace AzToolsFramework if (owningInstance->get().GetContainerEntityId() == entityId) { auto instancePtr = commonOwningInstance->get().DetachNestedInstance(owningInstance->get().GetInstanceAlias()); - RemoveLink(instancePtr, commonOwningInstance->get().GetTemplateId(), currentUndoBatch); + RemoveLink(instancePtr, commonOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch()); } else { @@ -907,17 +887,12 @@ namespace AzToolsFramework command->SetParent(selCommand); } - selCommand->SetParent(currentUndoBatch); + selCommand->SetParent(undoBatch.GetUndoBatch()); { AZ_PROFILE_SCOPE(AZ::Debug::ProfileCategory::AzToolsFramework, "Internal::DeleteEntities:RunRedo"); selCommand->RunRedo(); } - if (createdUndo) - { - ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::EndUndoBatch); - } - return AZ::Success(); } @@ -1029,8 +1004,7 @@ namespace AzToolsFramework bool PrefabPublicHandler::RetrieveAndSortPrefabEntitiesAndInstances( const EntityList& inputEntities, Instance& commonRootEntityOwningInstance, - EntityList& outEntities, AZStd::vector>& outInstances, - bool shouldDetach) const + EntityList& outEntities, AZStd::vector& outInstances) const { if (inputEntities.size() == 0) { @@ -1114,16 +1088,14 @@ namespace AzToolsFramework for (AZ::Entity* entity : entities) { - AZ::Entity* outEntity = (shouldDetach) ? commonRootEntityOwningInstance.DetachEntity(entity->GetId()).release() : entity; - outEntities.emplace_back(outEntity); + outEntities.emplace_back(entity); } outInstances.clear(); outInstances.reserve(instances.size()); for (Instance* instancePtr : instances) { - AZStd::unique_ptr outInstance = (shouldDetach) ? commonRootEntityOwningInstance.DetachNestedInstance(instancePtr->GetInstanceAlias()) : AZStd::unique_ptr(instancePtr); - outInstances.push_back(AZStd::move(outInstance)); + outInstances.push_back(instancePtr); } return (outEntities.size() + outInstances.size()) > 0; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index b6128f0ab9..b4e428efcc 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -65,7 +65,7 @@ namespace AzToolsFramework private: PrefabOperationResult DeleteFromInstance(const EntityIdList& entityIds, bool deleteDescendants); bool RetrieveAndSortPrefabEntitiesAndInstances(const EntityList& inputEntities, Instance& commonRootEntityOwningInstance, - EntityList& outEntities, AZStd::vector>& outInstances, bool shouldDetach = true) const; + EntityList& outEntities, AZStd::vector& outInstances) const; InstanceOptionalReference GetOwnerInstanceByEntityId(AZ::EntityId entityId) const; bool EntitiesBelongToSameInstance(const EntityIdList& entityIds) const; diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabDuplicateTests.cpp b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabDuplicateTests.cpp new file mode 100644 index 0000000000..514942166f --- /dev/null +++ b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabDuplicateTests.cpp @@ -0,0 +1,129 @@ +/* +* All or portions of this file Copyright (c) Amazon.com, Inc. or its affiliates or +* its licensors. +* +* For complete copyright and license terms please see the LICENSE at the root of this +* distribution (the "License"). All use of this software is governed by the License, +* or, if provided, by the license below or the license accompanying this file. Do not +* remove or modify any license notices. This file is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* +*/ + +#include + +#include +#include +#include + +namespace UnitTest +{ + using PrefabDuplicateTest = PrefabTestFixture; + + TEST_F(PrefabDuplicateTest, PrefabDuplicate_DuplicateSingleEntitySucceeds) + { + AZStd::string entityName("Same Name"); + AZ::Entity* entity1 = CreateEntity(entityName.c_str()); + entity1->Deactivate(); + entity1->CreateComponent(); + entity1->Activate(); + + AzToolsFramework::EditorEntityContextRequestBus::Broadcast( + &AzToolsFramework::EditorEntityContextRequests::HandleEntitiesAdded, AzToolsFramework::EntityList{ entity1 }); + AZStd::unique_ptr newInstance = m_prefabSystemComponent->CreatePrefab( + { entity1 }, + {}, + PrefabMockFilePath); + + // We've created a prefab with a single Entity, so there should only be one EntityAlias in our instance + EXPECT_EQ(newInstance->GetEntityAliases().size(), 1); + + // Duplicate the Entity and trigger the UpdateTemplateInstancesInQueue so the changes get propagated + m_prefabPublicInterface->DuplicateEntitiesInInstance(AzToolsFramework::EntityIdList{ entity1->GetId() }); + m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue(); + + // We duplicated a single Entity, so there should now be two EntityAliases + EXPECT_EQ(newInstance->GetEntityAliases().size(), 2); + + newInstance->GetConstEntities([&](const AZ::Entity& entity) + { + // Both of the entities should have the same name + EXPECT_EQ(entity.GetName(), entityName); + + // Both of the entities should have the PrefabTestComponent we added + auto testComponent = entity.FindComponent(); + EXPECT_NE(nullptr, testComponent); + + return true; + }); + } + + TEST_F(PrefabDuplicateTest, PrefabDuplicate_DuplicateMultipleEntitiesAndFixesReferences) + { + AZ::Entity* parentEntity = CreateEntity("Parent Entity"); + + AZ::Entity* childEntity = CreateEntity("Child Entity"); + childEntity->Deactivate(); + auto newComponent = childEntity->CreateComponent(); + childEntity->Activate(); + + // Set the EntityId reference property on our PrefabTestComponent so we can + // verify that arbitrary EntityId's are fixed up properly + newComponent->m_entityIdProperty = parentEntity->GetId(); + + AzToolsFramework::EditorEntityContextRequestBus::Broadcast( + &AzToolsFramework::EditorEntityContextRequests::HandleEntitiesAdded, AzToolsFramework::EntityList{ parentEntity, childEntity }); + + AZStd::unique_ptr newInstance = m_prefabSystemComponent->CreatePrefab( + { parentEntity, childEntity }, + {}, + PrefabMockFilePath); + + // We've created a prefab with two entities, so there should be two EntityAliases in our instance + EXPECT_EQ(newInstance->GetEntityAliases().size(), 2); + + // Duplicate the entities and trigger the UpdateTemplateInstancesInQueue so the changes get propagated + m_prefabPublicInterface->DuplicateEntitiesInInstance(AzToolsFramework::EntityIdList{ parentEntity->GetId(), childEntity->GetId() }); + m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue(); + + // We duplicated two entities, so there should now be four EntityAliases + EXPECT_EQ(newInstance->GetEntityAliases().size(), 4); + + AzToolsFramework::EntityIdList parentEntityIds; + newInstance->GetConstEntities([&](const AZ::Entity& entity) + { + // Gather the parent EntityIds by tracking which entities don't have a PrefabTestComponent + auto testComponent = entity.FindComponent(); + if (!testComponent) + { + parentEntityIds.push_back(entity.GetId()); + } + + return true; + }); + + // There should only be two parents + EXPECT_EQ(parentEntityIds.size(), 2); + + // Verify that the EntityId reference on the PrefabTestComponent on the children correspond + // to unique entities, which will verify that the EntityIds are fixed up on duplicate + newInstance->GetConstEntities([&](const AZ::Entity& entity) + { + // Only the child entities have a PrefabTestComponent + auto testComponent = entity.FindComponent(); + if (testComponent) + { + auto it = AZStd::find(parentEntityIds.begin(), parentEntityIds.end(), testComponent->m_entityIdProperty); + EXPECT_NE(it, parentEntityIds.end()); + + // Erase when we find it so that the matches will be unique + parentEntityIds.erase(it); + } + + return true; + }); + + // Verify we matched each of the parent EntityIds + EXPECT_EQ(parentEntityIds.size(), 0); + } +} diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp index 3a8d9cc7eb..dbf7397fec 100644 --- a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp +++ b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp @@ -32,6 +32,9 @@ namespace UnitTest m_prefabLoaderInterface = AZ::Interface::Get(); EXPECT_TRUE(m_prefabLoaderInterface); + m_prefabPublicInterface = AZ::Interface::Get(); + EXPECT_TRUE(m_prefabPublicInterface); + m_instanceUpdateExecutorInterface = AZ::Interface::Get(); EXPECT_TRUE(m_instanceUpdateExecutorInterface); diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h index af90309867..1338dba0dd 100644 --- a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h +++ b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h @@ -57,6 +57,7 @@ namespace UnitTest PrefabSystemComponent* m_prefabSystemComponent = nullptr; PrefabLoaderInterface* m_prefabLoaderInterface = nullptr; + PrefabPublicInterface* m_prefabPublicInterface = nullptr; InstanceUpdateExecutorInterface* m_instanceUpdateExecutorInterface = nullptr; InstanceToTemplateInterface* m_instanceToTemplateInterface = nullptr; }; diff --git a/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake b/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake index e54aa187e4..cd3796a64e 100644 --- a/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake +++ b/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake @@ -54,6 +54,7 @@ set(FILES Prefab/Spawnable/SpawnableMetaDataTests.cpp Prefab/MockPrefabFileIOActionValidator.cpp Prefab/MockPrefabFileIOActionValidator.h + Prefab/PrefabDuplicateTests.cpp Prefab/PrefabEntityAliasTests.cpp Prefab/PrefabInstanceToTemplatePropagatorTests.cpp Prefab/PrefabInstantiateTests.cpp