NagyD / SDLPoP

An open-source port of Prince of Persia, based on the disassembly of the DOS version.
GNU General Public License v3.0
1.1k stars 141 forks source link

Fix SDL_Joystick interfering with SDL_GameController interface #145

Closed Falcury closed 6 years ago

Falcury commented 7 years ago

This should fix issue #144. SDL_JOYBUTTONDOWN and SDL_JOYAXISMOTION events were handled, even though this is not necessary for controllers that work with the SDL_GameController interface. This caused strange behavior for those controllers (at least for the Xbox 360 controller, which I used to test this).

Falcury commented 7 years ago

I added an extra commit https://github.com/NagyD/SDLPoP/pull/145/commits/6afe10debf9375787265cbc922d57e1bdb2833b1 where I restructured the code a bit, to make it more compact. I collapsed the switch statement to avoid duplicating code that would get called for all three joystick events:

case SDL_JOYBUTTONDOWN:
case SDL_JOYBUTTONUP:
case SDL_JOYAXISMOTION:
    // Only handle the event if the joystick is incompatible with the SDL_GameController interface.
    // (Otherwise it will interfere with the normal action of the SDL_GameController API.)
    if (!using_sdl_joystick_interface) {
        break;
    }
#ifdef USE_AUTO_INPUT_MODE
    // TODO: Disregard SDL_JOYAXISMOTION events within joystick 'dead zone'
    if (!is_joyst_mode) {
        is_joyst_mode = 1;
        is_keyboard_mode = 0;
    }
#endif

I also added some TODO comments, because the code doesn't handle the 'dead zone' of the joystick at the moment.

Right now the code checks whether the joystick axis value is below or above zero, and the corresponding joy_hat_states value is set accordingly. (Note that the axis value can range anywhere from INT16_MIN to INT16_MAX.)

Maybe instead of mapping the joystick axes into the joy_hat_states variable, it would be better to copy the axis value into the joy_axis variable? Then, in read_joyst_control(), get_joystick_state() would get called, where the actual movement direction would be decided. (That would also take care of the 'dead zone')

(Like this:)

if (event.jaxis.axis == SDL_JOYSTICK_X_AXIS) {
    joy_axis[SDL_CONTROLLER_AXIS_LEFTX] = event.jaxis.value;
}
else if (event.jaxis.axis == SDL_JOYSTICK_Y_AXIS) {
    joy_axis[SDL_CONTROLLER_AXIS_LEFTY] = event.jaxis.value;
}

Note that I only have an Xbox 360 controller to test this with, so I can't verify that that would actually work better with classic joystick controllers. @vanfanel, maybe you can chime in on this?

NagyD commented 6 years ago

So, can I merge this, or should we wait for @vanfanel?

Falcury commented 6 years ago

The only thing I'm not sure about is the appropriate dead zone threshold for classic joystick controllers. Maybe a lower value might be appropriate. I could just make the threshold configurable... I'll add a commit for that.

Falcury commented 6 years ago

I could just make the threshold configurable... I'll add a commit for that.

Done. Also finished the TODO I left in the code earlier, about dead zone handling for the SDL_Joystick API. And collapsed the iniprocess... procedures in options.c because I wanted to avoid copying and pasting it in order to add an extra data type (int).

Note that I still cannot test whether this works correctly with older joystick controllers, because I only tested it by forcing the use of the SDL_Joystick API with the Xbox 360 controller.

But I would still say, merge this, because the game is really unplayable with at least the Xbox 360 controller at the moment.

NagyD commented 6 years ago

Okay, I merged it.

And collapsed the iniprocess... procedures in options.c because I wanted to avoid copying and pasting it in order to add an extra data type (int).

The new code is almost like a C++ template. :-)

By the way, do we really need to use four different integer data types?

Falcury commented 6 years ago

By the way, do we really need to use four different integer data types?

Probably not. I think I made the joystick threshold an int, mainly because I wanted to prevent unexpected behavior if people accidentally entered a number beyond the expected range. But I suppose that same argument could hold for many of the other variables. What do you think?

I wonder if the compiler actually inlines those functions, by the way. I haven't checked that.