Fixing pivot offset when performing undo on Transform component (#6150)

* Fixing undo offest on transform component

Signed-off-by: Mikhail Naumov <mnaumov@amazon.com>

* Fixining possible issue with firing an unnecessary OnTransformChanged event when entity is manually disabled/enabled

Signed-off-by: Mikhail Naumov <mnaumov@amazon.com>

* Adding better comments

Signed-off-by: Mikhail Naumov <mnaumov@amazon.com>

* Unit tests

Signed-off-by: Mikhail Naumov <mnaumov@amazon.com>

* Cherrupicking changes

Signed-off-by: Mikhail Naumov <mnaumov@amazon.com>

Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.cpp

@@ -860,18 +860,20 @@ namespace AzToolsFramework
                     return;
                 }
 
-                bool isDuringUndoRedo = false;
-                EBUS_EVENT_RESULT(isDuringUndoRedo, AzToolsFramework::ToolsApplicationRequests::Bus, IsDuringUndoRedo);
-                if (!isDuringUndoRedo)
+                bool suppressTransformChangedEvent = m_suppressTransformChangedEvent;
+                // temporarily disable calling OnTransformChanged, because CheckApplyCachedWorldTransform is not guaranteed
+                // to call it when m_cachedWorldTransform is identity. We send it manually later.
+                m_suppressTransformChangedEvent = false;
+                // When parent comes online, compute local TM from world TM.
+                CheckApplyCachedWorldTransform(parentTransform->GetWorldTM());
+                if (!m_initialized)
                 {
-                    // When parent comes online, compute local TM from world TM.
-                    CheckApplyCachedWorldTransform(parentTransform->GetWorldTM());
-                }
-                else
-                {
-                    // During undo operations, just apply our local TM.
+                    m_initialized = true;
+                    // If this is the first time this entity is being activated, manually compute OnTransformChanged
+                    // this can occur when either the entity first created or undo/redo command is performed
                     OnTransformChanged(AZ::Transform::Identity(), parentTransform->GetWorldTM());
                 }
+                m_suppressTransformChangedEvent = suppressTransformChangedEvent;
 
                 auto& parentChildIds = GetParentTransformComponent()->m_childrenEntityIds;
                 if (parentChildIds.end() == AZStd::find(parentChildIds.begin(), parentChildIds.end(), GetEntityId()))

Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.h

@@ -242,6 +242,9 @@ namespace AzToolsFramework
             // element is used rather than a data element.
             bool m_addNonUniformScaleButton = false;
 
+            // Used to check whether entity was just created vs manually reactivated. Set true after OnEntityActivated is called the first time.
+            bool m_initialized = false;
+
             // Deprecated
             AZ::InterpolationMode m_interpolatePosition;
             AZ::InterpolationMode m_interpolateRotation;

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp

@@ -11,6 +11,8 @@
 #include <AzCore/Component/TransformBus.h>
 #include <AzToolsFramework/Prefab/PrefabDomUtils.h>
 #include <AzToolsFramework/Prefab/PrefabLoaderInterface.h>
+#include <AzToolsFramework/ToolsComponents/TransformComponent.h>
+#include <AzToolsFramework/Entity/PrefabEditorEntityOwnershipInterface.h>
 #include <Prefab/PrefabTestComponent.h>
 #include <Prefab/PrefabTestDomUtils.h>
 
@@ -50,6 +52,15 @@ namespace UnitTest
 
         GetApplication()->RegisterComponentDescriptor(PrefabTestComponent::CreateDescriptor());
         GetApplication()->RegisterComponentDescriptor(PrefabTestComponentWithUnReflectedTypeMember::CreateDescriptor());
+
+        AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult(
+            m_undoStack, &AzToolsFramework::ToolsApplicationRequestBus::Events::GetUndoStack);
+        AZ_Assert(m_undoStack, "Failed to look up undo stack from tools application");
+    }
+
+    void PrefabTestFixture::TearDownEditorFixtureImpl()
+    {
+        m_undoStack = nullptr;
     }
 
     AZStd::unique_ptr<ToolsTestApplication> PrefabTestFixture::CreateTestApplication()
@@ -57,7 +68,20 @@ namespace UnitTest
         return AZStd::make_unique<PrefabTestToolsApplication>("PrefabTestApplication");
     }
 
-    AZ::Entity* PrefabTestFixture::CreateEntity(const char* entityName, const bool shouldActivate)
+    void PrefabTestFixture::CreateRootPrefab()
+    {
+        auto entityOwnershipService = AZ::Interface<AzToolsFramework::PrefabEditorEntityOwnershipInterface>::Get();
+        ASSERT_TRUE(entityOwnershipService != nullptr);
+        entityOwnershipService->CreateNewLevelPrefab("UnitTestRoot.prefab", "");
+        auto rootEntityReference = entityOwnershipService->GetRootPrefabInstance()->get().GetContainerEntity();
+        ASSERT_TRUE(rootEntityReference.has_value());
+        auto& rootEntity = rootEntityReference->get();
+        rootEntity.Deactivate();
+        rootEntity.CreateComponent<AzToolsFramework::Components::TransformComponent>();
+        rootEntity.Activate();
+    }
+
+    AZ::Entity* PrefabTestFixture::CreateEntity(AZStd::string entityName, const bool shouldActivate)
     {
         // Circumvent the EntityContext system and generate a new entity with a transformcomponent
         AZ::Entity* newEntity = aznew AZ::Entity(entityName);
@@ -71,8 +95,43 @@ namespace UnitTest
         return newEntity;
     }
 
+    AZ::EntityId PrefabTestFixture::CreateEntityUnderRootPrefab(AZStd::string name, AZ::EntityId parentId)
+    {
+        auto createResult = m_prefabPublicInterface->CreateEntity(parentId, AZ::Vector3());
+        AZ_Assert(createResult.IsSuccess(), "Failed to create entity: %s", createResult.GetError().c_str());
+        AZ::EntityId entityId = createResult.GetValue();
+
+        AZ::Entity* entity = nullptr;
+        AZ::ComponentApplicationBus::BroadcastResult(entity, &AZ::ComponentApplicationRequests::FindEntity, entityId);
+
+        entity->Deactivate();
+
+        entity->SetName(name);
+
+        // Normally, in invalid parent ID should automatically parent us to the root prefab, but currently in the unit test
+        // environment entities aren't created with a default transform component, so CreateEntity won't correctly parent.
+        // We get the actual target parent ID here, then create our missing transform component.
+        if (!parentId.IsValid())
+        {
+            auto prefabEditorEntityOwnershipInterface = AZ::Interface<AzToolsFramework::PrefabEditorEntityOwnershipInterface>::Get();
+            parentId = prefabEditorEntityOwnershipInterface->GetRootPrefabInstance()->get().GetContainerEntityId();
+        }
+
+        auto transform = aznew AzToolsFramework::Components::TransformComponent;
+        transform->SetParent(parentId);
+        entity->AddComponent(transform);
+
+        entity->Activate();
+
+        // Update our undo cache entry to include the rename / reparent as one atomic operation.
+        m_prefabPublicInterface->GenerateUndoNodesForEntityChangeAndUpdateCache(entityId, m_undoStack->GetTop());
+        m_prefabSystemComponent->OnSystemTick();
+
+        return entityId;
+    }
+
     void PrefabTestFixture::CompareInstances(const AzToolsFramework::Prefab::Instance& instanceA,
-        const AzToolsFramework::Prefab::Instance& instanceB, bool shouldCompareLinkIds, bool shouldCompareContainerEntities)
+                                             const AzToolsFramework::Prefab::Instance& instanceB, bool shouldCompareLinkIds, bool shouldCompareContainerEntities)
     {
         AzToolsFramework::Prefab::TemplateId templateAId = instanceA.GetTemplateId();
         AzToolsFramework::Prefab::TemplateId templateBId = instanceB.GetTemplateId();
@@ -125,4 +184,22 @@ namespace UnitTest
             EXPECT_EQ(entityInInstance->GetState(), AZ::Entity::State::Active);
         }
     }
+
+    void PrefabTestFixture::ProcessDeferredUpdates()
+    {
+        // Force a prefab propagation for updates that are deferred to the next tick.
+        m_prefabSystemComponent->OnSystemTick();
+    }
+
+    void PrefabTestFixture::Undo()
+    {
+        m_undoStack->Undo();
+        ProcessDeferredUpdates();
+    }
+
+    void PrefabTestFixture::Redo()
+    {
+        m_undoStack->Redo();
+        ProcessDeferredUpdates();
+    }
 }

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h

@@ -49,10 +49,13 @@ namespace UnitTest
         inline static const char* CarPrefabMockFilePath = "SomePathToCar";
 
         void SetUpEditorFixtureImpl() override;
+        void TearDownEditorFixtureImpl() override;
 
         AZStd::unique_ptr<ToolsTestApplication> CreateTestApplication() override;
 
-        AZ::Entity* CreateEntity(const char* entityName, const bool shouldActivate = true);
+        void CreateRootPrefab();
+        AZ::Entity* CreateEntity(AZStd::string entityName, const bool shouldActivate = true);
+        AZ::EntityId CreateEntityUnderRootPrefab(AZStd::string name, AZ::EntityId parentId = AZ::EntityId());
 
         void CompareInstances(const Instance& instanceA, const Instance& instanceB, bool shouldCompareLinkIds = true,
             bool shouldCompareContainerEntities = true);
@@ -62,10 +65,20 @@ namespace UnitTest
         //! Validates that all entities within a prefab instance are in 'Active' state.
         void ValidateInstanceEntitiesActive(Instance& instance);
 
+        // Kicks off any updates scheduled for the next tick
+        virtual void ProcessDeferredUpdates();
+
+        // Performs an undo operation and ensures the tick-scheduled updates happen
+        void Undo();
+
+        // Performs a redo operation and ensures the tick-scheduled updates happen
+        void Redo();
+
         PrefabSystemComponent* m_prefabSystemComponent = nullptr;
         PrefabLoaderInterface* m_prefabLoaderInterface = nullptr;
         PrefabPublicInterface* m_prefabPublicInterface = nullptr;
         InstanceUpdateExecutorInterface* m_instanceUpdateExecutorInterface = nullptr;
         InstanceToTemplateInterface* m_instanceToTemplateInterface = nullptr;
+        AzToolsFramework::UndoSystem::UndoStack* m_undoStack = nullptr;
     };
 }

Code/Framework/AzToolsFramework/Tests/TransformComponent.cpp

@@ -18,7 +18,10 @@
 #include <AzFramework/Components/TransformComponent.h>
 
 #include <AzToolsFramework/Application/ToolsApplication.h>
+#include <AzToolsFramework/UnitTest/AzToolsFrameworkTestHelpers.h>
 #include <AzToolsFramework/ToolsComponents/TransformComponent.h>
+#include <AzToolsFramework/Entity/PrefabEditorEntityOwnershipInterface.h>
+#include <Prefab/PrefabTestFixture.h>
 
 #include <AZTestShared/Math/MathTestHelpers.h>
 
@@ -1063,4 +1066,67 @@ R"DELIMITER(<ObjectStream version="1">
             }
         }
     }
+
+    // Fixture provides a root prefab with Transform component and listens for TransformNotificationBus.
+    class TransformComponentActivationTest
+        : public PrefabTestFixture
+        , public TransformNotificationBus::Handler
+    {
+    protected:
+        void SetUpEditorFixtureImpl() override
+        {
+            PrefabTestFixture::SetUpEditorFixtureImpl();
+
+            CreateRootPrefab();
+        }
+
+        void TearDownEditorFixtureImpl() override
+        {
+            BusDisconnect();
+            
+            PrefabTestFixture::TearDownEditorFixtureImpl();
+        }
+        
+        void OnTransformChanged(const Transform& /*local*/, const Transform& /*world*/) override
+        {
+            m_transformUpdated = true;
+        }
+
+        void MoveEntity(AZ::EntityId entityId)
+        {
+            AzToolsFramework::ScopedUndoBatch undoBatch("Move Entity");
+            TransformBus::Event(entityId, &TransformInterface::SetWorldTranslation, Vector3(1.f, 0.f, 0.f));
+        }
+        
+        bool m_transformUpdated = false;
+    };
+    
+    TEST_F(TransformComponentActivationTest, TransformChangedEventIsSentWhenEntityIsActivatedViaUndoRedo)
+    {
+        AZ::EntityId entityId = CreateEntityUnderRootPrefab("Entity");
+        MoveEntity(entityId);
+        BusConnect(entityId);
+
+        // verify that undoing/redoing move operations fires TransformChanged event
+        Undo();
+        EXPECT_TRUE(m_transformUpdated);
+        m_transformUpdated = false;
+
+        Redo();
+        EXPECT_TRUE(m_transformUpdated);
+        m_transformUpdated = false;
+    }
+
+    TEST_F(TransformComponentActivationTest, TransformChangedEventIsNotSentWhenEntityIsDeactivatedAndActivated)
+    {
+        AZ::EntityId entityId = CreateEntityUnderRootPrefab("Entity");
+        BusConnect(entityId);
+
+        // verify that simply activating/deactivating an entity does not fire TransformChanged event
+        Entity* entity = nullptr;
+        ComponentApplicationBus::BroadcastResult(entity, &AZ::ComponentApplicationRequests::FindEntity, entityId);
+        entity->Deactivate();
+        entity->Activate();
+        EXPECT_FALSE(m_transformUpdated);
+    }
 } // namespace UnitTest

Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp

@@ -37,16 +37,11 @@ namespace UnitTest
             m_model->Initialize();
             m_modelTester =
                 AZStd::make_unique<QAbstractItemModelTester>(m_model.get(), QAbstractItemModelTester::FailureReportingMode::Fatal);
-
-            AzToolsFramework::ToolsApplicationRequestBus::BroadcastResult(
-                m_undoStack, &AzToolsFramework::ToolsApplicationRequestBus::Events::GetUndoStack);
-            AZ_Assert(m_undoStack, "Failed to look up undo stack from tools application");
-
+            
             // Create a new root prefab - the synthetic "NewLevel.prefab" that comes in by default isn't suitable for outliner tests
             // because it's created before the EditorEntityModel that our EntityOutlinerListModel subscribes to, and we want to
             // recreate it as part of the fixture regardless.
-            auto entityOwnershipService = AZ::Interface<AzToolsFramework::PrefabEditorEntityOwnershipInterface>::Get();
-            entityOwnershipService->CreateNewLevelPrefab("UnitTestRoot.prefab", "");
+            CreateRootPrefab();
         }
 
         void TearDownEditorFixtureImpl() override
@@ -56,45 +51,7 @@ namespace UnitTest
             m_model.reset();
             PrefabTestFixture::TearDownEditorFixtureImpl();
         }
-
-        // Creates an entity with a given name as one undoable operation
-        // Parents to parentId, or the root prefab container entity if parentId is invalid
-        AZ::EntityId CreateNamedEntity(AZStd::string name, AZ::EntityId parentId = AZ::EntityId())
-        {
-            auto createResult = m_prefabPublicInterface->CreateEntity(parentId, AZ::Vector3());
-            AZ_Assert(createResult.IsSuccess(), "Failed to create entity: %s", createResult.GetError().c_str());
-            AZ::EntityId entityId = createResult.GetValue();
-
-            AZ::Entity* entity = nullptr;
-            AZ::ComponentApplicationBus::BroadcastResult(entity, &AZ::ComponentApplicationRequests::FindEntity, entityId);
-
-            entity->Deactivate();
-
-            entity->SetName(name);
-
-            // Normally, in invalid parent ID should automatically parent us to the root prefab, but currently in the unit test
-            // environment entities aren't created with a default transform component, so CreateEntity won't correctly parent.
-            // We get the actual target parent ID here, then create our missing transform component.
-            if (!parentId.IsValid())
-            {
-                auto prefabEditorEntityOwnershipInterface = AZ::Interface<AzToolsFramework::PrefabEditorEntityOwnershipInterface>::Get();
-                parentId = prefabEditorEntityOwnershipInterface->GetRootPrefabInstance()->get().GetContainerEntityId();
-            }
-
-            auto transform = aznew AzToolsFramework::Components::TransformComponent;
-            transform->SetParent(parentId);
-            entity->AddComponent(transform);
-
-            entity->Activate();
-
-            // Update our undo cache entry to include the rename / reparent as one atomic operation.
-            m_prefabPublicInterface->GenerateUndoNodesForEntityChangeAndUpdateCache(entityId, m_undoStack->GetTop());
-
-            ProcessDeferredUpdates();
-
-            return entityId;
-        }
-
+        
         // Helper to visualize debug state
         void PrintModel()
         {
@@ -125,32 +82,16 @@ namespace UnitTest
         }
 
         // Kicks off any updates scheduled for the next tick
-        void ProcessDeferredUpdates()
+        void ProcessDeferredUpdates() override
         {
-            // Force a prefab propagation for updates that are deferred to the next tick.
-            m_prefabSystemComponent->OnSystemTick();
+            PrefabTestFixture::ProcessDeferredUpdates();
 
             // Ensure the model process its entity update queue
             m_model->ProcessEntityUpdates();
         }
-
-        // Performs an undo operation and ensures the tick-scheduled updates happen
-        void Undo()
-        {
-            m_undoStack->Undo();
-            ProcessDeferredUpdates();
-        }
-
-        // Performs a redo operation and ensures the tick-scheduled updates happen
-        void Redo()
-        {
-            m_undoStack->Redo();
-            ProcessDeferredUpdates();
-        }
-
+        
         AZStd::unique_ptr<AzToolsFramework::EntityOutlinerListModel> m_model;
         AZStd::unique_ptr<QAbstractItemModelTester> m_modelTester;
-        AzToolsFramework::UndoSystem::UndoStack* m_undoStack = nullptr;
     };
 
     TEST_F(EntityOutlinerTest, TestCreateFlatHierarchyUndoAndRedoWorks)
@@ -159,7 +100,7 @@ namespace UnitTest
 
         for (size_t i = 0; i < entityCount; ++i)
         {
-            CreateNamedEntity(AZStd::string::format("Entity%zu", i));
+            CreateEntityUnderRootPrefab(AZStd::string::format("Entity%zu", i));
             EXPECT_EQ(m_model->rowCount(GetRootIndex()), i + 1);
         }
 
@@ -195,7 +136,7 @@ namespace UnitTest
         AZ::EntityId parentId;
         for (int i = 0; i < depth; i++)
         {
-            parentId = CreateNamedEntity(AZStd::string::format("EntityDepth%i", i), parentId);
+            parentId = CreateEntityUnderRootPrefab(AZStd::string::format("EntityDepth%i", i), parentId);
             EXPECT_EQ(modelDepth(), i + 1);
         }