You cannot select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
f0e6841ca8
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> |
4 years ago | |
|---|---|---|
| .. | ||
| Entity | 4 years ago | |
| EntityOwnershipService | 5 years ago | |
| FocusMode | 4 years ago | |
| Prefab | 4 years ago | |
| Script | 4 years ago | |
| SliceStabilityTests | 4 years ago | |
| ToolsComponents | 4 years ago | |
| UI | 4 years ago | |
| Viewport | 4 years ago | |
| Visibility | 5 years ago | |
| ArchiveTests.cpp | 4 years ago | |
| AssetFileInfoListComparison.cpp | 4 years ago | |
| AssetSeedManager.cpp | 4 years ago | |
| AssetSystemMocks.h | 4 years ago | |
| AssetUtils.cpp | 5 years ago | |
| BoundsTestComponent.cpp | 4 years ago | |
| BoundsTestComponent.h | 4 years ago | |
| ComponentAdapterTests.cpp | 5 years ago | |
| ComponentAddRemove.cpp | 4 years ago | |
| ComponentModeTestDoubles.cpp | 5 years ago | |
| ComponentModeTestDoubles.h | 5 years ago | |
| ComponentModeTestFixture.cpp | 4 years ago | |
| ComponentModeTestFixture.h | 5 years ago | |
| ComponentModeTests.cpp | 5 years ago | |
| EditorTransformComponentSelectionTests.cpp | 4 years ago | |
| EditorVertexSelectionTests.cpp | 5 years ago | |
| EntityIdQLabelTests.cpp | 4 years ago | |
| EntityInspectorTests.cpp | 4 years ago | |
| EntityTestbed.h | 4 years ago | |
| FileFunc.cpp | 4 years ago | |
| FingerprintingTests.cpp | 5 years ago | |
| GenericComponentWrapperTest.cpp | 5 years ago | |
| InstanceDataHierarchy.cpp | 4 years ago | |
| IntegerPrimtitiveTestConfig.h | 5 years ago | |
| LogLines.cpp | 4 years ago | |
| Main.cpp | 5 years ago | |
| ManipulatorBoundsTests.cpp | 4 years ago | |
| ManipulatorCoreTests.cpp | 5 years ago | |
| ManipulatorViewTests.cpp | 5 years ago | |
| PerforceComponentTests.cpp | 5 years ago | |
| PlatformAddressedAssetCatalogTests.cpp | 4 years ago | |
| PropertyIntCtrlCommonTests.cpp | 5 years ago | |
| PropertyIntCtrlCommonTests.h | 5 years ago | |
| PropertyIntSliderCtrlTests.cpp | 5 years ago | |
| PropertyIntSpinCtrlTests.cpp | 5 years ago | |
| PropertyTreeEditorTests.cpp | 4 years ago | |
| PythonBindingTests.cpp | 5 years ago | |
| QtWidgetLimitsTests.cpp | 5 years ago | |
| SQLiteConnectionTests.cpp | 4 years ago | |
| Slice.cpp | 4 years ago | |
| SliceUpgradeTests.cpp | 5 years ago | |
| SliceUpgradeTestsData.h | 5 years ago | |
| Slices.cpp | 5 years ago | |
| SpinBoxTests.cpp | 4 years ago | |
| ThumbnailerTests.cpp | 5 years ago | |
| TransformComponent.cpp | 4 years ago | |
| UndoStack.cpp | 4 years ago | |
| aztoolsframeworktests_files.cmake | 4 years ago | |