Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Option to change the zoom rate. Decrease the default zoom rate by 35% #14774

Merged
merged 2 commits into from
May 29, 2019

Conversation

LukasPaczos
Copy link
Contributor

Closes #14136.

Additionally, changes the default zoom rate by 35% based on community feedback. I'd really appreciate any additional feedback on this change (b03c151) @mapbox/maps-android.

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label May 27, 2019
@LukasPaczos LukasPaczos added this to the release-oolong milestone May 27, 2019
@LukasPaczos LukasPaczos requested a review from tobrun May 27, 2019 15:44
@tobrun
Copy link
Member

tobrun commented May 27, 2019

Note sure if it's related to this change but this is the first time reproducing the following behavior below:

ezgif com-video-to-gif (70)

Some pinch gestures are being picked up as a scroll gesture

@tobrun
Copy link
Member

tobrun commented May 27, 2019

I compared our default zoom rate against a competitor and they seem to have an even lower zoom rate:
Below images were captured starting from a reference point (main square) and pinching out (without triggering veloctity + allmost full screen gesture).

image

image

@LukasPaczos
Copy link
Contributor Author

Some pinch gestures are being picked up as a scroll gesture

On the gif I see that when the scroll happens, you've started with one of the pointers outside of the view bounds (once on the toolbar and once on the system bar), could you retest and see if that might've been the cause (which would've been working as expected)?

... they seem to have an even lower zoom rate

Would you suggest to decrease the default rate even more?

@tobrun
Copy link
Member

tobrun commented May 28, 2019

On the gif I see that when the scroll happens, you've started with one of the pointers outside of the view bounds (once on the toolbar and once on the system bar), could you retest and see if that might've been the cause (which would've been working as expected)?

lol that will probably be it, ignore that remark.

Would you suggest to decrease the default rate even more?

I think if we want, we have a case for it. I would try to see what feels most fluent. During development, we do a lot of worldview to high level zoom zooming but irl you don't really do that. That said, if a developer wants to decrease the zoom rate, they can with proposed configuration so 🤷‍♂️

@@ -558,7 +559,7 @@ private double calculateScale(double velocityXY, boolean isScalingOut) {
}

private double getNewZoom(float scaleFactor, boolean quickZoom) {
double zoomBy = Math.log(scaleFactor) / Math.log(Math.PI / 2);
double zoomBy = (Math.log(scaleFactor) / Math.log(Math.PI / 2)) * 0.65 * uiSettings.getZoomRate();
Copy link
Member

Choose a reason for hiding this comment

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

make 0.65 a constant and name it so it explains its purpose.

*
* @param zoomRate The zoom rate.
*/
public void setZoomRate(float zoomRate) {
Copy link
Member

Choose a reason for hiding this comment

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

are only positive numbers supported? if yes, can we add something as @FloatRange(from = 0.0f)

@LukasPaczos LukasPaczos force-pushed the lp-zoom-rate-14136 branch from b508218 to 8faf9e1 Compare May 28, 2019 12:26
@LukasPaczos LukasPaczos force-pushed the lp-zoom-rate-14136 branch from 8faf9e1 to 865f378 Compare May 28, 2019 12:37
@LukasPaczos LukasPaczos merged commit 5bb0a49 into master May 29, 2019
@LukasPaczos LukasPaczos deleted the lp-zoom-rate-14136 branch May 29, 2019 08:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setZoomRate for android?
2 participants