Fix Buffer uploading

We used to load buffer data when creating it. This led to cases where
we would not take into account immediately buffer updates after creation.
This would then results in artifacts that would pop up on screen as actual
buffer data and count would be different

Change-Id: I5a2fad4fb5d7c1e1542cd0bf87ded16a111bc613
Task-number: QTBUG-60429
Reviewed-by: Oleg Evseev <ev.mipt@gmail.com>
Reviewed-by: Laszlo Agocs <laszlo.agocs@qt.io>

5 files changed, 36 insertions, 11 deletions

diff --git a/src/render/backend/renderer.cpp b/src/render/backend/renderer.cpp
index 4080a72eb..bd8687d5e 100644
--- a/src/render/backend/renderer.cpp
+++ b/src/render/backend/renderer.cpp
@@ -1013,12 +1013,11 @@ void Renderer::updateGLResources()
         const QVector<HBuffer> dirtyBufferHandles = std::move(m_dirtyBuffers);
         for (HBuffer handle: dirtyBufferHandles) {
             Buffer *buffer = m_nodesManager->bufferManager()->data(handle);
-            // Perform data upload
             // Forces creation if it doesn't exit
             if (!m_graphicsContext->hasGLBufferForBuffer(buffer))
                 m_graphicsContext->glBufferForRenderBuffer(buffer);
-            else if (buffer->isDirty()) // Otherwise update the glBuffer
-                m_graphicsContext->updateBuffer(buffer);
+            // Update the glBuffer data
+            m_graphicsContext->updateBuffer(buffer);
             buffer->unsetDirty();
         }
     }
diff --git a/src/render/geometry/buffer.cpp b/src/render/geometry/buffer.cpp
index ae5ede731..82dbbcc1b 100644
--- a/src/render/geometry/buffer.cpp
+++ b/src/render/geometry/buffer.cpp
@@ -88,6 +88,9 @@ void Buffer::executeFunctor()
 {
     Q_ASSERT(m_functor);
     m_data = (*m_functor)();
+    // Request data to be loaded
+    forceDataUpload();
+
     if (m_syncData) {
         // Send data back to the frontend
         auto e = Qt3DCore::QPropertyUpdatedChangePtr::create(peerId());
@@ -101,6 +104,8 @@ void Buffer::executeFunctor()
 //Called from th sendBufferJob
 void Buffer::updateDataFromGPUToCPU(QByteArray data)
 {
+    // Note: when this is called, data is what's currently in GPU memory
+    // so m_data shouldn't be reuploaded
     m_data = data;
     // Send data back to the frontend
     auto e = Qt3DCore::QPropertyUpdatedChangePtr::create(peerId());
@@ -121,12 +126,25 @@ void Buffer::initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr &chang
     m_access = data.access;
     m_bufferDirty = true;
 
+    if (!m_data.isEmpty())
+        forceDataUpload();
+
     m_functor = data.functor;
     Q_ASSERT(m_manager);
     if (m_functor)
         m_manager->addDirtyBuffer(peerId());
 }
 
+void Buffer::forceDataUpload()
+{
+    // We push back an update with offset = -1
+    // As this is the way to force data to be loaded
+    QBufferUpdate updateNewData;
+    updateNewData.offset = -1;
+    m_bufferUpdates.clear(); //previous updates are pointless
+    m_bufferUpdates.push_back(updateNewData);
+}
+
 void Buffer::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e)
 {
     if (e->type() == PropertyUpdated) {
@@ -136,13 +154,9 @@ void Buffer::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e)
             QByteArray newData = propertyChange->value().value<QByteArray>();
             bool dirty = m_data != newData;
             m_bufferDirty |= dirty;
-            if (dirty) {
-                QBufferUpdate updateNewData;
-                updateNewData.offset = -1;
-                m_bufferUpdates.clear(); //previous updates are pointless
-                m_bufferUpdates.push_back(updateNewData);
-            }
             m_data = newData;
+            if (dirty)
+                forceDataUpload();
         } else if (propertyName == QByteArrayLiteral("updateData")) {
             Qt3DRender::QBufferUpdate updateData = propertyChange->value().value<Qt3DRender::QBufferUpdate>();
             m_bufferUpdates.push_back(updateData);
diff --git a/src/render/geometry/buffer_p.h b/src/render/geometry/buffer_p.h
index 9d9606eb0..691d6cc60 100644
--- a/src/render/geometry/buffer_p.h
+++ b/src/render/geometry/buffer_p.h
@@ -90,6 +90,7 @@ public:
 
 private:
     void initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr &change) Q_DECL_FINAL;
+    void forceDataUpload();
 
     QBuffer::BufferType m_type;
     QBuffer::UsageType m_usage;
diff --git a/src/render/graphicshelpers/graphicscontext.cpp b/src/render/graphicshelpers/graphicscontext.cpp
index 65414e220..80e8267da 100644
--- a/src/render/graphicshelpers/graphicscontext.cpp
+++ b/src/render/graphicshelpers/graphicscontext.cpp
@@ -1501,8 +1501,6 @@ HGLBuffer GraphicsContext::createGLBufferFor(Buffer *buffer)
     if (!bindGLBuffer(b, bufferTypeToGLBufferType(buffer->type())))
         qCWarning(Render::Io) << Q_FUNC_INFO << "buffer binding failed";
 
-    // TO DO: Handle usage pattern
-    b->allocate(this, buffer->data().constData(), buffer->data().size(), false);
     return m_renderer->nodeManagers()->glBufferManager()->lookupHandle(buffer->peerId());
 }
 
@@ -1532,6 +1530,7 @@ void GraphicsContext::uploadDataToGLBuffer(Buffer *buffer, GLBuffer *b, bool rel
     QVector<Qt3DRender::QBufferUpdate> updates = std::move(buffer->pendingBufferUpdates());
     for (auto it = updates.begin(); it != updates.end(); ++it) {
         auto update = it;
+        // We have a partial update
         if (update->offset >= 0) {
             //accumulate sequential updates as single one
             int bufferSize = update->data.size();
@@ -1551,6 +1550,9 @@ void GraphicsContext::uploadDataToGLBuffer(Buffer *buffer, GLBuffer *b, bool rel
             // sometime use glMapBuffer rather than glBufferSubData
             b->update(this, update->data.constData(), update->data.size(), update->offset);
         } else {
+            // We have an update that was done by calling QBuffer::setData
+            // which is used to resize or entirely clear the buffer
+            // Note: we use the buffer data directly in that case
             const int bufferSize = buffer->data().size();
             b->allocate(this, bufferSize, false); // orphan the buffer
             b->allocate(this, buffer->data().constData(), bufferSize, false);
diff --git a/tests/auto/render/buffer/tst_buffer.cpp b/tests/auto/render/buffer/tst_buffer.cpp
index 7f7c0adb9..8049aaf37 100644
--- a/tests/auto/render/buffer/tst_buffer.cpp
+++ b/tests/auto/render/buffer/tst_buffer.cpp
@@ -89,6 +89,8 @@ private Q_SLOTS:
         QCOMPARE(renderBuffer.data(), buffer.data());
         QCOMPARE(renderBuffer.dataGenerator(), buffer.dataGenerator());
         QVERIFY(*renderBuffer.dataGenerator() == *buffer.dataGenerator());
+        QCOMPARE(renderBuffer.pendingBufferUpdates().size(), 1);
+        QCOMPARE(renderBuffer.pendingBufferUpdates().first().offset, -1);
     }
 
     void checkInitialAndCleanedUpState()
@@ -195,7 +197,10 @@ private Q_SLOTS:
         // THEN
         QCOMPARE(renderBuffer.data(), QByteArrayLiteral("LS9"));
         QVERIFY(renderBuffer.isDirty());
+        QCOMPARE(renderBuffer.pendingBufferUpdates().size(), 1);
+        QCOMPARE(renderBuffer.pendingBufferUpdates().first().offset, -1);
 
+        renderBuffer.pendingBufferUpdates().clear();
         renderBuffer.unsetDirty();
         QVERIFY(!renderBuffer.isDirty());
 
@@ -233,8 +238,11 @@ private Q_SLOTS:
         Qt3DCore::QPropertyUpdatedChangePtr change = arbiter.events.first().staticCast<Qt3DCore::QPropertyUpdatedChange>();
         QCOMPARE(change->propertyName(), "data");
         QCOMPARE(change->value().toByteArray(), QByteArrayLiteral("454"));
+        QCOMPARE(renderBuffer.pendingBufferUpdates().size(), 1);
+        QCOMPARE(renderBuffer.pendingBufferUpdates().first().offset, -1);
 
         arbiter.events.clear();
+        renderBuffer.pendingBufferUpdates().clear();
 
         // WHEN
         updateChange.reset(new Qt3DCore::QPropertyUpdatedChange(Qt3DCore::QNodeId()));
@@ -247,6 +255,7 @@ private Q_SLOTS:
 
         // THEN
         QVERIFY(!renderBuffer.pendingBufferUpdates().empty());
+        QCOMPARE(renderBuffer.pendingBufferUpdates().first().offset, 2);
         QVERIFY(renderBuffer.isDirty());
 
         renderBuffer.unsetDirty();