Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MBTiles raster support in WMS provider #33855

Merged
merged 2 commits into from
Jan 17, 2020

Conversation

wonder-sk
Copy link
Member

I have had this code around for a while and would like to hear what other devs think about it... (I don't mind if it does not get into 3.12.)

This PR adds MBTiles tiled raster map support to WMS provider so that it uses the same code paths
like WMTS or XYZ tiles.

Why move away from GDAL implementation? Here are the advantages of the approach through WMS provider:

  • correctly scaling tiles on high dpi display
  • better look when not zoomed to native resolution of the tiles. WMS provider uses smooth scaling while GDAL uses nearest neighbor by default.
  • map tile showing up while rendering (with gdal it's blank map until everything is loaded)
  • possible to use tile scale slider dock widget
  • faster - mainly a side effect of loading fewer tiles on high dpi display

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good to me, and quite straightforward. I think there's a good argument for this change too.

+0 to merge, +1 if you add a unit test.

{
QgsCoordinateTransform ct( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:4326" ) ), QgsCoordinateReferenceSystem( mSettings.mCrsId ),
transformContext() );
QgsPointXY customTopLeft = ct.transform( QgsPointXY( sourceExtentWgs84.xMinimum(), sourceExtentWgs84.yMaximum() ) );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try/catch

@nyalldawson
Copy link
Collaborator

(i'd be happy to see this merged for 3.12 too, I think it's a sufficiently low risk change)

@wonder-sk
Copy link
Member Author

Thanks Nyall for the fast review - I have added some basic tests and I would merge it for 3.12...

@wonder-sk wonder-sk added this to the 3.12.0 milestone Jan 17, 2020
@wonder-sk wonder-sk merged commit 56f40ec into qgis:master Jan 17, 2020
@timlinux
Copy link
Member

Any notes on how to use this @wonder-sk ? Must we enter a file:/// path to an mbtiles file in the URL

@timlinux timlinux added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Feb 17, 2020
@wonder-sk
Copy link
Member Author

sorry @timlinux I have missed the notification....

You don't need to do anything special - it is automatic. If you simply open a mbtiles file from the browser or drag'n'drop it to qgis, the WMS provider will be now used (instead of gdal).

@timlinux timlinux removed the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label May 14, 2020
@agiudiceandrea
Copy link
Contributor

Hi @wonder-sk, it looks like there are issue when the MBTiles layer's path contains multibytes characters (after unicode U+00FF): please see #56023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants