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

When fitBounds() is called with nonzero bearing, rotate padding #9821

Merged
merged 6 commits into from
Aug 26, 2020

Conversation

allison-strandberg
Copy link
Contributor

@allison-strandberg allison-strandberg commented Jun 24, 2020

Fixes #9641

This is my first PR here—let me know if I'm missing anything!

This PR changes the behavior of fitBounds(). Previously, when fitBounds() was called with asymmetrical padding and a nonzero bearing, the padding was applied without considering the map's rotation. For example, if the bearing was 90 degrees, a left padding value would be applied to the bottom of the map. Now, left padding is applied to the left side of the map regardless of the direction of north.

There are two main changes:

  • Allow a nonzero bearing to be passed from cameraForBounds(). Previously, the bearing passed to _cameraForBoxAndBearing() from this function was hardcoded as 0. This was the subject of PR Allow passing bearing as an option ot fitBounds #7618, which was not merged: I copied tests from that PR.
  • In _cameraForBoxAndBearing(), rotate both the offset and padding values by the bearing.

The following JSFiddles demonstrate the change. The "Fit to Kenya" button calls fitBounds with a bearing of 90 degrees and a left padding of 200 pixels.

Using v1.11.0: Padding is on the bottom.

Using this fork: Padding is on the left.

Questions

  1. In _cameraForBoxAndBearing(), the offset and padding are rotated, added together, then scaled by the zoom. Is this order correct?
  2. Are benchmark scores necessary for this PR?
  3. How do I apply a changelog label? Is including the string in the launch checklist's backticked changelog tag the right way to add a changelog entry?

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>When fitBounds() is called with nonzero bearing, rotate padding</changelog>

@ryanhamley ryanhamley requested a review from arindam1993 June 24, 2020 18:13
@waissbluth
Copy link

@arindam1993 please take a look at this when you have a chance. thanks!

@ryanhamley
Copy link
Contributor

@allison-strandberg thanks so much for the detailed PR (and for your patience)! This looks great to me and seems to work well in the linked example.

In _cameraForBoxAndBearing(), the offset and padding are rotated, added together, then scaled by the zoom. Is this order correct?

@arindam1993 can you confirm this?

Are benchmark scores necessary for this PR?

No, this shouldn't affect performance much.

How do I apply a changelog label? Is including the string in the launch checklist's backticked changelog tag the right way to add a changelog entry?

Yes, you added the changelog entry already. We also add a label such as "feature" or "bug" but I'll do that now.

@allison-strandberg
Copy link
Contributor Author

@arindam1993 do you have a timeline on reviewing this? thanks!

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @allison-strandberg , could you please describe in more detail why you think the offset should be rotated in more detail?

src/ui/camera.js Outdated Show resolved Hide resolved
@allison-strandberg
Copy link
Contributor Author

@arindam1993 I've submitted a change so that the offset is no longer rotated: does this look good to you?

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in looking at this, thanks this looks good to me now. 🙇 Thanks for working on this

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fitBounds() with bearing and uneven padding causes unexpected results
5 participants