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

Support smooth scrolling on notebook touchpads. #3369

Closed
wants to merge 4 commits into from

Conversation

yume-chan
Copy link
Contributor

@yume-chan yume-chan commented Jul 6, 2022

Fixes #3363

HID mode doesn't support this smooth scrolling, because sending fractional scrolling distance over standard HID mouse protocol seems complicated. I may investigate more in the future (plus horizontal scrolling support).

Only tested on Windows with both mouse and touchpad.

rom1v and others added 4 commits June 17, 2022 08:36
In bash, the variable is set using "export".
Document how to set environment variables from the terminal for bash,
cmd and PowerShell.
// up is positive
vscroll = CLAMP(event->preciseY, -1.0f, 1.0f);
} else {
hscroll = CLAMP(-event->x, -1, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nowadays the horizontal scrolling is actually inverted, so negated x and preciseX.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, looks suspicious. See #966 (comment)

Copy link
Contributor Author

@yume-chan yume-chan Jul 24, 2022

Choose a reason for hiding this comment

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

I have seen that issue before, but this also happens to mouses with horizontal scrolling wheels.

Windows also has a touchpad scrolling direction setting, but whatever it was set, vertical scrolling is always correct and horizontal scrolling is always inverted.

I will test on a mac tomorrow.

Copy link
Contributor Author

@yume-chan yume-chan Jul 25, 2022

Choose a reason for hiding this comment

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

Confirmed it's also inverted on macOS when using a mouse. My mac doesn't has touchpad so can't test that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the value of event->direction in each case?

Concretely, could you list for each case (mouse/touchpad on windows/macOS):

  • event->direction
  • event->x
  • event->y
  • "inverted" or "not inverted"

cc @fsw0422 as the author of #966 (I do not want to break the fix for #966)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I won't be with my mac until Friday, will try that later.

rom1v added a commit that referenced this pull request Jul 24, 2022
So that it can be reused for other fields.

PR #3369 <#3369>
rom1v pushed a commit that referenced this pull request Jul 24, 2022
Since SDL 2.0.18, the amount scrolled horizontally or vertically is
exposed as a float (between 0 and 1).

XX  DO NOT MERGE (YET)  XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
Remaining problem (?) before merging: the x and y value are clamped to
[0; 1], so if on some platforms a scroll event could be triggered with a
value greater than 1 or lower than -1, it is not forwarded correctly
anymore (this is the case both for the int or float versions).
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

Refs <https://wiki.libsdl.org/SDL_MouseWheelEvent>
Fixes #3363 <#3363>
PR #3369 <#3369>

Signed-off-by: Romain Vimont <[email protected]>
rom1v pushed a commit that referenced this pull request Jul 24, 2022
Since SDL 2.0.18, the amount scrolled horizontally or vertically is
exposed as a float (between 0 and 1). Forward a precise value to the
Android device when possible.

Refs <https://wiki.libsdl.org/SDL_MouseWheelEvent>
Fixes #3363 <#3363>
PR #3369 <#3369>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Jul 24, 2022

Thank you 👍 (and sorry for the delay).

I rebased on dev (and squashed your 2 commits), and made the following changes:

  • extracted a function fixedPointToFloat() used for pressure as a preliminary commit before your changes;
  • use this function in your commits;
  • remove the inversion of horizontal scrolling (it looks suspicious to me, cf In Mac OS, I think the natural scrolling doesn't work #966 (comment), please take a look and confirm that it works as expected for you);
  • used conditional compilation for SDL_VERSION_ATLEAST() (otherwise it would not compile with an older SDL version).

I was first annoyed by the CLAMP() between -1 and 1, but the Android side expects a value in this range: AXIS_VSCROLL, so it's better to clamp anyway (even if SDL could trigger values outside this range).

I could not test since on my computer, only -1 or +1 values are generated.

Please review and test this branch: pr3369.1.

@yume-chan yume-chan changed the base branch from master to dev July 24, 2022 15:18
rom1v added a commit that referenced this pull request Jul 26, 2022
For some reason, '%g' does not work correctly with MinGW.

Refs #3369 <#3369>
rom1v added a commit that referenced this pull request Aug 3, 2022
For some reason, '%g' does not work correctly with MinGW.

Refs #3369 <#3369>
PR #3399 <#3399>
rom1v added a commit that referenced this pull request Aug 3, 2022
rom1v added a commit that referenced this pull request Aug 3, 2022
rom1v added a commit that referenced this pull request Aug 3, 2022
rom1v added a commit that referenced this pull request Aug 3, 2022
It will allow to expose more binary util functions not related to
buffers.

PR #3369 <#3369>
rom1v added a commit that referenced this pull request Aug 3, 2022
rom1v added a commit that referenced this pull request Aug 3, 2022
rom1v added a commit that referenced this pull request Aug 3, 2022
To encode float values between -1 and 1.

PR #3369 <#3369>
rom1v pushed a commit that referenced this pull request Aug 3, 2022
Since SDL 2.0.18, the amount scrolled horizontally or vertically is
exposed as a float (between 0 and 1). Forward a precise value to the
Android device when possible.

Refs <https://wiki.libsdl.org/SDL_MouseWheelEvent>
Fixes #3363 <#3363>
PR #3369 <#3369>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Aug 3, 2022

I changed to encode as a signed integer (with a more natural two's complement):

  • -1.0f0x8000
  • -0.5f0xC000
  • 0.0f0x0000
  • 0.5f0x4000
  • 1.0f0x7FFF

I added the necessary util functions and unit tests beforehand, then adapted your commit to use them.

Please review and test pr3369.3.

Thank you 😉

@rom1v
Copy link
Collaborator

rom1v commented Aug 16, 2022

@yume-chan Did you have a change to test/review? (It's not urgent)

@yume-chan
Copy link
Contributor Author

Sorry, I'm still busy working on another project. I always have this on my todo list and will report back once that's finished.

rom1v added a commit that referenced this pull request Aug 28, 2022
rom1v added a commit that referenced this pull request Aug 28, 2022
rom1v added a commit that referenced this pull request Aug 28, 2022
rom1v added a commit that referenced this pull request Aug 28, 2022
It will allow to expose more binary util functions not related to
buffers.

PR #3369 <#3369>
rom1v added a commit that referenced this pull request Aug 28, 2022
rom1v added a commit that referenced this pull request Aug 28, 2022
rom1v added a commit that referenced this pull request Aug 28, 2022
To encode float values between -1 and 1.

PR #3369 <#3369>
rom1v pushed a commit that referenced this pull request Aug 28, 2022
Since SDL 2.0.18, the amount scrolled horizontally or vertically is
exposed as a float (between 0 and 1). Forward a precise value to the
Android device when possible.

Refs <https://wiki.libsdl.org/SDL_MouseWheelEvent>
Fixes #3363 <#3363>
PR #3369 <#3369>

Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Aug 28, 2022

I merged into dev. Everything looks good to me.

@metayan
Copy link

metayan commented Sep 2, 2022

This works great on my Mac, both with touchpad and mouse.
The scrolling is much nicer now.

The version should probably be bumped, because of the incompatible server/client.
When I tried to scroll with mismatched server, I got a lot of

[server] WARN: Unknown event type: -116

(with the number varying for different runs) and then scrcpy would become completely unresponsive. (That the scrolling didn't work was expected, but a little surprising that it didn't recover.)

Also, had to change PLATFORM and BUILD_TOOLS from 31 to 33 in server/build_without_gradle.sh, because otherwise I kept getting

[server] INFO: Device: Sony G8441 (Android 9)
[server] ERROR: Exception on thread Thread[main,5,main]
java.lang.AssertionError: java.lang.NullPointerException: 
 Attempt to invoke virtual method
 'java.lang.reflect.Method java.lang.Class.getMethod(java.lang.String, java.lang.Class[])'
 on a null object reference
        at com.genymobile.scrcpy.wrappers.WindowManager.registerRotationWatcher(WindowManager.java:108)
        at com.genymobile.scrcpy.Device.<init>(Device.java:85)
        at com.genymobile.scrcpy.Server.scrcpy(Server.java:65)
        at com.genymobile.scrcpy.Server.main(Server.java:335)
        at com.android.internal.os.RuntimeInit.nativeFinishInit(Native Method)
        at com.android.internal.os.RuntimeInit.main(RuntimeInit.java:340)
Caused by: java.lang.NullPointerException:
 Attempt to invoke virtual method
 'java.lang.reflect.Method java.lang.Class.getMethod(java.lang.String, java.lang.Class[])'
 on a null object reference
        at com.genymobile.scrcpy.wrappers.WindowManager.registerRotationWatcher(WindowManager.java:102)
        ... 5 more
2022-09-02 13:12:05.549 scrcpy[48321:5695314] DEBUG: Interrupting socket
2022-09-02 13:12:05.549 scrcpy[48321:5695314] DEBUG: Server disconnected
2022-09-02 13:12:05.549 scrcpy[48321:5695314] DEBUG: Server terminated

@yume-chan
Copy link
Contributor Author

@metayan Thanks for the verification!

In addition, can you verify does horizontal scrolling works (not flipped) on both touchpad and mouse (if you have a mouse with horizonal scroll wheel)? You can open the browser app on your phone and pinch out to zoom in, then scroll horizontally.

The version should probably be bumped

It will, when the new version is released.

then scrcpy would become completely unresponsive

That's expected. The packet size changed, so after one mismatched packet, the whole stream becomes out of sync.

Also, had to change PLATFORM and BUILD_TOOLS from 31 to 33

This looks unrelated to this PR, maybe you want to open a new issue for further investigation.

@metayan
Copy link

metayan commented Sep 2, 2022

@metayan Thanks for the verification!

In addition, can you verify does horizontal scrolling works (not flipped) on both touchpad and mouse (if you have a mouse with horizonal scroll wheel)? You can open the browser app on your phone and pinch out to zoom in, then scroll horizontally.

It is "flipped" on the touchpad.
With "Scroll direction: Natural" set in preferences, the vertical scroll follows the fingers, but the horizontal moves in the opposite direction.

I don't have a mouse with horizontal scroll wheel.

The version should probably be bumped

It will, when the new version is released.

Might be an idea to bump it immediately when changing the protocol, so anyone building from source will notice the mismatch right away. Your choice, of course. ;)

Also, had to change PLATFORM and BUILD_TOOLS from 31 to 33

This looks unrelated to this PR, maybe you want to open a new issue for further investigation.

Seems so. Will do.

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.

Handle high-precision scrolling when using notebook touchpads
3 participants