[labeling] Fix crash when labeling features and showing unplaced

labels and some labels have no candidate positions Fixes #38093
qgis · Sep 2, 2020 · 3afe74b · 3afe74b
1 parent ac79bfa
commit 3afe74b
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 12 deletions.
diff --git a/src/core/pal/problem.cpp b/src/core/pal/problem.cpp
@@ -656,27 +656,32 @@ void Problem::chain_search()
 
 QList<LabelPosition *> Problem::getSolution( bool returnInactive, QList<LabelPosition *> *unlabeled )
 {
-  QList<LabelPosition *> solList;
+  QList<LabelPosition *> finalLabelPlacements;
 
+  // loop through all features to be labeled
   for ( std::size_t i = 0; i < mFeatureCount; i++ )
   {
-    if ( mSol.activeLabelIds[i] != -1 )
+    const int labelId = mSol.activeLabelIds[i];
+    const bool foundNonOverlappingPlacement = labelId != -1;
+    const int startIndexForLabelPlacements = mFeatStartId[i];
+    const bool foundCandidatesForFeature = startIndexForLabelPlacements < static_cast< int >( mLabelPositions.size() );
+
+    if ( foundNonOverlappingPlacement )
     {
-      solList.push_back( mLabelPositions[ mSol.activeLabelIds[i] ].get() ); // active labels
+      finalLabelPlacements.push_back( mLabelPositions[ labelId ].get() ); // active labels
     }
-    else if ( returnInactive
-              || ( mFeatStartId[i] < static_cast< int >( mLabelPositions.size() ) &&
-                   ( mLabelPositions.at( mFeatStartId[i] )->getFeaturePart()->layer()->displayAll()
-                     || mLabelPositions.at( mFeatStartId[i] )->getFeaturePart()->alwaysShow() ) ) )
+    else if ( foundCandidatesForFeature &&
+              ( returnInactive // allowing any overlapping labels regardless of where they are from
+                || mLabelPositions.at( startIndexForLabelPlacements )->getFeaturePart()->layer()->displayAll() // allowing overlapping labels for the layer
+                || mLabelPositions.at( startIndexForLabelPlacements )->getFeaturePart()->alwaysShow() ) ) // allowing overlapping labels for the feature
     {
-      solList.push_back( mLabelPositions[ mFeatStartId[i] ].get() ); // unplaced label
+      finalLabelPlacements.push_back( mLabelPositions[ startIndexForLabelPlacements ].get() ); // unplaced label
     }
     else if ( unlabeled )
     {
-      const int startPos = mFeatStartId[i];
       // need to be careful here -- if the next feature's start id is the same as this one, then this feature had no candidates!
-      if ( startPos < static_cast< int >( mLabelPositions.size() ) && ( i == mFeatureCount - 1 || startPos != mFeatStartId[i + 1] ) )
-        unlabeled->push_back( mLabelPositions[ startPos ].get() );
+      if ( foundCandidatesForFeature && ( i == mFeatureCount - 1 || startIndexForLabelPlacements != mFeatStartId[i + 1] ) )
+        unlabeled->push_back( mLabelPositions[ startIndexForLabelPlacements ].get() );
     }
   }
 
@@ -687,7 +692,7 @@ QList<LabelPosition *> Problem::getSolution( bool returnInactive, QList<LabelPos
       unlabeled->append( position.get() );
   }
 
-  return solList;
+  return finalLabelPlacements;
 }
 
 void Problem::solution_cost()

diff --git a/tests/src/core/testqgslabelingengine.cpp b/tests/src/core/testqgslabelingengine.cpp
@@ -89,6 +89,7 @@ class TestQgsLabelingEngine : public QObject
     void testLineAnchorCurvedConstraints();
     void testLineAnchorHorizontal();
     void testLineAnchorHorizontalConstraints();
+    void testShowAllLabelsWhenALabelHasNoCandidates();
 
   private:
     QgsVectorLayer *vl = nullptr;
@@ -3212,5 +3213,67 @@ void TestQgsLabelingEngine::testLineAnchorHorizontalConstraints()
   QVERIFY( imageCheck( QStringLiteral( "horizontal_strict_anchor_end" ), img, 20 ) );
 }
 
+void TestQgsLabelingEngine::testShowAllLabelsWhenALabelHasNoCandidates()
+{
+  // test that showing all labels when a label has no candidate placements doesn't
+  // result in a crash
+  // refs https://github.com/qgis/QGIS/issues/38093
+
+  QgsPalLayerSettings settings;
+  setDefaultLabelParams( settings );
+
+  QgsTextFormat format = settings.format();
+  format.setSize( 20 );
+  format.setColor( QColor( 0, 0, 0 ) );
+  settings.setFormat( format );
+
+  settings.fieldName = QStringLiteral( "'xxxxxxxxxxxxxx'" );
+  settings.isExpression = true;
+  settings.placement = QgsPalLayerSettings::Line;
+  settings.lineSettings().setPlacementFlags( QgsLabeling::LinePlacementFlag::OnLine );
+  settings.obstacleSettings().setFactor( 10 );
+  settings.lineSettings().setOverrunDistance( 50 );
+
+  std::unique_ptr< QgsVectorLayer> vl2( new QgsVectorLayer( QStringLiteral( "LineString?crs=epsg:23700&field=l:string" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) ) );
+  vl2->setRenderer( new QgsNullSymbolRenderer() );
+
+  QgsFeature f;
+  f.setAttributes( QgsAttributes() << QVariant() );
+
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (-2446233 -5204828, -2342845 -5203825)" ) ) );
+  QVERIFY( vl2->dataProvider()->addFeature( f ) );
+
+  f.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (-2439207 -5198806, -2331302 -5197802)" ) ) );
+  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.setLabelingEngineSettings( createLabelEngineSettings() );
+  mapSettings.setDestinationCrs( vl2->crs() );
+
+  mapSettings.setOutputSize( size );
+  mapSettings.setExtent( QgsRectangle( -3328044.9, -5963176., -1127782.7, -4276844.3 ) );
+  mapSettings.setLayers( QList<QgsMapLayer *>() << vl2.get() );
+  mapSettings.setOutputDpi( 96 );
+
+  QgsLabelingEngineSettings engineSettings = mapSettings.labelingEngineSettings();
+  engineSettings.setFlag( QgsLabelingEngineSettings::DrawLabelRectOnly, true );
+  engineSettings.setFlag( QgsLabelingEngineSettings::UseAllLabels, true );
+  engineSettings.setFlag( QgsLabelingEngineSettings::DrawUnplacedLabels, true );
+  // engineSettings.setFlag( QgsLabelingEngineSettings::DrawCandidates, true );
+  mapSettings.setLabelingEngineSettings( engineSettings );
+
+  QgsMapRendererSequentialJob job( mapSettings );
+  job.start();
+  job.waitForFinished();
+
+  QImage img = job.renderedImage();
+  QVERIFY( imageCheck( QStringLiteral( "show_all_labels_when_no_candidates" ), img, 20 ) );
+}
+
 QGSTEST_MAIN( TestQgsLabelingEngine )
 #include "testqgslabelingengine.moc"
diff --git a/...w_all_labels_when_no_candidates/expected_show_all_labels_when_no_candidates.png b/...w_all_labels_when_no_candidates/expected_show_all_labels_when_no_candidates.png