Bug report #3609

in a reprojected raster "zoom to best scale (100%)" does not work

Added by Giovanni Manghi over 13 years ago. Updated over 8 years ago.

Status:Closed
Priority:High
Assignee:-
Category:Rasters
Affected QGIS version:master Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:Yes Resolution:
Crashes QGIS or corrupts data:No Copied to github as #:13668

Description

summary says it all

patch_for_3609.diff Magnifier (4.63 KB) Steven Mizuno, 2011-08-12 04:59 AM

Related issues

Duplicates QGIS Application - Bug report #3864: raster layer - zoom to best scale - scale is incorrect wi... Closed

Associated revisions

Revision 7f731ae3
Added by Steven Mizuno over 8 years ago

fix legendLayerZoomNative to use diagonal of source raster pixel

to match the use of the diagonal of mapCanvas pixel.
Also fix spelling in a comment.

(fixes #3609)

History

#1 Updated by Redmine Admin about 13 years ago

  • Status changed from Open to Closed
  • Resolution set to fixed

FIxed.

#2 Updated by Jürgen Fischer about 13 years ago

  • Status changed from Closed to Feedback
  • Resolution deleted (fixed)

see also #3864

#3 Updated by Steven Mizuno about 13 years ago

I have realized that the resulting scale is the square root of 2 larger than it should be, which gave me a clue.
The problem comes from the calculation for the ratio to zoom by - the 'width' of reprojected pixel is actually the diagonal of the pixel, but the raster pixel width is just the x distance

It appears the intent is to use the diagonal, but the raster layer doesn't provide the y raster units per pixel.

A simple fix is to just use the x distance.
line 1843 in QgsLegend could be

double width = qAbs( p1.x() - p2.x() ); // width of reprojected pixel

instead of 
double width = sqrt( p1.sqrDist( p2 ) ); // width of reprojected pixel

I have tried this.

#4 Updated by Paolo Cavallini almost 13 years ago

  • Tracker changed from Bug report to 4
  • Start date set to 2011-07-25
  • Pull Request or Patch supplied set to No
  • Status changed from Feedback to Open

#5 Updated by Steven Mizuno almost 13 years ago

Here is a better way to fix.

My proposed fix involves adding two functions to QgsRasterLayer:
  • rasterUnitsPerPixelX()
  • rasterUnitsPerPixelY()

and using these in QgsLegend::legendLayerZoomNative() to calculate the diagonal distance of a raster pixel.

This is good preparation for the future in handling non-square pixels.

The X and Y raster units per pixel values are already handled in QgsRasterLayer, so it is just a matter of providing API functions for use by other modules.

As rasterUnitsPerPixel() already returns the X value it is changed to call rasterUnitsPerPixelX() to avoid duplicate code.

Perhaps rasterUnitsPerPixel() could be removed at some point; its use probably should be deprecated.

I have also included Python interface functions.

#6 Updated by Steven Mizuno almost 13 years ago

  • Pull Request or Patch supplied changed from No to Yes

forgot to set Patch supplied

#7 Updated by Giovanni Manghi over 12 years ago

  • Target version changed from Version 1.7.0 to Version 1.7.4

#8 Updated by Giovanni Manghi about 12 years ago

  • Affected QGIS version set to master
  • Crashes QGIS or corrupts data set to No
  • Tracker changed from 4 to Bug report

#9 Updated by Paolo Cavallini about 12 years ago

  • Target version changed from Version 1.7.4 to Version 1.8.0

#10 Updated by Paolo Cavallini almost 12 years ago

  • Target version changed from Version 1.8.0 to Version 2.0.0

#11 Updated by Giovanni Manghi over 11 years ago

  • Assignee deleted (Redmine Admin)
  • Operating System deleted (All)
  • Status info deleted (0)
  • Status changed from Open to Closed
  • Resolution set to fixed

Seems to work fine in the latest master.

#12 Updated by Steven Mizuno almost 9 years ago

  • Resolution deleted (fixed)
  • Status changed from Closed to Reopened
  • Target version changed from Version 2.0.0 to Version 2.12

During a process of qualifying recent versions of QGIS I find that the" Zoom to best scale (100%)" still is not working correctly (or has reverted -- I'm not sure as I had been using a personal modified version from before v. 1.8). This is when OTF is enabled.

An example is an aerial photo at 1m ground sample distance is zoomed to 1:2673 rather than 1:3780 that would be proper for a 96dpi display.

The problem is due to a calculation error in QgisApp::legendLayerZoomNative() where the screen pixel size is determined by the square root of the squares of the x and y distances, but the raster layer pixel size is just the x distance.

Here is a reworking of the previously supplied patch.
in qgisapp.cpp:
at or about line 7622:

      mMapCanvas->zoomByFactor( qAbs( layer->rasterUnitsPerPixelX() / width ) );

change to:

      mMapCanvas->zoomByFactor( sqrt( layer->rasterUnitsPerPixelX() * layer->rasterUnitsPerPixelX() + layer->rasterUnitsPerPixelY() * layer->rasterUnitsPerPixelY() ) / width );

This takes square root of the quantity of the squares of the X and Y raster pixel distances added, the same as is done for the display pixel.

#13 Updated by Giovanni Manghi almost 9 years ago

  • Priority changed from Low to High

Steven Mizuno wrote:

During a process of qualifying recent versions of QGIS I find that the" Zoom to best scale (100%)" still is not working correctly (or has reverted -- I'm not sure as I had been using a personal modified version from before v. 1.8). This is when OTF is enabled.

An example is an aerial photo at 1m ground sample distance is zoomed to 1:2673 rather than 1:3780 that would be proper for a 96dpi display.

The problem is due to a calculation error in QgisApp::legendLayerZoomNative() where the screen pixel size is determined by the square root of the squares of the x and y distances, but the raster layer pixel size is just the x distance.

Here is a reworking of the previously supplied patch.
in qgisapp.cpp:
at or about line 7622:
[...]

change to:
[...]

This takes square root of the quantity of the squares of the X and Y raster pixel distances added, the same as is done for the display pixel.

many thanks for this patch. I would like to suggest to submit it as a Pull Request on github, otherwise here there is the real risk to be overlooked by core devs. Cheers!

#14 Updated by Steven Mizuno almost 9 years ago

I have created pull request #2314.

#15 Updated by Steven Mizuno over 8 years ago

  • Status changed from Reopened to Closed

Also available in: Atom PDF