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.

Sign up for GitHub

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

Jump to bottom

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

Open
nyanpasu64 opened this issue Mar 23, 2022 · 9 comments · May be fixed by #300
Open

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

nyanpasu64 opened this issue Mar 23, 2022 · 9 comments · May be fixed by #300

Comments

@nyanpasu64
Copy link

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 (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
Copy link

LTVA1 commented Mar 23, 2022

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

@kometbomb
Copy link
Owner

kometbomb commented Mar 23, 2022 via email

@nyanpasu64
Copy link
Author

nyanpasu64 commented Mar 23, 2022 •
edited
Loading

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):

  • 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.

@kometbomb
Copy link
Owner

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
Copy link
Author

libsdl-org/SDL#4376 (comment) 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, libsdl-org/SDL#4792 was rejected for libsdl-org/SDL#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 libsdl-org/SDL#4376), as those aren't batched like WM_MOUSEMOVE.

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

@nyanpasu64
Copy link
Author

nyanpasu64 commented Mar 23, 2022 •
edited
Loading

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
Copy link
Contributor

rofl0r commented Mar 23, 2022

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.

This was referenced Mar 23, 2022
Open
Open
@kometbomb
Copy link
Owner

kometbomb commented Mar 24, 2022 via email

@kometbomb
Copy link
Owner

kometbomb commented Oct 11, 2022 via email

@edwintorok edwintorok mentioned this issue Feb 11, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Combine sequential mouse movement events nyanpasu64/klystrack
4 participants
@rofl0r @nyanpasu64 @kometbomb @LTVA1