From c5615f812ccc9d235747a1c5ba396ac2e9a23690 Mon Sep 17 00:00:00 2001 From: Mikhail Naumov <82239319+AMZN-mnaumov@users.noreply.github.com> Date: Mon, 6 Dec 2021 08:46:47 -0600 Subject: [PATCH] Fixing pivot offset when performing undo on Transform component (#6150) * Fixing undo offest on transform component Signed-off-by: Mikhail Naumov * Fixining possible issue with firing an unnecessary OnTransformChanged event when entity is manually disabled/enabled Signed-off-by: Mikhail Naumov * Adding better comments Signed-off-by: Mikhail Naumov * Unit tests Signed-off-by: Mikhail Naumov * Cherrupicking changes Signed-off-by: Mikhail Naumov --- .../ToolsComponents/TransformComponent.cpp | 20 ++--- .../ToolsComponents/TransformComponent.h | 3 + .../Tests/Prefab/PrefabTestFixture.cpp | 81 ++++++++++++++++++- .../Tests/Prefab/PrefabTestFixture.h | 15 +++- .../Tests/TransformComponent.cpp | 66 +++++++++++++++ .../Tests/UI/EntityOutlinerTests.cpp | 75 ++--------------- 6 files changed, 181 insertions(+), 79 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.cpp index 7ab7dfdfe4..1f056729d7 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.cpp +++ b/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())) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.h b/Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.h index 931d3b48d6..08d1392d74 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/TransformComponent.h +++ b/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; diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp index ed30de09c4..3c58f9df4e 100644 --- a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp +++ b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.cpp @@ -11,6 +11,8 @@ #include #include #include +#include +#include #include #include @@ -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 PrefabTestFixture::CreateTestApplication() @@ -57,7 +68,20 @@ namespace UnitTest return AZStd::make_unique("PrefabTestApplication"); } - AZ::Entity* PrefabTestFixture::CreateEntity(const char* entityName, const bool shouldActivate) + void PrefabTestFixture::CreateRootPrefab() + { + auto entityOwnershipService = AZ::Interface::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(); + 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::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(); + } } diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h index 0a78ded1a6..84409397dc 100644 --- a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h +++ b/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 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; }; } diff --git a/Code/Framework/AzToolsFramework/Tests/TransformComponent.cpp b/Code/Framework/AzToolsFramework/Tests/TransformComponent.cpp index 4fbebc8526..3849ed23c9 100644 --- a/Code/Framework/AzToolsFramework/Tests/TransformComponent.cpp +++ b/Code/Framework/AzToolsFramework/Tests/TransformComponent.cpp @@ -18,7 +18,10 @@ #include #include +#include #include +#include +#include #include @@ -1063,4 +1066,67 @@ R"DELIMITER( } } } + + // 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 diff --git a/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp b/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp index fa8de64c62..0035b26b2a 100644 --- a/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp +++ b/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp @@ -37,16 +37,11 @@ namespace UnitTest m_model->Initialize(); m_modelTester = AZStd::make_unique(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::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::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 m_model; AZStd::unique_ptr 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); }