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

max_zoom and default tiles #232

Open
hupe13 opened this issue Aug 1, 2023 · 17 comments
Open

max_zoom and default tiles #232

hupe13 opened this issue Aug 1, 2023 · 17 comments

Comments

@hupe13
Copy link
Collaborator

hupe13 commented Aug 1, 2023

Hi Bozdoz,

Due to a bug in my plugin, I took one more look at max_zoom. I would like to change

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

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.

@bozdoz
Copy link
Owner

bozdoz commented Aug 1, 2023

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

@hupe13
Copy link
Collaborator Author

hupe13 commented Aug 1, 2023

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.

@bozdoz
Copy link
Owner

bozdoz commented Aug 1, 2023

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

@hupe13
Copy link
Collaborator Author

hupe13 commented Aug 1, 2023

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
Copy link
Collaborator Author

hupe13 commented Aug 2, 2023

New approach:

$tile_layer_options = array(
'tileSize' => empty($tilesize) ? $settings->get('tilesize') : $tilesize,
'subdomains' => empty($subdomains) ? $settings->get('map_tile_url_subdomains') : $subdomains,
'id' => empty($mapid) ? $settings->get('mapid') : $mapid,
'accessToken' => empty($accesstoken) ? $settings->get('accesstoken') : $accesstoken,
'zoomOffset' => empty($zoomoffset) ? $settings->get('zoomoffset') : $zoomoffset,
'noWrap' => filter_var(empty($nowrap) ? $settings->get('tile_no_wrap') : $nowrap, FILTER_VALIDATE_BOOLEAN),
'maxZoom' => $atts['maxZoom']
);

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.

@bozdoz
Copy link
Owner

bozdoz commented Aug 2, 2023

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

@hupe13
Copy link
Collaborator Author

hupe13 commented Aug 4, 2023

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

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?

@bozdoz
Copy link
Owner

bozdoz commented Aug 4, 2023

Thanks! I'll take a look next week.

@bozdoz
Copy link
Owner

bozdoz commented Aug 4, 2023

Why remove subdomains?

@hupe13
Copy link
Collaborator Author

hupe13 commented Aug 4, 2023

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

@bozdoz
Copy link
Owner

bozdoz commented Aug 4, 2023

Great find. Makes sense

@bozdoz
Copy link
Owner

bozdoz commented Aug 14, 2023

Could you make a pull request of that branch?

@hupe13
Copy link
Collaborator Author

hupe13 commented Sep 21, 2023

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.

@bozdoz
Copy link
Owner

bozdoz commented Sep 21, 2023

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?

@hupe13
Copy link
Collaborator Author

hupe13 commented Sep 21, 2023

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
Copy link
Collaborator Author

hupe13 commented Sep 23, 2023

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

The differences to your reposiitory are:

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?

@bozdoz
Copy link
Owner

bozdoz commented Sep 23, 2023

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

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

No branches or pull requests

2 participants