kometbomb / klystrack

A chiptune tracker
http://kometbomb.github.io/klystrack/
Other
481 stars 29 forks source link

On Linux, with 1000 Hz mouse, klystrack lags when moving mouse around menu #299

Open nyanpasu64 opened 2 years ago

nyanpasu64 commented 2 years ago

I have a Logitech G203 mouse running at 1000 Hz.

On Klystrack git, when I right-click Klystrack menu bar and drag my mouse around, klystrack lags heavily and falls behind the mouse position, slowly processing mouse movement events. If I try closing the window, KDE considers it unresponsive, and offers to force quit the app. I think it's trying to redraw the screen 1000 times per second, instead of skipping ahead to the latest mouse position on each frame.

Version: fe6e7465512bee32bf8e724857a7fbab902b8db0 (from https://aur.archlinux.org/packages/klystrack-git) Operating System: Arch Linux KDE Plasma Version: 5.24.3 KDE Frameworks Version: 5.92.0 Qt Version: 5.15.3 Kernel Version: 5.16.15-zen1-1-zen (64-bit) Graphics Platform: X11 Processors: 12 × AMD Ryzen 5 5600X 6-Core Processor Memory: 15.6 GiB of RAM Graphics Processor: NVIDIA GeForce GT 730/PCIe/SSE2

LTVA1 commented 2 years ago

Yes, you are right, it updates mouse position in the same loop where redraw function is called

kometbomb commented 2 years ago

Yes, as @LTVA1 already said the refresh would have to be clamped to a lower rate somehow even if the mouse sends many times more events than there usually is. Or, the whole refresh could happen only at a stable rate which probably is enough considering the mouse cursor will not lag.

ke 23. maalisk. 2022 klo 3.26 nyanpasu64 @.***) kirjoitti:

I have a Logitech G203 mouse running at 1000 Hz.

On Klystrack git, when I right-click Klystrack menu bar and drag my mouse around, klystrack lags heavily and falls behind the mouse position, slowly processing mouse movement events. If I try closing the window, KDE considers it unresponsive, and offers to force quit the app. I think it's trying to redraw the screen 1000 times per second, instead of skipping ahead to the latest mouse position on each frame.

Version: fe6e746 https://github.com/kometbomb/klystrack/commit/fe6e7465512bee32bf8e724857a7fbab902b8db0 (from https://aur.archlinux.org/packages/klystrack-git) Operating System: Arch Linux KDE Plasma Version: 5.24.3 KDE Frameworks Version: 5.92.0 Qt Version: 5.15.3 Kernel Version: 5.16.15-zen1-1-zen (64-bit) Graphics Platform: X11 Processors: 12 × AMD Ryzen 5 5600X 6-Core Processor Memory: 15.6 GiB of RAM Graphics Processor: NVIDIA GeForce GT 730/PCIe/SSE2

— Reply to this email directly, view it on GitHub https://github.com/kometbomb/klystrack/issues/299, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIIV2JUAOXITQTUEP3S333VBJXK7ANCNFSM5RMRE63A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

nyanpasu64 commented 2 years ago

I have a fix which appears to work on my machine (sorry for the wildly inconsistent formatting):

            // ensure the last event is a mouse click so it gets passed to the draw/event code
            if (e.type == SDL_MOUSEBUTTONDOWN)
                break;

            if (e.type == SDL_MOUSEMOTION) {
                // If the next event is not a mouse movement event, process current mouse
                // movement event immediately.
                SDL_Event next_e;
                SDL_PumpEvents();
                if (SDL_PeepEvents(&next_e, 1, SDL_PEEKEVENT, SDL_MOUSEMOTION, SDL_MOUSEMOTION) <= 0)
                {
                    break;
                }
            }

I considered breaking when motion.state changed, but decided it was unnecessary since you respond to mouse down/up events instead of state. Maybe I should add it anyway, since check_drag_event() and main() read motion.state. Unfortunately implementing this fix (or for that matter, altering it to break when motion.state changes) requires copy-pasting this block of code in at least 3 places in klystrack, and 2 places in the klystron submodule (preventing me from making a single PR fixing the issue):

I don't think there are any more spots I missed; I searched for ensure the last event is a mouse click and replaced each occurrence, then searched for SDL_MOUSEMOTION to verify none of the other matches were used to break out of SDL_PollEvent loops. I'm unsure why klystron/tools/editor/src/main.c has SDL_MOUSEMOTION but no early exit; is it not needed to handle the SDL_MouseMotionEvent outside of the loop in this file, but necessary in other files?

I'm interested in GUI programming, and evaluating the options of hand-rolled UI (klystrack, schism tracker, snes tracker) vs. immediate-mode frameworks (egui mostly works but has layout limitations and often incorrect sizing on the first frame of a window) vs. retained-mode UI frameworks (Qt is mostly full-featured but comes with a lot of drudge work). To me, the copy-pasting involved in klystrack's hand-rolled UI is a point against it, but I'm not sure if it can be avoided easily or not.

kometbomb commented 2 years ago

This is very nice, thanks. It should be easy enough to just make two PR's to the two repos.

You should look into the prototracker GUI instead, the one in klystrack is terrible and frankly just something I threw together as fast as I could so that I can get going with the actual interesting audio stuff. Prototracker has proper event handling and dirty rectangle checking so that it doesn't have to redraw everything when something happens.

nyanpasu64 commented 2 years ago

https://github.com/libsdl-org/SDL/issues/4376#issuecomment-841654449 suggests only calling SDL_PumpEvents once per frame. The docs didn't make clear that that was valid, but I'd have to again revise my code patch to not pump events before peeping. (And if I wanted to not poll events on each loop, that would be a bigger change, but it's not necessary to handle 1000 Hz mice on Linux, and I can't test with 8000 Hz.)

Is it SDL's responsibility to combine SDL_MouseMotionEvent on each frame? Unsure. What about mouse move events when the app is falling behind many visual frames? Maybe not, that would prevent apps running at 20fps from tracking precise mouse movement. And I don't know if SDL can tell the difference.

On Windows, https://github.com/libsdl-org/SDL/pull/4792 was rejected for https://github.com/libsdl-org/SDL/pull/4794 (I think it changes behavior regardless of OS). Note that the latter makes polling events on each loop no longer pump the system APIs on each iteration (which is O(n^2) slow), so on modern SDL we don't have to restructure our loops to avoid polling on each loop.

IIRC the main source of event spam is WM_INPUT (as mentioned in https://github.com/libsdl-org/SDL/issues/4376), as those aren't batched like WM_MOUSEMOVE.

Is the batching implemented by SDL or Windows? Should I ask these questions upstream?

nyanpasu64 commented 2 years ago

Update:

With the same mouse, klystrack 1.7.6 on Windows (with proper GPU drivers installed) does not lag when moving the mouse. So this change to the SDL event processing loop is only needed on Linux. Do you still want the change?

altering it to break when motion.state changes

I've decided not to do this, because it doesn't matter if a user is holding the mouse button for a fraction of a visual frame... Maybe I should anyway, since some of your drag-drop code is level-triggered on motion.state?

Another concern is whether to break on the last event before a state change, the first event after a state change (harder), or both (even harder). I might just do the first one, since sub-frame timing really shouldn't matter all that much.


Should SDL_MOUSEBUTTONDOWN and SDL_MOUSEMOTION handling be extracted to a function? If so, where would I put it?

rofl0r commented 2 years ago

Should I ask these questions upstream?

maybe not a bad idea. SDL could check if the previous queued event is mousemotion when it gets a new event, and then just overwrite the last one instead of queuing another one.

kometbomb commented 2 years ago

Doesn't that nullify the purpose of the increased accuracy? :) I could theoretically wiggle the mouse really fast at 1000 Hz and cause events that go zig-zag but if you overwrite the events... no zig-zag.

ke 23. maalisk. 2022 klo 22.51 rofl0r @.***) kirjoitti:

Should I ask these questions upstream?

maybe not a bad idea. SDL could check if the previous queued event is mousemotion when it gets a new event, and then just overwrite the last one instead of queuing another one.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

kometbomb commented 1 year ago

This is very nice, thanks. It should be easy enough to just make two PR's to the two repos.

You should look into the prototracker GUI instead, the one in klystrack is terrible and frankly just something I threw together as fast as I could so that I can get going with the actual interesting audio stuff. Prototracker has proper event handling and dirty rectangle checking so that it doesn't have to redraw everything when something happens.

ke 23. maalisk. 2022 klo 8.50 nyanpasu64 @.***) kirjoitti:

I have a fix which appears to work on my machine:

      // ensure the last event is a mouse click so it gets passed to the draw/event code
      if (e.type == SDL_MOUSEBUTTONDOWN)
          break;

      if (e.type == SDL_MOUSEMOTION) {
          // If the next event is not a mouse movement event, process current mouse
          // movement event immediately.
          SDL_Event next_e;
          SDL_PumpEvents();
          if (SDL_PeepEvents(&next_e, 1, SDL_PEEKEVENT, SDL_MOUSEMOTION, SDL_MOUSEMOTION) <= 0)
          {
              break;
          }
      }

I considered breaking when motion.state changed, but decided it was unnecessary since you respond to mouse down/up events instead of state. Maybe I should add it anyway, since check_drag_event() and main() read motion.state.

Unfortunately implementing (or altering) this fix requires copy-pasting this block of code in at least 3 places in klystrack, and 2 places in the klystron submodule (preventing me from making a single PR fixing the issue):

  • klystron/src/gui/filebox.c
  • klystron/src/gui/msgbox.c
  • src/help.c
  • src/import/hubdialog.c
  • src/main.c

I don't think there are any more spots I missed; I searched for ensure the last event is a mouse click and replaced each occurrence, then searched for SDL_MOUSEMOTION to verify none of the other matches were used to break out of SDL_PollEvent loops. I'm unsure why klystron/tools/editor/src/main.c has SDL_MOUSEMOTION but no early exit; is it not needed to handle the SDL_MouseMotionEvent outside of the loop in this file, but necessary in other files?

I'm interested in GUI programming, and evaluating the options of hand-rolled UI (klystrack, schism tracker, snes tracker) vs. immediate-mode frameworks (egui mostly works but has layout limitations and often incorrect sizing on the first frame of a window) vs. retained-mode UI frameworks (Qt is mostly full-featured but comes with a lot of drudge work). To me, the copy-pasting involved in klystrack's hand-rolled UI is a point against it, but I'm not sure if it can be avoided easily or not.

— Reply to this email directly, view it on GitHub https://github.com/kometbomb/klystrack/issues/299#issuecomment-1075985280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIIV2LVTY37IMZYGL4372LVBK5LJANCNFSM5RMRE63A . You are receiving this because you commented.Message ID: @.***>