From 61e7a5fc90154bd4d35fea7d6cbd413ce61fa02e Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 10 Jun 2020 12:49:37 +1000 Subject: [PATCH] When scaling geometries for tesselation, ensure we don't change the geometry's aspect ratio If we scale by an uneven amount in the x vs y plane, then the resultant tesselation uses a misrepresentation of the actual shape of the geometry, resulting in a poor quality tesselation. Follow up 8ee1c20b Fixes #37077 --- src/core/qgstessellator.cpp | 25 ++++++++++++------------- tests/src/3d/testqgstessellator.cpp | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/core/qgstessellator.cpp b/src/core/qgstessellator.cpp index 1925d465b97b..9b4dbfa27957 100644 --- a/src/core/qgstessellator.cpp +++ b/src/core/qgstessellator.cpp @@ -268,7 +268,7 @@ inline double _round_coord( double x ) } -static QgsCurve *_transform_ring_to_new_base( const QgsLineString &curve, const QgsPoint &pt0, const QMatrix4x4 *toNewBase, float scaleX, float scaleY ) +static QgsCurve *_transform_ring_to_new_base( const QgsLineString &curve, const QgsPoint &pt0, const QMatrix4x4 *toNewBase, const float scale ) { int count = curve.numPoints(); QVector x; @@ -295,8 +295,8 @@ static QgsCurve *_transform_ring_to_new_base( const QgsLineString &curve, const v = toNewBase->map( v ); // scale coordinates - v.setX( v.x() * scaleX ); - v.setY( v.y() * scaleY ); + v.setX( v.x() * scale ); + v.setY( v.y() * scale ); // we also round coordinates before passing them to poly2tri triangulation in order to fix possible numerical // stability issues. We had crashes with nearly collinear points where one of the points was off by a tiny bit (e.g. by 1e-20). @@ -316,12 +316,12 @@ static QgsCurve *_transform_ring_to_new_base( const QgsLineString &curve, const } -static QgsPolygon *_transform_polygon_to_new_base( const QgsPolygon &polygon, const QgsPoint &pt0, const QMatrix4x4 *toNewBase, float scaleX, float scaleY ) +static QgsPolygon *_transform_polygon_to_new_base( const QgsPolygon &polygon, const QgsPoint &pt0, const QMatrix4x4 *toNewBase, const float scale ) { QgsPolygon *p = new QgsPolygon; - p->setExteriorRing( _transform_ring_to_new_base( *qgsgeometry_cast< const QgsLineString * >( polygon.exteriorRing() ), pt0, toNewBase, scaleX, scaleY ) ); + p->setExteriorRing( _transform_ring_to_new_base( *qgsgeometry_cast< const QgsLineString * >( polygon.exteriorRing() ), pt0, toNewBase, scale ) ); for ( int i = 0; i < polygon.numInteriorRings(); ++i ) - p->addInteriorRing( _transform_ring_to_new_base( *qgsgeometry_cast< const QgsLineString * >( polygon.interiorRing( i ) ), pt0, toNewBase, scaleX, scaleY ) ); + p->addInteriorRing( _transform_ring_to_new_base( *qgsgeometry_cast< const QgsLineString * >( polygon.interiorRing( i ) ), pt0, toNewBase, scale ) ); return p; } @@ -494,13 +494,12 @@ void QgsTessellator::addPolygon( const QgsPolygon &polygon, float extrusionHeigh const QgsPoint ptStart( exterior->startPoint() ); const QgsPoint pt0( QgsWkbTypes::PointZ, ptStart.x(), ptStart.y(), std::isnan( ptStart.z() ) ? 0 : ptStart.z() ); - const float scaleX = !mBounds.isNull() ? 10000.0 / mBounds.width() : 1.0; - const float scaleY = !mBounds.isNull() ? 10000.0 / mBounds.height() : 1.0; + const float scale = mBounds.isNull() ? 1.0 : std::max( 10000.0 / mBounds.width(), 10000.0 / mBounds.height() ); // subtract ptFirst from geometry for better numerical stability in triangulation // and apply new 3D vector base if the polygon is not horizontal - std::unique_ptr polygonNew( _transform_polygon_to_new_base( polygon, pt0, toNewBase.get(), scaleX, scaleY ) ); + std::unique_ptr polygonNew( _transform_polygon_to_new_base( polygon, pt0, toNewBase.get(), scale ) ); if ( _minimum_distance_between_coordinates( *polygonNew ) < 0.001 ) { @@ -574,8 +573,8 @@ void QgsTessellator::addPolygon( const QgsPolygon &polygon, float extrusionHeigh QVector4D pt( p->x, p->y, mNoZ ? 0 : z[p], 0 ); if ( toOldBase ) pt = *toOldBase * pt; - const double fx = ( pt.x() / scaleX ) - mOriginX + pt0.x(); - const double fy = ( pt.y() / scaleY ) - mOriginY + pt0.y(); + const double fx = ( pt.x() / scale ) - mOriginX + pt0.x(); + const double fy = ( pt.y() / scale ) - mOriginY + pt0.y(); const double fz = mNoZ ? 0 : ( pt.z() + extrusionHeight + pt0.z() ); if ( fz < zMin ) zMin = fz; @@ -596,8 +595,8 @@ void QgsTessellator::addPolygon( const QgsPolygon &polygon, float extrusionHeigh QVector4D pt( p->x, p->y, mNoZ ? 0 : z[p], 0 ); if ( toOldBase ) pt = *toOldBase * pt; - const double fx = ( pt.x() / scaleX ) - mOriginX + pt0.x(); - const double fy = ( pt.y() / scaleY ) - mOriginY + pt0.y(); + const double fx = ( pt.x() / scale ) - mOriginX + pt0.x(); + const double fy = ( pt.y() / scale ) - mOriginY + pt0.y(); const double fz = mNoZ ? 0 : ( pt.z() + extrusionHeight + pt0.z() ); mData << fx << fz << -fy; if ( mAddNormals ) diff --git a/tests/src/3d/testqgstessellator.cpp b/tests/src/3d/testqgstessellator.cpp index 1448bfe06753..f91479aaaa3b 100644 --- a/tests/src/3d/testqgstessellator.cpp +++ b/tests/src/3d/testqgstessellator.cpp @@ -21,6 +21,8 @@ #include "qgspolygon.h" #include "qgstessellator.h" #include "qgsmultipolygon.h" +#include "qgslogger.h" +#include "qgsgeometry.h" static bool qgsVectorNear( const QVector3D &v1, const QVector3D &v2, double eps ) { @@ -140,6 +142,7 @@ class TestQgsTessellator : public QObject void testNoZ(); void testTriangulationDoesNotCrash(); void testCrash2DTriangle(); + void narrowPolygon(); private: }; @@ -407,5 +410,19 @@ void TestQgsTessellator::testCrash2DTriangle() t.addPolygon( polygon, 0 ); // must not crash - that's all we test here } +void TestQgsTessellator::narrowPolygon() +{ + // test that tesselation of a long narrow polygon results in a "nice" tesselation (i.e. not lots of long thin triangles) + // refs https://github.com/qgis/QGIS/issues/37077 + QgsPolygon polygon; + polygon.fromWkt( "Polygon ((383393.53728186257649213 4902093.79335568379610777, 383383.73728171654511243 4902092.99335567187517881, 383375.25399118528002873 4902092.8891992112621665, 383368.08741026872303337 4902093.48088630195707083, 383362.87084332120139152 4902093.91129046399146318, 383359.60429034277331084 4902094.18041169829666615, 383357.23274148383643478 4902093.6530067715793848, 383355.75619674433255568 4902092.32907568290829659, 383355.57501344417687505 4902090.56084021460264921, 383356.68919158342760056 4902088.34830036386847496, 383361.07830215193098411 4902086.48156050778925419, 383368.74234514962881804 4902084.96062064450234175, 383380.44909519288921729 4902084.10479906108230352, 383396.19855228182859719 4902083.91409575659781694, 383406.97328086948255077 4902084.21874411031603813, 383412.77328095590928569 4902085.01874412223696709, 383416.70496230950811878 4902086.31405469868332148, 383418.76832493022084236 4902088.1046758396551013, 383419.69647055567475036 4902089.60464367642998695, 383419.48939918575342745 4902090.81395820714533329, 383418.40130056580528617 4902092.01381655503064394, 383416.43217469577211887 4902093.20421871729195118, 383411.19502930447924882 4902093.89790377207100391, 383402.68986439192667603 4902094.09487171657383442, 383393.53728186257649213 4902093.79335568379610777))" ); + QgsTessellator t( polygon.boundingBox(), false ); + t.addPolygon( polygon, 0 ); + QgsGeometry res( t.asMultiPolygon() ); + res.translate( polygon.boundingBox().xMinimum(), polygon.boundingBox().yMinimum() ); + QgsDebugMsg( res.asWkt( 0 ) ); + QCOMPARE( res.asWkt( 0 ), QStringLiteral( "MultiPolygonZ (((383357 4902094 0, 383356 4902092 0, 383356 4902091 0, 383357 4902094 0)),((383357 4902088 0, 383357 4902094 0, 383356 4902091 0, 383357 4902088 0)),((383357 4902088 0, 383361 4902086 0, 383357 4902094 0, 383357 4902088 0)),((383357 4902094 0, 383361 4902086 0, 383360 4902094 0, 383357 4902094 0)),((383363 4902094 0, 383360 4902094 0, 383361 4902086 0, 383363 4902094 0)),((383368 4902093 0, 383363 4902094 0, 383361 4902086 0, 383368 4902093 0)),((383368 4902093 0, 383361 4902086 0, 383369 4902085 0, 383368 4902093 0)),((383368 4902093 0, 383369 4902085 0, 383375 4902093 0, 383368 4902093 0)),((383375 4902093 0, 383369 4902085 0, 383380 4902084 0, 383375 4902093 0)),((383375 4902093 0, 383380 4902084 0, 383384 4902093 0, 383375 4902093 0)),((383384 4902093 0, 383380 4902084 0, 383396 4902084 0, 383384 4902093 0)),((383394 4902094 0, 383384 4902093 0, 383396 4902084 0, 383394 4902094 0)),((383403 4902094 0, 383394 4902094 0, 383396 4902084 0, 383403 4902094 0)),((383396 4902084 0, 383407 4902084 0, 383403 4902094 0, 383396 4902084 0)),((383411 4902094 0, 383403 4902094 0, 383407 4902084 0, 383411 4902094 0)),((383411 4902094 0, 383407 4902084 0, 383413 4902085 0, 383411 4902094 0)),((383411 4902094 0, 383413 4902085 0, 383416 4902093 0, 383411 4902094 0)),((383416 4902093 0, 383413 4902085 0, 383417 4902086 0, 383416 4902093 0)),((383419 4902088 0, 383416 4902093 0, 383417 4902086 0, 383419 4902088 0)),((383418 4902092 0, 383416 4902093 0, 383419 4902088 0, 383418 4902092 0)),((383418 4902092 0, 383419 4902088 0, 383419 4902091 0, 383418 4902092 0)),((383419 4902091 0, 383419 4902088 0, 383420 4902090 0, 383419 4902091 0)))" ) ); +} + QGSTEST_MAIN( TestQgsTessellator ) #include "testqgstessellator.moc"