Fix rendering of negative values in stacked bar diagram

Currently, negative values are treated the same way as positive values when rendering stacked bar diagrams, which can lead to rendering issues, because we can end up with negative lengths and incorrect scaling. This changes the rendering to track the negative values separately and render them in a group below the x axis. The rendering for positive values is unchanged, above the x axis.
qgis · Jul 30, 2020 · 8a76045 · 8a76045
1 parent 6f0b7b0
commit 8a76045
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 16 deletions.
diff --git a/src/core/diagram/qgsstackedbardiagram.cpp b/src/core/diagram/qgsstackedbardiagram.cpp
@@ -54,7 +54,7 @@ QSizeF QgsStackedBarDiagram::diagramSize( const QgsFeature &feature, const QgsRe
   }
 
   bool ok = false;
-  double value = attrVal.toDouble( &ok );
+  double value = fabs( attrVal.toDouble( &ok ) );
   if ( !ok )
   {
     return QSizeF(); //zero size if attribute is missing
@@ -129,12 +129,6 @@ QSizeF QgsStackedBarDiagram::diagramSize( const QgsAttributes &attributes, const
     return QSizeF(); //zero size if no attributes
   }
 
-  double totalSum = 0;
-  for ( int i = 0; i < attributes.count(); ++i )
-  {
-    totalSum += attributes.at( i ).toDouble();
-  }
-
   // eh - this method returns size in unknown units ...! We'll have to fake it and use a rough estimation of
   // a conversion factor to painter units...
   // TODO QGIS 4.0 -- these methods should all use painter units, dependent on the render context scaling...
@@ -166,7 +160,8 @@ void QgsStackedBarDiagram::renderDiagram( const QgsFeature &feature, QgsRenderCo
     return;
   }
 
-  QList<double> values;
+  QList< QPair<double, QColor> > values;
+  QList< QPair<double, QColor> > negativeValues;
 
   QgsExpressionContext expressionContext = c.expressionContext();
   expressionContext.setFeature( feature );
@@ -175,14 +170,27 @@ void QgsStackedBarDiagram::renderDiagram( const QgsFeature &feature, QgsRenderCo
 
   values.reserve( s.categoryAttributes.size() );
   double total = 0;
+  double negativeTotal = 0;
+
+  QList< QColor >::const_iterator colIt = s.categoryColors.constBegin();
   for ( const QString &cat : qgis::as_const( s.categoryAttributes ) )
   {
     QgsExpression *expression = getExpression( cat, expressionContext );
     double currentVal = expression->evaluate( &expressionContext ).toDouble();
-    values.push_back( currentVal );
-    total += currentVal;
+    total += fabs( currentVal );
+    if ( currentVal >= 0 )
+    {
+      values.push_back( qMakePair( currentVal, *colIt ) );
+    }
+    else
+    {
+      negativeTotal += currentVal;
+      negativeValues.push_back( qMakePair( -currentVal, *colIt ) );
+    }
+    colIt++;
   }
 
+
   const double spacing = c.convertToPainterUnits( s.spacing(), s.spacingUnit(), s.spacingMapUnitScale() );
   const double totalSpacing = std::max( 0, s.categoryAttributes.size() - 1 ) * spacing;
 
@@ -203,9 +211,12 @@ void QgsStackedBarDiagram::renderDiagram( const QgsFeature &feature, QgsRenderCo
     scaledMaxVal -= totalSpacing;
 
   double currentOffset = 0;
+  if ( !negativeValues.isEmpty() )
+  {
+    currentOffset = negativeTotal / total * scaledMaxVal - ( negativeValues.size() - 1 ) * spacing;
+  }
   double scaledWidth = sizePainterUnits( s.barWidth, s, c );
 
-
   double baseX = position.x();
   double baseY = position.y();
 
@@ -222,13 +233,17 @@ void QgsStackedBarDiagram::renderDiagram( const QgsFeature &feature, QgsRenderCo
   setPenWidth( mPen, s, c );
   p->setPen( mPen );
 
-  QList<double>::const_iterator valIt = values.constBegin();
-  QList< QColor >::const_iterator colIt = s.categoryColors.constBegin();
-  for ( ; valIt != values.constEnd(); ++valIt, ++colIt )
+  while ( !negativeValues.isEmpty() )
+  {
+    values.push_front( negativeValues.takeLast() );
+  }
+
+  QList< QPair<double, QColor> >::const_iterator valIt = values.constBegin();
+  for ( ; valIt != values.constEnd(); ++valIt )
   {
-    double length = *valIt / total * scaledMaxVal;
+    double length = valIt->first / total * scaledMaxVal;
 
-    mCategoryBrush.setColor( *colIt );
+    mCategoryBrush.setColor( valIt->second );
     p->setBrush( mCategoryBrush );
 
     switch ( s.diagramOrientation )

diff --git a/tests/src/core/testqgsdiagram.cpp b/tests/src/core/testqgsdiagram.cpp
@@ -542,6 +542,62 @@ class TestQgsDiagram : public QObject
       QVERIFY( imageCheck( "stacked_axis_right" ) );
     }
 
+    void testStackedNegative()
+    {
+      QgsDiagramSettings ds;
+      QColor col1 = Qt::red;
+      QColor col2 = Qt::yellow;
+      col1.setAlphaF( 0.5 );
+      col2.setAlphaF( 0.5 );
+      ds.categoryColors = QList<QColor>() << col1 << col2;
+      ds.categoryAttributes = QList<QString>() << QStringLiteral( "\"Pilots\"" ) << QStringLiteral( "-\"Cabin Crew\"" );
+      ds.minimumScale = -1;
+      ds.maximumScale = -1;
+      ds.minimumSize = 0;
+      ds.penColor = Qt::green;
+      ds.penWidth = .5;
+      ds.scaleByArea = true;
+      ds.sizeType = QgsUnitTypes::RenderMillimeters;
+      ds.size = QSizeF( 5, 5 );
+      ds.rotationOffset = 0;
+      ds.setSpacing( 3 );
+      ds.setShowAxis( true );
+
+      QgsStringMap props;
+      props.insert( QStringLiteral( "width" ), QStringLiteral( "2" ) );
+      props.insert( QStringLiteral( "color" ), QStringLiteral( "#ff00ff" ) );
+      ds.setAxisLineSymbol( QgsLineSymbol::createSimple( props ) );
+
+      QgsLinearlyInterpolatedDiagramRenderer *dr = new QgsLinearlyInterpolatedDiagramRenderer();
+      dr->setLowerValue( 0.0 );
+      dr->setLowerSize( QSizeF( 0.0, 0.0 ) );
+      dr->setUpperValue( 10 );
+      dr->setUpperSize( QSizeF( 40, 40 ) );
+      dr->setClassificationField( QStringLiteral( "Staff" ) );
+      dr->setDiagram( new QgsStackedBarDiagram() );
+      dr->setDiagramSettings( ds );
+      mPointsLayer->setDiagramRenderer( dr );
+
+      QgsDiagramLayerSettings dls = QgsDiagramLayerSettings();
+      dls.setPlacement( QgsDiagramLayerSettings::OverPoint );
+      dls.setShowAllDiagrams( true );
+      mPointsLayer->setDiagramLayerSettings( dls );
+
+      QVERIFY( imageCheck( "stacked_negative_up" ) );
+
+      ds.diagramOrientation = QgsDiagramSettings::Down;
+      dr->setDiagramSettings( ds );
+      QVERIFY( imageCheck( "stacked_negative_down" ) );
+
+      ds.diagramOrientation = QgsDiagramSettings::Left;
+      dr->setDiagramSettings( ds );
+      QVERIFY( imageCheck( "stacked_negative_left" ) );
+
+      ds.diagramOrientation = QgsDiagramSettings::Right;
+      dr->setDiagramSettings( ds );
+      QVERIFY( imageCheck( "stacked_negative_right" ) );
+    }
+
     void testPieDiagramExpression()
     {
       QgsDiagramSettings ds;

diff --git a/...ages/diagrams/expected_stacked_negative_down/expected_stacked_negative_down.png b/...ages/diagrams/expected_stacked_negative_down/expected_stacked_negative_down.png
diff --git a/...ages/diagrams/expected_stacked_negative_left/expected_stacked_negative_left.png b/...ages/diagrams/expected_stacked_negative_left/expected_stacked_negative_left.png
diff --git a/...es/diagrams/expected_stacked_negative_right/expected_stacked_negative_right.png b/...es/diagrams/expected_stacked_negative_right/expected_stacked_negative_right.png
diff --git a/...l_images/diagrams/expected_stacked_negative_up/expected_stacked_negative_up.png b/...l_images/diagrams/expected_stacked_negative_up/expected_stacked_negative_up.png