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

Fix issues with invalid Outliner entries

Browse Source 
The EntityOutlinerListModel was violating the QAbstractItemModel contract in a few cases, as reported by `QAbstractItemModelTester`. The important ones causing issues were:
- Entry order was not guaranteed, leading to model indices pointing at invalid data
- Parent/child relationships could be temporarily invalid due to a change I made in EditorEntityModel::RemoveEntity to try to avoid an unnecessary reparent operation - as it turned out, the parent/child data was being cached even for recreated entities and not clearing child data could cause issues
- `EntityOutlinerListModel::ProcessEntityUpdates` was emitting data changed between two indices that didn't necessarily share a parent, which is [undefined behavior](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged)

The other reported issues (that weren't really causing issues with `QTreeView`) were:
- The root index had flags other than `Qt::ItemIsDropEnabled`
- `rowCount` showed all columns as having children
- `parent` showed indices as being parented to a non-0 column

This change introduces fixes for the above issues, namely:
- Reverts my change to `EditorEntityModel::RemoveEntity` to ensure we don't have invalid parent/child references sitting in the cache
- Ensures `EditorEntityModelEntry` child ordering is guaranteed sorted by EntityId, to prevent the `EntityOutlinerListModel` from having indices pointed at invalid data*.
- Fixes various model sanity issues, such as `rowCount` being 0 for indices with a non-0 column

Two unit tests were added to reproduce the invalid behavior and validate the fix: TestCreateFlatHierarchyUndoAndRedoWorks and TestCreateNestedHierarchyUndoAndRedoWorks
This change focuses on correctness over performance. My subjective in-Editor outliner experience is about the same, but it may be worthwhile to expand the test coverage with a benchmarking suite to look into areas for optimization.

*As a rough illustration of the previous child ordering behavior, consider the following entity hierarchy:
```
Root (EID 9999)
|_ Child1 (EID 2)
|_ Child2 (EID 3)
|_ Child3 (EID 4)
```
With an representations like the following pseudocode:
```
// EditorEntityModel representation
EditorEntityModelEntry root;
root.children[0] = 2;
root.children[1] = 3;
root.children[2] = 4;

// EditorOutlinerListModel representation
// row, column, user data (64 bit uint)
child1 = QModelIndex(0, 0, 2)
child2 = QModelIndex(1, 0, 3)
child3 = QModelIndex(2, 0, 4)
```

When removing a child, the `EditorEntityModel` used to do roughly the following:
```
// Swap and pop the last child
int indexToRemove = 0;
swap(root.children[indexToRemove], root.children[root.children.size() - 1]);
root.children.resize(root.children.size() - 1);
model.notifyRemoved(root, indexToRemove); // model removes the row indicated

// Leading to this EditorEntityModel state
root.children[0] = 4;
root.children[1] = 3;

// And this EntityOutlinerListModel state, note that the row indices are swapped from the indices in the backing storage
child2 = QModelIndex(0, 0, 3)
child3 = QModelIndex(1, 0, 4)
```

A QModelIndex having a row that doesn't match its underlying data is undefined behavior, and was the source of an intermittent crash in our `QSortFilterProxyModel` as subsequent updates to the wrong row led to an invalid proxy state.

Signed-off-by: nvsickle <nvsickle@amazon.com>
This commit is contained in:
nvsickle
2021-10-17 16:55:55 -07:00
parent fcd6360c26
commit f0e6841ca8
5 changed files with 271 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