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

Prevent pitch and zoom from exceeding limits #103

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

macdrevx
Copy link
Contributor

@macdrevx macdrevx commented Feb 12, 2021

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-ios changelog: <changelog>Prevent pitch and zoom from exceeding limits. Also updates default maximum pitch to 85 degrees.</changelog>.

Summary of changes

  • Consolidates logic from setCamera(centerCoordinate:padding:anchor:zoom:bearing:pitch:animated:duration:completion:) into setCamera(to:animated:duration:completion:)
  • Clamps pitch to min and max from MapCameraOptions
  • Refactors the clamp helper into an extension on Comparable that uses a ClosedRange parameter
  • Improves testSetCamera() which had several assertions that checked for "changes" that were identical to the initial values.
  • Updates default maximumPitch to 85 degrees

User impact (optional)

When continuing to use the pitch gesture after reaching the maximum or minimum pitch, the map will no longer require you to "undo" those superfluous gestures to reverse the pitch.

@macdrevx macdrevx added the bug 🪲 Something is broken! label Feb 12, 2021
@@ -165,7 +165,7 @@ public class CameraManager {
- Parameter completion: The completion block to execute after the transition has occurred.
*/

public func setCamera(to newCamera: CameraOptions,
public func setCamera(to camera: CameraOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

@julianrex we spoke about cleaning up these methods. But with this change to only having this single setCamera method. It removes the ability to opt out of optimizing bearing. We should probably add that logic here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on discussion with @julianrex we'll move forward with this change and revisit adding an option to opt-out of bearing optimization.

@macdrevx macdrevx force-pushed the ah/enforce-pitch-and-zoom-limits branch from 443bba0 to d50e767 Compare February 13, 2021 01:36
anchor: camera.anchor,
zoom: camera.zoom?.clamped(to: mapCameraOptions.minimumZoomLevel...mapCameraOptions.maximumZoomLevel),
bearing: optimizeBearing(startBearing: mapView.bearing, endBearing: camera.bearing),
pitch: camera.pitch?.clamped(to: mapCameraOptions.minimumPitch...mapCameraOptions.maximumPitch))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also increase the default maximumPitch value in MapCameraOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. What should I change it to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 85. cc: @mapbox/maps-android

@@ -0,0 +1,11 @@
internal extension Comparable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nice!

@macdrevx macdrevx force-pushed the ah/enforce-pitch-and-zoom-limits branch from d50e767 to de71fa1 Compare February 15, 2021 18:12
@macdrevx macdrevx enabled auto-merge (squash) February 15, 2021 18:29
@macdrevx macdrevx merged commit 19dac6b into main Feb 15, 2021
@codecov
Copy link

codecov bot commented Feb 15, 2021

Codecov Report

Merging #103 (de71fa1) into main (26c1445) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   48.81%   48.88%   +0.07%     
==========================================
  Files         100      101       +1     
  Lines        5607     5605       -2     
==========================================
+ Hits         2737     2740       +3     
+ Misses       2870     2865       -5     
Flag Coverage Δ
unit-tests 48.88% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...MapboxMapsFoundation/Camera/MapCameraOptions.swift 100.00% <ø> (ø)
...ox/MapboxMapsFoundation/Camera/CameraManager.swift 76.71% <100.00%> (+0.98%) ⬆️
...apboxMapsFoundation/Camera/FlyToInterpolator.swift 88.11% <100.00%> (-0.17%) ⬇️
...MapsFoundation/Extensions/Comparable+Clamped.swift 100.00% <100.00%> (ø)

@macdrevx macdrevx deleted the ah/enforce-pitch-and-zoom-limits branch February 15, 2021 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something is broken!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants