When calling QgsGeorefTransform::updateParametersFromGcps the source …

…points must ALWAYS be in source layer coordinates
qgis · Feb 9, 2022 · d5575b6 · d5575b6
1 parent 9acb8ec
commit d5575b6
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 25 deletions.
diff --git a/src/app/georeferencer/qgsgcplist.cpp b/src/app/georeferencer/qgsgcplist.cpp
@@ -33,10 +33,10 @@ QgsGCPList::QgsGCPList( const QgsGCPList &list )
   }
 }
 
-void QgsGCPList::createGCPVectors( QVector<QgsPointXY> &mapCoords, QVector<QgsPointXY> &pixelCoords, const QgsCoordinateReferenceSystem &targetCrs )
+void QgsGCPList::createGCPVectors( QVector<QgsPointXY> &sourceCoordinates, QVector<QgsPointXY> &destinationCoordinates, const QgsCoordinateReferenceSystem &targetCrs )
 {
-  mapCoords   = QVector<QgsPointXY>( size() );
-  pixelCoords = QVector<QgsPointXY>( size() );
+  sourceCoordinates = QVector<QgsPointXY>( size() );
+  destinationCoordinates = QVector<QgsPointXY>( size() );
   QgsPointXY transCoords;
   for ( int i = 0, j = 0; i < sizeAll(); i++ )
   {
@@ -49,19 +49,18 @@ void QgsGCPList::createGCPVectors( QVector<QgsPointXY> &mapCoords, QVector<QgsPo
         {
           transCoords = QgsCoordinateTransform( pt->destinationCrs(), targetCrs,
                                                 QgsProject::instance() ).transform( pt->destinationMapCoords() );
-          mapCoords[j] = transCoords;
+          destinationCoordinates[j] = transCoords;
           pt->setTransCoords( transCoords );
         }
         catch ( const QgsException & )
         {
-          mapCoords[j] = pt->destinationMapCoords();
+          destinationCoordinates[j] = pt->destinationMapCoords();
         }
       }
       else
-        mapCoords[j] = pt->destinationMapCoords();
+        destinationCoordinates[j] = pt->destinationMapCoords();
 
-      // TODO -- this must be converted to pixels!!!
-      pixelCoords[j] = pt->sourceCoords();
+      sourceCoordinates[j] = pt->sourceCoords();
       j++;
     }
   }

diff --git a/src/app/georeferencer/qgsgcplist.h b/src/app/georeferencer/qgsgcplist.h
@@ -30,7 +30,7 @@ class QgsGCPList : public QList<QgsGeorefDataPoint *>
     QgsGCPList() = default;
     QgsGCPList( const QgsGCPList &list );
 
-    void createGCPVectors( QVector<QgsPointXY> &mapCoords, QVector<QgsPointXY> &pixelCoords, const QgsCoordinateReferenceSystem &targetCrs );
+    void createGCPVectors( QVector<QgsPointXY> &sourceCoordinates, QVector<QgsPointXY> &destinationCoordinates, const QgsCoordinateReferenceSystem &targetCrs );
     int size() const;
     int sizeAll() const;
 

diff --git a/src/app/georeferencer/qgsgcplistmodel.cpp b/src/app/georeferencer/qgsgcplistmodel.cpp
@@ -82,14 +82,15 @@ void QgsGCPListModel::updateModel()
   QString unitType;
   const QgsSettings s;
   bool mapUnitsPossible = false;
-  QVector<QgsPointXY> mapCoords, pixelCoords;
+  QVector<QgsPointXY> sourceCoordinates;
+  QVector<QgsPointXY> destinationCoordinates;
 
-  mGCPList->createGCPVectors( mapCoords, pixelCoords,
+  mGCPList->createGCPVectors( sourceCoordinates, destinationCoordinates,
                               QgsCoordinateReferenceSystem( s.value( QStringLiteral( "/Plugin-GeoReferencer/targetsrs" ) ).toString() ) );
 
   if ( mGeorefTransform )
   {
-    bTransformUpdated = mGeorefTransform->updateParametersFromGcps( pixelCoords, mapCoords, true );
+    bTransformUpdated = mGeorefTransform->updateParametersFromGcps( sourceCoordinates, destinationCoordinates, true );
     mapUnitsPossible = mGeorefTransform->providesAccurateInverseTransformation();
   }
 

diff --git a/src/app/georeferencer/qgsgeorefmainwindow.cpp b/src/app/georeferencer/qgsgeorefmainwindow.cpp
@@ -2000,14 +2000,15 @@ bool QgsGeoreferencerMainWindow::checkReadyGeoref()
 
 bool QgsGeoreferencerMainWindow::updateGeorefTransform()
 {
-  QVector<QgsPointXY> mapCoords, pixelCoords;
+  QVector<QgsPointXY> sourceCoordinates;
+  QVector<QgsPointXY> destinationCoords;
   if ( mGCPListWidget->gcpList() )
-    mGCPListWidget->gcpList()->createGCPVectors( mapCoords, pixelCoords, mProjection );
+    mGCPListWidget->gcpList()->createGCPVectors( sourceCoordinates, destinationCoords, mProjection );
   else
     return false;
 
   // Parametrize the transform with GCPs
-  if ( !mGeorefTransform.updateParametersFromGcps( pixelCoords, mapCoords, true ) )
+  if ( !mGeorefTransform.updateParametersFromGcps( sourceCoordinates, destinationCoords, true ) )
   {
     return false;
   }

diff --git a/src/app/georeferencer/qgsgeoreftransform.cpp b/src/app/georeferencer/qgsgeoreftransform.cpp
@@ -102,8 +102,8 @@ bool QgsGeorefTransform::updateParametersFromGcps( const QVector<QgsPointXY> &so
   }
   if ( mRasterChangeCoords.hasExistingGeoreference() )
   {
-    const QVector<QgsPointXY> pixelCoordsCorrect = mRasterChangeCoords.getPixelCoords( sourceCoordinates );
-    mParametersInitialized = mGeorefTransformImplementation->updateParametersFromGcps( sourceCoordinates, pixelCoordsCorrect, invertYAxis );
+    const QVector<QgsPointXY> sourcePixelCoordinates = mRasterChangeCoords.getPixelCoords( sourceCoordinates );
+    mParametersInitialized = mGeorefTransformImplementation->updateParametersFromGcps( sourcePixelCoordinates, destinationCoordinates, invertYAxis );
   }
   else
   {

diff --git a/tests/src/app/testqgsgeoreferencer.cpp b/tests/src/app/testqgsgeoreferencer.cpp
@@ -184,13 +184,7 @@ void TestQgsGeoreferencer::testTransformImageWithExistingGeoreference()
 
   QVERIFY( transform.hasExistingGeoreference() );
 
-  // currently disabled -- it is ambiguous whether calling updateParametersFromGcps should be using PIXEL coordinates as source or layer CRS coordinates, and is quite broken either
-  // way. Here the disabled tests assume layer CRS coordinates, but in the actual georeferencer a mix of pixel/layer coordinates are used.
-  // it would be better if all calls to updateParametersFromGcps ALWAYS use pixel coordinates to avoid the confusion, with the caller transforming from layer CRS coordinates
-  // back to pixel coordinates before calling this method.
-#if 0
-  // source coordinates here should be raster CRS - ie. 32633
-  // first use a "null" transform
+  // when calling updateParametersFromGcps the source list MUST be in source layer CRS, not pixels!
   QVERIFY( transform.updateParametersFromGcps( {QgsPointXY( 783414, 3350122 ), QgsPointXY( 791344, 3349795 ), QgsPointXY( 783077, 334093 ), QgsPointXY( 791134, 3341401 )},
   {QgsPointXY( 783414, 3350122 ), QgsPointXY( 791344, 3349795 ), QgsPointXY( 783077, 334093 ), QgsPointXY( 791134, 3341401 )}, true ) );
 
@@ -225,7 +219,6 @@ void TestQgsGeoreferencer::testTransformImageWithExistingGeoreference()
   QVERIFY( transform.transform( QgsPointXY( 791234, 3341601 ), res, false ) );
   QGSCOMPARENEAR( res.x(), 166.168859649, 0.1 );
   QGSCOMPARENEAR( res.y(), -167.0548245614, 0.1 );
-#endif
 }
 
 void TestQgsGeoreferencer::testRasterChangeCoords()