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

Rotation bugfix. #982

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rotation bugfix. #982

wants to merge 1 commit into from

Conversation

atsar
Copy link

@atsar atsar commented May 18, 2016

No description provided.

@runspired
Copy link
Contributor

As this is a reversal of atsar@287720a I'd like more info on what the perceived bug is.

@atsar
Copy link
Author

atsar commented Jul 29, 2016

The getRotation function returns the change of the angle since the rotation start. When you land two fingers on the screen, it gives start[0] and start[1] points that determine the orientation relative to the screen. Then, when you move them, it gives end[0] and end[1] - new current finger locations. So, the rotation is the difference (minus) between the end orientation and the start orientation. If you put plus there, it simply gives incorrect number.
Imagine that you just start the rotation. Then end=start, and the rotation should be zero. Having plus in the formula gives you the doubled angle between the line connecting your fingers and the screen horizontal, which is meaningless.

@arschmitz
Copy link
Contributor

arschmitz commented Aug 2, 2016

@runspired the logic in this PR is correct let me explain.

the formula to get the angle relative to the screen for 2 points is
angle = atan2( y2 - y1, x2 - x1 ) * 180 / Math.PI

No lets take the very simple case of our starting points being at 1,1 and 3,1 using the formula above this gives us a starting angle of 0

No we move the second point to 3,3 this is a 45 degree rotation and this is confirmed using the formula above. Since we started at 0 and end at 45 45 - 0 = 45 so the total rotation is 45 which is also the result of the current formula 45 + 0 = 45 This is correct since we rotated in the positive direction.

This can also be confirmed in the negative or counterclockwise direction. Again starting at 1,1 and 3,1 this time we move the first point to 1,3 and leave the second one in place. The above formula gives us an angle of -45 then subtracting the 2 angles we get -45 - 0 = -45 which is correct since we rotated in the negative direction this time.

Where the current formula fails to work is in a non zero case. take starting points of 1,1 and and 3,3 and ending points of 1,1 and 1,3 the angles for these points is 45 and 90 respectively the current formula would give 90 + 45 = 135 which is clearly wrong since we only rotated 45 degrees in the positive direction. the new formula would yield 90 - 45 = 45 which is the correct result.

@arschmitz arschmitz closed this Aug 2, 2016
@arschmitz arschmitz reopened this Aug 2, 2016
@arschmitz
Copy link
Contributor

looking back at 287720a this is not a regression of #791 which it claims to fix but rather #791 was never fixed and this commit was just not correct.

@runspired
Copy link
Contributor

I would like to get this merged @arschmitz I think it resolves the 90deg jump issue.

@arschmitz
Copy link
Contributor

arschmitz commented Oct 19, 2016

@runspired sounds good to me based on my comment above i think this is completely correct we just need to get the conflict resolved

@sebastianrothe
Copy link

whats the status on this merge ?

@bobrosoft
Copy link

@arschmitz hey there! Almost a year has passed. Can it be merged? Need it resolved.

@magicznyleszek
Copy link

@arschmitz is this project dead?

@laborc8
Copy link

laborc8 commented Mar 8, 2019

I had big problems with the pinch and rotate example on the webiste. No solution really worked.
But this bugfix totally solved it.
So for everyone who is looking for a bugfix for the pinch and rotate problem (jumping the moment you touch aso...) use this bugfix - just change the + to a minus and it works

squadette added a commit to squadette/hammer.js that referenced this pull request Aug 24, 2019
@squadette
Copy link

This has been applied in proposed 2.1.0: squadette#1

@arschmitz, thank you for the detailed explanation.

@asturur
Copy link

asturur commented Dec 27, 2019

Any chance to merge this fix if we open a pr with conflict solved?

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.

9 participants