Revise buffer traversal in bounding volume calculation

Take the index attribute (and buffer) into account, when present.

Rely on QGeometryRenderer::vertexCount(), when set.

Initially done on dev as: 1d874d33da9174a82fccc266c66a22af006ac8ef

Task-number: QTBUG-65590
Change-Id: I98d1865e3581272471d6b93b9633da38dc77163d
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>
Reviewed-by: Laszlo Agocs <laszlo.agocs@qt.io>

3 files changed, 278 insertions, 66 deletions

diff --git a/src/render/backend/buffervisitor_p.h b/src/render/backend/buffervisitor_p.h
index 7149e21ae..97851dab0 100644
--- a/src/render/backend/buffervisitor_p.h
+++ b/src/render/backend/buffervisitor_p.h
@@ -95,39 +95,7 @@ public:
         Q_UNUSED(ndx); Q_UNUSED(x); Q_UNUSED(y); Q_UNUSED(z); Q_UNUSED(w);
     }
 
-    bool apply(const GeometryRenderer *renderer, const QString &attributeName)
-    {
-        if (renderer == nullptr || renderer->instanceCount() != 1) {
-            return false;
-        }
-
-        Geometry *geom = m_manager->lookupResource<Geometry, GeometryManager>(renderer->geometryId());
-
-        if (!geom)
-            return false;
-
-        Attribute *attribute = nullptr;
-
-        const auto attrIds = geom->attributes();
-        for (const Qt3DCore::QNodeId attrId : attrIds) {
-            attribute = m_manager->lookupResource<Attribute, AttributeManager>(attrId);
-            if (attribute){
-                if (attribute->name() == attributeName
-                        || (attributeName == QStringLiteral("default")
-                            && attribute->name() == QAttribute::defaultTextureCoordinateAttributeName())) {
-                    break;
-                }
-            }
-            attribute = nullptr;
-        }
-
-        if (!attribute)
-            return false;
-
-        return apply(attribute);
-    }
-
-    bool apply(Qt3DRender::Render::Attribute *attribute)
+    bool apply(Qt3DRender::Render::Attribute *attribute, Qt3DRender::Render::Attribute *indexAttribute, int drawVertexCount)
     {
         if (attribute->vertexBaseType() != VertexBaseType)
             return false;
@@ -136,12 +104,36 @@ public:
 
         auto data = m_manager->lookupResource<Buffer, BufferManager>(attribute->bufferId())->data();
         auto buffer = BufferTypeInfo::castToType<VertexBaseType>(data, attribute->byteOffset());
-        switch (dataSize) {
-        case 1: traverseCoordinates1(buffer, attribute->byteStride(), attribute->count()); break;
-        case 2: traverseCoordinates2(buffer, attribute->byteStride(), attribute->count()); break;
-        case 3: traverseCoordinates3(buffer, attribute->byteStride(), attribute->count()); break;
-        case 4: traverseCoordinates4(buffer, attribute->byteStride(), attribute->count()); break;
-        default: Q_UNREACHABLE();
+
+        if (indexAttribute) {
+            auto indexData = m_manager->lookupResource<Buffer, BufferManager>(indexAttribute->bufferId())->data();
+            if (indexAttribute->vertexBaseType() == QAttribute::UnsignedShort) {
+                auto indexBuffer = BufferTypeInfo::castToType<QAttribute::UnsignedShort>(indexData, indexAttribute->byteOffset());
+                switch (dataSize) {
+                case 1: traverseCoordinates1Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                case 2: traverseCoordinates2Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                case 3: traverseCoordinates3Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                case 4: traverseCoordinates4Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                default: Q_UNREACHABLE();
+                }
+            } else {
+                auto indexBuffer = BufferTypeInfo::castToType<QAttribute::UnsignedInt>(indexData, indexAttribute->byteOffset());
+                switch (dataSize) {
+                case 1: traverseCoordinates1Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                case 2: traverseCoordinates2Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                case 3: traverseCoordinates3Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                case 4: traverseCoordinates4Indexed(buffer, attribute->byteStride(), indexBuffer, drawVertexCount); break;
+                default: Q_UNREACHABLE();
+                }
+            }
+        } else {
+            switch (dataSize) {
+            case 1: traverseCoordinates1(buffer, attribute->byteStride(), drawVertexCount); break;
+            case 2: traverseCoordinates2(buffer, attribute->byteStride(), drawVertexCount); break;
+            case 3: traverseCoordinates3(buffer, attribute->byteStride(), drawVertexCount); break;
+            case 4: traverseCoordinates4(buffer, attribute->byteStride(), drawVertexCount); break;
+            default: Q_UNREACHABLE();
+            }
         }
 
         return true;
@@ -161,6 +153,19 @@ protected:
         }
     }
 
+    template <typename Coordinate, typename IndexElem>
+    void traverseCoordinates1Indexed(Coordinate *coordinates,
+                                     const uint byteStride,
+                                     IndexElem *indices,
+                                     const uint count)
+    {
+        const uint stride = byteStride / sizeof(Coordinate);
+        for (uint i = 0; i < count; ++i) {
+            const uint n = stride * indices[i];
+            visit(i, coordinates[n]);
+        }
+    }
+
     template <typename Coordinate>
     void traverseCoordinates2(Coordinate *coordinates,
                               const uint byteStride,
@@ -173,6 +178,20 @@ protected:
         }
     }
 
+
+    template <typename Coordinate, typename IndexElem>
+    void traverseCoordinates2Indexed(Coordinate *coordinates,
+                                     const uint byteStride,
+                                     IndexElem *indices,
+                                     const uint count)
+    {
+        const uint stride = byteStride / sizeof(Coordinate);
+        for (uint i = 0; i < count; ++i) {
+            const uint n = stride * indices[i];
+            visit(i, coordinates[n], coordinates[n + 1]);
+        }
+    }
+
     template <typename Coordinate>
     void traverseCoordinates3(Coordinate *coordinates,
                               const uint byteStride,
@@ -185,6 +204,19 @@ protected:
         }
     }
 
+    template <typename Coordinate, typename IndexElem>
+    void traverseCoordinates3Indexed(Coordinate *coordinates,
+                                     const uint byteStride,
+                                     IndexElem *indices,
+                                     const uint count)
+    {
+        const uint stride = byteStride / sizeof(Coordinate);
+        for (uint i = 0; i < count; ++i) {
+            const uint n = stride * indices[i];
+            visit(i, coordinates[n], coordinates[n + 1], coordinates[n + 2]);
+        }
+    }
+
     template <typename Coordinate>
     void traverseCoordinates4(Coordinate *coordinates,
                               const uint byteStride,
@@ -197,6 +229,19 @@ protected:
         }
     }
 
+    template <typename Coordinate, typename IndexElem>
+    void traverseCoordinates4Indexed(Coordinate *coordinates,
+                                     const uint byteStride,
+                                     IndexElem *indices,
+                                     const uint count)
+    {
+        const uint stride = byteStride / sizeof(Coordinate);
+        for (uint i = 0; i < count; ++i) {
+            const uint n = stride * indices[i];
+            visit(i, coordinates[n], coordinates[n + 1], coordinates[n + 2], coordinates[n + 3]);
+        }
+    }
+
     NodeManagers *m_manager;
 };
 
diff --git a/src/render/jobs/calcboundingvolumejob.cpp b/src/render/jobs/calcboundingvolumejob.cpp
index 7bbab307c..0a6e5dfca 100644
--- a/src/render/jobs/calcboundingvolumejob.cpp
+++ b/src/render/jobs/calcboundingvolumejob.cpp
@@ -81,10 +81,10 @@ public:
 
     const Sphere& result() { return m_volume; }
 
-    bool apply(Qt3DRender::Render::Attribute *positionAttribute)
+    bool apply(Qt3DRender::Render::Attribute *positionAttribute, Qt3DRender::Render::Attribute *indexAttribute, int drawVertexCount)
     {
         FindExtremePoints findExtremePoints(m_manager);
-        if (!findExtremePoints.apply(positionAttribute))
+        if (!findExtremePoints.apply(positionAttribute, indexAttribute, drawVertexCount))
             return false;
 
         // Calculate squared distance for the pairs of points
@@ -109,7 +109,7 @@ public:
         m_volume.setRadius((q - c).length());
 
         ExpandSphere expandSphere(m_manager, m_volume);
-        if (!expandSphere.apply(positionAttribute))
+        if (!expandSphere.apply(positionAttribute, indexAttribute, drawVertexCount))
             return false;
 
         return true;
@@ -195,6 +195,8 @@ void calculateLocalBoundingVolume(NodeManagers *manager, Entity *node)
         Geometry *geom = manager->lookupResource<Geometry, GeometryManager>(gRenderer->geometryId());
 
         if (geom) {
+            int drawVertexCount = gRenderer->vertexCount(); // may be 0, gets changed below if so
+
             Qt3DRender::Render::Attribute *positionAttribute = manager->lookupResource<Attribute, AttributeManager>(geom->boundingPositionAttribute());
 
             // Use the default position attribute if attribute is null
@@ -212,36 +214,66 @@ void calculateLocalBoundingVolume(NodeManagers *manager, Entity *node)
                     || positionAttribute->attributeType() != QAttribute::VertexAttribute
                     || positionAttribute->vertexBaseType() != QAttribute::Float
                     || positionAttribute->vertexSize() < 3) {
-                qWarning() << "QGeometry::boundingVolumePositionAttribute position Attribute not suited for bounding volume computation";
+                qWarning("calculateLocalBoundingVolume: Position attribute not suited for bounding volume computation");
                 return;
             }
 
-            if (positionAttribute) {
-                Buffer *buf = manager->lookupResource<Buffer, BufferManager>(positionAttribute->bufferId());
-                // No point in continuing if the positionAttribute doesn't have a suitable buffer
-                if (!buf) {
-                    qWarning() << "ObjectPicker position Attribute not referencing a valid buffer";
-                    return;
-                }
+            Buffer *buf = manager->lookupResource<Buffer, BufferManager>(positionAttribute->bufferId());
+            // No point in continuing if the positionAttribute doesn't have a suitable buffer
+            if (!buf) {
+                qWarning("calculateLocalBoundingVolume: Position attribute not referencing a valid buffer");
+                return;
+            }
+
+            // Check if there is an index attribute.
+            Qt3DRender::Render::Attribute *indexAttribute = nullptr;
+            Buffer *indexBuf = nullptr;
+            const QVector<Qt3DCore::QNodeId> attributes = geom->attributes();
+
+            for (Qt3DCore::QNodeId attrNodeId : attributes) {
+                Qt3DRender::Render::Attribute *attr = manager->lookupResource<Attribute, AttributeManager>(attrNodeId);
+                if (attr && attr->attributeType() == Qt3DRender::QAttribute::IndexAttribute) {
+                    indexBuf = manager->lookupResource<Buffer, BufferManager>(attr->bufferId());
+                    if (indexBuf) {
+                        indexAttribute = attr;
 
-                // Buf will be set to not dirty once it's loaded
-                // in a job executed after this one
-                // We need to recompute the bounding volume
-                // If anything in the GeometryRenderer has changed
-                if (buf->isDirty() ||
-                        node->isBoundingVolumeDirty() ||
-                        positionAttribute->isDirty() ||
-                        geom->isDirty() ||
-                        gRenderer->isDirty()) {
-
-                    BoundingVolumeCalculator reader(manager);
-                    if (reader.apply(positionAttribute)) {
-                        node->localBoundingVolume()->setCenter(reader.result().center());
-                        node->localBoundingVolume()->setRadius(reader.result().radius());
-                        node->unsetBoundingVolumeDirty();
+                        if (!drawVertexCount)
+                            drawVertexCount = indexAttribute->count();
+
+                        if (indexAttribute->vertexBaseType() != QAttribute::UnsignedShort
+                                && indexAttribute->vertexBaseType() != QAttribute::UnsignedInt)
+                        {
+                            qWarning("calculateLocalBoundingVolume: Unsupported index attribute type");
+                            return;
+                        }
+
+                        break;
                     }
                 }
             }
+
+            if (!indexAttribute && !drawVertexCount)
+                drawVertexCount = positionAttribute->count();
+
+            // Buf will be set to not dirty once it's loaded
+            // in a job executed after this one
+            // We need to recompute the bounding volume
+            // If anything in the GeometryRenderer has changed
+            if (buf->isDirty() ||
+                    node->isBoundingVolumeDirty() ||
+                    positionAttribute->isDirty() ||
+                    geom->isDirty() ||
+                    gRenderer->isDirty() ||
+                    (indexAttribute && indexAttribute->isDirty()) ||
+                    (indexBuf && indexBuf->isDirty()))
+            {
+                BoundingVolumeCalculator reader(manager);
+                if (reader.apply(positionAttribute, indexAttribute, drawVertexCount)) {
+                    node->localBoundingVolume()->setCenter(reader.result().center());
+                    node->localBoundingVolume()->setRadius(reader.result().radius());
+                    node->unsetBoundingVolumeDirty();
+                }
+            }
         }
     }
 
diff --git a/tests/auto/render/boundingsphere/tst_boundingsphere.cpp b/tests/auto/render/boundingsphere/tst_boundingsphere.cpp
index fcbfaf6ba..767df4604 100644
--- a/tests/auto/render/boundingsphere/tst_boundingsphere.cpp
+++ b/tests/auto/render/boundingsphere/tst_boundingsphere.cpp
@@ -65,6 +65,8 @@
 #include <Qt3DExtras/qcuboidmesh.h>
 #include <Qt3DExtras/qplanemesh.h>
 
+#include <qbackendnodetester.h>
+
 QT_BEGIN_NAMESPACE
 
 namespace Qt3DRender {
@@ -100,6 +102,7 @@ public:
     Qt3DRender::Render::FrameGraphNode *frameGraphRoot() const { return d_func()->m_renderer->frameGraphRoot(); }
     Qt3DRender::Render::RenderSettings *renderSettings() const { return d_func()->m_renderer->settings(); }
     Qt3DRender::Render::Entity *sceneRoot() const { return m_sceneRoot; }
+    Qt3DRender::Render::AbstractRenderer *renderer() const { return d_func()->m_renderer; }
 
 private:
     Render::Entity *m_sceneRoot;
@@ -153,7 +156,7 @@ void runRequiredJobs(Qt3DRender::TestAspect *test)
 
 } // anonymous
 
-class tst_BoundingSphere : public QObject
+class tst_BoundingSphere : public Qt3DCore::QBackendNodeTester
 {
     Q_OBJECT
 private:
@@ -194,6 +197,138 @@ private Q_SLOTS:
         QVERIFY(qAbs(boundingSphere->center().y() - sphereCenter.y()) < 0.000001f);
         QVERIFY(qAbs(boundingSphere->center().z() - sphereCenter.z()) < 0.000001f);
     }
+
+    void checkCustomGeometry_data()
+    {
+        QTest::addColumn<int>("drawVertexCount");
+        QTest::addColumn<int>("indexByteOffset");
+        QTest::addColumn<QVector3D>("expectedCenter");
+        QTest::addColumn<float>("expectedRadius");
+        QTest::newRow("all") << 0 << 0 << QVector3D(-0.488892f, 0.0192147f, -75.4804f) << 25.5442f;
+        QTest::newRow("first only") << 3 << 0 << QVector3D(0, 1, -100) << 1.0f;
+        QTest::newRow("second only") << 3 << int(3 * sizeof(ushort)) << QVector3D(0, -1, -50) << 1.0f;
+    }
+
+    void checkCustomGeometry()
+    {
+        QFETCH(int, drawVertexCount);
+        QFETCH(int, indexByteOffset);
+        QFETCH(QVector3D, expectedCenter);
+        QFETCH(float, expectedRadius);
+
+        // two triangles with different Z, and an index buffer
+        QByteArray vdata;
+        vdata.resize(6 * 3 * sizeof(float));
+        float *vp = reinterpret_cast<float *>(vdata.data());
+        *vp++ = -1.0f;
+        *vp++ = 1.0f;
+        *vp++ = -100.0f;
+        *vp++ = 0.0f;
+        *vp++ = 0.0f;
+        *vp++ = -100.0f;
+        *vp++ = 1.0f;
+        *vp++ = 1.0f;
+        *vp++ = -100.0f;
+
+        *vp++ = -1.0f;
+        *vp++ = -1.0f;
+        *vp++ = -50.0f;
+        *vp++ = 0.0f;
+        *vp++ = 0.0f;
+        *vp++ = -50.0f;
+        *vp++ = 1.0f;
+        *vp++ = -1.0f;
+        *vp++ = -50.0f;
+
+        QByteArray idata;
+        idata.resize(6 * sizeof(ushort));
+        ushort *ip = reinterpret_cast<ushort *>(idata.data());
+        *ip++ = 0;
+        *ip++ = 1;
+        *ip++ = 2;
+        *ip++ = 3;
+        *ip++ = 4;
+        *ip++ = 5;
+
+        QScopedPointer<Qt3DCore::QEntity> entity(new Qt3DCore::QEntity);
+        QScopedPointer<Qt3DRender::TestAspect> test(new Qt3DRender::TestAspect(entity.data()));
+        Qt3DRender::QBuffer *vbuffer = new Qt3DRender::QBuffer;
+        Qt3DRender::QBuffer *ibuffer = new Qt3DRender::QBuffer;
+
+        vbuffer->setData(vdata);
+        Qt3DRender::Render::Buffer *vbufferBackend = test->nodeManagers()->bufferManager()->getOrCreateResource(vbuffer->id());
+        vbufferBackend->setRenderer(test->renderer());
+        vbufferBackend->setManager(test->nodeManagers()->bufferManager());
+        simulateInitialization(vbuffer, vbufferBackend);
+
+        ibuffer->setData(idata);
+        Qt3DRender::Render::Buffer *ibufferBackend = test->nodeManagers()->bufferManager()->getOrCreateResource(ibuffer->id());
+        ibufferBackend->setRenderer(test->renderer());
+        ibufferBackend->setManager(test->nodeManagers()->bufferManager());
+        simulateInitialization(ibuffer, ibufferBackend);
+
+        Qt3DRender::QGeometry *g = new Qt3DRender::QGeometry;
+        for (int i = 0; i < 2; ++i)
+            g->addAttribute(new Qt3DRender::QAttribute);
+
+        const QVector<Qt3DRender::QAttribute *> attrs = g->attributes();
+        Qt3DRender::QAttribute *attr = attrs[0];
+        attr->setBuffer(vbuffer);
+        attr->setName(Qt3DRender::QAttribute::defaultPositionAttributeName());
+        attr->setVertexBaseType(Qt3DRender::QAttribute::Float);
+        attr->setVertexSize(3);
+        attr->setCount(6);
+        attr->setByteOffset(0);
+        attr->setByteStride(3 * sizeof(float));
+
+        attr = attrs[1];
+        attr->setBuffer(ibuffer);
+        attr->setAttributeType(Qt3DRender::QAttribute::IndexAttribute);
+        attr->setVertexBaseType(Qt3DRender::QAttribute::UnsignedShort);
+        attr->setVertexSize(1);
+        attr->setCount(6);
+        attr->setByteOffset(indexByteOffset);
+
+        Qt3DRender::QGeometryRenderer *gr = new Qt3DRender::QGeometryRenderer;
+        gr->setVertexCount(drawVertexCount); // when 0, indexAttribute->count() is used instead
+        gr->setGeometry(g);
+        entity->addComponent(gr);
+
+        Qt3DRender::Render::Attribute *attr0Backend = test->nodeManagers()->attributeManager()->getOrCreateResource(attrs[0]->id());
+        attr0Backend->setRenderer(test->renderer());
+        simulateInitialization(attrs[0], attr0Backend);
+        Qt3DRender::Render::Attribute *attr1Backend = test->nodeManagers()->attributeManager()->getOrCreateResource(attrs[1]->id());
+        attr1Backend->setRenderer(test->renderer());
+        simulateInitialization(attrs[1], attr1Backend);
+
+        Qt3DRender::Render::Geometry *gBackend = test->nodeManagers()->geometryManager()->getOrCreateResource(g->id());
+        gBackend->setRenderer(test->renderer());
+        simulateInitialization(g, gBackend);
+
+        Qt3DRender::Render::GeometryRenderer *grBackend = test->nodeManagers()->geometryRendererManager()->getOrCreateResource(gr->id());
+        grBackend->setRenderer(test->renderer());
+        grBackend->setManager(test->nodeManagers()->geometryRendererManager());
+        simulateInitialization(gr, grBackend);
+
+        Qt3DRender::Render::Entity *entityBackend = test->nodeManagers()->renderNodesManager()->getOrCreateResource(entity->id());
+        entityBackend->setRenderer(test->renderer());
+        simulateInitialization(entity.data(), entityBackend);
+
+        Qt3DRender::Render::CalculateBoundingVolumeJob calcBVolume;
+        calcBVolume.setManagers(test->nodeManagers());
+        calcBVolume.setRoot(test->sceneRoot());
+        calcBVolume.run();
+
+        QVector3D center = entityBackend->localBoundingVolume()->center();
+        float radius = entityBackend->localBoundingVolume()->radius();
+        qDebug() << radius << center;
+
+        // truncate and compare integers only
+        QVERIFY(int(radius) == int(expectedRadius));
+        QVERIFY(int(center.x()) == int(expectedCenter.x()));
+        QVERIFY(int(center.y()) == int(expectedCenter.y()));
+        QVERIFY(int(center.z()) == int(expectedCenter.z()));
+    }
 };
 
 QTEST_MAIN(tst_BoundingSphere)