Avoid cloning symbols during label rendering

This is expensive and unnecessary
qgis · Jul 23, 2019 · e208bc5 · e208bc5
1 parent baf2006
commit e208bc5
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 16 deletions.
diff --git a/python/core/auto_generated/qgspallabeling.sip.in b/python/core/auto_generated/qgspallabeling.sip.in
@@ -484,7 +484,7 @@ Register a feature for labeling.
                 must have already had the feature and fields sets prior to calling this method.
 :param labelFeature: if using :py:class:`QgsLabelingEngine`, this will receive the label feature. Not available
                      in Python bindings.
-:param symbol: feature symbol to label (ownership is transferred to the label feature)
+:param symbol: feature symbol to label (ownership is not transferred, and ``symbol`` must exist until the labeling is complete)
 %End
 
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context );

diff --git a/src/core/qgslabelfeature.h b/src/core/qgslabelfeature.h
@@ -393,16 +393,16 @@ class CORE_EXPORT QgsLabelFeature
      *
      * \since QGIS 3.10
      */
-    const QgsSymbol *symbol() { return mSymbol.get(); }
+    const QgsSymbol *symbol() { return mSymbol; }
 
     /**
      * Sets the feature \a symbol associated with this label.
-     * Ownership of \a symbol is transferred to the label feature.
+     * Ownership of \a symbol is not transferred to the label feature, .
      * \see symbol()
      *
      * \since QGIS 3.10
      */
-    void setSymbol( const QgsSymbol *symbol SIP_TRANSFER ) { mSymbol.reset( symbol ); }
+    void setSymbol( const QgsSymbol *symbol ) { mSymbol = symbol; }
 
   protected:
     //! Pointer to PAL layer (assigned when registered to PAL)
@@ -471,7 +471,7 @@ class CORE_EXPORT QgsLabelFeature
 
     QgsFeature mFeature;
 
-    std::unique_ptr<const QgsSymbol> mSymbol;
+    const QgsSymbol *mSymbol = nullptr;
 
 };
 

diff --git a/src/core/qgslabelingengine.cpp b/src/core/qgslabelingengine.cpp
@@ -401,6 +401,8 @@ void QgsLabelingEngine::run( QgsRenderContext &context )
       symbolScope = QgsExpressionContextUtils::updateSymbolScope( lf->symbol(), symbolScope );
     }
     lf->provider()->drawLabel( context, label );
+    // finished with symbol -- we can't keep it around after this, it may be deleted
+    lf->setSymbol( nullptr );
   }
 
   // draw unplaced labels. These are always rendered on top
@@ -423,6 +425,8 @@ void QgsLabelingEngine::run( QgsRenderContext &context )
         symbolScope = QgsExpressionContextUtils::updateSymbolScope( lf->symbol(), symbolScope );
       }
       lf->provider()->drawUnplacedLabel( context, label );
+      // finished with symbol -- we can't keep it around after this, it may be deleted
+      lf->setSymbol( nullptr );
     }
   }
 

diff --git a/src/core/qgspallabeling.h b/src/core/qgspallabeling.h
@@ -904,7 +904,7 @@ class CORE_EXPORT QgsPalLayerSettings
      * symbol, the obstacle geometry should represent the bounds of the offset symbol). If not set,
      * the feature's original geometry will be used as an obstacle for labels. Not available
      * in Python bindings.
-     * \param symbol feature symbol to label (ownership is transferred to the label feature)
+     * \param symbol feature symbol to label (ownership is not transferred, and \a symbol must exist until the labeling is complete)
      */
     void registerFeature( const QgsFeature &f, QgsRenderContext &context,
                           QgsLabelFeature **labelFeature SIP_PYARGREMOVE = nullptr,

diff --git a/src/core/qgsrulebasedlabeling.cpp b/src/core/qgsrulebasedlabeling.cpp
@@ -359,7 +359,7 @@ QgsRuleBasedLabeling::Rule::RegisterResult QgsRuleBasedLabeling::Rule::registerF
   // do we have active subprovider for the rule?
   if ( subProviders.contains( this ) && mIsActive )
   {
-    subProviders[this]->registerFeature( feature, context, obstacleGeometry, symbol ? symbol->clone() : nullptr );
+    subProviders[this]->registerFeature( feature, context, obstacleGeometry, symbol );
     registered = true;
   }
 
@@ -383,12 +383,10 @@ QgsRuleBasedLabeling::Rule::RegisterResult QgsRuleBasedLabeling::Rule::registerF
   {
     for ( Rule *rule : qgis::as_const( mElseRules ) )
     {
-      registered |= rule->registerFeature( feature, context, subProviders, obstacleGeometry, symbol ? symbol->clone() : nullptr ) != Filtered;
+      registered |= rule->registerFeature( feature, context, subProviders, obstacleGeometry, symbol ) != Filtered;
     }
   }
 
-  delete symbol;
-
   if ( !mIsActive )
     return Inactive;
   else if ( registered )

diff --git a/src/core/qgsvectorlayerlabelprovider.h b/src/core/qgsvectorlayerlabelprovider.h
@@ -77,9 +77,9 @@ class CORE_EXPORT QgsVectorLayerLabelProvider : public QgsAbstractLabelProvider
      * should be used as an obstacle for labels (e.g., if the feature has been rendered with an offset point
      * symbol, the obstacle geometry should represent the bounds of the offset symbol). If not set,
      * the feature's original geometry will be used as an obstacle for labels.
-     * \param symbol feature symbol to label (ownership is transferred to the label feature)
+     * \param symbol feature symbol to label (ownership is not transferred - the symbol must exist until after labeling is complete)
      */
-    virtual void registerFeature( const QgsFeature &feature, QgsRenderContext &context, const QgsGeometry &obstacleGeometry = QgsGeometry(), const QgsSymbol *symbol SIP_TRANSFER = nullptr );
+    virtual void registerFeature( const QgsFeature &feature, QgsRenderContext &context, const QgsGeometry &obstacleGeometry = QgsGeometry(), const QgsSymbol *symbol = nullptr );
 
     /**
      * Returns the geometry for a point feature which should be used as an obstacle for labels. This

diff --git a/src/core/qgsvectorlayerrenderer.cpp b/src/core/qgsvectorlayerrenderer.cpp
@@ -309,21 +309,21 @@ void QgsVectorLayerRenderer::drawRenderer( QgsFeatureIterator &fit )
         {
           QgsGeometry obstacleGeometry;
           QgsSymbolList symbols = mRenderer->originalSymbolsForFeature( fet, mContext );
-          std::unique_ptr< QgsSymbol > symbol;
+          QgsSymbol *symbol = nullptr;
           if ( !symbols.isEmpty() && fet.geometry().type() == QgsWkbTypes::PointGeometry )
           {
             obstacleGeometry = QgsVectorLayerLabelProvider::getPointObstacleGeometry( fet, mContext, symbols );
           }
 
           if ( !symbols.isEmpty() )
           {
-            symbol.reset( symbols.at( 0 )->clone() );
-            QgsExpressionContextUtils::updateSymbolScope( symbol.get(), symbolScope );
+            symbol = symbols.at( 0 );
+            QgsExpressionContextUtils::updateSymbolScope( symbol, symbolScope );
           }
 
           if ( mLabelProvider )
           {
-            mLabelProvider->registerFeature( fet, mContext, obstacleGeometry, symbol.release() );
+            mLabelProvider->registerFeature( fet, mContext, obstacleGeometry, symbol );
           }
           if ( mDiagramProvider )
           {