shramov / leaflet-plugins

Plugins for Leaflet library
http://psha.org.ru/b/leaflet-plugins.html
MIT License
722 stars 289 forks source link

fix: yandex zoom animation #284

Closed oranmor closed 5 years ago

oranmor commented 5 years ago

https://github.com/shramov/leaflet-plugins/issues/129

johnd0e commented 5 years ago

Unfortunately animation is delayed, and not synchronized with other leaflet objects. Just add any marker to see it:

L.marker([67.6755, 33.936]).addTo(map);

BTW, _limitedUpdate is not used.

oranmor commented 5 years ago

I see. I've added a zoomanim event, it's no longer delayed, but the sync isn't 100% accurate.

johnd0e commented 5 years ago

Normally we do not need to implement animation by ourselves, as it processed by Leaflet, via css transition. Animating transition via ymaps API looks like workaround. But we should try to find proper solution.

I do not know why transition is not applied to ymaps container.

At first I thought that it may be related to asynchronousness of ymaps API (all that setZoom, setCenter, panTo return promises). But I haven't find places in code where this might impact.

So question is what's wrong with ymaps container? Why it is not processed by leaflet animation routines? We have no such issue with similar google plugin.

johnd0e commented 5 years ago

I've added a zoomanim event,

Now _update is called twice, and it leads to unpleasant artifacts.

oranmor commented 5 years ago

@johnd0e I can't reproduce this. Perhaps it called twice because you have two yandex layers on the map? There are two of them in yandex.html example (normal map and traffic map). Try to disable traffic.

johnd0e commented 5 years ago

I can't reproduce this.

Sorry, I meant setCenter.

  1. zoomanim event -> _zoomAnim -> setCenter
  2. moveend event -> _update -> setCenter
brunob commented 5 years ago

Should this one be closed in favor of #285 ?

johnd0e commented 5 years ago

No.

285 contains only part not related to zoom animation.

johnd0e commented 5 years ago

@oranmor

So question is what's wrong with ymaps container? Why it is not processed by leaflet animation routines? We have no such issue with similar google plugin.

As I figured out, GoogleMutant is based on L.GridLayer, which has some routines like _animateZoom. It may be possible to re-implement similar routines for Yandex.

johnd0e commented 5 years ago

@oranmor

Here is the part which implements animation in GridLayer.

With yandex canvas it should be even simpler, more like in ImageOverlay:

    _animateZoom: function (e) {
        var map = this._map;
        var topLeft = map._getNewPixelOrigin(e.center, e.zoom);
                var offset = map.project(map.getBounds().getNorthWest(), e.zoom)._subtract(topLeft);
        L.DomUtil.setTransform(this._container, offset, map.getZoomScale(e.zoom));
    },

It would be better to avoid using private methods

johnd0e commented 5 years ago

@brunob Fix is included in #285 now.

brunob commented 5 years ago

So we can close this one ?

johnd0e commented 5 years ago

Sure, if @oranmor is happy with my fix

brunob commented 5 years ago

Closing in favor of #285