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 double padding on camera when setting bounds #3354

Merged
merged 10 commits into from
Feb 13, 2024

Conversation

naftalibeder
Copy link
Collaborator

This fixes a configuration like

// Map height: 800
setCamera({
  bounds: ...,
  padding: {
    paddingTop: 0,
    paddingBottom: 400,
  },
})

producing the warning

Unable to calculate camera for given bounds/geometry, padding is greater than map's width or height

due to the center and zoom already including the calculated padding, and then the padding being reapplied in the camera configuration.

I believe this has always been an issue, but it's become problematic in Mapbox 11, as the internal camera now fails to move if total padding exceeds the map size.

I also went ahead and added a condition that scales down the padding so it doesn't ever exceed the map size. What do you think about this? My thinking is: when working with the native Mapbox SDK, you can easily check e.g:

if padding.top + padding.bottom >= map.bounds.height { ... }

but it's not as easy in JavaScript to get the current map height, so that kind of check is more brittle.

Ideally, this sort of overflow would be compensated for in the Mapbox SDK, but I'm not sure if I would consider it a bug vs. just a design choice I don't like. :)

Comment on lines 380 to 386
if let _minNS = minZoomLevel, let _min = CGFloat(exactly: _minNS), let _zoom = zoom, _zoom < _min {
zoom = _min
}
if let _maxNS = maxZoomLevel, let _max = CGFloat(exactly: _maxNS), let _zoom = zoom, _zoom < _max {
zoom = _max
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clamping logic doesn't actually have an effect - the map automatically restricts according to the min/max zoom settings.

@naftalibeder
Copy link
Collaborator Author

@mfazekas I left a couple of comments on the code.

On another note, there's some idiosyncratic behavior depending on the combination of padding and min/max zoom limits. This exists in the native SDK, so it's probably not something we need to deal with here, but I wanted to check.

Basically, with no minZoomLevel defined, increasing the padding can result in a massive zoom out, as the bounding box gets "squished" to a single point. But with minZoomLevel set, it becomes possible to increase the padding such that the bounds move completely offscreen. I think a reasonable expectation would be for the map to always show the provided bounds, scaling down padding if needed. This seems like a bug, but I'm not actually sure it would be considered one.

The video should help clarify. Thoughts?

zoom-limits.mov

@mfazekas
Copy link
Contributor

mfazekas commented Feb 8, 2024

@naftalibeder ci needs a fix here

@mfazekas
Copy link
Contributor

mfazekas commented Feb 9, 2024

@naftalibeder pls merge the PR as CI was broken on main

@mfazekas
Copy link
Contributor

CI error:

| Error: src/examples/V10/CameraAnimation.tsx(1,43): error TS6133: 'color' is declared but its value is never read.

@mfazekas mfazekas merged commit 24edfeb into rnmapbox:main Feb 13, 2024
10 checks passed
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.

2 participants