Skip to content

Commit

Permalink
Fix crash in node tool after deleting the whole geometry (fixes #15659)
Browse files Browse the repository at this point in the history
Made sure that both closestVertex() and closestSegment() return negative
distance on error (e.g. with null or emtpy geometry).

Also fixes snapping when dealing with layers with null/invalid geometries

(cherry picked from commit c093d51)
  • Loading branch information
wonder-sk committed Oct 31, 2016
1 parent dd4b34e commit 24fbe1a
Show file tree
Hide file tree
Showing 12 changed files with 72 additions and 22 deletions.
2 changes: 1 addition & 1 deletion python/core/geometry/qgsgeometry.sip
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ class QgsGeometry
* @param afterVertex will be set to the vertex index of the next vertex after the closest one. Will be set to -1 if
* not present.
* @param sqrDist will be set to the square distance between the closest vertex and the specified point
* @returns closest point in geometry
* @returns closest point in geometry. If not found (empty geometry), returns null point nad sqrDist is negative.
*/
//TODO QGIS 3.0 - rename beforeVertex to previousVertex, afterVertex to nextVertex
QgsPoint closestVertex( const QgsPoint& point, int& atVertex /Out/, int& beforeVertex /Out/, int& afterVertex /Out/, double& sqrDist /Out/ ) const;
Expand Down
7 changes: 3 additions & 4 deletions src/app/nodetool/qgsmaptoolnodetool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,12 +274,11 @@ void QgsMapToolNodeTool::canvasPressEvent( QgsMapMouseEvent* e )

// get geometry and find if snapping is near it
int atVertex, beforeVertex, afterVertex;
double dist;
QgsPoint closestLayerVertex = mSelectedFeature->geometry()->closestVertex( layerCoordPoint, atVertex, beforeVertex, afterVertex, dist );
dist = sqrt( dist );
double sqrDist; // will be negative on error
QgsPoint closestLayerVertex = mSelectedFeature->geometry()->closestVertex( layerCoordPoint, atVertex, beforeVertex, afterVertex, sqrDist );

mSnapper.snapToCurrentLayer( e->pos(), snapResults, QgsSnapper::SnapToVertex, tol );
if ( dist <= tol )
if ( sqrDist >= 0 && sqrt( sqrDist ) <= tol )
{
// some vertex selected
mMoving = true;
Expand Down
3 changes: 3 additions & 0 deletions src/core/geometry/qgscircularstring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,9 @@ double QgsCircularString::closestSegment( const QgsPointV2& pt, QgsPointV2& segm
}
}

if ( minDist == std::numeric_limits<double>::max() )
return -1; // error: no segments

segmentPt = minDistSegmentPoint;
vertexAfter = minDistVertexAfter;
vertexAfter.part = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/core/geometry/qgscurvepolygon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ double QgsCurvePolygon::closestSegment( const QgsPointV2& pt, QgsPointV2& segmen
{
if ( !mExteriorRing )
{
return 0.0;
return -1;
}
QList<QgsCurve*> segmentList;
segmentList.append( mExteriorRing );
Expand Down
9 changes: 7 additions & 2 deletions src/core/geometry/qgsgeometry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ QgsPoint QgsGeometry::closestVertex( const QgsPoint& point, int& atVertex, int&
{
if ( !d->geometry )
{
sqrDist = -1;
return QgsPoint( 0, 0 );
}

Expand Down Expand Up @@ -609,12 +610,14 @@ double QgsGeometry::closestVertexWithContext( const QgsPoint& point, int& atVert
{
if ( !d->geometry )
{
return 0.0;
return -1;
}

QgsVertexId vId;
QgsPointV2 pt( point.x(), point.y() );
QgsPointV2 closestPoint = QgsGeometryUtils::closestVertex( *( d->geometry ), pt, vId );
if ( !vId.isValid() )
return -1;
atVertex = vertexNrFromVertexId( vId );
return QgsGeometryUtils::sqrDistance2D( closestPoint, pt );
}
Expand All @@ -628,14 +631,16 @@ double QgsGeometry::closestSegmentWithContext(
{
if ( !d->geometry )
{
return 0;
return -1;
}

QgsPointV2 segmentPt;
QgsVertexId vertexAfter;
bool leftOfBool;

double sqrDist = d->geometry->closestSegment( QgsPointV2( point.x(), point.y() ), segmentPt, vertexAfter, &leftOfBool, epsilon );
if ( sqrDist < 0 )
return -1;

minDistPoint.setX( segmentPt.x() );
minDistPoint.setY( segmentPt.y() );
Expand Down
2 changes: 1 addition & 1 deletion src/core/geometry/qgsgeometry.h
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class CORE_EXPORT QgsGeometry
* @param afterVertex will be set to the vertex index of the next vertex after the closest one. Will be set to -1 if
* not present.
* @param sqrDist will be set to the square distance between the closest vertex and the specified point
* @returns closest point in geometry
* @returns closest point in geometry. If not found (empty geometry), returns null point nad sqrDist is negative.
*/
//TODO QGIS 3.0 - rename beforeVertex to previousVertex, afterVertex to nextVertex
QgsPoint closestVertex( const QgsPoint& point, int& atVertex, int& beforeVertex, int& afterVertex, double& sqrDist ) const;
Expand Down
1 change: 1 addition & 0 deletions src/core/geometry/qgsgeometryutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ QgsPointV2 QgsGeometryUtils::closestVertex( const QgsAbstractGeometry& geom, con
double minDist = std::numeric_limits<double>::max();
double currentDist = 0;
QgsPointV2 minDistPoint;
id = QgsVertexId(); // set as invalid

QgsVertexId vertexId;
QgsPointV2 vertex;
Expand Down
8 changes: 6 additions & 2 deletions src/core/geometry/qgsgeometryutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ class CORE_EXPORT QgsGeometryUtils
*/
static QList<QgsLineString*> extractLineStrings( const QgsAbstractGeometry* geom );

/** Returns the closest vertex to a geometry for a specified point
/** Returns the closest vertex to a geometry for a specified point.
* On error null point will be returned and "id" argument will be invalid.
*/
static QgsPointV2 closestVertex( const QgsAbstractGeometry& geom, const QgsPointV2& pt, QgsVertexId& id );

Expand Down Expand Up @@ -263,7 +264,7 @@ class CORE_EXPORT QgsGeometryUtils
for ( int i = 0; i < container.size(); ++i )
{
sqrDist = container.at( i )->closestSegment( pt, segmentPt, vertexAfter, leftOf, epsilon );
if ( sqrDist < minDist )
if ( sqrDist >= 0 && sqrDist < minDist )
{
minDist = sqrDist;
minDistSegmentX = segmentPt.x();
Expand Down Expand Up @@ -293,6 +294,9 @@ class CORE_EXPORT QgsGeometryUtils
}
}

if ( minDist == std::numeric_limits<double>::max() )
return -1; // error: no segments

segmentPt.setX( minDistSegmentX );
segmentPt.setY( minDistSegmentY );
vertexAfter = minDistVertexAfter;
Expand Down
10 changes: 2 additions & 8 deletions src/core/geometry/qgslinestring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -763,16 +763,10 @@ double QgsLineString::closestSegment( const QgsPointV2& pt, QgsPointV2& segmentP
double segmentPtX, segmentPtY;

int size = mX.size();
if ( size == 0 )
if ( size == 0 || size == 1 )
{
vertexAfter = QgsVertexId( 0, 0, 0 );
return sqrDist;
}
else if ( size == 1 )
{
segmentPt = pointN( 0 );
vertexAfter = QgsVertexId( 0, 0, 1 );
return QgsGeometryUtils::sqrDistance2D( pt, segmentPt );
return -1;
}
for ( int i = 1; i < size; ++i )
{
Expand Down
2 changes: 2 additions & 0 deletions src/core/qgspointlocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ class QgsPointLocator_VisitorNearestVertex : public IVisitor
int vertexIndex, beforeVertex, afterVertex;
double sqrDist;
QgsPoint pt = geom->closestVertex( mSrcPoint, vertexIndex, beforeVertex, afterVertex, sqrDist );
if ( sqrDist < 0 )
return; // probably empty geometry

QgsPointLocator::Match m( QgsPointLocator::Vertex, mLocator->mLayer, id, sqrt( sqrDist ), pt, vertexIndex );
// in range queries the filter may reject some matches
Expand Down
5 changes: 2 additions & 3 deletions tests/src/core/testqgsgeometry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2047,11 +2047,10 @@ void TestQgsGeometry::lineString()
//closest segment
QgsLineString l35;
bool leftOf = false;
p = QgsPointV2(); // reset all coords to zero
( void )l35.closestSegment( QgsPointV2( 1, 2 ), p, v, 0, 0 ); //empty line, just want no crash
l35.setPoints( QgsPointSequence() << QgsPointV2( 5, 10 ) );
QVERIFY( qgsDoubleNear( l35.closestSegment( QgsPointV2( 5, 10 ), p, v, 0, 0 ), 0 ) );
QCOMPARE( p, QgsPointV2( 5, 10 ) );
QCOMPARE( v, QgsVertexId( 0, 0, 1 ) );
QVERIFY( l35.closestSegment( QgsPointV2( 5, 10 ), p, v, 0, 0 ) < 0 );
l35.setPoints( QgsPointSequence() << QgsPointV2( 5, 10 ) << QgsPointV2( 10, 10 ) );
QVERIFY( qgsDoubleNear( l35.closestSegment( QgsPointV2( 4, 11 ), p, v, &leftOf, 0 ), 2.0 ) );
QCOMPARE( p, QgsPointV2( 5, 10 ) );
Expand Down
43 changes: 43 additions & 0 deletions tests/src/core/testqgspointlocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "qgsgeometry.h"
#include "qgsmaplayerregistry.h"
#include "qgspointlocator.h"
#include "qgspolygon.h"


struct FilterExcludePoint : public QgsPointLocator::MatchFilter
Expand Down Expand Up @@ -261,6 +262,48 @@ class TestQgsPointLocator : public QObject
QVERIFY( m2.isValid() );
QCOMPARE( m2.point(), QgsPoint( 1, 1 ) );
}

void testNullGeometries()
{
QgsVectorLayer* vlNullGeom = new QgsVectorLayer( "Polygon", "x", "memory" );
QgsFeature ff( 0 );
ff.setGeometry( QgsGeometry() );
QgsFeatureList flist;
flist << ff;
vlNullGeom->dataProvider()->addFeatures( flist );

QgsPointLocator loc( vlNullGeom, 0, nullptr );

QgsPointLocator::Match m1 = loc.nearestVertex( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
QVERIFY( !m1.isValid() );

QgsPointLocator::Match m2 = loc.nearestEdge( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
QVERIFY( !m2.isValid() );

delete vlNullGeom;
}

void testEmptyGeometries()
{
QgsVectorLayer* vlEmptyGeom = new QgsVectorLayer( "Polygon", "x", "memory" );
QgsFeature ff( 0 );
QgsGeometry g;
g.setGeometry( new QgsPolygonV2() );
ff.setGeometry( g );
QgsFeatureList flist;
flist << ff;
vlEmptyGeom->dataProvider()->addFeatures( flist );

QgsPointLocator loc( vlEmptyGeom, 0, nullptr );

QgsPointLocator::Match m1 = loc.nearestVertex( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
QVERIFY( !m1.isValid() );

QgsPointLocator::Match m2 = loc.nearestEdge( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
QVERIFY( !m2.isValid() );

delete vlEmptyGeom;
}
};

QTEST_MAIN( TestQgsPointLocator )
Expand Down

0 comments on commit 24fbe1a

Please sign in to comment.