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: yandex zoom animation #284

Closed
wants to merge 2 commits into from

Conversation

oranmor
Copy link

@oranmor oranmor commented May 23, 2019

@oranmor oranmor force-pushed the feature/yandex-zoom-animation branch from a8a2dab to decdd27 Compare May 23, 2019 06:59
@johnd0e
Copy link
Contributor

johnd0e commented May 23, 2019

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

oranmor commented May 23, 2019

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

@johnd0e
Copy link
Contributor

johnd0e commented May 23, 2019

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
Copy link
Contributor

johnd0e commented May 23, 2019

I've added a zoomanim event,

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

@oranmor
Copy link
Author

oranmor commented May 23, 2019

@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
Copy link
Contributor

johnd0e commented May 23, 2019

I can't reproduce this.

Sorry, I meant setCenter.

  1. zoomanim event -> _zoomAnim -> setCenter
  2. moveend event -> _update -> setCenter

@brunob
Copy link
Collaborator

brunob commented May 23, 2019

Should this one be closed in favor of #285 ?

@johnd0e
Copy link
Contributor

johnd0e commented May 23, 2019

No.
#285 contains only part not related to zoom animation.

@johnd0e
Copy link
Contributor

johnd0e commented May 24, 2019

@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
Copy link
Contributor

johnd0e commented May 28, 2019

@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
Copy link
Contributor

johnd0e commented May 30, 2019

@brunob
Fix is included in #285 now.

@brunob
Copy link
Collaborator

brunob commented May 30, 2019

So we can close this one ?

@johnd0e
Copy link
Contributor

johnd0e commented May 30, 2019

Sure, if @oranmor is happy with my fix

@brunob
Copy link
Collaborator

brunob commented Jul 17, 2019

Closing in favor of #285

@brunob brunob closed this Jul 17, 2019
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.

None yet

3 participants