Use an exact test when checking if a curve is closed

Otherwise we get cases where calling isClosed() on a curve with small (<1E-08) differences between the first and last vertex incorrectly returns true. This is an issue when converting rings to GEOS, as it rejects them as being unclosed, yet calling QgsCurve::close on them has no effect... This is the underlying root cause of #36346. In this particular case the calculation of the bounds of the marker symbol (used for the label obstacle calculation) results in a very small rectangle (since it's in map (geographic) units, and the map is very zoomed in) -- so the 1E-08 tolerance here is completely unsuitable and calling close() on the marker bounds had no effect, ultimately resulting in an unclosed ring being passed to GEOS. Fixes #36346
qgis · Jun 2, 2020 · 3aa5e56 · 3aa5e56
1 parent df41d78
commit 3aa5e56
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 3 deletions.
diff --git a/src/core/geometry/qgscurve.cpp b/src/core/geometry/qgscurve.cpp
@@ -46,10 +46,10 @@ bool QgsCurve::isClosed() const
   QgsPoint start = startPoint();
   QgsPoint end = endPoint();
 
-  bool closed = qgsDoubleNear( start.x(), end.x(), 1E-8 ) &&
-                qgsDoubleNear( start.y(), end.y(), 1E-8 );
+  bool closed = qgsDoubleNear( start.x(), end.x() ) &&
+                qgsDoubleNear( start.y(), end.y() );
   if ( is3D() && closed )
-    closed &= qgsDoubleNear( start.z(), end.z(), 1E-8 ) || ( std::isnan( start.z() ) && std::isnan( end.z() ) );
+    closed &= qgsDoubleNear( start.z(), end.z() ) || ( std::isnan( start.z() ) && std::isnan( end.z() ) );
   return closed;
 }
 

diff --git a/tests/src/core/testqgsgeometry.cpp b/tests/src/core/testqgsgeometry.cpp
@@ -3597,6 +3597,19 @@ void TestQgsGeometry::lineString()
   QVERIFY( l11.isClosed() );
   QCOMPARE( l11.numPoints(), 5 );
   QCOMPARE( l11.pointN( 4 ), QgsPoint( 1, 2 ) );
+
+  // tiny differences
+  l11.setPoints( QgsPointSequence() << QgsPoint( 0.000000000000001, 0.000000000000002 )
+                 << QgsPoint( 0.000000000000011, 0.000000000000002 )
+                 << QgsPoint( 0.000000000000011, 0.000000000000022 )
+                 << QgsPoint( 0.000000000000001, 0.000000000000022 ) );
+  QVERIFY( !l11.isClosed() );
+  l11.close();
+  QVERIFY( l11.isClosed() );
+  QCOMPARE( l11.numPoints(), 5 );
+  QGSCOMPARENEAR( l11.pointN( 4 ).x(), 0.000000000000001, 0.00000000000000001 );
+  QGSCOMPARENEAR( l11.pointN( 4 ).y(), 0.000000000000002, 0.00000000000000001 );
+
   //test that m values aren't considered when testing for closedness
   l11.setPoints( QgsPointSequence() << QgsPoint( QgsWkbTypes::PointM, 1, 2, 0, 3 )
                  << QgsPoint( QgsWkbTypes::PointM, 11, 2, 0, 4 )