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

Linux/GTK3: add gamepad hotplug support #855

Merged
merged 6 commits into from
Nov 4, 2024

Conversation

thesourcehim
Copy link
Contributor

Support for reconnecting gamepads and connecting new ones while desmume is already running. For that I added corresponding event processing to ctrlssdl, also modifying how connected joystick info is stored (and fixing joystick uninit in the process: previously open_joysticks array was allocated but never populated, but used in uninit function nevertheless), and how pressed button id is calculated (previously joystick instance id was used but it changes every time gamepad is reconnected, so now joystick index is used instead).
This can be backported to GTK2 but GTK2 is now ahead of my branch after recent commits, so please review this PR as it is for now.

@thesourcehim
Copy link
Contributor Author

@rofl0r If it's ok, I can port this to GTK2 too, so you can test.

@@ -4118,6 +4128,7 @@ common_gtk_main(GApplication *app, gpointer user_data)
video->SetFilterParameteri(VF_PARAM_SCANLINE_D, _scanline_filter_d);

RedrawScreen();
g_timeout_add(200, OutOfLoopJoyDeviceCheckTimerFunc, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd guess that between plugging in a joystick and moving the mouse to the controls menu, at least a second passes, so polling less often would make sense

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second thought, why poll at all ? sdl sends an event when a device is plugged in, so we could simply notify the frontend about that, or am i missing something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must read the event from the queue as far as I understand, but when emulation is not started, events are not processed in the loop (EmuLoop function). When EmuLoop starts, this timer fucntion exits.

@@ -39,7 +41,8 @@ u16 nbr_joy;
mouse_status mouse;
static int fullscreen;

static SDL_Joystick **open_joysticks = NULL;
//List of currently connected joysticks (key - instance id, value - SDL_Joystick structure, joystick number)
static std::unordered_map<SDL_JoystickID, std::pair<SDL_Joystick *, SDL_JoystickID> > open_joysticks;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like overkill, currently we support a max of 16 joysticks, which means the joystick id we use fits into one byte... and the average gamer certainly has max 4 joysticks connected so everything would fit into one cache line using a dumb loop over an array. i'm pretty certain the overhead involved in the use of a map for such tiny amount of data is way slower

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which means the joystick id we use fits into one byte

actually, this could be derived from the array index, so it's for free. we only need to make sure to fix the ordering of the array on insertion/removal of a device

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I probably just use static array of 16 elements?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, sounds like the best option. in the unlikely case that more than 16 joys are connected, we just have to discard those > 15.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redone with static array. Note that there's nbr_joy variable declared as extern in ctrlssdl.h that is now only keeps number of joysticks detected at the start, not the actual number of currently connected joysticks, because joysticks can be removed from any element of the array upon disconnect, so frontends should not rely on that variable (though as far as I can tell, they don't).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added int get_number_of_joysticks()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, please proceed and use it in the interface frontend and remove nbr_joys. (just grep posix subdir for nbr_joy to find the file).
with that out of the way i can test it as soon as the gtk2 conversion is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I also make corresponding changes to gtk2 frontend in my branch?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

free( open_joysticks);
for (auto & it: open_joysticks) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd really cherish if we could stick to C++03 features for the time being, following the overall style of the existing code and not gratuitously breaking the build for people with older compilers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open_joysticks is now static array and is iterated with int index.

@rofl0r
Copy link
Collaborator

rofl0r commented Nov 4, 2024

thanks, i currently don't have time to test this exhaustively so i'll just go ahead and merge as-is.
if my testing reveals any issues i'm gonna fix it at that point.

@rofl0r rofl0r merged commit 3b1989a into TASEmulators:master Nov 4, 2024
9 checks passed
@thesourcehim thesourcehim deleted the linux_gamepad_hotplug branch November 4, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants