Skip to content

Conversation

@gujjwal00
Copy link
Contributor

Related: #59

This PR removes the jerky behavior of scaling operation.

double fs = Math.sqrt(xfs * xfs + yfs * yfs);
if (Math.abs(1.0 - detector.getScaleFactor())<0.02)
consumed = false;
if (fs * 2< Math.abs(detector.getCurrentSpan() - detector.getPreviousSpan()))
Copy link
Owner

Choose a reason for hiding this comment

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

any idea why it was that way @gujjwal00 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is handling two separate issues:

  • During scaling, onScale callback can be invoked by Android for very small amount of scaling factor. Returning false from these calls will cause them to accumulate. They are handled once they are large enough (± .02). This is what consumed is used for. This PR reduces the delta to ± .01
  • Variables inScaling, xInitialFocus, yInitialFocus, fs etc. and the check on line 68 are used to differentiate between 'Pinch' & 'Two-finger Swipe/Scroll'. For pinch gesture, focus shift (fs) would be less then the change in finger span (detector.getCurrentSpan() - detector.getPreviousSpan()). This PR removes this logic and lets Android differentiate between these gestures.

I think we can allow smooth scaling even if we want to keep the 2nd behavior. But there will always be edge cases because these two gestures can sometimes be ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a second thought, let me look more into this tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

OK. I was asking out of curiosity. If the new code works, I'm fine with it.

@bk138 bk138 self-assigned this Jun 9, 2020
@gujjwal00
Copy link
Contributor Author

Root of the issue is that some two finger swipe gestures are interpreted as 'Pinch' & 'Scroll' both at the same time.

Current scaling code tries to detect this and stops scaling if it thinks that gesture is 'Scroll' . That's why zooming is jerky. This PR removes that check from scaling code so that scaling is smoother for 'Pinch' gestures.

On 'Scroll' side, we were triggering two-finger fling even if both fingers were moving in different directions. This PR adds a check to ignore such gestures so there should be less false-positive fling gestures (Arguably, this is more of a hack rather than a clean solution).

IMO we can handle these gestures cleanly if we were to 'pan' on two-finger scroll but that's out of scope for this PR and I will defer that discussion to #58 . As for this PR, it is ready to merge.

@bk138
Copy link
Owner

bk138 commented Jun 18, 2020

While scaling is super smooth now, this broke the two-finger fling gesture detection user for injecting cursor events LeftRightUpDown at https://github.com/bk138/multivnc/blob/master/android/app/src/main/java/com/coboltforge/dontmind/multivnc/VncCanvasActivity.java#L202 ☹️

@gujjwal00
Copy link
Contributor Author

I have only made the detection more strict. Is it not working entirely for you?

@bk138
Copy link
Owner

bk138 commented Jun 18, 2020

No two-finger fling at all anymore :-/ Using a Galaxy S7, Android 8.1.

@bk138
Copy link
Owner

bk138 commented Jun 19, 2020

I'll have a look ASAP! Thanks!

@gujjwal00
Copy link
Contributor Author

gujjwal00 commented Jun 19, 2020

I tested with two other devices but it is working for me.

I have removed changes related to fling detection from this PR. @bk138 can you please check again? oops!

@bk138
Copy link
Owner

bk138 commented Jun 19, 2020

Still no luck: both two-finger fling and three-finger panning don't work with this branch, tested with a Galaxy S7 and a S10. Any idea where to tinker?

@gujjwal00
Copy link
Contributor Author

gujjwal00 commented Jun 20, 2020

Can you please try reverting commit df9331d ? Maybe Samsung has patched ScaleGestureDetector.

@bk138
Copy link
Owner

bk138 commented Jun 21, 2020

Can you please try reverting commit df9331d ? Maybe Samsung has patched ScaleGestureDetector.

Same :-/. In fact, 08b52e7 is enough to break 2-finger-fling and 3-finger pan.

@gujjwal00
Copy link
Contributor Author

gujjwal00 commented Jun 21, 2020

Thanks for checking!

I am guessing that ScaleGestureDetector on the mentioned device is triggering the scale gesture a bit too early. After scale gesture is started we give preference to scaling over scrolling. So I have two possible solutions:

  1. Added couple of commits to current PR which makes our scroll gesture handling independent of scaling.
  2. Created a separate branch smooth-scaling-alt with different implementation of smooth scaling.

Can you please take a look? I have already taken too much of your time.

@bk138
Copy link
Owner

bk138 commented Jun 25, 2020

This PR here now works much better in that fling gestures are now detected. However, I still get a minimal simultaneous scale as well, by ~ 10%.

@bk138
Copy link
Owner

bk138 commented Jun 25, 2020

And smooth-scaling-alt works perfectly, at least for me 🎉

Do you want to change anything in that branch or is it ready to be merged?

Thanks for the hard work, again!

@gujjwal00 gujjwal00 closed this Jun 25, 2020
@gujjwal00 gujjwal00 reopened this Jun 25, 2020
@gujjwal00
Copy link
Contributor Author

Glad to know its working! 🙂

However, I still get a minimal simultaneous scale as well, by ~ 10%.

Yeah, that is expected.

Do you want to change anything in that branch or is it ready to be merged?

Let me just add few comments in the code.
I have couple of questions but they can be discussed in a separate issue.

@gujjwal00
Copy link
Contributor Author

Closing in favor of #72

@gujjwal00 gujjwal00 closed this Jun 25, 2020
@gujjwal00 gujjwal00 deleted the smooth-scaling-3 branch April 3, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants