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

fix: preserve accumulated drag-scroll distance, queued or coalesced #52

Closed
wants to merge 4 commits into from

Conversation

ankostis
Copy link

@ankostis ankostis commented Feb 20, 2024

The job of the Drag-scroll distance-accumulating buffer is to reduce mouse's input-distance by dividing it by CHARYBDIS_DRAGSCROLL_BUFFER_SIZE.

Problem (fixed)

Before this PR, when the buffer was full (accumulated-distance > CHARYBDIS_DRAGSCROLL_BUFFER_SIZE),
a ±1 wheel-event was emitted and the spill-over distance was dumped,
beginning the next accumulation cycle (every POINTING_DEVICE_TASK_THROTTLE_MS) always from 0.

In other words, the distance division was getting rid of the remainder,
which at times can be above 1000!
This effectively caps the attainable scroll-speed.

With sensible defaults (SCROLL_DPI: 100), the effect was a subtle jerk and capped scrolling speed,
but in higher DPIs, or with mouse-acceleration1 enabled, and feeding to drag-scroll,
all expected additional velocity was simply lost2.

Feature added

This PR additionally retrofits the drag-scroll with a new config CHARYBDIS_DRAGSCROLL_SEND_COALESCED
that controls how accumulated drag-scroll distance from fast moves are emitted:

  • undefined: queued and emitted as ±1 events
    even after "wheel" has stopped moving; scrolling in the opposite direction will immediately cancel any queued events;
  • defined: scroll-wheel events with values greater than ±1 are immediately emitted.

Mouse acceleration

To enjoy this you need a potent source of mouse distance, like the Mouse acceleration mod2 ("maccel").
But bkb code in keyboards/bastardkb/charybdis/charybdis.c:pointing_device_task_kb() drives the user task
(where the maccel runs) after drag-scroll code, so you have to reverse the x2 lines in the above function
into this:

report_mouse_t pointing_device_task_kb(report_mouse_t mouse_report) {
    if (is_keyboard_master()) {
        mouse_report = pointing_device_task_user(mouse_report);
        pointing_device_task_charybdis(&mouse_report);
    }
    return mouse_report;
}

The last commit in this PR does just that.

:::{note}
You may see this PR in action in ankosti's charyoku keymap for charybdis-4x6.
:::

@ankostis ankostis changed the title fix(bkb) drag-scroll was dumping spill-over distance > fix: preserve accumulated drag-scroll distance Feb 20, 2024
@ankostis
Copy link
Author

Apologies for fat-fingering the OP text & title, ignore this email notification.

@ankostis ankostis marked this pull request as draft February 20, 2024 21:06
@ankostis
Copy link
Author

ankostis commented Feb 20, 2024

Introduced an unforeseen enhancement:

The accumulation fix allowed for "travelling", like "inertia", ie. keep scrolling while mouse has stopped moving,
assuming enough distance had been accumulated, eg. due to high acceleration.
That's very much wanted for big files and streams eg. Discord.

But there must be a way to immediately break travelling, eg when we found what we wanted
and instinctively moving the mouse in the reverse direction.
In that case, the mouse has to work against accumulated distance before acting.

@ankostis ankostis marked this pull request as ready for review February 20, 2024 23:21
@ankostis ankostis marked this pull request as draft February 21, 2024 00:25
@ankostis
Copy link
Author

ankostis commented Feb 21, 2024

Turned this PR back into "draft" because with the the scroll-distance accumulation fixed,
we now have the opportunity to choose from x2 behaviors (or have both on demand):

  • Inertia Queued: keep sending accumulated scrolling distance even after user has stopped dragging scroll
  • Flush: Coalesced: divide and send all scrolling distance at once on every duty-cycle

Although the flush coalesce idea is sound and certain apps can readily cope with it,
inertia is more cool because you can see the whole thing scrolling in front your eyes.

So it may be worth it to have them both, and configurable/activated on demand.
What do you think?

@0xcharly
Copy link
Collaborator

Hi, and thanks for taking the time to look into the behavior and submit a PR.

Now that QMK has introduced their pointing device subsystem, there's no reason to maintain or update this piece of bespoke Charybdis code anymore. It is scheduled to be entirely removed and replaced with the QMK core feature.

For this reason I'd suggest upstreaming your changes directly to QMK, if need be.

reducing effectively the attenable scroll-speed.
From another POV, scroll-buffer's input-speed division was getting rid
of the remainder.

The effect was a subtle jerk with sensible defaults (SCROLL_DPI: 100)
but in higher DPIs or
with *maccel* [1] enabled and redirecting to drag-scroll,
all expected addionall velocity, dissipates.

NOTE: to enable *maccel*, you need to reverse the lines in
`pointing_device_task_kb()` so that `pointing_device_task_user()`
is invoked first.

WARNING: there might be an issue when drag-scroll DPI is 100[2].

[1]: https://github.com/finrod09/qmk_userspace_features/tree/maccel-assym_s_curve/maccel
[2]: https://discord.com/channels/681309835135811648/1203417572742135808/1209118208997466142
The previous fix allows for "travelling", keep scrolling while mouse
has stopped moving, if enough distnance accumulated, and that's good
for big files.  But when reversing direction, mouse has to work against
accumulated distance before acting.

- refact: new dir-checks required changing the axis-reversal macros
  to work via temp-vars.
since it avoids subtracting from an increasing accumulator.

- refact: group ops by axis (not by stage).
±1 scroll-wheel events.

- doc: explain new `CHARYBDIS_DRAGSCROLL_SEND_COALESCED` define.
@ankostis
Copy link
Author

ankostis commented Feb 21, 2024

For this reason I'd suggest upstreaming your changes directly to QMK, if need be.

From what i have learned, the new subsystem won't have anything in common, so no point to push it there. And i absolutely understand the burden of attending code destined for retirement, and why this PR must be rejected, not to spur further development.

On a personal note, i take pride from my work, so i made this to play along with the great maccel feature, forming a pretty functional dead-end duo that can stay in limbo to denote the fact.

Only allow me to settle any loose ends on this PR, OP, title, etc, because
"if a job's worth doing, it's worth doing well" :-)

@ankostis ankostis changed the title fix: preserve accumulated drag-scroll distance fix: preserve accumulated drag-scroll distance, queued or coalesced Feb 21, 2024
@ankostis ankostis marked this pull request as ready for review February 21, 2024 21:28
@ankostis ankostis closed this Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants