diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp index 6b6f1475bb..02243a8c77 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp @@ -282,14 +282,6 @@ namespace AzToolsFramework containerEntity->AddComponent(aznew Prefab::EditorPrefabComponent()); HandleEntitiesAdded({containerEntity}); HandleEntitiesAdded(entities); - - // Update the template of the instance since we modified the entities of the instance by calling HandleEntitiesAdded. - Prefab::PrefabDom serializedInstance; - if (Prefab::PrefabDomUtils::StoreInstanceInPrefabDom(addedInstance, serializedInstance)) - { - m_prefabSystemComponent->UpdatePrefabTemplate(addedInstance.GetTemplateId(), serializedInstance); - } - return addedInstance; } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp index b1c5f64ef9..a21c5301aa 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp @@ -270,7 +270,7 @@ namespace AzToolsFramework return parentInstance; } - void InstanceToTemplatePropagator::AddPatchesToLink(PrefabDom& patches, Link& link) + void InstanceToTemplatePropagator::AddPatchesToLink(const PrefabDom& patches, Link& link) { PrefabDom& linkDom = link.GetLinkDom(); PrefabDomValueReference linkPatchesReference = @@ -279,7 +279,14 @@ namespace AzToolsFramework // This logic only covers addition of patches. If patches already exists, the given list of patches must be appended to them. if (!linkPatchesReference.has_value()) { - linkDom.AddMember(rapidjson::StringRef(PrefabDomUtils::PatchesName), patches, linkDom.GetAllocator()); + /* + If the original allocator the patches were created with gets destroyed, then the patches would become garbage in the + linkDom. Since we cannot guarantee the lifecycle of the patch allocators, we are doing a copy of the patches here to + associate them with the linkDom's allocator. + */ + PrefabDom patchesCopy; + patchesCopy.CopyFrom(patches, linkDom.GetAllocator()); + linkDom.AddMember(rapidjson::StringRef(PrefabDomUtils::PatchesName), patchesCopy, linkDom.GetAllocator()); } } } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.h index ca1f3a7c91..9a6aad8ac1 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.h @@ -41,7 +41,7 @@ namespace AzToolsFramework void ApplyPatchesToInstance(const AZ::EntityId& entityId, PrefabDom& patches, const Instance& instanceToAddPatches) override; - void AddPatchesToLink(PrefabDom& patches, Link& link); + void AddPatchesToLink(const PrefabDom& patches, Link& link); private: diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomTypes.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomTypes.h index e32f817227..e897630a91 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomTypes.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomTypes.h @@ -27,6 +27,7 @@ namespace AzToolsFramework using PrefabDomList = AZStd::vector; using PrefabDomReference = AZStd::optional>; + using PrefabDomConstReference = AZStd::optional>; using PrefabDomValueReference = AZStd::optional>; using PrefabDomValueConstReference = AZStd::optional>; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 14537683ef..9649818ed9 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -122,30 +122,49 @@ 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) + { + 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. + Prefab::PrefabDom serializedInstance; + if (Prefab::PrefabDomUtils::StoreInstanceInPrefabDom(instanceToCreate->get(), serializedInstance)) + { + m_prefabSystemComponentInterface->UpdatePrefabTemplate(instanceToCreate->get().GetTemplateId(), serializedInstance); + } + 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."); + + // 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); + undoBatch.GetUndoBatch(), containerEntityId, false); }); + // Create a link between the templates of the newly created instance and the instance it's being parented under. CreateLink( - topLevelEntities, instanceToCreate->get(), commonRootEntityOwningInstance->get().GetTemplateId(), undoBatch.GetUndoBatch(), - commonRootEntityId); + topLevelEntities, instanceToCreate->get(), commonRootEntityOwningInstance->get().GetTemplateId(), + undoBatch.GetUndoBatch(), commonRootEntityId); - // Change top level entities to be parented to the container entity - // Mark them as dirty so this change is correctly applied to the template for (AZ::Entity* topLevelEntity : topLevelEntities) { AZ::EntityId topLevelEntityId = topLevelEntity->GetId(); if (topLevelEntityId.IsValid()) { - m_prefabUndoCache.UpdateCache(topLevelEntityId); - undoBatch.MarkEntityDirty(topLevelEntityId); - AZ::TransformBus::Event(topLevelEntityId, &AZ::TransformBus::Events::SetParent, containerEntityId); + m_prefabUndoCache.UpdateCache(topLevelEntity->GetId()); + + // Parenting entities would mark entities as dirty. But we want to unmark the top level entities as dirty because + // if we don't, the template created would be updated and cause issues with undo operation followed by instantiation. + ToolsApplicationRequests::Bus::Broadcast( + &ToolsApplicationRequests::Bus::Events::RemoveDirtyEntity, topLevelEntity->GetId()); } } @@ -296,7 +315,7 @@ namespace AzToolsFramework void PrefabPublicHandler::CreateLink( const EntityList& topLevelEntities, Instance& sourceInstance, TemplateId targetTemplateId, - UndoSystem::URSequencePoint* undoBatch, AZ::EntityId commonRootEntityId) + UndoSystem::URSequencePoint* undoBatch, AZ::EntityId commonRootEntityId, const bool isUndoRedoSupportNeeded) { AZ::EntityId containerEntityId = sourceInstance.GetContainerEntityId(); AZ::Entity* containerEntity = GetEntityById(containerEntityId); @@ -322,9 +341,19 @@ namespace AzToolsFramework m_instanceToTemplateInterface->GeneratePatch(patch, containerEntityDomBefore, containerEntityDomAfter); m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(patch, containerEntityId); - LinkId linkId = PrefabUndoHelpers::CreateLink( - sourceInstance.GetTemplateId(), targetTemplateId, patch, sourceInstance.GetInstanceAlias(), - undoBatch); + LinkId linkId; + if (isUndoRedoSupportNeeded) + { + linkId = PrefabUndoHelpers::CreateLink( + sourceInstance.GetTemplateId(), targetTemplateId, AZStd::move(patch), sourceInstance.GetInstanceAlias(), undoBatch); + } + else + { + linkId = m_prefabSystemComponentInterface->CreateLink( + targetTemplateId, sourceInstance.GetTemplateId(), sourceInstance.GetInstanceAlias(), patch, + InvalidLinkId); + m_prefabSystemComponentInterface->PropagateTemplateChanges(targetTemplateId); + } sourceInstance.SetLinkId(linkId); @@ -357,7 +386,7 @@ namespace AzToolsFramework patchesCopyForUndoSupport.CopyFrom(nestedInstanceLinkPatches->get(), patchesCopyForUndoSupport.GetAllocator()); PrefabUndoHelpers::RemoveLink( sourceInstance->GetTemplateId(), targetTemplateId, sourceInstance->GetInstanceAlias(), sourceInstance->GetLinkId(), - patchesCopyForUndoSupport, undoBatch); + AZStd::move(patchesCopyForUndoSupport), undoBatch); } PrefabOperationResult PrefabPublicHandler::SavePrefab(AZ::IO::Path filePath) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index 03b3827328..138bc84aa0 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -77,10 +77,11 @@ namespace AzToolsFramework * \param targetInstance The id of the target template. * \param undoBatch The undo batch to set as parent for this create link action. * \param commonRootEntityId The id of the entity that the source instance should be parented under. + * \param isUndoRedoSupportNeeded The flag indicating whether the link should be created with undo/redo support or not. */ void CreateLink( const EntityList& topLevelEntities, Instance& sourceInstance, TemplateId targetTemplateId, - UndoSystem::URSequencePoint* undoBatch, AZ::EntityId commonRootEntityId); + UndoSystem::URSequencePoint* undoBatch, AZ::EntityId commonRootEntityId, const bool isUndoRedoSupportNeeded = true); /** * Removes the link between template of the sourceInstance and the template corresponding to targetTemplateId. diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp index 3910e80f75..c4e6415b02 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp @@ -583,7 +583,7 @@ namespace AzToolsFramework const TemplateId& linkTargetId, const TemplateId& linkSourceId, const InstanceAlias& instanceAlias, - const PrefabDomReference linkPatch, + const PrefabDomConstReference linkPatches, const LinkId& linkId) { if (linkTargetId == InvalidTemplateId) @@ -667,9 +667,9 @@ namespace AzToolsFramework rapidjson::StringRef(PrefabDomUtils::SourceName), rapidjson::StringRef(sourceTemplate.GetFilePath().c_str()), newLink.GetLinkDom().GetAllocator()); - if (linkPatch && linkPatch->get().IsArray() && !(linkPatch->get().Empty())) + if (linkPatches && linkPatches->get().IsArray() && !(linkPatches->get().Empty())) { - m_instanceToTemplatePropagator.AddPatchesToLink(linkPatch.value(), newLink); + m_instanceToTemplatePropagator.AddPatchesToLink(linkPatches.value(), newLink); } //update the target template dom to have the proper values for the source template dom diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h index 30d03ba2ef..0a9a450f64 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.h @@ -156,7 +156,7 @@ namespace AzToolsFramework const TemplateId& linkTargetId, const TemplateId& linkSourceId, const InstanceAlias& instanceAlias, - const PrefabDomReference linkPatch, + const PrefabDomConstReference linkPatches, const LinkId& linkId = InvalidLinkId) override; /** diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h index 6491d67401..f47941254a 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponentInterface.h @@ -43,9 +43,9 @@ namespace AzToolsFramework PrefabDomValue::MemberIterator& instanceIterator, InstanceOptionalReference instance) = 0; //creates a new Link - virtual LinkId CreateLink(const TemplateId& linkTargetId, const TemplateId& linkSourceId, - const InstanceAlias& instanceAlias, const PrefabDomReference linkPatch, - const LinkId& linkId = InvalidLinkId) = 0; + virtual LinkId CreateLink( + const TemplateId& linkTargetId, const TemplateId& linkSourceId, const InstanceAlias& instanceAlias, + const PrefabDomConstReference linkPatches, const LinkId& linkId = InvalidLinkId) = 0; virtual void RemoveLink(const LinkId& linkId) = 0; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.cpp index ad0cdc166a..aadcdcdea0 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.cpp @@ -124,7 +124,7 @@ namespace AzToolsFramework const TemplateId& targetId, const TemplateId& sourceId, const InstanceAlias& instanceAlias, - PrefabDomReference linkPatches, + PrefabDom linkPatches, const LinkId linkId) { m_targetId = targetId; @@ -132,10 +132,7 @@ namespace AzToolsFramework m_instanceAlias = instanceAlias; m_linkId = linkId; - if (linkPatches.has_value()) - { - m_linkPatches = AZStd::move(linkPatches->get()); - } + m_linkPatches = AZStd::move(linkPatches); //if linkId is invalid, set as ADD if (m_linkId == InvalidLinkId) @@ -228,7 +225,7 @@ namespace AzToolsFramework if (link.has_value()) { - m_linkDomPrevious = AZStd::move(link->get().GetLinkDom()); + m_linkDomPrevious.CopyFrom(link->get().GetLinkDom(), m_linkDomPrevious.GetAllocator()); } //get source templateDom @@ -275,7 +272,7 @@ namespace AzToolsFramework if (patchesIter == m_linkDomNext.MemberEnd()) { m_linkDomNext.AddMember( - rapidjson::GenericStringRef(PrefabDomUtils::PatchesName), patchLinkCopy, m_linkDomNext.GetAllocator()); + rapidjson::GenericStringRef(PrefabDomUtils::PatchesName), AZStd::move(patchLinkCopy), m_linkDomNext.GetAllocator()); } else { @@ -303,9 +300,7 @@ namespace AzToolsFramework return; } - PrefabDom moveLink; - moveLink.CopyFrom(linkDom, linkDom.GetAllocator()); - link->get().GetLinkDom() = AZStd::move(moveLink); + link->get().SetLinkDom(linkDom); //propagate the link changes link->get().UpdateTarget(); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.h index 49bae4eda0..33d9e5ad33 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndo.h @@ -101,7 +101,7 @@ namespace AzToolsFramework const TemplateId& targetId, const TemplateId& sourceId, const InstanceAlias& instanceAlias, - PrefabDomReference linkPatches = PrefabDomReference(), + PrefabDom linkPatches = PrefabDom(), const LinkId linkId = InvalidLinkId); void Undo() override; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.cpp index 24eb71e055..5ba8d3c940 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.cpp @@ -34,11 +34,11 @@ namespace AzToolsFramework } LinkId CreateLink( - TemplateId sourceTemplateId, TemplateId targetTemplateId, PrefabDomReference patch, + TemplateId sourceTemplateId, TemplateId targetTemplateId, PrefabDom patch, const InstanceAlias& instanceAlias, UndoSystem::URSequencePoint* undoBatch) { auto linkAddUndo = aznew PrefabUndoInstanceLink("Create Link"); - linkAddUndo->Capture(targetTemplateId, sourceTemplateId, instanceAlias, patch, InvalidLinkId); + linkAddUndo->Capture(targetTemplateId, sourceTemplateId, instanceAlias, AZStd::move(patch), InvalidLinkId); linkAddUndo->SetParent(undoBatch); linkAddUndo->Redo(); @@ -47,10 +47,10 @@ namespace AzToolsFramework void RemoveLink( TemplateId sourceTemplateId, TemplateId targetTemplateId, const InstanceAlias& instanceAlias, LinkId linkId, - PrefabDomReference linkPatches, UndoSystem::URSequencePoint* undoBatch) + PrefabDom linkPatches, UndoSystem::URSequencePoint* undoBatch) { auto linkRemoveUndo = aznew PrefabUndoInstanceLink("Remove Link"); - linkRemoveUndo->Capture(targetTemplateId, sourceTemplateId, instanceAlias, linkPatches, linkId); + linkRemoveUndo->Capture(targetTemplateId, sourceTemplateId, instanceAlias, AZStd::move(linkPatches), linkId); linkRemoveUndo->SetParent(undoBatch); linkRemoveUndo->Redo(); } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.h index 6429df3b04..17cf7f52c1 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoHelpers.h @@ -22,11 +22,11 @@ namespace AzToolsFramework const Instance& instance, AZStd::string_view undoMessage, const PrefabDom& instanceDomBeforeUpdate, UndoSystem::URSequencePoint* undoBatch); LinkId CreateLink( - TemplateId sourceTemplateId, TemplateId targetTemplateId, PrefabDomReference patch, + TemplateId sourceTemplateId, TemplateId targetTemplateId, PrefabDom patch, const InstanceAlias& instanceAlias, UndoSystem::URSequencePoint* undoBatch); void RemoveLink( TemplateId sourceTemplateId, TemplateId targetTemplateId, const InstanceAlias& instanceAlias, LinkId linkId, - PrefabDomReference linkPatches, UndoSystem::URSequencePoint* undoBatch); + PrefabDom linkPatches, UndoSystem::URSequencePoint* undoBatch); } } // namespace Prefab } // namespace AzToolsFramework diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUndoLinkTests.cpp b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUndoLinkTests.cpp index 2122b116d8..3d540a7a95 100644 --- a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUndoLinkTests.cpp +++ b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUndoLinkTests.cpp @@ -120,7 +120,7 @@ namespace UnitTest //create an undo node to apply the patch and prep for undo PrefabUndoInstanceLink undoInstanceLinkNode("Undo Link Patch"); - undoInstanceLinkNode.Capture(rootTemplateId, nestedTemplateId, aliases[0], patch, InvalidLinkId); + undoInstanceLinkNode.Capture(rootTemplateId, nestedTemplateId, aliases[0], AZStd::move(patch), InvalidLinkId); undoInstanceLinkNode.Redo(); m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue(); @@ -196,7 +196,7 @@ namespace UnitTest //create an undo node to apply the patch and prep for undo PrefabUndoInstanceLink undoInstanceLinkNode("Undo Link Patch"); - undoInstanceLinkNode.Capture(rootTemplateId, nestedTemplateId, aliases[0], linkPatch, InvalidLinkId); + undoInstanceLinkNode.Capture(rootTemplateId, nestedTemplateId, aliases[0], AZStd::move(linkPatch), InvalidLinkId); undoInstanceLinkNode.Redo(); m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue();