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

Add scrollspeed for Menu and increase the default for non apple system #2616

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jan 19, 2023

Description

Fixes https://discourse.julialang.org/t/how-to-increase-a-scrolling-speed-in-makie-menu/93221

I also added a news entry for #2614 since I forgot to do it there and this is a small pr as well.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 19, 2023

We could also try normalizing with sign(scroll) but I think it's better to have varying speed multipliers depending on the OS so that the extra resolution is used when available.

@MakieBot
Copy link
Collaborator

MakieBot commented Jan 19, 2023

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 36.82s (36.76, 36.89) 0.05+- 17.58s (17.35, 17.82) 0.18+- 15.49s (15.34, 15.66) 0.11+- 9.94ms (9.73, 10.08) 0.11+- 113.89ms (112.80, 115.26) 0.96+-
master 36.92s (36.85, 37.01) 0.07+- 17.52s (17.25, 17.79) 0.19+- 15.42s (15.12, 15.66) 0.20+- 10.01ms (9.95, 10.14) 0.09+- 113.57ms (112.86, 114.09) 0.51+-
evaluation -0.29%, -0.11s faster ✓ (-1.81d, 0.01p, 0.06std) +0.37%, 0.06s invariant (0.34d, 0.53p, 0.19std) +0.47%, 0.07s invariant (0.45d, 0.42p, 0.16std) -0.73%, -0.07ms invariant (-0.73d, 0.20p, 0.10std) +0.28%, 0.32ms invariant (0.41d, 0.46p, 0.74std)
CairoMakie 31.76s (31.67, 31.87) 0.07+- 16.97s (16.86, 17.07) 0.06+- 2.54s (2.52, 2.55) 0.01+- 9.65ms (9.58, 9.71) 0.05+- 4.40ms (4.31, 4.54) 0.09+-
master 31.80s (31.67, 31.91) 0.10+- 16.92s (16.83, 17.01) 0.07+- 2.54s (2.52, 2.56) 0.02+- 9.58ms (9.54, 9.62) 0.03+- 4.35ms (4.16, 4.51) 0.13+-
evaluation -0.13%, -0.04s invariant (-0.47d, 0.40p, 0.08std) +0.30%, 0.05s invariant (0.74d, 0.19p, 0.07std) +0.07%, 0.0s invariant (0.14d, 0.80p, 0.01std) +0.79%, 0.08ms slower X (2.01d, 0.00p, 0.04std) +1.14%, 0.05ms invariant (0.44d, 0.43p, 0.11std)
WGLMakie 45.58s (45.00, 46.95) 0.86+- 21.54s (20.89, 22.73) 0.69+- 21.07s (20.47, 22.35) 0.69+- 13.95ms (13.00, 16.03) 1.04+- 1.74s (1.70, 1.77) 0.03+-
master 45.43s (44.78, 46.78) 0.69+- 21.66s (20.97, 22.93) 0.69+- 20.92s (20.51, 22.25) 0.61+- 13.87ms (12.81, 16.11) 1.11+- 1.73s (1.68, 1.82) 0.05+-
evaluation +0.31%, 0.14s invariant (0.18d, 0.74p, 0.78std) -0.56%, -0.12s invariant (-0.18d, 0.75p, 0.69std) +0.74%, 0.16s invariant (0.24d, 0.66p, 0.65std) +0.54%, 0.08ms invariant (0.07d, 0.90p, 1.08std) +0.25%, 0.0s invariant (0.11d, 0.84p, 0.04std)

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 19, 2023

Test failure is unrelated

@jkrumbiegel
Copy link
Member

I actually don't think it's an apple thing, I think it's a trackpad vs mouse thing. A scroll wheel usually has a discrete scroll behavior and that seems to trigger a value of 1. And trackpad values come much more frequently but with smaller values.

@jkrumbiegel
Copy link
Member

For example, this is what I get when I scroll a little bit, like a centimeter downwards, on my macbook:

s = Scene()
on(println, s.events.scroll)
s
julia> (0.0, 0.1)
(0.4, 0.6000000000000001)
(0.4, 0.4)
(0.2, 0.4)
(0.2, 0.4)
(0.1, 0.1)
(0.1, 0.4)
(0.2, 0.4)
(0.4, 0.5)
(0.4, 0.4)
(0.30000000000000004, 0.30000000000000004)
(0.1, 0.1)
(0.1, 0.5)
(0.0, 0.1)
(0.1, 0.30000000000000004)
(0.30000000000000004, 0.4)
(0.5, 0.5)
(0.30000000000000004, 0.5)
(0.30000000000000004, 0.4)
(0.2, 0.4)
(0.2, 0.30000000000000004)
(0.5, 0.5)
(0.2, 0.30000000000000004)
(0.1, 0.1)
(0.30000000000000004, 0.30000000000000004)
(0.2, 0.4)
(0.0, 0.1)
(0.1, 0.30000000000000004)
(0.1, 0.0)

I don't know what the correct way is to reconcile that with a mouse that outputs (0, 1) for every discrete scroll event.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 20, 2023

Hmm, I thought it was kind of the inverse issue where ios had larger numbers. But if it's trackpads that have more scroll events and feel faster then I'm not sure what to do about it. Something like scroll_y == sign(scroll_y) ? 10 * scroll_y : scroll_y might work but seems prone to glitchyness to me. Maybe we should just add the scroll speed attribute here and try to figure out a solution for trackpad vs mousewheel some other time?

@jkrumbiegel
Copy link
Member

One hacky workaround I thought about was to simply feed the scroll values through some logic that checks if there has ever been a value other than 0, 1 or -1. If yes, then apply a trackpad scaling behavior, otherwise scroll wheel.

@mkoculak
Copy link
Contributor

My scroll wheel generates whole values, but if I scroll fast enough, I can get 2, 3, or 4 returned also.
So maybe just whole vs fractional values?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 20, 2023

On my old laptop the trackpad is recognized as -1/0/+1 on ubuntu. On windows it's not working at all anymore. I guess my drivers got corrupted somehow...

I guess we could check something like is_integer = is_integer && (scroll_y == sign(scroll_y) * abs(scroll_y)) though that would still be problematic when switching between mouse and trackpad. It doesn't look like we have a better choice though

Comment on lines 295 to 297
# Hack to differentiate mousewheel and trackpad scrolling
step = (isinteger(y) ? 15.0 : 1.0) * m.scroll_speed[] * y
new_y = max(min(t[2] - step, 0), height(menuscene.px_area[]) - listheight[])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume this will cause problems with trackpads but maybe it's worth trying?

@jkrumbiegel
Copy link
Member

I think for the heuristic we do need some value tracking, because a trackpad can also output integers:

s = Scene()
on(vals -> any(x -> x != 0 && isinteger(x), vals) && println(vals), s.events.scroll)
s

With this I get occasional output when scrolling around:

julia> (0.4, -2.0)
(1.1, 9.0)
(0.0, -5.0)
(0.8, 5.0)
(0.0, -3.0)
(0.0, -11.0)
(0.4, 1.0)
(-0.5, -2.0)
(0.0, 3.0)
(0.0, 2.0)
(1.0, 5.9)
(0.0, 9.0)
(0.0, 2.0)
(0.0, -15.0)
(0.8, 3.0)
(0.0, 5.0)
(-0.4, -3.0)
(0.0, -1.0)
(-0.30000000000000004, -1.0)
(0.8, 2.0)
(0.7000000000000001, 2.0)
(0.4, 1.0)

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 24, 2023

Yea, that's what I expected. Since this would be generally useful, should we adjust the scroll event to be (dx, dy, has_always_been_integer)? In that case we may want to split into two mini pr's, one for scroll_speed and one being a breaking change in the Events struct?

@jkrumbiegel
Copy link
Member

Wouldn't it be easiest to modify the number sent out by the event struct already so that users get usably transformed values? Kind of doing what glfw is not doing for us. The values would change compared to what they are now but I wouldn't consider that change breaking.

In terms of the logic, I would set the default to assume mouse and change that to touchpad when a non-integer is encountered maybe? That would flip the trackpad correctly from the first non-integer sample, which given the accelerating motion of the fingers should happen pretty much immediately? I have no idea if there aren't mice with smoothly varying wheels that would break this scheme but I guess we'll find out?

Another question is if this decision can be made across scenes or if it's per scene?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 24, 2023

Wouldn't it be easiest to modify the number sent out by the event struct already so that users get usably transformed values? Kind of doing what glfw is not doing for us. The values would change compared to what they are now but I wouldn't consider that change breaking.

I generally prefer making changes like that a bit later, just in case some use cases do not want the change. But we can do that as well.

In terms of the logic, I would set the default to assume mouse and change that to touchpad when a non-integer is encountered maybe? That would flip the trackpad correctly from the first non-integer sample, which given the accelerating motion of the fingers should happen pretty much immediately? I have no idea if there aren't mice with smoothly varying wheels that would break this scheme but I guess we'll find out?

Yea sounds good. From what googling about this topic showed me it seems like apple mouses have fractional scrolling on macs at least, while 3rd party ones do not. I have two logitech mouses with free swinging scroll wheels which only produce integer scroll steps on ubuntu and windows.)

Another question is if this decision can be made across scenes or if it's per scene?

If we do the scaling before events.scroll it would be global. With (dx, dy, has_always_been_integer) the decision would be pushed to individual listeners, so every interaction could choose what to do with it.

@jkrumbiegel
Copy link
Member

I should find the mouse I have lying around here somewhere and test it on mac.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 24, 2023

@jkrumbiegel
Copy link
Member

Weird that glfw's also using seemingly arbitrary constants for scaling. I guess for Mac it would matter whether the active device hasPreciseScrollingDeltas or not

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 24, 2023

Yea. Godot also has a hardcoded scaling if hasPreciseScrollingDeltas, but 0.03 in that case 🤷
https:/godotengine/godot/blob/4fa6edc888cfacd5346bf08afa14b5f5a9bd6d0c/platform/macos/godot_content_view.mm#L786-L821

@SimonDanisch
Copy link
Member

Should we merge this for the next release?

@SimonDanisch SimonDanisch merged commit 032343e into master Mar 15, 2023
@SimonDanisch SimonDanisch deleted the ff/menu_scrollspeed branch March 15, 2023 10:40
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.

5 participants