Bug report #15829: georeferencer - logical error during loadGCPs - QGIS Application - QGIS Issue Tracking

Bug report #15829

georeferencer - logical error during loadGCPs

Added by Mark Johnson over 7 years ago. Updated over 7 years ago.

Status:	Closed
Priority:	High
Assignee:	-
Category:	C++ plugins/Georeferencer
Affected QGIS version:	2.18.0	Regression?:	No
Operating System:		Easy fix?:	No
Pull Request or Patch supplied:	No	Resolution:
Crashes QGIS or corrupts data:	No	Copied to github as #:	23749

Description

While working on the georeferencer, I have noticed a logical error during the function 'loadGCPs'.

When the .points file is being read, after reading each point, the function 'addPoint' is called.

'addPoint'
- creates a new QgsGeorefDataPoint
- creates a SIGNAL for the QgsGeorefDataPoint
- appends it to 'mPoints'

and then calls mGCPListWidget->setGCPList( &mPoints );

which will (re-)build the QTableView with any needed calculations.

In a case where there are 892 points
- 'setGCPList' is called 892 times and takes a good minute to load

When this is corrected to call 'setGCPList' after the building of 'mPoints' has been completed
- the loading takes a few seconds, since the QTableView is only being build once

Since the QTableView is not needed during the reading of the .points file
- 'setGCPList' should only be called once

Associated revisions

Revision 59a0e2fb
Added by Mark Johnson over 7 years ago

Fix #15829 georeferencer - resolve logical error during loadGCPs

History

#1 Updated by Mark Johnson over 7 years ago

This could then be something in the form of:

bool QgsGeorefPluginGui::loadGCPs( /*bool verbose*/ )
{
  QFile pointFile( mGCPpointsFileName );
  if ( !pointFile.open( QIODevice::ReadOnly ) )
  {
    return false;
  }

  clearGCPData();

  QTextStream points( &pointFile );
  QString line = points.readLine();
  while ( !points.atEnd() )
  {
 ...
     QgsPoint mapCoords( ls.at( 0 ).toDouble(), ls.at( 1 ).toDouble() ); // map x,y
     QgsPoint pixelCoords( ls.at( 2 ).toDouble(), ls.at( 3 ).toDouble() ); // pixel x,y
     QgsGeorefDataPoint* pnt;
     if ( ls.count() == 5 )
     {
      bool enable = ls.at( 4 ).toInt();
      pnt = new QgsGeorefDataPoint( mCanvas, mIface->mapCanvas(), pixelCoords, mapCoords, enable);
     }
     else
       pnt = new QgsGeorefDataPoint( mCanvas, mIface->mapCanvas(), pixelCoords, mapCoords,true);

     mPoints.append( pnt );
     connect( mCanvas, SIGNAL( extentsChanged() ), pnt, SLOT( updateCoords() ) );
     ++i;
   }
   mInitialPoints = mPoints;
   mGCPsDirty = true;
  }
  if ( mGCPsDirty )
  {
    mGCPListWidget->setGCPList( &mPoints );
    updateGeorefTransform();
    mCanvas->refresh();
    mIface->mapCanvas()->refresh();
  }

  return true;
}

#2 Updated by Nyall Dawson over 7 years ago

Mark - this looks good! Could you submit this as a pull request to the QGIS Github repo (https://github.com/qgis/QGIS)? That's the usual approach to get changes like this merged in.