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

Camera Pan Fix #120

Merged
merged 18 commits into from
Feb 24, 2021
Merged

Camera Pan Fix #120

merged 18 commits into from
Feb 24, 2021

Conversation

neelmistry94
Copy link
Contributor

PRs must be submitted under the terms of our Contributor License Agreement CLA.

Fixes: #59

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'

Summary of changes

This PR provides a stop gap solution to account for pitched maps when panning. The logic is the same that is provided in Android to account for panning flings.

PanningStopGap

@neelmistry94 neelmistry94 added the bug 🪲 Something is broken! label Feb 18, 2021
@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@a53b6d1). Click here to learn what that means.
The diff coverage is 47.05%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #120   +/-   ##
=======================================
  Coverage        ?   48.95%           
=======================================
  Files           ?      101           
  Lines           ?     5689           
  Branches        ?        0           
=======================================
  Hits            ?     2785           
  Misses          ?     2904           
  Partials        ?        0           
Flag Coverage Δ
unit-tests 48.95% <47.05%> (?)

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

Impacted Files Coverage Δ
...psGestures/GestureHandlers/PanGestureHandler.swift 64.28% <0.00%> (ø)
Mapbox/MapboxMapsGestures/GestureManager.swift 47.55% <0.00%> (ø)
...ox/MapboxMapsFoundation/Camera/CameraManager.swift 75.87% <61.53%> (ø)

*/
func shiftCenterCoordinate(by offset: CGPoint) -> CLLocationCoordinate2D {
func shiftCenterCoordinate(by offset: CGPoint, didFling: Bool) -> CLLocationCoordinate2D {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename to drift since that's what we've been using elsewhere.

pitch == 0.0 {
self.cameraManager.moveCamera(by: offset, rotation: nil, pitch: nil, zoom: nil, animated: true)
}
self.cameraManager.moveCamera(by: offset, rotation: nil, pitch: nil, zoom: nil, animated: true, didFling: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always true? The pan can end without drift too.

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, from what I saw in the existing behavior and my understanding of hydrogen, all panEnded has the potential to end with a drift and we should account for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, really? Not sure if that's true. Would be worth a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would we test this?

@@ -464,13 +466,25 @@ public class CameraManager {
return CLLocationCoordinate2D(latitude: 0, longitude: 0)
}

var pitchFactor: CGFloat = mapView.pitch
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to get an explanation of what this math is trying to do in the comments here.

*/
func shiftCenterCoordinate(by offset: CGPoint) -> CLLocationCoordinate2D {
func shiftCenterCoordinate(by offset: CGPoint, drift: Bool) -> CLLocationCoordinate2D {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is inaccurate, isn't it? This drift property only does something if the map is pitched it seems like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, so if the shift is reflecting a drift, we need to account for pitch in the code. What would be a more appropriate fix here?

@neelmistry94 neelmistry94 merged commit 8e849d3 into main Feb 24, 2021
@neelmistry94 neelmistry94 deleted the nm/camera_pan_fix_2 branch February 24, 2021 20:50
OdNairy pushed a commit that referenced this pull request Aug 22, 2023
Create end-to-end debuggable environment
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.

Camera jumps to different locations when panning
2 participants