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

Fix #USE_SDL2_GAMECONTROLLER code path #782

Merged
merged 1 commit into from
Nov 1, 2018
Merged

Fix #USE_SDL2_GAMECONTROLLER code path #782

merged 1 commit into from
Nov 1, 2018

Conversation

rngbus
Copy link
Contributor

@rngbus rngbus commented Jul 16, 2018

Fix #USE_SDL2_GAMECONTROLLER code path

The code path was not actually using the SDL2 GameController API.

Added override for CreateGamePadDriver to Sdl2Factory so that the code path is actually used.
Fixed some small typos.
Note these changes are only in effect if #USE_SDL2_GAMECONTROLLER is defined.

Purpose of this PR

The #USE_SDL2_GAMECONTROLLER code path was broken. Due to a missing override of CreateGamePadDriver() in Sdl2Factory, it was using the mapped game controller driver which falls back to the SDL2 joystick api instead of the game controller api. Since the SDL2 GameController api supports steam controller configuration and also maps many more gamepads than OpenTK, this is a very useful #define to have working.

This affects only the input portion of OpenTK, and only if #USE_SDL2_GAMECONTROLLER is defined, which by default it is not.

Testing status

I have tested this with a variety of gamepads including XBox One controllers on two windows 10 machines using SDL 2.0.8

Fix #USE_SDL2_GAMECONTROLLER code path

The code path was not actually using the SDL2 GameController API.

Added override for CreateGamePadDriver to Sdl2Factory so that the code path is actually used.
Fixed some small typos.
Note these changes are only in effect if #USE_SDL2_GAMECONTROLLER is defined.
return string.Empty;
}

public bool SetVibration(int index, float left, float right)
Copy link
Contributor

@VPeruS VPeruS Jul 16, 2018

Choose a reason for hiding this comment

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

You don't need to add this, I guess. You can just move it to common part

@Nihlus
Copy link
Contributor

Nihlus commented Jul 21, 2018

Nicely done! If this replaces the current implementation and works better, I think we can drop the old one and remove the conditional compilation, making this the default.

@rngbus
Copy link
Contributor Author

rngbus commented Jul 22, 2018

Thanks! You could define the conditional by default, but I would keep the option to fall back to OpenTK joystick handling in-case somebody wants to do their own custom mapping. I'll shortly be checking in a fix that updates the opentk joystick mappings to the latest sdl database, which will make it better (but it still won't work with steam controller configuration this way afaik).

@Nihlus
Copy link
Contributor

Nihlus commented Jul 22, 2018

I'd prefer to switch them around, so that this would be the default without having to define any extra compile-time symbols. We're trying to move away from compile-time conditionals as much as possible :)

@leezer3
Copy link
Contributor

leezer3 commented Jul 24, 2018

This is more of a wondering that a suggestion, but still:
Could we separate the SDL2 gamepad and joystick interfaces into separate backends?

This would mean we could then select via ToolkitOptions which we want.
IRIC we can choose between SDL and native that way at the minute, so the base framework is definitely there for that.

@Nihlus
Copy link
Contributor

Nihlus commented Jul 24, 2018

That API is probably going to change significantly in 4.0, so staying within the old code's constraints and design until we get to it might be wise.

@Perksey
Copy link
Contributor

Perksey commented Nov 1, 2018

@rngbus What’s the status of this?

@varon varon merged commit 85fea59 into opentk:4.0 Nov 1, 2018
@varon
Copy link
Member

varon commented Nov 1, 2018

Thanks @rngbus. We look forward to more contributions in the future!

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.

None yet

6 participants