libretro / fuse-libretro

A port of the Fuse Unix Spectrum Emulator to libretro
GNU General Public License v3.0
36 stars 47 forks source link

Adding Joypad to Keyboard remapping option #50

Closed pjft closed 6 years ago

pjft commented 6 years ago

Hi.

I'm submitting a PR to add an option to remap the joypad buttons to simulate keyboard inputs.

It works as intended, but there are a few subtleties that I'd like others' help or inputs on. I don't think they're related to this particular change, but I'd love to be able to fix them as otherwise this is not really as functional as I'd like it to be. Also, in the process of troubleshooting everything that wasn't working as intended, I noticed that the overlay wasn't simulating the Symbol Shift input on Raspbian at least (it showed as Caps Shift). I'd be surprised if this had never been noticed, so I'm open to learning that it worked as intended on other OSes, but the current version does work here. An easy test is to load Cybernoid, for instance, go to redefine keys, and try to use the overlay to send a Symbol Shift event. It will register as Caps Shift. Other games exhibit similar behaviors (games that expect Symbol Shift to do something).

Thank you @leiradel for all the pointers in the issue I had opened, and apologies for the delay - most of my time with this has been on weekends, and my debugging has been mostly with logs. I don't have a proper dev environment at the moment, and it's also my first time with more core libretro work, so I appreciate any feedback and pointers.

Issues that I'd like to fix - either in this PR or separately:

Other outstanding questions about things that were there before:

Well, that's mostly it. I'd love to get anyone's feedback and pointers in the right direction in regards to anything I should change, or anything that I may have misinterpreted.

Thank you!

andres-asm commented 6 years ago

Thanks for your implementation.

That said, there is really no need for this... The is a frontend level implementation now, check dosbox/bluemsx.

Of course it's not perfect but it's like one line of code basically, and improving the RetroArch keymapper would benefit all cores.

leiradel commented 6 years ago

@fr500 I took a look at blueMSX and I can't see how it works. RETRO_DEVICE_MAPPER is a core side device, how can the frontend be aware of it? How do you configure the mappings in the frontend, you associate RetroPad buttons to keys? If so, why aren't the keys transparently passed to the core when the buttons are pressed? I can't understand even why the core has to be aware of all that if it's a frontend thing.

leiradel commented 6 years ago

@pjft thanks for the PR. RETRO_DEVICE_KEYBOARD_JOYSTICK is not really a real hardware device for the ZX Spectrum so I can't merge the code with it. Controller buttons should be translated to keyboard keys without the need of that device.

I'd advice to hold on the PR a bit though, until we can figure out this mapper thing @fr500 talked about.

pjft commented 6 years ago

Thanks both for your notes.

@fr500 I do apologize, though, but I'd need a bit more context here. When you say "there is no need for this", could you elaborate a bit - is it the functionality, the whole PR in the way it's done, or any specific part of the code?

As mentioned, I'd certainly like to make this happen in a way that makes the most sense, so I'm open to all feedback here.

The approach here was, per the previous conversation on the conversation I started last week similar to the one in 81-libretro.

@leiradel Certainly, happy to sort that out! As I mentioned, first time diving into this specific area so any pointers are more than welcome.

I had no idea that the devices had to be actual existing ones in real life, apologies. Would the recommendation be to not create one such fake hardware device and explicitly list "Keyboard" there, so that if someone selects that it accepts both physical keyboard inputs as well as joypad mappings to keyboard? Or would "none" be it?

EDIT: Or would it rather be something like in blueMSX, "RetroPad Mapper"?

Thanks.

Also, it seems there's a build failure for 3ds and wiiu, but from looking at the logs I can't figure out how it directly relates to the particular commit it seems to be flagging. Happy to revert it though.

Let me know how to best proceed in this regard then.

leiradel commented 6 years ago

In 81-libretro, there's a mapping from controller buttons to keyboard keys in the gloabal variable keybovl_t zx81ovl, field joymap, which is initialized to all zeros. In the update_variables function, when the user changes the controller button mapping, that field is updated to reflect the change.

In retro_run, which is called once per frame, keybovl_update is called to handle input. All button presses are checked in joymap. If the corresponding entry is 0, it's considered a regular joystick button and handled as such. If it's different from zero, it contains the key mapped to that button, and a keypress is simulated instead of a button press.

In fuse-libretro something similar to the joymap field must be created. An array of 16 input_key, on for each controller button, is enough. It has to be a global variable because it'll be accessed by multiple files. In libretro.c, initialize it with zeroes somewhere, probably in retro_load_game, and update it in update_variables to the correct input_key value depending on the user choice.

In src/compat/ui.c, function ui_event, change the translate function to return the corresponding joystick button if the entry in joymap is 0, or the change the joymap content if it's not 0. At line 148, you'll have to figure out if translate has returned a joystick button or a keyboard key, and generate the correct event. Maybe make translate also return the type of event?

After removing RETRO_DEVICE_KEYBOARD_JOYSTICK I think I can merge your PR regardless of the mapper @fr500 mentioned. It'll be a great addition to the core, until I can figure out what this mapping thing in the frontend is.

pjft commented 6 years ago

Hi @leiradel , thanks for the note.

Just to make sure I'm not misunderstanding this, I believe that this is fairly what I have implemented so far after your recommendation to look into 81-libretro.

Are you referring to a different thing or a change in the way I've implemented it in the PR (asides from the device which I fully understand), or were you overall describing what would be the expected implementation?

In regards to your suggestion, just to make sure I understood the proposal for how to handle those mappings:

Is that it?

Question: Would those mappings override the inputs for both Player 1 and Player 2? Should I maybe add options for Player 2 as separate? Only Player 1?

Let me know.

Thank you once again, and have a great day.

leiradel commented 6 years ago

Yes, you're right. I started digging into the 81-libretro implementation and documenting everything, and got carried out. You're PR is almost on target, you just have to remove the RETRO_DEVICE_KEYBOARD_JOYSTICK device and merge translate_keyboard and translate I believe.

pjft commented 6 years ago

Sounds great, that's more than fair. I'll follow up on those recommendations then, thank you for your time!

Have a great afternoon.

pjft commented 6 years ago

Hi all,

This has been redone according to the latest comments here, I believe, for your consideration.

As usual, feedback is appreciated.

Have a great evening, and thanks once again.

leiradel commented 6 years ago

Looks good! Some questions/considerations:

  1. Why are you limiting mappings to the 1st port only?
  2. Core options in core_vars must start fuse_
  3. Why are the core options joypad_start, joypad_l2, and joypad_r2 commented?
pjft commented 6 years ago

Oh my. Thanks for looking through that. Answers inline:

  1. Well, the keys are mapped to buttons, and having the same button for P1, ..., Pn mapped to the exact same key in the keyboard doesn't seem to match any use case I can think of. It especially breaks multiplayer games where one player would want to use the keyboard, as the other players would be mapped to exactly the same buttons. Unless we'd have options for remapping each of the players buttons (P1 Joypad Left, etc, P2 Joypad Left, etc). I also think 81-libretro does it like that, though that wasn't really a reason for me to pursue the P1 only mapping, more of a happy coincidence.
  2. Thanks. I'll fix that.
  3. Because I messed up a rebase, apologies. I was still trying to get those to work but I guess I messed that in the final rebase, my bad! Thank you for spotting those.
pjft commented 6 years ago

Apologies for the mess up.

I've now added support for L2, R2 and Start buttons to be remapped, and - I hope - I've named the variables correctly.

Let me know your thoughts in regards to whether to add the option for further players to map their keys as well, and what your considerations be in that regard.

Thank you.

leiradel commented 6 years ago

No apologies necessary, the PR is in great shape and works like a charm. If you want to add mappings for L3 and R3 (my bad, I forgot to mention them before), let me know and I'll hold the merge.

pjft commented 6 years ago

Oh, I completely forgot about those. I certainly will, I'll submit them tomorrow.

Thanks.

andres-asm commented 6 years ago

Hey @leiradel @pjft sorry I didn't remeber to reply. Any RETRO_DEVICE_KEYBOARD (or bit shifted RETRO_DEVICE_KEYBOARD) gets instant access to the keymapper. No need to do anything core-side.

This means, pressing Joystick A in the frontend can be passed as Keyboard Enter with zero effort.

I don't mind if you go this route instead, it's up to you, but I do think this belongs in frontend realm, that way we can work on ONE implementation and refine it to perfection instead of having to work on one implementation PER CORE.

In blueMSX when device is joypad you get this:

image

If you change it to RETRO_KEYBOARD it changes to this:

image

image

With one side benefit, the changes can be saved like other remaps.

Sorry for not replying earlier

andres-asm commented 6 years ago

DosBOX too:

image

Basically my method is... zero effort (in the core side)

Again, up to you guys.

pjft commented 6 years ago

Hi @fr500, and thanks for the detailed reply.

I see, I certainly want aware of that.

Let me explore that for the next few days and see what comes out of it, both advantages and drawbacks. I'm certainly keen on making this as good an implementation as possible, so this certainly feels like a good approach in principle.

Let me see what I get to and get back to you. :)

The only thing I can think of upfront is that for the games that used joysticks + a few other buttons, the challenge would probably still remain, though I'm also not sure how much of an issue that would be since ultimately those games would also allow you to play with just the keyboard altogether.

In regards to saving the keymaps, I see your point. In that case they'd be saved as game specific remaps. In the current case, though, I'm under the impression they can also be saved as game-specific core options.

Thank you. I'll look into it further. Have a fantastic day!

leiradel commented 6 years ago

@pjft raised a valid concern. ZX Spectrum joysticks only have up, down, left, and right, and one action button, and we want to continue using these as a regular joystick buttons.

However, many games use additional keyboard keys for gameplay, and since the RetroPad has many more buttons than those of the joystick it makes sense to map the spare ones to the keyboard.

So if the frontend keymapper is an all or nothing mapping from buttons to keys, it doens't solve this specific issue, and we would have to iterate it before removing this functionality from the core.

leiradel commented 6 years ago

@pjft I'll merge your PR when you add L3 and R3. Then we can discuss the frontend keymapper and understand if it fits the needs of this core.

andres-asm commented 6 years ago

I removed my post because I'm not certain anymore :P

Anyway I'm not 100% sure it blocks joypad inputs from being read I think it doesn't.

leiradel commented 6 years ago

@pjft thanks, will be merging soon

pjft commented 6 years ago

Hi both.

So, a few updates:

Let me know.

leiradel commented 6 years ago

@fr500 I think an example will make things more clear.

I need that Left, Right, Up, Down, and A, be passed to the core as their respective RetroPad button presses. B must be passed as the Return key, X as the Space key, and Y as the Left Shift key. The rest of the RetroPad buttons don't matter.

Can the frontend mapper be configured like that?

leiradel commented 6 years ago

I pushed a commit to add L3 and R3 support. Now, I don't have a pad with those buttons at hand, but I'm getting one to test tomorrow. As such, to confirm, this has not been tested. Treat it as such - don't merge yet until I report back, unless you happen to end up testing them yourselves.

Ok.

There's something I'm not thrilled with. The controls page shows the buttons being labeled as "Fire", "Fire", "Up", etc, and in the background it maps them to the Retro Joypad buttons A, B, X, etc. Now, if the user changes the default button mappings, it all gets messy for the user, as the actual "A" mapping, for instance, won't really match the controller's expected "A" button. Would anyone object if the labels change to "Fire (A)", "Up (B)", etc? Any other ideas? Is it a non-issue?

I don't understand.

andres-asm commented 6 years ago

It won't exclude UDLRA from the keymapper GUI ATM, so the user would have to make sure not to use a button twice, but other than that it will allow you to do so just fine.

In blueMSX I haven't added what you supported, just added it right now as a quick hack:

image

image

Notice that the gamepad mapper and the keymapper's columns are.. inverted, when I implemented the keymapper I did this on purpose to be able to tell easily and also because that allows a crucial feature that the gamepad remapper doesn't have which is to add an UNMAPPED option which is rather important.

At some time in the future I'd like to change the gamepad mapper columns to match with the keymapper.

image

Just wanted to show that there is no limitation from using the joystick and the frontend keymapper at the same time.

andres-asm commented 6 years ago

I think you should go ahead with what you have right now though, the frontend implementation still needs work

pjft commented 6 years ago

Of course, even I have trouble reading through what I wrote :)

I suppose it's also minor, so if after this it's still not clear, please don't spend your time trying to figure it out.

I expect the "A" button to be the right button in the control pad, like in a SNES controller.

However, in the Controls page, the assignment for the RETRO_DEVICE_ID_JOYPAD_A can be changed to any button in the controller. If you read the code, you'll see that the label for that button is "Fire".

Let's assume the user changes his mappings so that this "Fire" button is no longer the equivalent to the right button on the pad, but rather the one on the bottom (button "B" on the SNES pad).

Now, the user has his perceived button B assigned internally to RETRO_DEVICE_ID_JOYPAD_A. Still, he has no idea what button it maps to internally in the core.

The mapping options I implemented assign keys to these RETRO_DEVICE_ID_JOYPAD_XXX entries, so in effect when the user is now remapping a key to "Joypad Button A" in the new options menu, it will affect RETRO_DEVICE_ID_JOYPAD_A which is in turn mapped to button "B" on the SNES pad because of his changes.

That's why I was asking if it would be useful to add the label on the Controls page to what button it's mapping - so, "Fire (A)" or something.

Once again, this is a minor thing, so I'm not sure how much of an issue it will be, but thought I'd raise it here for the sake of completeness.

Best.

pjft commented 6 years ago

@fr500 Wow, thanks for detailing it. I am genuinely intrigued by that alternative.

I have to say I'm not one to push for something that won't make sense in the medium-to-long run, so if you feel that this should really be done via the keymapper, I honestly have no emotional attachment to my current implementation.

I trust yours and @leiradel 's judgement to make the most appropriate calls here.

Thank you both.

pjft commented 6 years ago

Hi all,

Just to confirm I have tested L3 and R3 and all works well.

I squashed it all into a single commit.

Thanks for the patience.

leiradel commented 6 years ago

Thanks people, lets have this mapping in the core until the frontend mapping has some more iterations.

pjft commented 6 years ago

Thank you both! Hope it helps for now.