Address reviews

qgis · Apr 8, 2021 · e9f8cab · e9f8cab
1 parent a547f17
commit e9f8cab
Show file tree

Hide file tree

Showing 6 changed files with 126 additions and 117 deletions.
diff --git a/python/core/auto_generated/pointcloud/qgspointcloudlayer.sip.in b/python/core/auto_generated/pointcloud/qgspointcloudlayer.sip.in
@@ -48,7 +48,7 @@ Constructor for LayerOptions with optional ``transformContext``.
     };
 
 
-    explicit QgsPointCloudLayer( const QString &path = QString(),
+    explicit QgsPointCloudLayer( const QString &uri = QString(),
                                  const QString &baseName = QString(),
                                  const QString &providerLib = QStringLiteral( "pointcloud" ),
                                  const QgsPointCloudLayer::LayerOptions &options = QgsPointCloudLayer::LayerOptions() );

diff --git a/src/core/pointcloud/qgspointcloudlayer.cpp b/src/core/pointcloud/qgspointcloudlayer.cpp
@@ -34,21 +34,17 @@
 #include <QUrl>
 #include "qgseptprovider.h"
 
-QgsPointCloudLayer::QgsPointCloudLayer( const QString &path,
+QgsPointCloudLayer::QgsPointCloudLayer( const QString &uri,
                                         const QString &baseName,
                                         const QString &providerLib,
                                         const QgsPointCloudLayer::LayerOptions &options )
-  : QgsMapLayer( QgsMapLayerType::PointCloudLayer, baseName, path )
+  : QgsMapLayer( QgsMapLayerType::PointCloudLayer, baseName, uri )
   , mElevationProperties( new QgsPointCloudLayerElevationProperties( this ) )
 {
-  if ( path.startsWith( QStringLiteral( "http" ) ) )
-    mDataSourceType = "remote";
-  else
-    mDataSourceType = "file";
-  if ( !path.isEmpty() && !providerLib.isEmpty() )
+  if ( !uri.isEmpty() && !providerLib.isEmpty() )
   {
     QgsDataProvider::ProviderOptions providerOptions { options.transformContext };
-    setDataSource( path, baseName, providerLib, providerOptions, options.loadDefaultStyle );
+    setDataSource( uri, baseName, providerLib, providerOptions, options.loadDefaultStyle );
 
     if ( !options.skipIndexGeneration && mDataProvider && mDataProvider->isValid() )
       mDataProvider.get()->generateIndex();

diff --git a/src/core/pointcloud/qgspointcloudlayer.h b/src/core/pointcloud/qgspointcloudlayer.h
@@ -88,7 +88,7 @@ class CORE_EXPORT QgsPointCloudLayer : public QgsMapLayer
     /**
      * Constructor - creates a point cloud layer
      */
-    explicit QgsPointCloudLayer( const QString &path = QString(),
+    explicit QgsPointCloudLayer( const QString &uri = QString(),
                                  const QString &baseName = QString(),
                                  const QString &providerLib = QStringLiteral( "pointcloud" ),
                                  const QgsPointCloudLayer::LayerOptions &options = QgsPointCloudLayer::LayerOptions() );
@@ -185,8 +185,6 @@ class CORE_EXPORT QgsPointCloudLayer : public QgsMapLayer
     std::unique_ptr<QgsPointCloudRenderer> mRenderer;
 
     QgsPointCloudLayerElevationProperties *mElevationProperties = nullptr;
-
-    QString mDataSourceType;
 };
 
 

diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp
@@ -181,131 +181,146 @@ bool QgsPointCloudLayerRenderer::render()
 
   if ( pc->accessType() == QgsPointCloudIndex::AccessType::Local )
   {
-    for ( const IndexedPointCloudNode &n : nodes )
+    nodesDrawn += renderNodesSync( nodes, pc, context, request, canceled );
+  }
+  else if ( pc->accessType() == QgsPointCloudIndex::AccessType::Remote )
+  {
+    nodesDrawn += renderNodesAsync( nodes, pc, context, request, canceled );
+  }
+
+#ifdef QGISDEBUG
+  QgsDebugMsgLevel( QStringLiteral( "totals: %1 nodes | %2 points | %3ms" ).arg( nodesDrawn )
+                    .arg( context.pointsRendered() )
+                    .arg( t.elapsed() ), 2 );
+#endif
+
+  mRenderer->stopRender( context );
+
+  mReadyToCompose = true;
+  return !canceled;
+}
+
+int QgsPointCloudLayerRenderer::renderNodesSync( const QVector<IndexedPointCloudNode> &nodes, QgsPointCloudIndex *pc, QgsPointCloudRenderContext &context, QgsPointCloudRequest &request, bool &canceled )
+{
+  int nodesDrawn = 0;
+  for ( const IndexedPointCloudNode &n : nodes )
+  {
+    if ( context.renderContext().renderingStopped() )
     {
-      if ( context.renderContext().renderingStopped() )
-      {
-        QgsDebugMsgLevel( "canceled", 2 );
-        canceled = true;
-        break;
-      }
-      std::unique_ptr<QgsPointCloudBlock> block( pc->nodeData( n, request ) );
+      QgsDebugMsgLevel( "canceled", 2 );
+      canceled = true;
+      break;
+    }
+    std::unique_ptr<QgsPointCloudBlock> block( pc->nodeData( n, request ) );
 
-      if ( !block )
-        continue;
+    if ( !block )
+      continue;
 
-      context.setAttributes( block->attributes() );
+    context.setAttributes( block->attributes() );
 
-      mRenderer->renderBlock( block.get(), context );
-      ++nodesDrawn;
+    mRenderer->renderBlock( block.get(), context );
+    ++nodesDrawn;
 
-      // as soon as first block is rendered, we can start showing layer updates.
-      // but if we are blocking render updates (so that a previously cached image is being shown), we wait
-      // at most e.g. 3 seconds before we start forcing progressive updates.
-      if ( !mBlockRenderUpdates || mElapsedTimer.elapsed() > MAX_TIME_TO_USE_CACHED_PREVIEW_IMAGE )
-      {
-        mReadyToCompose = true;
-      }
+    // as soon as first block is rendered, we can start showing layer updates.
+    // but if we are blocking render updates (so that a previously cached image is being shown), we wait
+    // at most e.g. 3 seconds before we start forcing progressive updates.
+    if ( !mBlockRenderUpdates || mElapsedTimer.elapsed() > MAX_TIME_TO_USE_CACHED_PREVIEW_IMAGE )
+    {
+      mReadyToCompose = true;
     }
   }
-  else
+  return nodesDrawn;
+}
+
+int QgsPointCloudLayerRenderer::renderNodesAsync( const QVector<IndexedPointCloudNode> &nodes, QgsPointCloudIndex *pc, QgsPointCloudRenderContext &context, QgsPointCloudRequest &request, bool &canceled )
+{
+  int nodesDrawn = 0;
+
+  QElapsedTimer downloadTimer;
+  downloadTimer.start();
+
+  // Async loading of nodes
+  QVector<QgsPointCloudBlock *> blocks( nodes.size(), nullptr );
+  QVector<QgsPointCloudBlockHandle *> blockHandles( 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 = [&]()
   {
-    QElapsedTimer downloadTimer;
-    downloadTimer.start();
-
-    // Async loading of nodes
-    QVector<QgsPointCloudBlock *> blocks( nodes.size(), nullptr );
-    QVector<QgsPointCloudBlockHandle *> blockHandles( 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 )
+    // 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 )
     {
-      const IndexedPointCloudNode &n = nodes[i];
-      QgsPointCloudBlockHandle *blockHandle = pc->asyncNodeData( n, request );
-      blockHandles[ i ] = blockHandle;
-      QObject::connect( blockHandle, &QgsPointCloudBlockHandle::blockLoadingSucceeded, [ &, i ]( QgsPointCloudBlock * block )
+      if ( block )
       {
-        if ( block )
-        {
-          blocks[ i ] = block;
-        }
-        else
-        {
-          QgsDebugMsg( QStringLiteral( "Unable to load node %1" ).arg( n.toString() ) );
-        }
-        finishedLoadingBlock[ i ] = true;
-        checkIfFinished();
-      } );
-      QObject::connect( blockHandle, &QgsPointCloudBlockHandle::blockLoadingFailed, [ &, i ]( const QString & errorStr )
+        blocks[ i ] = block;
+      }
+      else
       {
-        QgsDebugMsg( QStringLiteral( "Unable to load node %1, error: %2" ).arg( n.toString(), errorStr ) );
-        finishedLoadingBlock[ i ] = true;
-        checkIfFinished();
-      } );
-    }
-    timer.start( 100000 );
-    // Wait for all point cloud nodes to finish loading
-    loop.exec();
+        QgsDebugMsg( QStringLiteral( "Unable to load node %1" ).arg( n.toString() ) );
+      }
+      finishedLoadingBlock[ i ] = true;
+      checkIfFinished();
+    } );
+    QObject::connect( blockHandle, &QgsPointCloudBlockHandle::blockLoadingFailed, [ &, i ]( const QString & errorStr )
+    {
+      QgsDebugMsg( QStringLiteral( "Unable to load node %1, error: %2" ).arg( n.toString(), errorStr ) );
+      finishedLoadingBlock[ i ] = true;
+      checkIfFinished();
+    } );
+  }
+  timer.start( 100000 );
+  // Wait for all point cloud nodes to finish loading
+  loop.exec();
 
-    QgsDebugMsg( QStringLiteral( "Downloaded in : %1ms" ).arg( downloadTimer.elapsed() ) );
+  QgsDebugMsg( QStringLiteral( "Downloaded in : %1ms" ).arg( downloadTimer.elapsed() ) );
 
-    // Render all the point cloud blocks sequentially
-    for ( int i = 0; i < nodes.size(); ++i )
+  // Render all the point cloud blocks sequentially
+  for ( int i = 0; i < nodes.size(); ++i )
+  {
+    if ( context.renderContext().renderingStopped() )
     {
-      if ( context.renderContext().renderingStopped() )
-      {
-        QgsDebugMsgLevel( "canceled", 2 );
-        canceled = true;
-        break;
-      }
+      QgsDebugMsgLevel( "canceled", 2 );
+      canceled = true;
+      break;
+    }
 
-      if ( !blocks[ i ] )
-        continue;
+    if ( !blocks[ i ] )
+      continue;
 
-      context.setAttributes( blocks[ i ]->attributes() );
+    context.setAttributes( blocks[ i ]->attributes() );
 
-      mRenderer->renderBlock( blocks[ i ], context );
-      ++nodesDrawn;
+    mRenderer->renderBlock( blocks[ i ], context );
+    ++nodesDrawn;
 
-      // as soon as first block is rendered, we can start showing layer updates.
-      // but if we are blocking render updates (so that a previously cached image is being shown), we wait
-      // at most e.g. 3 seconds before we start forcing progressive updates.
-      if ( !mBlockRenderUpdates || mElapsedTimer.elapsed() > MAX_TIME_TO_USE_CACHED_PREVIEW_IMAGE )
-      {
-        mReadyToCompose = true;
-      }
+    // as soon as first block is rendered, we can start showing layer updates.
+    // but if we are blocking render updates (so that a previously cached image is being shown), we wait
+    // at most e.g. 3 seconds before we start forcing progressive updates.
+    if ( !mBlockRenderUpdates || mElapsedTimer.elapsed() > MAX_TIME_TO_USE_CACHED_PREVIEW_IMAGE )
+    {
+      mReadyToCompose = true;
     }
+  }
 
-    for ( int i = 0; i < nodes.size(); ++i )
+  for ( int i = 0; i < nodes.size(); ++i )
+  {
+    if ( blocks[ i ] )
+      delete blocks[ i ];
+    if ( blockHandles[ i ] )
     {
-      if ( blocks[ i ] )
-        delete blocks[ i ];
-      if ( blockHandles[ i ] )
-      {
-        blockHandles[ i ]->disconnect();
-        delete blockHandles[ i ];
-      }
+      blockHandles[ i ]->disconnect();
+      delete blockHandles[ i ];
     }
   }
-
-#ifdef QGISDEBUG
-  QgsDebugMsgLevel( QStringLiteral( "totals: %1 nodes | %2 points | %3ms" ).arg( nodesDrawn )
-                    .arg( context.pointsRendered() )
-                    .arg( t.elapsed() ), 2 );
-#endif
-
-  mRenderer->stopRender( context );
-
-  mReadyToCompose = true;
-  return !canceled;
+  return nodesDrawn;
 }
 
 bool QgsPointCloudLayerRenderer::forceRasterRender() const

diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.h b/src/core/pointcloud/qgspointcloudlayerrenderer.h
@@ -68,6 +68,9 @@ class CORE_EXPORT QgsPointCloudLayerRenderer: public QgsMapLayerRenderer
   private:
     QVector<IndexedPointCloudNode> traverseTree( const QgsPointCloudIndex *pc, const QgsRenderContext &context, IndexedPointCloudNode n, double maxErrorPixels, double nodeErrorPixels );
 
+    int renderNodesSync( const QVector<IndexedPointCloudNode> &nodes, QgsPointCloudIndex *pc, QgsPointCloudRenderContext &context, QgsPointCloudRequest &request, bool &canceled );
+    int renderNodesAsync( const QVector<IndexedPointCloudNode> &nodes, QgsPointCloudIndex *pc, QgsPointCloudRenderContext &context, QgsPointCloudRequest &request, bool &canceled );
+
     QgsPointCloudLayer *mLayer = nullptr;
 
     std::unique_ptr< QgsPointCloudRenderer > mRenderer;

diff --git a/src/core/pointcloud/qgsremoteeptpointcloudindex.cpp b/src/core/pointcloud/qgsremoteeptpointcloudindex.cpp
@@ -46,9 +46,6 @@
 
 ///@cond PRIVATE
 
-#define PROVIDER_KEY QStringLiteral( "ept" )
-#define PROVIDER_DESCRIPTION QStringLiteral( "EPT point cloud provider" )
-
 QgsRemoteEptPointCloudIndex::QgsRemoteEptPointCloudIndex() : QgsPointCloudIndex()
 {
   mTileDownloadManager = QgsApplication::tileDownloadManager();