Fix map.fitBounds() with non-zero bearing #11568
Conversation
…thods: * `map.fitBounds()` * `map.fitScreenCoordinates()` * `map.cameraForBounds()` Consider 45 and 135 rotations and how all four corners need to be taken into account. Changes in tests: * Fix tests for camera => #cameraForBounds (they were incorrect) * Add new test for camera => #fitBounds with non-zero bearing * Changed input coordinates in tests camera => #fitScreenCoordinates to make the input region not square
|
|
There was a problem hiding this comment.
Thanks for your work on this @TannerPerrien! I tested this out and it seems to work.
I've added a few small suggestions for code styling.
Another nice touch would be pushing a change to padding.html to make manual testing easier. Perhaps the call to fitBounds should preserve the current map bearing?
src/ui/camera.js
Outdated
| const p0world = tr.project(p0LatLng); | ||
| const p1world = tr.project(p1LatLng); | ||
| const p2world = tr.project(new LngLat(p0LatLng.lng, p1LatLng.lat)); | ||
| const p3world = tr.project(new LngLat(p1LatLng.lng, p0LatLng.lat)); |
There was a problem hiding this comment.
Nit: I think this can be simplified by calculating the latter two points in world coordinates, i.e.:
| const p3world = tr.project(new LngLat(p1LatLng.lng, p0LatLng.lat)); | |
| const p3world = new Point(p1world.x, p0world.y); |
There was a problem hiding this comment.
Much better. Thanks!
src/ui/camera.js
Outdated
| const upperRight = new Point(Math.max(p0rotated.x, p1rotated.x), Math.max(p0rotated.y, p1rotated.y)); | ||
| const lowerLeft = new Point(Math.min(p0rotated.x, p1rotated.x), Math.min(p0rotated.y, p1rotated.y)); | ||
| const p2rotated = p2world.rotate(-bearing * Math.PI / 180); | ||
| const p3rotated = p3world.rotate(-bearing * Math.PI / 180); |
There was a problem hiding this comment.
Nit: Can we keep using the degToRad helper function for consistency here?
* Updated padding.html: added a control to toggle if the `fitBounds` should use the map's current bearing
src/ui/camera.js
Outdated
| const upperRight = new Point(Math.max(p0rotated.x, p1rotated.x), Math.max(p0rotated.y, p1rotated.y)); | ||
| const lowerLeft = new Point(Math.min(p0rotated.x, p1rotated.x), Math.min(p0rotated.y, p1rotated.y)); | ||
| const p2rotated = p2world.rotate(-degToRad(bearing)); | ||
| const p3rotated = p3world.rotate(-degToRad(bearing)); |
There was a problem hiding this comment.
I think that -degToRad(bearing) should be pulled into its own variable.
There was a problem hiding this comment.
Good call. Apologies for the slop!
Fix bounds/bearing calculation bug (#10064) in the following methods:
map.fitBounds()map.fitScreenCoordinates()map.cameraForBounds()Consider bearings of 45 and 135 and how all four corners need to be taken into account.
Changes in tests:
Before
After
Launch Checklist
mapbox-gl-jschangelog:<changelog>Fix bounds calculation where map.fitBounds(), map.fitScreenCoordinates(), and map.cameraForBounds() behave incorrectly with non-zero bearing. (#10064)</changelog>