Merge pull request #5297 from aws-lumberyard-dev/nvsickle/FixEntityOrdering

Fix Entity Outliner sort order with Prefabs enabled

AutomatedTesting/Gem/PythonTests/editor/EditorScripts/EntityOutliner_EntityOrdering.py

@@ -0,0 +1,146 @@
+"""
+Copyright (c) Contributors to the Open 3D Engine Project.
+For complete copyright and license terms please see the LICENSE at the root of this distribution.
+
+SPDX-License-Identifier: Apache-2.0 OR MIT
+"""
+
+
+class Tests:
+    entities_sorted = (
+        "Entities sorted in the expected order",
+        "Entities sorted in an incorrect order",
+    )
+
+
+def EntityOutliner_EntityOrdering():
+    """
+    Summary:
+    Verify that manual entity ordering in the entity outliner works and is stable.
+
+    Expected Behavior:
+    Several entities are created, some are manually ordered, and their order
+    is maintained, even when new entities are added.
+
+    Test Steps:
+    1) Open the empty Prefab Base level
+    2) Add 5 entities to the outliner
+    3) Move "Entity1" to the top of the order
+    4) Move "Entity4" to the bottom of the order
+    5) Add another new entity, ensure the rest of the order is unchanged
+    """
+
+    import editor_python_test_tools.pyside_utils as pyside_utils
+    import azlmbr.legacy.general as general
+    from editor_python_test_tools.utils import Report
+    from editor_python_test_tools.utils import TestHelper as helper
+    from PySide2 import QtCore, QtWidgets, QtGui, QtTest
+
+    # Grab the Editor, Entity Outliner, and Outliner Model
+    editor_window = pyside_utils.get_editor_main_window()
+    entity_outliner = pyside_utils.find_child_by_hierarchy(
+        editor_window, ..., "EntityOutlinerWidgetUI", ..., "m_objectTree"
+    )
+    entity_outliner_model = entity_outliner.model()
+
+    # Get the outliner index for the root prefab container entity
+    def get_root_prefab_container_index():
+        return entity_outliner_model.index(0, 0)
+
+    # Get the outliner index for the top level entity of a given name
+    def index_for_name(name):
+        root_index = get_root_prefab_container_index()
+        for row in range(entity_outliner_model.rowCount(root_index)):
+            row_index = entity_outliner_model.index(row, 0, root_index)
+            if row_index.data() == name:
+                return row_index
+        return None
+
+    # Validate that the outliner top level entity order matches the expected order
+    def verify_entities_sorted(expected_order):
+        actual_order = []
+        root_index = get_root_prefab_container_index()
+        for row in range(entity_outliner_model.rowCount(root_index)):
+            row_index = entity_outliner_model.index(row, 0, root_index)
+            actual_order.append(row_index.data())
+
+        sorted_correctly = actual_order == expected_order
+        Report.result(Tests.entities_sorted, sorted_correctly)
+        if not sorted_correctly:
+            print(f"Expected entity order: {expected_order}")
+            print(f"Actual entity order: {actual_order}")
+
+    # Creates an entity from the outliner context menu
+    def create_entity():
+        pyside_utils.trigger_context_menu_entry(
+            entity_outliner, "Create entity", index=get_root_prefab_container_index()
+        )
+        # Wait a tick after entity creation to let events process
+        general.idle_wait(0.0)
+
+    # Moves an entity (wrapped by move_entity_before and move_entity_after)
+    def _move_entity(source_name, target_name, move_after=False):
+        source_index = index_for_name(source_name)
+        target_index = index_for_name(target_name)
+
+        target_row = target_index.row()
+        if move_after:
+            target_row += 1
+
+        # Generate MIME data and directly inject it into the model instead of
+        # generating mouse click operations, as it's more reliable and we're
+        # testing the underlying drag & drop logic as opposed to Qt's mouse
+        # handling here
+        mime_data = entity_outliner_model.mimeData([source_index])
+        entity_outliner_model.dropMimeData(
+            mime_data, QtCore.Qt.MoveAction, target_row, 0, target_index.parent()
+        )
+        QtWidgets.QApplication.processEvents()
+
+    # Move an entity before another entity in the order by dragging the source above the target
+    move_entity_before = lambda source_name, target_name: _move_entity(
+        source_name, target_name, move_after=False
+    )
+    # Move an entity after another entity in the order by dragging the source beloew the target
+    move_entity_after = lambda source_name, target_name: _move_entity(
+        source_name, target_name, move_after=True
+    )
+
+    expected_order = []
+
+    # 1) Open the empty Prefab Base level
+    helper.init_idle()
+    helper.open_level("Prefab", "Base")
+
+    # 2) Add 5 entities to the outliner
+    ENTITIES_TO_ADD = 5
+    for i in range(ENTITIES_TO_ADD):
+        create_entity()
+
+        # Our new entity should be given a name with a number automatically
+        new_entity = f"Entity{i+1}"
+        # The new entity should be added to the top of its parent entity
+        expected_order = [new_entity] + expected_order
+
+        verify_entities_sorted(expected_order)
+
+    # 3) Move "Entity1" to the top of the order
+    move_entity_before("Entity1", "Entity5")
+    expected_order = ["Entity1", "Entity5", "Entity4", "Entity3", "Entity2"]
+    verify_entities_sorted(expected_order)
+
+    # 4) Move "Entity4" to the bottom of the order
+    move_entity_after("Entity4", "Entity2")
+    expected_order = ["Entity1", "Entity5", "Entity3", "Entity2", "Entity4"]
+    verify_entities_sorted(expected_order)
+
+    # 5) Add another new entity, ensure the rest of the order is unchanged
+    create_entity()
+    expected_order = ["Entity6", "Entity1", "Entity5", "Entity3", "Entity2", "Entity4"]
+    verify_entities_sorted(expected_order)
+
+
+if __name__ == "__main__":
+    from editor_python_test_tools.utils import Report
+
+    Report.start_test(EntityOutliner_EntityOrdering)

AutomatedTesting/Gem/PythonTests/editor/TestSuite_Main.py

@@ -41,3 +41,15 @@ class TestAutomation(TestAutomationBase):
         from .EditorScripts import BasicEditorWorkflows_LevelEntityComponentCRUD as test_module
         self._run_test(request, workspace, editor, test_module, batch_mode=False, autotest_mode=False,
                        use_null_renderer=False)
+
+    def test_EntityOutlienr_EntityOrdering(self, request, workspace, editor, launcher_platform):
+        from .EditorScripts import EntityOutliner_EntityOrdering as test_module
+        self._run_test(
+            request, 
+            workspace, 
+            editor, 
+            test_module, 
+            batch_mode=False, 
+            autotest_mode=True,
+            extra_cmdline_args=["--regset=/Amazon/Preferences/EnablePrefabSystem=true"]
+        )

AutomatedTesting/Gem/PythonTests/smoke/test_Editor_NewExistingLevels_Works.py

@@ -28,4 +28,4 @@ class TestAutomation(TestAutomationBase):
 
         from . import Editor_NewExistingLevels_Works as test_module
 
-        self._run_test(request, workspace, editor, test_module)
+        self._run_test(request, workspace, editor, test_module, extra_cmdline_args=["--regset=/Amazon/Preferences/EnablePrefabSystem=false"])

Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityHelpers.cpp

@@ -15,6 +15,7 @@
 #include <AzCore/std/sort.h>
 #include <AzCore/RTTI/AttributeReader.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <AzFramework/API/ApplicationAPI.h>
 #include <AzToolsFramework/Commands/EntityStateCommand.h>
 #include <AzToolsFramework/ContainerEntity/ContainerEntityInterface.h>
 #include <AzToolsFramework/Entity/EditorEntityInfoBus.h>
@@ -468,6 +469,18 @@ namespace AzToolsFramework
         EntityIdList children;
         EditorEntityInfoRequestBus::EventResult(children, parentId, &EditorEntityInfoRequestBus::Events::GetChildren);
 
+        // If Prefabs are enabled, don't check the order for an invalid parent, just return its children (i.e. the root container entity)
+        // There will currently always be one root container entity, so there's no order to retrieve
+        if (!parentId.IsValid())
+        {
+            bool isPrefabEnabled = false;
+            AzFramework::ApplicationRequests::Bus::BroadcastResult(isPrefabEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled);
+            if (isPrefabEnabled)
+            {
+                return children;
+            }
+        }
+
         EntityIdList entityChildOrder;
         AZ::EntityId sortEntityId = GetEntityIdForSortInfo(parentId);
         EditorEntitySortRequestBus::EventResult(entityChildOrder, sortEntityId, &EditorEntitySortRequestBus::Events::GetChildEntityOrderArray);

Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.cpp

@@ -11,6 +11,8 @@
 #include <AzCore/Debug/Profiler.h>
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/std/sort.h>
+#include <AzFramework/API/ApplicationAPI.h>
+#include <AzToolsFramework/Prefab/PrefabPublicInterface.h>
 
 static_assert(sizeof(AZ::u64) == sizeof(AZ::EntityId), "We use AZ::EntityId for Persistent ID, which is a u64 under the hood. These must be the same size otherwise the persistent id will have to be rewritten");
 
@@ -144,6 +146,12 @@ namespace AzToolsFramework
         bool EditorEntitySortComponent::AddChildEntityInternal(const AZ::EntityId& entityId, bool addToBack, EntityOrderArray::iterator insertPosition)
         {
             AZ_PROFILE_FUNCTION(AzToolsFramework);
+
+            if (m_ignoreIncomingOrderChanges)
+            {
+                return true;
+            }
+
             auto entityItr = m_childEntityOrderCache.find(entityId);
             if (entityItr == m_childEntityOrderCache.end())
             {
@@ -198,6 +206,12 @@ namespace AzToolsFramework
         bool EditorEntitySortComponent::RemoveChildEntity(const AZ::EntityId& entityId)
         {
             AZ_PROFILE_FUNCTION(AzToolsFramework);
+
+            if (m_ignoreIncomingOrderChanges)
+            {
+                return true;
+            }
+
             auto entityItr = m_childEntityOrderCache.find(entityId);
             if (entityItr != m_childEntityOrderCache.end())
             {
@@ -250,11 +264,30 @@ namespace AzToolsFramework
             }
         }
 
+        void EditorEntitySortComponent::OnPrefabInstancePropagationBegin()
+        {
+            m_ignoreIncomingOrderChanges = true;
+        }
+
+        void EditorEntitySortComponent::OnPrefabInstancePropagationEnd()
+        {
+            m_ignoreIncomingOrderChanges = false;
+        }
+
         void EditorEntitySortComponent::MarkDirtyAndSendChangedEvent()
         {
             // mark the order as dirty before sending the ChildEntityOrderArrayUpdated event in order for PrepareSave to be properly handled in the case 
             // one of the event listeners needs to build the InstanceDataHierarchy
             m_entityOrderIsDirty = true;
+
+            // Force an immediate update for prefabs, which won't receive PrepareSave
+            bool isPrefabEnabled = false;
+            AzFramework::ApplicationRequests::Bus::BroadcastResult(
+                isPrefabEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled);
+            if (isPrefabEnabled)
+            {
+                PrepareSave();
+            }
             EditorEntitySortNotificationBus::Event(GetEntityId(), &EditorEntitySortNotificationBus::Events::ChildEntityOrderArrayUpdated);
         }
 
@@ -264,10 +297,20 @@ namespace AzToolsFramework
             // This is a special case for certain EditorComponents only!
             EditorEntitySortRequestBus::Handler::BusConnect(GetEntityId());
             EditorEntityContextNotificationBus::Handler::BusConnect();
+            AzToolsFramework::Prefab::PrefabPublicNotificationBus::Handler::BusConnect();
         }
 
         void EditorEntitySortComponent::Activate()
         {
+            // Run the post-serialize handler if prefabs are enabled because PostLoad won't be called automatically
+            bool isPrefabEnabled = false;
+            AzFramework::ApplicationRequests::Bus::BroadcastResult(
+                isPrefabEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled);
+            if (isPrefabEnabled)
+            {
+                PostLoad();
+            }
+
             // Send out that the order for our entity is now updated
             EditorEntitySortNotificationBus::Event(GetEntityId(), &EditorEntitySortNotificationBus::Events::ChildEntityOrderArrayUpdated);
         }

Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntitySortComponent.h

@@ -10,6 +10,7 @@
 #include "EditorEntitySortBus.h"
 #include <AzToolsFramework/ToolsComponents/EditorComponentBase.h>
 #include <AzToolsFramework/Entity/EditorEntityContextBus.h>
+#include <AzToolsFramework/Prefab/PrefabPublicNotificationBus.h>
 #include <AzCore/Serialization/SerializeContext.h>
 
 namespace AzToolsFramework
@@ -20,6 +21,7 @@ namespace AzToolsFramework
             : public AzToolsFramework::Components::EditorComponentBase
             , public EditorEntitySortRequestBus::Handler
             , public EditorEntityContextNotificationBus::Handler
+            , public AzToolsFramework::Prefab::PrefabPublicNotificationBus::Handler
         {
         public:
             AZ_COMPONENT(EditorEntitySortComponent, "{6EA1E03D-68B2-466D-97F7-83998C8C27F0}", EditorComponentBase);
@@ -45,6 +47,9 @@ namespace AzToolsFramework
             // EditorEntityContextNotificationBus::Handler
             void OnEntityStreamLoadSuccess() override;
             //////////////////////////////////////////////////////////////////////////
+
+            void OnPrefabInstancePropagationBegin() override;
+            void OnPrefabInstancePropagationEnd() override;
         private:
             void MarkDirtyAndSendChangedEvent();
             bool AddChildEntityInternal(const AZ::EntityId& entityId, bool addToBack, EntityOrderArray::iterator insertPosition);
@@ -106,6 +111,7 @@ namespace AzToolsFramework
             EntityOrderCache m_childEntityOrderCache; ///< The map of entity id to index for quick look up
 
             bool m_entityOrderIsDirty = true; ///< This flag indicates our stored serialization order data is out of date and must be rebuilt before serialization occurs
+            bool m_ignoreIncomingOrderChanges = false; ///< This is set when prefab propagation occurs so that non-authored order changes can be ignored
         };
     }
 } // namespace AzToolsFramework

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

@@ -1144,6 +1144,10 @@ namespace AzToolsFramework
 
             AZ::EntityId firstEntityIdToDelete = entityIdsNoFocusContainer[0];
             InstanceOptionalReference commonOwningInstance = GetOwnerInstanceByEntityId(firstEntityIdToDelete);
+            if (!commonOwningInstance.has_value())
+            {
+                return AZ::Failure(AZStd::string("Cannot delete entities belonging to an invalid instance"));
+            }
 
             // If the first entity id is a container entity id, then we need to mark its parent as the common owning instance because you
             // cannot delete an instance from itself.