[pal] Improvements

- Improve memory safety and clarify ownership - Try harder to merge connected features, to better handle situations like T intersections or roads which divide into two lanes - Fix labels are sometimes placed in conflict with line features when the conflicting line has the same label yet we couldn't successfully merge the two line parts
qgis · Apr 9, 2021 · 3ee49a6 · 3ee49a6
1 parent 80964bb
commit 3ee49a6
Show file tree

Hide file tree

Showing 9 changed files with 65 additions and 52 deletions.
diff --git a/src/core/pal/labelposition.h b/src/core/pal/labelposition.h
@@ -326,15 +326,15 @@ namespace pal
        *
        * \see setGlobalId()
        */
-      long long globalId() const { return mGlobalId; }
+      unsigned int globalId() const { return mGlobalId; }
 
       /**
        * Sets the global \a id for the candidate, which is unique for a single run of the pal
        * labelling engine.
        *
        * \see globalId()
        */
-      void setGlobalId( long long id ) { mGlobalId = id; }
+      void setGlobalId( unsigned int id ) { mGlobalId = id; }
 
     protected:
 
@@ -364,7 +364,7 @@ namespace pal
 
     private:
 
-      long long mGlobalId = 0;
+      unsigned int mGlobalId = 0;
       std::unique_ptr< LabelPosition > mNextPart;
 
       double mCost;

diff --git a/src/core/pal/layer.cpp b/src/core/pal/layer.cpp
@@ -66,7 +66,6 @@ Layer::~Layer()
 {
   mMutex.lock();
 
-  qDeleteAll( mFeatureParts );
   qDeleteAll( mObstacleParts );
 
   mMutex.unlock();
@@ -104,7 +103,7 @@ bool Layer::registerFeature( QgsLabelFeature *lf )
   bool addedFeature = false;
 
   double geom_size = -1, biggest_size = -1;
-  std::unique_ptr<FeaturePart> biggest_part;
+  std::unique_ptr<FeaturePart> biggestPart;
 
   // break the (possibly multi-part) geometry into simple geometries
   std::unique_ptr<QLinkedList<const GEOSGeometry *>> simpleGeometries( Util::unmulti( lf->geometry() ) );
@@ -183,14 +182,14 @@ bool Layer::registerFeature( QgsLabelFeature *lf )
       if ( geom_size > biggest_size )
       {
         biggest_size = geom_size;
-        biggest_part.reset( fpart.release() );
+        biggestPart = std::move( fpart );
       }
       // don't add the feature part now, do it later
     }
     else
     {
       // feature part is ready!
-      addFeaturePart( fpart.release(), lf->labelText() );
+      addFeaturePart( std::move( fpart ), lf->labelText() );
       addedFeature = true;
     }
   }
@@ -249,9 +248,9 @@ bool Layer::registerFeature( QgsLabelFeature *lf )
   locker.unlock();
 
   // if using only biggest parts...
-  if ( ( !lf->labelAllParts() || lf->hasFixedPosition() ) && biggest_part )
+  if ( ( !lf->labelAllParts() || lf->hasFixedPosition() ) && biggestPart )
   {
-    addFeaturePart( biggest_part.release(), lf->labelText() );
+    addFeaturePart( std::move( biggestPart ), lf->labelText() );
     addedFeature = true;
   }
 
@@ -265,16 +264,16 @@ bool Layer::registerFeature( QgsLabelFeature *lf )
 }
 
 
-void Layer::addFeaturePart( FeaturePart *fpart, const QString &labelText )
+void Layer::addFeaturePart( std::unique_ptr<FeaturePart> fpart, const QString &labelText )
 {
-  // add to list of layer's feature parts
-  mFeatureParts << fpart;
-
   // add to hashtable with equally named feature parts
   if ( mMergeLines && !labelText.isEmpty() )
   {
-    mConnectedHashtable[ labelText ].append( fpart );
+    mConnectedHashtable[ labelText ].append( fpart.get() );
   }
+
+  // add to list of layer's feature parts
+  mFeatureParts.emplace_back( std::move( fpart ) );
 }
 
 void Layer::addObstaclePart( FeaturePart *fpart )
@@ -306,45 +305,57 @@ void Layer::joinConnectedFeatures()
   int connectedFeaturesId = 0;
   for ( auto it = mConnectedHashtable.constBegin(); it != mConnectedHashtable.constEnd(); ++it )
   {
-    QVector<FeaturePart *> parts = it.value();
-    connectedFeaturesId++;
+    QVector<FeaturePart *> partsToMerge = it.value();
 
     // need to start with biggest parts first, to avoid merging in side branches before we've
     // merged the whole of the longest parts of the joined network
-    std::sort( parts.begin(), parts.end(), []( FeaturePart * a, FeaturePart * b )
+    std::sort( partsToMerge.begin(), partsToMerge.end(), []( FeaturePart * a, FeaturePart * b )
     {
       return a->length() > b->length();
     } );
 
     // go one-by-one part, try to merge
-    while ( parts.count() > 1 )
+    while ( partsToMerge.count() > 1 )
     {
+      connectedFeaturesId++;
+
       // part we'll be checking against other in this round
-      FeaturePart *partCheck = parts.takeFirst();
+      FeaturePart *partToJoinTo = partsToMerge.takeFirst();
+      mConnectedFeaturesIds.insert( partToJoinTo->featureId(), connectedFeaturesId );
 
-      FeaturePart *otherPart = _findConnectedPart( partCheck, parts );
-      if ( otherPart )
+      // loop through all other parts
+      QVector< FeaturePart *> partsLeftToTryThisRound = partsToMerge;
+      while ( !partsLeftToTryThisRound.empty() )
       {
-        // merge points from partCheck to p->item
-        if ( otherPart->mergeWithFeaturePart( partCheck ) )
+        if ( FeaturePart *otherPart = _findConnectedPart( partToJoinTo, partsLeftToTryThisRound ) )
         {
-          mConnectedFeaturesIds.insert( partCheck->featureId(), connectedFeaturesId );
-          mConnectedFeaturesIds.insert( otherPart->featureId(), connectedFeaturesId );
+          partsLeftToTryThisRound.removeOne( otherPart );
+          if ( partToJoinTo->mergeWithFeaturePart( otherPart ) )
+          {
+            mConnectedFeaturesIds.insert( otherPart->featureId(), connectedFeaturesId );
 
-          mFeatureParts.removeOne( partCheck );
-          delete partCheck;
+            // otherPart was merged into partToJoinTo, so now we completely delete the redundant feature part which was merged in
+            partsToMerge.removeAll( otherPart );
+            auto matchingPartIt = std::find_if( mFeatureParts.begin(), mFeatureParts.end(), [otherPart]( const std::unique_ptr< FeaturePart> &part ) { return part.get() == otherPart; } );
+            Q_ASSERT( matchingPartIt != mFeatureParts.end() );
+            mFeatureParts.erase( matchingPartIt );
+          }
+        }
+        else
+        {
+          // no candidate parts remain which we could possibly merge in
+          break;
         }
       }
     }
   }
   mConnectedHashtable.clear();
 
   // Expunge feature parts that are smaller than the minimum size required
-  mFeatureParts.erase( std::remove_if( mFeatureParts.begin(), mFeatureParts.end(), []( FeaturePart * part )
+  mFeatureParts.erase( std::remove_if( mFeatureParts.begin(), mFeatureParts.end(), []( const std::unique_ptr< FeaturePart > &part )
   {
     if ( part->feature()->minimumSize() != 0.0 && part->length() < part->feature()->minimumSize() )
     {
-      delete part;
       return true;
     }
     return false;
@@ -359,10 +370,12 @@ int Layer::connectedFeatureId( QgsFeatureId featureId ) const
 void Layer::chopFeaturesAtRepeatDistance()
 {
   GEOSContextHandle_t geosctxt = QgsGeos::getGEOSHandler();
-  QLinkedList<FeaturePart *> newFeatureParts;
-  while ( !mFeatureParts.isEmpty() )
+  std::deque< std::unique_ptr< FeaturePart > > newFeatureParts;
+  while ( !mFeatureParts.empty() )
   {
-    std::unique_ptr< FeaturePart > fpart( mFeatureParts.takeFirst() );
+    std::unique_ptr< FeaturePart > fpart = std::move( mFeatureParts.front() );
+    mFeatureParts.pop_front();
+
     const GEOSGeometry *geom = fpart->geos();
     double chopInterval = fpart->repeatDistance();
 
@@ -460,10 +473,9 @@ void Layer::chopFeaturesAtRepeatDistance()
 #endif
           }
           GEOSGeometry *newgeom = GEOSGeom_createLineString_r( geosctxt, cooSeq );
-          FeaturePart *newfpart = new FeaturePart( fpart->feature(), newgeom );
-          newFeatureParts.append( newfpart );
-          repeatParts.push_back( newfpart );
-
+          std::unique_ptr< FeaturePart > newfpart = std::make_unique< FeaturePart >( fpart->feature(), newgeom );
+          repeatParts.push_back( newfpart.get() );
+          newFeatureParts.emplace_back( std::move( newfpart ) );
           break;
         }
         double c = ( lambda - len[cur - 1] ) / ( len[cur] - len[cur - 1] );
@@ -483,23 +495,23 @@ void Layer::chopFeaturesAtRepeatDistance()
         }
 
         GEOSGeometry *newgeom = GEOSGeom_createLineString_r( geosctxt, cooSeq );
-        FeaturePart *newfpart = new FeaturePart( fpart->feature(), newgeom );
-        newFeatureParts.append( newfpart );
+        std::unique_ptr< FeaturePart > newfpart = std::make_unique< FeaturePart >( fpart->feature(), newgeom );
+        repeatParts.push_back( newfpart.get() );
+        newFeatureParts.emplace_back( std::move( newfpart ) );
         part.clear();
         part.push_back( p );
-        repeatParts.push_back( newfpart );
       }
 
       for ( FeaturePart *partPtr : repeatParts )
         partPtr->setTotalRepeats( repeatParts.count() );
     }
     else
     {
-      newFeatureParts.append( fpart.release() );
+      newFeatureParts.emplace_back( std::move( fpart ) );
     }
   }
 
-  mFeatureParts = newFeatureParts;
+  mFeatureParts = std::move( newFeatureParts );
 }
 
 

diff --git a/src/core/pal/layer.h b/src/core/pal/layer.h
@@ -322,7 +322,7 @@ namespace pal
       QString mName;
 
       //! List of feature parts
-      QLinkedList<FeaturePart *> mFeatureParts;
+      std::deque< std::unique_ptr< FeaturePart > > mFeatureParts;
 
       //! List of obstacle parts
       QList<FeaturePart *> mObstacleParts;
@@ -355,7 +355,7 @@ namespace pal
       QMutex mMutex;
 
       //! Add newly created feature part into r tree and to the list
-      void addFeaturePart( FeaturePart *fpart, const QString &labelText = QString() );
+      void addFeaturePart( std::unique_ptr< FeaturePart > fpart, const QString &labelText = QString() );
 
       //! Add newly created obstacle part into r tree and to the list
       void addObstaclePart( FeaturePart *fpart );

diff --git a/src/core/pal/pal.cpp b/src/core/pal/pal.cpp
@@ -156,7 +156,7 @@ std::unique_ptr<Problem> Pal::extract( const QgsRectangle &extent, const QgsGeom
     QMutexLocker locker( &layer->mMutex );
 
     // generate candidates for all features
-    for ( FeaturePart *featurePart : std::as_const( layer->mFeatureParts ) )
+    for ( const std::unique_ptr< FeaturePart > &featurePart : std::as_const( layer->mFeatureParts ) )
     {
       if ( isCanceled() )
         break;
@@ -204,7 +204,7 @@ std::unique_ptr<Problem> Pal::extract( const QgsRectangle &extent, const QgsGeom
 
         // valid features are added to fFeats
         std::unique_ptr< Feats > ft = std::make_unique< Feats >();
-        ft->feature = featurePart;
+        ft->feature = featurePart.get();
         ft->shape = nullptr;
         ft->candidates = std::move( candidates );
         ft->priority = featurePart->calculatePriority();
@@ -213,7 +213,7 @@ std::unique_ptr<Problem> Pal::extract( const QgsRectangle &extent, const QgsGeom
       else
       {
         // no candidates, so generate a default "point on surface" one
-        std::unique_ptr< LabelPosition > unplacedPosition = featurePart->createCandidatePointOnSurface( featurePart );
+        std::unique_ptr< LabelPosition > unplacedPosition = featurePart->createCandidatePointOnSurface( featurePart.get() );
         if ( !unplacedPosition )
           continue;
 
@@ -226,7 +226,7 @@ std::unique_ptr<Problem> Pal::extract( const QgsRectangle &extent, const QgsGeom
 
           // valid features are added to fFeats
           std::unique_ptr< Feats > ft = std::make_unique< Feats >();
-          ft->feature = featurePart;
+          ft->feature = featurePart.get();
           ft->shape = nullptr;
           ft->candidates = std::move( candidates );
           ft->priority = featurePart->calculatePriority();

diff --git a/src/core/pal/pal.h b/src/core/pal/pal.h
@@ -264,8 +264,8 @@ namespace pal
       int mTenure = 10;
       double mCandListSize = 0.2;
 
-      long long mNextCandidateId = 1;
-      mutable QHash< QPair< long long, long long >, bool > mCandidateConflicts;
+      unsigned int mNextCandidateId = 1;
+      mutable QHash< QPair< unsigned int, unsigned int >, bool > mCandidateConflicts;
 
       /**
        * \brief show partial labels (cut-off by the map canvas) or not

diff --git a/tests/src/core/testqgslabelingengine.cpp b/tests/src/core/testqgslabelingengine.cpp
@@ -1150,11 +1150,12 @@ void TestQgsLabelingEngine::testMergingLinesWithForks()
   settings.isExpression = true;
   settings.placement = QgsPalLayerSettings::Curved;
   settings.labelPerPart = false;
+  settings.dist = 1;
   settings.lineSettings().setMergeLines( true );
 
   // if treated individually, none of these parts are long enough for the label to fit -- but the label should be rendered if the mergeLines setting is true
   std::unique_ptr< QgsVectorLayer> vl2( new QgsVectorLayer( QStringLiteral( "LineString?crs=epsg:3946&field=id:integer" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) ) );
-  vl2->setRenderer( new QgsNullSymbolRenderer() );
+  vl2->setRenderer( new QgsSingleSymbolRenderer( QgsLineSymbol::createSimple( { {QStringLiteral( "color" ), QStringLiteral( "#000000" )}, {QStringLiteral( "outline_width" ), 0.6} } ) ) );
 
   QgsFeature f;
   f.setAttributes( QgsAttributes() << 1 );
@@ -1219,9 +1220,9 @@ void TestQgsLabelingEngine::testMergingLinesWithMinimumSize()
   settings.lineSettings().setMergeLines( true );
   settings.thinningSettings().setMinimumFeatureSize( 90.0 );
 
-  // if treated individually, none of these parts are long enough for the label to fit -- but the label should be rendered if the mergeLines setting is true
+  // if treated individually, none of these parts exceed the minimum feature size set above -- but the label should be rendered if the mergeLines setting is true
   std::unique_ptr< QgsVectorLayer> vl2( new QgsVectorLayer( QStringLiteral( "LineString?crs=epsg:3946&field=id:integer" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) ) );
-  vl2->setRenderer( new QgsNullSymbolRenderer() );
+  vl2->setRenderer( new QgsSingleSymbolRenderer( QgsLineSymbol::createSimple( { {QStringLiteral( "color" ), QStringLiteral( "#000000" )}, {QStringLiteral( "outline_width" ), 0.6} } ) ) );
 
   QgsFeature f;
   f.setAttributes( QgsAttributes() << 1 );

diff --git a/...ngine/expected_label_merged_minimum_size/expected_label_merged_minimum_size.png b/...ngine/expected_label_merged_minimum_size/expected_label_merged_minimum_size.png
diff --git a/...abel_multipart_touching_branches/expected_label_multipart_touching_branches.png b/...abel_multipart_touching_branches/expected_label_multipart_touching_branches.png
diff --git a/...multipart_touching_branches/expected_label_multipart_touching_branches_mask.png b/...multipart_touching_branches/expected_label_multipart_touching_branches_mask.png