Address Martin reviews

qgis · Apr 8, 2021 · e00881c · e00881c
1 parent 9af8bfd
commit e00881c
Show file tree

Hide file tree

Showing 10 changed files with 85 additions and 122 deletions.
diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
@@ -639,7 +639,7 @@ set(QGIS_CORE_SRCS
   pointcloud/qgspointcloudextentrenderer.cpp
   pointcloud/qgspointcloudrequest.cpp
   pointcloud/qgspointcloudblock.cpp
-  pointcloud/qgspointcloudblockhandle.cpp
+  pointcloud/qgspointcloudblockrequest.cpp
   pointcloud/qgspointcloudlayer.cpp
   pointcloud/qgspointcloudlayerelevationproperties.cpp
   pointcloud/qgspointcloudlayerrenderer.cpp
@@ -1346,7 +1346,7 @@ set(QGIS_CORE_HDRS
   pointcloud/qgspointcloudextentrenderer.h
   pointcloud/qgspointcloudrequest.h
   pointcloud/qgspointcloudblock.h
-  pointcloud/qgspointcloudblockhandle.h
+  pointcloud/qgspointcloudblockrequest.h
   pointcloud/qgspointcloudlayer.h
   pointcloud/qgspointcloudlayerelevationproperties.h
   pointcloud/qgspointcloudlayerrenderer.h

diff --git a/src/core/pointcloud/qgseptpointcloudindex.cpp b/src/core/pointcloud/qgseptpointcloudindex.cpp
@@ -297,7 +297,7 @@ QgsPointCloudBlock *QgsEptPointCloudIndex::nodeData( const IndexedPointCloudNode
   }
 }
 
-QgsPointCloudBlockHandle *QgsEptPointCloudIndex::asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request )
+QgsPointCloudBlockRequest *QgsEptPointCloudIndex::asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request )
 {
   Q_UNUSED( n );
   Q_UNUSED( request );

diff --git a/src/core/pointcloud/qgseptpointcloudindex.h b/src/core/pointcloud/qgseptpointcloudindex.h
@@ -47,7 +47,7 @@ class CORE_EXPORT QgsEptPointCloudIndex: public QgsPointCloudIndex
     void load( const QString &fileName ) override;
 
     QgsPointCloudBlock *nodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) override;
-    QgsPointCloudBlockHandle *asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) override;
+    QgsPointCloudBlockRequest *asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) override;
 
     QgsCoordinateReferenceSystem crs() const override;
     int pointCount() const override;

diff --git a/...e/pointcloud/qgspointcloudblockhandle.cpp → .../pointcloud/qgspointcloudblockrequest.cpp b/...e/pointcloud/qgspointcloudblockhandle.cpp → .../pointcloud/qgspointcloudblockrequest.cpp
@@ -1,5 +1,5 @@
 /***************************************************************************
-                         qgspointcloudblockhandle.cpp
+                         qgspointcloudblockrequest.cpp
                          --------------------
     begin                : March 2021
     copyright            : (C) 2021 by Belgacem Nedjima
@@ -15,49 +15,59 @@
  *                                                                         *
  ***************************************************************************/
 
-#include "qgspointcloudblockhandle.h"
+#include "qgspointcloudblockrequest.h"
 
 #include "qgstiledownloadmanager.h"
 #include "qgseptdecoder.h"
 
 //
-// QgsPointCloudBlockHandle
+// QgsPointCloudBlockRequest
 //
 
 ///@cond PRIVATE
 
-QgsPointCloudBlockHandle::QgsPointCloudBlockHandle( const QString &dataType, const QgsPointCloudAttributeCollection &attributes, const QgsPointCloudAttributeCollection &requestedAttributes, QgsTileDownloadManagerReply *tileDownloadManagerReply )
+QgsPointCloudBlockRequest::QgsPointCloudBlockRequest( const QString &dataType, const QgsPointCloudAttributeCollection &attributes, const QgsPointCloudAttributeCollection &requestedAttributes, QgsTileDownloadManagerReply *tileDownloadManagerReply )
   : mDataType( dataType ), mAttributes( attributes ), mRequestedAttributes( requestedAttributes ), mTileDownloadManagetReply( tileDownloadManagerReply )
 {
-  connect( mTileDownloadManagetReply, &QgsTileDownloadManagerReply::finished, this, &QgsPointCloudBlockHandle::blockFinishedLoading );
+  connect( tileDownloadManagerReply, &QgsTileDownloadManagerReply::finished, this, &QgsPointCloudBlockRequest::blockFinishedLoading );
 }
 
-void QgsPointCloudBlockHandle::blockFinishedLoading()
+QgsPointCloudBlock *QgsPointCloudBlockRequest::block()
 {
+  return mBlock.get();
+}
+
+QString QgsPointCloudBlockRequest::errorStr()
+{
+  return mErrorStr;
+}
+
+void QgsPointCloudBlockRequest::blockFinishedLoading()
+{
+  mBlock.reset( nullptr );
   if ( mTileDownloadManagetReply->error() == QNetworkReply::NetworkError::NoError )
   {
-    QgsPointCloudBlock *block = nullptr;
     if ( mDataType == QLatin1String( "binary" ) )
     {
-      block = QgsEptDecoder::decompressBinary( mTileDownloadManagetReply->data(), mAttributes, mRequestedAttributes );
+      mBlock.reset( QgsEptDecoder::decompressBinary( mTileDownloadManagetReply->data(), mAttributes, mRequestedAttributes ) );
     }
     else if ( mDataType == QLatin1String( "zstandard" ) )
     {
-      block = QgsEptDecoder::decompressZStandard( mTileDownloadManagetReply->data(), mAttributes, mRequestedAttributes );
+      mBlock.reset( QgsEptDecoder::decompressZStandard( mTileDownloadManagetReply->data(), mAttributes, mRequestedAttributes ) );
     }
     else if ( mDataType == QLatin1String( "laszip" ) )
     {
-      block = QgsEptDecoder::decompressLaz( mTileDownloadManagetReply->data(), mAttributes, mRequestedAttributes );
+      mBlock.reset( QgsEptDecoder::decompressLaz( mTileDownloadManagetReply->data(), mAttributes, mRequestedAttributes ) );
     }
-    if ( block == nullptr )
-      emit blockLoadingFailed( QStringLiteral( "unknown data type %1;" ).arg( mDataType ) +  mTileDownloadManagetReply->errorString() );
-    else
-      emit blockLoadingSucceeded( block );
+
+    if ( !mBlock.get() )
+      mErrorStr = QStringLiteral( "unknown data type %1;" ).arg( mDataType ) +  mTileDownloadManagetReply->errorString();
   }
   else
   {
-    emit blockLoadingFailed( mTileDownloadManagetReply->errorString() );
+    mErrorStr = mTileDownloadManagetReply->errorString();
   }
+  emit finished();
 }
 
 ///@endcond
diff --git a/...ore/pointcloud/qgspointcloudblockhandle.h → ...re/pointcloud/qgspointcloudblockrequest.h b/...ore/pointcloud/qgspointcloudblockhandle.h → ...re/pointcloud/qgspointcloudblockrequest.h
@@ -1,5 +1,5 @@
 /***************************************************************************
-                         qgspointcloudblockhandle.h
+                         qgspointcloudblockrequest.h
                          --------------------
     begin                : March 2021
     copyright            : (C) 2021 by Belgacem Nedjima
@@ -15,8 +15,8 @@
  *                                                                         *
  ***************************************************************************/
 
-#ifndef QGSPOINTCLOUDBLOCKHANDLE_H
-#define QGSPOINTCLOUDBLOCKHANDLE_H
+#ifndef QGSPOINTCLOUDBLOCKREQUEST_H
+#define QGSPOINTCLOUDBLOCKREQUEST_H
 
 #include <QObject>
 
@@ -36,24 +36,37 @@ class QgsPointCloudBlock;
  *
  * \since QGIS 3.20
  */
-class CORE_EXPORT QgsPointCloudBlockHandle : public QObject
+class CORE_EXPORT QgsPointCloudBlockRequest : public QObject
 {
     Q_OBJECT
   public:
-    //! Constructor
-    QgsPointCloudBlockHandle( const QString &dataType, const QgsPointCloudAttributeCollection &attributes, const QgsPointCloudAttributeCollection &requestedAttributes, QgsTileDownloadManagerReply *tileDownloadManagerReply );
+    /**
+     * QgsPointCloudBlockRequest constructor
+     * Note: the istanced object will take ownership over \a tileDownloadManagerReply
+     */
+    QgsPointCloudBlockRequest( const QString &dataType, const QgsPointCloudAttributeCollection &attributes, const QgsPointCloudAttributeCollection &requestedAttributes, QgsTileDownloadManagerReply *tileDownloadManagerReply );
+
+    /**
+     * Returns the requested block. if the returned block is nullptr, that means the data request failed
+     * Note: the returned block is owned by QgsPointCloudBlockRequest and will be deallocated once QgsPointCloudBlockRequest instance is deallocated
+     */
+    QgsPointCloudBlock *block();
+
+    //! Returns the error message string of the request
+    QString errorStr();
+
   signals:
-    //! Emitted when the block is loaded successfully
-    void blockLoadingSucceeded( QgsPointCloudBlock *block );
-    //! Emitted when the block loading has failed
-    void blockLoadingFailed( const QString &errorStr );
+    //! Emitted when the request processing has finished
+    void finished();
   private:
     QString mDataType;
     QgsPointCloudAttributeCollection mAttributes;
     QgsPointCloudAttributeCollection mRequestedAttributes;
-    QgsTileDownloadManagerReply *mTileDownloadManagetReply = nullptr;
+    std::unique_ptr<QgsTileDownloadManagerReply> mTileDownloadManagetReply = nullptr;
+    std::unique_ptr<QgsPointCloudBlock> mBlock = nullptr;
+    QString mErrorStr;
   private slots:
     void blockFinishedLoading();
 };
 
-#endif // QGSPOINTCLOUDBLOCKHANDLE_H
+#endif // QGSPOINTCLOUDBLOCKREQUEST_H
diff --git a/src/core/pointcloud/qgspointcloudindex.h b/src/core/pointcloud/qgspointcloudindex.h
@@ -39,7 +39,7 @@
 class QgsPointCloudRequest;
 class QgsPointCloudAttributeCollection;
 class QgsCoordinateReferenceSystem;
-class QgsPointCloudBlockHandle;
+class QgsPointCloudBlockRequest;
 
 /**
  * \ingroup core
@@ -230,7 +230,7 @@ class CORE_EXPORT QgsPointCloudIndex: public QObject
      *
      * May return nullptr in case the node is not present or any other problem with loading
      */
-    virtual QgsPointCloudBlockHandle *asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) = 0;
+    virtual QgsPointCloudBlockRequest *asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) = 0;
 
     //! Returns extent of the data
     QgsRectangle extent() const { return mExtent; }

diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp
@@ -17,7 +17,6 @@
 
 #include <QElapsedTimer>
 #include <QPointer>
-#include <QTimer>
 
 #include "qgspointcloudlayerrenderer.h"
 #include "qgspointcloudlayer.h"
@@ -34,7 +33,7 @@
 #include "qgsmessagelog.h"
 #include "qgscircle.h"
 #include "qgsmapclippingutils.h"
-#include "qgspointcloudblockhandle.h"
+#include "qgspointcloudblockrequest.h"
 
 QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *layer, QgsRenderContext &context )
   : QgsMapLayerRenderer( layer->id(), &context )
@@ -240,45 +239,26 @@ int QgsPointCloudLayerRenderer::renderNodesAsync( const QVector<IndexedPointClou
   downloadTimer.start();
 
   // Async loading of nodes
-  QVector<QgsPointCloudBlock *> blocks( nodes.size(), nullptr );
-  QVector<QgsPointCloudBlockHandle *> blockHandles( nodes.size(), nullptr );
+  QVector<QgsPointCloudBlockRequest *> blockRequests( nodes.size(), nullptr );
   QVector<bool> finishedLoadingBlock( nodes.size(), false );
-  QTimer timer;
   QEventLoop loop;
-  QObject::connect( &timer, &QTimer::timeout, &loop, &QEventLoop::quit );
   // Note: All capture by reference warnings here shouldn't be an issue since we have an event loop, so locals won't be deallocated
-  auto checkIfFinished = [&]()
-  {
-    // If all blocks are loaded, exit the event loop
-    if ( !finishedLoadingBlock.contains( false ) ) loop.exit();
-  };
   for ( int i = 0; i < nodes.size(); ++i )
   {
     const IndexedPointCloudNode &n = nodes[i];
-    QgsPointCloudBlockHandle *blockHandle = pc->asyncNodeData( n, request );
-    blockHandles[ i ] = blockHandle;
-    QObject::connect( blockHandle, &QgsPointCloudBlockHandle::blockLoadingSucceeded, [ &, i ]( QgsPointCloudBlock * block )
+    QgsPointCloudBlockRequest *blockReuqest = pc->asyncNodeData( n, request );
+    blockRequests[ i ] = blockReuqest;
+    QObject::connect( blockReuqest, &QgsPointCloudBlockRequest::finished, [ &, i, blockReuqest ]()
     {
-      if ( block )
-      {
-        blocks[ i ] = block;
-      }
-      else
+      if ( !blockReuqest->block() )
       {
-        QgsDebugMsg( QStringLiteral( "Unable to load node %1" ).arg( n.toString() ) );
+        QgsDebugMsg( QStringLiteral( "Unable to load node %1, error: %2" ).arg( n.toString(), blockReuqest->errorStr() ) );
       }
       finishedLoadingBlock[ i ] = true;
-      checkIfFinished();
-    } );
-    QObject::connect( blockHandle, &QgsPointCloudBlockHandle::blockLoadingFailed, [ &, i ]( const QString & errorStr )
-    {
-      Q_UNUSED( errorStr );
-      QgsDebugMsg( QStringLiteral( "Unable to load node %1, error: %2" ).arg( n.toString(), errorStr ) );
-      finishedLoadingBlock[ i ] = true;
-      checkIfFinished();
+      // If all blocks are loaded, exit the event loop
+      if ( !finishedLoadingBlock.contains( false ) ) loop.exit();
     } );
   }
-  timer.start( 100000 );
   // Wait for all point cloud nodes to finish loading
   loop.exec();
 
@@ -294,12 +274,12 @@ int QgsPointCloudLayerRenderer::renderNodesAsync( const QVector<IndexedPointClou
       break;
     }
 
-    if ( !blocks[ i ] )
+    if ( !blockRequests[ i ]->block() )
       continue;
 
-    context.setAttributes( blocks[ i ]->attributes() );
+    context.setAttributes( blockRequests[ i ]->block()->attributes() );
 
-    mRenderer->renderBlock( blocks[ i ], context );
+    mRenderer->renderBlock( blockRequests[ i ]->block(), context );
     ++nodesDrawn;
 
     // as soon as first block is rendered, we can start showing layer updates.
@@ -313,13 +293,8 @@ int QgsPointCloudLayerRenderer::renderNodesAsync( const QVector<IndexedPointClou
 
   for ( int i = 0; i < nodes.size(); ++i )
   {
-    if ( blocks[ i ] )
-      delete blocks[ i ];
-    if ( blockHandles[ i ] )
-    {
-      blockHandles[ i ]->disconnect();
-      delete blockHandles[ i ];
-    }
+    if ( blockRequests[ i ] )
+      blockRequests[ i ]->deleteLater();
   }
   return nodesDrawn;
 }