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
  • Loading branch information
wonder-sk committed Oct 7, 2016
1 parent 706431e commit c093d51
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 @@ -179,7 +179,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 @@ -272,12 +272,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/qgscircularstringv2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,9 @@ double QgsCircularStringV2::closestSegment( const QgsPointV2& pt, QgsPointV2& se
}
}

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/qgscurvepolygonv2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,7 @@ double QgsCurvePolygonV2::closestSegment( const QgsPointV2& pt, QgsPointV2& segm
{
if ( !mExteriorRing )
{
return 0.0;
return -1;
}
QList<QgsCurveV2*> 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 @@ -347,6 +347,7 @@ QgsPoint QgsGeometry::closestVertex( const QgsPoint& point, int& atVertex, int&
{
if ( !d->geometry )
{
sqrDist = -1;
return QgsPoint( 0, 0 );
}

Expand Down Expand Up @@ -587,12 +588,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 @@ -606,14 +609,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 @@ -221,7 +221,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 QgsAbstractGeometryV2& geom, c
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<QgsLineStringV2*> extractLineStrings( const QgsAbstractGeometryV2* 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 QgsAbstractGeometryV2& geom, const QgsPointV2& pt, QgsVertexId& id );

Expand Down Expand Up @@ -250,7 +251,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 @@ -280,6 +281,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/qgslinestringv2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -747,16 +747,10 @@ double QgsLineStringV2::closestSegment( const QgsPointV2& pt, QgsPointV2& segmen
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 @@ -104,6 +104,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 @@ -2038,11 +2038,10 @@ void TestQgsGeometry::lineStringV2()
//closest segment
QgsLineStringV2 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( QgsPointSequenceV2() << 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( QgsPointSequenceV2() << 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 "qgspolygonv2.h"


struct FilterExcludePoint : public QgsPointLocator::MatchFilter
Expand Down Expand Up @@ -259,6 +260,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 c093d51

Please sign in to comment.