birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #4749 from aws-lumberyard-dev/nvsickle/OutlinerDuplicateEntryFixes

Browse Source 
Fix issues with invalid Outliner entries
This commit is contained in:
Nicholas Van Sickle
2021-10-18 14:01:31 -07:00
committed by GitHub
parent d692cde146 f0e6841ca8
commit 434641382b
5 changed files with 272 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzToolsFramework/AzToolsFramework/Entity/EditorEntityModel.cpp
+45 -30
View File
@@ -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<AZ::u64>(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<AZ::u64>(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<AZ::u64>(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<AZ::u64>(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<AZ::u64>(targetChildPositionItr - m_children.begin());
}
AZStd::string EditorEntityModel::EditorEntityModelEntry::GetName() const
Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.cpp
+26 -29
View File
@@ -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<AZ::u64>(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)
Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerListModel.hxx
+2 -1
View File
@@ -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<AZ::EntityId> m_entitySelectQueue;
AZStd::unordered_set<AZ::EntityId> m_entityExpandQueue;
Code/Framework/AzToolsFramework/Tests/UI/EntityOutlinerTests.cpp
+197
View File
@@ -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 <AzCore/Serialization/SerializeContext.h>
#include <AzTest/AzTest.h>
#include <AzFramework/Entity/EntityContextBus.h>
#include <AzToolsFramework/API/ToolsApplicationAPI.h>
#include <AzToolsFramework/Entity/EditorEntityContextComponent.h>
#include <AzToolsFramework/Entity/PrefabEditorEntityOwnershipInterface.h>
#include <AzToolsFramework/ToolsComponents/TransformComponent.h>
#include <AzToolsFramework/UI/Outliner/EntityOutlinerListModel.hxx>
#include <AzToolsFramework/UI/Prefab/PrefabIntegrationManager.h>
#include <AzToolsFramework/Undo/UndoSystem.h>
#include <Prefab/PrefabTestFixture.h>
#include <QAbstractItemModelTester>
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<AzToolsFramework::EntityOutlinerListModel>();
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", "");
}
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<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());
// 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<AZStd::pair<QModelIndex, int>> 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<AzToolsFramework::EntityOutlinerListModel> m_model;
AZStd::unique_ptr<QAbstractItemModelTester> 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
Code/Framework/AzToolsFramework/Tests/aztoolsframeworktests_files.cmake
+2
View File
@@ -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