-
Notifications
You must be signed in to change notification settings - Fork 150
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
Zoom focal point #1122
Zoom focal point #1122
Conversation
Sources/MapboxMaps/Gestures/GestureHandlers/DoubleTapToZoomInGestureHandler.swift
Outdated
Show resolved
Hide resolved
Sources/MapboxMaps/Gestures/GestureHandlers/DoubleTapToZoomInGestureHandler.swift
Outdated
Show resolved
Hide resolved
Sources/MapboxMaps/Gestures/GestureHandlers/PinchGestureHandlerImpl1.swift
Outdated
Show resolved
Hide resolved
Sources/MapboxMaps/Gestures/GestureHandlers/ZoomGestureHandlerProtocol.swift
Outdated
Show resolved
Hide resolved
I'm working on doing away with the two pinch gesture implementations. It probably won't land until later this week or early next week, but I wanted to give you a heads-up about it since pan impl 2 is going away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holding further feedback until we discuss these initial comments.
Sources/MapboxMaps/Gestures/GestureHandlers/QuickZoomGestureHandler.swift
Outdated
Show resolved
Hide resolved
b8bc6cf
to
599f725
Compare
Sources/MapboxMaps/Gestures/GestureHandlers/PinchBehaviors/PinchBehaviorProvider.swift
Outdated
Show resolved
Hide resolved
/// By default, gestures rotate and zoom around the center of the gesture. Set this property to rotate and zoom around a fixed point instead. | ||
/// | ||
/// In order to force the map to only revolve around the specified focal point, disable | ||
/// ``GestureOptions/pinchPanEnabled`` and ``GestureOptions/panEnabled``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to disable GestureOptions.panEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving regular pan enabled will enable the visible portion of the map to be moved relative to the specified focal point. So a completely different location might end up under the focal point instead of the location shown initially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pengdev any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're going to mention panEnabled
it should be in the context of keeping a map coordinate fixed at specific focal point. That's a slightly different use case than forcing rotation and zoom to happen at a specified point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understanding the issue here?
On Android I think we have a focalPoint variable in the CameraAnimations plugin, where gestures in some cases cache the original focalPoint during certain gestures and restore the original focalPoint when the gesture is finished.
cc @kiryldz for more context on how we handle focal point on camera animations and gestures plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After further debate, we've decided to allow gesture options to have pinchPanEnabled == true
and focalPoint != nil
simultaneously. In this case, focalPoint
will be ignored for the pinch gesture, but it will still apply to the zoom gestures. When pinch gesture starts, we'll log a warning to inform developers that they may want to set pinchPanEnabled = false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it so 👍
Sources/MapboxMaps/Gestures/GestureHandlers/PinchGestureHandler.swift
Outdated
Show resolved
Hide resolved
Tests/MapboxMapsTests/Gestures/GestureHandlers/Mocks/MockDoubleTapToZoomInGestureHandler.swift
Outdated
Show resolved
Hide resolved
Tests/MapboxMapsTests/Gestures/GestureHandlers/DoubleTapToZoomInGestureHandlerTests.swift
Outdated
Show resolved
Hide resolved
Tests/MapboxMapsTests/Gestures/GestureHandlers/DoubleTapToZoomInGestureHandlerTests.swift
Outdated
Show resolved
Hide resolved
Tests/MapboxMapsTests/Gestures/GestureHandlers/PinchBehaviors/PinchBehaviorProviderTests.swift
Outdated
Show resolved
Hide resolved
let secondFocalPoint = CGPoint.random() | ||
doubleTapToZoomInGestureHandler.focalPoint = secondFocalPoint | ||
|
||
XCTAssertEqual(gestureManager.options.focalPoint, doubleTapToZoomInGestureHandler.focalPoint) | ||
|
||
doubleTapToZoomInGestureHandler.focalPoint = nil | ||
|
||
XCTAssertNil(gestureManager.options.focalPoint) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend omitting this aspect of the test. Unlike setting isEnabled
on a publicly-exposed gesture recognizer, we don't expose a way for developers to modify focalPoint on doubleTapToZoomInGestureHandler — it's just an implementation detail.
The other part of the test is the important part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it's not fully removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it should be 🤞
CHANGELOG.md
Outdated
* Zoom focal point ([#1122](https://github.com/mapbox/mapbox-maps-ios/pull/1122)) | ||
* Add focalPoint property to zoom and rotate gestures ([#1122](https://github.com/mapbox/mapbox-maps-ios/pull/1122)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: duplicated change log entries for the same PR, probably we want to remove the first one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted, rebase artefacts, removed it.
@@ -1,3 +1,5 @@ | |||
import CoreGraphics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm not familiar with swift, but is it needed as there's no other change in this class besides this import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Xcode has started auto-importing frameworks which is leading to a lot of things like this happening. Probably there were other changes to this file in a previous iteration of this PR which led to this import which was then not reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macdrevx exactly! I'm willing to keep it, Xcode has a point - we do use CoreGrapics
types here.
@@ -1,9 +1,11 @@ | |||
import UIKit | |||
@_implementationOnly import MapboxCommon_Private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import can be removed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one stays, but the one from GestureOptions
goes :)
26f688a
to
75ffef8
Compare
Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>
This PR introduces the ability to override the focal point for zoom gestures. If the focal point is set e.g. to the center of the map, then zoom will anchor on that point, disregarding the actual touch location of any zoom gesture.
List of gestures supporting focal point override:
Fixes #879
Pull request checklist:
## main
heading near the top).