wp-plugin-leaflet-map icon indicating copy to clipboard operation
wp-plugin-leaflet-map copied to clipboard

max_zoom and default tiles

Open hupe13 opened this issue 2 years ago • 17 comments

Hi Bozdoz,

Due to a bug in my plugin, I took one more look at max_zoom. I would like to change https://github.com/bozdoz/wp-plugin-leaflet-map/blob/b6bafe8f4fd1373776201768f5a179d55fcdc845/shortcodes/class.map-shortcode.php#L81-L82 to

        $atts['max_zoom'] = empty($max_zoom) ?
            $settings->get('default_max_zoom') : 
            ($max_zoom <= $settings->get('default_max_zoom') ? $max_zoom : $settings->get('default_max_zoom'));

Default max zoom should be the max zoom of default tiles or smaller.

I make a pull request.

hupe13 avatar Aug 01 '23 18:08 hupe13

I don't understand. If the user passes max-zoom, why would we override that with the default max zoom?

bozdoz avatar Aug 01 '23 19:08 bozdoz

Some tiles have a max zoom and the max zoom from map must not be greater as the max zoom from tiles. If the max zoom from map is greater as max zoom from tiles you get grey areas.

hupe13 avatar Aug 01 '23 19:08 hupe13

Just seems like a difficult problem to debug if someone wants to override the default but can't

bozdoz avatar Aug 01 '23 20:08 bozdoz

Or we need to have two options max_zoom, one for map and one for tiles. And the same for min_zoom. https://leafletjs.com/reference.html#map-minzoom https://leafletjs.com/reference.html#map-maxzoom

hupe13 avatar Aug 01 '23 20:08 hupe13

New approach:

https://github.com/bozdoz/wp-plugin-leaflet-map/blob/b6bafe8f4fd1373776201768f5a179d55fcdc845/shortcodes/class.map-shortcode.php#L193-L201

We need some more options. There is a repository leaflet-providers and here are the options for many tileservers. Is there a convenient way to consider many different options without defining a setting for each possible one? For now you have defined some options for MapQuest.

To avoid grey areas there is an option maxNativeZoom.

hupe13 avatar Aug 02 '23 07:08 hupe13

I suppose min and max zoom overlap too much with tilelayer min and max; why don't we just add tileMaxZoom?

bozdoz avatar Aug 02 '23 13:08 bozdoz

This I have done now. I prepared a pull request. Sorry about the removed spaces, this makes my editor automatically.

class.leaflet-map.php:

  • leafletjs version 1.9.4

class.plugin-settings.php

  • changed default max_zoom to 18, because https://leafletjs.com/reference.html#map-maxzoom and https://leafletjs.com/reference.html#tilelayer-maxzoom
  • removed subdomains from default map_tile_url
  • new option map_tile_maxzoom

readme.txt

  • WordPress 6.3

shortcodes/class.map-shortcode.php

  • changed getting map maxZoom, if empty
  • tile maxZoom

templates/settings.php:

  • added _e(' - Please change JavaScript-URL and CSS-URL.', 'leaflet-map');

What do you mean?

hupe13 avatar Aug 04 '23 19:08 hupe13

Thanks! I'll take a look next week.

bozdoz avatar Aug 04 '23 21:08 bozdoz

Why remove subdomains?

bozdoz avatar Aug 04 '23 21:08 bozdoz

I read this in german OSM forum. There is a link to an issue. And in leaflet-providers.js are the tiles without subdomain.

hupe13 avatar Aug 04 '23 21:08 hupe13

Great find. Makes sense

bozdoz avatar Aug 04 '23 22:08 bozdoz

Could you make a pull request of that branch?

bozdoz avatar Aug 14 '23 22:08 bozdoz

max_zoom and map_tile_maxzoom do not work yet together, and it takes time to test. If I write [leaflet-map zoom=18 max_zoom=23 map_tile_maxzoom="20" tileurl=https://....] I get grey tiles if I zoom to a zoomlevel greater than map_tile_maxzoom. It should not be possible to zoom to that zoomlevel.

hupe13 avatar Sep 21 '23 06:09 hupe13

Right but what would you expect given a shortcode like that? Are you suggesting that map max zoom should be restricted to map tile max zoom no matter what?

bozdoz avatar Sep 21 '23 11:09 bozdoz

I made a commit. Now it works as I expect. But it does not work, if detect-retina is true and it is a retina display. Then the maxZoom should be one step lower.

hupe13 avatar Sep 21 '23 16:09 hupe13

I think, I found the solution, it works with Retina also now.

The differences to your reposiitory are:

  • changed default max_zoom to 18, because https://leafletjs.com/reference.html#map-maxzoom and https://leafletjs.com/reference.html#tilelayer-maxzoom
  • removed subdomains from default map_tile_url
  • new option map_tile_maxzoom
  • new option map_tile_minzoom
  • no grey tiles anymore

Maybe we need to change "Info: your leaflet version may be out-of-sync with the latest default version: 1.9.4". Many don't know what to do with it. But you do it, your english is better than mine.

What do you mean?

hupe13 avatar Sep 23 '23 16:09 hupe13

Yeah I looked into changing it. Maybe we could output the js and css urls we expect for latest.

bozdoz avatar Sep 23 '23 17:09 bozdoz