Updates based on PeterP's review

qgis · Jan 16, 2020 · ee92e10 · ee92e10
1 parent 1085810
commit ee92e10
Show file tree

Hide file tree

Showing 12 changed files with 65 additions and 43 deletions.
diff --git a/python/3d/auto_generated/qgsabstractvectorlayer3drenderer.sip.in b/python/3d/auto_generated/qgsabstractvectorlayer3drenderer.sip.in
@@ -100,15 +100,15 @@ Returns tiling settings of the renderer
   protected:
     void copyBaseProperties( QgsAbstractVectorLayer3DRenderer *r ) const;
 %Docstring
-copy common properties of this object to another object
+Copies common properties of this object to another object
 %End
     void writeXmlBaseProperties( QDomElement &elem, const QgsReadWriteContext &context ) const;
 %Docstring
-write common properties of this object to DOM element
+Writes common properties of this object to DOM element
 %End
     void readXmlBaseProperties( const QDomElement &elem, const QgsReadWriteContext &context );
 %Docstring
-read common properties of this object from DOM element
+Reads common properties of this object from DOM element
 %End
 
 };

diff --git a/src/3d/qgs3dmapscene.h b/src/3d/qgs3dmapscene.h
@@ -20,6 +20,8 @@
 
 #include <Qt3DCore/QEntity>
 
+#include "qgsfeatureid.h"
+
 namespace Qt3DRender
 {
   class QRenderSettings;
@@ -47,8 +49,6 @@ class Qgs3DMapSettings;
 class QgsTerrainEntity;
 class QgsChunkedEntity;
 
-#include "qgsfeatureid.h"
-
 
 /**
  * \ingroup 3d
@@ -73,7 +73,10 @@ class _3D_EXPORT Qgs3DMapScene : public Qt3DCore::QEntity
     //! Returns number of pending jobs of the terrain entity
     int terrainPendingJobsCount() const;
 
-    //! Returns number of pending jobs for all chunked entities
+    /**
+     * Returns number of pending jobs for all chunked entities
+     * \since QGIS 3.12
+     */
     int totalPendingJobsCount() const;
 
     //! Enumeration of possible states of the 3D scene
@@ -102,7 +105,11 @@ class _3D_EXPORT Qgs3DMapScene : public Qt3DCore::QEntity
     void terrainEntityChanged();
     //! Emitted when the number of terrain's pending jobs changes
     void terrainPendingJobsCountChanged();
-    //! Emitted when the total number of pending jobs changes
+
+    /**
+     * Emitted when the total number of pending jobs changes
+     * \since QGIS 3.12
+     */
     void totalPendingJobsCountChanged();
     //! Emitted when the scene's state has changed
     void sceneStateChanged();

diff --git a/src/3d/qgs3dutils.cpp b/src/3d/qgs3dutils.cpp
@@ -456,6 +456,7 @@ static QgsRectangle _tryReprojectExtent2D( const QgsRectangle &extent, const Qgs
     catch ( const QgsCsException & )
     {
       // bad luck, can't reproject for some reason
+      QgsDebugMsg( QStringLiteral( "3D utils: transformation of extent failed: " ) + extentMapCrs.toString( -1 ) );
     }
   }
   return extentMapCrs;

diff --git a/src/3d/qgs3dutils.h b/src/3d/qgs3dutils.h
@@ -131,14 +131,28 @@ class _3D_EXPORT Qgs3DUtils
     //! Converts 3D world coordinates to map coordinates (applies offset and turns (x,y,z) into (x,-z,y))
     static QgsVector3D worldToMapCoordinates( const QgsVector3D &worldCoords, const QgsVector3D &origin );
 
-    //! Converts extent (in map layer's CRS) to axis aligned bounding box in 3D world coordinates
+    /**
+     * Converts extent (in map layer's CRS) to axis aligned bounding box in 3D world coordinates
+     * \since QGIS 3.12
+     */
     static QgsAABB layerToWorldExtent( const QgsRectangle &extent, double zMin, double zMax, const QgsCoordinateReferenceSystem &layerCrs, const QgsVector3D &mapOrigin, const QgsCoordinateReferenceSystem &mapCrs, const QgsCoordinateTransformContext &context );
-    //! Converts axis aligned bounding box in 3D world coordinates to extent in map layer CRS
+
+    /**
+     * Converts axis aligned bounding box in 3D world coordinates to extent in map layer CRS
+     * \since QGIS 3.12
+     */
     static QgsRectangle worldToLayerExtent( const QgsAABB &bbox, const QgsCoordinateReferenceSystem &layerCrs, const QgsVector3D &mapOrigin, const QgsCoordinateReferenceSystem &mapCrs, const QgsCoordinateTransformContext &context );
 
-    //! Converts map extent to axis aligned bounding box in 3D world coordinates
+    /**
+     * Converts map extent to axis aligned bounding box in 3D world coordinates
+     * \since QGIS 3.12
+     */
     static QgsAABB mapToWorldExtent( const QgsRectangle &extent, double zMin, double zMax, const QgsVector3D &mapOrigin );
-    //! Converts axis aligned bounding box in 3D world coordinates to extent in map coordinates
+
+    /**
+     * Converts axis aligned bounding box in 3D world coordinates to extent in map coordinates
+     * \since QGIS 3.12
+     */
     static QgsRectangle worldToMapExtent( const QgsAABB &bbox, const QgsVector3D &mapOrigin );
 
     //! Transforms a world point from (origin1, crs1) to (origin2, crs2)
@@ -150,6 +164,7 @@ class _3D_EXPORT Qgs3DUtils
      * The implementation scans a small amount of features and looks at the Z values of geometries
      * (we don't need exact range, just a rough estimate is fine to know where to expect the data to be).
      * For layers with geometries without Z values, the returned range will be [0, 0].
+     * \since QGIS 3.12
      */
     static void estimateVectorLayerZRange( QgsVectorLayer *layer, double &zMin, double &zMax );
 

diff --git a/src/3d/qgsabstractvectorlayer3drenderer.cpp b/src/3d/qgsabstractvectorlayer3drenderer.cpp
@@ -43,9 +43,7 @@ void QgsVectorLayer3DTilingSettings::readXml( const QDomElement &elem )
 //////////////////
 
 
-QgsAbstractVectorLayer3DRenderer::QgsAbstractVectorLayer3DRenderer()
-{
-}
+QgsAbstractVectorLayer3DRenderer::QgsAbstractVectorLayer3DRenderer() = default;
 
 void QgsAbstractVectorLayer3DRenderer::setLayer( QgsVectorLayer *layer )
 {

diff --git a/src/3d/qgsabstractvectorlayer3drenderer.h b/src/3d/qgsabstractvectorlayer3drenderer.h
@@ -92,11 +92,11 @@ class _3D_EXPORT QgsAbstractVectorLayer3DRenderer : public QgsAbstract3DRenderer
     void resolveReferences( const QgsProject &project ) override;
 
   protected:
-    //! copy common properties of this object to another object
+    //! Copies common properties of this object to another object
     void copyBaseProperties( QgsAbstractVectorLayer3DRenderer *r ) const;
-    //! write common properties of this object to DOM element
+    //! Writes common properties of this object to DOM element
     void writeXmlBaseProperties( QDomElement &elem, const QgsReadWriteContext &context ) const;
-    //! read common properties of this object from DOM element
+    //! Reads common properties of this object from DOM element
     void readXmlBaseProperties( const QDomElement &elem, const QgsReadWriteContext &context );
 
   private:

diff --git a/src/3d/qgsrulebasedchunkloader_p.cpp b/src/3d/qgsrulebasedchunkloader_p.cpp
@@ -75,7 +75,7 @@ QgsRuleBasedChunkLoader::QgsRuleBasedChunkLoader( const QgsRuleBasedChunkLoaderF
 
   QFuture<void> future = QtConcurrent::run( [req, this]
   {
-    QgsEventTracing::ScopedEvent e( "3D", "RB chunk load" );
+    QgsEventTracing::ScopedEvent e( QStringLiteral( "3D" ), QStringLiteral( "RB chunk load" ) );
 
     QgsFeature f;
     QgsFeatureIterator fi = mSource->getFeatures( req );
@@ -155,9 +155,7 @@ QgsRuleBasedChunkLoaderFactory::QgsRuleBasedChunkLoaderFactory( const Qgs3DMapSe
 {
 }
 
-QgsRuleBasedChunkLoaderFactory::~QgsRuleBasedChunkLoaderFactory()
-{
-}
+QgsRuleBasedChunkLoaderFactory::~QgsRuleBasedChunkLoaderFactory() = default;
 
 QgsChunkLoader *QgsRuleBasedChunkLoaderFactory::createChunkLoader( QgsChunkNode *node ) const
 {

diff --git a/src/3d/qgsrulebasedchunkloader_p.h b/src/3d/qgsrulebasedchunkloader_p.h
@@ -30,15 +30,14 @@
 #include "qgschunkloader_p.h"
 #include "qgsfeature3dhandler_p.h"
 #include "qgschunkedentity_p.h"
+#include "qgsrulebased3drenderer.h"
 
 class Qgs3DMapSettings;
 class QgsVectorLayer;
 class QgsVectorLayerFeatureSource;
 class QgsAbstract3DSymbol;
 class QgsFeature3DHandler;
 
-#include "qgsrulebased3drenderer.h"
-
 
 /**
  * \ingroup 3d
@@ -50,7 +49,7 @@ class QgsFeature3DHandler;
 class QgsRuleBasedChunkLoaderFactory : public QgsChunkLoaderFactory
 {
   public:
-    //! Constructs the factory
+    //! Constructs the factory (vl and rootRule must not be null)
     QgsRuleBasedChunkLoaderFactory( const Qgs3DMapSettings &map, QgsVectorLayer *vl, QgsRuleBased3DRenderer::Rule *rootRule, int leafLevel );
     ~QgsRuleBasedChunkLoaderFactory() override;
 
@@ -75,7 +74,7 @@ class QgsRuleBasedChunkLoaderFactory : public QgsChunkLoaderFactory
 class QgsRuleBasedChunkLoader : public QgsChunkLoader
 {
   public:
-    //! Constructs the loader
+    //! Constructs the loader (factory and node must not be null)
     QgsRuleBasedChunkLoader( const QgsRuleBasedChunkLoaderFactory *factory, QgsChunkNode *node );
     ~QgsRuleBasedChunkLoader() override;
 

diff --git a/src/3d/qgsvectorlayerchunkloader_p.cpp b/src/3d/qgsvectorlayerchunkloader_p.cpp
@@ -20,7 +20,7 @@
 #include "qgschunknode_p.h"
 #include "qgspolygon3dsymbol_p.h"
 #include "qgseventtracing.h"
-
+#include "qgslogger.h"
 #include "qgsvectorlayer.h"
 #include "qgsvectorlayerfeatureiterator.h"
 
@@ -52,22 +52,31 @@ QgsVectorLayerChunkLoader::QgsVectorLayerChunkLoader( const QgsVectorLayerChunkL
   QgsVectorLayer *layer = mFactory->mLayer;
   const Qgs3DMapSettings &map = mFactory->mMap;
 
-  if ( mFactory->mSymbol->type() == QLatin1String( "polygon" ) )
-    mHandler = Qgs3DSymbolImpl::handlerForPolygon3DSymbol( layer, *static_cast<QgsPolygon3DSymbol *>( mFactory->mSymbol.get() ) );
-  else if ( mFactory->mSymbol->type() == QLatin1String( "point" ) )
-    mHandler = Qgs3DSymbolImpl::handlerForPoint3DSymbol( layer, *static_cast<QgsPoint3DSymbol *>( mFactory->mSymbol.get() ) );
-  else if ( mFactory->mSymbol->type() == QLatin1String( "line" ) )
-    mHandler = Qgs3DSymbolImpl::handlerForLine3DSymbol( layer, *static_cast<QgsLine3DSymbol *>( mFactory->mSymbol.get() ) );
+  QgsFeature3DHandler *handler = nullptr;
+  QString symbolType = mFactory->mSymbol->type();
+  if ( symbolType == QLatin1String( "polygon" ) )
+    handler = Qgs3DSymbolImpl::handlerForPolygon3DSymbol( layer, *static_cast<QgsPolygon3DSymbol *>( mFactory->mSymbol.get() ) );
+  else if ( symbolType == QLatin1String( "point" ) )
+    handler = Qgs3DSymbolImpl::handlerForPoint3DSymbol( layer, *static_cast<QgsPoint3DSymbol *>( mFactory->mSymbol.get() ) );
+  else if ( symbolType == QLatin1String( "line" ) )
+    handler = Qgs3DSymbolImpl::handlerForLine3DSymbol( layer, *static_cast<QgsLine3DSymbol *>( mFactory->mSymbol.get() ) );
   else
-    return;  // TODO: how to handle error
+  {
+    QgsDebugMsg( QStringLiteral( "Unknown 3D symbol type for vector layer: " ) + symbolType );
+    return;
+  }
+  mHandler.reset( handler );
 
   QgsExpressionContext exprContext( Qgs3DUtils::globalProjectLayerExpressionContext( layer ) );
   exprContext.setFields( layer->fields() );
   mContext.setExpressionContext( exprContext );
 
   QSet<QString> attributeNames;
   if ( !mHandler->prepare( mContext, attributeNames ) )
-    return;  // TODO: how to handle errors
+  {
+    QgsDebugMsg( QStringLiteral( "Failed to prepare 3D feature handler!" ) );
+    return;
+  }
 
   // build the feature request
   QgsFeatureRequest req;
@@ -84,7 +93,7 @@ QgsVectorLayerChunkLoader::QgsVectorLayerChunkLoader( const QgsVectorLayerChunkL
 
   QFuture<void> future = QtConcurrent::run( [req, this]
   {
-    QgsEventTracing::ScopedEvent e( "3D", "VL chunk load" );
+    QgsEventTracing::ScopedEvent e( QStringLiteral( "3D" ), QStringLiteral( "VL chunk load" ) );
 
     QgsFeature f;
     QgsFeatureIterator fi = mSource->getFeatures( req );
@@ -110,9 +119,6 @@ QgsVectorLayerChunkLoader::~QgsVectorLayerChunkLoader()
     disconnect( mFutureWatcher, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
     mFutureWatcher->waitForFinished();
   }
-
-  delete mHandler;
-  mHandler = nullptr;
 }
 
 void QgsVectorLayerChunkLoader::cancel()

diff --git a/src/3d/qgsvectorlayerchunkloader_p.h b/src/3d/qgsvectorlayerchunkloader_p.h
@@ -84,7 +84,7 @@ class QgsVectorLayerChunkLoader : public QgsChunkLoader
 
   private:
     const QgsVectorLayerChunkLoaderFactory *mFactory;
-    QgsFeature3DHandler *mHandler = nullptr;
+    std::unique_ptr<QgsFeature3DHandler> mHandler;
     Qgs3DRenderContext mContext;
     std::unique_ptr<QgsVectorLayerFeatureSource> mSource;
     bool mCanceled = false;

diff --git a/src/3d/symbols/qgsline3dsymbol_p.cpp b/src/3d/symbols/qgsline3dsymbol_p.cpp
@@ -167,7 +167,7 @@ void QgsBufferedLine3DSymbolHandler::finalize( Qt3DCore::QEntity *parent, const
 
 void QgsBufferedLine3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, const Qgs3DRenderContext &context, LineData &out, bool selected )
 {
-  if ( !out.tessellator->dataVerticesCount() )
+  if ( out.tessellator->dataVerticesCount() == 0 )
     return;  // nothing to show - no need to create the entity
 
   Qt3DExtras::QPhongMaterial *mat = _material( mSymbol );

diff --git a/src/3d/symbols/qgspolygon3dsymbol_p.cpp b/src/3d/symbols/qgspolygon3dsymbol_p.cpp
@@ -37,8 +37,6 @@
 
 /// @cond PRIVATE
 
-class QgsTessellator;
-
 
 class QgsPolygon3DSymbolHandler : public QgsFeature3DHandler
 {
@@ -218,7 +216,7 @@ void QgsPolygon3DSymbolHandler::finalize( Qt3DCore::QEntity *parent, const Qgs3D
 
 void QgsPolygon3DSymbolHandler::makeEntity( Qt3DCore::QEntity *parent, const Qgs3DRenderContext &context, PolygonData &out, bool selected )
 {
-  if ( !out.tessellator->dataVerticesCount() )
+  if ( out.tessellator->dataVerticesCount() == 0 )
     return;  // nothing to show - no need to create the entity
 
   Qt3DExtras::QPhongMaterial *mat = material( mSymbol );