Skip to content

Commit

Permalink
Safer memory management in geos
Browse files Browse the repository at this point in the history 
Also speed up avoid intersections by removing unnecessary geometry
cloning
  • Loading branch information
@nyalldawson
nyalldawson committed Oct 13, 2017
1 parent c3fdaa9 commit 947b0cc
Show file tree
Hide file tree
Showing 8 changed files with 16 additions and 24 deletions.
1 change: 1 addition & 0 deletions doc/api_break.dox
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1289,6 +1289,7 @@ QgsGeometryEngine {#qgis_api_break_3_0_QgsGeometryEngine}
- `centroid()` returns the point instead of working on a parameter. The return value is a `nullptr` when `false` has been returned in the past.
- `pointOnSurface()` returns the point instead of working on a parameter. The return value is a `nullptr` when `false` has been returned in the past.
- splitGeometry() now returns new geometries as QgsGeometry, instead of QgsAbstractGeometry
- combine() now requires a list of QgsGeometry, instead of QgsAbstractGeometry


QgsGeometrySimplifier {#qgis_api_break_3_0_QgsGeometrySimplifier}
Expand Down
2 changes: 1 addition & 1 deletion python/core/geometry/qgsgeometryengine.sip
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class QgsGeometryEngine
:rtype: QgsAbstractGeometry
%End

virtual QgsAbstractGeometry *combine( const QList< QgsAbstractGeometry * > &geometries, QString *errorMsg = 0 ) const = 0 /Factory/;
virtual QgsAbstractGeometry *combine( const QList< QgsGeometry > &geometries, QString *errorMsg = 0 ) const = 0 /Factory/;
%Docstring
Calculate the combination of this and ``geometries``.

Expand Down
11 changes: 1 addition & 10 deletions src/core/geometry/qgsgeometry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2228,17 +2228,8 @@ QgsGeometry QgsGeometry::unaryUnion( const QList<QgsGeometry> &geometries )
{
QgsGeos geos( nullptr );

QList<QgsAbstractGeometry *> geomV2List;
for ( const QgsGeometry &g : geometries )
{
if ( !( g.isNull() ) )
{
geomV2List.append( g.geometry() );
}
}

QString error;
std::unique_ptr< QgsAbstractGeometry > geom( geos.combine( geomV2List, &error ) );
std::unique_ptr< QgsAbstractGeometry > geom( geos.combine( geometries, &error ) );
QgsGeometry result( std::move( geom ) );
result.mLastError = error;
return result;
Expand Down
6 changes: 2 additions & 4 deletions src/core/geometry/qgsgeometryeditutils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ std::unique_ptr<QgsAbstractGeometry> QgsGeometryEditUtils::avoidIntersections( c
if ( avoidIntersectionsLayers.isEmpty() )
return nullptr; //no intersections stored in project does not mean error

QList< QgsAbstractGeometry * > nearGeometries;
QList< QgsGeometry > nearGeometries;

//go through list, convert each layer to vector layer and call QgsVectorLayer::removePolygonIntersections for each
for ( QgsVectorLayer *currentLayer : avoidIntersectionsLayers )
Expand All @@ -262,7 +262,7 @@ std::unique_ptr<QgsAbstractGeometry> QgsGeometryEditUtils::avoidIntersections( c
if ( !f.hasGeometry() )
continue;

nearGeometries << f.geometry().geometry()->clone();
nearGeometries << f.geometry();
}
}

Expand All @@ -271,9 +271,7 @@ std::unique_ptr<QgsAbstractGeometry> QgsGeometryEditUtils::avoidIntersections( c
return nullptr;
}


std::unique_ptr< QgsAbstractGeometry > combinedGeometries( geomEngine->combine( nearGeometries ) );
qDeleteAll( nearGeometries );
if ( !combinedGeometries )
{
return nullptr;
Expand Down
2 changes: 1 addition & 1 deletion src/core/geometry/qgsgeometryengine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class CORE_EXPORT QgsGeometryEngine
*
* \since QGIS 3.0 \a geom is a pointer
*/
virtual QgsAbstractGeometry *combine( const QList< QgsAbstractGeometry * > &geometries, QString *errorMsg = nullptr ) const = 0 SIP_FACTORY;
virtual QgsAbstractGeometry *combine( const QList< QgsGeometry > &geometries, QString *errorMsg = nullptr ) const = 0 SIP_FACTORY;

/**
* Calculate the symmetric difference of this and \a geom.
Expand Down
11 changes: 7 additions & 4 deletions src/core/geometry/qgsgeos.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,14 +333,17 @@ QgsAbstractGeometry *QgsGeos::combine( const QgsAbstractGeometry *geom, QString
return overlay( geom, UNION, errorMsg ).release();
}

QgsAbstractGeometry *QgsGeos::combine( const QList<QgsAbstractGeometry *> &geomList, QString *errorMsg ) const

This comment has been minimized.

Sign in to view

Copy link
@manisandro

manisandro Oct 19, 2017

Member

@nyalldawson What's the reason behind this change? It pretty much does the opposite of speeding up things in the geometry checker because I now need to construct a QgsGeometry for each geometry to combine, instead of just being able to pass the QgsAbstractGeometries which I already have.

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Oct 20, 2017

Author Collaborator

Because every use of that method in master is already working from a list of QgsGeometry - not QgsAbstractGeometries. So they are being forced to create a new list just to pass to this method.

Check through this commit:

  • QgsGeometry::unaryUnion - already has a list of QgsGeometry, forced to create a new list
  • QgsGeometryEditUtils::avoidIntersections and QgsGeometryGapCheck::collectErrors - already were constructing a list, so no extra list construction was added. And even worse, they were being forced to clone the abstract geometries just to call this method.

So basically - no extra list construction added, some avoided, lots of geometry cloning avoided. Win all round (including for the geometry checker)!

I can't see anything in #5410 that changes this, and in that PR you've reverted the benefit of avoiding the clone by reintroducing that method. Ouch!

This comment has been minimized.

Sign in to view

Copy link
@manisandro

manisandro Oct 20, 2017

Member

@nyalldawson I've just re-added the method using the abstract geometries, the reason being [1]. The method taking the list of QgsGeometry is still there. And after all the reason I'm actually asking is to see whether I should be doing anything differently. From your reply I understand that there was no real use-case for the method taking the list of QgsAbstractGeometry in master previously, but why can't we just keep both?

[1] https://github.com/manisandro/QGIS/blob/geomchecker/src/analysis/vector/geometry_checker/qgsgeometrygapcheck.cpp#L43

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Oct 20, 2017

Author Collaborator

Yes - but look at what you're doing there:

QList<QgsAbstractGeometry *> geomList;
...
geomList.append( layerFeature.geometry()->clone() ); <-- ouch, a clone!!
...
QgsAbstractGeometry *unionGeom = geomEngine->combine( geomList, &errMsg );

vs:

QList geomList;
...
geomList.append( layerFeature.geometry() ); <-- no clone, nice and fast
...
QgsAbstractGeometry *unionGeom = geomEngine->combine( geomList, &errMsg );

This comment has been minimized.

Sign in to view

Copy link
@manisandro

manisandro Oct 20, 2017

Member

Uhm but layerFeature.geometry() is a QgsAbstractGeometry, not a QgsGeometry, the clone is simply due to the scope of the layerFeature iterator. Or what am I missing here?

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Oct 20, 2017

Author Collaborator

So hang on, I was supposed to know in advance that you'd be submitting a PR the next day which changes master behavior? ;) None of us QGIS devs are THAT good! I'm looking at master, and QgsGeometryGapCheck::collectErrors uses QgsFeatures, and where feature.geometry() is a QgsGeometry => this commit makes a big improvement for master and is clearly justifiable.

I haven't had a chance to look yet at your new PR, but I'd question why QgsAbstractGeometry is being used for LayerFeature instead of QgsGeometry. QgsGeometry is much safer for memory management, and in almost all cases faster to use because it avoids cloning the abstract geometries. (e.g. using it for combine would avoid the unecessary clone)

This comment has been minimized.

Sign in to view

Copy link
@manisandro

manisandro Oct 21, 2017

Member

Perhaps my original question wasn't optimally formulated, but it was a genuine question about the reason for the change, I don't think I ever said it shouldn't have been done or that you should have had a crystal ball or whatever... All I'm saying is that I'd rather not have the other method removed and whether we can't just keep both.

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Oct 21, 2017

Author Collaborator

Ok, truce ;)

What I'm wondering is whether we can just leave the alternate (i.e. abstract geometry) version as either a util function in the geometry checker OR implement it in qgsgeos alone, but not expose via QgsGeometryEngine. (I'm just thinking about minimising api complexity here).

Would that work for you?

This comment has been minimized.

Sign in to view

Copy link
@manisandro

manisandro Oct 23, 2017

Member

I mean, sure, but as I view things one should rather attempt to make it easier to switch out the geometry engine, not adding additional explicit requirements on a particular implementation just for the sake of API preferences. Also with all methods in QgsGeometryEngine working with QgsAbstractGeometry, it looks like an odd thing to me that one particular method only works with QgsGeometry. As I said, if the QgsGeometry method works better for some code, great, but I don't see why we can't just have the other one also.
QgsAbstractGeometry *QgsGeos::combine( const QList<QgsGeometry> &geomList, QString *errorMsg ) const
{

QVector< GEOSGeometry * > geosGeometries;
geosGeometries.resize( geomList.size() );
for ( int i = 0; i < geomList.size(); ++i )
geosGeometries.reserve( geomList.size() );
for ( const QgsGeometry &g : geomList )
{
geosGeometries[i] = asGeos( geomList.at( i ), mPrecision );
if ( !g )
continue;

geosGeometries << asGeos( g.geometry(), mPrecision );
}

GEOSGeometry *geomUnion = nullptr;
Expand Down
2 changes: 1 addition & 1 deletion src/core/geometry/qgsgeos.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine
std::unique_ptr< QgsAbstractGeometry > subdivide( int maxNodes, QString *errorMsg = nullptr ) const;

QgsAbstractGeometry *combine( const QgsAbstractGeometry *geom, QString *errorMsg = nullptr ) const override;
QgsAbstractGeometry *combine( const QList< QgsAbstractGeometry *> &, QString *errorMsg = nullptr ) const override;
QgsAbstractGeometry *combine( const QList< QgsGeometry > &, QString *errorMsg = nullptr ) const override;
QgsAbstractGeometry *symDifference( const QgsAbstractGeometry *geom, QString *errorMsg = nullptr ) const override;
QgsAbstractGeometry *buffer( double distance, int segments, QString *errorMsg = nullptr ) const override;
QgsAbstractGeometry *buffer( double distance, int segments, int endCapStyle, int joinStyle, double miterLimit, QString *errorMsg = nullptr ) const override;
Expand Down
5 changes: 2 additions & 3 deletions src/plugins/geometry_checker/checks/qgsgeometrygapcheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ void QgsGeometryGapCheck::collectErrors( QList<QgsGeometryCheckError *> &errors,
if ( progressCounter ) progressCounter->fetchAndAddRelaxed( 1 );

// Collect geometries, build spatial index
QList<QgsAbstractGeometry *> geomList;
QList<QgsGeometry > geomList;
const QgsFeatureIds &featureIds = ids.isEmpty() ? mFeaturePool->getFeatureIds() : ids;
Q_FOREACH ( QgsFeatureId id, featureIds )
{
QgsFeature feature;
if ( mFeaturePool->get( id, feature ) )
{
geomList.append( feature.geometry().geometry()->clone() );
geomList.append( feature.geometry() );
}
}

Expand All @@ -46,7 +46,6 @@ void QgsGeometryGapCheck::collectErrors( QList<QgsGeometryCheckError *> &errors,
// Create union of geometry
QString errMsg;
QgsAbstractGeometry *unionGeom = geomEngine->combine( geomList, &errMsg );
qDeleteAll( geomList );
delete geomEngine;
if ( !unionGeom )
{
Expand Down

0 comments on commit 947b0cc

Please sign in to comment.