diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp index ce1bf84db0..eaff64ba2e 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp @@ -381,22 +381,13 @@ namespace AzToolsFramework return; } - bool isPrefabSystemEnabled = false; - AzFramework::ApplicationRequests::Bus::BroadcastResult( - isPrefabSystemEnabled, &AzFramework::ApplicationRequests::IsPrefabSystemEnabled); - - // For slices, orphan any children that remain attached to the entity - // For prefabs, this is an unneeded operation because the prefab system handles the orphans - // and the extra reparenting operation can be problematic for consumers subscribed to entity - // events, such as the entity outliner. - if (!isPrefabSystemEnabled) + // Even though these child entities will immediately be destroyed, their entity info may be recycled + // Ensure they don't have any lingering inaccurate parent data + auto children = entityInfo.GetChildren(); + for (auto childId : children) { - auto children = entityInfo.GetChildren(); - for (auto childId : children) - { - ReparentChild(childId, AZ::EntityId(), entityId); - m_entityOrphanTable[entityId].insert(childId); - } + ReparentChild(childId, AZ::EntityId(), entityId); + m_entityOrphanTable[entityId].insert(childId); } m_savedOrderInfo[entityId] = AZStd::make_pair(entityInfo.GetParent(), entityInfo.GetIndexForSorting()); @@ -1200,26 +1191,41 @@ namespace AzToolsFramework auto childItr = m_childIndexCache.find(childId); if (childItr == m_childIndexCache.end()) { - //cache indices for faster lookup - m_childIndexCache[childId] = static_cast(m_children.size()); - m_children.push_back(childId); + // m_children is guaranteed to be ordered by EntityId, do a sorted insertion + auto insertedChildIndex = AZStd::upper_bound(m_children.begin(), m_children.end(), childId); + insertedChildIndex = m_children.insert(insertedChildIndex, childId); + + // Cache all affected child indices for fast lookup + for (auto it = insertedChildIndex; it != m_children.end(); ++it) + { + const AZ::u64 newChildIndex = static_cast(it - m_children.begin()); + m_childIndexCache[*it] = newChildIndex; + } } } void EditorEntityModel::EditorEntityModelEntry::RemoveChild(AZ::EntityId childId) { - auto childItr = m_childIndexCache.find(childId); - if (childItr != m_childIndexCache.end()) + // Retrieve our child index from the cache + auto cachedIndexItr = m_childIndexCache.find(childId); + if (cachedIndexItr == m_childIndexCache.end()) { - // Take the last entry and move it into the removed spot instead of deleting the entry and having to move all - // following entries one step down. - AZ::EntityId backEntity = m_children.back(); - m_children[childItr->second] = backEntity; - // Update cached index for the moved id to the new index. - m_childIndexCache[backEntity] = childItr->second; - // Now remove the deleted id from the children and cache. - m_childIndexCache.erase(childId); - m_children.erase(m_children.end() - 1); + AZ_Assert(false, "Attempted to remove an unknown child"); + return; + } + + // Build an iterator for m_children based on our cached index + auto childItr = m_children.begin() + cachedIndexItr->second; + + // Remove our child from the cache + m_childIndexCache.erase(cachedIndexItr); + + // Remove our child, fix up the cache entries for any subsequent children + auto elementsToFixItr = m_children.erase(childItr); + for (auto it = elementsToFixItr; it != m_children.end(); ++it) + { + const AZ::u64 newChildIndex = static_cast(it - m_children.begin()); + m_childIndexCache[*it] = newChildIndex; } } @@ -1256,8 +1262,17 @@ namespace AzToolsFramework AZ::u64 EditorEntityModel::EditorEntityModelEntry::GetChildIndex(AZ::EntityId childId) const { + // Return the cached index, if available. auto childItr = m_childIndexCache.find(childId); - return childItr != m_childIndexCache.end() ? childItr->second : static_cast(m_children.size()); + if (childItr != m_childIndexCache.end()) + { + return childItr->second; + } + + // On initialization, GetChildIndex may be queried for a childId that is not yet in the child list. + // Return the position it would be inserted at in EditorEntityModelEntry::AddChild + auto targetChildPositionItr = AZStd::upper_bound(m_children.begin(), m_children.end(), childId); + return static_cast(targetChildPositionItr - m_children.begin()); } AZStd::string EditorEntityModel::EditorEntityModelEntry::GetName() const diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.cpp index 234696ff2a..3f8023c1e3 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.cpp @@ -119,6 +119,12 @@ namespace AzToolsFramework int EntityOutlinerListModel::rowCount(const QModelIndex& parent) const { + // For QTreeView models, non-0 columns shouldn't have children + if (parent.isValid() && parent.column() != 0) + { + return 0; + } + auto parentId = GetEntityFromIndex(parent); AZStd::size_t childCount = 0; @@ -133,17 +139,13 @@ namespace AzToolsFramework QModelIndex EntityOutlinerListModel::index(int row, int column, const QModelIndex& parent) const { - // sanity check - if (!hasIndex(row, column, parent) || (parent.isValid() && parent.column() != 0) || (row < 0 || row >= rowCount(parent))) - { - return QModelIndex(); - } - auto parentId = GetEntityFromIndex(parent); + // We have the row and column, so we just need the child ID to construct our index AZ::EntityId childId; EditorEntityInfoRequestBus::EventResult(childId, parentId, &EditorEntityInfoRequestBus::Events::GetChild, row); - return GetIndexFromEntity(childId, column); + AZ_Assert(childId.IsValid(), "No child found for parent"); + return createIndex(row, column, static_cast(childId)); } QVariant EntityOutlinerListModel::data(const QModelIndex& index, int role) const @@ -517,13 +519,18 @@ namespace AzToolsFramework { AZ::EntityId parentId; EditorEntityInfoRequestBus::EventResult(parentId, id, &EditorEntityInfoRequestBus::Events::GetParent); - return GetIndexFromEntity(parentId, index.column()); + return GetIndexFromEntity(parentId, 0); } return QModelIndex(); } Qt::ItemFlags EntityOutlinerListModel::flags(const QModelIndex& index) const { + if (!index.isValid()) + { + return Qt::ItemIsDropEnabled; + } + Qt::ItemFlags itemFlags = QAbstractItemModel::flags(index); switch (index.column()) { @@ -1208,6 +1215,10 @@ namespace AzToolsFramework void EntityOutlinerListModel::ProcessEntityUpdates() { AZ_PROFILE_FUNCTION(Editor); + if (!m_entityChangeQueued) + { + return; + } m_entityChangeQueued = false; if (m_layoutResetQueued) { @@ -1236,31 +1247,14 @@ namespace AzToolsFramework { AZ_PROFILE_SCOPE(Editor, "EntityOutlinerListModel::ProcessEntityUpdates:ChangeQueue"); - // its faster to just do a bulk data change than to carefully pick out indices - // so we'll just merge all ranges into a single range rather than try to make gaps - QModelIndex firstChangeIndex; - QModelIndex lastChangeIndex; - for (auto entityId : m_entityChangeQueue) { - auto myIndex = GetIndexFromEntity(entityId, ColumnName); - if ((!firstChangeIndex.isValid())||(firstChangeIndex.row() > myIndex.row())) + if (entityId.IsValid()) { - firstChangeIndex = myIndex; + const QModelIndex beginIndex = GetIndexFromEntity(entityId, ColumnName); + const QModelIndex endIndex = createIndex(beginIndex.row(), VisibleColumnCount - 1, beginIndex.internalId()); + emit dataChanged(beginIndex, endIndex); } - - if ((!lastChangeIndex.isValid())||(lastChangeIndex.row() < myIndex.row())) - { - // expand it to be the last column: - lastChangeIndex = myIndex; - } - } - - if (firstChangeIndex.isValid()) - { - // expand to cover all visible columns: - lastChangeIndex = createIndex(lastChangeIndex.row(), VisibleColumnCount - 1, lastChangeIndex.internalPointer()); - emit dataChanged(firstChangeIndex, lastChangeIndex); } m_entityChangeQueue.clear(); @@ -1382,6 +1376,9 @@ namespace AzToolsFramework m_isFilterDirty = true; QueueAncestorUpdate(parentId); emit EnableSelectionUpdates(true); + + // Remove any pending updates for this removed entity. + m_entityChangeQueue.erase(childId); } void EntityOutlinerListModel::OnEntityInfoUpdatedOrderBegin(AZ::EntityId parentId, AZ::EntityId childId, AZ::u64 index) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.hxx b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.hxx index 7b946c6304..8176867038 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.hxx +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.hxx @@ -145,6 +145,8 @@ namespace AzToolsFramework void SetSortMode(EntityOutliner::DisplaySortMode sortMode) { m_sortMode = sortMode; } void SetDropOperationInProgress(bool inProgress); + void ProcessEntityUpdates(); + Q_SIGNALS: void ExpandEntity(const AZ::EntityId& entityId, bool expand); void SelectEntity(const AZ::EntityId& entityId, bool select); @@ -178,7 +180,6 @@ namespace AzToolsFramework void QueueEntityUpdate(AZ::EntityId entityId); void QueueAncestorUpdate(AZ::EntityId entityId); void QueueEntityToExpand(AZ::EntityId entityId, bool expand); - void ProcessEntityUpdates(); void ProcessEntityInfoResetEnd(); AZStd::unordered_set m_entitySelectQueue; AZStd::unordered_set m_entityExpandQueue; diff --git a/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp b/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp new file mode 100644 index 0000000000..614959d7e0 --- /dev/null +++ b/Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp @@ -0,0 +1,197 @@ +/* + * 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 + * + */ + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace UnitTest +{ + // Test fixture for the entity outliner model that uses a QAbstractItemModelTester to validate the state of the model + // when QAbstractItemModel signals fire. Tests will exit with a fatal error if an invalid state is detected. + class EntityOutlinerTest : public PrefabTestFixture + { + protected: + void SetUpEditorFixtureImpl() override + { + PrefabTestFixture::SetUpEditorFixtureImpl(); + GetApplication()->RegisterComponentDescriptor(AzToolsFramework::EditorEntityContextComponent::CreateDescriptor()); + + m_model = AZStd::make_unique(); + 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", ""); + } + + void TearDownEditorFixtureImpl() override + { + m_undoStack = nullptr; + m_modelTester.reset(); + 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()); + + // Force a prefab propagation as updates are deferred to the next tick. + m_prefabSystemComponent->OnSystemTick(); + + return entityId; + } + + // Helper to visualize debug state + void PrintModel() + { + AZStd::deque> indices; + indices.push_back({ m_model->index(0, 0), 0 }); + while (!indices.empty()) + { + auto [index, depth] = indices.front(); + indices.pop_front(); + + QString indentString; + for (int i = 0; i < depth; ++i) + { + indentString += " "; + } + qDebug() << (indentString + index.data(Qt::DisplayRole).toString()) << index.internalId(); + for (int i = 0; i < m_model->rowCount(index); ++i) + { + indices.emplace_back(m_model->index(i, 0, index), depth + 1); + } + } + }; + + // Gets the index of the root prefab, i.e. the "New Level" container entity + QModelIndex GetRootIndex() const + { + return m_model->index(0, 0); + } + + AZStd::unique_ptr m_model; + AZStd::unique_ptr m_modelTester; + AzToolsFramework::UndoSystem::UndoStack* m_undoStack = nullptr; + }; + + TEST_F(EntityOutlinerTest, TestCreateFlatHierarchyUndoAndRedoWorks) + { + constexpr size_t entityCount = 10; + + for (size_t i = 0; i < entityCount; ++i) + { + CreateNamedEntity(AZStd::string::format("Entity%zu", i)); + EXPECT_EQ(m_model->rowCount(GetRootIndex()), i + 1); + } + m_model->ProcessEntityUpdates(); + + for (int i = entityCount; i > 0; --i) + { + m_undoStack->Undo(); + EXPECT_EQ(m_model->rowCount(GetRootIndex()), i - 1); + } + m_model->ProcessEntityUpdates(); + + for (size_t i = 0; i < entityCount; ++i) + { + m_undoStack->Redo(); + EXPECT_EQ(m_model->rowCount(GetRootIndex()), i + 1); + } + m_model->ProcessEntityUpdates(); + } + + TEST_F(EntityOutlinerTest, TestCreateNestedHierarchyUndoAndRedoWorks) + { + constexpr size_t depth = 5; + + auto modelDepth = [this]() -> int + { + int depth = 0; + QModelIndex index = GetRootIndex(); + while (m_model->rowCount(index) > 0) + { + ++depth; + index = m_model->index(0, 0, index); + } + return depth; + }; + + AZ::EntityId parentId; + for (int i = 0; i < depth; i++) + { + parentId = CreateNamedEntity(AZStd::string::format("EntityDepth%i", i), parentId); + EXPECT_EQ(modelDepth(), i + 1); + m_model->ProcessEntityUpdates(); + } + + for (int i = depth - 1; i >= 0; --i) + { + m_undoStack->Undo(); + EXPECT_EQ(modelDepth(), i); + m_model->ProcessEntityUpdates(); + } + + for (int i = 0; i < depth; ++i) + { + m_undoStack->Redo(); + EXPECT_EQ(modelDepth(), i + 1); + m_model->ProcessEntityUpdates(); + } + } +} // namespace UnitTest diff --git a/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake b/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake index ef6774f620..008c09188b 100644 --- a/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake +++ b/Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake @@ -120,6 +120,8 @@ set(FILES ToolsComponents/EditorLayerComponentTests.cpp ToolsComponents/EditorTransformComponentTests.cpp TransformComponent.cpp + UI/EntityIdQLineEditTests.cpp + UI/EntityOutlinerTests.cpp UI/EntityPropertyEditorTests.cpp UndoStack.cpp Viewport/ClusterTests.cpp