LYN-1818 | [USE CASE] Reparenting between different prefab instances by drag/drop in the Outliner (#1088)

* Add the last known parent to the prefab undo cache to detect changes in the owning instance.
Still WIP.

* Progress in handling reparenting. Still WIP, need a change in CreateLink that will be addressed in a separate branch and then merged back.

* A few fixes, reparenting now works with entities. Still working on instances.

* Fix assert crashing the Editor because of the arguments being in the wrong order.

* Handle moving the patches when removing and recreating links when reparenting nested instances.

* Rearrange some code to prevent including instance removal in instance update undo node, as it would be redundant and cause errors in some edge cases.

* Reorder instance reparenting to account for correct order of operation during undo/redo

* Fix order of operations to support multiple operations in one edit (reparenting to non-container entities while changing instance)

* Add function to refresh patches on links to allow aliases to be restored correctly on reparenting.

* Removed RefreshEntityPatchOnLink function. Introduced a simpler way of handling porting patches.

* Removing unnecessary code that was left after testing.

* Minor fixes to naming and comments.

* Restore previous error, no longer printing the failed patch.

* Remove unused includes.

* Restore include removed by mistake.

* Simplified patches retrieval by using internal function. Renamed some internal functions and variables to be more accurate.

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp

@@ -297,7 +297,7 @@ namespace AzToolsFramework
             m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(patch, containerEntityId);
 
             // Update the cache - this prevents these changes from being stored in the regular undo/redo nodes
-            m_prefabUndoCache.Store(containerEntityId, AZStd::move(containerEntityDomAfter));
+            m_prefabUndoCache.Store(containerEntityId, AZStd::move(containerEntityDomAfter), parentEntityId);
 
             return AZStd::move(patch);
         }
@@ -595,53 +595,198 @@ namespace AzToolsFramework
         {
             // Create Undo node on entities if they belong to an instance
             InstanceOptionalReference owningInstance = m_instanceEntityMapperInterface->FindOwningInstance(entityId);
+            if (!owningInstance.has_value())
+            {
+                return;
+            }
 
-            if (owningInstance.has_value())
+            AZ::Entity* entity = GetEntityById(entityId);
+            if (!entity)
             {
-                PrefabDom afterState;
-                AZ::Entity* entity = GetEntityById(entityId);
-                if (entity)
+                m_prefabUndoCache.PurgeCache(entityId);
+                return;
+            }
+
+            PrefabDom beforeState;
+            AZ::EntityId beforeParentId;
+            m_prefabUndoCache.Retrieve(entityId, beforeState, beforeParentId);
+
+            PrefabDom afterState;
+            AZ::EntityId afterParentId;
+            AZ::TransformBus::EventResult(afterParentId, entityId, &AZ::TransformBus::Events::GetParentId);
+
+            m_instanceToTemplateInterface->GenerateDomForEntity(afterState, *entity);
+
+            PrefabDom patch;
+            m_instanceToTemplateInterface->GeneratePatch(patch, beforeState, afterState);
+            m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(patch, entityId);
+
+            if (patch.IsArray() && !patch.Empty() && beforeState.IsObject())
+            {
+                bool isInstanceContainerEntity = IsInstanceContainerEntity(entityId) && !IsLevelInstanceContainerEntity(entityId);
+                bool isNewParentOwnedByDifferentInstance = false;
+
+                if (beforeParentId != afterParentId)
                 {
-                    PrefabDom beforeState;
-                    m_prefabUndoCache.Retrieve(entityId, beforeState);
+                    // If the entity parent changed, verify if the owning instance changed too
+                    InstanceOptionalReference beforeOwningInstance = m_instanceEntityMapperInterface->FindOwningInstance(beforeParentId);
+                    InstanceOptionalReference afterOwningInstance = m_instanceEntityMapperInterface->FindOwningInstance(afterParentId);
+
+                    if (beforeOwningInstance.has_value() && afterOwningInstance.has_value() &&
+                        (&beforeOwningInstance->get() != &afterOwningInstance->get()))
+                    {
+                        isNewParentOwnedByDifferentInstance = true;
+                    }
+                }
+
+                if (isInstanceContainerEntity)
+                {
+                    if (isNewParentOwnedByDifferentInstance)
+                    {
+                        Internal_HandleInstanceChange(parentUndoBatch, entity, beforeParentId, afterParentId);
 
-                    m_instanceToTemplateInterface->GenerateDomForEntity(afterState, *entity);
+                        PrefabDom afterStateafterReparenting;
+                        m_instanceToTemplateInterface->GenerateDomForEntity(afterStateafterReparenting, *entity);
 
-                    PrefabDom patch;
-                    m_instanceToTemplateInterface->GeneratePatch(patch, beforeState, afterState);
+                        PrefabDom newPatch;
+                        m_instanceToTemplateInterface->GeneratePatch(newPatch, afterState, afterStateafterReparenting);
+                        m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(newPatch, entityId);
 
-                    if (patch.IsArray() && !patch.Empty() && beforeState.IsObject())
+                        InstanceOptionalReference owningInstanceAfterReparenting =
+                            m_instanceEntityMapperInterface->FindOwningInstance(entityId);
+
+                        Internal_HandleContainerOverride(
+                            parentUndoBatch, entityId, newPatch, owningInstanceAfterReparenting->get().GetLinkId());
+                    }
+                    else
                     {
-                        if (IsInstanceContainerEntity(entityId) && !IsLevelInstanceContainerEntity(entityId))
-                        {
-                            m_instanceToTemplateInterface->AppendEntityAliasToPatchPaths(patch, entityId);
+                        Internal_HandleContainerOverride(
+                            parentUndoBatch, entityId, patch, owningInstance->get().GetLinkId());
+                    }
+                }
+                else
+                {
+                    Internal_HandleEntityChange(parentUndoBatch, entityId, beforeState, afterState);
+
+                    if (isNewParentOwnedByDifferentInstance)
+                    {
+                        Internal_HandleInstanceChange(parentUndoBatch, entity, beforeParentId, afterParentId);
+                    }
+                }
+            }
 
-                            // Save these changes as patches to the link
-                            PrefabUndoLinkUpdate* linkUpdate =
-                                aznew PrefabUndoLinkUpdate(AZStd::to_string(static_cast<AZ::u64>(entityId)));
-                            linkUpdate->SetParent(parentUndoBatch);
-                            linkUpdate->Capture(patch, owningInstance->get().GetLinkId());
+            m_prefabUndoCache.UpdateCache(entityId);
+        }
 
-                            linkUpdate->Redo();
-                        }
-                        else
-                        {
-                            // Update the state of the entity
-                            PrefabUndoEntityUpdate* state = aznew PrefabUndoEntityUpdate(AZStd::to_string(static_cast<AZ::u64>(entityId)));
-                            state->SetParent(parentUndoBatch);
-                            state->Capture(beforeState, afterState, entityId);
+        void PrefabPublicHandler::Internal_HandleContainerOverride(
+            UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, const PrefabDom& patch, const LinkId linkId)
+        {
+            // Save these changes as patches to the link
+            PrefabUndoLinkUpdate* linkUpdate = aznew PrefabUndoLinkUpdate(AZStd::to_string(static_cast<AZ::u64>(entityId)));
+            linkUpdate->SetParent(undoBatch);
+            linkUpdate->Capture(patch, linkId);
 
-                            state->Redo();
+            linkUpdate->Redo();
+        }
+
+        void PrefabPublicHandler::Internal_HandleEntityChange(
+            UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, PrefabDom& beforeState, PrefabDom& afterState)
+        {
+            // Update the state of the entity
+            PrefabUndoEntityUpdate* state = aznew PrefabUndoEntityUpdate(AZStd::to_string(static_cast<AZ::u64>(entityId)));
+            state->SetParent(undoBatch);
+            state->Capture(beforeState, afterState, entityId);
+
+            state->Redo();
+        }
+
+        void PrefabPublicHandler::Internal_HandleInstanceChange(
+            UndoSystem::URSequencePoint* undoBatch, AZ::Entity* entity, AZ::EntityId beforeParentId, AZ::EntityId afterParentId)
+        {
+            // If the entity parent changed, verify if the owning instance changed too
+            InstanceOptionalReference beforeOwningInstance = m_instanceEntityMapperInterface->FindOwningInstance(beforeParentId);
+            InstanceOptionalReference afterOwningInstance = m_instanceEntityMapperInterface->FindOwningInstance(afterParentId);
+
+            EntityList entities;
+            AZStd::vector<Instance*> instances;
+
+            // Retrieve all descendant entities and instances of this entity that belonged to the same owning instance.
+            RetrieveAndSortPrefabEntitiesAndInstances({ entity }, beforeOwningInstance->get(), entities, instances);
+
+            AZStd::vector<AZStd::unique_ptr<Instance>> instanceUniquePtrs;
+            AZStd::vector<AZStd::pair<Instance*, PrefabDom>> instancePatches;
+
+            // Remove Entities and Instances from the prior instance
+            {
+                // Remove Instances
+                for (Instance* nestedInstance : instances)
+                {
+                    auto linkRef = m_prefabSystemComponentInterface->FindLink(nestedInstance->GetLinkId());
+
+                    PrefabDom oldLinkPatches;
+
+                    if (linkRef.has_value())
+                    {
+                        auto patches = linkRef->get().GetLinkPatches();
+                        if (patches.has_value())
+                        {
+                            oldLinkPatches.CopyFrom(patches->get(), oldLinkPatches.GetAllocator());
                         }
                     }
 
-                    // Update the cache
-                    m_prefabUndoCache.Store(entityId, AZStd::move(afterState));
+                    auto nestedInstanceUniquePtr = beforeOwningInstance->get().DetachNestedInstance(nestedInstance->GetInstanceAlias());
+                    RemoveLink(nestedInstanceUniquePtr, beforeOwningInstance->get().GetTemplateId(), undoBatch);
+
+                    instancePatches.emplace_back(AZStd::make_pair(nestedInstanceUniquePtr.get(), AZStd::move(oldLinkPatches)));
+                    instanceUniquePtrs.emplace_back(AZStd::move(nestedInstanceUniquePtr));
                 }
-                else
+
+                // Get the previous state of the prior instance for undo/redo purposes
+                PrefabDom beforeInstanceDomBeforeRemoval;
+                m_instanceToTemplateInterface->GenerateDomForInstance(beforeInstanceDomBeforeRemoval, beforeOwningInstance->get());
+
+                // Remove Entities
+                for (AZ::Entity* nestedEntity : entities)
+                {
+                    beforeOwningInstance->get().DetachEntity(nestedEntity->GetId()).release();
+                }
+
+                // Create the Update node for the prior owning instance
+                // Instance removal will be taken care of from the RemoveLink function for undo/redo purposes
+                PrefabUndoHelpers::UpdatePrefabInstance(
+                    beforeOwningInstance->get(), "Update prior prefab instance", beforeInstanceDomBeforeRemoval, undoBatch);
+            }
+
+            // Add Entities and Instances to new instance
+            {
+                // Add Instances
+                for (auto& instanceUniquePtr : instanceUniquePtrs)
                 {
-                    m_prefabUndoCache.PurgeCache(entityId);
+                    afterOwningInstance->get().AddInstance(AZStd::move(instanceUniquePtr));
                 }
+
+                // Create Links
+                for (auto& instanceInfo : instancePatches)
+                {
+                    // Add a new link with the old dom
+                    CreateLink(
+                        *instanceInfo.first, afterOwningInstance->get().GetTemplateId(), undoBatch,
+                        AZStd::move(instanceInfo.second));
+                }
+
+                // Get the previous state of the new instance for undo/redo purposes
+                PrefabDom afterInstanceDomBeforeAdd;
+                m_instanceToTemplateInterface->GenerateDomForInstance(afterInstanceDomBeforeAdd, afterOwningInstance->get());
+
+                // Add Entities
+                for (AZ::Entity* nestedEntity : entities)
+                {
+                    afterOwningInstance->get().AddEntity(*nestedEntity);
+                }
+
+                // Create the Update node for the new owning instance
+                PrefabUndoHelpers::UpdatePrefabInstance(
+                    afterOwningInstance->get(), "Update new prefab instance", afterInstanceDomBeforeAdd, undoBatch);
             }
         }
 

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h

@@ -90,8 +90,8 @@ namespace AzToolsFramework
             /**
              * Creates a link between the templates of an instance and its parent.
              * 
-             * \param sourceInstance The instance that corresponds to the source template of the link.
-             * \param targetInstance The id of the target template.
+             * \param sourceInstance The instance that corresponds to the source template of the link (child).
+             * \param targetInstance The id of the target template (parent).
              * \param undoBatch The undo batch to set as parent for this create link action.
              * \param patch The patch to store in the newly created link dom.
              * \param isUndoRedoSupportNeeded The flag indicating whether the link should be created with undo/redo support or not.
@@ -134,6 +134,12 @@ namespace AzToolsFramework
             bool IsCyclicalDependencyFound(
                 InstanceOptionalConstReference instance, const AZStd::unordered_set<AZ::IO::Path>& templateSourcePaths);
 
+            static void Internal_HandleContainerOverride(
+                UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, const PrefabDom& patch, const LinkId linkId);
+            static void Internal_HandleEntityChange(
+                UndoSystem::URSequencePoint* undoBatch, AZ::EntityId entityId, PrefabDom& beforeState, PrefabDom& afterState);
+            void Internal_HandleInstanceChange(UndoSystem::URSequencePoint* undoBatch, AZ::Entity* entity, AZ::EntityId beforeParentId, AZ::EntityId afterParentId);
+
             void UpdateLinkPatchesWithNewEntityAliases(
                 PrefabDom& linkPatch,
                 const AZStd::unordered_map<AZ::EntityId, AZStd::string>& oldEntityAliases,

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp

@@ -706,14 +706,14 @@ namespace AzToolsFramework
                 "Prefab - PrefabSystemComponent::RemoveLink - "
                 "Failed to remove Link with Id '%llu' for Instance '%s' of source Template with Id '%llu' "
                 "from TemplateToLinkIdsMap.",
-                linkId, link.GetSourceTemplateId(), link.GetInstanceName().c_str());
+                linkId, link.GetInstanceName().c_str(), link.GetSourceTemplateId());
 
             result = RemoveLinkFromTargetTemplate(linkId, link);
             AZ_Assert(result,
                 "Prefab - PrefabSystemComponent::RemoveLink - "
                 "Failed to remove Link with Id '%llu' for Instance '%s' of source Template with Id '%llu' "
                 "from target Template with Id '%llu'.",
-                linkId, link.GetSourceTemplateId(), link.GetInstanceName().c_str(), link.GetTargetTemplateId());
+                linkId, link.GetInstanceName().c_str(), link.GetSourceTemplateId(), link.GetTargetTemplateId());
 
             m_linkIdMap.erase(linkId);
 

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoCache.cpp

@@ -73,14 +73,16 @@ namespace AzToolsFramework
             }
 
             PrefabDom oldData;
-            Retrieve(entityId, oldData);
+            AZ::EntityId oldParentId;
+            Retrieve(entityId, oldData, oldParentId);
 
             UpdateCache(entityId);
 
             PrefabDom newData;
-            Retrieve(entityId, newData);
+            AZ::EntityId newParentId;
+            Retrieve(entityId, newData, newParentId);
 
-            if (newData != oldData)
+            if (newData != oldData || oldParentId != newParentId)
             {
                 // display a useful message
                 AZ::Entity* entity = nullptr;
@@ -106,7 +108,7 @@ namespace AzToolsFramework
             // Clear out newly generated data and
             // replace with original data to ensure debug mode has the same data as profile/release
             // in the event of the consistency check failing.
-            m_entitySavedStates[entityId] = AZStd::move(oldData);
+            m_entitySavedStates[entityId] = {AZStd::move(oldData), oldParentId};
 
 #endif // ENABLE_UNDOCACHE_CONSISTENCY_CHECKS
         }
@@ -140,10 +142,13 @@ namespace AzToolsFramework
                 return;
             }
 
+            AZ::EntityId parentId;
+            AZ::TransformBus::EventResult(parentId, entityId, &AZ::TransformBus::Events::GetParentId);
+
             // Capture it
             PrefabDom entityDom;
             m_instanceToTemplateInterface->GenerateDomForEntity(entityDom, *entity);
-            m_entitySavedStates.emplace(AZStd::make_pair(entityId, AZStd::move(entityDom)));
+            m_entitySavedStates[entityId] = {AZStd::move(entityDom), parentId};
 
             AZLOG("Prefab Undo", "Correctly updated cache for entity of id %llu (%s)", static_cast<AZ::u64>(entityId), entity->GetName().c_str());
 
@@ -155,7 +160,7 @@ namespace AzToolsFramework
             m_entitySavedStates.erase(entityId);
         }
 
-        bool PrefabUndoCache::Retrieve(const AZ::EntityId& entityId, PrefabDom& outDom)
+        bool PrefabUndoCache::Retrieve(const AZ::EntityId& entityId, PrefabDom& outDom, AZ::EntityId& parentId)
         {
             auto it = m_entitySavedStates.find(entityId);
 
@@ -164,14 +169,15 @@ namespace AzToolsFramework
                 return false;
             }
 
-            outDom = AZStd::move(m_entitySavedStates[entityId]);
+            outDom = AZStd::move(m_entitySavedStates[entityId].dom);
+            parentId = m_entitySavedStates[entityId].parentId;
             m_entitySavedStates.erase(entityId);
             return true;
         }
 
-        void PrefabUndoCache::Store(const AZ::EntityId& entityId, PrefabDom&& dom)
+        void PrefabUndoCache::Store(const AZ::EntityId& entityId, PrefabDom&& dom, const AZ::EntityId& parentId)
         {
-            m_entitySavedStates.emplace(AZStd::make_pair(entityId, AZStd::move(dom)));
+            m_entitySavedStates[entityId] = {AZStd::move(dom), parentId};
         }
 
         void PrefabUndoCache::Clear()

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabUndoCache.h

@@ -46,14 +46,19 @@ namespace AzToolsFramework
             void Validate(const AZ::EntityId& entityId) override;
 
             // Retrieve the last known state for an entity
-            bool Retrieve(const AZ::EntityId& entityId, PrefabDom& outDom);
+            bool Retrieve(const AZ::EntityId& entityId, PrefabDom& outDom, AZ::EntityId& parentId);
 
             // Store dom as the cached state of entityId
-            void Store(const AZ::EntityId& entityId, PrefabDom&& dom);
+            void Store(const AZ::EntityId& entityId, PrefabDom&& dom, const AZ::EntityId& parentId);
 
         private:
-            typedef AZStd::unordered_map<AZ::EntityId, PrefabDom> EntityDomMap;
-            EntityDomMap m_entitySavedStates;
+            struct PrefabUndoCacheItem
+            {
+                PrefabDom dom;
+                AZ::EntityId parentId;
+            };
+            typedef AZStd::unordered_map<AZ::EntityId, PrefabUndoCacheItem> EntityCache;
+            EntityCache m_entitySavedStates;
 
             InstanceEntityMapperInterface* m_instanceEntityMapperInterface = nullptr;
             InstanceToTemplateInterface* m_instanceToTemplateInterface = nullptr;