Merge pull request #675 from aws-lumberyard-dev/Prefab/CreatePrefab

Get instantiation to work after creation of a prefab is undone

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;
         }
 

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());
             }
         }
     }

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:
 

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomTypes.h

@@ -27,6 +27,7 @@ namespace AzToolsFramework
         using PrefabDomList = AZStd::vector<PrefabDom>;
 
         using PrefabDomReference = AZStd::optional<AZStd::reference_wrapper<PrefabDom>>;
+        using PrefabDomConstReference = AZStd::optional<AZStd::reference_wrapper<const PrefabDom>>;
         using PrefabDomValueReference = AZStd::optional<AZStd::reference_wrapper<PrefabDomValue>>;
         using PrefabDomValueConstReference = AZStd::optional<AZStd::reference_wrapper<const PrefabDomValue>>;
 

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<Instance>& 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)

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.

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

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;
 
             /**

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;
 

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();

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;

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();
             }

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

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();