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

Map shaking after setMaxBounds and setMinZoom #8532

Closed
4 tasks done
SashaLifashkin opened this issue Oct 5, 2022 · 13 comments · Fixed by #8534
Closed
4 tasks done

Map shaking after setMaxBounds and setMinZoom #8532

SashaLifashkin opened this issue Oct 5, 2022 · 13 comments · Fixed by #8534

Comments

@SashaLifashkin
Copy link

Checklist

  • I've looked at the documentation to make sure the behavior isn't documented and expected.
  • I'm sure this is an issue with Leaflet, not with my app or other dependencies (Angular, Cordova, React, etc.).
  • I've searched through the current issues to make sure this hasn't been reported yet.
  • I agree to follow the Code of Conduct that this project adheres to.

Steps to reproduce

  1. Create a map.
  2. Set its maximum borders and minimum zoom to those currently visible on the screen.

const map = L.map('map').setView([50.450036, 30.5241361], 13)

L.tileLayer('http://{s}.tile.osm.org/{z}/{x}/{y}.png', {
attribution:
'&copy; <a href="http://osm.org/copyright">OpenStreetMap</a> contributors',
}).addTo(map)

map.setMaxBounds(map.getBounds()).setMinZoom(map.getZoom())

Expected behavior

The map is limited by the maximum borders and the minimum zoom without any unexpected behavior.

Current behavior

Map is limited by the maximum borders and the minimum zoom, but the map is moving (shaking).
I have looked at these issues: #1946, #2107, #2011, #2081, #1866, #2187. But it seems that there is no solution to my problem there, so I don't think it's a duplicate to any of previous issues. If it is, could you please help me to resolve this issue?
Thank you!

Minimal example reproducing the issue

https://jsfiddle.net/fd3mertj/1/

Environment

  • Leaflet version: 1.8.0, 1.9.1
  • Browser (with version): Mozilla 105.0.1 (64 bit), Opera 90.0.4480.84
  • OS/Platform (with version): Windows 10 laptop
@Malvoz
Copy link
Member

Malvoz commented Oct 5, 2022

Can reproduce in Chrome 106.0.5249.91 on Windows 10, I see a very small but noticeable wobble, stops for me if window is resized. Also sometimes had to resize the window to reproduce.

@Malvoz Malvoz added needs investigation and removed needs triage Triage pending labels Oct 5, 2022
@druwan
Copy link

druwan commented Oct 5, 2022

I believe that the issue comes from the dynamic restriction of setMaxBounds. So a quick workaround to stop the shaking that worked for me, was to change the geographical bound of setBounds

Maybe it can work for you too until someone with more knowledge joins the discussion. 😄

@SashaLifashkin
Copy link
Author

Can reproduce in Chrome 106.0.5249.91 on Windows 10, I see a very small but noticeable wobble, stops for me if window is resized. Also sometimes had to resize the window to reproduce.

Reproduced in Chrome 106.0.5249.103 I also see wobble. Yes, the shaking stops if I change the window size or zoom but that's not the solution

@Falke-Design
Copy link
Member

@SashaLifashkin can you please share a video

@SashaLifashkin
Copy link
Author

@SashaLifashkin can you please share a video

https://jsfiddle.net/fd3mertj/1/
From this link and some of the code I added, the problem is visible.

@SashaLifashkin
Copy link
Author

I believe that the issue comes from the dynamic restriction of setMaxBounds. So a quick workaround to stop the shaking that worked for me, was to change the geographical bound of setBounds

Maybe it can work for you too until someone with more knowledge joins the discussion. 😄

Unfortunately this solution didn't work for me.

@rjackson
Copy link
Contributor

rjackson commented Oct 6, 2022

This seems to be an intermittent problem, but I can repeat it. It takes a bit of adjusting the screen to figure out a screen size that does reproduce the issue – and even then it can seemingly fix itself without intervention.

We have Map._panInsideMaxBounds set up as a moveend event listener, and at certain screen / view sizes it seems to infinitely re-trigger itself.

We are supposed to break out of this recursion in Map.panInsideBounds by checking if a recalculated center is identical to the current center:

Leaflet/src/map/Map.js

Lines 503 to 514 in 4372a9f

panInsideBounds: function (bounds, options) {
this._enforcingBounds = true;
var center = this.getCenter(),
newCenter = this._limitCenter(center, this._zoom, toLatLngBounds(bounds));
if (!center.equals(newCenter)) {
this.panTo(newCenter, options);
}
this._enforcingBounds = false;
return this;
},

But our centers do not match up. Here are the center and newCenter values I've logged across many ticks of this recursive loop:

center newCenter
LatLng {lat: 50.450036, lng: 30.5241361} LatLng {lat: 50.450036000000004, lng: 30.524307761376956}
LatLng {lat: 50.45007183381818, lng: 30.52431106567383} LatLng {lat: 50.45007183381818, lng: 30.52413940429688}
LatLng {lat: 50.450036, lng: 30.5241361} LatLng {lat: 50.450036000000004, lng: 30.524307761376956}
LatLng {lat: 50.45007183381818, lng: 30.52431106567383} LatLng {lat: 50.45007183381818, lng: 30.52413940429688}
LatLng {lat: 50.450036, lng: 30.5241361} LatLng {lat: 50.450036000000004, lng: 30.524307761376956}
LatLng {lat: 50.45007183381818, lng: 30.52431106567383} LatLng {lat: 50.45007183381818, lng: 30.52413940429688}

Taking a look at Map._limitCenter, here are logged values for centerPoint, viewHalf, viewBounds.min, viewBounds.max, offset:

centerPoint viewHalf viewBounds.min viewBounds.max offset
Point {x: 1226393, y: 707140} Point {x: 220, y: 200} Point {x: 1226173, y: 706940} Point {x: 1226613, y: 707340} Point {x: -1, y: 0}
Point {x: 1226391.9807510755, y: 707140.3278318285} Point {x: 220, y: 200} Point {x: 1226171.9807510755, y: 706940.3278318285} Point {x: 1226611.9807510755, y: 707340.3278318285} Point {x: 1, y: 0}
Point {x: 1226393, y: 707140} Point {x: 220, y: 200} Point {x: 1226173, y: 706940} Point {x: 1226613, y: 707340} Point {x: -1, y: 0}
Point {x: 1226391.9807510755, y: 707140.3278318285} Point {x: 220, y: 200} Point {x: 1226171.9807510755, y: 706940.3278318285} Point {x: 1226611.9807510755, y: 707340.3278318285} Point {x: 1, y: 0}
Point {x: 1226393, y: 707140} Point {x: 220, y: 200} Point {x: 1226173, y: 706940} Point {x: 1226613, y: 707340} Point {x: -1, y: 0}
Point {x: 1226391.9807510755, y: 707140.3278318285} Point {x: 220, y: 200} Point {x: 1226171.9807510755, y: 706940.3278318285} Point {x: 1226611.9807510755, y: 707340.3278318285} Point {x: 1, y: 0}

I believe this might be related to floating point precision between the map bounds and its projected center. Each new projected center throws the bounds off slightly, leading to a different projected center.

I think we already have code that's attempting to fix this in Map._limitCenter for sub-pixel offsets:

Leaflet/src/map/Map.js

Lines 1545 to 1550 in 4372a9f

// an infinite loop of tiny offsets.
if (offset.round().equals([0, 0])) {
return center;
}
return this.unproject(centerPoint.add(offset), zoom);

I'd propose expanding that to ignore up to 1px offsets, as we seem to be seeing here.

I'll put a PR together with that proposal, if nobody objects?

@mourner
Copy link
Member

mourner commented Oct 6, 2022

@rjackson great analysis! Please submit a PR of course.

@jonkoops
Copy link
Collaborator

jonkoops commented Oct 8, 2022

Do we perhaps feel like this fix deserves a backport for v1?

@Falke-Design
Copy link
Member

Don't know, I think we should look that we get away from v1

@jonkoops
Copy link
Collaborator

jonkoops commented Oct 8, 2022

Agreed, however #8536 was already cherry-picked, so I think we're going to do a release anyways.

@Falke-Design
Copy link
Member

Yes that's true. Let us cherry pick this, because we make a release anyway

@jonkoops
Copy link
Collaborator

jonkoops commented Oct 8, 2022

Done 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants