Re: [hatari-devel] [PATCH] Add joystick button index configuration

[Robin Sergeant <robin.sergeant@xxxxxxxxx>]

Hi Eero,

Thanks for the feedback (see initial comments inline below)

>
> Hi Robin,
>
> On 25.7.2023 2.30, Robin Sergeant wrote:
> > Sorry for the spam, but the patch in my previous email was corrupted by
> > gmail.  So re-sending using git send-email to correct that.
>
> Thanks for the patch!
>
>
> > Currently the main joystick button (fire) is hardcoded as SDL button
> > index 0, which is not the best choice for every joystick.  For example
> > on my Speedlink Competition Pro the buttons are arranged like this:
> >
> > (3)(0) - main buttons
> > (4)(1) - small buttons behind
> >
> > Button 0 is therefore only the correct choice for left handed players,
> > and others (like me!) would prefer to use button 3.  As this was
> > driving me nuts I've created a patch that you might like to include?
>
> While your patch looks fine, configuring joysticks is not Hatari
> specific problem, it applies to all programs using joysticks.
>
> Therefore such configuring is expected to be done outside / before
> starting Hatari, using some OS specific utility for it.
>
> Many years ago when Hatari still used SDL1, I've used this for
> calibration on Linux:
>         https://jstest-gtk.gitlab.io/
>
> But nowadays things seem to work differently.  Here are some notes on
> how to do it now, and with SDL2:
> https://wiki.archlinux.org/title/Gamepad#Remapping_of_gamepad_buttons_and_more
>

I couldn't see an obvious way of remapping the buttons in OSX, but
SDL_GAMECONTROLLERCONFIG does sound like a possibility.  I will
investigate more at the weekend.

Looking at other emulators FS-UAE does not have a problem because it
just treats all buttons as fire (so all 4 buttons on my joystick
work).  VICE is similar, but only 3 of the 4 buttons act as FIRE
(button 1 doesn't seem to do anything). MAME lets you assign joystick
buttons to game buttons from the config UI (just need to press each
button).

As a quick and dirty fix, would it make sense to treat the currently
unused buttons as FIRE?  That would solve my particular problem (if
button 3 was treated like button 0), but I feel that more control
would be better to cater for other joysticks and user preferences.
Hence the config in my patch!

> > It adds 3 new integers config values for each Joystick, e.g.
> >
> > [Joystick1]
> > ...
> > nJoyBut1Index = 3
> > nJoyBut2Index = 4
> > nJoyBut3Index = 0
>  >
> > NB But2 is the space/jump button, and But3 is the auto fire button.
> > Defaults are 0, 1 and 2 (matching the current behaviour).  I haven't
> > added these to the UI, but the config file can be manually edited to
> > swap the buttons.
>
> As configuring joysticks outside of Hatari seems more complicated
> nowadays, doing it in Hatari itself could be considered, but then it
> would definitely need to have support in SDL GUI, so that  Hatari users
> know about it & can use it easily.
>
> What the others think?  Thomas?  Nicolas?
>
>
> Because SDL GUI changes are quite a bit of work, it's best to first mail
> an ASCII proposal of what the new GUI layout would look like.
>

I agree that a GUI change would be needed.  Rather than expecting
users to know the indexes of their buttons it would be good to offer
simple config like MAKE.  Perhaps a way to cycle through each Hatari
game action (Fire, Jump/Space, auto-fire) with text like this:

FIRE (button 0) - Press button to change

Something along those lines anyway.  I could dig into the code and
look at the current UI more closely...

> Or, you could just test how SDL2 joystick mapping works, and contribute
> documentation update for that...
>
>
>  > Any comments or questions?
>
> Some generic comments on contributions:
>
> Changes to different functionality (e.g. config support for joy button
> mappings, and GUI support for them) should be in separate patches, as
> should any white-space changes.
>

I didn't intentionally make white-space changes, but my editor was set
to strip trailing spaces.  I know this can be annoying when you are
trying to compare code changes!

> Patches should be sent as attachment (not inline, like they're now) so
> they can be saved & applied directly.
>

I agree attachments are easier, but I wasn't sure if this list
accepted them.  However, you can actually apply patches sent with "git
send-email" directly by using "git am".  At least for my second email
this worked fine (I saved it from Gmail and applied it to another
branch without making any edits to the email contents).  I'm only an
occasional Git user, but once I figured out how to do this it seemed
really clever.

> It's better to commit the changes to a local tree first and use
> something like "git format-patch HEAD~1" [1] to produce the patch(es),
> so they can be applied directly to Hatari Git with correct attributions.

I think that's what I did, but documenting a workflow somewhere would
be great.  All projects do things a little differently of course.

>
> Proposals for following documentation updates would also be great:
> * Note about the new feature for release-notes.txt
> * Contributors list update for authors.txt
>
> (We may slightly update the doc texts & commit descriptions before
> pushing the changes.)
>
>
>         - Eero
>
> [1] Number after "HEAD~" indicates how many commits from the git branch
> top you want to include to given patch set.
>
> > Br,
> >
> > Robin
> > ---
> >   src/configuration.c          | 25 +++++++++++++++++++++++--
> >   src/includes/configuration.h |  1 +
> >   src/joy.c                    | 14 +++++++++-----
> >   3 files changed, 33 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/configuration.c b/src/configuration.c
> > index de6006d0..e07b7327 100644
> > --- a/src/configuration.c
> > +++ b/src/configuration.c
> > @@ -108,6 +108,9 @@ static const struct Config_Tag configs_Joystick0[] =
> >       { "bEnableAutoFire", Bool_Tag, &ConfigureParams.Joysticks.Joy[0].bEnableAutoFire },
> >       { "bEnableJumpOnFire2", Bool_Tag, &ConfigureParams.Joysticks.Joy[0].bEnableJumpOnFire2 },
> >       { "nJoyId", Int_Tag, &ConfigureParams.Joysticks.Joy[0].nJoyId },
> > +     { "nJoyBut1Index", Int_Tag, &ConfigureParams.Joysticks.Joy[0].nJoyBut1Index },
> > +     { "nJoyBut2Index", Int_Tag, &ConfigureParams.Joysticks.Joy[0].nJoyBut2Index },
> > +     { "nJoyBut3Index", Int_Tag, &ConfigureParams.Joysticks.Joy[0].nJoyBut3Index },
> >       { "kUp", Key_Tag, &ConfigureParams.Joysticks.Joy[0].nKeyCodeUp },
> >       { "kDown", Key_Tag, &ConfigureParams.Joysticks.Joy[0].nKeyCodeDown },
> >       { "kLeft", Key_Tag, &ConfigureParams.Joysticks.Joy[0].nKeyCodeLeft },
> > @@ -123,6 +126,9 @@ static const struct Config_Tag configs_Joystick1[] =
> >       { "bEnableAutoFire", Bool_Tag, &ConfigureParams.Joysticks.Joy[1].bEnableAutoFire },
> >       { "bEnableJumpOnFire2", Bool_Tag, &ConfigureParams.Joysticks.Joy[1].bEnableJumpOnFire2 },
> >       { "nJoyId", Int_Tag, &ConfigureParams.Joysticks.Joy[1].nJoyId },
> > +     { "nJoyBut1Index", Int_Tag, &ConfigureParams.Joysticks.Joy[1].nJoyBut1Index },
> > +     { "nJoyBut2Index", Int_Tag, &ConfigureParams.Joysticks.Joy[1].nJoyBut2Index },
> > +     { "nJoyBut3Index", Int_Tag, &ConfigureParams.Joysticks.Joy[1].nJoyBut3Index },
> >       { "kUp", Key_Tag, &ConfigureParams.Joysticks.Joy[1].nKeyCodeUp },
> >       { "kDown", Key_Tag, &ConfigureParams.Joysticks.Joy[1].nKeyCodeDown },
> >       { "kLeft", Key_Tag, &ConfigureParams.Joysticks.Joy[1].nKeyCodeLeft },
> > @@ -138,6 +144,9 @@ static const struct Config_Tag configs_Joystick2[] =
> >       { "bEnableAutoFire", Bool_Tag, &ConfigureParams.Joysticks.Joy[2].bEnableAutoFire },
> >       { "bEnableJumpOnFire2", Bool_Tag, &ConfigureParams.Joysticks.Joy[2].bEnableJumpOnFire2 },
> >       { "nJoyId", Int_Tag, &ConfigureParams.Joysticks.Joy[2].nJoyId },
> > +     { "nJoyBut1Index", Int_Tag, &ConfigureParams.Joysticks.Joy[2].nJoyBut1Index },
> > +     { "nJoyBut2Index", Int_Tag, &ConfigureParams.Joysticks.Joy[2].nJoyBut2Index },
> > +     { "nJoyBut3Index", Int_Tag, &ConfigureParams.Joysticks.Joy[2].nJoyBut3Index },
> >       { "kUp", Key_Tag, &ConfigureParams.Joysticks.Joy[2].nKeyCodeUp },
> >       { "kDown", Key_Tag, &ConfigureParams.Joysticks.Joy[2].nKeyCodeDown },
> >       { "kLeft", Key_Tag, &ConfigureParams.Joysticks.Joy[2].nKeyCodeLeft },
> > @@ -153,6 +162,9 @@ static const struct Config_Tag configs_Joystick3[] =
> >       { "bEnableAutoFire", Bool_Tag, &ConfigureParams.Joysticks.Joy[3].bEnableAutoFire },
> >       { "bEnableJumpOnFire2", Bool_Tag, &ConfigureParams.Joysticks.Joy[3].bEnableJumpOnFire2 },
> >       { "nJoyId", Int_Tag, &ConfigureParams.Joysticks.Joy[3].nJoyId },
> > +     { "nJoyBut1Index", Int_Tag, &ConfigureParams.Joysticks.Joy[3].nJoyBut1Index },
> > +     { "nJoyBut2Index", Int_Tag, &ConfigureParams.Joysticks.Joy[3].nJoyBut2Index },
> > +     { "nJoyBut3Index", Int_Tag, &ConfigureParams.Joysticks.Joy[3].nJoyBut3Index },
> >       { "kUp", Key_Tag, &ConfigureParams.Joysticks.Joy[3].nKeyCodeUp },
> >       { "kDown", Key_Tag, &ConfigureParams.Joysticks.Joy[3].nKeyCodeDown },
> >       { "kLeft", Key_Tag, &ConfigureParams.Joysticks.Joy[3].nKeyCodeLeft },
> > @@ -168,6 +180,9 @@ static const struct Config_Tag configs_Joystick4[] =
> >       { "bEnableAutoFire", Bool_Tag, &ConfigureParams.Joysticks.Joy[4].bEnableAutoFire },
> >       { "bEnableJumpOnFire2", Bool_Tag, &ConfigureParams.Joysticks.Joy[4].bEnableJumpOnFire2 },
> >       { "nJoyId", Int_Tag, &ConfigureParams.Joysticks.Joy[4].nJoyId },
> > +     { "nJoyBut1Index", Int_Tag, &ConfigureParams.Joysticks.Joy[4].nJoyBut1Index },
> > +     { "nJoyBut2Index", Int_Tag, &ConfigureParams.Joysticks.Joy[4].nJoyBut2Index },
> > +     { "nJoyBut3Index", Int_Tag, &ConfigureParams.Joysticks.Joy[4].nJoyBut3Index },
> >       { "kUp", Key_Tag, &ConfigureParams.Joysticks.Joy[4].nKeyCodeUp },
> >       { "kDown", Key_Tag, &ConfigureParams.Joysticks.Joy[4].nKeyCodeDown },
> >       { "kLeft", Key_Tag, &ConfigureParams.Joysticks.Joy[4].nKeyCodeLeft },
> > @@ -183,6 +198,9 @@ static const struct Config_Tag configs_Joystick5[] =
> >       { "bEnableAutoFire", Bool_Tag, &ConfigureParams.Joysticks.Joy[5].bEnableAutoFire },
> >       { "bEnableJumpOnFire2", Bool_Tag, &ConfigureParams.Joysticks.Joy[5].bEnableJumpOnFire2 },
> >       { "nJoyId", Int_Tag, &ConfigureParams.Joysticks.Joy[5].nJoyId },
> > +     { "nJoyBut1Index", Int_Tag, &ConfigureParams.Joysticks.Joy[5].nJoyBut1Index },
> > +     { "nJoyBut2Index", Int_Tag, &ConfigureParams.Joysticks.Joy[5].nJoyBut2Index },
> > +     { "nJoyBut3Index", Int_Tag, &ConfigureParams.Joysticks.Joy[5].nJoyBut3Index },
> >       { "kUp", Key_Tag, &ConfigureParams.Joysticks.Joy[5].nKeyCodeUp },
> >       { "kDown", Key_Tag, &ConfigureParams.Joysticks.Joy[5].nKeyCodeDown },
> >       { "kLeft", Key_Tag, &ConfigureParams.Joysticks.Joy[5].nKeyCodeLeft },
> > @@ -609,6 +627,9 @@ void Configuration_SetDefault(void)
> >               ConfigureParams.Joysticks.Joy[i].bEnableAutoFire = false;
> >               ConfigureParams.Joysticks.Joy[i].bEnableJumpOnFire2 = true;
> >               ConfigureParams.Joysticks.Joy[i].nJoyId = (i > maxjoy ? maxjoy : i);
> > +             ConfigureParams.Joysticks.Joy[i].nJoyBut1Index = 0;
> > +             ConfigureParams.Joysticks.Joy[i].nJoyBut2Index = 1;
> > +             ConfigureParams.Joysticks.Joy[i].nJoyBut3Index = 2;
> >               ConfigureParams.Joysticks.Joy[i].nKeyCodeUp = SDLK_UP;
> >               ConfigureParams.Joysticks.Joy[i].nKeyCodeDown = SDLK_DOWN;
> >               ConfigureParams.Joysticks.Joy[i].nKeyCodeLeft = SDLK_LEFT;
> > @@ -631,12 +652,12 @@ void Configuration_SetDefault(void)
> >       ConfigureParams.Keyboard.nKbdLayout = TOS_LANG_UNKNOWN;
> >       ConfigureParams.Keyboard.nLanguage = TOS_LANG_UNKNOWN;
> >       strcpy(ConfigureParams.Keyboard.szMappingFileName, "");
> > -
> > +
> >       /* Set defaults for Shortcuts */
> >       ConfigureParams.Shortcut.withoutModifier[SHORTCUT_OPTIONS] = SDLK_F12;
> >       ConfigureParams.Shortcut.withoutModifier[SHORTCUT_FULLSCREEN] = SDLK_F11;
> >       ConfigureParams.Shortcut.withoutModifier[SHORTCUT_PAUSE] = SDLK_PAUSE;
> > -
> > +
> >       ConfigureParams.Shortcut.withModifier[SHORTCUT_DEBUG] = SDLK_PAUSE;
> >       ConfigureParams.Shortcut.withModifier[SHORTCUT_OPTIONS] = SDLK_o;
> >       ConfigureParams.Shortcut.withModifier[SHORTCUT_FULLSCREEN] = SDLK_f;
> > diff --git a/src/includes/configuration.h b/src/includes/configuration.h
> > index 86e26c37..fa92760a 100644
> > --- a/src/includes/configuration.h
> > +++ b/src/includes/configuration.h
> > @@ -169,6 +169,7 @@ typedef struct
> >     bool bEnableAutoFire;
> >     bool bEnableJumpOnFire2;
> >     int nJoyId;
> > +  int nJoyBut1Index, nJoyBut2Index, nJoyBut3Index;
> >     int nKeyCodeUp, nKeyCodeDown, nKeyCodeLeft, nKeyCodeRight, nKeyCodeFire;
> >   } JOYSTICK;
> >
> > diff --git a/src/joy.c b/src/joy.c
> > index 3fd0b884..10d1fae0 100644
> > --- a/src/joy.c
> > +++ b/src/joy.c
> > @@ -194,8 +194,12 @@ void Joy_UnInit(void)
> >    * Read details from joystick using SDL calls
> >    * NOTE ID is that of SDL
> >    */
> > -static bool Joy_ReadJoystick(int nSdlJoyID, JOYREADING *pJoyReading)
> > +static bool Joy_ReadJoystick(int nStJoyId, JOYREADING *pJoyReading)
> >   {
> > +     int nSdlJoyID = ConfigureParams.Joysticks.Joy[nStJoyId].nJoyId;
> > +     int button1 = ConfigureParams.Joysticks.Joy[nStJoyId].nJoyBut1Index;
> > +     int button2 = ConfigureParams.Joysticks.Joy[nStJoyId].nJoyBut2Index;
> > +     int button3 = ConfigureParams.Joysticks.Joy[nStJoyId].nJoyBut3Index;
> >       unsigned hat = SDL_JoystickGetHat(sdlJoystick[nSdlJoyID], 0);
> >
> >       /* Joystick is OK, read position from the configured joystick axis */
> > @@ -211,12 +215,12 @@ static bool Joy_ReadJoystick(int nSdlJoyID, JOYREADING *pJoyReading)
> >       if (hat & SDL_HAT_DOWN)
> >               pJoyReading->YPos = 32767;
> >       /* Sets bit #0 if button #1 is pressed: */
> > -     pJoyReading->Buttons = SDL_JoystickGetButton(sdlJoystick[nSdlJoyID], 0);
> > +     pJoyReading->Buttons = SDL_JoystickGetButton(sdlJoystick[nSdlJoyID], button1);
> >       /* Sets bit #1 if button #2 is pressed: */
> > -     if (SDL_JoystickGetButton(sdlJoystick[nSdlJoyID], 1))
> > +     if (SDL_JoystickGetButton(sdlJoystick[nSdlJoyID], button2))
> >               pJoyReading->Buttons |= JOYREADING_BUTTON2;
> >       /* Sets bit #2 if button #3 is pressed: */
> > -     if (SDL_JoystickGetButton(sdlJoystick[nSdlJoyID], 2))
> > +     if (SDL_JoystickGetButton(sdlJoystick[nSdlJoyID], button3))
> >               pJoyReading->Buttons |= JOYREADING_BUTTON3;
> >
> >       return true;
> > @@ -308,7 +312,7 @@ uint8_t Joy_GetStickData(int nStJoyId)
> >               }
> >
> >               /* Read real joystick and map to emulated ST joystick for emulation */
> > -             if (!Joy_ReadJoystick(nSdlJoyId, &JoyReading))
> > +             if (!Joy_ReadJoystick(nStJoyId, &JoyReading))
> >               {
> >                       /* Something is wrong, we cannot read the joystick from SDL */
> >                       bJoystickWorking[nSdlJoyId] = false;
>
>