address review

qgis · Apr 13, 2023 · f89591b · f89591b
1 parent 2c6f391
commit f89591b
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 58 deletions.
diff --git a/src/core/pointcloud/qgspointclouddataprovider.h b/src/core/pointcloud/qgspointclouddataprovider.h
@@ -58,7 +58,7 @@ class CORE_EXPORT QgsPointCloudDataProvider: public QgsDataProvider
       ReadLayerMetadata = 1 << 0, //!< Provider can read layer metadata from data store.
       WriteLayerMetadata = 1 << 1, //!< Provider can write layer metadata to the data store. See QgsDataProvider::writeLayerMetadata()
       CreateRenderer = 1 << 2, //!< Provider can create 2D renderers using backend-specific formatting information. See QgsPointCloudDataProvider::createRenderer().
-      ContainSubIndexes = 1 << 3, //!< Provider can contain multiple indexes. Virtual point cloud files for example
+      ContainSubIndexes = 1 << 3, //!< Provider can contain multiple indexes. Virtual point cloud files for example (since QGIS 3.32)
     };
 
     Q_DECLARE_FLAGS( Capabilities, Capability )
@@ -178,12 +178,12 @@ class CORE_EXPORT QgsPointCloudDataProvider: public QgsDataProvider
     /**
      * Triggers loading of the point cloud index for the \a n th sub index
      *
-     * If the provider does not support multiple indexes then loadIndex() is called
+     * Only applies to providers that support multiple indexes
      *
      * \note Not available in Python bindings
      * \since QGIS 3.32
      */
-    virtual void loadIndex( int n ) SIP_SKIP { Q_UNUSED( n ) return loadIndex(); }
+    virtual void loadSubIndex( int n ) SIP_SKIP { Q_UNUSED( n ) return; }
 
     /**
      * Returns whether provider has index which is valid

diff --git a/src/core/pointcloud/qgspointcloudlayer.cpp b/src/core/pointcloud/qgspointcloudlayer.cpp
@@ -972,13 +972,13 @@ void QgsPointCloudLayer::loadIndexesForRenderContext( QgsRenderContext &renderer
     for ( int i = 0; i < subIndex.size(); ++i )
     {
       // no need to load as it's there
-      if ( subIndex.at( i ).index )
+      if ( subIndex.at( i ).index() )
         continue;
 
-      if ( subIndex.at( i ).extent.intersects( renderExtent ) &&
-           renderExtent.width() < subIndex.at( i ).extent.width() )
+      if ( subIndex.at( i ).extent().intersects( renderExtent ) &&
+           renderExtent.width() < subIndex.at( i ).extent().width() )
       {
-        mDataProvider->loadIndex( i );
+        mDataProvider->loadSubIndex( i );
       }
     }
   }

diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp
@@ -47,7 +47,8 @@ QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *laye
     return;
 
   mRenderer.reset( mLayer->renderer()->clone() );
-  mSubExtentsRenderer.reset( new QgsPointCloudExtentRenderer() );
+  if ( !mSubIndexes.isEmpty() )
+    mSubIndexExtentRenderer.reset( new QgsPointCloudExtentRenderer() );
 
   if ( mLayer->dataProvider()->index() )
   {
@@ -160,29 +161,29 @@ bool QgsPointCloudLayerRenderer::render()
   }
   else
   {
+    mSubIndexExtentRenderer->startRender( context );
     for ( const auto &si : mSubIndexes )
     {
       if ( canceled )
         break;
 
-      QgsPointCloudIndex *pc = si.index.get();
+      QgsPointCloudIndex *pc = si.index();
 
-      if ( !renderExtent.intersects( si.extent ) )
+      if ( !renderExtent.intersects( si.extent() ) )
         continue;
 
-      if ( !pc || !pc->isValid() || renderExtent.width() > si.extent.width() )
+      if ( !pc || !pc->isValid() || renderExtent.width() > si.extent().width() )
       {
         // when dealing with virtual point clouds, we want to render the individual extents when zoomed out
         // and only use the selected renderer when zoomed in
-        mSubExtentsRenderer->startRender( context );
-        mSubExtentsRenderer->renderExtent( si.geometry, context );
-        mSubExtentsRenderer->stopRender( context );
+        mSubIndexExtentRenderer->renderExtent( si.polygonBounds(), context );
       }
       else
       {
         canceled = !renderIndex( pc );
       }
     }
+    mSubIndexExtentRenderer->stopRender( context );
   }
 
   mRenderer->stopRender( context );

diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.h b/src/core/pointcloud/qgspointcloudlayerrenderer.h
@@ -80,7 +80,7 @@ class CORE_EXPORT QgsPointCloudLayerRenderer: public QgsMapLayerRenderer
     QgsPointCloudLayer *mLayer = nullptr;
 
     std::unique_ptr< QgsPointCloudRenderer > mRenderer;
-    std::unique_ptr< QgsPointCloudExtentRenderer > mSubExtentsRenderer;
+    std::unique_ptr< QgsPointCloudExtentRenderer > mSubIndexExtentRenderer;
 
     QgsVector3D mScale;
     QgsVector3D mOffset;

diff --git a/src/core/pointcloud/qgspointcloudsubindex.h b/src/core/pointcloud/qgspointcloudsubindex.h
@@ -27,14 +27,52 @@
 
 class QgsPointCloudIndex;
 
+/**
+ * \brief Represents an individual index and metadata for the virtual point cloud data provider.
+ *
+ * The index is initially NULLPTR until the virtual point cloud data provider explicitly loads the uri.
+ *
+ * \since QGIS 3.32
+ */
 class QgsPointCloudSubIndex
 {
   public:
-    std::shared_ptr<QgsPointCloudIndex> index;
-    QString uri;
-    QgsRectangle extent;
-    QgsGeometry geometry;
-    qint64 count = 0;
+    //! Constructor
+    QgsPointCloudSubIndex( const QString &uri, const QgsGeometry &geometry, const QgsRectangle &extent, qint64 count )
+      : mUri( uri )
+      , mExtent( extent )
+      , mGeometry( geometry )
+      , mPointCount( count )
+    {
+    }
+
+    //! Returns a pointer to the point cloud index. May be NULLPTR if not loaded.
+    QgsPointCloudIndex *index() const { return mIndex.get(); }
+
+    //! Sets the point cloud index to \a index.
+    void setIndex( QgsPointCloudIndex *index ) { mIndex.reset( index ); }
+
+    //! Returns the uri for this sub index
+    QString uri() const { return mUri; }
+
+    //! Returns the extent for this sub index in the index's crs coordinates.
+    QgsRectangle extent() const { return mExtent; }
+
+    /**
+     * Returns the bounds of the sub index in the index's crs coordinates as a multi polygon geometry.
+     * This can be the same as the extent or a more detailed geometry like a convex hull if available.
+     */
+    QgsGeometry polygonBounds() const { return mGeometry; }
+
+    //! The number of points contained in the index.
+    qint64 pointCount() const { return mPointCount; }
+
+  private:
+    std::shared_ptr<QgsPointCloudIndex> mIndex;
+    QString mUri;
+    QgsRectangle mExtent;
+    QgsGeometry mGeometry;
+    qint64 mPointCount = 0;
 };
 
 ///@endcond

diff --git a/src/core/providers/vpc/qgsvirtualpointcloudprovider.cpp b/src/core/providers/vpc/qgsvirtualpointcloudprovider.cpp
@@ -168,7 +168,7 @@ void QgsVirtualPointCloudProvider::parseFile()
   if ( data["type"] != "FeatureCollection" ||
        !data.contains( "features" ) )
   {
-    appendError( QgsErrorMessage( QStringLiteral( "Invalid VPC file" ), QString( "asd" ) ) );
+    appendError( QgsErrorMessage( QStringLiteral( "Invalid VPC file" ) ) );
     return;
   }
 
@@ -192,13 +192,30 @@ void QgsVirtualPointCloudProvider::parseFile()
       continue;
     }
 
-    QgsPointCloudSubIndex si;
+    QString uri;
+    qint64 count;
+    QgsRectangle extent;
+    QgsGeometry geometry;
 
-    nlohmann::json firstAsset = *f["assets"].begin();
 
-    si.uri = QString::fromStdString( firstAsset["href"] );
+    for ( const auto &asset : f["assets"] )
+    {
+      if ( asset.contains( "href" ) )
+      {
+        uri = QString::fromStdString( asset["href"] );
+        break;
+      }
+    }
+
+    // Only COPC and EPT formats are currently supported. Other files will only have their bounds rendered
+    if ( !uri.endsWith( QStringLiteral( "ept.json" ), Qt::CaseSensitivity::CaseInsensitive ) &&
+         !uri.endsWith( QStringLiteral( "copc.laz" ), Qt::CaseSensitivity::CaseInsensitive ) )
+    {
+      QgsDebugMsg( QStringLiteral( "Unsupported point cloud uri: %1" ).arg( uri ) );
+    }
+
     if ( f["properties"].contains( "pc:count" ) )
-      si.count = f["properties"]["pc:count"];
+      count = f["properties"]["pc:count"];
 
     if ( !mCrs.isValid() )
     {
@@ -212,8 +229,8 @@ void QgsVirtualPointCloudProvider::parseFile()
     if ( f["properties"].contains( "proj:bbox" ) )
     {
       nlohmann::json nativeBbox = f["properties"]["proj:bbox"];
-      si.extent = QgsRectangle( nativeBbox[0].get<double>(), nativeBbox[1].get<double>(),
-                                nativeBbox[3].get<double>(), nativeBbox[4].get<double>() );
+      extent = QgsRectangle( nativeBbox[0].get<double>(), nativeBbox[1].get<double>(),
+                             nativeBbox[3].get<double>(), nativeBbox[4].get<double>() );
     }
     else if ( f.contains( "bbox" ) && mCrs.isValid() )
     {
@@ -223,7 +240,7 @@ void QgsVirtualPointCloudProvider::parseFile()
 
       try
       {
-        si.extent = transform.transformBoundingBox( bbox );
+        extent = transform.transformBoundingBox( bbox );
       }
       catch ( QgsCsException & )
       {
@@ -240,7 +257,7 @@ void QgsVirtualPointCloudProvider::parseFile()
     const auto geom = f["geometry"];
     try
     {
-      QgsMultiPolygonXY geometry;
+      QgsMultiPolygonXY multiPolygon;
       if ( geom.at( "type" ) == "Polygon" )
       {
         QgsPolygonXY polygon;
@@ -253,7 +270,7 @@ void QgsVirtualPointCloudProvider::parseFile()
           }
           polygon.append( ring );
         }
-        geometry.append( polygon );
+        multiPolygon.append( polygon );
       }
       else if ( geom.at( "type" ) == "MultiPolygon" )
       {
@@ -269,25 +286,25 @@ void QgsVirtualPointCloudProvider::parseFile()
             }
             part.append( ring );
           }
-          geometry.append( part );
+          multiPolygon.append( part );
         }
       }
       else
       {
         QgsDebugMsg( QStringLiteral( "Unexpected geometry type: %1" ).arg( QString::fromStdString( geom.at( "type" ) ) ) );
       }
-      si.geometry = QgsGeometry::fromMultiPolygonXY( geometry );
+      geometry = QgsGeometry::fromMultiPolygonXY( multiPolygon );
     }
     catch ( std::exception &e )
     {
       QgsDebugMsg( QStringLiteral( "Malformed geometry item: %1" ).arg( QString::fromStdString( e.what() ) ) );
       return;
     }
 
-    if ( si.uri.startsWith( QLatin1String( "./" ) ) )
+    if ( uri.startsWith( QLatin1String( "./" ) ) )
     {
       // resolve relative path
-      si.uri = fInfo.absoluteDir().absoluteFilePath( si.uri );
+      uri = fInfo.absoluteDir().absoluteFilePath( uri );
     }
 
     if ( f["properties"].contains( "pc:schemas" ) )
@@ -299,18 +316,22 @@ void QgsVirtualPointCloudProvider::parseFile()
     }
 
     if ( transform.isValid() && !transform.isShortCircuited() )
+    {
       try
       {
-        si.geometry.transform( transform );
+        geometry.transform( transform );
       }
       catch ( QgsCsException & )
       {
         QgsDebugMsg( QStringLiteral( "Cannot transform geometry to layer crs." ) );
         continue;
       }
-    mPolygonBounds->addPart( si.geometry );
+    }
+
+    mPolygonBounds->addPart( geometry );
+    mPointCount += count;
+    QgsPointCloudSubIndex si( uri, geometry, extent, count );
     mSubLayers.push_back( si );
-    mPointCount += si.count;
   }
   mExtent = mPolygonBounds->boundingBox();
   populateAttributeCollection( attributeNames );
@@ -321,7 +342,7 @@ QgsGeometry QgsVirtualPointCloudProvider::polygonBounds() const
   return *mPolygonBounds;
 }
 
-void QgsVirtualPointCloudProvider::loadIndex( int i )
+void QgsVirtualPointCloudProvider::loadSubIndex( int i )
 {
   QGIS_PROTECT_QOBJECT_THREAD_ACCESS
 
@@ -330,32 +351,32 @@ void QgsVirtualPointCloudProvider::loadIndex( int i )
 
   QgsPointCloudSubIndex &sl = mSubLayers[ i ];
   // Index already loaded -> no need to load
-  if ( sl.index )
+  if ( sl.index() )
     return;
 
-  if ( sl.uri.startsWith( QStringLiteral( "http" ), Qt::CaseSensitivity::CaseInsensitive ) )
+  if ( sl.uri().startsWith( QStringLiteral( "http" ), Qt::CaseSensitivity::CaseInsensitive ) )
   {
-    if ( sl.uri.endsWith( QStringLiteral( "copc.laz" ), Qt::CaseSensitivity::CaseInsensitive ) )
-      sl.index.reset( new QgsRemoteCopcPointCloudIndex() );
-    else if ( sl.uri.endsWith( QStringLiteral( "ept.json" ), Qt::CaseSensitivity::CaseInsensitive ) )
-      sl.index.reset( new QgsRemoteEptPointCloudIndex() );
+    if ( sl.uri().endsWith( QStringLiteral( "copc.laz" ), Qt::CaseSensitivity::CaseInsensitive ) )
+      sl.setIndex( new QgsRemoteCopcPointCloudIndex() );
+    else if ( sl.uri().endsWith( QStringLiteral( "ept.json" ), Qt::CaseSensitivity::CaseInsensitive ) )
+      sl.setIndex( new QgsRemoteEptPointCloudIndex() );
   }
   else
   {
-    if ( sl.uri.endsWith( QStringLiteral( "copc.laz" ), Qt::CaseSensitivity::CaseInsensitive ) )
-      sl.index.reset( new QgsCopcPointCloudIndex() );
-    else if ( sl.uri.endsWith( QStringLiteral( "ept.json" ), Qt::CaseSensitivity::CaseInsensitive ) )
-      sl.index.reset( new QgsEptPointCloudIndex() );
+    if ( sl.uri().endsWith( QStringLiteral( "copc.laz" ), Qt::CaseSensitivity::CaseInsensitive ) )
+      sl.setIndex( new QgsCopcPointCloudIndex() );
+    else if ( sl.uri().endsWith( QStringLiteral( "ept.json" ), Qt::CaseSensitivity::CaseInsensitive ) )
+      sl.setIndex( new QgsEptPointCloudIndex() );
   }
 
-  if ( !sl.index )
+  if ( !sl.index() )
     return;
 
-  sl.index->load( sl.uri );
+  sl.index()->load( sl.uri() );
 
   // if expression is broken or index is missing a required field, set to "false" so it returns no points
-  if ( !sl.index->setSubsetString( mSubsetString ) )
-    sl.index->setSubsetString( QStringLiteral( "false" ) );
+  if ( !sl.index()->setSubsetString( mSubsetString ) )
+    sl.index()->setSubsetString( QStringLiteral( "false" ) );
 }
 
 void QgsVirtualPointCloudProvider::populateAttributeCollection( QSet<QString> names )
@@ -427,12 +448,12 @@ bool QgsVirtualPointCloudProvider::setSubsetString( const QString &subset, bool
 
   for ( const auto &i : std::as_const( mSubLayers ) )
   {
-    if ( !i.index )
+    if ( !i.index() )
       continue;
 
     // if expression is broken or index is missing a required field, set to "false" so it returns no points
-    if ( !i.index->setSubsetString( subset ) )
-      i.index->setSubsetString( QStringLiteral( "false" ) );
+    if ( !i.index()->setSubsetString( subset ) )
+      i.index()->setSubsetString( QStringLiteral( "false" ) );
   }
 
   mSubsetString = subset;

diff --git a/src/core/providers/vpc/qgsvirtualpointcloudprovider.h b/src/core/providers/vpc/qgsvirtualpointcloudprovider.h
@@ -55,7 +55,7 @@ class QgsVirtualPointCloudProvider: public QgsPointCloudDataProvider
     PointCloudIndexGenerationState indexingState( ) override { return PointCloudIndexGenerationState::Indexed; }
     QgsGeometry polygonBounds() const override;
     QVector<QgsPointCloudSubIndex> subIndexes() override { return mSubLayers; }
-    void loadIndex( int i ) override;
+    void loadSubIndex( int i ) override;
     bool setSubsetString( const QString &subset, bool updateFeatureCount = false ) override;
     QgsPointCloudRenderer *createRenderer( const QVariantMap &configuration = QVariantMap() ) const override SIP_FACTORY;
     bool renderInPreview( const QgsDataProvider::PreviewContext & ) override { return false; }

diff --git a/tests/src/providers/testqgsvirtualpointcloudprovider.cpp b/tests/src/providers/testqgsvirtualpointcloudprovider.cpp
@@ -280,7 +280,7 @@ void TestQgsVirtualPointCloudProvider::testLazyLoading()
   QCOMPARE( subIndexes.size(), 18 );
   int loadedIndexes = 0;
   for ( const auto &si : subIndexes )
-    if ( si.index )
+    if ( si.index() )
       ++loadedIndexes;
 
   QCOMPARE( loadedIndexes, 0 );
@@ -291,7 +291,7 @@ void TestQgsVirtualPointCloudProvider::testLazyLoading()
   layer->loadIndexesForRenderContext( ctx );
   subIndexes = layer->dataProvider()->subIndexes();
   for ( const auto &si : subIndexes )
-    if ( si.index )
+    if ( si.index() )
       ++loadedIndexes;
 
   QCOMPARE( loadedIndexes, 4 );