Address comments from Nyall's code review

qgis · Sep 30, 2017 · 518dd17 · 518dd17
1 parent 041704e
commit 518dd17
Show file tree

Hide file tree

Showing 38 changed files with 96 additions and 141 deletions.
diff --git a/python/core/3d/qgs3drendererregistry.sip b/python/core/3d/qgs3drendererregistry.sip
@@ -1,7 +1,7 @@
 /************************************************************************
  * This file has been generated automatically from                      *
  *                                                                      *
- * src/core/./3d/qgs3drendererregistry.h                                *
+ * src/core/3d/qgs3drendererregistry.h                                  *
  *                                                                      *
  * Do not edit manually ! Edit header and run scripts/sipify.pl again   *
  ************************************************************************/
@@ -90,7 +90,7 @@ class Qgs3DRendererRegistry
 /************************************************************************
  * This file has been generated automatically from                      *
  *                                                                      *
- * src/core/./3d/qgs3drendererregistry.h                                *
+ * src/core/3d/qgs3drendererregistry.h                                  *
  *                                                                      *
  * Do not edit manually ! Edit header and run scripts/sipify.pl again   *
  ************************************************************************/
diff --git a/python/core/3d/qgsabstract3drenderer.sip b/python/core/3d/qgsabstract3drenderer.sip
@@ -1,7 +1,7 @@
 /************************************************************************
  * This file has been generated automatically from                      *
  *                                                                      *
- * src/core/./3d/qgsabstract3drenderer.h                                *
+ * src/core/3d/qgsabstract3drenderer.h                                  *
  *                                                                      *
  * Do not edit manually ! Edit header and run scripts/sipify.pl again   *
  ************************************************************************/
@@ -59,7 +59,7 @@ Resolves references to other objects - second phase of loading - after readXml()
 /************************************************************************
  * This file has been generated automatically from                      *
  *                                                                      *
- * src/core/./3d/qgsabstract3drenderer.h                                *
+ * src/core/3d/qgsabstract3drenderer.h                                  *
  *                                                                      *
  * Do not edit manually ! Edit header and run scripts/sipify.pl again   *
  ************************************************************************/
diff --git a/python/core/qgsmaplayer.sip b/python/core/qgsmaplayer.sip
@@ -763,18 +763,12 @@ Return pointer to layer's undo stack
     void setRenderer3D( QgsAbstract3DRenderer *renderer /Transfer/ );
 %Docstring
  Sets 3D renderer for the layer. Takes ownership of the renderer.
-.. note::
-
-   not available in Python bindings
 .. versionadded:: 3.0
 %End
 
     QgsAbstract3DRenderer *renderer3D() const;
 %Docstring
  Returns 3D renderer associated with the layer. May be null.
-.. note::
-
-   not available in Python bindings
 .. versionadded:: 3.0
  :rtype: QgsAbstract3DRenderer
 %End

diff --git a/src/3d/CMakeLists.txt b/src/3d/CMakeLists.txt
@@ -59,7 +59,6 @@ SET(QGIS_3D_MOC_HDRS
 
   terrain/qgsdemterraintileloader_p.h
   terrain/qgsterrainentity_p.h
-  terrain/qgsterraingenerator.h
   terrain/qgsterraintexturegenerator_p.h
   terrain/qgsterraintextureimage_p.h
   terrain/qgsterraintileentity_p.h

diff --git a/src/3d/chunks/qgschunkloader_p.cpp b/src/3d/chunks/qgschunkloader_p.cpp
@@ -2,12 +2,5 @@
 
 ///@cond PRIVATE
 
-QgsChunkLoader::~QgsChunkLoader()
-{
-}
-
-QgsChunkLoaderFactory::~QgsChunkLoaderFactory()
-{
-}
 
 /// @endcond
diff --git a/src/3d/chunks/qgschunkloader_p.h b/src/3d/chunks/qgschunkloader_p.h
@@ -28,7 +28,7 @@ class QgsChunkLoader : public QgsChunkQueueJob
     {
     }
 
-    virtual ~QgsChunkLoader();
+    virtual ~QgsChunkLoader() = default;
 
     //! Run in main thread to use loaded data.
     //! Returns entity attached to the given parent entity in disabled state
@@ -44,7 +44,7 @@ class QgsChunkLoader : public QgsChunkQueueJob
 class QgsChunkLoaderFactory
 {
   public:
-    virtual ~QgsChunkLoaderFactory();
+    virtual ~QgsChunkLoaderFactory() = default;
 
     //! Creates loader for the given chunk node. Ownership of the returned is passed to the caller.
     virtual QgsChunkLoader *createChunkLoader( QgsChunkNode *node ) const = 0;

diff --git a/src/3d/chunks/qgschunkqueuejob_p.cpp b/src/3d/chunks/qgschunkqueuejob_p.cpp
@@ -2,10 +2,6 @@
 
 ///@cond PRIVATE
 
-QgsChunkQueueJob::~QgsChunkQueueJob()
-{
-}
-
 void QgsChunkQueueJob::cancel()
 {
   // TODO: what to do...

diff --git a/src/3d/chunks/qgschunkqueuejob_p.h b/src/3d/chunks/qgschunkqueuejob_p.h
@@ -43,8 +43,6 @@ class QgsChunkQueueJob : public QObject
     {
     }
 
-    virtual ~QgsChunkQueueJob();
-
     //! Returns chunk node of this job
     QgsChunkNode *chunk() { return mNode; }
 

diff --git a/src/3d/qgs3dutils.cpp b/src/3d/qgs3dutils.cpp
@@ -108,7 +108,7 @@ bool Qgs3DUtils::clampAltitudes( QgsPolygonV2 *polygon, AltitudeClamping altClam
     centroid = polygon->centroid();
 
   QgsCurve *curve = const_cast<QgsCurve *>( polygon->exteriorRing() );
-  QgsLineString *lineString = dynamic_cast<QgsLineString *>( curve );
+  QgsLineString *lineString = qgsgeometry_cast<QgsLineString *>( curve );
   if ( !lineString )
     return false;
 
@@ -117,7 +117,7 @@ bool Qgs3DUtils::clampAltitudes( QgsPolygonV2 *polygon, AltitudeClamping altClam
   for ( int i = 0; i < polygon->numInteriorRings(); ++i )
   {
     QgsCurve *curve = const_cast<QgsCurve *>( polygon->interiorRing( i ) );
-    QgsLineString *lineString = dynamic_cast<QgsLineString *>( curve );
+    QgsLineString *lineString = qgsgeometry_cast<QgsLineString *>( curve );
     if ( !lineString )
       return false;
 
@@ -156,10 +156,9 @@ QList<QVector3D> Qgs3DUtils::positions( const Qgs3DMapSettings &map, QgsVectorLa
     if ( f.geometry().isNull() )
       continue;
 
-    QgsAbstractGeometry *g = f.geometry().geometry();
-    if ( QgsWkbTypes::flatType( g->wkbType() ) == QgsWkbTypes::Point )
+    const QgsAbstractGeometry *g = f.geometry().geometry();
+    if ( const QgsPoint *pt = qgsgeometry_cast< const QgsPoint *>( g ) )
     {
-      QgsPoint *pt = static_cast<QgsPoint *>( g );
       // TODO: use Z coordinates if the point is 3D
       float h = map.terrainGenerator()->heightAt( pt->x(), pt->y(), map ) * map.terrainVerticalScale();
       positions.append( QVector3D( pt->x() - map.originX(), h, -( pt->y() - map.originY() ) ) );

diff --git a/src/3d/qgstilingscheme.h b/src/3d/qgstilingscheme.h
@@ -1,6 +1,8 @@
 #ifndef QGSTILINGSCHEME_H
 #define QGSTILINGSCHEME_H
 
+#include "qgis_3d.h"
+
 #include <qgscoordinatereferencesystem.h>
 #include <qgspointxy.h>
 
@@ -11,7 +13,7 @@ class QgsRectangle;
  * The origin (tile [0,0]) is in bottom-left corner.
  * \since QGIS 3.0
  */
-class QgsTilingScheme
+class _3D_EXPORT QgsTilingScheme
 {
   public:
     //! Creates invalid tiling scheme

diff --git a/src/3d/qgsvectorlayer3drenderer.cpp b/src/3d/qgsvectorlayer3drenderer.cpp
@@ -32,10 +32,6 @@ QgsVectorLayer3DRenderer::QgsVectorLayer3DRenderer( QgsAbstract3DSymbol *s )
 {
 }
 
-QgsVectorLayer3DRenderer::~QgsVectorLayer3DRenderer()
-{
-}
-
 QgsVectorLayer3DRenderer *QgsVectorLayer3DRenderer::clone() const
 {
   QgsVectorLayer3DRenderer *r = new QgsVectorLayer3DRenderer( mSymbol ? mSymbol->clone() : nullptr );

diff --git a/src/3d/qgsvectorlayer3drenderer.h b/src/3d/qgsvectorlayer3drenderer.h
@@ -28,7 +28,7 @@ class _3D_EXPORT QgsVectorLayer3DRendererMetadata : public Qgs3DRendererAbstract
     QgsVectorLayer3DRendererMetadata();
 
     //! Creates an instance of a 3D renderer based on a DOM element with renderer configuration
-    virtual QgsAbstract3DRenderer *createRenderer( QDomElement &elem, const QgsReadWriteContext &context ) override;
+    virtual QgsAbstract3DRenderer *createRenderer( QDomElement &elem, const QgsReadWriteContext &context ) override SIP_FACTORY;
 };
 
 
@@ -42,7 +42,7 @@ class _3D_EXPORT QgsVectorLayer3DRenderer : public QgsAbstract3DRenderer
   public:
     //! Takes ownership of the symbol object
     explicit QgsVectorLayer3DRenderer( QgsAbstract3DSymbol *s SIP_TRANSFER = nullptr );
-    ~QgsVectorLayer3DRenderer();
+    ~QgsVectorLayer3DRenderer() = default;
 
     //! Sets vector layer associated with the renderer
     void setLayer( QgsVectorLayer *layer );
@@ -55,8 +55,8 @@ class _3D_EXPORT QgsVectorLayer3DRenderer : public QgsAbstract3DRenderer
     const QgsAbstract3DSymbol *symbol() const;
 
     QString type() const override { return "vector"; }
-    QgsVectorLayer3DRenderer *clone() const override;
-    Qt3DCore::QEntity *createEntity( const Qgs3DMapSettings &map ) const override;
+    QgsVectorLayer3DRenderer *clone() const override SIP_FACTORY;
+    Qt3DCore::QEntity *createEntity( const Qgs3DMapSettings &map ) const override SIP_FACTORY;
 
     void writeXml( QDomElement &elem, const QgsReadWriteContext &context ) const override;
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context ) override;

diff --git a/src/3d/shaders/light.inc.frag b/src/3d/shaders/light.inc.frag
@@ -1,3 +1,6 @@
+
+// copy of light.inc.frag from qt3d extras
+
 const int MAX_LIGHTS = 8;
 const int TYPE_POINT = 0;
 const int TYPE_DIRECTIONAL = 1;

diff --git a/src/3d/symbols/qgsabstract3dsymbol.h b/src/3d/symbols/qgsabstract3dsymbol.h
@@ -2,6 +2,7 @@
 #define QGSABSTRACT3DSYMBOL_H
 
 #include "qgis_3d.h"
+#include "qgis_sip.h"
 
 class QDomElement;
 class QString;
@@ -20,12 +21,12 @@ class QgsReadWriteContext;
 class _3D_EXPORT QgsAbstract3DSymbol
 {
   public:
-    virtual ~QgsAbstract3DSymbol() {}
+    virtual ~QgsAbstract3DSymbol() = default;
 
     //! Returns identifier of symbol type. Each 3D symbol implementation should return a different type.
     virtual QString type() const = 0;
     //! Returns a new instance of the symbol with the same settings
-    virtual QgsAbstract3DSymbol *clone() const = 0;
+    virtual QgsAbstract3DSymbol *clone() const = 0 SIP_FACTORY;
 
     //! Writes symbol configuration to the given DOM element
     virtual void writeXml( QDomElement &elem, const QgsReadWriteContext &context ) const = 0;

diff --git a/src/3d/symbols/qgsline3dsymbol.cpp b/src/3d/symbols/qgsline3dsymbol.cpp
@@ -21,15 +21,15 @@ void QgsLine3DSymbol::writeXml( QDomElement &elem, const QgsReadWriteContext &co
 
   QDomDocument doc = elem.ownerDocument();
 
-  QDomElement elemDataProperties = doc.createElement( "data" );
-  elemDataProperties.setAttribute( "alt-clamping", Qgs3DUtils::altClampingToString( mAltClamping ) );
-  elemDataProperties.setAttribute( "alt-binding", Qgs3DUtils::altBindingToString( mAltBinding ) );
-  elemDataProperties.setAttribute( "height", mHeight );
-  elemDataProperties.setAttribute( "extrusion-height", mExtrusionHeight );
-  elemDataProperties.setAttribute( "width", mWidth );
+  QDomElement elemDataProperties = doc.createElement( QStringLiteral( "data" ) );
+  elemDataProperties.setAttribute( QStringLiteral( "alt-clamping" ), Qgs3DUtils::altClampingToString( mAltClamping ) );
+  elemDataProperties.setAttribute( QStringLiteral( "alt-binding" ), Qgs3DUtils::altBindingToString( mAltBinding ) );
+  elemDataProperties.setAttribute( QStringLiteral( "height" ), mHeight );
+  elemDataProperties.setAttribute( QStringLiteral( "extrusion-height" ), mExtrusionHeight );
+  elemDataProperties.setAttribute( QStringLiteral( "width" ), mWidth );
   elem.appendChild( elemDataProperties );
 
-  QDomElement elemMaterial = doc.createElement( "material" );
+  QDomElement elemMaterial = doc.createElement( QStringLiteral( "material" ) );
   mMaterial.writeXml( elemMaterial );
   elem.appendChild( elemMaterial );
 }
@@ -38,13 +38,13 @@ void QgsLine3DSymbol::readXml( const QDomElement &elem, const QgsReadWriteContex
 {
   Q_UNUSED( context );
 
-  QDomElement elemDataProperties = elem.firstChildElement( "data" );
-  mAltClamping = Qgs3DUtils::altClampingFromString( elemDataProperties.attribute( "alt-clamping" ) );
-  mAltBinding = Qgs3DUtils::altBindingFromString( elemDataProperties.attribute( "alt-binding" ) );
-  mHeight = elemDataProperties.attribute( "height" ).toFloat();
-  mExtrusionHeight = elemDataProperties.attribute( "extrusion-height" ).toFloat();
-  mWidth = elemDataProperties.attribute( "width" ).toFloat();
+  QDomElement elemDataProperties = elem.firstChildElement( QStringLiteral( "data" ) );
+  mAltClamping = Qgs3DUtils::altClampingFromString( elemDataProperties.attribute( QStringLiteral( "alt-clamping" ) ) );
+  mAltBinding = Qgs3DUtils::altBindingFromString( elemDataProperties.attribute( QStringLiteral( "alt-binding" ) ) );
+  mHeight = elemDataProperties.attribute( QStringLiteral( "height" ) ).toFloat();
+  mExtrusionHeight = elemDataProperties.attribute( QStringLiteral( "extrusion-height" ) ).toFloat();
+  mWidth = elemDataProperties.attribute( QStringLiteral( "width" ) ).toFloat();
 
-  QDomElement elemMaterial = elem.firstChildElement( "material" );
+  QDomElement elemMaterial = elem.firstChildElement( QStringLiteral( "material" ) );
   mMaterial.readXml( elemMaterial );
 }
diff --git a/src/3d/symbols/qgsline3dsymbol.h b/src/3d/symbols/qgsline3dsymbol.h
@@ -18,7 +18,7 @@ class _3D_EXPORT QgsLine3DSymbol : public QgsAbstract3DSymbol
     QgsLine3DSymbol();
 
     QString type() const override { return "line"; }
-    QgsAbstract3DSymbol *clone() const override;
+    QgsAbstract3DSymbol *clone() const override SIP_FACTORY;
 
     void writeXml( QDomElement &elem, const QgsReadWriteContext &context ) const override;
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context ) override;

diff --git a/src/3d/symbols/qgsline3dsymbol_p.cpp b/src/3d/symbols/qgsline3dsymbol_p.cpp
@@ -100,7 +100,7 @@ Qt3DRender::QGeometryRenderer *QgsLine3DSymbolEntityNode::renderer( const Qgs3DM
     if ( QgsWkbTypes::isCurvedType( geom.geometry()->wkbType() ) )
       geom = QgsGeometry( geom.geometry()->segmentize() );
 
-    QgsAbstractGeometry *g = geom.geometry();
+    const QgsAbstractGeometry *g = geom.geometry();
 
     QgsGeos engine( g );
     QgsAbstractGeometry *buffered = engine.buffer( symbol.width() / 2., nSegments, endCapStyle, joinStyle, mitreLimit ); // factory

diff --git a/src/3d/symbols/qgspoint3dsymbol.cpp b/src/3d/symbols/qgspoint3dsymbol.cpp
@@ -17,31 +17,31 @@ void QgsPoint3DSymbol::writeXml( QDomElement &elem, const QgsReadWriteContext &c
 {
   QDomDocument doc = elem.ownerDocument();
 
-  QDomElement elemMaterial = doc.createElement( "material" );
+  QDomElement elemMaterial = doc.createElement( QStringLiteral( "material" ) );
   mMaterial.writeXml( elemMaterial );
   elem.appendChild( elemMaterial );
 
   QVariantMap shapePropertiesCopy( mShapeProperties );
   shapePropertiesCopy["model"] = QVariant( context.pathResolver().writePath( shapePropertiesCopy["model"].toString() ) );
 
-  QDomElement elemShapeProperties = doc.createElement( "shape-properties" );
+  QDomElement elemShapeProperties = doc.createElement( QStringLiteral( "shape-properties" ) );
   elemShapeProperties.appendChild( QgsXmlUtils::writeVariant( shapePropertiesCopy, doc ) );
   elem.appendChild( elemShapeProperties );
 
-  QDomElement elemTransform = doc.createElement( "transform" );
-  elemTransform.setAttribute( "matrix", Qgs3DUtils::matrix4x4toString( mTransform ) );
+  QDomElement elemTransform = doc.createElement( QStringLiteral( "transform" ) );
+  elemTransform.setAttribute( QStringLiteral( "matrix" ), Qgs3DUtils::matrix4x4toString( mTransform ) );
   elem.appendChild( elemTransform );
 }
 
 void QgsPoint3DSymbol::readXml( const QDomElement &elem, const QgsReadWriteContext &context )
 {
-  QDomElement elemMaterial = elem.firstChildElement( "material" );
+  QDomElement elemMaterial = elem.firstChildElement( QStringLiteral( "material" ) );
   mMaterial.readXml( elemMaterial );
 
-  QDomElement elemShapeProperties = elem.firstChildElement( "shape-properties" );
+  QDomElement elemShapeProperties = elem.firstChildElement( QStringLiteral( "shape-properties" ) );
   mShapeProperties = QgsXmlUtils::readVariant( elemShapeProperties.firstChildElement() ).toMap();
   mShapeProperties["model"] = QVariant( context.pathResolver().readPath( mShapeProperties["model"].toString() ) );
 
-  QDomElement elemTransform = elem.firstChildElement( "transform" );
-  mTransform = Qgs3DUtils::stringToMatrix4x4( elemTransform.attribute( "matrix" ) );
+  QDomElement elemTransform = elem.firstChildElement( QStringLiteral( "transform" ) );
+  mTransform = Qgs3DUtils::stringToMatrix4x4( elemTransform.attribute( QStringLiteral( "matrix" ) ) );
 }
diff --git a/src/3d/symbols/qgspoint3dsymbol.h b/src/3d/symbols/qgspoint3dsymbol.h
@@ -19,7 +19,7 @@ class _3D_EXPORT QgsPoint3DSymbol : public QgsAbstract3DSymbol
     QgsPoint3DSymbol();
 
     QString type() const override { return "point"; }
-    QgsAbstract3DSymbol *clone() const override;
+    QgsAbstract3DSymbol *clone() const override SIP_FACTORY;
 
     void writeXml( QDomElement &elem, const QgsReadWriteContext &context ) const override;
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context ) override;

diff --git a/src/3d/symbols/qgspoint3dsymbol_p.cpp b/src/3d/symbols/qgspoint3dsymbol_p.cpp
@@ -138,6 +138,7 @@ void QgsPoint3DSymbolInstancedEntityFactory::addEntityForSelectedPoints( const Q
   QgsFeatureRequest req;
   req.setDestinationCrs( map.crs() );
   req.setFilterFids( layer->selectedFeatureIds() );
+  req.setSubsetOfAttributes( QgsAttributeList() );
 
   // build the entity
   QgsPoint3DSymbolInstancedEntityNode *entity = new QgsPoint3DSymbolInstancedEntityNode( map, layer, symbol, req );
@@ -153,6 +154,7 @@ void QgsPoint3DSymbolInstancedEntityFactory::addEntityForNotSelectedPoints( cons
   // build the feature request to select features
   QgsFeatureRequest req;
   req.setDestinationCrs( map.crs() );
+  req.setSubsetOfAttributes( QgsAttributeList() );
 
   QgsFeatureIds notSelected = layer->allFeatureIds();
   notSelected.subtract( layer->selectedFeatureIds() );
@@ -297,6 +299,7 @@ void QgsPoint3DSymbolModelEntityFactory::addEntitiesForSelectedPoints( const Qgs
 {
   QgsFeatureRequest req;
   req.setDestinationCrs( map.crs() );
+  req.setSubsetOfAttributes( QgsAttributeList() );
   req.setFilterFids( layer->selectedFeatureIds() );
 
   addMeshEntities( map, layer, req, symbol, parent, true );
@@ -309,6 +312,7 @@ void QgsPoint3DSymbolModelEntityFactory::addEntitiesForNotSelectedPoints( const
   // build the feature request to select features
   QgsFeatureRequest req;
   req.setDestinationCrs( map.crs() );
+  req.setSubsetOfAttributes( QgsAttributeList() );
   QgsFeatureIds notSelected = layer->allFeatureIds();
   notSelected.subtract( layer->selectedFeatureIds() );
   req.setFilterFids( notSelected );