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

fitBounds padding is not zooming correctly #4846

Closed
bryanjjohnson opened this issue Jun 16, 2017 · 6 comments · Fixed by #5520
Closed

fitBounds padding is not zooming correctly #4846

bryanjjohnson opened this issue Jun 16, 2017 · 6 comments · Fixed by #5520
Labels

Comments

@bryanjjohnson
Copy link

Using fitBounds with padding, I wanted to increase the padding on the left. I noticed that it seemed way off though for larger padding values.

http://jsbin.com/nadukifexa/edit?html,output

In the example, I drew a 400px wide box, then set the padding on the left to 420 (400 + 20). You would expect the fit map to be just off of the right side of the box. Instead it is way off. To achieve the expected result, I had to use half and set it to 220 (200 + 20).

@mollymerp
Copy link
Contributor

@bryanjjohnson thanks for the issue report and the test case. I think we're incorrectly accounting for the padding when we approximate the zoom level of the bounded region.

@pastcompute
Copy link

pastcompute commented Sep 6, 2017

Hi @mollymerp I seem to be having a similar-ish problem
I have code that calls fitBounds() via onLoad, and then shortly later via a resize. The first fit is about 10% too far zoomed in, and the second fit is a little better but still wrong. Any later calls subsequent fitBounds() works correctly and consistently correctly.

If I increase the padding from 24 to 36 the zoom in is worse. If I increase the padding to 44 then things get a lot better but still inconsistent with any subsequent fitBounds. But if the padding is 48 the second fit via resize() is perfectly consistent and correct with all subsequent calls. So for some reason 48 pixels is a magic number...

I'm could potentially set up a test case but it will be a bit tricky

@benmcginnis
Copy link

I'm having a similar issue, which I've demonstrated in this codepen: https://codepen.io/mcginnisb/pen/rGPNPW?editors=0011 (requires mapbox token)

It just uses this example: https://www.mapbox.com/mapbox-gl-js/example/custom-marker-icons/

and tries to offset the map by the height of the form. I think the zoom issue gets exacerbated when you have a very tight map that should be able to fit all the markers but you get a "

"Map cannot fit within canvas with the given bounds, padding, and/or offset." warning.

It happens at a window height of around 240px.

@benmcginnis
Copy link

I actually think I figured out my issue. If I provide an offset in actual pixels, it's too big and triggers the error, but if I divide the offset.top by the result of map.getZoom() it works perfectly, but is that the intended usage?

@GunnarSturla
Copy link
Contributor

I also had the same problem (as @bryanjjohnson ). It seems to become more noticeable when the paddingOffset gets bigger.

I think the problem might be that the offset should be divided by two, in order to find the offset from center. By adding the difference between padding (left - right) to the offset, the center gets shifted too far (by the whole "reduction" of available area, rather than just half). Does that make sense?

Anyway, I think the fix is an easy one; divide the padding offset by two:

const paddingOffset = [(options.padding.left - options.padding.right) / 2, (options.padding.top - options.padding.bottom) / 2],

Here is the original example that uses a fork with the fix: http://jsbin.com/rihunorico/edit?html,output

@benmcginnis
Copy link

I should clarify my last comment. It doesn't quite "work perfectly" but it definitely helped. Not sure precisely what the math problem is.

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

Successfully merging a pull request may close this issue.

5 participants