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

Allow passing bearing as an option ot fitBounds #7618

Closed

Conversation

dentuzhik
Copy link

@dentuzhik dentuzhik commented Nov 23, 2018

Launch Checklist

I know there have been other attempts to get it to fitBounds, but I got it working fairly easiy with a following snippet of code (react based), hence decide to file a PR.

mapInstance._fitInternal(
  mapInstance._cameraForBoxAndBearing(
    bounds.getNorthWest(),
    bounds.getSouthEast(),
    mapInstance.getBearing(),
    {},
  ),
);

I decided not to go with all the other checklist items, before getting any feedback, but surely can add docs and tests if necessary.

Previous art:
#1340
#1338

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good apart from the following:

  • This will fail when no options are passed to cameraForBounds — that's why the build fails.
  • You could write this in a much shorter way with:
const bearing = options && options.bearing || 0;

@mvriel
Copy link

mvriel commented Feb 25, 2019

Mentioning @dentuzhik in the hopes he will incorporate the suggested changes :)

@dentuzhik dentuzhik force-pushed the feature/fitbounds-allow-bearing branch from 4ca7dae to 835a90a Compare March 1, 2019 08:40
@dentuzhik
Copy link
Author

Not sure why occasionally test-flow fails, tried locally on my machine, it passes:
image

Anyway I've fixed an issue with passing no options, and added a warning for passing only numeric value. Plus some trivial test cases to cover that. I didn't quite understand why setting bearing changes transform zoom value, but since it seems like it's out of scope of this PR I have adapted tests.

Wasn't sure about perf benchmarks in this case, seems unnecessary

@dentuzhik
Copy link
Author

In regards to shorter way of writing, decided to keep it unchanged in favour of adding a warning about bearing being numeric, hope it's ok @mourner

@mvriel
Copy link

mvriel commented Mar 1, 2019

An interesting observation I made when testing with workarounds in our product is that outcome of the calculations in _cameraForBoxAndBearing are not correct when a bearing and pitch is provided. With a bearing of 0 and a bound determined with 2 LatLng coordinates it exactly fits in the viewport but when you apply a bearing it frequently happens that the coordinates are outside of the viewport.

Same goes when you have a pitch; the pitch makes the bounding box a trapezium shape instead of a rectangle and the _cameraForBoxAndBearing does not (and cannot) compensate for this

@mvriel
Copy link

mvriel commented Mar 1, 2019

To give an idea of the errors that we encounter:

image

@dentuzhik
Copy link
Author

@mvriel it seems that it deserves a separate issue and PR. Zoom level changes based on bearing change also seems suspicious, but let's see what @mourner says

@asheemmamoowala
Copy link
Contributor

Same goes when you have a pitch; the pitch makes the bounding box a trapezium shape instead of a rectangle and the _cameraForBoxAndBearing does not (and cannot) compensate for this

_cameraForBoxAndBearing does not take into account the pitch when computing the camera. This is pending implementation.

@asheemmamoowala
Copy link
Contributor

Closing this as stale. @dentuzhik feel free to re-open the PR when you've had a chance to address the issues mentioned at #7618 (review)

@spencerthayer
Copy link

Has this been resolved?

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.

5 participants