Fix #10355 (crash) and #10338 (overlapping polygons) in inverted poly…

…gon renderer Conflicts: python/core/symbology-ng/qgsinvertedpolygonrenderer.sip src/core/symbology-ng/qgsinvertedpolygonrenderer.cpp src/core/symbology-ng/qgsinvertedpolygonrenderer.h src/gui/symbology-ng/qgsinvertedpolygonrendererwidget.cpp src/gui/symbology-ng/qgsinvertedpolygonrendererwidget.h
qgis · Jun 10, 2014 · 3fe12df · 3fe12df
2 parents ae9048a + 25346fe
commit 3fe12df
Show file tree

Hide file tree

Showing 18 changed files with 396 additions and 76 deletions.
diff --git a/python/core/symbology-ng/qgsinvertedpolygonrenderer.sip b/python/core/symbology-ng/qgsinvertedpolygonrenderer.sip
@@ -71,4 +71,14 @@ class QgsInvertedPolygonRenderer : QgsFeatureRendererV2
     /** @returns the current embedded renderer
      */
     const QgsFeatureRendererV2* embeddedRenderer() const;
+
+    /** @returns true if the geometries are to be preprocessed (merged with an union) before rendering.*/
+    bool preprocessingEnabled() const;
+    /**
+        @param enabled enables or disables the preprocessing.
+        When enabled, geometries will be merged with an union before being rendered.
+        It allows to fix some rendering artefacts (when rendering overlapping polygons for instance).
+        This will involve some CPU-demanding computations and is thus disabled by default.
+    */
+    void setPreprocessingEnabled( bool enabled );
 };
diff --git a/src/core/symbology-ng/qgsinvertedpolygonrenderer.cpp b/src/core/symbology-ng/qgsinvertedpolygonrenderer.cpp
@@ -29,6 +29,7 @@
 
 QgsInvertedPolygonRenderer::QgsInvertedPolygonRenderer( const QgsFeatureRendererV2* subRenderer )
     : QgsFeatureRendererV2( "invertedPolygonRenderer" )
+    , mPreprocessingEnabled( false )
 {
   if ( subRenderer )
   {
@@ -68,7 +69,6 @@ void QgsInvertedPolygonRenderer::startRender( QgsRenderContext& context, const Q
     return;
   }
 
-  mSubRenderer->startRender( context, fields );
   mFeaturesCategoryMap.clear();
   mFeatureDecorations.clear();
   mFields = fields;
@@ -78,36 +78,52 @@ void QgsInvertedPolygonRenderer::startRender( QgsRenderContext& context, const Q
   // It must be computed in the destination CRS if reprojection is enabled.
   const QgsMapToPixel& mtp( context.mapToPixel() );
 
+  if ( !context.painter() )
+  {
+    return;
+  }
+
   // convert viewport to dest CRS
   QRect e( context.painter()->viewport() );
   // add some space to hide borders and tend to infinity
-  e.adjust( -e.width()*10, -e.height()*10, e.width()*10, e.height()*10 );
+  e.adjust( -e.width()*5, -e.height()*5, e.width()*5, e.height()*5 );
   QgsPolyline exteriorRing;
   exteriorRing << mtp.toMapCoordinates( e.topLeft() );
   exteriorRing << mtp.toMapCoordinates( e.topRight() );
   exteriorRing << mtp.toMapCoordinates( e.bottomRight() );
   exteriorRing << mtp.toMapCoordinates( e.bottomLeft() );
   exteriorRing << mtp.toMapCoordinates( e.topLeft() );
 
-  mTransform = context.coordinateTransform();
+  // copy the rendering context
+  mContext = context;
+
   // If reprojection is enabled, we must reproject during renderFeature
   // and act as if there is no reprojection
   // If we don't do that, there is no need to have a simple rectangular extent
   // that covers the whole screen
   // (a rectangle in the destCRS cannot be expressed as valid coordinates in the sourceCRS in general)
-  if ( mTransform )
+  if ( context.coordinateTransform() )
   {
     // disable projection
-    context.setCoordinateTransform( 0 );
+    mContext.setCoordinateTransform( 0 );
+    // recompute extent so that polygon clipping is correct
+    QRect v( context.painter()->viewport() );
+    mContext.setExtent( QgsRectangle( mtp.toMapCoordinates( v.topLeft() ), mtp.toMapCoordinates( v.bottomRight() ) ) );
+    // do we have to recompute the MapToPixel ?
   }
 
   mExtentPolygon.clear();
   mExtentPolygon.append( exteriorRing );
+
+  mSubRenderer->startRender( mContext, fields );
 }
 
 bool QgsInvertedPolygonRenderer::renderFeature( QgsFeature& feature, QgsRenderContext& context, int layer, bool selected, bool drawVertexMarker )
 {
-  Q_UNUSED( context );
+  if ( !context.painter() )
+  {
+    return false;
+  }
 
   // store this feature as a feature to render with decoration if needed
   if ( selected || drawVertexMarker )
@@ -153,33 +169,32 @@ bool QgsInvertedPolygonRenderer::renderFeature( QgsFeature& feature, QgsRenderCo
     return false;
   }
 
-  // We build here a "reversed" geometry of all the polygons
+  if ( ! mSymbolCategories.contains( catId ) )
-  //
-  // The final geometry is a multipolygon F, with :
-  // * the first polygon of F having the current extent as its exterior ring
-  // * each polygon's exterior ring is added as interior ring of the first polygon of F
-  // * each polygon's interior ring is added as new polygons in F
-  //
-  // No validity check is done, on purpose, it will be very slow and painting
-  // operations do not need geometries to be valid
-  if ( ! mFeaturesCategoryMap.contains( catId ) )
   {
     // the exterior ring must be a square in the destination CRS
     CombinedFeature cFeat;
     cFeat.multiPolygon.append( mExtentPolygon );
     // store the first feature
     cFeat.feature = feature;
-    mFeaturesCategoryMap.insert( catId, cFeat );
+    mSymbolCategories.insert( catId, mSymbolCategories.count() );
+    mFeaturesCategoryMap.append( cFeat );
   }
 
-  // update the gometry
+  // update the geometry
-  CombinedFeature& cFeat = mFeaturesCategoryMap[catId];
+  CombinedFeature& cFeat = mFeaturesCategoryMap[ mSymbolCategories[catId] ];
   QgsMultiPolygon multi;
   QgsGeometry* geom = feature.geometry();
   if ( !geom )
   {
     return false;
   }
+
+  const QgsCoordinateTransform* xform = context.coordinateTransform();
+  if ( xform )
+  {
+    geom->transform( *xform );
+  }
+
   if (( geom->wkbType() == QGis::WKBPolygon ) ||
       ( geom->wkbType() == QGis::WKBPolygon25D ) )
   {
@@ -191,43 +206,49 @@ bool QgsInvertedPolygonRenderer::renderFeature( QgsFeature& feature, QgsRenderCo
     multi = geom->asMultiPolygon();
   }
 
-  for ( int i = 0; i < multi.size(); i++ )
+  if ( mPreprocessingEnabled )
   {
-    // add the exterior ring as interior ring to the first polygon
+    // preprocessing
-    if ( mTransform )
+    if ( ! cFeat.feature.geometry() )
     {
-      QgsPolyline new_ls;
+      // first feature: add the current geometry
-      QgsPolyline& old_ls = multi[i][0];
+      cFeat.feature.setGeometry( new QgsGeometry( *geom ) );
-      for ( int k = 0; k < old_ls.size(); k++ )
-      {
-        new_ls.append( mTransform->transform( old_ls[k] ) );
-      }
-      cFeat.multiPolygon[0].append( new_ls );
     }
     else
     {
-      cFeat.multiPolygon[0].append( multi[i][0] );
+      // other features: combine them (union)
-    }
+      QgsGeometry* combined = cFeat.feature.geometry()->combine( geom );
-    // add interior rings as new polygons
+      if ( combined && combined->isGeosValid() )
-    for ( int j = 1; j < multi[i].size(); j++ )
-    {
-      QgsPolygon new_poly;
-      if ( mTransform )
       {
-        QgsPolyline new_ls;
+        cFeat.feature.setGeometry( combined );
-        QgsPolyline& old_ls = multi[i][j];
-        for ( int k = 0; k < old_ls.size(); k++ )
-        {
-          new_ls.append( mTransform->transform( old_ls[k] ) );
-        }
-        new_poly.append( new_ls );
       }
-      else
+    }
+  }
+  else
+  {
+    // No preprocessing involved.
+    // We build here a "reversed" geometry of all the polygons
+    //
+    // The final geometry is a multipolygon F, with :
+    // * the first polygon of F having the current extent as its exterior ring
+    // * each polygon's exterior ring is added as interior ring of the first polygon of F
+    // * each polygon's interior ring is added as new polygons in F
+    //
+    // No validity check is done, on purpose, it will be very slow and painting
+    // operations do not need geometries to be valid
+
+    for ( int i = 0; i < multi.size(); i++ )
+    {
+      // add the exterior ring as interior ring to the first polygon
+      cFeat.multiPolygon[0].append( multi[i][0] );
+
+      // add interior rings as new polygons
+      for ( int j = 1; j < multi[i].size(); j++ )
       {
+        QgsPolygon new_poly;
         new_poly.append( multi[i][j] );
+        cFeat.multiPolygon.append( new_poly );
       }
-
-      cFeat.multiPolygon.append( new_poly );
     }
   }
   return true;
@@ -239,12 +260,33 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )
   {
     return;
   }
+  if ( !context.painter() )
+  {
+    return;
+  }
 
   for ( FeatureCategoryMap::iterator cit = mFeaturesCategoryMap.begin(); cit != mFeaturesCategoryMap.end(); ++cit )
   {
-    QgsFeature feat( cit.value().feature );
+    QgsFeature feat( cit->feature );
-    feat.setGeometry( QgsGeometry::fromMultiPolygon( cit.value().multiPolygon ) );
+    if ( !mPreprocessingEnabled )
-    mSubRenderer->renderFeature( feat, context );
+    {
+      // no preprocessing - the final polygon has already been prepared
+      feat.setGeometry( QgsGeometry::fromMultiPolygon( cit->multiPolygon ) );
+    }
+    else
+    {
+      // preprocessing mode - we still have to invert (using difference)
+      if ( feat.geometry() )
+      {
+        QScopedPointer<QgsGeometry> rect( QgsGeometry::fromPolygon( mExtentPolygon ) );
+        QgsGeometry *final = rect->difference( feat.geometry() );
+        if ( final )
+        {
+          feat.setGeometry( final );
+        }
+      }
+    }
+    mSubRenderer->renderFeature( feat, mContext );
   }
 
   // when no features are visible, we still have to draw the exterior rectangle
@@ -256,22 +298,16 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )
     // empty feature with default attributes
     QgsFeature feat( mFields );
     feat.setGeometry( QgsGeometry::fromPolygon( mExtentPolygon ) );
-    mSubRenderer->renderFeature( feat, context );
+    mSubRenderer->renderFeature( feat, mContext );
   }
 
   // draw feature decorations
   foreach ( FeatureDecoration deco, mFeatureDecorations )
   {
-    mSubRenderer->renderFeature( deco.feature, context, deco.layer, deco.selected, deco.drawMarkers );
+    mSubRenderer->renderFeature( deco.feature, mContext, deco.layer, deco.selected, deco.drawMarkers );
   }
 
-  mSubRenderer->stopRender( context );
+  mSubRenderer->stopRender( mContext );
-
-  if ( mTransform )
-  {
-    // restore the coordinate transform if needed
-    context.setCoordinateTransform( mTransform );
-  }
 }
 
 QString QgsInvertedPolygonRenderer::dump() const
@@ -285,12 +321,17 @@ QString QgsInvertedPolygonRenderer::dump() const
 
 QgsFeatureRendererV2* QgsInvertedPolygonRenderer::clone()
 {
+  QgsInvertedPolygonRenderer* newRenderer;
   if ( mSubRenderer.isNull() )
   {
-    return new QgsInvertedPolygonRenderer( 0 );
+    newRenderer = new QgsInvertedPolygonRenderer( 0 );
+  }
+  else
+  {
+    newRenderer = new QgsInvertedPolygonRenderer( mSubRenderer->clone() );
   }
-  // else
+  newRenderer->setPreprocessingEnabled( preprocessingEnabled() );
-  return new QgsInvertedPolygonRenderer( mSubRenderer->clone() );
+  return newRenderer;
 }
 
 QgsFeatureRendererV2* QgsInvertedPolygonRenderer::create( QDomElement& element )
@@ -302,13 +343,15 @@ QgsFeatureRendererV2* QgsInvertedPolygonRenderer::create( QDomElement& element )
   {
     r->setEmbeddedRenderer( QgsFeatureRendererV2::load( embeddedRendererElem ) );
   }
+  r->setPreprocessingEnabled( element.attribute( "preprocessing", "0" ).toInt() == 1 );
   return r;
 }
 
 QDomElement QgsInvertedPolygonRenderer::save( QDomDocument& doc )
 {
   QDomElement rendererElem = doc.createElement( RENDERER_TAG_NAME );
   rendererElem.setAttribute( "type", "invertedPolygonRenderer" );
+  rendererElem.setAttribute( "preprocessing", preprocessingEnabled() ? "1" : "0" );
 
   if ( mSubRenderer )
   {
@@ -390,3 +433,4 @@ bool QgsInvertedPolygonRenderer::willRenderFeature( QgsFeature& feat )
   }
   return mSubRenderer->willRenderFeature( feat );
 }
+
diff --git a/src/core/symbology-ng/qgsinvertedpolygonrenderer.h b/src/core/symbology-ng/qgsinvertedpolygonrenderer.h
@@ -108,6 +108,16 @@ class CORE_EXPORT QgsInvertedPolygonRenderer : public QgsFeatureRendererV2
      */
     const QgsFeatureRendererV2* embeddedRenderer() const;
 
+    /** @returns true if the geometries are to be preprocessed (merged with an union) before rendering.*/
+    bool preprocessingEnabled() const { return mPreprocessingEnabled; }
+    /**
+        @param enabled enables or disables the preprocessing.
+        When enabled, geometries will be merged with an union before being rendered.
+        It allows to fix some rendering artefacts (when rendering overlapping polygons for instance).
+        This will involve some CPU-demanding computations and is thus disabled by default.
+    */
+    void setPreprocessingEnabled( bool enabled ) { mPreprocessingEnabled = enabled; }
+
   private:
     /** Private copy constructor. @see clone() */
     QgsInvertedPolygonRenderer( const QgsInvertedPolygonRenderer& );
@@ -123,15 +133,18 @@ class CORE_EXPORT QgsInvertedPolygonRenderer : public QgsFeatureRendererV2
       QgsMultiPolygon multiPolygon; //< the final combined geometry
       QgsFeature feature;           //< one feature (for attriute-based rendering)
     };
-    typedef QMap< QByteArray, CombinedFeature > FeatureCategoryMap;
+    typedef QVector<CombinedFeature> FeatureCategoryMap;
-    /** where features are stored, based on their symbol category */
+    /** where features are stored, based on the index of their symbol category @see mSymbolCategories */
     FeatureCategoryMap mFeaturesCategoryMap;
 
+    /** maps a category to an index */
+    QMap<QByteArray, int> mSymbolCategories;
+
     /** the polygon used as exterior ring that covers the current extent */
     QgsPolygon mExtentPolygon;
 
-    /** the current coordinate transform (or null) */
+    /** the context used for rendering */
-    const QgsCoordinateTransform* mTransform;
+    QgsRenderContext mContext;
 
     /** fields of each feature*/
     QgsFields mFields;
@@ -149,6 +162,9 @@ class CORE_EXPORT QgsInvertedPolygonRenderer : public QgsFeatureRendererV2
           feature( a_feature ), selected( a_selected ), drawMarkers( a_drawMarkers ), layer( a_layer ) {}
     };
     QList<FeatureDecoration> mFeatureDecorations;
+
+    /** whether to preprocess (merge) geometries before rendering*/
+    bool mPreprocessingEnabled;
 };
 
 

diff --git a/src/gui/symbology-ng/qgsinvertedpolygonrendererwidget.cpp b/src/gui/symbology-ng/qgsinvertedpolygonrendererwidget.cpp
@@ -68,6 +68,9 @@ QgsInvertedPolygonRendererWidget::QgsInvertedPolygonRendererWidget( QgsVectorLay
   {
     // an existing inverted renderer
     mRenderer.reset( static_cast<QgsInvertedPolygonRenderer*>( renderer ) );
+    mMergePolygonsCheckBox->blockSignals( true );
+    mMergePolygonsCheckBox->setCheckState( mRenderer->preprocessingEnabled() ? Qt::Checked : Qt::Unchecked );
+    mMergePolygonsCheckBox->blockSignals( false );
   }
 
   int currentEmbeddedIdx = 0;
@@ -123,12 +126,16 @@ void QgsInvertedPolygonRendererWidget::on_mRendererComboBox_currentIndexChanged(
   {
     mEmbeddedRendererWidget.reset( m->createRendererWidget( mLayer, mStyle, const_cast<QgsFeatureRendererV2*>( mRenderer->embeddedRenderer() )->clone() ) );
 
-    if ( mLayout->count() > 1 )
+    if ( mLayout->count() > 2 )
     {
       // remove the current renderer widget
-      mLayout->takeAt( 1 );
+      mLayout->takeAt( 2 );
     }
     mLayout->addWidget( mEmbeddedRendererWidget.data() );
   }
 }
 
+void QgsInvertedPolygonRendererWidget::on_mMergePolygonsCheckBox_stateChanged( int state )
+{
+  mRenderer->setPreprocessingEnabled( state == Qt::Checked );
+}