Address some of the reviews

qgis · Nov 16, 2020 · 8faa5ee · 8faa5ee
1 parent 270e64a
commit 8faa5ee
Show file tree

Hide file tree

Showing 12 changed files with 36 additions and 73 deletions.
diff --git a/python/3d/auto_generated/qgspointcloudlayer3drenderer.sip.in b/python/3d/auto_generated/qgspointcloudlayer3drenderer.sip.in
@@ -43,9 +43,12 @@ Returns point cloud layer associated with the renderer
     virtual QgsPointCloudLayer3DRenderer *clone() const /Factory/;
 
 
-    void setSymbol( QgsPointCloud3DSymbol *symbol );
+    void setSymbol( QgsPointCloud3DSymbol *symbol /Transfer/ );
 %Docstring
-Sets 3D symbol associated with the renderer
+Sets the 3D ``symbol`` associated with the renderer.
+Ownership of ``symbol`` is transferred to the renderer.
+
+.. seealso:: :py:func:`symbol`
 %End
     const QgsPointCloud3DSymbol *symbol() const;
 %Docstring

diff --git a/src/3d/qgs3dutils.cpp b/src/3d/qgs3dutils.cpp
@@ -36,7 +36,6 @@
 #include "qgsline3dsymbol.h"
 #include "qgspoint3dsymbol.h"
 #include "qgspolygon3dsymbol.h"
-//#include "qgspointcloud3dsymbol.h"
 
 #include <Qt3DExtras/QPhongMaterial>
 

diff --git a/src/3d/qgspointcloudlayer3drenderer.cpp b/src/3d/qgspointcloudlayer3drenderer.cpp
@@ -26,6 +26,7 @@
 #include "qgs3dsymbolregistry.h"
 #include "qgspointcloud3dsymbol.h"
 
+#include "qgis.h"
 
 QgsPointCloudLayer3DRendererMetadata::QgsPointCloudLayer3DRendererMetadata()
   : Qgs3DRendererAbstractMetadata( QStringLiteral( "pointcloud" ) )
@@ -79,12 +80,12 @@ Qt3DCore::QEntity *QgsPointCloudLayer3DRenderer::createEntity( const Qgs3DMapSet
   if ( !pcl || !pcl->dataProvider() || !pcl->dataProvider()->index() )
     return nullptr;
 
-  return new QgsPointCloudLayerChunkedEntity( pcl->dataProvider()->index(), map, mSymbol );
+  return new QgsPointCloudLayerChunkedEntity( pcl->dataProvider()->index(), map, mSymbol.get() );
 }
 
 void QgsPointCloudLayer3DRenderer::setSymbol( QgsPointCloud3DSymbol *symbol )
 {
-  mSymbol = dynamic_cast<QgsPointCloud3DSymbol *>( symbol->clone() );
+  mSymbol.reset( symbol );
 }
 
 void QgsPointCloudLayer3DRenderer::writeXml( QDomElement &elem, const QgsReadWriteContext &context ) const
@@ -109,7 +110,8 @@ void QgsPointCloudLayer3DRenderer::readXml( const QDomElement &elem, const QgsRe
   mLayerRef = QgsMapLayerRef( elem.attribute( QStringLiteral( "layer" ) ) );
 
   QDomElement elemSymbol = elem.firstChildElement( QStringLiteral( "symbol" ) );
-  if ( !mSymbol ) mSymbol = new QgsPointCloud3DSymbol;
+  if ( !mSymbol )
+    mSymbol = qgis::make_unique< QgsPointCloud3DSymbol >();
   mSymbol->readXml( elemSymbol, context );
 }
 

diff --git a/src/3d/qgspointcloudlayer3drenderer.h b/src/3d/qgspointcloudlayer3drenderer.h
@@ -70,18 +70,22 @@ class _3D_EXPORT QgsPointCloudLayer3DRenderer : public QgsAbstract3DRenderer
     QgsPointCloudLayer3DRenderer *clone() const override SIP_FACTORY;
     Qt3DCore::QEntity *createEntity( const Qgs3DMapSettings &map ) const override SIP_SKIP;
 
-    //! Sets 3D symbol associated with the renderer
-    void setSymbol( QgsPointCloud3DSymbol *symbol );
+    /**
+     * Sets the 3D \a symbol associated with the renderer.
+      * Ownership of \a symbol is transferred to the renderer.
+      * \see symbol()
+      */
+    void setSymbol( QgsPointCloud3DSymbol *symbol SIP_TRANSFER );
     //! Returns 3D symbol associated with the renderer
-    const QgsPointCloud3DSymbol *symbol() const { return mSymbol; }
+    const QgsPointCloud3DSymbol *symbol() const { return mSymbol.get(); }
 
     void writeXml( QDomElement &elem, const QgsReadWriteContext &context ) const override;
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context ) override;
     void resolveReferences( const QgsProject &project ) override;
 
   private:
     QgsMapLayerRef mLayerRef; //!< Layer used to extract mesh data from
-    QgsPointCloud3DSymbol *mSymbol = nullptr;
+    std::unique_ptr< QgsPointCloud3DSymbol > mSymbol;
 
   private:
 #ifdef SIP_RUN

diff --git a/src/3d/qgspointcloudlayerchunkloader_p.cpp b/src/3d/qgspointcloudlayerchunkloader_p.cpp
@@ -292,7 +292,7 @@ QgsChunkLoader *QgsPointCloudLayerChunkLoaderFactory::createChunkLoader( QgsChun
 {
   QgsChunkNodeId id = node->tileId();
   Q_ASSERT( mPointCloudIndex->hasNode( IndexedPointCloudNode( id.d, id.x, id.y, id.z ) ) );
-  return new QgsPointCloudLayerChunkLoader( this, node, mSymbol );
+  return new QgsPointCloudLayerChunkLoader( this, node, dynamic_cast<QgsPointCloud3DSymbol *>( mSymbol->clone() ) );
 }
 
 QgsAABB nodeBoundsToAABB( QgsPointCloudDataBounds nodeBounds, QgsVector3D offset, QgsVector3D scale, const Qgs3DMapSettings &map );

diff --git a/src/3d/qgspointcloudlayerchunkloader_p.h b/src/3d/qgspointcloudlayerchunkloader_p.h
@@ -139,7 +139,11 @@ class QgsPointCloudLayerChunkLoaderFactory : public QgsChunkLoaderFactory
 class QgsPointCloudLayerChunkLoader : public QgsChunkLoader
 {
   public:
-    //! Constructs the loader
+
+    /**
+     * Constructs the loader
+     * QgsPointCloudLayerChunkLoader takes ownership over symbol
+     */
     QgsPointCloudLayerChunkLoader( const QgsPointCloudLayerChunkLoaderFactory *factory, QgsChunkNode *node, QgsPointCloud3DSymbol *symbol );
     ~QgsPointCloudLayerChunkLoader() override;
 

diff --git a/src/3d/symbols/qgspointcloud3dsymbol.cpp b/src/3d/symbols/qgspointcloud3dsymbol.cpp
@@ -15,7 +15,6 @@
 
 #include "qgspointcloud3dsymbol.h"
 
-// TODO: For some reason whwn I define function on a .cpp file they don't get included in the qgis_app target
 
 QgsPointCloud3DSymbol::QgsPointCloud3DSymbol()
   : QgsAbstract3DSymbol()

diff --git a/src/app/3d/qgspointcloud3dsymbolwidget.cpp b/src/app/3d/qgspointcloud3dsymbolwidget.cpp
@@ -19,52 +19,17 @@
 #include "qgspointcloud3dsymbol.h"
 #include "qgspointcloudlayer3drenderer.h"
 
-QgsPointCloud3DSymbolWidget::QgsPointCloud3DSymbolWidget( QgsPointCloudLayer *layer, QWidget *parent )
+QgsPointCloud3DSymbolWidget::QgsPointCloud3DSymbolWidget( QgsPointCloud3DSymbol *symbol, QWidget *parent )
   : QWidget( parent )
-  , mLayer( layer )
+//  , mLayer( layer )
 {
   this->setupUi( this );
-
-  if ( layer )
-  {
-    QgsPointCloudLayer3DRenderer *renderer = dynamic_cast<QgsPointCloudLayer3DRenderer *>( layer->renderer3D() );
-    QgsPointCloud3DSymbol *symbol;
-    if ( renderer != nullptr )
-    {
-      symbol = const_cast<QgsPointCloud3DSymbol *>( renderer->symbol() );
-      setSymbol( symbol );
-    }
-    else
-    {
-      symbol = new QgsPointCloud3DSymbol;
-      setSymbol( symbol );
-      delete symbol;
-    }
-  }
+  if ( symbol )
+    setSymbol( symbol );
 
   connect( mPointSizeSpinBox, qgis::overload<double>::of( &QDoubleSpinBox::valueChanged ), this, &QgsPointCloud3DSymbolWidget::changed );
 }
 
-void QgsPointCloud3DSymbolWidget::setLayer( QgsPointCloudLayer *layer )
-{
-  mLayer = layer;
-  if ( layer )
-  {
-    QgsPointCloudLayer3DRenderer *renderer = dynamic_cast<QgsPointCloudLayer3DRenderer *>( layer->renderer3D() );
-    QgsPointCloud3DSymbol *symbol;
-    if ( renderer != nullptr )
-    {
-      symbol = const_cast<QgsPointCloud3DSymbol *>( renderer->symbol() );
-      setSymbol( symbol );
-    }
-    else
-    {
-      symbol = new QgsPointCloud3DSymbol;
-      setSymbol( symbol );
-      delete symbol;
-    }
-  }
-}
 
 void QgsPointCloud3DSymbolWidget::setSymbol( QgsPointCloud3DSymbol *symbol )
 {
@@ -73,7 +38,6 @@ void QgsPointCloud3DSymbolWidget::setSymbol( QgsPointCloud3DSymbol *symbol )
 
 QgsPointCloud3DSymbol *QgsPointCloud3DSymbolWidget::symbol() const
 {
-  // TODO: fix memory leak
   QgsPointCloud3DSymbol *symb = new QgsPointCloud3DSymbol;
   symb->setPointSize( mPointSizeSpinBox->value() );
   return symb;

diff --git a/src/app/3d/qgspointcloud3dsymbolwidget.h b/src/app/3d/qgspointcloud3dsymbolwidget.h
@@ -26,20 +26,14 @@ class QgsPointCloud3DSymbolWidget : public QWidget, private Ui::QgsPointCloud3DS
     Q_OBJECT
 
   public:
-    explicit QgsPointCloud3DSymbolWidget( QgsPointCloudLayer *layer, QWidget *parent = nullptr );
+    explicit QgsPointCloud3DSymbolWidget( QgsPointCloud3DSymbol *symbol, QWidget *parent = nullptr );
 
-    void setLayer( QgsPointCloudLayer *pointCloudLayer );
-
-    QgsPointCloudLayer *pointCloudLayer() const { return mLayer; }
     void setSymbol( QgsPointCloud3DSymbol *symbol );
 
     QgsPointCloud3DSymbol *symbol() const;
 
   signals:
     void changed();
-
-  private:
-    QgsPointCloudLayer *mLayer = nullptr;
 };
 
 #endif // QGSPOINTCLOUD3DSYMBOLWIDGET_H
diff --git a/src/app/3d/qgspointcloudlayer3drendererwidget.cpp b/src/app/3d/qgspointcloudlayer3drendererwidget.cpp
@@ -35,7 +35,7 @@ QgsPointCloudLayer3DRendererWidget::QgsPointCloudLayer3DRendererWidget( QgsPoint
   mChkEnabled = new QCheckBox( tr( "Enable 3D Renderer" ), this );
   layout->addWidget( mChkEnabled );
 
-  mWidgetPointCloudSymbol = new QgsPointCloud3DSymbolWidget( layer, this );
+  mWidgetPointCloudSymbol = new QgsPointCloud3DSymbolWidget( nullptr, this );
   layout->addWidget( mWidgetPointCloudSymbol );
 
   connect( mChkEnabled, &QCheckBox::clicked, this, &QgsPointCloudLayer3DRendererWidget::onEnabledClicked );
@@ -44,21 +44,18 @@ QgsPointCloudLayer3DRendererWidget::QgsPointCloudLayer3DRendererWidget( QgsPoint
 
 void QgsPointCloudLayer3DRendererWidget::setRenderer( const QgsPointCloudLayer3DRenderer *renderer )
 {
-  mRenderer.reset( renderer ? renderer->clone() : nullptr );
   if ( renderer != nullptr )
     mWidgetPointCloudSymbol->setSymbol( const_cast<QgsPointCloud3DSymbol *>( renderer->symbol() ) );
   whileBlocking( mChkEnabled )->setChecked( renderer ? renderer->symbol()->isEnabled() : false );
 }
 
 QgsPointCloudLayer3DRenderer *QgsPointCloudLayer3DRendererWidget::renderer()
 {
-  // TODO: use unique_ptr for point cloud symbol
+  QgsPointCloudLayer3DRenderer *renderer = new QgsPointCloudLayer3DRenderer;
   QgsPointCloud3DSymbol *sym = mWidgetPointCloudSymbol->symbol();
-  mRenderer.reset( new QgsPointCloudLayer3DRenderer );
-  mRenderer->setSymbol( sym );
-  delete sym;
-  mRenderer->setLayer( qobject_cast<QgsPointCloudLayer *>( mLayer ) );
-  return mRenderer.get();
+  renderer->setSymbol( sym );
+  renderer->setLayer( qobject_cast<QgsPointCloudLayer *>( mLayer ) );
+  return renderer;
 }
 
 void QgsPointCloudLayer3DRendererWidget::apply()
@@ -81,9 +78,6 @@ void QgsPointCloudLayer3DRendererWidget::onEnabledClicked()
 
 void QgsPointCloudLayer3DRendererWidget::syncToLayer( QgsMapLayer *layer )
 {
-  mLayer = layer ;
-  QgsPointCloudLayer *pointCloudLayer = qobject_cast<QgsPointCloudLayer *>( layer );
-  mWidgetPointCloudSymbol->setLayer( pointCloudLayer );
   QgsAbstract3DRenderer *r = layer->renderer3D();
   if ( r && r->type() == QLatin1String( "pointcloud" ) )
   {

diff --git a/src/app/3d/qgspointcloudlayer3drendererwidget.h b/src/app/3d/qgspointcloudlayer3drendererwidget.h
@@ -21,6 +21,7 @@
 #include "qgsmaplayerconfigwidget.h"
 #include "qgspointcloudlayer3drenderer.h"
 #include "qgsmaplayerconfigwidgetfactory.h"
+#include "qgspointcloud3dsymbol.h"
 
 class QCheckBox;
 
@@ -51,7 +52,6 @@ class QgsPointCloudLayer3DRendererWidget : public QgsMapLayerConfigWidget
   private:
     QCheckBox *mChkEnabled = nullptr;
     QgsPointCloud3DSymbolWidget *mWidgetPointCloudSymbol = nullptr;
-    std::unique_ptr<QgsPointCloudLayer3DRenderer> mRenderer;
 };
 
 class QgsPointCloudLayer3DRendererWidgetFactory : public QObject, public QgsMapLayerConfigWidgetFactory

diff --git a/src/app/qgslayerstylingwidget.cpp b/src/app/qgslayerstylingwidget.cpp
@@ -683,7 +683,7 @@ void QgsLayerStylingWidget::updateCurrentWidgetLayer()
 #ifdef HAVE_3D
             if ( !mPointCloud3DWidget )
             {
-              mPointCloud3DWidget = new QgsPointCloudLayer3DRendererWidget( nullptr, mMapCanvas, mWidgetStack );
+              mPointCloud3DWidget = new QgsPointCloudLayer3DRendererWidget( pcLayer, mMapCanvas, mWidgetStack );
               mPointCloud3DWidget->setDockMode( true );
               connect( mPointCloud3DWidget, &QgsPointCloudLayer3DRendererWidget::widgetChanged, this, &QgsLayerStylingWidget::autoApply );
             }