From 9a9526a069f66e09b339ace5cf90cec7d8ddf11f Mon Sep 17 00:00:00 2001
From: codereader <greebo@angua.at>
Date: Sun, 6 Mar 2022 05:51:20 +0100
Subject: [PATCH] #5912: Unit tests covering IGeometryStore::getBounds(), which
 is only considering referenced vertices in the bounds calculation now.

---
 include/igeometrystore.h    |  6 ++-
 libs/render/GeometryStore.h | 13 +++++--
 test/GeometryStore.cpp      | 77 +++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/include/igeometrystore.h b/include/igeometrystore.h
index c219c31092..56ec69b087 100644
--- a/include/igeometrystore.h
+++ b/include/igeometrystore.h
@@ -142,7 +142,11 @@ class IGeometryStore
     // for a certain amount of time, at the latest until allocateSlot or deallocateSlot are invoked.
     virtual RenderParameters getRenderParameters(Slot slot) = 0;
 
-    // Returns the bounds of the geometry stored in the given slot
+    /**
+     * Returns the bounds of the geometry stored in the given slot.
+     * Note that this will only take those vertices into account 
+     * that are actually referenced by any index in the slot.
+     */ 
     virtual AABB getBounds(Slot slot) = 0;
 };
 
diff --git a/libs/render/GeometryStore.h b/libs/render/GeometryStore.h
index 0b7a963d2f..1996c9f754 100644
--- a/libs/render/GeometryStore.h
+++ b/libs/render/GeometryStore.h
@@ -229,17 +229,22 @@ class GeometryStore :
 
     AABB getBounds(Slot slot) override
     {
-        auto vertexSlot = GetVertexSlot(slot);
         auto& current = getCurrentBuffer();
 
+        // Acquire the slot containing the vertices
+        auto vertexSlot = GetVertexSlot(slot);
         auto vertex = current.vertices.getBufferStart() + current.vertices.getOffset(vertexSlot);
-        auto numVertices = current.vertices.getSize(vertexSlot);
+
+        // Get the indices and use them to iterate over the vertices
+        auto indexSlot = GetIndexSlot(slot); 
+        auto indexPointer = current.indices.getBufferStart() + current.indices.getOffset(indexSlot);
+        auto numIndices = current.indices.getNumUsedElements(indexSlot);
 
         AABB bounds;
 
-        for (auto i = 0; i < numVertices; ++i, ++vertex)
+        for (auto i = 0; i < numIndices; ++i, ++indexPointer)
         {
-            bounds.includePoint(vertex->vertex);
+            bounds.includePoint(vertex[*indexPointer].vertex);
         }
 
         return bounds;
diff --git a/test/GeometryStore.cpp b/test/GeometryStore.cpp
index 2e1d27b366..af1635d9f8 100644
--- a/test/GeometryStore.cpp
+++ b/test/GeometryStore.cpp
@@ -627,4 +627,81 @@ TEST(GeometryStore, ResizeIndexRemapData)
     EXPECT_THROW(store.resizeData(secondarySlot, 6, remap.size()), std::logic_error);
 }
 
+TEST(GeometryStore, RegularSlotBounds)
+{
+    render::GeometryStore store(NullSyncObjectProvider::Instance());
+
+    // Allocate a slot to hold indexed vertices
+    auto vertices = generateVertices(3, 15 * 20);
+    auto indices = generateIndices(vertices);
+
+    auto slot = store.allocateSlot(vertices.size(), indices.size());
+    store.updateData(slot, vertices, indices);
+
+    // This slot's indices are referencing all vertices, so the bounds
+    // calculated should match the bounds of the entire vertex set.
+    AABB localBounds;
+    for (const auto& vertex : vertices)
+    {
+        localBounds.includePoint(vertex.vertex);
+    }
+
+    auto slotBounds = store.getBounds(slot);
+
+    EXPECT_TRUE(math::isNear(slotBounds.getOrigin(), localBounds.getOrigin(), 0.01)) << "Bounds origin mismatch";
+    EXPECT_TRUE(math::isNear(slotBounds.getExtents(), localBounds.getExtents(), 0.01)) << "Bounds extents mismatch";
+
+    // Store a new set of indices in this slot that is just using every second vertex
+    std::vector<unsigned int> newIndices;
+    for (auto i = 0; i < vertices.size(); i += 2)
+    {
+        newIndices.push_back(i);
+    }
+
+    store.updateData(slot, vertices, newIndices);
+
+    localBounds = AABB();
+    for (auto index : newIndices)
+    {
+        localBounds.includePoint(vertices[index].vertex);
+    }
+
+    slotBounds = store.getBounds(slot);
+
+    EXPECT_TRUE(math::isNear(slotBounds.getOrigin(), localBounds.getOrigin(), 0.01)) << "Bounds origin mismatch";
+    EXPECT_TRUE(math::isNear(slotBounds.getExtents(), localBounds.getExtents(), 0.01)) << "Bounds extents mismatch";
+}
+
+TEST(GeometryStore, IndexRemappingSlotBounds)
+{
+    render::GeometryStore store(NullSyncObjectProvider::Instance());
+
+    // Allocate a slot to hold indexed vertices
+    auto vertices = generateVertices(3, 15 * 20);
+    auto indices = generateIndices(vertices);
+
+    auto primarySlot = store.allocateSlot(vertices.size(), indices.size());
+    store.updateData(primarySlot, vertices, indices);
+
+    // Set up a remapping slot with a smaller set of indices referencing the primary slot vertices
+    std::vector<unsigned int> newIndices(vertices.size() / 4);
+    std::iota(newIndices.begin(), newIndices.end(), 0);
+
+    auto indexSlot = store.allocateIndexSlot(primarySlot, newIndices.size());
+    store.updateIndexData(indexSlot, newIndices);
+
+    // Calculate the bounds of our offline mapping
+    AABB localBounds;
+    for (auto index : newIndices)
+    {
+        localBounds.includePoint(vertices[index].vertex);
+    }
+
+    // Query the bounds of this index slot, it should be the same
+    auto slotBounds = store.getBounds(indexSlot);
+
+    EXPECT_TRUE(math::isNear(slotBounds.getOrigin(), localBounds.getOrigin(), 0.01)) << "Bounds origin mismatch";
+    EXPECT_TRUE(math::isNear(slotBounds.getExtents(), localBounds.getExtents(), 0.01)) << "Bounds extents mismatch";
+}
+
 }