[labeling] Ensure "merge connected features" setting works correctly

with line networks that contains forks and branches And simplify memory management Refs #12173
qgis · May 30, 2019 · e0aa09c · e0aa09c
1 parent 7213030
commit e0aa09c
Show file tree

Hide file tree

Showing 4 changed files with 85 additions and 43 deletions.
diff --git a/src/core/pal/layer.cpp b/src/core/pal/layer.cpp
@@ -74,9 +74,6 @@ Layer::~Layer()
   qDeleteAll( mFeatureParts );
   qDeleteAll( mObstacleParts );
 
-  //should already be empty
-  qDeleteAll( mConnectedHashtable );
-
   delete mFeatureIndex;
   delete mObstacleIndex;
 
@@ -284,19 +281,7 @@ void Layer::addFeaturePart( FeaturePart *fpart, const QString &labelText )
   // add to hashtable with equally named feature parts
   if ( mMergeLines && !labelText.isEmpty() )
   {
-    QLinkedList< FeaturePart *> *lst;
-    if ( !mConnectedHashtable.contains( labelText ) )
-    {
-      // entry doesn't exist yet
-      lst = new QLinkedList<FeaturePart *>;
-      mConnectedHashtable.insert( labelText, lst );
-      mConnectedTexts << labelText;
-    }
-    else
-    {
-      lst = mConnectedHashtable.value( labelText );
-    }
-    lst->append( fpart ); // add to the list
+    mConnectedHashtable[ labelText ].append( fpart );
   }
 }
 
@@ -313,18 +298,18 @@ void Layer::addObstaclePart( FeaturePart *fpart )
   mObstacleIndex->Insert( bmin, bmax, fpart );
 }
 
-static FeaturePart *_findConnectedPart( FeaturePart *partCheck, QLinkedList<FeaturePart *> *otherParts )
+static FeaturePart *_findConnectedPart( FeaturePart *partCheck, const QVector<FeaturePart *> &otherParts )
 {
   // iterate in the rest of the parts with the same label
-  QLinkedList<FeaturePart *>::const_iterator p = otherParts->constBegin();
-  while ( p != otherParts->constEnd() )
+  auto it = otherParts.constBegin();
+  while ( it != otherParts.constEnd() )
   {
-    if ( partCheck->isConnected( *p ) )
+    if ( partCheck->isConnected( *it ) )
     {
       // stop checking for other connected parts
-      return *p;
+      return *it;
     }
-    ++p;
+    ++it;
   }
 
   return nullptr; // no connected part found...
@@ -334,21 +319,24 @@ void Layer::joinConnectedFeatures()
 {
   // go through all label texts
   int connectedFeaturesId = 0;
-  const auto constMConnectedTexts = mConnectedTexts;
-  for ( const QString &labelText : constMConnectedTexts )
+  for ( auto it = mConnectedHashtable.constBegin(); it != mConnectedHashtable.constEnd(); ++it )
   {
-    if ( !mConnectedHashtable.contains( labelText ) )
-      continue; // shouldn't happen
-
+    const QString labelTExt = it.key();
+    QVector<FeaturePart *> parts = it.value();
     connectedFeaturesId++;
 
-    QLinkedList<FeaturePart *> *parts = mConnectedHashtable.value( labelText );
+    // 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 )
+    {
+      return a->length() > b->length();
+    } );
 
     // go one-by-one part, try to merge
-    while ( !parts->isEmpty() && parts->count() > 1 )
+    while ( parts.count() > 1 )
     {
       // part we'll be checking against other in this round
-      FeaturePart *partCheck = parts->takeFirst();
+      FeaturePart *partCheck = parts.takeFirst();
 
       FeaturePart *otherPart = _findConnectedPart( partCheck, parts );
       if ( otherPart )
@@ -379,19 +367,8 @@ void Layer::joinConnectedFeatures()
         }
       }
     }
-
-    // we're done processing feature parts with this particular label text
-    delete parts;
-    mConnectedHashtable.remove( labelText );
   }
-
-  // we're done processing connected features
-
-  //should be empty, but clear to be safe
-  qDeleteAll( mConnectedHashtable );
   mConnectedHashtable.clear();
-
-  mConnectedTexts.clear();
 }
 
 int Layer::connectedFeatureId( QgsFeatureId featureId ) const

diff --git a/src/core/pal/layer.h b/src/core/pal/layer.h
@@ -304,8 +304,7 @@ namespace pal
       //obstacle r-tree
       RTree<FeaturePart *, double, 2, double, 8, 4> *mObstacleIndex;
 
-      QHash< QString, QLinkedList<FeaturePart *>* > mConnectedHashtable;
-      QStringList mConnectedTexts;
+      QHash< QString, QVector<FeaturePart *> > mConnectedHashtable;
       QHash< QgsFeatureId, int > mConnectedFeaturesIds;
 
       QMutex mMutex;

diff --git a/tests/src/core/testqgslabelingengine.cpp b/tests/src/core/testqgslabelingengine.cpp
@@ -54,6 +54,7 @@ class TestQgsLabelingEngine : public QObject
     void testParallelLabelSmallFeature();
     void testAdjacentParts();
     void testTouchingParts();
+    void testMergingLinesWithForks();
     void testLabelBoundary();
     void testLabelBlockingRegion();
     void testLabelRotationWithReprojection();
@@ -915,6 +916,71 @@ void TestQgsLabelingEngine::testTouchingParts()
   QVERIFY( imageCheck( QStringLiteral( "label_multipart_touching_lines" ), img, 20 ) );
 }
 
+void TestQgsLabelingEngine::testMergingLinesWithForks()
+{
+  // test that the "merge connected features" setting works well with line networks
+  // containing forks and small side branches
+  QgsPalLayerSettings settings;
+  setDefaultLabelParams( settings );
+
+  QgsTextFormat format = settings.format();
+  format.setSize( 20 );
+  format.setColor( QColor( 0, 0, 0 ) );
+  settings.setFormat( format );
+
+  settings.fieldName = QStringLiteral( "'XXXXXXXXXXXXXXXXXXXXXXXXXX'" );
+  settings.isExpression = true;
+  settings.placement = QgsPalLayerSettings::Curved;
+  settings.labelPerPart = false;
+  settings.mergeLines = 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() );
+
+  QgsFeature f;
+  f.setAttributes( QgsAttributes() << 1 );
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (190000 5000010, 190100 5000000)" ) ) );
+  QVERIFY( vl2->dataProvider()->addFeature( f ) );
+  // side branch
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (190100 5000000, 190100 5000010)" ) ) );
+  QVERIFY( vl2->dataProvider()->addFeature( f ) );
+  // side branch
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (190100 5000000, 190100 4999995)" ) ) );
+  QVERIFY( vl2->dataProvider()->addFeature( f ) );
+  // main road continues, note that we deliberately split this up into non-consecutive sections, just for extra checks!
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (190120 5000000, 190200 5000000)" ) ) );
+  QVERIFY( vl2->dataProvider()->addFeature( f ) );
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (190120 5000000, 190100 5000000)" ) ) );
+  QVERIFY( vl2->dataProvider()->addFeature( f ) );
+
+  vl2->setLabeling( new QgsVectorLayerSimpleLabeling( settings ) );  // TODO: this should not be necessary!
+  vl2->setLabelsEnabled( true );
+
+  // make a fake render context
+  QSize size( 640, 480 );
+  QgsMapSettings mapSettings;
+  mapSettings.setDestinationCrs( vl2->crs() );
+
+  mapSettings.setOutputSize( size );
+  mapSettings.setExtent( vl2->extent() );
+  mapSettings.setLayers( QList<QgsMapLayer *>() << vl2.get() );
+  mapSettings.setOutputDpi( 96 );
+
+  QgsLabelingEngineSettings engineSettings = mapSettings.labelingEngineSettings();
+  engineSettings.setFlag( QgsLabelingEngineSettings::UsePartialCandidates, false );
+  engineSettings.setFlag( QgsLabelingEngineSettings::DrawLabelRectOnly, true );
+  //engineSettings.setFlag( QgsLabelingEngineSettings::DrawCandidates, true );
+  mapSettings.setLabelingEngineSettings( engineSettings );
+
+  QgsMapRendererSequentialJob job( mapSettings );
+  job.start();
+  job.waitForFinished();
+
+  QImage img = job.renderedImage();
+  QVERIFY( imageCheck( QStringLiteral( "label_multipart_touching_branches" ), img, 20 ) );
+}
+
 void TestQgsLabelingEngine::testLabelBoundary()
 {
   // test that no labels are drawn outside of the specified label boundary

diff --git a/...abel_multipart_touching_branches/expected_label_multipart_touching_branches.png b/...abel_multipart_touching_branches/expected_label_multipart_touching_branches.png