From 85872b0bed9065f3dc489dfbb5569e794dd0ecf7 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 19 Nov 2024 08:44:41 +1100 Subject: [PATCH 1/8] Show actual original coordinate in QgsCsException message We were incorrectly showing the "transformed" coordinate result in the exception message, which usually ended up inf, inf after the transform fails. This lead to a useless 'forward transform of (inf, inf)' message in the exception, which was misleading as it looks like we were trying to transform a (inf, inf) point. Now we get the (useful!) actual coordinate we were trying to transform in the exception message. (cherry picked from commit c8d25f6ceff4450687c5ad29d09e3686dd3f59d4) --- src/core/proj/qgscoordinatetransform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/proj/qgscoordinatetransform.cpp b/src/core/proj/qgscoordinatetransform.cpp index 2911a4ac72eb..5076c80a2e21 100644 --- a/src/core/proj/qgscoordinatetransform.cpp +++ b/src/core/proj/qgscoordinatetransform.cpp @@ -880,7 +880,7 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * for ( int i = 0; i < numPoints; ++i ) { - points += QStringLiteral( "(%1, %2)\n" ).arg( x[i], 0, 'f' ).arg( y[i], 0, 'f' ); + points += QStringLiteral( "(%1, %2)\n" ).arg( xprev[i], 0, 'f' ).arg( yprev[i], 0, 'f' ); } const QString dir = ( direction == Qgis::TransformDirection::Forward ) ? QObject::tr( "forward transform" ) : QObject::tr( "inverse transform" ); From 69b62890b3f5c258d859b2f3c1e73a2775491eb3 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 19 Nov 2024 08:47:26 +1100 Subject: [PATCH 2/8] Don't needlessly split QgsCsException message to multilines when we don't need to (cherry picked from commit 276cbec30e9a44ce383d37fbbb6e8837b95483ee) --- src/core/proj/qgscoordinatetransform.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform.cpp b/src/core/proj/qgscoordinatetransform.cpp index 5076c80a2e21..622bbcdedc44 100644 --- a/src/core/proj/qgscoordinatetransform.cpp +++ b/src/core/proj/qgscoordinatetransform.cpp @@ -878,17 +878,17 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * //something bad happened.... QString points; + const QChar delim = numPoints > 1 ? '\n' : ' '; for ( int i = 0; i < numPoints; ++i ) { - points += QStringLiteral( "(%1, %2)\n" ).arg( xprev[i], 0, 'f' ).arg( yprev[i], 0, 'f' ); + points += QStringLiteral( "(%1, %2)" ).arg( xprev[i], 0, 'f' ).arg( yprev[i], 0, 'f' ) + delim; } - const QString dir = ( direction == Qgis::TransformDirection::Forward ) ? QObject::tr( "forward transform" ) : QObject::tr( "inverse transform" ); + const QString dir = ( direction == Qgis::TransformDirection::Forward ) ? QObject::tr( "Forward transform" ) : QObject::tr( "Inverse transform" ); - const QString msg = QObject::tr( "%1 of\n" - "%2" - "Error: %3" ) + const QString msg = QObject::tr( "%1 of%2%3Error: %4" ) .arg( dir, + QString( delim ), points, projResult < 0 ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ) ); From 4b5d2af96f7dde61fb5abed1122d4d4708d151bb Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 19 Nov 2024 08:48:04 +1100 Subject: [PATCH 3/8] Show actual proj error message in QgsCsException The old code mistakenly assumed that proj error codes were all values < 0, which they haven't been since proj 8.0 This meant all QgsCsExceptions were incorrectly including "Error: Fallback transform failed", instead of the actual error message from proj. Now we get eg "Forward transform of (-7603859.000000, -7324441.000000) Error: Invalid coordinate", which makes much more sense (cherry picked from commit 00c7268020aab2516efa49cbba43566b80cc9c82) --- src/core/proj/qgscoordinatetransform.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform.cpp b/src/core/proj/qgscoordinatetransform.cpp index 622bbcdedc44..e0c0b555c6f7 100644 --- a/src/core/proj/qgscoordinatetransform.cpp +++ b/src/core/proj/qgscoordinatetransform.cpp @@ -817,6 +817,9 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * } } + // something which won't clash with Proj's real error codes! + constexpr int PROJ_RESULT_FALLBACK_OPERATION_FAILED = -481516; + mFallbackOperationOccurred = false; if ( actualRes != 0 && ( d->mAvailableOpCount > 1 || d->mAvailableOpCount == -1 ) // only use fallbacks if more than one operation is possible -- otherwise we've already tried it and it failed @@ -845,7 +848,7 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * // hmm - something very odd here. We can't trust proj_errno( transform ), as that's giving us incorrect error numbers // (such as "failed to load datum shift file", which is definitely incorrect for a default proj created operation!) // so we resort to testing values ourselves... - projResult = std::isinf( xprev[0] ) || std::isinf( yprev[0] ) || std::isinf( zprev[0] ) ? 1 : 0; + projResult = std::isinf( xprev[0] ) || std::isinf( yprev[0] ) || std::isinf( zprev[0] ) ? PROJ_RESULT_FALLBACK_OPERATION_FAILED : 0; } if ( projResult == 0 ) @@ -890,8 +893,7 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * .arg( dir, QString( delim ), points, - projResult < 0 ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ) ); - + projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ) ); // don't flood console with thousands of duplicate transform error messages if ( msg != mLastError ) From e6ac595dc846b47a11bc3f56f9d5b7621b3e63e9 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 19 Nov 2024 08:54:54 +1100 Subject: [PATCH 4/8] Avoid deprecated proj_errno_string on proj >= 8.0 (cherry picked from commit 16de21a78ec15fa3c9f2add0829a81c57874b33c) --- src/core/proj/qgscoordinatereferencesystem.cpp | 4 ++++ src/core/proj/qgscoordinatetransform.cpp | 9 ++++++++- src/core/proj/qgscoordinatetransform_p.cpp | 8 ++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/core/proj/qgscoordinatereferencesystem.cpp b/src/core/proj/qgscoordinatereferencesystem.cpp index ecd4acd143cf..d87005c0995f 100644 --- a/src/core/proj/qgscoordinatereferencesystem.cpp +++ b/src/core/proj/qgscoordinatereferencesystem.cpp @@ -1627,7 +1627,11 @@ void QgsCoordinateReferenceSystem::setProjString( const QString &proj4String ) { #ifdef QGISDEBUG const int errNo = proj_context_errno( ctx ); +#if PROJ_VERSION_MAJOR>=8 + QgsDebugError( QStringLiteral( "proj string rejected: %1" ).arg( proj_context_errno_string( ctx, errNo ) ) ); +#else QgsDebugError( QStringLiteral( "proj string rejected: %1" ).arg( proj_errno_string( errNo ) ) ); +#endif #endif d->mIsValid = false; } diff --git a/src/core/proj/qgscoordinatetransform.cpp b/src/core/proj/qgscoordinatetransform.cpp index e0c0b555c6f7..1124a31a67a7 100644 --- a/src/core/proj/qgscoordinatetransform.cpp +++ b/src/core/proj/qgscoordinatetransform.cpp @@ -889,11 +889,18 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * const QString dir = ( direction == Qgis::TransformDirection::Forward ) ? QObject::tr( "Forward transform" ) : QObject::tr( "Inverse transform" ); +#if PROJ_VERSION_MAJOR>=8 + PJ_CONTEXT *projContext = QgsProjContext::get(); + const QString projError = projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_context_errno_string( projContext, projResult ) ) : QObject::tr( "Fallback transform failed" ); +#else + const QString projError = projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ); +#endif + const QString msg = QObject::tr( "%1 of%2%3Error: %4" ) .arg( dir, QString( delim ), points, - projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ) ); + projError ); // don't flood console with thousands of duplicate transform error messages if ( msg != mLastError ) diff --git a/src/core/proj/qgscoordinatetransform_p.cpp b/src/core/proj/qgscoordinatetransform_p.cpp index 789d71789ed8..226a813848c1 100644 --- a/src/core/proj/qgscoordinatetransform_p.cpp +++ b/src/core/proj/qgscoordinatetransform_p.cpp @@ -331,7 +331,11 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() const int errNo = proj_context_errno( context ); if ( errNo && errNo != -61 ) { +#if PROJ_VERSION_MAJOR>=8 + nonAvailableError = QString( proj_context_errno_string( context, errNo ) ); +#else nonAvailableError = QString( proj_errno_string( errNo ) ); +#endif } else { @@ -465,7 +469,11 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() const QStringList projErrors = errorLogger.errors(); if ( errNo && errNo != -61 ) { +#if PROJ_VERSION_MAJOR>=8 + nonAvailableError = QString( proj_context_errno_string( context, errNo ) ); +#else nonAvailableError = QString( proj_errno_string( errNo ) ); +#endif } else if ( !projErrors.empty() ) { From e32ea96283ed4675acd8420775f58f21a5f4ad8b Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 19 Nov 2024 08:59:03 +1100 Subject: [PATCH 5/8] Include crs IDs in QgsCsException message Eg: Forward transform (EPSG:4326 to EPSG:3857) of (-7603859.000000, -7324441.000000) Error: Invalid coordinate (cherry picked from commit 9b69cafa16b775611515c45762ab378c398b2840) --- src/core/proj/qgscoordinatetransform.cpp | 4 +++- .../src/python/test_qgscoordinatetransform.py | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/core/proj/qgscoordinatetransform.cpp b/src/core/proj/qgscoordinatetransform.cpp index 1124a31a67a7..1345cda69ca3 100644 --- a/src/core/proj/qgscoordinatetransform.cpp +++ b/src/core/proj/qgscoordinatetransform.cpp @@ -896,8 +896,10 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * const QString projError = projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ); #endif - const QString msg = QObject::tr( "%1 of%2%3Error: %4" ) + const QString msg = QObject::tr( "%1 (%2 to %3) of%4%5Error: %6" ) .arg( dir, + ( direction == Qgis::TransformDirection::Forward ) ? d->mSourceCRS.authid() : d->mDestCRS.authid(), + ( direction == Qgis::TransformDirection::Forward ) ? d->mDestCRS.authid() : d->mSourceCRS.authid(), QString( delim ), points, projError ); diff --git a/tests/src/python/test_qgscoordinatetransform.py b/tests/src/python/test_qgscoordinatetransform.py index 24514f3f6f42..7a87fac0e117 100644 --- a/tests/src/python/test_qgscoordinatetransform.py +++ b/tests/src/python/test_qgscoordinatetransform.py @@ -17,6 +17,8 @@ QgsCoordinateTransformContext, QgsProject, QgsRectangle, + QgsPointXY, + QgsCsException ) import unittest from qgis.testing import start_app, QgisTestCase @@ -162,6 +164,22 @@ def testTransformBoundingBoxFullWorldToWebMercator(self): self.assertAlmostEqual(transformedExtent.xMaximum(), 20037508.343, delta=1e-3) self.assertAlmostEqual(transformedExtent.yMaximum(), 44927335.427, delta=1e-3) + def test_cs_exception(self): + ct = QgsCoordinateTransform(QgsCoordinateReferenceSystem('EPSG:4326'), + QgsCoordinateReferenceSystem('EPSG:3857'), QgsProject.instance()) + point = QgsPointXY(-7603859, -7324441) + with self.assertRaises(QgsCsException) as e: + ct.transform(point) + self.assertEqual(str(e.exception), 'Forward transform (EPSG:4326 to EPSG:3857) of (-7603859.000000, -7324441.000000) Error: Invalid coordinate') + + # reverse transform + ct = QgsCoordinateTransform(QgsCoordinateReferenceSystem('EPSG:3857'), + QgsCoordinateReferenceSystem('EPSG:4326'), QgsProject.instance()) + point = QgsPointXY(-7603859, -7324441) + with self.assertRaises(QgsCsException) as e: + ct.transform(point, Qgis.TransformDirection.Reverse) + self.assertEqual(str(e.exception), 'Inverse transform (EPSG:4326 to EPSG:3857) of (-7603859.000000, -7324441.000000) Error: Invalid coordinate') + if __name__ == '__main__': unittest.main() From ec9e285de703ee866b1a06215056bec78d90f1fa Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 19 Nov 2024 09:11:55 +1000 Subject: [PATCH 6/8] Remove hardcoded proj error numbers (cherry picked from commit b682d36b06f20065994b0707f1306a6c295b8790) --- src/core/proj/qgscoordinatetransform_p.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform_p.cpp b/src/core/proj/qgscoordinatetransform_p.cpp index 226a813848c1..6b949077108d 100644 --- a/src/core/proj/qgscoordinatetransform_p.cpp +++ b/src/core/proj/qgscoordinatetransform_p.cpp @@ -329,7 +329,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() { // huh? const int errNo = proj_context_errno( context ); - if ( errNo && errNo != -61 ) + if ( errNo ) { #if PROJ_VERSION_MAJOR>=8 nonAvailableError = QString( proj_context_errno_string( context, errNo ) ); @@ -339,6 +339,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() } else { + // in theory should never be hit! nonAvailableError = QObject::tr( "No coordinate operations are available between these two reference systems" ); } } @@ -467,7 +468,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() { const int errNo = proj_context_errno( context ); const QStringList projErrors = errorLogger.errors(); - if ( errNo && errNo != -61 ) + if ( errNo ) { #if PROJ_VERSION_MAJOR>=8 nonAvailableError = QString( proj_context_errno_string( context, errNo ) ); From bbb91748436352185e6a5e86149faccf85e97d76 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 20 Nov 2024 06:24:58 +1000 Subject: [PATCH 7/8] Use bool to track errors from fallback operations (cherry picked from commit 5733e3d3f2cd60942356f2a6f586fdbcf3eaea19) --- src/core/proj/qgscoordinatetransform.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform.cpp b/src/core/proj/qgscoordinatetransform.cpp index 1345cda69ca3..29bc5f89a1be 100644 --- a/src/core/proj/qgscoordinatetransform.cpp +++ b/src/core/proj/qgscoordinatetransform.cpp @@ -817,10 +817,8 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * } } - // something which won't clash with Proj's real error codes! - constexpr int PROJ_RESULT_FALLBACK_OPERATION_FAILED = -481516; - mFallbackOperationOccurred = false; + bool errorOccurredDuringFallbackOperation = false; if ( actualRes != 0 && ( d->mAvailableOpCount > 1 || d->mAvailableOpCount == -1 ) // only use fallbacks if more than one operation is possible -- otherwise we've already tried it and it failed && ( d->mAllowFallbackTransforms || mBallparkTransformsAreAppropriate ) ) @@ -845,13 +843,14 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * // So here just check proj_errno() for single point transform if ( numPoints == 1 ) { + projResult = proj_errno( transform ); // hmm - something very odd here. We can't trust proj_errno( transform ), as that's giving us incorrect error numbers // (such as "failed to load datum shift file", which is definitely incorrect for a default proj created operation!) // so we resort to testing values ourselves... - projResult = std::isinf( xprev[0] ) || std::isinf( yprev[0] ) || std::isinf( zprev[0] ) ? PROJ_RESULT_FALLBACK_OPERATION_FAILED : 0; + errorOccurredDuringFallbackOperation = std::isinf( xprev[0] ) || std::isinf( yprev[0] ) || std::isinf( zprev[0] ); } - if ( projResult == 0 ) + if ( !errorOccurredDuringFallbackOperation ) { memcpy( x, xprev.data(), sizeof( double ) * numPoints ); memcpy( y, yprev.data(), sizeof( double ) * numPoints ); @@ -876,7 +875,7 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * z[pos] = std::numeric_limits::quiet_NaN(); } - if ( projResult != 0 ) + if ( projResult != 0 || errorOccurredDuringFallbackOperation ) { //something bad happened.... QString points; @@ -891,9 +890,9 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double * #if PROJ_VERSION_MAJOR>=8 PJ_CONTEXT *projContext = QgsProjContext::get(); - const QString projError = projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_context_errno_string( projContext, projResult ) ) : QObject::tr( "Fallback transform failed" ); + const QString projError = !errorOccurredDuringFallbackOperation ? QString::fromUtf8( proj_context_errno_string( projContext, projResult ) ) : QObject::tr( "Fallback transform failed" ); #else - const QString projError = projResult != PROJ_RESULT_FALLBACK_OPERATION_FAILED ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ); + const QString projError = !errorOccurredDuringFallbackOperation ? QString::fromUtf8( proj_errno_string( projResult ) ) : QObject::tr( "Fallback transform failed" ); #endif const QString msg = QObject::tr( "%1 (%2 to %3) of%4%5Error: %6" ) From 4e235c2c36a82b1dd530dc0aff3faad406448eee Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 21 Nov 2024 12:48:14 +1000 Subject: [PATCH 8/8] Fix test --- tests/src/python/test_qgscoordinatetransform.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/src/python/test_qgscoordinatetransform.py b/tests/src/python/test_qgscoordinatetransform.py index 7a87fac0e117..a2e70523eefa 100644 --- a/tests/src/python/test_qgscoordinatetransform.py +++ b/tests/src/python/test_qgscoordinatetransform.py @@ -12,6 +12,7 @@ import qgis # NOQA from qgis.core import ( + Qgis, QgsCoordinateReferenceSystem, QgsCoordinateTransform, QgsCoordinateTransformContext,