From 45074c651afe3e860c2f5828e8dfe8614829c3ee Mon Sep 17 00:00:00 2001 From: srikappa Date: Mon, 17 May 2021 17:10:50 -0700 Subject: [PATCH 1/2] Fix bugs with creating prefabs with nested entities --- .../Instance/InstanceUpdateExecutor.cpp | 8 +++-- .../AzToolsFramework/Prefab/Link/Link.cpp | 10 +++--- .../Prefab/PrefabPublicHandler.cpp | 34 +++++++++++++++---- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp index 80ac86c974..6dfdfef39b 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.cpp @@ -120,9 +120,13 @@ namespace AzToolsFramework } } - auto findInstancesResult = m_templateInstanceMapperInterface->FindInstancesOwnedByTemplate(instanceTemplateId)->get(); + auto findInstancesResult = m_templateInstanceMapperInterface->FindInstancesOwnedByTemplate(instanceTemplateId); + AZ_Assert( + findInstancesResult.has_value(), "Prefab Instances corresponding to template with id %llu couldn't be found.", + instanceTemplateId); - if (findInstancesResult.find(instanceToUpdate) == findInstancesResult.end()) + if (findInstancesResult == AZStd::nullopt || + findInstancesResult->get().find(instanceToUpdate) == findInstancesResult->get().end()) { // Since nested instances get reconstructed during propagation, remove any nested instance that no longer // maps to a template. diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Link/Link.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Link/Link.cpp index e0834ed53b..308749ab28 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Link/Link.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Link/Link.cpp @@ -182,16 +182,16 @@ namespace AzToolsFramework else { AZ::JsonSerializationResult::ResultCode applyPatchResult = AZ::JsonSerialization::ApplyPatch( - linkedInstanceDom, + sourceTemplateDomCopy, targetTemplatePrefabDom.GetAllocator(), - sourceTemplatePrefabDom, patchesReference->get(), AZ::JsonMergeApproach::JsonPatch); + linkedInstanceDom.CopyFrom(sourceTemplateDomCopy, targetTemplatePrefabDom.GetAllocator()); if (applyPatchResult.GetProcessing() != AZ::JsonSerializationResult::Processing::Completed) { - AZ_Error("Prefab", false, - "Link::UpdateTarget - " - "ApplyPatches failed for Prefab DOM from source Template '%u' and target Template '%u'.", + AZ_Error( + "Prefab", false, + "Link::UpdateTarget - ApplyPatches failed for Prefab DOM from source Template '%u' and target Template '%u'.", m_sourceTemplateId, m_targetTemplateId); return false; } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 5ecff637a1..9bf101ccf5 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -63,7 +63,7 @@ namespace AzToolsFramework PrefabOperationResult PrefabPublicHandler::CreatePrefab(const AZStd::vector& entityIds, AZ::IO::PathView filePath) { - EntityList inputEntityList, topLevelEntities; + EntityList inputEntityList, topLevelEntities, topLevelNonContainerEntities; AZ::EntityId commonRootEntityId; InstanceOptionalReference commonRootEntityOwningInstance; PrefabOperationResult findCommonRootOutcome = FindCommonRootOwningInstance( @@ -73,6 +73,14 @@ namespace AzToolsFramework return findCommonRootOutcome; } + for (AZ::Entity* toplevelentity : topLevelEntities) + { + if (!IsInstanceContainerEntity(toplevelentity->GetId())) + { + topLevelNonContainerEntities.push_back(toplevelentity); + } + } + InstanceOptionalReference instanceToCreate; { // Initialize Undo Batch object @@ -122,11 +130,11 @@ namespace AzToolsFramework AZ::EntityId containerEntityId = instanceToCreate->get().GetContainerEntityId(); - // Parent the entities to the container entity. Parenting the container entities of the instances passed to createPrefab - // will be done during the creation of links below. - for (AZ::Entity* topLevelEntity : entities) + // Parent the non-container top level entities to the container entity. + // Parenting the top level container entities will be done during the creation of links. + for (AZ::Entity* entity : topLevelNonContainerEntities) { - AZ::TransformBus::Event(topLevelEntity->GetId(), &AZ::TransformBus::Events::SetParent, containerEntityId); + AZ::TransformBus::Event(entity->GetId(), &AZ::TransformBus::Events::SetParent, containerEntityId); } // Update the template of the instance since the entities are modified since the template creation. @@ -142,11 +150,25 @@ namespace AzToolsFramework AZ_Assert( nestedInstanceContainerEntity, "Invalid container entity found for the nested instance used in prefab creation."); + AZ::EntityId parentId; + AZ::TransformBus::EventResult( + parentId, nestedInstanceContainerEntity->get().GetId(), &AZ::TransformBus::Events::GetParentId); + + auto entityIterator = AZStd::find_if( + entities.begin(), entities.end(), [parentId](AZ::Entity* entity) { return entity->GetId() == parentId; }); + + // If the previous parent entity of the nested instance is not part of the entities of the newly created prefab, + // then set the parent of the nested prefab as the container entity of the newly created prefab. + if (entityIterator == entities.end()) + { + parentId = containerEntityId; + } + // These link creations shouldn't be undone because that would put the template in a non-usable state if a user // chooses to instantiate the template after undoing the creation. CreateLink( {&nestedInstanceContainerEntity->get()}, *nestedInstance, instanceToCreate->get().GetTemplateId(), - undoBatch.GetUndoBatch(), containerEntityId, false); + undoBatch.GetUndoBatch(), parentId, false); }); // Create a link between the templates of the newly created instance and the instance it's being parented under. From 761a77a4363deda44d79cf6e1f93bf80639c57bf Mon Sep 17 00:00:00 2001 From: srikappa Date: Mon, 17 May 2021 18:51:23 -0700 Subject: [PATCH 2/2] Avoid creating a new list for non-container top level entities --- .../Prefab/PrefabPublicHandler.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 9f466db520..e6bb8c7dee 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -63,7 +63,7 @@ namespace AzToolsFramework PrefabOperationResult PrefabPublicHandler::CreatePrefab(const AZStd::vector& entityIds, AZ::IO::PathView filePath) { - EntityList inputEntityList, topLevelEntities, topLevelNonContainerEntities; + EntityList inputEntityList, topLevelEntities; AZ::EntityId commonRootEntityId; InstanceOptionalReference commonRootEntityOwningInstance; PrefabOperationResult findCommonRootOutcome = FindCommonRootOwningInstance( @@ -73,14 +73,6 @@ namespace AzToolsFramework return findCommonRootOutcome; } - for (AZ::Entity* toplevelentity : topLevelEntities) - { - if (!IsInstanceContainerEntity(toplevelentity->GetId())) - { - topLevelNonContainerEntities.push_back(toplevelentity); - } - } - InstanceOptionalReference instanceToCreate; { // Initialize Undo Batch object @@ -132,9 +124,12 @@ namespace AzToolsFramework // Parent the non-container top level entities to the container entity. // Parenting the top level container entities will be done during the creation of links. - for (AZ::Entity* entity : topLevelNonContainerEntities) + for (AZ::Entity* topLevelEntity : topLevelEntities) { - AZ::TransformBus::Event(entity->GetId(), &AZ::TransformBus::Events::SetParent, containerEntityId); + if (!IsInstanceContainerEntity(topLevelEntity->GetId())) + { + AZ::TransformBus::Event(topLevelEntity->GetId(), &AZ::TransformBus::Events::SetParent, containerEntityId); + } } // Update the template of the instance since the entities are modified since the template creation.