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

don't trigger a zoom by 0 #1990

Merged
merged 1 commit into from
Feb 1, 2016
Merged

don't trigger a zoom by 0 #1990

merged 1 commit into from
Feb 1, 2016

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jan 25, 2016

Touching a trackpad with two fingers was triggering a zoom by 0, which was making icons jump around (switching from nearest neighbour to linear interpolation).

look ok @lucaswoj?

@@ -97,6 +97,7 @@ ScrollZoom.prototype = {
},

_zoom: function (delta) {
if (delta === 0) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be more robust as delta < EPSILON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, but I don't think it matters in this particular case

Copy link
Contributor

Choose a reason for hiding this comment

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

Being more robust always matters! It might fix similar problems on other platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you thinking about this from a "floating point issues may cause this check to fail" point of view or a "some legitimately non-zero but still tiny value will trigger a switch to linear but not move the map" view?

Copy link
Contributor

Choose a reason for hiding this comment

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

Touching a trackpad with two fingers was triggering a zoom by 0, which was making icons jump around (switching from nearest neighbour to linear interpolation).

I was thinking that another platform might trigger a zoom by 0.000001 in this scenario

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very concerned, just wondering what your thoughts are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I don't have a strong opinion either way, I'm just trying to figure out / learn if that actually makes sense to do this

I'm thinking that since there will always be a cutoff, whether it's epsilon or 0, some values that could be skipped might not be. I think epsilon would be more arbitrary than 0 because it would be just a blind guess. What makes 1 * epsilon a better cutoff than 2 * epsilon or 0.5 * epsilon? What makes 1 * epsilon better than 0 * epsilon?

It would make sense to use a non-zero value if we had real examples, but I don't know if a blind guess for a problem we haven't seen actually improves anything. It seems like it just makes everything a tiny bit more complicated

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Ship it with 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, #1832 covers introducing ε for pan and rotation, but should probably be extended to zoom too.

Touching a trackpad with two fingers was triggering a zoom by 0, which
was making icons jump around (switching from nearest neighbour to
linear interpolation).
@ansis ansis merged commit 50ebed9 into master Feb 1, 2016
@ansis ansis removed the in progress label Feb 1, 2016
@ansis ansis deleted the zoom-by-zero branch February 1, 2016 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants