Refactor Input?

[mspraggs]

Could we reduce the amount of code in this class in some way? Although I think I understand what the usefuls_ variable is trying to do (something like a finite state machine, right?) I can't help but wonder if there's a cleaner way to do this. My question originally came out of wanting to write this in Unit:

void handleInput()
{
    Input& input = Input::instance();
    switch (input.id()) {
    case SDLK_UP:
        ...
        break;
}

and similar. Perhaps this will result in shifting all the same code into Unit, in which case ignore me.

In any case, it might be nice to tidy up Input a bit.

[Fyll]

Yeah, Input could possibly do with a bit of a tidy. As you say, that would have to shift all of the input into Unit, and part of the idea of this was that Unit wouldn't need to care about what the inputs were (as in SDL_UP vs SDL_w), and wouldn't need to care where they came from (as in, we could switch to X instead of SDL, and Unit wouldn't care).

I might have a shot of compactifying a few things, but I'm not too sure what I could do. Is it mainly the memoriseUsefuls (or whatever it's called) function that's your main concern?

[mspraggs]

I've had a think, and I have an idea. Here's a summary:

class Input
{
private:
    void checkKeys()
    {
         for(auto& event : events_) {
             int sym = event.event.key.keysym.sym;
             if (std::find(validKeys_.begin(), validKeys_.end(), sym) != validKeys_.end()) {
                 keyStates_[sym] = event.event.type == SDL_KEYDOWN or event.event.type == SDL_MOUSEBUTTONDOWN;
            }
        }
    }

    std::map<int, bool> keyStates_;
    std::vector<int> validKeys_; // Contains SDL keys that we use.
}

Then bool up(); etc. would become return keyStates_[SDLK_UP] or keyStates_[SDLK_w]; and so on. Something else might be needed to make this work with the other AI code, but this seems like a reasonably clean solution.

[Fyll]

The reason for the fiddly bits (the 'usefuls_' array) is because the player needs to be able to hold a key down. When you hold a key down, SDL sends a signal saying it's been pressed, then gives you roughly a second to release the button before it realises you're holding it. This is very unfortunate, as it means if you hold left to run (for example), you take one step, wait for about a second, and then start running.

Technically, it'd make the code more compact if the AI's had their own usefuls_ arrays, but that'd leave a lot of empty data hanging around, and would probably be a bit confusing.

[mspraggs]

Wouldn't the std::map above work in the same way? It's basically an array, but the indexes now become the SDL key values.

[Fyll]

Ah, sorry. I'm not really sure how std::map works. Can it handle a key_released case?

[mspraggs]

It is basically used to store objects in an array, but they're indexed with an arbitrary type. E.g. you could have a map between a char and an integer, or char and a function pointer, or some such.

The idea would be you just take the SDL key integer value and use that as the input for the map, then store a true/false value in the map depending on whether a key's been pressed. In the above I loop through the events, and for any that are in the vector of valid keys (i.e. ones we're interested in), we store the logical or of whether the event is a keydown or a mousedown event. This way true will be stored when you have either keydown or mousedown, otherwise false.

This has the added advantage of storing the state of each key separately. Currently you can start running left, then tap the "a" key quickly, which will switch the relevant value in usefuls_ to false, even though you are still holding the left key. The code above I don't think has this problem.

[Fyll]

I think I see...

So if something has happened with a key that we're interested in, it stores if the thing that happened was you pressing it down. Seems smart.

Huh, I hadn't noticed that issue. Fair enough. Should I implement this, or do you want to?

[mspraggs]

Well it'll store something regardless of whether it's key down or key up, just it'll store a true value if it's a key/mousedown event, and false otherwise.

I'll have a stab. If I run into trouble with the AI code I'll let you know.

[mspraggs]

Ok, this is mostly working now. The only problem I'm having is with weapon switching. I'm not sure how you handled this before (I'll check the code in a second), but the game is now super sensitive to the space bar, such that even tapping space quickly often cycles through several weapons. I'll push what I've done now and try to locate this bug. If you have any thoughts as to why this happens let me know.

[mspraggs]

Also I apologise in advance for the two multi-line if statements. I may create validateButton and validateKey inline functions to clean this up further.

[Fyll]

It's probably because space now uses the same key-held-down code that the other buttons do. Previously, it was handled separately so you didn't get constant holding-down messages when you press the button.

You've seen my code. Do you really think I'm going to complain about multi-line if statements? :P

[mspraggs]

Hmm. If I comment out the space key code in validKeys_ in Input's constructor, it's not so bad, but now I get weapon switching for keyup events as well :-/. Strange thing is I don't see how the new implementation of checkForKey is so different to the previous one that this would occur.

[Fyll]

Space didn't used to use checkForKey(), it manually queried event(). (As did backspace, but that doesn't really matter).

One thing I have been slightly tempted to do is to define my own enum of inputs, so that we don't keep needing separate key and mouse things. Yeah, a function would need to convert the SDL_Key and SDL_Mouse data to this type, but it would mean we could combine the mouse inputs and the key inputs into one thing, which'd simplify the checking process a bit. (EDIT: Also, then we could get rid of the 0x300 and 0x400. I know they work, but using numbers instead of enums in code like that just seems... Painful)

[mspraggs]

Are you sure about space? According to the history it used to call checkForKey: https://github.com/mspraggs/potentia/blob/03ca3847af3b02c6976f4e55d9ac83dd3c4e4e81/lib/Input.hpp

I'm inclined to agree. Perhaps another map defined in Constants.hpp could remove the need for a function.

[Fyll]

Ah, my mistake. It did used to use checkForKey(), however, checkForKey() didn't have the id == 0 exception, so checkForKey() rooted through the events to check if space had been pressed since the last scan, whereas now it gets caught by the id == 0 exception and tests if it has been pressed and not released.

[mspraggs]

Hmmm... I think checkForKey should work the same as before if space and backspace aren't in validKeys. The if statement needs validateKey to return true (meaning the key is in validKeys), otherwise it just proceeds to run the previous implementation.

I'll whip out gdb and see if I can get to the bottom of this.

[Fyll]

Okay, I've pushed a version using my own enum. Does it all look good now?

[mspraggs]

Looks great! I've tidied up a few of the if statements, but these aren't big changes.

Ah, I wondered how to initialise a std::map statically... now I know! One thing to watch is mixing buttons and keys. I think I encountered a problem here since one of mouse buttons has the same integer value as one of the keys. Can't remember which one... Should we put the enum in Constants.hpp along with the other enums? Or perhaps here is best, given here is the only place it's used.