Reparenting - introduce loop detection on instance reparenting (DCO fix) (#1752)

* Detect loops in reparenting code and assert.

Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

* Add warning when detecting a cyclical dependancy. Revert reparenting on loop.

Signed-off-by: daimini <82231674+AMZN-daimini@users.noreply.github.com>
Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

* Use GetActiveWindow helper to handle window edge cases

Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

Code/Framework/AzToolsFramework/AzToolsFramework/Application/ToolsApplication.cpp

@@ -45,6 +45,7 @@
 #include <AzToolsFramework/UI/UICore/QTreeViewStateSaver.hxx>
 #include <AzToolsFramework/UI/UICore/QWidgetSavedState.h>
 #include <AzToolsFramework/UI/UICore/ProgressShield.hxx>
+#include <AzToolsFramework/UI/UICore/WidgetHelpers.h>
 #include <AzToolsFramework/Slice/SliceUtilities.h>
 #include <AzToolsFramework/ToolsComponents/EditorInspectorComponent.h>
 #include <AzToolsFramework/API/ComponentEntityObjectBus.h>
@@ -1589,7 +1590,13 @@ namespace AzToolsFramework
                 // Multiple changes to the same entity are just split between different undo nodes.
                 for (AZ::EntityId entityId : m_dirtyEntities)
                 {
-                    prefabPublicInterface->GenerateUndoNodesForEntityChangeAndUpdateCache(entityId, m_currentBatchUndo);
+                    auto outcome = prefabPublicInterface->GenerateUndoNodesForEntityChangeAndUpdateCache(entityId, m_currentBatchUndo);
+
+                    if (!outcome.IsSuccess())
+                    {
+                        QMessageBox::warning(
+                            AzToolsFramework::GetActiveWindow(), QString("Error"), QString(outcome.GetError().c_str()), QMessageBox::Ok, QMessageBox::Ok);
+                    }
                 }
             }
         }

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

@@ -587,21 +587,21 @@ namespace AzToolsFramework
             return AZ::Success(entityId);
         }
 
-        void PrefabPublicHandler::GenerateUndoNodesForEntityChangeAndUpdateCache(
+        PrefabOperationResult PrefabPublicHandler::GenerateUndoNodesForEntityChangeAndUpdateCache(
             AZ::EntityId entityId, UndoSystem::URSequencePoint* parentUndoBatch)
         {
             // Create Undo node on entities if they belong to an instance
             InstanceOptionalReference owningInstance = m_instanceEntityMapperInterface->FindOwningInstance(entityId);
             if (!owningInstance.has_value())
             {
-                return;
+                return AZ::Success();
             }
 
             AZ::Entity* entity = GetEntityById(entityId);
             if (!entity)
             {
                 m_prefabUndoCache.PurgeCache(entityId);
-                return;
+                return AZ::Success();
             }
 
             PrefabDom beforeState;
@@ -633,6 +633,41 @@ namespace AzToolsFramework
                         (&beforeOwningInstance->get() != &afterOwningInstance->get()))
                     {
                         isNewParentOwnedByDifferentInstance = true;
+
+                        // Detect loops. Assert if an instance has been reparented in such a way to generate circular dependencies.
+                        AZStd::vector<Instance*> instancesInvolved;
+
+                        if (isInstanceContainerEntity)
+                        {
+                            instancesInvolved.push_back(&owningInstance->get());
+                        }
+                        else
+                        {
+                            // Retrieve all nested instances that are part of the subtree under the current entity.
+                            EntityList entities;
+                            RetrieveAndSortPrefabEntitiesAndInstances({ entity }, beforeOwningInstance->get(), entities, instancesInvolved);
+                        }
+
+                        for (Instance* instance : instancesInvolved)
+                        {
+                            const PrefabDom& templateDom =
+                                m_prefabSystemComponentInterface->FindTemplateDom(instance->GetTemplateId());
+                            AZStd::unordered_set<AZ::IO::Path> templatePaths;
+                            PrefabDomUtils::GetTemplateSourcePaths(templateDom, templatePaths);
+
+                            if (IsCyclicalDependencyFound(afterOwningInstance->get(), templatePaths))
+                            {
+                                // Cancel the operation by restoring the previous parent
+                                AZ::TransformBus::Event(entityId, &AZ::TransformBus::Events::SetParent, beforeParentId);
+                                m_prefabUndoCache.UpdateCache(entityId);
+
+                                // Skip the creation of an undo node
+                                return AZ::Failure(AZStd::string::format(
+                                    "Reparent Prefab operation aborted - Cyclical dependency detected\n(%s depends on %s).",
+                                    instance->GetTemplateSourcePath().Native().c_str(),
+                                    afterOwningInstance->get().GetTemplateSourcePath().Native().c_str()));
+                            }
+                        }
                     }
                 }
 
@@ -673,6 +708,8 @@ namespace AzToolsFramework
             }
 
             m_prefabUndoCache.UpdateCache(entityId);
+
+            return AZ::Success();
         }
 
         void PrefabPublicHandler::Internal_HandleContainerOverride(

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

@@ -46,7 +46,7 @@ namespace AzToolsFramework
             PrefabOperationResult SavePrefab(AZ::IO::Path filePath) override;
             PrefabEntityResult CreateEntity(AZ::EntityId parentId, const AZ::Vector3& position) override;
             
-            void GenerateUndoNodesForEntityChangeAndUpdateCache(AZ::EntityId entityId, UndoSystem::URSequencePoint* parentUndoBatch) override;
+            PrefabOperationResult GenerateUndoNodesForEntityChangeAndUpdateCache(AZ::EntityId entityId, UndoSystem::URSequencePoint* parentUndoBatch) override;
 
             bool IsInstanceContainerEntity(AZ::EntityId entityId) const override;
             bool IsLevelInstanceContainerEntity(AZ::EntityId entityId) const override;

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicInterface.h

@@ -79,8 +79,9 @@ namespace AzToolsFramework
              * 
              * @param entityId The entity to patch.
              * @param parentUndoBatch The undo batch the undo nodes should be parented to.
+             * @return Returns Success if the node was generated correctly, or an error message otherwise.
              */
-            virtual void GenerateUndoNodesForEntityChangeAndUpdateCache(
+            virtual PrefabOperationResult GenerateUndoNodesForEntityChangeAndUpdateCache(
                 AZ::EntityId entityId, UndoSystem::URSequencePoint* parentUndoBatch) = 0;
             
             /**