Bug report #3609

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

Added by Giovanni Manghi over 14 years ago. Updated almost 10 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 almost 10 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 over 14 years ago

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

FIxed.

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

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

see also #3864

#3 Updated by Steven Mizuno about 14 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 about 14 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 14 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 14 years ago

  • Pull Request or Patch supplied changed from No to Yes

forgot to set Patch supplied

#7 Updated by Giovanni Manghi over 13 years ago

  • Target version changed from Version 1.7.0 to Version 1.7.4

#8 Updated by Giovanni Manghi over 13 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 over 13 years ago

  • Target version changed from Version 1.7.4 to Version 1.8.0

#10 Updated by Paolo Cavallini almost 13 years ago

  • Target version changed from Version 1.8.0 to Version 2.0.0

#11 Updated by Giovanni Manghi almost 13 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 10 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 10 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 10 years ago

I have created pull request #2314.

#15 Updated by Steven Mizuno almost 10 years ago

  • Status changed from Reopened to Closed

Also available in: Atom PDF