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

Fix extra tile usage #4193

Merged
merged 8 commits into from Feb 8, 2016
Merged

Fix extra tile usage #4193

merged 8 commits into from Feb 8, 2016

Conversation

IvanSanchez
Copy link
Member

Should fix #4180

Added a new viewprereset map event, run before the viewreset event.

So the sequence for when fadeAnimation is false is: unload tiles on viewprereset, then move the map to the proper viewport, then load tiles on viewreset. This makes PhantomJS a viable option for unit-testing tile loading counts.

Unit tests in graphical browers do fail as of now, though.

@@ -444,7 +445,8 @@ L.GridLayer = L.Layer.extend({

_getTiledPixelBounds: function (center) {
var map = this._map,
scale = map.getZoomScale(map.getZoom(), this._tileZoom),
mapZoom = map._animatingZoom ? Math.max(map._animateToZoom, map.getZoom()) : map.getZoom(),
scale = map.getZoomScale(mapZoom, this._tileZoom),
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to insist a bit, but as per my understanding we don't need scale: if the map size is 600px at zoom 10 it will still be 600px at zoom 11 or 17 or 17.358, isn't it? Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yohanboniface You're missing fractional zoom. In those cases, 0.5 < scale < 2. See the debug/tests/tile-bounds.html file.

e.g. if the zoom level is 17.348, and the map is 600px, then there are less than 600px worth of tiles (and less tiles are loaded) because tile pixels are bigger.

@IvanSanchez IvanSanchez added this to the 1.0-rc1 milestone Feb 4, 2016
@mourner
Copy link
Member

mourner commented Feb 4, 2016

Added a new viewprereset map event, run before the viewreset event.

That's just asking for double brainmelt in future. :)

@IvanSanchez
Copy link
Member Author

That's just asking for double brainmelt in future. :)

@mourner Actually, I don't think so. Layers should clear everything up on viewprereset, then add everything back again in viewreset. Makes sense.

God, I want leafdoc merged in so I can document these kind of things inline right away.

@yohanboniface
Copy link
Member

I want leafdoc merged in so I can document these kind of things inline right away.

Why are we not there yet?

@IvanSanchez
Copy link
Member Author

I think this PR introduces some kind of race condition on flyTo animations that makes tiles to not be pruned when switching levels, likely due to https://github.com/Leaflet/Leaflet/pull/4193/files#diff-f1e6be67599c594731fff6191c710420L655 - must find a way to trigger a pruning once in a while.

@IvanSanchez IvanSanchez changed the title WIP: Fix extra tile usage Fix extra tile usage Feb 4, 2016
@hyperknot
Copy link
Collaborator

Some debugging on tile-events.html:

  1. The load count for TileLayers is not reflecting the actual img requests from the browser tools. In my case: grid = 20, postiron = 40, img requests = 20.
  2. It is happening when grid is added to map. If map.addLayer(grid); is commented out, the tileLayer values are correct (except tileloadstart).
  3. tileloadstart does not fire for initial loading of tileLayer. Adding setTimeout to gridLayer fixes tileLayer.
   setTimeout(function(){
            this.fire('tileloadstart', {
                tile: tile,
                coords: coords
            });
        }.bind(this));

@hyperknot hyperknot mentioned this pull request Feb 8, 2016
@hyperknot
Copy link
Collaborator

#4145 also OK with this one:
http://jsbin.com/ratoli/9/edit?html,js,output

mourner added a commit that referenced this pull request Feb 8, 2016
@mourner mourner merged commit 01455c0 into master Feb 8, 2016
@mourner mourner deleted the grid-extra-loads branch February 8, 2016 14:53
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

Successfully merging this pull request may close these issues.

Fixing 4x tile usage
4 participants