search

Leaflet / Leaflet

🍃 JavaScript library for mobile-friendly interactive maps 🇺🇦
https://leafletjs.com
BSD 2-Clause "Simplified" License
40.28k stars 5.76k forks source link

Added zomeTune feature and continuous zoom #1941

Closed calandoa closed 10 years ago

calandoa commented 10 years ago

I added an option for the tile layer object which allows to use a set of tile with a different zoom level.

For instance, we want a zoom range between 4 and 10, but we only have the tiles for 6 and 8. Setting the zoomTune option with the following command allows to redirect zoom 4, 5 and 6 to tiles with level 6, and other zooms to tiles with level 8:

L.tileLayer('foo/{z}/{y}/{x}.bar', {
        minZoom: 4, maxZoom: 10,
        zoomTune: [ 4, 4, 4, 8, 8, 8, 8 ]
});

It appears that implementing continuous zoom (aka non integer zoom level) was very easy after this, so I added a few modifications to the different control for testing and for making the best use of this new feature.

Note that maxNativeZoom and detectRetina seem useless (or at least can be simplified) with this new feature. Also, the code is not really optimized when changing the zoom slightly, as all tiles are discarded and put back. Only changing their size and position would be faster, but this was really too complicated with the current architecture.

Note also that I am a complete beginner in JavaScript / git / GitHub, so there might be problems with this patch. BTW it has not been tested with jake (not working on my host) but with a few different browsers (Opera / Firefox / Chrome with Linux / Windows / Android).

mourner commented 10 years ago

Sorry for being late, I just got back from vacation and will thoroughly review this pull. Seems like a very interesting approach, although it complicates the code a bit.

surjikal commented 10 years ago

Continuous zoom would be amazing!

calandoa commented 10 years ago

No problem, but it is now my turn to be in holidays :)

I can work on any correction when I am back (in about 10 days).

Vladimir Agafonkin notifications@github.com wrote:

Sorry for being late, I just got back from vacation and will thoroughly review this pull. Seems like a very interesting approach, although it complicates the code a bit.

Reply to this email directly or view it on GitHub: https://github.com/Leaflet/Leaflet/pull/1941#issuecomment-23349958

dlev- commented 10 years ago

I know I'm not a contributor to this project, but I looked over the change, and it looked good to me (I reviewed it as I would a change in my work codebase). I'm excited about this new functionality.

dlev- commented 10 years ago

I just set up a very quick demo to test this code. When I mousewheel-zoom and I'm not on integer tile levels, the tile requests fail and the map goes grey.

calandoa commented 10 years ago

This should work better now. The problem occurred because the code was assuming the option zoomTune (as described above) was given.

surjikal commented 10 years ago

The zooming doesn't feel fluid when scrolling on my mac.. it's almost as if the discrete zoom levels were still there, but smaller. I was expecting it to continuous like the new google maps. I didn't set a zoomTune parameter, if that makes a difference.

calandoa commented 10 years ago

The fluid animation is not related to this patch, it depends on AnimationZoom module, after doing some tests with and without. Did you included this module?

dlev- commented 10 years ago

I've been playing with these changes, and I keep seeing times when it tries to request tiles with an x value of 123.99999997 or something like that (i.e. close to an integer value, but not quite integer). If I round the request, then I start seeing wrong tile requests at very low zoom levels (zoomed way out).

calandoa commented 10 years ago

Can you provide your html file so I can try to reproduce this bug?

dlev- commented 10 years ago

Sure. Put this in the Leaflet base dir. Then just zoom in and out with mouse wheel or shift-click the +/- buttons in the zoom control

<html>
<head>
    <title>Leaflet Quick Start Guide Example</title>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="dist/leaflet.css" />
    <link rel="stylesheet" href="debug/css/screen.css" />
</head>
<body>
    <div id="map" style="width: 600px; height: 400px"></div>

    <script type="text/javascript" src="build/deps.js"></script>
    <script src="debug/leaflet-include.js"></script>
    <script>
        var map = L.map('map').setView([51.505, -0.09], 13);
        L.tileLayer('http://a.tile.cloudmade.com/BC9A493B41014CAABB98F0471D759707/997/256/{z}/{x}/{y}.png', {
            zoomTune: [ 0,  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 16, 17, 18],
            minZoom: 0,
            maxZoom: 18
        }).addTo(map);
    </script>
</body>
</html>

Eventually within 10 interactions I'll see bad tile requests as I described above. I've been mostly playing in chrome.

calandoa commented 10 years ago

After doing some testing, I could not reproduce the problem with:

Note that I commented inclusion of debug/css/screen.css, but I do not think it is the cause.

May be the compiler command? Can you provide your "java -jar closure-compiler/compiler.jar -js etc..." I will reuse the same. The one I am using:

java -jar closure-compiler/compiler.jar --js src/Leaflet.js --js src/core/Util.js --js src/core/Class.js --js src/core/Events.js --js src/core/Browser.js --js src/geometry/Point.js --js src/geometry/Bounds.js --js src/geometry/Transformation.js --js src/dom/DomUtil.js --js src/geo/LatLng.js --js src/geo/LatLngBounds.js --js src/geo/projection/Projection.js --js src/geo/projection/Projection.SphericalMercator.js --js src/geo/projection/Projection.LonLat.js --js src/geo/crs/CRS.js --js src/geo/crs/CRS.Simple.js --js src/geo/crs/CRS.EPSG3857.js --js src/geo/crs/CRS.EPSG4326.js --js src/map/Map.js --js src/layer/tile/TileLayer.js --js src/layer/marker/Icon.js --js src/layer/marker/Icon.Default.js --js src/layer/marker/Marker.js --js src/layer/Popup.js --js src/layer/marker/Marker.Popup.js --js src/layer/vector/Path.js --js src/layer/vector/Path.SVG.js --js src/layer/vector/Path.Popup.js --js src/geometry/LineUtil.js --js src/layer/vector/Polyline.js --js src/geometry/PolyUtil.js --js src/layer/vector/Polygon.js --js src/layer/vector/Rectangle.js --js src/dom/DomEvent.js --js src/dom/Draggable.js --js src/core/Handler.js --js src/map/handler/Map.Drag.js --js src/map/handler/Map.DoubleClickZoom.js --js src/map/handler/Map.ScrollWheelZoom.js --js src/map/handler/Map.BoxZoom.js --js src/layer/marker/Marker.Drag.js --js src/control/Control.js --js src/control/Control.Zoom.js --js src/control/Control.Attribution.js --js src/control/Control.Scale.js --js src/control/Control.Layers.js --js_output_file try/try.js
dlev- commented 10 years ago

Hrm. I'm not compiling. The file I referenced just includes the source files. I consistently see this problem on IE10, win 7chrome, and mac chrome if I slowly mousewheel-zoom out. I leave the console open so I can see the bad requests.

calandoa commented 10 years ago

I did many tries again by directly referencing the sources, but the problem is not appearing :(

dlev- commented 10 years ago

In the test html I pasted above, replace the map creation line with this one

var map = L.map('map').setView([51.504718976922824, -0.09650819952279344], 12.5);

Just loading that html file now repros the problem on chrome, ff, and IE10.

calandoa commented 10 years ago

I have just tried your last patch with Opera, Firefox, Chrome and IE, this one cannot be reproduced. Are you sure your repository is up to date? I remember a similar problem months ago that I had corrected.

dlev- commented 10 years ago

Looks like that could be the issue. I applied your patch on top of the 0.7.1 stable branch.

gouilaz commented 10 years ago

I also applied everything on 0.7.1 and it works without any problems, so far. Nice work, calandoa. Will test it now on several mobile devices and will have a look at your modification more extensively.

dlev- commented 10 years ago

Sounds like the error was on my side. Sorry about that!

gouilaz commented 10 years ago

I get some ugly white gaps between tiles in some zoom levels, e.g.:

var map = L.map('map').setView([51.505, -0.09], 3.1);

or

var map = L.map('map').setView([51.505, -0.09], 4.3);

It happens nearly all the time at the X.1 and X.3 decimals. Other decimal levels work perfectly. Bug happens accross all three browser, Chrome, IE and Firefox (newest versions) and is also visible on mobile devices, tested on android 4.3, stock browser, chrome mobile and firefox mobile.

Any suggestion, calandoa ?

EDIT: Have some cases where even smaller zoom levels are called. It happens also at levels, such as 3.21, etc.

calandoa commented 10 years ago

Problem reproduced. I will investigate to understand the cause.

gouilaz commented 10 years ago

I am currently debugging your modification, too. As far as I can tell it has something to do with the createTile function, see Line 532 at TileLayer.js. You added a "tileVirtSize" to calculate the correct inbetween zoom scale of a tile. I debugged at a zoom level of 3.1. The "tileVirtSize" variable produces a value of "274.37400640929104" which seem to create this weird behaviour. Rounding the "tileVirtSize" variable to the nearest integer didn't help at all. I will further investigate if this is really the cause and report back.

calandoa commented 10 years ago

I found out the problem. It is due to the fact that the sum of rounded floats is not always equal to the rounded sum of the same floats. I added a patch to correct this.

However, if this is now working better with PC browsers, I sometimes still get gaps on my android smartphone. Unfortunately, I do not know here how to get the javascript console output.

BTW, I also added a feature which automatically zoom out when zooming or moving on a location where no tiles can be loaded.

dlev- commented 10 years ago

Nice fix. Both with and without that fix I'm seeing bad tile requests at zoom level 0.1 (to repro just use the HTML I provided 2 weeks ago and change the initial zoom value from 13 to 0.1). If I pan West, the tiles look fine, but panning East I just see bad tile requests.

gouilaz commented 10 years ago

This looks a lot better now on desktop browsers. I am not seeing the gap issue on my android phone, yet. Will report if it is happening at all.

While debugging your script, I noticed a new bug. It only happens on my both mobile devices (Samsung S4 / S3). This bug was not introduced with your newest patch. It happens with and without your current patch. Between a zoom level of 0.63 up to 0.86 only a quarter of tiles are displayed. The bottom row and right column of tiles just don't show up. This behaviour is consistent accross all mobile browser that I tested: Samsung S4 Stock Browser / Chrome / Chrome Beta / Firefox and Baidu Browser.

mourner commented 10 years ago

Quick update: after some serious refactoring of Leaflet zoom animation logic in the master branch (in addition to the GridLayer refactoring), I'm now ready to implement the ability to set zoom to a fractional value. But differently — not by resizing the tiles, because this is slow and may introduce gaps, but by transforming the tile container in the same vain as it happens during zoom animation. But this pull is still really useful as a reference for borrowing ideas, so I'm keeping it open.

gouilaz commented 10 years ago

Thanks for the update, mourner. Your approach sounds interesting. Keep us informed as soon as you want to bounce your first version at us. We are ready for debugging and testing.

dlev- commented 10 years ago

@calandoa I think I have a fix for the issue I found. In TileLayer._getWrapTileNum I changed 

size = crs.getSize(this._map.getZoom());

to 

size = crs.getSize(this._getTileZoom())

Sorry but I can't easily generate a pull request right now.

@gouilaz does this help the issue you reported?

@mourner Yay!

calandoa commented 10 years ago

@dlev- and @gouilaz I cannot reproduce these two bugs, however, I added both of you as contributors, so if the patch above correct anything feel free to commit it (not familiar with github but I suppose this will give you the permission).

mourner commented 10 years ago

Just got CSS Transforms powered fractional zoom working (though still some quirks left to fix), the work is happening here https://github.com/Leaflet/Leaflet/pull/2382

mourner commented 10 years ago

Closing the pull but keeping an eye on it, will continue working on the easey branch with continuous /fractional zoom stuff.