Secretchronicles / TSC

An open source two-dimensional platform game.
https://secretchronicles.org/
GNU General Public License v3.0
204 stars 50 forks source link

Do not simulate keyboard events from joystick handling code #499

Open Quintus opened 8 years ago

Quintus commented 8 years ago

The joystick handling code in joystick.cpp currently often transforms joystick events into keyboard events and the submits them into the keyboard event handler in keyboard.cpp. This break’s the keyboard handler’s assumption of being called only during regular event handling, especially this is problematic because the joystick code does not take care that there is a corresponding key release event for a key press event it generated. Therefore, this design is flawed.

Instead, the handler code has to be extracted in such a way that the separate event handlers can call it directly, rather than having to simulate events. I.e. this:

Joystick handler -> Keyboard handler -> stuff
Keyboard handler -> stuff

has to be changed into this:

Joystick handler -> Do_Stuff() -> stuff
Keyboard handler -> Do_Stuff() -> stuff

I came over this while working on #491.

Valete, Quintus

datahead8888 commented 8 years ago

I get the impression the old SMC code was forcing SDL keyboard events to be generated (not quite sure where offhand). The current TSC code has lots of checks that directly check the SFML keyboard state. It all seems to assume that joystick events are translated into keyboard events in SDL/SFML.

This is forcing a workaround to be implemented in #532 -- I get the impression that SFML does not generate joystick events as frequently as SDL. The only way to be 100% sure of the difference between API's would be to debug the SDL version of TSC for a while.

In the new architecture, we need to spend some time talking about how input is abstracted. Ideally any source of input (keyboard, joystick, unknown physical source, or even a logical source (maybe AI)) could be plugged into it.

In order for this ticket to be worked in the old code, however, we probably want to eliminate direct checking of the SFML keyboard state through most of the game code -- it would need to check in a centralized class for the state.

Quintus commented 8 years ago

The current TSC code has lots of checks that directly check the SFML keyboard state. It all seems to assume that joystick events are translated into keyboard events in SDL/SFML.

Yes. The old SDL-based TSC code used lots of these direct checks, and I ported them one by one to use the SFML API instead. And yes, joystick handling is implemented by simulating keyboard events. I would say that SMC (sic) had originally only keyboard support and then later joystick support was added the lazy way: instead of rewriting the input handler, the solution causing the least work was chosen, no matter how hacky it was.

In the new architecture, we need to spend some time talking about how input is abstracted.

Yes, but please take this either to #534 or to the mailinglist.

it would need to check in a centralized class for the state.

There are a number of centralised classes, cKeyboard, cJoystick and cMouse. It's just that the game code doesn't always bother to use them. And as I said earlier, I don't think it is worth to make grave architectural changes to TSC anymore. Rather have the thinking effort go into the new architecture directly.

Vale, Quintus

Quintus commented 8 years ago

@datahead8888 Here is the code that generates SFML events: https://github.com/Secretchronicles/TSC/blob/0d2023cfdfee3879d66bb1f9926e8d85c7c872bc/tsc/src/input/joystick.cpp#L151

It instanciatse the sf::Event class and then fills it before submitting the event to the keyboard handler.

Vale, Quintus

datahead8888 commented 8 years ago

A lot of the game actions such as ducking and level exit activation take place in response to key down or key up events. This, as far as I know, assumes that keyboard or joystick events are continually streaming into the game. In SFML this is not the case for the joystick, which shows that this architecture is a bit of a mess. Thus the level player class should ideally be modified to eliminate this assumption, if we address these joystick concerns in the current code.