From 838202873a00a9e2b9cf22cb66c3f68d885401bf Mon Sep 17 00:00:00 2001 From: Tommy Walton <82672795+amzn-tommy@users.noreply.github.com> Date: Mon, 24 May 2021 10:10:50 -0700 Subject: [PATCH] Fix for ATOM-15595 : OctreeNode silently evicts entries that are larger than the size of the root node (#870) - Fixed a bug where an entry would get removed from the octree when being updated if it was too large to be fully contained by the root node (the desired behavior is that it just lives in the root node) - Added a unit test to ensure that large entries can exist in the root node - Updated the unit tests to manually count the number of entries instead of relying on GetEntryCount, since GetEntryCount was reporting an unreliable count before this bug was fixed. --- .../Visibility/OctreeSystemComponent.cpp | 3 +- Code/Framework/Tests/OctreeTests.cpp | 95 +++++++++++++++---- 2 files changed, 79 insertions(+), 19 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Visibility/OctreeSystemComponent.cpp b/Code/Framework/AzFramework/AzFramework/Visibility/OctreeSystemComponent.cpp index cdb7ce0dd9..9ca17a6736 100644 --- a/Code/Framework/AzFramework/AzFramework/Visibility/OctreeSystemComponent.cpp +++ b/Code/Framework/AzFramework/AzFramework/Visibility/OctreeSystemComponent.cpp @@ -123,8 +123,9 @@ namespace AzFramework OctreeNode* insertCheck = this; while (insertCheck != nullptr) { - if (AZ::ShapeIntersection::Contains(insertCheck->m_bounds, boundingVolume)) + if (AZ::ShapeIntersection::Contains(insertCheck->m_bounds, boundingVolume) || !insertCheck->m_parent) { + // Insert here if the entry is fully contained or if we've reached the root node return insertCheck->Insert(octreeScene, entry); } insertCheck = insertCheck->m_parent; diff --git a/Code/Framework/Tests/OctreeTests.cpp b/Code/Framework/Tests/OctreeTests.cpp index 29c7ef3ff3..27c4fdf0eb 100644 --- a/Code/Framework/Tests/OctreeTests.cpp +++ b/Code/Framework/Tests/OctreeTests.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -94,6 +95,20 @@ namespace UnitTest AZ::Console* m_console; }; + void ValidateEntryCountEqualsExpectedCount(const IVisibilityScene* visScene, uint32_t expectedEntryCount) + { + // InsertOrUpdateEntry assumes that updating an existing entry won't change the count + // so it doesn't modify the counter used by GetEntryCount. + // If an entry is removed from the octree as an unintended side effect of updating an existing entry, + // GetEntryCount can't be relied upon to report the actual entry count. + // So manually count the entries when using the entry count for validation. + uint32_t manualEntryCount = 0; + visScene->EnumerateNoCull([&manualEntryCount](const AzFramework::IVisibilityScene::NodeData& nodeData) { manualEntryCount += nodeData.m_entries.size(); }); + + EXPECT_EQ(manualEntryCount, expectedEntryCount); + EXPECT_EQ(visScene->GetEntryCount(), expectedEntryCount); + } + TEST_F(OctreeTests, InsertDeleteSingleEntry) { AzFramework::VisibilityEntry visEntry; @@ -102,11 +117,11 @@ namespace UnitTest m_octreeScene->InsertOrUpdateEntry(visEntry); EXPECT_TRUE(visEntry.m_internalNode != nullptr); EXPECT_TRUE(visEntry.m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); m_octreeScene->RemoveEntry(visEntry); EXPECT_TRUE(visEntry.m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 0); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 0); EXPECT_TRUE(true); //TEST } @@ -121,34 +136,34 @@ namespace UnitTest m_octreeScene->InsertOrUpdateEntry(visEntry[0]); EXPECT_TRUE(visEntry[0].m_internalNode != nullptr); EXPECT_TRUE(visEntry[0].m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); m_octreeScene->InsertOrUpdateEntry(visEntry[1]); // This should force a split of the root node EXPECT_TRUE(visEntry[1].m_internalNode != nullptr); EXPECT_TRUE(visEntry[1].m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 2); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 2); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + m_octreeScene->GetChildNodeCount()); m_octreeScene->InsertOrUpdateEntry(visEntry[2]); // This should force a split of the roots +/+/+ child node EXPECT_TRUE(visEntry[2].m_internalNode != nullptr); EXPECT_TRUE(visEntry[2].m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 3); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 3); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + (2 * m_octreeScene->GetChildNodeCount())); m_octreeScene->RemoveEntry(visEntry[2]); EXPECT_TRUE(visEntry[2].m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 2); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 2); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + m_octreeScene->GetChildNodeCount()); m_octreeScene->RemoveEntry(visEntry[1]); EXPECT_TRUE(visEntry[1].m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); m_octreeScene->RemoveEntry(visEntry[0]); EXPECT_TRUE(visEntry[0].m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 0); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 0); } TEST_F(OctreeTests, UpdateSingleEntry) @@ -159,19 +174,19 @@ namespace UnitTest m_octreeScene->InsertOrUpdateEntry(visEntry); EXPECT_TRUE(visEntry.m_internalNode != nullptr); EXPECT_TRUE(visEntry.m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); visEntry.m_boundingVolume = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-0.5f), AZ::Vector3(0.5f)); m_octreeScene->InsertOrUpdateEntry(visEntry); EXPECT_TRUE(visEntry.m_internalNode != nullptr); EXPECT_TRUE(visEntry.m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); m_octreeScene->RemoveEntry(visEntry); EXPECT_TRUE(visEntry.m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 0); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 0); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); } @@ -185,19 +200,19 @@ namespace UnitTest m_octreeScene->InsertOrUpdateEntry(visEntry[0]); EXPECT_TRUE(visEntry[0].m_internalNode != nullptr); EXPECT_TRUE(visEntry[0].m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); m_octreeScene->InsertOrUpdateEntry(visEntry[1]); // This should force a split of the root node EXPECT_TRUE(visEntry[1].m_internalNode != nullptr); EXPECT_TRUE(visEntry[1].m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 2); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 2); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + m_octreeScene->GetChildNodeCount()); m_octreeScene->InsertOrUpdateEntry(visEntry[2]); // This should force a split of the roots +/+/+ child node EXPECT_TRUE(visEntry[2].m_internalNode != nullptr); EXPECT_TRUE(visEntry[2].m_internalNodeIndex == 0); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 3); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 3); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + (2 * m_octreeScene->GetChildNodeCount())); visEntry[1].m_boundingVolume = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-0.9f), AZ::Vector3(-0.6f)); @@ -206,22 +221,22 @@ namespace UnitTest m_octreeScene->InsertOrUpdateEntry(visEntry[0]); m_octreeScene->InsertOrUpdateEntry(visEntry[1]); m_octreeScene->InsertOrUpdateEntry(visEntry[2]); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 3); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 3); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + (2 * m_octreeScene->GetChildNodeCount())); m_octreeScene->RemoveEntry(visEntry[2]); EXPECT_TRUE(visEntry[2].m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 2); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 2); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1 + m_octreeScene->GetChildNodeCount()); m_octreeScene->RemoveEntry(visEntry[1]); EXPECT_TRUE(visEntry[1].m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 1); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 1); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); m_octreeScene->RemoveEntry(visEntry[0]); EXPECT_TRUE(visEntry[0].m_internalNode == nullptr); - EXPECT_TRUE(m_octreeScene->GetEntryCount() == 0); + ValidateEntryCountEqualsExpectedCount(m_octreeScene, 0); EXPECT_TRUE(m_octreeScene->GetNodeCount() == 1); } @@ -365,4 +380,48 @@ namespace UnitTest AZ::Frustum bound3 = AZ::Frustum(AZ::ViewFrustumAttributes(frustumTransform, 1.0f, 2.0f * atanf(0.5f), 2.6f, 2.9f)); EnumerateMultipleEntriesHelper(m_octreeScene, bound1, bound2, bound3); } + + TEST_F(OctreeTests, InsertOrUpdateEntry_OverFillRootNodeWithLargeEntries_EntriesAreNotLost) + { + // Validate that the octree works if you exceed the max entry count with large entries, + // which will overfill the root node since they can't be distributed to child nodes + + // Get the max extents and entries-per-node for the octree + AZ::IConsole* console = AZ::Interface::Get(); + EXPECT_TRUE(console); + + float maxExtents = 0.0f; + AZ::GetValueResult getCvarResult = console->GetCvarValue("bg_octreeMaxWorldExtents", maxExtents); + EXPECT_EQ(getCvarResult, AZ::GetValueResult::Success); + + uint32_t maxEntriesPerNode = 0; + getCvarResult = console->GetCvarValue("bg_octreeNodeMaxEntries", maxEntriesPerNode); + EXPECT_EQ(getCvarResult, AZ::GetValueResult::Success); + + // Create root entries that would exceed the size of the root node + AZ::Aabb exceedMaxExtents = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-maxExtents - 1.0f), AZ::Vector3(maxExtents + 1.0f)); + uint32_t exceedMaxEntriesPerNode = maxEntriesPerNode + 1; + + AzFramework::VisibilityEntry visEntry; + visEntry.m_boundingVolume = exceedMaxExtents; + AZStd::vector visEntries(exceedMaxEntriesPerNode, visEntry); + + // Insert them all into the scene + for (AzFramework::VisibilityEntry& entry : visEntries) + { + m_octreeScene->InsertOrUpdateEntry(entry); + } + + // Expect all the entries to be in the scene + ValidateEntryCountEqualsExpectedCount(m_octreeScene, visEntries.size()); + + // Update them, without making any actual changes + for (AzFramework::VisibilityEntry& entry : visEntries) + { + m_octreeScene->InsertOrUpdateEntry(entry); + } + + // Expect all the entries to be in the scene + ValidateEntryCountEqualsExpectedCount(m_octreeScene, visEntries.size()); + } }