From daeab4bbb112755d9c4eabc8cdac3f92fec5202e Mon Sep 17 00:00:00 2001 From: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> Date: Wed, 2 Feb 2022 09:17:59 -0800 Subject: [PATCH] Focus Mode | Switch instance referencing in Prefab Focus Handler to use more reliable Instance handles (#7304) * Add function to Prefab Instances allowing to get a reference to a nested instance by alias. Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> * Refactor the Prefab Focus Handler to use RootAliasPaths to store the reference to the currently focused prefab instance instead of the previous method (entityId of the prefab container). Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> * Use existing FindNestedInstance method instead of adding new one. Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> * Minor fixes to style and comments Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> * Removing indexing in GetInstanceReferenceFromRootAliasPath, turn variables into constants where possible. Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> * Modified the Prefab Focus test fixture to ensure the entity hierarchy is generated under the Prefab EOS, allowing tests to work with the new implementation of the focus mode handler. Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> * Ensure RootAliasPath is iterated by reference in GetInstanceReferenceFromRootAliasPath Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com> --- .../Prefab/Instance/Instance.h | 2 +- .../Prefab/PrefabFocusHandler.cpp | 155 ++++++++---------- .../Prefab/PrefabFocusHandler.h | 13 +- .../Prefab/PrefabFocus/PrefabFocusTests.cpp | 22 ++- 4 files changed, 91 insertions(+), 101 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h index b63eca309a..77ad526979 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/Instance.h @@ -37,6 +37,7 @@ namespace AzToolsFramework using AliasPath = AZ::IO::Path; using AliasPathView = AZ::IO::PathView; + using RootAliasPath = AliasPath; using EntityAlias = AZStd::string; using EntityAliasView = AZStd::string_view; using InstanceAlias = AZStd::string; @@ -177,7 +178,6 @@ namespace AzToolsFramework AZStd::pair GetInstanceAndEntityIdFromAliasPath(AliasPathView relativeAliasPath); AZStd::pair GetInstanceAndEntityIdFromAliasPath(AliasPathView relativeAliasPath) const; - /** * Gets the aliases of all the nested instances, which are sourced by the template with the given id. * diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.cpp index 3426c56c99..d8a89109ef 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.cpp @@ -97,9 +97,9 @@ namespace AzToolsFramework::Prefab ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequestBus::Events::SetSelectedEntities, selectedEntities); } - // Edit Prefab + // Add undo element { - auto editUndo = aznew PrefabFocusUndo("Edit Prefab"); + auto editUndo = aznew PrefabFocusUndo("Focus Prefab"); editUndo->Capture(entityId); editUndo->SetParent(undoBatch.GetUndoBatch()); FocusOnPrefabInstanceOwningEntityId(entityId); @@ -120,7 +120,7 @@ namespace AzToolsFramework::Prefab } // Retrieve parent of currently focused prefab. - InstanceOptionalReference parentInstance = GetReferenceFromContainerEntityId(m_instanceFocusHierarchy[hierarchySize - 2]); + InstanceOptionalReference parentInstance = GetInstanceReferenceFromRootAliasPath(m_instanceFocusHierarchy[hierarchySize - 2]); // Use container entity of parent Instance for focus operations. AZ::EntityId entityId = parentInstance->get().GetContainerEntityId(); @@ -136,9 +136,9 @@ namespace AzToolsFramework::Prefab ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequestBus::Events::SetSelectedEntities, selectedEntities); } - // Edit Prefab + // Add undo element { - auto editUndo = aznew PrefabFocusUndo("Edit Prefab"); + auto editUndo = aznew PrefabFocusUndo("Focus Prefab"); editUndo->Capture(entityId); editUndo->SetParent(undoBatch.GetUndoBatch()); FocusOnPrefabInstanceOwningEntityId(entityId); @@ -154,7 +154,12 @@ namespace AzToolsFramework::Prefab return AZ::Failure(AZStd::string("Prefab Focus Handler: Invalid index on FocusOnPathIndex.")); } - InstanceOptionalReference focusedInstance = GetReferenceFromContainerEntityId(m_instanceFocusHierarchy[index]); + InstanceOptionalReference focusedInstance = GetInstanceReferenceFromRootAliasPath(m_instanceFocusHierarchy[index]); + + if (!focusedInstance.has_value()) + { + return AZ::Failure(AZStd::string::format("Prefab Focus Handler: Could not retrieve instance at index %i.", index)); + } return FocusOnOwningPrefab(focusedInstance->get().GetContainerEntityId()); } @@ -192,12 +197,12 @@ namespace AzToolsFramework::Prefab } // Close all container entities in the old path. - CloseInstanceContainers(m_instanceFocusHierarchy); + SetInstanceContainersOpenState(m_instanceFocusHierarchy, false); - AZ::EntityId previousContainerEntityId = m_focusedInstanceContainerEntityId; + const RootAliasPath previousContainerRootAliasPath = m_focusedInstanceRootAliasPath; + const InstanceOptionalConstReference previousFocusedInstance = GetInstanceReferenceFromRootAliasPath(previousContainerRootAliasPath); - // Do not store the container for the root instance, use an invalid EntityId instead. - m_focusedInstanceContainerEntityId = focusedInstance->get().GetParentInstance().has_value() ? focusedInstance->get().GetContainerEntityId() : AZ::EntityId(); + m_focusedInstanceRootAliasPath = focusedInstance->get().GetAbsoluteInstanceAliasPath(); m_focusedTemplateId = focusedInstance->get().GetTemplateId(); // Focus on the descendants of the container entity in the Editor, if the interface is initialized. @@ -214,7 +219,15 @@ namespace AzToolsFramework::Prefab // Refresh the read-only cache, if the interface is initialized. if (m_readOnlyEntityQueryInterface) { - m_readOnlyEntityQueryInterface->RefreshReadOnlyState({ previousContainerEntityId, m_focusedInstanceContainerEntityId }); + EntityIdList containerEntities; + + if (previousFocusedInstance.has_value()) + { + containerEntities.push_back(previousFocusedInstance->get().GetContainerEntityId()); + } + containerEntities.push_back(focusedInstance->get().GetContainerEntityId()); + + m_readOnlyEntityQueryInterface->RefreshReadOnlyState(containerEntities); } // Refresh path variables. @@ -222,7 +235,7 @@ namespace AzToolsFramework::Prefab RefreshInstanceFocusPath(); // Open all container entities in the new path. - OpenInstanceContainers(m_instanceFocusHierarchy); + SetInstanceContainersOpenState(m_instanceFocusHierarchy, true); PrefabFocusNotificationBus::Broadcast(&PrefabFocusNotifications::OnPrefabFocusChanged); @@ -237,17 +250,12 @@ namespace AzToolsFramework::Prefab InstanceOptionalReference PrefabFocusHandler::GetFocusedPrefabInstance( [[maybe_unused]] AzFramework::EntityContextId entityContextId) const { - return GetReferenceFromContainerEntityId(m_focusedInstanceContainerEntityId); + return GetInstanceReferenceFromRootAliasPath(m_focusedInstanceRootAliasPath); } AZ::EntityId PrefabFocusHandler::GetFocusedPrefabContainerEntityId([[maybe_unused]] AzFramework::EntityContextId entityContextId) const { - if (m_focusedInstanceContainerEntityId.IsValid()) - { - return m_focusedInstanceContainerEntityId; - } - - if (auto instance = GetReferenceFromContainerEntityId(m_focusedInstanceContainerEntityId); instance.has_value()) + if (const InstanceOptionalConstReference instance = GetInstanceReferenceFromRootAliasPath(m_focusedInstanceRootAliasPath); instance.has_value()) { return instance->get().GetContainerEntityId(); } @@ -262,19 +270,13 @@ namespace AzToolsFramework::Prefab return false; } - InstanceOptionalReference instance = m_instanceEntityMapperInterface->FindOwningInstance(entityId); + const InstanceOptionalConstReference instance = m_instanceEntityMapperInterface->FindOwningInstance(entityId); if (!instance.has_value()) { return false; } - // If this is owned by the root instance, that corresponds to an invalid m_focusedInstanceContainerEntityId. - if (!instance->get().GetParentInstance().has_value()) - { - return !m_focusedInstanceContainerEntityId.IsValid(); - } - - return (instance->get().GetContainerEntityId() == m_focusedInstanceContainerEntityId); + return (instance->get().GetAbsoluteInstanceAliasPath() == m_focusedInstanceRootAliasPath); } bool PrefabFocusHandler::IsOwningPrefabInFocusHierarchy(AZ::EntityId entityId) const @@ -284,18 +286,10 @@ namespace AzToolsFramework::Prefab return false; } - // If the focus is on the root, m_focusedInstanceContainerEntityId will be the invalid id. - // In those case all entities are in the focus hierarchy and should return true. - if (!m_focusedInstanceContainerEntityId.IsValid()) - { - return true; - } - - InstanceOptionalReference instance = m_instanceEntityMapperInterface->FindOwningInstance(entityId); - + InstanceOptionalConstReference instance = m_instanceEntityMapperInterface->FindOwningInstance(entityId); while (instance.has_value()) { - if (instance->get().GetContainerEntityId() == m_focusedInstanceContainerEntityId) + if (instance->get().GetAbsoluteInstanceAliasPath() == m_focusedInstanceRootAliasPath) { return true; } @@ -330,9 +324,9 @@ namespace AzToolsFramework::Prefab // Determine if the entityId is the container for any of the instances in the vector. auto result = AZStd::find_if( m_instanceFocusHierarchy.begin(), m_instanceFocusHierarchy.end(), - [&, entityId](const AZ::EntityId& containerEntityId) + [&, entityId](const RootAliasPath& rootAliasPath) { - InstanceOptionalReference instance = GetReferenceFromContainerEntityId(containerEntityId); + InstanceOptionalReference instance = GetInstanceReferenceFromRootAliasPath(rootAliasPath); return (instance->get().GetContainerEntityId() == entityId); } ); @@ -357,9 +351,9 @@ namespace AzToolsFramework::Prefab // Determine if the templateId matches any of the instances in the vector. auto result = AZStd::find_if( m_instanceFocusHierarchy.begin(), m_instanceFocusHierarchy.end(), - [&, templateId](const AZ::EntityId& containerEntityId) + [&, templateId](const RootAliasPath& rootAliasPath) { - InstanceOptionalReference instance = GetReferenceFromContainerEntityId(containerEntityId); + InstanceOptionalReference instance = GetInstanceReferenceFromRootAliasPath(rootAliasPath); return (instance->get().GetTemplateId() == templateId); } ); @@ -376,20 +370,10 @@ namespace AzToolsFramework::Prefab { m_instanceFocusHierarchy.clear(); - AZStd::list instanceFocusList; - - InstanceOptionalReference currentInstance = GetReferenceFromContainerEntityId(m_focusedInstanceContainerEntityId); + InstanceOptionalConstReference currentInstance = GetInstanceReferenceFromRootAliasPath(m_focusedInstanceRootAliasPath); while (currentInstance.has_value()) { - if (currentInstance->get().GetParentInstance().has_value()) - { - m_instanceFocusHierarchy.emplace_back(currentInstance->get().GetContainerEntityId()); - } - else - { - m_instanceFocusHierarchy.emplace_back(AZ::EntityId()); - } - + m_instanceFocusHierarchy.emplace_back(currentInstance->get().GetAbsoluteInstanceAliasPath()); currentInstance = currentInstance->get().GetParentInstance(); } @@ -406,9 +390,9 @@ namespace AzToolsFramework::Prefab size_t index = 0; size_t maxIndex = m_instanceFocusHierarchy.size() - 1; - for (const AZ::EntityId& containerEntityId : m_instanceFocusHierarchy) + for (const RootAliasPath& rootAliasPath : m_instanceFocusHierarchy) { - InstanceOptionalReference instance = GetReferenceFromContainerEntityId(containerEntityId); + const InstanceOptionalConstReference instance = GetInstanceReferenceFromRootAliasPath(rootAliasPath); if (instance.has_value()) { AZStd::string prefabName; @@ -436,60 +420,59 @@ namespace AzToolsFramework::Prefab } } - void PrefabFocusHandler::OpenInstanceContainers(const AZStd::vector& instances) const + void PrefabFocusHandler::SetInstanceContainersOpenState(const AZStd::vector& instances, bool openState) const { // If this is called outside the Editor, this interface won't be initialized. if (!m_containerEntityInterface) { return; } - - for (const AZ::EntityId& containerEntityId : instances) - { - InstanceOptionalReference instance = GetReferenceFromContainerEntityId(containerEntityId); - - if (instance.has_value()) - { - m_containerEntityInterface->SetContainerOpen(instance->get().GetContainerEntityId(), true); - } - } - } - void PrefabFocusHandler::CloseInstanceContainers(const AZStd::vector& instances) const - { - // If this is called outside the Editor, this interface won't be initialized. - if (!m_containerEntityInterface) + for (const RootAliasPath& rootAliasPath : instances) { - return; - } - - for (const AZ::EntityId& containerEntityId : instances) - { - InstanceOptionalReference instance = GetReferenceFromContainerEntityId(containerEntityId); + InstanceOptionalReference instance = GetInstanceReferenceFromRootAliasPath(rootAliasPath); if (instance.has_value()) { - m_containerEntityInterface->SetContainerOpen(instance->get().GetContainerEntityId(), false); + m_containerEntityInterface->SetContainerOpen(instance->get().GetContainerEntityId(), openState); } } } - InstanceOptionalReference PrefabFocusHandler::GetReferenceFromContainerEntityId(AZ::EntityId containerEntityId) const + InstanceOptionalReference PrefabFocusHandler::GetInstanceReferenceFromRootAliasPath(RootAliasPath rootAliasPath) const { - if (!containerEntityId.IsValid()) + PrefabEditorEntityOwnershipInterface* prefabEditorEntityOwnershipInterface = + AZ::Interface::Get(); + + if (prefabEditorEntityOwnershipInterface) { - PrefabEditorEntityOwnershipInterface* prefabEditorEntityOwnershipInterface = - AZ::Interface::Get(); + InstanceOptionalReference instance = prefabEditorEntityOwnershipInterface->GetRootPrefabInstance(); - if (!prefabEditorEntityOwnershipInterface) + for (const auto& pathElement : rootAliasPath) { - return AZStd::nullopt; + if (pathElement.Native() == rootAliasPath.begin()->Native()) + { + // If the root is not the root Instance, the rootAliasPath is invalid. + if (pathElement.Native() != instance->get().GetInstanceAlias()) + { + return InstanceOptionalReference(); + } + } + else + { + // If the instance alias can't be found, the rootAliasPath is invalid. + instance = instance->get().FindNestedInstance(pathElement.Native()); + if (!instance.has_value()) + { + return InstanceOptionalReference(); + } + } } - return prefabEditorEntityOwnershipInterface->GetRootPrefabInstance(); + return instance; } - return m_instanceEntityMapperInterface->FindOwningInstance(containerEntityId); + return InstanceOptionalReference(); } } // namespace AzToolsFramework::Prefab diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.h index 4f3dbdd708..75e0a9f1f4 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabFocusHandler.h @@ -76,18 +76,17 @@ namespace AzToolsFramework::Prefab void RefreshInstanceFocusList(); void RefreshInstanceFocusPath(); - void OpenInstanceContainers(const AZStd::vector& instances) const; - void CloseInstanceContainers(const AZStd::vector& instances) const; + void SetInstanceContainersOpenState(const AZStd::vector& instances, bool openState) const; - InstanceOptionalReference GetReferenceFromContainerEntityId(AZ::EntityId containerEntityId) const; + InstanceOptionalReference GetInstanceReferenceFromRootAliasPath(RootAliasPath rootAliasPath) const; - //! The EntityId of the prefab container entity for the instance the editor is currently focusing on. - AZ::EntityId m_focusedInstanceContainerEntityId = AZ::EntityId(); + //! The alias path for the instance the editor is currently focusing on, starting from the root instance. + RootAliasPath m_focusedInstanceRootAliasPath = RootAliasPath(); //! The templateId of the focused instance. TemplateId m_focusedTemplateId; //! The list of instances going from the root (index 0) to the focused instance, - //! referenced by their prefab container's EntityId. - AZStd::vector m_instanceFocusHierarchy; + //! referenced by their alias path from the root instance. + AZStd::vector m_instanceFocusHierarchy; //! A path containing the filenames of the instances in the focus hierarchy, separated with a /. AZ::IO::Path m_instanceFocusPath; diff --git a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabFocus/PrefabFocusTests.cpp b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabFocus/PrefabFocusTests.cpp index 6bf964f038..df36a50a8b 100644 --- a/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabFocus/PrefabFocusTests.cpp +++ b/Code/Framework/AzToolsFramework/Tests/Prefab/PrefabFocus/PrefabFocusTests.cpp @@ -41,6 +41,11 @@ namespace UnitTest &AzToolsFramework::EditorEntityContextRequests::HandleEntitiesAdded, AzToolsFramework::EntityList{ m_entityMap[Passenger1EntityName], m_entityMap[Passenger2EntityName], m_entityMap[CityEntityName] }); + // Initialize Prefab EOS Interface + AzToolsFramework::PrefabEditorEntityOwnershipInterface* prefabEditorEntityOwnershipInterface = + AZ::Interface::Get(); + ASSERT_TRUE(prefabEditorEntityOwnershipInterface); + // Create a car prefab from the passenger1 entity. The container entity will be created as part of the process. AZStd::unique_ptr carInstance = m_prefabSystemComponent->CreatePrefab({ m_entityMap[Passenger1EntityName] }, {}, "test/car"); @@ -59,11 +64,14 @@ namespace UnitTest ASSERT_TRUE(streetInstance); m_instanceMap[StreetEntityName] = streetInstance.get(); - // Create a city prefab that nests the street instances created above and the city entity. The container entity will be created as part of the process. - m_rootInstance = - m_prefabSystemComponent->CreatePrefab({ m_entityMap[CityEntityName] }, MakeInstanceList(AZStd::move(streetInstance)), "test/city"); - ASSERT_TRUE(m_rootInstance); - m_instanceMap[CityEntityName] = m_rootInstance.get(); + // Use the Prefab EOS root instance as the City instance. This will ensure functions that go through the EOS work in these tests too. + m_rootInstance = prefabEditorEntityOwnershipInterface->GetRootPrefabInstance(); + ASSERT_TRUE(m_rootInstance.has_value()); + + m_rootInstance->get().AddEntity(*m_entityMap[CityEntityName]); + m_rootInstance->get().AddInstance(AZStd::move(streetInstance)); + + m_instanceMap[CityEntityName] = &m_rootInstance->get(); } void SetUpEditorFixtureImpl() override @@ -84,7 +92,7 @@ namespace UnitTest void TearDownEditorFixtureImpl() override { - m_rootInstance.release(); + m_rootInstance->get().Reset(); PrefabTestFixture::TearDownEditorFixtureImpl(); } @@ -92,7 +100,7 @@ namespace UnitTest AZStd::unordered_map m_entityMap; AZStd::unordered_map m_instanceMap; - AZStd::unique_ptr m_rootInstance; + InstanceOptionalReference m_rootInstance; PrefabFocusInterface* m_prefabFocusInterface = nullptr; PrefabFocusPublicInterface* m_prefabFocusPublicInterface = nullptr;