From 17dbe55189ff586df47d66ed3b6404b6850988ac Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 22 Apr 2021 13:57:08 -0700 Subject: [PATCH 1/2] Added undo and redo support for nested prefab creation --- .../PrefabEditorEntityOwnershipService.cpp | 4 +- .../Prefab/Instance/Instance.cpp | 9 +++- .../Prefab/Instance/Instance.h | 1 + .../Prefab/PrefabPublicHandler.cpp | 48 +++++++------------ .../Prefab/PrefabSystemComponent.cpp | 29 ++++++----- .../Prefab/PrefabSystemComponent.h | 10 ++-- .../Prefab/PrefabSystemComponentInterface.h | 4 +- 7 files changed, 54 insertions(+), 51 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp index 2424658ecf..363e01f889 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp @@ -277,7 +277,7 @@ namespace AzToolsFramework AZ::IO::PathView filePath, Prefab::InstanceOptionalReference instanceToParentUnder) { AZStd::unique_ptr createdPrefabInstance = - m_prefabSystemComponent->CreatePrefab(entities, AZStd::move(nestedPrefabInstances), filePath); + m_prefabSystemComponent->CreatePrefab(entities, AZStd::move(nestedPrefabInstances), filePath, nullptr, false); if (createdPrefabInstance) { @@ -296,7 +296,7 @@ namespace AzToolsFramework Prefab::PrefabDom serializedInstance; if (Prefab::PrefabDomUtils::StoreInstanceInPrefabDom(addedInstance, serializedInstance)) { - m_prefabSystemComponent->UpdatePrefabTemplate(addedInstance.GetTemplateId(), serializedInstance); + m_prefabSystemComponent->UpdatePrefabTemplate(addedInstance.GetTemplateId(), serializedInstance, false); } return addedInstance; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.cpp index 0a5b43482e..8e7dbe36f6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.cpp @@ -317,7 +317,6 @@ namespace AzToolsFramework removedNestedInstance = AZStd::move(nestedInstanceIterator->second); removedNestedInstance->m_parent = nullptr; - removedNestedInstance->m_alias = InstanceAlias(); m_nestedInstances.erase(instanceAlias); } @@ -392,6 +391,14 @@ namespace AzToolsFramework } } + void Instance::GetNestedInstances(const AZStd::function&)>& callback) + { + for (auto& [instanceAlias, instance] : m_nestedInstances) + { + callback(instance); + } + } + void Instance::GetEntities(const AZStd::function&)>& callback) { for (auto& [entityAlias, entity] : m_entities) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h index d1c7a4d853..a91faec5bd 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h @@ -113,6 +113,7 @@ namespace AzToolsFramework void GetConstEntities(const AZStd::function& callback); void GetNestedEntities(const AZStd::function&)>& callback); void GetEntities(const AZStd::function&)>& callback); + void GetNestedInstances(const AZStd::function&)>& callback); /** * Gets the alias for a given EnitityId in the Instance DOM. diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 46bd924f30..85da969f6c 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -101,6 +101,10 @@ namespace AzToolsFramework nestedInstance->GetInstanceAlias(), nestedInstance->GetLinkId(), undoBatch.GetUndoBatch()); } + PrefabUndoHelpers::UpdatePrefabInstance( + commonRootEntityOwningInstance->get(), "Update prefab instance", commonRootInstanceDomBeforeCreate, + undoBatch.GetUndoBatch()); + auto prefabEditorEntityOwnershipInterface = AZ::Interface::Get(); if (!prefabEditorEntityOwnershipInterface) { @@ -118,13 +122,21 @@ namespace AzToolsFramework "(A null instance is returned).")); } - PrefabUndoHelpers::UpdatePrefabInstance( - commonRootEntityOwningInstance->get(), "Update prefab instance", commonRootInstanceDomBeforeCreate, undoBatch.GetUndoBatch()); + AZ::EntityId containerEntityId = instanceToCreate->get().GetContainerEntityId(); + + instanceToCreate->get().GetNestedInstances([&](AZStd::unique_ptr& nestedInstance) { + AZ_Assert(nestedInstance, "Invalid nested instance found in the new prefab created."); + EntityOptionalReference nestedInstanceContainerEntity = nestedInstance->GetContainerEntity(); + AZ_Assert( + nestedInstanceContainerEntity, "Invalid container entity found for the nested instance used in prefab creation."); + CreateLink( + {&nestedInstanceContainerEntity->get()}, *nestedInstance, instanceToCreate->get().GetTemplateId(), + undoBatch.GetUndoBatch(), containerEntityId); + }); CreateLink( topLevelEntities, instanceToCreate->get(), commonRootEntityOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch(), commonRootEntityId); - AZ::EntityId containerEntityId = instanceToCreate->get().GetContainerEntityId(); // Change top level entities to be parented to the container entity // Mark them as dirty so this change is correctly applied to the template @@ -225,19 +237,7 @@ namespace AzToolsFramework PrefabOperationResult PrefabPublicHandler::SavePrefab(AZ::IO::Path filePath) { - auto prefabSystemComponentInterface = AZ::Interface::Get(); - if (!prefabSystemComponentInterface) - { - AZ_Assert( - false, - "Prefab - PrefabPublicHandler - " - "Prefab System Component Interface could not be found. " - "Check that it is being correctly initialized."); - return AZ::Failure( - AZStd::string("SavePrefab - Internal error (Prefab System Component Interface could not be found).")); - } - - auto templateId = prefabSystemComponentInterface->GetTemplateIdFromFilePath(filePath.c_str()); + auto templateId = m_prefabSystemComponentInterface->GetTemplateIdFromFilePath(filePath.c_str()); if (templateId == InvalidTemplateId) { @@ -438,26 +438,14 @@ namespace AzToolsFramework PrefabRequestResult PrefabPublicHandler::HasUnsavedChanges(AZ::IO::Path prefabFilePath) const { - auto prefabSystemComponentInterface = AZ::Interface::Get(); - if (!prefabSystemComponentInterface) - { - AZ_Assert( - false, - "Prefab - PrefabPublicHandler - " - "Prefab System Component Interface could not be found. " - "Check that it is being correctly initialized."); - return AZ::Failure( - AZStd::string("HasUnsavedChanges - Internal error (Prefab System Component Interface could not be found).")); - } - - auto templateId = prefabSystemComponentInterface->GetTemplateIdFromFilePath(prefabFilePath.c_str()); + auto templateId = m_prefabSystemComponentInterface->GetTemplateIdFromFilePath(prefabFilePath.c_str()); if (templateId == InvalidTemplateId) { return AZ::Failure(AZStd::string("HasUnsavedChanges - Path error. Path could be invalid, or the prefab may not be loaded in this level.")); } - return AZ::Success(prefabSystemComponentInterface->IsTemplateDirty(templateId)); + return AZ::Success(m_prefabSystemComponentInterface->IsTemplateDirty(templateId)); } PrefabOperationResult PrefabPublicHandler::DeleteEntitiesInInstance(const EntityIdList& entityIds) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp index 9070630e56..4b25c80fd3 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp @@ -91,8 +91,9 @@ namespace AzToolsFramework m_instanceUpdateExecutor.UpdateTemplateInstancesInQueue(); } - AZStd::unique_ptr PrefabSystemComponent::CreatePrefab(const AZStd::vector& entities, AZStd::vector>&& instancesToConsume, - AZ::IO::PathView filePath, AZStd::unique_ptr containerEntity) + AZStd::unique_ptr PrefabSystemComponent::CreatePrefab( + const AZStd::vector& entities, AZStd::vector>&& instancesToConsume, + AZ::IO::PathView filePath, AZStd::unique_ptr containerEntity, bool shouldCreateLinks) { AZ::IO::Path relativeFilePath = m_prefabLoader.GetRelativePathToProject(filePath); if (GetTemplateIdFromFilePath(relativeFilePath) != InvalidTemplateId) @@ -122,7 +123,7 @@ namespace AzToolsFramework newInstance->SetTemplateSourcePath(relativeFilePath); - TemplateId newTemplateId = CreateTemplateFromInstance(*newInstance); + TemplateId newTemplateId = CreateTemplateFromInstance(*newInstance, shouldCreateLinks); if (newTemplateId == InvalidTemplateId) { AZ_Error("Prefab", false, @@ -157,7 +158,7 @@ namespace AzToolsFramework } } - void PrefabSystemComponent::UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) + void PrefabSystemComponent::UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom, bool shouldPropagateTemplateChanges) { auto templateToUpdate = FindTemplate(templateId); if (templateToUpdate) @@ -167,7 +168,10 @@ namespace AzToolsFramework { templateDomToUpdate.CopyFrom(updatedDom, templateDomToUpdate.GetAllocator()); templateToUpdate->get().MarkAsDirty(true); - PropagateTemplateChanges(templateId); + if (shouldPropagateTemplateChanges) + { + PropagateTemplateChanges(templateId); + } } } } @@ -288,7 +292,7 @@ namespace AzToolsFramework return newInstance; } - TemplateId PrefabSystemComponent::CreateTemplateFromInstance(Instance& instance) + TemplateId PrefabSystemComponent::CreateTemplateFromInstance(Instance& instance, bool shouldCreateLinks) { // We will register the template to match the path the instance has const AZ::IO::Path& templateSourcePath = instance.GetTemplateSourcePath(); @@ -322,14 +326,15 @@ namespace AzToolsFramework return InvalidTemplateId; } - if (!GenerateLinksForNewTemplate(newTemplateId, instance)) + if (shouldCreateLinks) { - // Clear new template and any links associated with it - RemoveTemplate(newTemplateId); - - return InvalidTemplateId; + if (!GenerateLinksForNewTemplate(newTemplateId, instance)) + { + // Clear new template and any links associated with it + RemoveTemplate(newTemplateId); + return InvalidTemplateId; + } } - return newTemplateId; } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h index 40780b9775..d04b760aae 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h @@ -190,8 +190,10 @@ namespace AzToolsFramework * @param filePath the path to associate the template of the new instance to * @return A pointer to the newly created instance. nullptr on failure */ - AZStd::unique_ptr CreatePrefab(const AZStd::vector& entities, AZStd::vector>&& instancesToConsume, - AZ::IO::PathView filePath, AZStd::unique_ptr containerEntity = nullptr) override; + AZStd::unique_ptr CreatePrefab( + const AZStd::vector& entities, AZStd::vector>&& instancesToConsume, + AZ::IO::PathView filePath, AZStd::unique_ptr containerEntity = nullptr, + bool ShouldCreateLinks = true) override; PrefabDom& FindTemplateDom(TemplateId templateId) override; @@ -201,7 +203,7 @@ namespace AzToolsFramework * @param templateId The id of the template to update. * @param updatedDom The DOM to update the template with. */ - void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) override; + void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom, bool shouldPropagateTemplateChanges = true) override; void PropagateTemplateChanges(TemplateId templateId) override; @@ -262,7 +264,7 @@ namespace AzToolsFramework * along with any new Prefab Links representing any of the nested instances present * @param instance The instance used to generate the new Template */ - TemplateId CreateTemplateFromInstance(Instance& instance); + TemplateId CreateTemplateFromInstance(Instance& instance, bool shouldCreateLinks); /** * Connect two templates with given link, and a nested instance value iterator diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h index 8b94935cc1..d957a14b61 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h @@ -55,13 +55,13 @@ namespace AzToolsFramework virtual void SetTemplateDirtyFlag(const TemplateId& templateId, bool dirty) = 0; virtual PrefabDom& FindTemplateDom(TemplateId templateId) = 0; - virtual void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) = 0; + virtual void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom, bool shouldPropagateTemplateChanges = true) = 0; virtual void PropagateTemplateChanges(TemplateId templateId) = 0; virtual AZStd::unique_ptr InstantiatePrefab(const TemplateId& templateId) = 0; virtual AZStd::unique_ptr CreatePrefab(const AZStd::vector& entities, AZStd::vector>&& instancesToConsume, AZ::IO::PathView filePath, - AZStd::unique_ptr containerEntity = nullptr) = 0; + AZStd::unique_ptr containerEntity = nullptr, bool ShouldCreateLinks = true) = 0; }; From a9af90be5ab54ce5f370fef6f6a49120e456b81e Mon Sep 17 00:00:00 2001 From: srikappa Date: Thu, 22 Apr 2021 17:01:29 -0700 Subject: [PATCH 2/2] Removed the shouldPropagateTemplateChanges flag to revisit optimization later and fixed some function comments --- .../Entity/PrefabEditorEntityOwnershipService.cpp | 2 +- .../Prefab/PrefabSystemComponent.cpp | 7 ++----- .../AzToolsFramework/Prefab/PrefabSystemComponent.h | 13 +++++++++---- .../Prefab/PrefabSystemComponentInterface.h | 2 +- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp index dd1fdec74d..07be2a6644 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp @@ -298,7 +298,7 @@ namespace AzToolsFramework Prefab::PrefabDom serializedInstance; if (Prefab::PrefabDomUtils::StoreInstanceInPrefabDom(addedInstance, serializedInstance)) { - m_prefabSystemComponent->UpdatePrefabTemplate(addedInstance.GetTemplateId(), serializedInstance, false); + m_prefabSystemComponent->UpdatePrefabTemplate(addedInstance.GetTemplateId(), serializedInstance); } return addedInstance; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp index 53652933a4..d89a388e91 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp @@ -159,7 +159,7 @@ namespace AzToolsFramework } } - void PrefabSystemComponent::UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom, bool shouldPropagateTemplateChanges) + void PrefabSystemComponent::UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) { auto templateToUpdate = FindTemplate(templateId); if (templateToUpdate) @@ -169,10 +169,7 @@ namespace AzToolsFramework { templateDomToUpdate.CopyFrom(updatedDom, templateDomToUpdate.GetAllocator()); templateToUpdate->get().MarkAsDirty(true); - if (shouldPropagateTemplateChanges) - { - PropagateTemplateChanges(templateId); - } + PropagateTemplateChanges(templateId); } } } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h index d04b760aae..d4499d1e2d 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h @@ -187,7 +187,10 @@ namespace AzToolsFramework * @param entities A vector of entities that will be used in the new instance. May be empty * @param instances A vector of Prefab Instances that will be nested in the new instance, will be consumed and moved. * May be empty - * @param filePath the path to associate the template of the new instance to + * @param filePath the path to associate the template of the new instance to. + * @param containerEntity The container entity for the prefab to be created. It will be created if a nullptr is provided. + * @param shouldCreateLinks The flag indicating if links should be created between the templates of the instance + * and its nested instances. * @return A pointer to the newly created instance. nullptr on failure */ AZStd::unique_ptr CreatePrefab( @@ -199,11 +202,11 @@ namespace AzToolsFramework /** * Updates a template with the given updated DOM. - * + * * @param templateId The id of the template to update. * @param updatedDom The DOM to update the template with. */ - void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom, bool shouldPropagateTemplateChanges = true) override; + void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) override; void PropagateTemplateChanges(TemplateId templateId) override; @@ -262,7 +265,9 @@ namespace AzToolsFramework /** * Takes a prefab instance and generates a new Prefab Template * along with any new Prefab Links representing any of the nested instances present - * @param instance The instance used to generate the new Template + * @param instance The instance used to generate the new Template. + * @param shouldCreateLinks The flag indicating if links should be created between the templates of the instance + * and its nested instances. */ TemplateId CreateTemplateFromInstance(Instance& instance, bool shouldCreateLinks); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h index d957a14b61..f0ea99f3dd 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h @@ -55,7 +55,7 @@ namespace AzToolsFramework virtual void SetTemplateDirtyFlag(const TemplateId& templateId, bool dirty) = 0; virtual PrefabDom& FindTemplateDom(TemplateId templateId) = 0; - virtual void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom, bool shouldPropagateTemplateChanges = true) = 0; + virtual void UpdatePrefabTemplate(TemplateId templateId, const PrefabDom& updatedDom) = 0; virtual void PropagateTemplateChanges(TemplateId templateId) = 0; virtual AZStd::unique_ptr InstantiatePrefab(const TemplateId& templateId) = 0;