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

improve linux joystick detection #902

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

allefant
Copy link
Contributor

This correctly identifies X/Y/Z axes as well as common button types.
More work needs to be done but this also adds enough log information
that someone could just send us an allegro.log and we could improve from
that.

My old xbox 360 controller has 100% of axes and buttons correctly
identified with his.

Before the patch all the buttons were just named B and the axes were
mapped completely wrong, mapping triggers as axes and swapping X and Y
axes of the sticks.

This correctly identifies X/Y/Z axes as well as common button types.
More work needs to be done but this also adds enough log information
that someone could just send us an allegro.log and we could improve from
that.

My old xbox 360 controller has 100% of axes and buttons correctly
identified with his.

Before the patch all the buttons were just named B and the axes were
mapped completely wrong, mapping triggers as axes and swapping X and Y
axes of the sticks.
@fatcerberus
Copy link
Contributor

Does this work properly with non-Xbox gamepads? I know we've had issues with Xbox controllers in the past where attempting to fix them breaks things with other devices.

@allefant
Copy link
Contributor Author

allefant commented Apr 14, 2018

I don't know. But if you look at the changes the old code just was wrong - e.g. it simply ignored whether axes are X/Y/Z and just alternated between X and Y. And it completely ignored the button type. So I would say the chance that the new code is any worse than the old is close to zero :)

Still - do you have any more information about those issues you had in the past?

Copy link

@igormorgado igormorgado left a comment

Choose a reason for hiding this comment

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

just a readout

if (strlen(name) < 4) {
al_draw_text(font, fg, x + 13, y + 8, ALLEGRO_ALIGN_CENTRE, name);
char name2[4];
if (strlen(name) >= 4) {

Choose a reason for hiding this comment

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

is this not the case for a strncpy?

strncpy(name2, name, 3);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so - strncpy will not copy the final 0.

@igormorgado
Copy link

about the axis auto discovery, I think (cannot test all joysticks in the world) that will probably break something, SDL went to the path to manual configuration instead auto discovery. They probably had a good reason for that.

@fatcerberus
Copy link
Contributor

fatcerberus commented Apr 14, 2018

Still - do you have any more information about those issues you had in the past?

They weren't my issues--I just remember there being a good amount of discussion in the past w.r.t. incorrect behavior for Xbox controllers; I'm also pretty sure I remember a fix being reverted because it turned out to break other things.

edit: This was the reverted change I was thinking of: #662
It seems that was a change specifically for macOS, so might not be relevant to this PR after all. Feel free to pretend I didn't say anything 😄

@allefant
Copy link
Contributor Author

allefant commented Apr 14, 2018

Let's see if someone can test it with one or two other joysticks. If something currently has 4 axes and designates them all as X axis, it would still show up as 2 sticks with X/Y axis each right now, since the current code completely ignores the designation. My change would make it show up as 4 sticks with a single X axis each. So yes, I can see cases where something would end up worse than before if evdev returns wrong information to begin with. (With my only test device evdev returns correct information.)

I looked at the SDL axis/button database and it is very easy to use: https://github.com/gabomdq/SDL_GameControllerDB/blob/master/gamecontrollerdb.txt

And it also means the mapping would be identical under Windows and OSX so it would be a big advantage. However it seems SDL is actually using this only in their GameController API - their Joystick API works the same as ours and does not use that database at all, just a direct mapping. So we probably would have to investigate this a bit more and maybe also use an additional API for it.

@SiegeLord
Copy link
Member

SiegeLord commented Apr 21, 2018

Alright, I tested this with 2 of my joysticks, 3 configurations (F310 has a physical switch between DInput and XInput).

Current code

F310, XInput

  • 11 buttons (sees the center button)
  • 4 2D sticks, triggers and right thumb mixed up between 2 sticks

F310, DInput

  • 12 buttons, (triggers as buttons, no center button)
  • 3 2D sticks

PSX controller in adapter

  • 12 buttons (triggers as buttons, but this is fine as PSX controller has no pressure sensitivity for triggers)
  • 3 2D sticks

New code

F310 XInput

  • 11 buttons (have names, center button detected), side bumpers incorrectly called triggers
  • 2 3D sticks (triggers + thumbs combined)

F310 DInput

  • 12 buttons (triggers as buttons)
  • 1 3D stick, combining left thumb + one axis of right thumb
  • 1 1D stick, other axis of right thumb
  • 1 2D stick, D-pad

PSX controller in adapter

  • Same as F310 DInput above, screwed up sticks

So... hit and miss. The only concrete improvement are the button names for one of the controllers (although still not usable as is, as it calls side bumpers triggers).

@beoran
Copy link
Contributor

beoran commented Apr 22, 2018 via email

@allefant
Copy link
Contributor Author

Are we still not doing this with the PR? (except for the part about legacy drivers which is in fact keeping a custom database for those yourself)

@beoran
Copy link
Contributor

beoran commented Apr 23, 2018 via email

@elias-pschernig
Copy link
Member

I re-read it, I feel that's exactly what the PR does, maybe minus some smaller things. But the main problem is it seems two of SiegeLord's joystick simply do not follow the new Linux kernel guidelines at all, likely using some kind of legacy driver from before those gamepad specifications were written. The only way to make those work seems to be (as that document admits) to keep a database of device uuids and hardcode a mapping for them.

@beoran
Copy link
Contributor

beoran commented Apr 23, 2018 via email

@SiegeLord
Copy link
Member

I'd argue that none of my controllers are mapped correctly. The correct mapping for F310 (and the XBox controller it is emulating) is 11 butons, 2 1D axes for the triggers and 2 2D axes for the analog sticks. I think we should hold off on merging this code until we got a remapping setup in place.

@fatcerberus
Copy link
Contributor

2 1D axes for the triggers

I note for the record that Windows itself seems to treat Xbox controllers as having a single 1D axis which is the sum of both triggers. Kind of weird. No idea if this is the same for other platforms.

@SiegeLord
Copy link
Member

SiegeLord commented Apr 28, 2018

Hmm... well, for me on Windows Allegro does treat F310's triggers as 2 1D sticks (interestingly, it treats the D-pad as separate 4 buttons, and doesn't detect the center button). Notable, F310 in DInput mode is not detected at all!

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.

6 participants