Skip to content

Commit

Permalink
Rename QgsGeometry::geometry as QgsGeometry::get()
Browse files Browse the repository at this point in the history
Because feature.geometry().geometry() is confusing, and impossible
to search for in python code (e.g. is input.geometry() a QgsGeometry
or a QgsAbstractGeometry?)

But more importantantly: also add a const version
QgsGeometry::constGet(). The non-const
version is slow, since it now forces a detach to avoid corrupting
geometries (since QgsGeometry is shared, it's not safe to directly
access its primitive QgsAbstractGeometry and start messing with
it without first detaching). This is a big risk in the 2.x API
which could potentially corrupt feature geometries with unexpected
outcomes.

Update all uses to constGet where possible.
  • Loading branch information
nyalldawson committed Oct 25, 2017
1 parent a89dfde commit 7036106
Show file tree
Hide file tree
Showing 83 changed files with 417 additions and 357 deletions.
2 changes: 1 addition & 1 deletion python/core/geometry/qgsabstractgeometry.sip
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ class QgsAbstractGeometry
:rtype: bool
%End

virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ ) = 0;
virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ ) const = 0;
%Docstring
Returns the vertices adjacent to a specified ``vertex`` within a geometry.
.. versionadded:: 3.0
Expand Down
2 changes: 1 addition & 1 deletion python/core/geometry/qgscurve.sip
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class QgsCurve: QgsAbstractGeometry

virtual bool nextVertex( QgsVertexId &id, QgsPoint &vertex /Out/ ) const;

virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ );
virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ ) const;

virtual int vertexNumberFromVertexId( QgsVertexId id ) const;

Expand Down
2 changes: 1 addition & 1 deletion python/core/geometry/qgscurvepolygon.sip
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ Adds an interior ring to the geometry (takes ownership)

virtual bool nextVertex( QgsVertexId &id, QgsPoint &vertex /Out/ ) const;

virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ );
virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ ) const;

virtual bool hasCurvedSegments() const;

Expand Down
45 changes: 38 additions & 7 deletions python/core/geometry/qgsgeometry.sip
Original file line number Diff line number Diff line change
Expand Up @@ -84,19 +84,50 @@ Copy constructor will prompt a deep copy of the object

~QgsGeometry();

QgsAbstractGeometry *geometry() const;
const QgsAbstractGeometry *constGet() const;
%Docstring
Returns the underlying geometry store.
.. versionadded:: 2.10
.. seealso:: setGeometry
Returns a non-modifiable (const) reference to the underlying abstract geometry primitive.

This is much faster then calling the non-const get() method.

.. note::

In QGIS 2.x this method was named geometry().

.. versionadded:: 3.0
.. seealso:: primitive()
.. seealso:: set()
:rtype: QgsAbstractGeometry
%End

QgsAbstractGeometry *get();
%Docstring
Returns a modifiable (non-const) reference to the underlying abstract geometry primitive.

This method can be slow to call, as it may trigger a detachment of the geometry
and a deep copy. Where possible, use constGet() instead.

.. note::

In QGIS 2.x this method was named geometry().

.. versionadded:: 3.0
.. seealso:: constGet()
.. seealso:: set()
:rtype: QgsAbstractGeometry
%End

void setGeometry( QgsAbstractGeometry *geometry /Transfer/ );
void set( QgsAbstractGeometry *geometry /Transfer/ );
%Docstring
Sets the underlying geometry store. Ownership of geometry is transferred.
.. versionadded:: 2.10
.. seealso:: geometry

.. note::

In QGIS 2.x this method was named setGeometry().

.. versionadded:: 3.0
.. seealso:: get()
.. seealso:: constGet()
%End

bool isNull() const;
Expand Down
2 changes: 1 addition & 1 deletion python/core/geometry/qgsgeometrycollection.sip
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class QgsGeometryCollection: QgsAbstractGeometry

virtual QgsAbstractGeometry *boundary() const /Factory/;

virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ );
virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ ) const;

virtual int vertexNumberFromVertexId( QgsVertexId id ) const;

Expand Down
2 changes: 1 addition & 1 deletion python/core/geometry/qgspoint.sip
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ class QgsPoint: QgsAbstractGeometry

virtual bool nextVertex( QgsVertexId &id, QgsPoint &vertex /Out/ ) const;

virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ );
virtual void adjacentVertices( QgsVertexId vertex, QgsVertexId &previousVertex /Out/, QgsVertexId &nextVertex /Out/ ) const;


virtual double vertexAngle( QgsVertexId vertex ) const;
Expand Down
8 changes: 4 additions & 4 deletions src/analysis/network/qgsvectorlayerdirector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,9 @@ void QgsVectorLayerDirector::makeGraph( QgsGraphBuilderInterface *builder, const
}

QgsMultiPolyline mpl;
if ( QgsWkbTypes::flatType( feature.geometry().geometry()->wkbType() ) == QgsWkbTypes::MultiLineString )
if ( QgsWkbTypes::flatType( feature.geometry().wkbType() ) == QgsWkbTypes::MultiLineString )
mpl = feature.geometry().asMultiPolyline();
else if ( QgsWkbTypes::flatType( feature.geometry().geometry()->wkbType() ) == QgsWkbTypes::LineString )
else if ( QgsWkbTypes::flatType( feature.geometry().wkbType() ) == QgsWkbTypes::LineString )
mpl.push_back( feature.geometry().asPolyline() );

QgsMultiPolyline::iterator mplIt;
Expand Down Expand Up @@ -308,9 +308,9 @@ void QgsVectorLayerDirector::makeGraph( QgsGraphBuilderInterface *builder, const

// begin features segments and add arc to the Graph;
QgsMultiPolyline mpl;
if ( QgsWkbTypes::flatType( feature.geometry().geometry()->wkbType() ) == QgsWkbTypes::MultiLineString )
if ( QgsWkbTypes::flatType( feature.geometry().wkbType() ) == QgsWkbTypes::MultiLineString )
mpl = feature.geometry().asMultiPolyline();
else if ( QgsWkbTypes::flatType( feature.geometry().geometry()->wkbType() ) == QgsWkbTypes::LineString )
else if ( QgsWkbTypes::flatType( feature.geometry().wkbType() ) == QgsWkbTypes::LineString )
mpl.push_back( feature.geometry().asPolyline() );

QgsMultiPolyline::iterator mplIt;
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/processing/qgsalgorithmboundary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ QgsFeature QgsBoundaryAlgorithm::processFeature( const QgsFeature &feature, QgsP
if ( feature.hasGeometry() )
{
QgsGeometry inputGeometry = feature.geometry();
QgsGeometry outputGeometry = QgsGeometry( inputGeometry.geometry()->boundary() );
QgsGeometry outputGeometry = QgsGeometry( inputGeometry.constGet()->boundary() );
if ( !outputGeometry )
{
feedback->reportError( QObject::tr( "No boundary for feature %1 (possibly a closed linestring?)'" ).arg( feature.id() ) );
Expand Down
8 changes: 4 additions & 4 deletions src/analysis/processing/qgsalgorithmclip.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ QVariantMap QgsClipAlgorithm::processAlgorithm( const QVariantMap &parameters, Q
}

// use prepared geometries for faster intersection tests
std::unique_ptr< QgsGeometryEngine > engine( QgsGeometry::createGeometryEngine( combinedClipGeom.geometry() ) );
std::unique_ptr< QgsGeometryEngine > engine( QgsGeometry::createGeometryEngine( combinedClipGeom.constGet() ) );
engine->prepareGeometry();

QgsFeatureIds testedFeatureIds;
Expand Down Expand Up @@ -154,15 +154,15 @@ QVariantMap QgsClipAlgorithm::processAlgorithm( const QVariantMap &parameters, Q
}
testedFeatureIds.insert( inputFeature.id() );

if ( !engine->intersects( inputFeature.geometry().geometry() ) )
if ( !engine->intersects( inputFeature.geometry().constGet() ) )
continue;

QgsGeometry newGeometry;
if ( !engine->contains( inputFeature.geometry().geometry() ) )
if ( !engine->contains( inputFeature.geometry().constGet() ) )
{
QgsGeometry currentGeometry = inputFeature.geometry();
newGeometry = combinedClipGeom.intersection( currentGeometry );
if ( newGeometry.wkbType() == QgsWkbTypes::Unknown || QgsWkbTypes::flatType( newGeometry.geometry()->wkbType() ) == QgsWkbTypes::GeometryCollection )
if ( newGeometry.wkbType() == QgsWkbTypes::Unknown || QgsWkbTypes::flatType( newGeometry.wkbType() ) == QgsWkbTypes::GeometryCollection )
{
QgsGeometry intCom = inputFeature.geometry().combine( newGeometry );
QgsGeometry intSym = inputFeature.geometry().symDifference( newGeometry );
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/processing/qgsalgorithmconvexhull.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ QgsFeature QgsConvexHullAlgorithm::processFeature( const QgsFeature &feature, Qg
if ( outputGeometry )
{
QgsAttributes attrs = f.attributes();
attrs << outputGeometry.geometry()->area()
<< outputGeometry.geometry()->perimeter();
attrs << outputGeometry.constGet()->area()
<< outputGeometry.constGet()->perimeter();
f.setAttributes( attrs );
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/processing/qgsalgorithmdropmzvalues.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ QgsFeature QgsDropMZValuesAlgorithm::processFeature( const QgsFeature &feature,
QgsFeature f = feature;
if ( f.hasGeometry() )
{
std::unique_ptr< QgsAbstractGeometry > newGeom( f.geometry().geometry()->clone() );
std::unique_ptr< QgsAbstractGeometry > newGeom( f.geometry().constGet()->clone() );
if ( mDropM )
newGeom->dropMValue();
if ( mDropZ )
Expand Down
18 changes: 9 additions & 9 deletions src/analysis/processing/qgsalgorithmextractbylocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ void QgsLocationBasedAlgorithm::process( QgsFeatureSource *targetSource,

if ( !engine )
{
engine.reset( QgsGeometry::createGeometryEngine( f.geometry().geometry() ) );
engine.reset( QgsGeometry::createGeometryEngine( f.geometry().constGet() ) );
engine->prepareGeometry();
}

Expand All @@ -146,31 +146,31 @@ void QgsLocationBasedAlgorithm::process( QgsFeatureSource *targetSource,
switch ( predicate )
{
case Intersects:
isMatch = engine->intersects( testFeature.geometry().geometry() );
isMatch = engine->intersects( testFeature.geometry().constGet() );
break;
case Contains:
isMatch = engine->contains( testFeature.geometry().geometry() );
isMatch = engine->contains( testFeature.geometry().constGet() );
break;
case Disjoint:
if ( engine->intersects( testFeature.geometry().geometry() ) )
if ( engine->intersects( testFeature.geometry().constGet() ) )
{
disjointSet.remove( testFeature.id() );
}
break;
case IsEqual:
isMatch = engine->isEqual( testFeature.geometry().geometry() );
isMatch = engine->isEqual( testFeature.geometry().constGet() );
break;
case Touches:
isMatch = engine->touches( testFeature.geometry().geometry() );
isMatch = engine->touches( testFeature.geometry().constGet() );
break;
case Overlaps:
isMatch = engine->overlaps( testFeature.geometry().geometry() );
isMatch = engine->overlaps( testFeature.geometry().constGet() );
break;
case Within:
isMatch = engine->within( testFeature.geometry().geometry() );
isMatch = engine->within( testFeature.geometry().constGet() );
break;
case Crosses:
isMatch = engine->crosses( testFeature.geometry().geometry() );
isMatch = engine->crosses( testFeature.geometry().constGet() );
break;
}
if ( isMatch )
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/processing/qgsalgorithmfixgeometries.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ QgsFeature QgsFixGeometriesAlgorithm::processFeature( const QgsFeature &feature,
}

if ( outputGeometry.wkbType() == QgsWkbTypes::Unknown ||
QgsWkbTypes::flatType( outputGeometry.geometry()->wkbType() ) == QgsWkbTypes::GeometryCollection )
QgsWkbTypes::flatType( outputGeometry.wkbType() ) == QgsWkbTypes::GeometryCollection )
{
// keep only the parts of the geometry collection with correct type
const QList< QgsGeometry > tmpGeometries = outputGeometry.asGeometryCollection();
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/processing/qgsalgorithmjoinwithlines.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ QVariantMap QgsJoinWithLinesAlgorithm::processAlgorithm( const QVariantMap &para
{
QgsPoint p;
if ( feature.geometry().type() == QgsWkbTypes::PointGeometry && !feature.geometry().isMultipart() )
p = *static_cast< QgsPoint *>( feature.geometry().geometry() );
p = *static_cast< const QgsPoint *>( feature.geometry().constGet() );
else
p = *static_cast< QgsPoint *>( feature.geometry().pointOnSurface().geometry() );
p = *static_cast< const QgsPoint *>( feature.geometry().pointOnSurface().constGet() );
if ( hasZ && !p.is3D() )
p.addZValue( 0 );
if ( hasM && !p.isMeasure() )
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/processing/qgsalgorithmlineintersection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ QVariantMap QgsLineIntersectionAlgorithm::processAlgorithm( const QVariantMap &p
if ( !lines.empty() )
{
// use prepared geometries for faster intersection tests
std::unique_ptr< QgsGeometryEngine > engine( QgsGeometry::createGeometryEngine( inGeom.geometry() ) );
std::unique_ptr< QgsGeometryEngine > engine( QgsGeometry::createGeometryEngine( inGeom.constGet() ) );
engine->prepareGeometry();

QgsFeatureRequest request = QgsFeatureRequest().setFilterFids( lines );
Expand All @@ -178,7 +178,7 @@ QVariantMap QgsLineIntersectionAlgorithm::processAlgorithm( const QVariantMap &p
}

QgsGeometry tmpGeom = inFeatureB.geometry();
if ( engine->intersects( tmpGeom.geometry() ) )
if ( engine->intersects( tmpGeom.constGet() ) )
{
QgsMultiPoint points;
QgsGeometry intersectGeom = inGeom.intersection( tmpGeom );
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/processing/qgsalgorithmmeancoordinates.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ QVariantMap QgsMeanCoordinatesAlgorithm::processAlgorithm( const QVariantMap &pa

QgsVertexId vid;
QgsPoint pt;
const QgsAbstractGeometry *g = feat.geometry().geometry();
const QgsAbstractGeometry *g = feat.geometry().constGet();
// NOTE - should this be including the duplicate nodes for closed rings? currently it is,
// but I suspect that the expected behavior would be to NOT include these
while ( g->nextVertex( vid, pt ) )
Expand Down
14 changes: 7 additions & 7 deletions src/analysis/processing/qgsalgorithmsplitwithlines.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ QVariantMap QgsSplitWithLinesAlgorithm::processAlgorithm( const QVariantMap &par
QgsGeometry splitGeom = splitGeoms.value( line );
if ( !engine )
{
engine.reset( QgsGeometry::createGeometryEngine( inGeom.geometry() ) );
engine.reset( QgsGeometry::createGeometryEngine( inGeom.constGet() ) );
engine->prepareGeometry();
}

if ( engine->intersects( splitGeom.geometry() ) )
if ( engine->intersects( splitGeom.constGet() ) )
{
QList< QgsGeometry > splitGeomParts = splitGeom.asGeometryCollection();
splittingLines.append( splitGeomParts );
Expand All @@ -159,7 +159,7 @@ QVariantMap QgsSplitWithLinesAlgorithm::processAlgorithm( const QVariantMap &par
QList< QgsGeometry > outGeoms;

// use prepared geometries for faster intersection tests
std::unique_ptr< QgsGeometryEngine > splitGeomEngine( QgsGeometry::createGeometryEngine( splitGeom.geometry() ) );
std::unique_ptr< QgsGeometryEngine > splitGeomEngine( QgsGeometry::createGeometryEngine( splitGeom.constGet() ) );
splitGeomEngine->prepareGeometry();
while ( !inGeoms.empty() )
{
Expand All @@ -172,12 +172,12 @@ QVariantMap QgsSplitWithLinesAlgorithm::processAlgorithm( const QVariantMap &par
if ( !inGeom )
continue;

if ( splitGeomEngine->intersects( inGeom.geometry() ) )
if ( splitGeomEngine->intersects( inGeom.constGet() ) )
{
QgsGeometry before = inGeom;
if ( splitterPList.empty() )
{
const QgsCoordinateSequence sequence = splitGeom.geometry()->coordinateSequence();
const QgsCoordinateSequence sequence = splitGeom.constGet()->coordinateSequence();
for ( const QgsRingSequence &part : sequence )
{
for ( const QgsPointSequence &ring : part )
Expand Down Expand Up @@ -237,12 +237,12 @@ QVariantMap QgsSplitWithLinesAlgorithm::processAlgorithm( const QVariantMap &par
bool passed = true;
if ( QgsWkbTypes::geometryType( aGeom.wkbType() ) == QgsWkbTypes::LineGeometry )
{
int numPoints = aGeom.geometry()->nCoordinates();
int numPoints = aGeom.constGet()->nCoordinates();

if ( numPoints <= 2 )
{
if ( numPoints == 2 )
passed = !static_cast< QgsCurve * >( aGeom.geometry() )->isClosed(); // tests if vertex 0 = vertex 1
passed = !static_cast< const QgsCurve * >( aGeom.constGet() )->isClosed(); // tests if vertex 0 = vertex 1
else
passed = false; // sometimes splitting results in lines of zero length
}
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/processing/qgsalgorithmtransect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ QVariantMap QgsTransectAlgorithm::processAlgorithm( const QVariantMap &parameter
QgsGeometry inputGeometry = feat.geometry();

inputGeometry.convertToMultiType();
const QgsMultiLineString *multiLine = static_cast< QgsMultiLineString * >( inputGeometry.geometry() );
const QgsMultiLineString *multiLine = static_cast< const QgsMultiLineString * >( inputGeometry.constGet() );
for ( int id = 0; id < multiLine->numGeometries(); ++id )
{
const QgsLineString *line = static_cast< const QgsLineString * >( multiLine->geometryN( id ) );
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ void QgsFeaturePool::updateFeature( QgsFeature &feature )
get( feature.id(), origFeature );

QgsGeometryMap geometryMap;
geometryMap.insert( feature.id(), QgsGeometry( feature.geometry().geometry()->clone() ) );
geometryMap.insert( feature.id(), QgsGeometry( feature.geometry().constGet()->clone() ) );
QgsChangedAttributesMap changedAttributesMap;
QgsAttributeMap attribMap;
for ( int i = 0, n = feature.attributes().size(); i < n; ++i )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void QgsGeometryAngleCheck::fixError( QgsGeometryCheckError *error, int method,
return;
}
QgsGeometry featureGeometry = feature.geometry();
QgsAbstractGeometry *geometry = featureGeometry.geometry();
QgsAbstractGeometry *geometry = featureGeometry.get();
QgsVertexId vidx = error->vidx();

// Check if point still exists
Expand Down
Loading

1 comment on commit 7036106

@nirvn
Copy link
Contributor

@nirvn nirvn commented on 7036106 Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyalldawson , error when WITH_3D is enabled:

/home/webmaster/dev/cpp/QGIS/src/3d/qgs3dutils.cpp: In static member function ‘static QList<QVector3D> Qgs3DUtils::positions(const Qgs3DMapSettings&, QgsVectorLayer*, const QgsFeatureRequest&)’:
/home/webmaster/dev/cpp/QGIS/src/3d/qgs3dutils.cpp:174:49: error: ‘class QgsGeometry’ has no member named ‘geometry’; did you mean ‘QgsGeometry’?
     const QgsAbstractGeometry *g = f.geometry().geometry();
...
/home/webmaster/dev/cpp/QGIS/src/3d/symbols/qgspolygon3dsymbol_p.cpp: In member function ‘Qt3DRender::QGeometryRenderer* QgsPolygon3DSymbolEntityNode::renderer(const Qgs3DMapSettings&, const QgsPolygon3DSymbol&, const QgsVectorLayer*, const QgsFeatureRequest&)’:
/home/webmaster/dev/cpp/QGIS/src/3d/symbols/qgspolygon3dsymbol_p.cpp:145:42: error: ‘class QgsGeometry’ has no member named ‘geometry’; did you mean ‘QgsGeometry’?
     if ( QgsWkbTypes::isCurvedType( geom.geometry()->wkbType() ) )
                                          ^~~~~~~~
                                          QgsGeometry
/home/webmaster/dev/cpp/QGIS/src/3d/symbols/qgspolygon3dsymbol_p.cpp:146:32: error: ‘class QgsGeometry’ has no member named ‘geometry’; did you mean ‘QgsGeometry’?
       geom = QgsGeometry( geom.geometry()->segmentize() );
                                ^~~~~~~~
                                QgsGeometry
/home/webmaster/dev/cpp/QGIS/src/3d/symbols/qgspolygon3dsymbol_p.cpp:148:41: error: ‘class QgsGeometry’ has no member named ‘geometry’; did you mean ‘QgsGeometry’?
     const QgsAbstractGeometry *g = geom.geometry();

Please sign in to comment.