Details

Reviewers

Masakazu Ito (djnz)
Brecht Van Lommel (brecht)
Andrea Weikert (elubie)
Sergey Sharybin (sergey)
Martijn Berger (juicyfruit)
Juergen Herrmann (shadowrom)

Group Reviewers

Platform: Windows

Commits

rBS26d7d995dbef: Fix Windows mouse wheel scroll speed
rB26d7d995dbef: Fix Windows mouse wheel scroll speed

Summary

In Windows, event dispatching code is throwing out the wheel scroll count value.
Despite of how many fast you move the wheel, it only make one-notch scroll event.

This patch convert wheel event to multiple 1-notch wheel events.

This also correct the handling of smooth scroll mouse wheel (which can report smaller than 1-notch wheel movement) by accumulating the small wheel delta values.

Diff Detail

Event Timeline

Change seems reasonable.

Unclear to me is why we would want to preserve current behaviour with yet another environment variable.
Are there downsides ?

Thank you for your comment.

Environment variable is just for testing purpose, comparing the speed without rebuilding.

I imagine the environment variable is only for testing and will be removed in the final patch.

intern/ghost/intern/GHOST_SystemWin32.cpp
1061	This could be a member variable of GHOST_SystemWin32 I think.
1072–1073	Why is there a maximum here, to avoid a very high number of mouse wheel events? 5 seems a bit low.

Changed N_MAX to 999
Added print to show scroll value.

I can get to "wheel n=12" at most.
with Microsoft Basic Optical Mouse v2.0.

This is like "proof of concept" code.
So sorry about messy coding.
Is this should be posted as Bug instead of Patch?

I would like to hear about this problem.
As I don't know how many people are annoyed by this probem.

Because this is hightly dependent onenvironment (OS, CPU, GPU, Mouse, etc.)

Should I post on Patch Task (after closing this review ) ?
Sorry I'm newbie here and can't understand the system yet.

Is there some help page about this site ?

Can I close this patch review ?

This patch is very welcome and I know it's pretty annoying specially in file browser for me.
Count me for testing.

@Masakazu Ito (djnz): this is a bit of a special case because it's both a bug report and a patch. For a simple issue like this it should be fine to keep this review open and not create a separate bug report, it's easier if everything is in one place. If this review doesn't turn into a patch that we can commit we can always move it to a bug report later.

The code submission page links to some documentation about the process:
https://developer.blender.org/differential/diff/create/

Changed the default behavior to enable the patch.
Made the code into the function and clean ups.
Added more wheel debug log.

Log when wheel scrolling (at my usual speed) theme panel.
ticks increase more when screen update(event dispatching) is slow.

wheel:  zDelta=-120 (accum 0->0) ticks=1 time=7557 msec.(diff=7557 msec.)
wheel:  zDelta=-600 (accum 0->0) ticks=5 time=7655 msec.(diff=98 msec.)
wheel:  zDelta=-240 (accum 0->0) ticks=2 time=7742 msec.(diff=87 msec.)
wheel:  zDelta=-120 (accum 0->0) ticks=1 time=8093 msec.(diff=351 msec.)
wheel:  zDelta=-600 (accum 0->0) ticks=5 time=8183 msec.(diff=90 msec.)
wheel:  zDelta=-240 (accum 0->0) ticks=2 time=8280 msec.(diff=97 msec.)
wheel:  zDelta=-120 (accum 0->0) ticks=1 time=8556 msec.(diff=276 msec.)
wheel:  zDelta=-600 (accum 0->0) ticks=5 time=8647 msec.(diff=91 msec.)
wheel:  zDelta=-240 (accum 0->0) ticks=2 time=8733 msec.(diff=86 msec.)
wheel:  zDelta=-120 (accum 0->0) ticks=1 time=9062 msec.(diff=329 msec.)
wheel:  zDelta=-720 (accum 0->0) ticks=6 time=9153 msec.(diff=91 msec.)

Removed debug codes and made clean ups.

Almost looking good to me. I'd still remove the return value and maybe add a comment that the events are already handled inside processWheelEvent.

I think it's ok to have the static int accum inside the method - the method itself is static anyway, not sure how much is gained by making this a static member of the class.

intern/ghost/intern/GHOST_SystemWin32.cpp
723	See below, since always 0 is returned, you can get rid of the return value for this method alltogether
1063	With the new behavior only, this check is not necessary since all events are now handled in processWheelEvent. In this case you can probably also get rid of the return value alltogether

Made processWheelEvent() to return void

Looking good to me

Changed static accumulator to member variable.
Clear accumulator in WM_ACTIVATE event, and when scroll direction changed.

Also changed (short)HIWORD(wParam) to GET_WHEEL_DELTA_WPARAM(wParam)

Looking good to me too.

Ping, status here? Can someone handle this please? ;)

This revision now requires changes to proceed.Jul 21 2014, 10:22 PM

Ray Molenkamp (LazyDodo) commandeered this revision.Sep 21 2016, 5:50 PM

Ray Molenkamp (LazyDodo) mentioned this in D2250: Update diff to latest master..

Ray Molenkamp (LazyDodo) added a reviewer: Masakazu Ito (djnz).

This one has been stale for a while, I updated diff for latest master.

Ray Molenkamp (LazyDodo) added reviewers: Sergey Sharybin (sergey), Martijn Berger (juicyfruit).Sep 21 2016, 6:27 PM

If you tested it still works fine then I think you can go ahead and commit.

intern/ghost/intern/GHOST_SystemWin32.cpp
719–720	Same here, better use `int`.
734	And here as well.
intern/ghost/intern/GHOST_SystemWin32.h
381 ↗	(On Diff #7480)	Better use `int`, not point risking overflow.

Masakazu Ito (djnz) added inline comments.Sep 28 2016, 11:07 PM

intern/ghost/intern/GHOST_SystemWin32.h
381 ↗	(On Diff #7480)	As WHEEL_DELTA is defined as 120 in WinUser.h, Signed char is enough to store the value from -119 to 119.

In D143#52388, @Brecht Van Lommel (brecht) wrote:

If you tested it still works fine then I think you can go ahead and commit.

Blendify asked me to take a peek at this ticket, and update the diff if needed, I have tested it to the point of 'it didn't break, scrolling seems to work' but I'm unable to tell if it's any better or not. (having a run of the mill $10 generic mouse probably doesn't help) probably better if someone with a 'fancy' mouse gives this one a whirl first before committing it.

Brecht Van Lommel (brecht) added inline comments.Sep 28 2016, 11:17 PM

intern/ghost/intern/GHOST_SystemWin32.h
381 ↗	(On Diff #7480)	Ok, but still no good reason to cut it close I think. Things can break in refactoring and there's no downside to using ints.

In D143#52424, @Ray Molenkamp (LazyDodo) wrote:

Blendify asked me to take a peek at this ticket, and update the diff if needed, I have tested it to the point of 'it didn't break, scrolling seems to work' but I'm unable to tell if it's any better or not. (having a run of the mill $10 generic mouse probably doesn't help) probably better if someone with a 'fancy' mouse gives this one a whirl first before committing it.

That's good enough to commit I think, I can see no reason this would not work anymore.

Brecht Van Lommel (brecht) accepted this revision.Sep 28 2016, 11:25 PM

Brecht Van Lommel (brecht) edited edge metadata.

This revision is now accepted and ready to land.Sep 28 2016, 11:25 PM