8bitbubsy / pt2-clone

ProTracker 2 clone for Windows/macOS/Linux
https://16-bits.org
BSD 3-Clause "New" or "Revised" License
447 stars 32 forks source link

Mouse problems when SDL2 runs on its KMSDRM backend #10

Closed vanfanel closed 4 years ago

vanfanel commented 4 years ago

Hi there, @8bitbubsy

I am seeing this strange behaviour where software mouse cursor does not move, and left mouse button always ask me for confirmation to exit. With hardware mouse, it does not happen, only with software mouse.

-The issue only happens when running pt2-clone with SDL2 on KMSDRM, so it does not happen when SDL2 runs on the X11 backend.

-Other programs using mouse on SDL2 with the KMSDRM backend dont have this problem.

-It has started happening on recent versions of pt2-clone.

-SDL2 mouse test included with the SDL2 sources (tests/testrelative) works fine with SDL2+KMSDRM.

So, it could be that its not a pt2-clone problem, but a KMSDRM SDL2 backend instead. But I am one of the contributors of that backend, and I dont know where to start looking because I did not do the mouse part of the KMSDRM backend and pt2-clone is the only program that I see with failing mouse cursor on our KMSDRM backend.

So, any idea on what could be going on here? What could pt2-clone be doing different from other SDL2 programs and tests regarding mouse reading?

PD: KMSDRM is the lowest-level graphics backend for SDL2. Its there to give a low latency method to access graphics hardware on GNU/Linux, and works very well in general.

8bitbubsy commented 4 years ago

It could possibly be related to using SDL_GetGlobalMouseState() instead of SDL_GetMouseState(). The reason I started using the former is because I want to be able to handle "outside the window" cases. This is useful for when you are f.ex. dragging a scrollbar and moving the mouse outside of the window in the process, then releasing the mouse button.

EDIT: The reason the left mouse button press always makes the program ask if you want to quit, is because clicking at coord 0,0 brings up the exit dialog on original ProTracker, so I simulate it too. Since the mouse is not moving, it's probably stuck at 0,0.

vanfanel commented 4 years ago

@8bitbubsy You nailed it, yes: changing the SDL_GetGlobalMouseState() call to SDL_GetMouseState() makes mouse start working perfectly in KMSDRM. Good!! At least I wont lose one of my favorite programs in my small X-less systems!

I have been looking at the SDL_GetGlobalMouseState() docs in https://wiki.libsdl.org/SDL_GetGlobalMouseState and it seems to return the mouse position with respect to the desktop. Thing is, with KMSDRM there is NO deskop... I dont know what to do respect to this.

8bitbubsy commented 4 years ago

Yes, that's why I use that function. :-) If only there was a simple way to detect if there is a desktop or not... Maybe there is, but not that I am aware of.

vanfanel commented 4 years ago

Ah, I will make a tentative GetGlobalMouseState() implementation for KMSDRM, and just return the same that GetMouseState() would return. After all, there is no desktop to lie about :)

8bitbubsy commented 4 years ago

This is not very helpful for other users that might stumble upon the same problem though... I should try to come up with a solution for this, because it will probably break on other non-desktop systems too.

Also, you should remove the absolute->relative calculation in readMouseXY() if you use SDL_GetMouseState(). E.g. line 301 to line 310 has to be deleted in pt2_mouse.c (v1.15).

vanfanel commented 4 years ago

@8bitbubsy I have looked in the current libSDL2 sources, it seems that SDL_GetGlobalMouseState() is only implemented on X11, Windows and Cocoa (Mac?). So yes, any other x-less system will break with that function...

8bitbubsy commented 4 years ago

Yeah... I have no idea how to detect if a desktop is present or not in a clean, sensible way.

vanfanel commented 4 years ago

@8bitbubsy Maybe you could ask for the backend being used by SDL2 and chose accordingly? So you can be sure X11 backend has a desktop, but KMSDRM does not, etc... But, if you ask me, it should be a fullscreen-oriented program as ProTracker always was, and never worry about desktops or do anything with them (this is a personal opinion).

8bitbubsy commented 4 years ago

Maybe.... Also I disagree with your last statement there. Most people use their OS in a multitasking way. Making it fullscreen only would make it less practical in several ways. Drag and drop is gone, the pixels are way too thick (320x256 upscale to fit 2k or 4k is not going to look nice), etc.

vanfanel commented 4 years ago

@8bitbubsy My usercase is not very common, that is true. But 320x256 scaled (with "linear" quality, since "nearest" looks ugly, grainy, too cold and digital and causes shimmering in 1-pixel movements) to 1080p via SDL2, without using any CPU, looks beautiful.

For my builds I always have to:

1 - Force SDL_WINDOW_FULLSCREEN_DESKTOP flag on SDL_CreateWindow(), to avoid 1X pt2-clone (non scaled to save CPU and look nice when scaled by SDL2) appearing in a small screen area. 2- Change SetHint() for scaler quatity to "linear".

...And the result is wonderful.

In fact, in KMSDRM or X11, pressing F11 does not go to fullscreen, it just moves the small pt2-clone screen position.

I think a working FULLSCREEN option in the ini would be great, to start in fullscreen mode for desktop-less systems (SDL_WINDOW_FULLSCREEN_DESKTOP flag would be passed to CreateWindow if fullscreen is activated in the INI, AND "linear" filtering would be activated for nice-looking scaling to physcical screen resolution). I would be very happy with such an addition. But well, I can do my own builds with these things forced if you dont like the idea :)

8bitbubsy commented 4 years ago

The reason I use nearest is because I want pixel perfect upscaling. I dislike when pixels have different sizes depending on where they are located on the screen... This is being used in combination with centering the image, to keep an integer aspect ratio. Linear has a tendency to look very blurry, and stretching out 4:3 to 16:9 is absolute madness in my opinion! The upscaling should be done on the GPU side, but I know that on some systems like Raspberry Pi, using a high upscaling factor will make the rendering extremely slow for some reason. Maybe there is no proper GPU hardware mode for SDL2 on RPi.

Also regarding fullscreen mode not working in X11 (it works on the Linux distros I have tested): I'm not surprised. SDL2 + Linux = pray that things are consistant.

vanfanel commented 4 years ago

@8bitbubsy We have different tastes regarding graphics scaling. I like it blurry better than grainy. But anyway, as long as you keep the 1X scale option, it is enough for me.

But could you at least consider the FULLSCREEN option in the .INI so pt2-clone can be directly started in fullscreen mode?

8bitbubsy commented 4 years ago

Yep, I am implementing FULLSCREEN and PIXELFILTER (nearest, linear, best) options for protracker.ini. Also going to look into detecting KMSDRM somehow to remove the absolute mouse system.

vanfanel commented 4 years ago

Great to hear! :)

8bitbubsy commented 4 years ago

I commited some fixes now, could you try compiling the newest commit and see if it helps? Also remember to get the new protracker.ini from /release/other/

vanfanel commented 4 years ago

@8bitbubsy : It works perfectly well, and the new parameters in the INI to their job, but only on some screens. On others, it fails because the program tries to impose integer scaling in fullscreen mode, so in a 1360x768 screen in fullscreen mode, pt2 appears on a little rect on the center of the screen.

Could you please add a INTERGERSCALE (TRUE/FALSE) option to the .INI? If INTEGERSCALE is disabled, aspect ratio should still be respected. To disable aspect ratio, we already have the FULLSCRENSTRETCH option.

The idea would be to be able to combine FULLSCREEN=TRUE, FULLSCREENSTRETCH=TRUE, INTEGERSCALE=FALSE, and PIXELFILTER=LINEAR to get proper fullscreen, bilinear filtered and correct aspect ratio in ANY video mode. pt2-clone was already working like that a year ago or so.

8bitbubsy commented 4 years ago

Huh? Isn't that exactly what FULLSCREENSTRETCH does?

vanfanel commented 4 years ago

@8bitbubsy No. FULLSCREENSTRETCH just scales to fullscreen dimensions, destroying aspect ratio.

vanfanel commented 4 years ago

@8bitbubsy Ah, I have found a simple fix. Can you please pass SDL_WINDOW_FULLSCREEN_DESKTOP to SDL_CreateWindow() ? That seems to work in any screen with that instead of calling SDL_SetWindowFullscreen() after SDL_CreateWindow()...

8bitbubsy commented 4 years ago

I tested doing that now, and it breaks many things. It breaks the mouse cursor scaling, it breaks the mouse coords scaling etc. Doing it like this requires refactoring of the code. The current code works fine on my system, at least.

vanfanel commented 4 years ago

@8bitbubsy I believe that SDL_SetWindowFullscreen() is broken on KMSDRM, yes. Its not even implemented. I will try to fix it, too, as I did with SDL_GetGlobalMouseState() yesterday. But that wont be available for most people until next stable SDL2 version, I guess.

8bitbubsy commented 4 years ago

Ah I see, so you're actually a developer of the KMSDRM video driver for SDL2. Anyway, I'm not going to refactor the code just to support KMSDRM. It's better that the SDL2 KMSDRM video driver gets fixed instead, so people will have to wait then...

vanfanel commented 4 years ago

@8bitbubsy I agree with you. I will keep you informed when its fixed, anyway, so you know pt2-clone works fine with KMSDRM.

vanfanel commented 4 years ago

@8bitbubsy With the latest addition of INTEGERPIXERSCALING things work perfect on this side! This issue can be closed.

Thanks a lot!

I hope your (also INCREDIBLE) FT2 clone supports all this too soon :) I love them both.

8bitbubsy commented 4 years ago

The FT2 clone won't change in how it works with the pixel filter + stretching out etc, but I can add the KMSDRM fix for the mouse at least.

vanfanel commented 4 years ago

What I did WRT GetMouseGlobalState() was to set than function to run GetMouseState() instead in KMSDRM (so the Global version just get you the coordinates anyway), but anyway it would be nice if you add the fix because there could be other implications I am ignoring.