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

Support SDL2 half axes and inverted axes mappings. #38151

Merged
merged 3 commits into from
May 13, 2020

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Apr 23, 2020

Resolves #8368.

Note 1:
Although this is a breaking change, game controllers that worked before should continue to work in the same way. That been said, I don't have a game controller to test it with; so it will need to be tested by those that do.

The main change that may cause things to break is moving the left and right trigger "buttons" to axis controls i.e. where they should be. To support this move, I've removed the hacks that were in place to simulate axis events, but again, testing on all platforms will be required.

Note 2:
This change only enables support for SDL2 half axes and inverted axes. I have not implemented the support for controllers with half and inverted axes. Hopefully, people with access to game controllers that use these features will now be able to do this.

Edit: The third commit now implements half axis and inverted axis mappings.

I've split the PR into three commits:

  • The first commit enables the parsing of all SDL2 v2.10.0 compatible game controller mapping entries found in the SDL_GameControllerDB; including half axes and inverted axes. To parse the previously excluded entries, requires deleting core/input/default_controller_mappings.gen.cpp so it can be recreated.
    I have also not deleted the old 204 and 205 databases or the Godot database, because I don't know why they are there, but I suggest they are now deleted.
    Edit: Deleted with Input: Drop obsolete versions of SDL gamecontrollerdb #38292
  • The second commit replaces all the JoystickList enum entries with the two SDL enums entries: SDLJoyButton and SDLJoyAxis. Obviously any user, plugin, gdnative, etc. code that was using the old entries will need to be updated to use the new enums too.
  • The third commit implements the new half and inverted axis mappings.

Updated game controller test project from @akien-mga:
Joypads.zip
Edit: Now includes highlighting the axes activated on the joypad.

@akien-mga
Copy link
Member

akien-mga commented Apr 28, 2020

I have also not deleted the old 204 and 205 databases or the Godot database, because I don't know why they are there, but I suggest they are now deleted.

Yeah I've also been thinking about deleting them. My understanding is that the gamecontrollerdb.txt should fully supersede the 204 and 205 ones, even if we don't support some of the bindings with half axes or inverted axes.

As for the Godot-specific DB, its changes should be upstreamed if they are still relevant, or dropped.

Edit: I made #38292 to drop the obsolete mappings.

@akien-mga akien-mga added needs testing cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 28, 2020
@akien-mga
Copy link
Member

@madmiraal Just a heads-up that #38295 will introduce conflicts with this PR, but which should hopefully be easy to resolve.

I preferred to do this change before merging your PR to avoid breaking the continuity in the git history when checking git log or the History view on GitHub.

@akien-mga
Copy link
Member

I gave this a try on Linux but I get a crash when sending any event from my (generic) PS3 controller:

[1] /lib64/libc.so.6(+0x3dcc0) [0x7f159d0fdcc0] (??:0)
[2] InputFilter::_get_mapped_button_event(InputFilter::JoyDeviceMapping const&, int) (/home/akien/Projects/godot/godot.git/core/input/input_filter.cpp:1081)
[3] InputFilter::joy_button(int, int, bool) (/home/akien/Projects/godot/godot.git/core/input/input_filter.cpp:890 (discriminator 1))
[4] JoypadLinux::process_joypads() (/home/akien/Projects/godot/godot.git/platform/linuxbsd/joypad_linux.cpp:487)
[5] OS_LinuxBSD::run() (/home/akien/Projects/godot/godot.git/platform/linuxbsd/os_linuxbsd.cpp:259)
[6] godot-git(main+0xfa) [0x17fd38c] (/home/akien/Projects/godot/godot.git/platform/linuxbsd/godot_linuxbsd.cpp:57)
[7] /lib64/libc.so.6(__libc_start_main+0xeb) [0x7f159d0e6d8b] (??:0)
[8] godot-git(_start+0x2a) [0x17fd1ea] (/home/iurt/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/start.S:122)

Both the editor and the game crash similarly when they get an event.

Here's a modified version of the joypads demo I use for testing:
joypads.zip

@madmiraal
Copy link
Contributor Author

@akien-mga Thanks for testing. I was trying to be clever with iterating through a Vector's pointer and expecting a null at the end, which there isn't. I'll fix it.

@madmiraal
Copy link
Contributor Author

Rebased following #38295, fixed crash reported by @akien-mga and renamed enums JoyButtonList and JoyAxisList and renamed all their values to JOY_*

Updated the test project to work with master and all these changes:
Joypads.zip

@akien-mga
Copy link
Member

Thanks, I'll review and test again today.

@akien-mga
Copy link
Member

I confirm that it's working fine now on Linux.

The joypads demo doesn't work perfectly though as many of the button numbers got reassigned to new buttons, so what it shows being pressed is different from what the user presses. But I assume it's expected as per the breaks compat notice - I expect that the numbering used matching SDL's?

Notably the triggers on the PS3 controllers became axes instead of buttons, but I think that's on purpose? For me they only give 0/1 values, but I guess it's simply that my generic PS3 controller doesn't support intermediate values.

I'll see if I can port the demo properly for testing.

@akien-mga akien-mga requested review from groud and BastiaanOlij April 29, 2020 09:37
@akien-mga
Copy link
Member

One concern I have with these changes is that it makes the API specific to a "standard" Xbox 360 controller, which can typically map to a PlayStation controller, but which might lack some extra buttons/axes for other types of controllers, like VR ones.

See #12743 where @BastiaanOlij had to increase the number of supported axes from 8 to 10 for OpenVR.

I think that's also why the previous API used non-descriptive IDs like JOY_BUTTON_0, as what 0 corresponds to highly depends on what controller is being used. It's A on Xbox 360, Cross on PS, and can be something completely different on other joysticks or gamepads. The API thus provided aliases for common gamepad values to their matching numbered IDs.

I don't think it's bad per se to standardize on the Xbox layout as it's what is most commonly used in games, but that's a design change which warrants more discussion than improving support for SDL2 mappings, and we still need to allow flexibility for non-gamepads.

@akien-mga akien-mga requested a review from punto- April 29, 2020 09:50
@madmiraal
Copy link
Contributor Author

The joypads demo doesn't work perfectly though as many of the button numbers got reassigned to new buttons, so what it shows being pressed is different from what the user presses. But I assume it's expected as per the breaks compat notice - I expect that the numbering used matching SDL's?

Notably the triggers on the PS3 controllers became axes instead of buttons, but I think that's on purpose? For me they only give 0/1 values, but I guess it's simply that my generic PS3 controller doesn't support intermediate values.

I'll see if I can port the demo properly for testing.

Yes, the numbering now matches the SDL order. I've updated the test project so the buttons are in the new order, the triggers are now axes, and it now includes the "guide" button:
Joypads.zip

One concern I have with these changes is that it makes the API specific to a "standard" Xbox 360 controller, which can typically map to a PlayStation controller, but which might lack some extra buttons/axes for other types of controllers, like VR ones

I think that's also why the previous API used non-descriptive IDs like JOY_BUTTON_0, as what 0 corresponds to highly depends on what controller is being used. It's A on Xbox 360, Cross on PS, and can be something completely different on other joysticks or gamepads. The API thus provided aliases for common gamepad values to their matching numbered IDs.

I don't think it's bad per se to standardize on the Xbox layout as it's what is most commonly used in games, but that's a design change which warrants more discussion than improving support for SDL2 mappings, and we still need to allow flexibility for non-gamepads.

There no limit on the number of buttons or axes a controller can use. The SDL mapping simply makes sure that, for those controllers that have mappings, the controllers all send the same signals. The only limit is within SDL mappings, the number of outputs that can be mapped to. These are specified as the words that the enums are aligned to. It has nothing to do with the number of buttons or axis a controller has.

Note: There is a separate limit: the Godot Input mapping, which is currently set to the number of SDL buttons and twice the number of SDL axes:

static const char *_button_names[JOY_BUTTON_MAX] = {
"DualShock Cross, Xbox A, Nintendo B",
"DualShock Circle, Xbox B, Nintendo A",
"DualShock Square, Xbox X, Nintendo Y",
"DualShock Triangle, Xbox Y, Nintendo X",
"L, L1",
"R, R1",
"L2",
"R2",
"L3",
"R3",
"Select, DualShock Share, Nintendo -",
"Start, DualShock Options, Nintendo +",
"D-Pad Up",
"D-Pad Down",
"D-Pad Left",
"D-Pad Right"
};
static const char *_axis_names[JOY_AXIS_MAX * 2] = {
" (Left Stick Left)",
" (Left Stick Right)",
" (Left Stick Up)",
" (Left Stick Down)",
" (Right Stick Left)",
" (Right Stick Right)",
" (Right Stick Up)",
" (Right Stick Down)",
"", "", "", "",
"", " (L2)",
"", " (R2)"
};

There is no reason to link these to the SDL limits and the axes descriptions should be updated. I can do that as part of this PR if you like.

@madmiraal
Copy link
Contributor Author

Rebased following #38587.

@akien-mga
Copy link
Member

There are style issues following #38621: https://travis-ci.org/github/godotengine/godot/jobs/686479042

Otherwise let's merge it once CI is happy, from my tests this works quite well and there's still time to adjust further before 4.0 :)

I marked this for cherry-picking in 3.2 but this will likely require a slimmed down version that preserves full compatibility while adding support for the new bindings - it doesn't have to happen right away, we can wait to have more testing in master first.

@madmiraal
Copy link
Contributor Author

New clang-format changes applied.

I'll have a look at whether I can get a non-breaking version working for 3.2.

@akien-mga akien-mga merged commit aebe036 into godotengine:master May 13, 2020
@akien-mga
Copy link
Member

Thanks a ton!


BIND_GLOBAL_ENUM_CONSTANT(JOY_VR_GRIP);
BIND_GLOBAL_ENUM_CONSTANT(JOY_VR_PAD);
BIND_GLOBAL_ENUM_CONSTANT(JOY_VR_TRIGGER);
Copy link
Member

@aaronfranke aaronfranke May 13, 2020

Choose a reason for hiding this comment

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

So now the VR values are just gone? I use these, I would appreciate them staying in the engine...

EDIT: I can re-add these myself if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have documentation on how the VR buttons and axes map onto the SDL buttons and axes? I removed them, because I didn't know how the mapped. The old values were limited to these three and they didn't make sense. Furthermore, not only has the order of the enums changed to align with the "official" SDL values, triggers are now axes and not buttons; so simply giving them the old values won't work either.

Copy link
Member

@aaronfranke aaronfranke May 14, 2020

Choose a reason for hiding this comment

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

I don't understand what you mean. My HTC Vive's triggers are dual-stage, same with my Steam controller. They are both buttons and axes.

If this PR removed support for such things, that's absolutely terrible and I think this part should be reverted. I don't see why adding half-axes requires removing dual-stage triggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR implements the SDL mappings as SDL does. If your controller's triggers have two functions then my assumption is this would be reflected in their mappings. If not, then I'm missing something. It would be appreciated if you could test your controllers and provide feedback so things can be fixed if needed.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 15, 2020
@hoontee
Copy link
Contributor

hoontee commented May 29, 2020

Why remove L2/R2 button controls? Couldn't you keep both button and axis controls?

@madmiraal
Copy link
Contributor Author

Why remove L2/R2 button controls? Couldn't you keep both button and axis controls?

Simply because L2 and R2 are axes and not buttons. As stated above I assume controller's with dual-stage triggers have triggers mapped to buttons too.

It's also easy enough to convert a (half) axis event into a button event.
Instead of:
if Input.is_joy_button_pressed(device, button):
You can use:
if Input.get_joy_axis(device, axis) > 0.5:

@hoontee
Copy link
Contributor

hoontee commented Jul 27, 2020

@madmiraal

It's also easy enough to convert a (half) axis event into a button event.

What about is_action_pressed and is_action_just_pressed?

@madmiraal
Copy link
Contributor Author

Use get_action_strength() and a variable to keep track of whether or not it was considered pressed or released on the previous iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update gamepad remapping system to reflect changes in SDL code.
6 participants