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

throttle location.replace() #38

Open
meetar opened this issue Jan 13, 2015 · 4 comments
Open

throttle location.replace() #38

meetar opened this issue Jan 13, 2015 · 4 comments

Comments

@meetar
Copy link

meetar commented Jan 13, 2015

Currently the hash is updated with the onMapMove event, and in Mac Chrome (40.0.2214.69 beta (64-bit) at least), this causes a performance hit to map drawing as the location bar is spammed. I'd like to suggest a throttle on location.replace(), similar to the update() throttle – but everything including the update() is triggered by the onhashchange event, expecting that the hash is being updated constantly during moves. So it seems this would involve reorganizing the whole file, effectively inverting the relationship between onhashchange and onMapMove.

Is there a good reason not to do this? I have a working example that seems to function just as I want it to, but I don't know whether it would break some use case besides my own.

@jfirebaugh
Copy link
Contributor

I think it would help to see an example app that demonstrates the spammy behavior. By default leaflet-hash binds only to the moveend event, which should not get frequently triggered. Are you seeing a situation where it is frequently triggered, or binding to move perhaps?

@bcamper
Copy link

bcamper commented Jan 13, 2015

We might be getting ahead of ourselves here anyway, as neither of these are "standard" examples, but please bear with me :)

In the Leaflet 1.0 (master) branch, map.flyTo() appears to rapidly generate moveend events (maybe it shouldn't?). But I realize that's not a released version.

For the Tangram renderer (which bridges to leaflet as a plugin), we've used flyTo, and also would like to have our own programmatically controlled zoom transitions, which currently call map.setZoom() frequently (again on 1.0/master, with its fractional zoom support). I realize that's outside of the normal Leaflet animation system, so it might be ill-advised anyway -- maybe there's a better way? Honestly that was just based on trying a few approaches until I got what seemed to be the smoothest one (which was to disable leaflet zoom animations and call setZoom directly).

I understand if these are too specific to warrant a change to leaflet-hash, but any related advice would be appreciated. Thanks!

@jfirebaugh
Copy link
Contributor

map.flyTo() generating rapid moveend events is a bug; I opened an upstream issue for that.

It sounds like you have some pretty specific needs with Tangram and will probably have to write some custom animation code to get the result you are looking for without generating excessive move events. I would say that it's too specific to warrant a change to leaflet-hash, but @mlevans's call ultimately. :)

@bcamper
Copy link

bcamper commented Jan 13, 2015

Makes sense, thanks.

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

3 participants