• Create Account

## Old Keyboard State in Input Handler

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

16 replies to this topic

### #1iceman76  Members

Posted 03 July 2014 - 04:34 PM

I've been working on an input handler, using SDL2 and C++.  Right now I'm testing it with a Pong clone, but I want to be able to use it my next games.  Right now it detects if a key is currently down or not, but I start running into issues when I try to see if a key was pressed or released that frame.

I'm using two arrays, one for the current state and another for the state the previous frame.  Every update, I use memcpy to copy the new state to the old state, and then use SDL_PumpEvents to update the current array.

The issue is that this actually isn't keeping an array of the old keyboard state.  Instead, the old and new arrays are always the same, so the keys are never able to be tagged as "pressed" or "released". (I did some debugging and can confirm that keyboardOld is changing)  I'm sure this is an issue with memcpy and constant pointers and all of that fun stuff, I just don't have enough experience with them to figure out what's going wrong.

So, why are keyboardOld and keyboardNew always the same, even though I copy New to Old before updating New, and how can I fix it?  Thanks in advance!

Here's the code:

Input.h

#ifndef Input_h
#define Input_h

#include <SDL.h>
#include <cstring>
#include <stdio.h>

class Input{
public:
enum keys{
A = SDL_SCANCODE_A,
B = SDL_SCANCODE_B,
C = SDL_SCANCODE_C,
D = SDL_SCANCODE_D,
E = SDL_SCANCODE_E,
F = SDL_SCANCODE_F,
G = SDL_SCANCODE_G,
H = SDL_SCANCODE_H,
I = SDL_SCANCODE_I,
J = SDL_SCANCODE_J,
K = SDL_SCANCODE_K,
L = SDL_SCANCODE_L,
M = SDL_SCANCODE_M,
N = SDL_SCANCODE_N,
O = SDL_SCANCODE_O,
P = SDL_SCANCODE_P,
Q = SDL_SCANCODE_Q,
R = SDL_SCANCODE_R,
S = SDL_SCANCODE_S,
T = SDL_SCANCODE_T,
U = SDL_SCANCODE_U,
V = SDL_SCANCODE_V,
W = SDL_SCANCODE_W,
X = SDL_SCANCODE_X,
Y = SDL_SCANCODE_Y,
Z = SDL_SCANCODE_Z,
UP = SDL_SCANCODE_UP,
DOWN = SDL_SCANCODE_DOWN,
LEFT = SDL_SCANCODE_LEFT,
RIGHT = SDL_SCANCODE_RIGHT,
};

void init();
void update();
void close();

//True if a key is pressed
bool keyDown(int key);
//True if the key was pressed this frame
bool keyPressed(int key);
//True if the key was released this frame
bool keyReleased(int key);

private:
//Length of keyboard arrays (given by SDL_getKeyboardState)
int length;
//State of keyboard last frame
Uint8* keyboardOld;
//State of keyboard this frame
const Uint8* keyboardNew;
};

#endif


Input.cpp

#include "Input.h"

void Input::init(){
keyboardNew = SDL_GetKeyboardState(&length);

keyboardOld = new Uint8[length];
}

void Input::update(){
std::memcpy(keyboardOld, keyboardNew, length);

SDL_PumpEvents();
}

void Input::close(){
delete keyboardOld;
keyboardOld = nullptr;
}

//Returns true if the key is currently pressed
bool Input::keyDown(int key){
return keyboardNew[key];
}

//Returns true if the key was pressed this frame
bool Input::keyPressed(int key){
return (keyboardNew[key] && !keyboardOld[key]);
}

//Returns true if the key was released this frame
bool Input::keyReleased(int key){
return (!keyboardNew[key] && keyboardOld[key]);
}


And the relevant code from the main program:

				//Handle player input
input.update();

if(input.keyPressed(input.UP)){
player->y -=50;
}
if(input.keyReleased(input.UP)){
player->y -=50;
}
if(input.keyDown(input.DOWN)){
player->y +=1;
}
if(input.keyDown(input.LEFT)){
player->x -=1;
}
if(input.keyDown(input.RIGHT)){
player->x +=1;
}


### #2fredericvanmol  Members

Posted 04 July 2014 - 07:37 AM

I've never used SDL_GetKeyboardState(), but have you verified that keyboardNew changes?

Maybe you have to call this every time after SDL_PumpEvents()?

keyboardNew = SDL_GetKeyboardState(&length);

### #3Eck  Members

Posted 04 July 2014 - 08:57 AM

In your update function, I see where you're copying the new keyboard state to the old keyboard state, but you aren't updating the new keyboard state. I don't see how the "keydown" is working without this. Unless you're always newing up an instance of Input somewhere else in your program...

void Input::update(){
std::memcpy(keyboardOld, keyboardNew, length);
keyboardNew = SDL_GetKeyboardState(&length); // Add this line.
SDL_PumpEvents();
}


Give that a try and see what happens,

- Eck

EckTech Games - Games and Unity Assets I'm working on
Still Flying - My GameDev journal
The Shilwulf Dynasty - Campaign notes for my Rogue Trader RPG

### #4Buckeye  GDNet+

Posted 04 July 2014 - 09:12 AM

POPULAR

Right now it detects if a key is currently down or not, but I start running into issues when I try to see if a key was pressed or released that frame.

Just to confirm, you ARE seeing changes in the keyboardNew buffer, and the only problem you're having is copying that buffer?

If so, for debugging purposes, have you tried copying the values one-by-one? I.e.,

for(int i=0; i < length; i++) keyboardOld[i] = keyboardNew[i];


Supposedly, according to googled docs, a single call to SDL_GetKeyBoardState(...) returns a const pointer to an internal (SDL) buffer, which lasts the lifetime of the app. SDL_PumpEvents() fills that internal buffer with changes pending since the last Pump call. So.. it appears the posted code should work.

In addition, just FYI, the docs for SDL_PumpEvents states:

You can only call this function in the thread that set the video mode.

Edited by Buckeye, 04 July 2014 - 09:13 AM.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.

### #5iceman76  Members

Posted 04 July 2014 - 02:28 PM

Just to confirm, you ARE seeing changes in the keyboardNew buffer, and the only problem you're having is copying that buffer?

Yes, the keyboardNew buffer is definitely being updated correctly.  Like you said, SDL_PumpEvents updates the buffer that it's pointing at.

I just tried copying the values with the for loop, and the result is the same.

In addition, just FYI, the docs for SDL_PumpEvents states:

You can only call this function in the thread that set the video mode.

I haven't set up anything with multi-threading myself, so I don't think this should be an issue (unless SDL automatically does threading).

### #6Buckeye  GDNet+

Posted 04 July 2014 - 06:46 PM

I just tried copying the values with the for loop, and the result is the same.

For further info (or perhaps a solution,) try copying the new buffer to the "old" in your Init function.

Then change Update() to just:

SDL_PumpEvents(NULL);

Make your keyboard comparisons, THEN copy the new buffer to the old.

I.e., the process would be something like:

keyboardNew = GetKeyboardStates(&length);
keyboardOld = new Uint8[length];
// if the following shows correct results, try std::memcpy
for(int i=0; i < length; ...) { copy keyboardNew to keyboardOld } // make the initial copy
loop:
SDL_PumpEvents(NULL);
// check for changes
isThisKeyPressed();
...
isThatKeyPressed();
// done using keyboardNew for this time through the loop
{ copy keyboardNew to keyboardOld }


Also just confirming: input.keyPressed() is used to detect ONLY the initial downstate of a key (single event), as it will be false the next time through the loop, no matter what happens.

Edited by Buckeye, 04 July 2014 - 06:50 PM.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.

### #7fredericvanmol  Members

Posted 05 July 2014 - 01:31 AM

I've tried it here and it works fine so far. Could we see more of the code before input.update() is called?

### #8fredericvanmol  Members

Posted 05 July 2014 - 02:05 AM

I've also tried it with your Input class and it works as well. Here's the complete program.

There's one strange thing so far: if I hold both the g and b keys and then press h, the h isn't registered. I have no idea why that is at this point.

EDIT: sorry for the unformatted code, this should be better.

EDIT2: use delete[] instead of delete to get rid of the keyboardOld array

#include <iostream>
#include <SDL.h>

const int SCREEN_WIDTH    = 800;
const int SCREEN_HEIGHT    = 600;

class Input
{
public:
enum keys
{
A = SDL_SCANCODE_A,
B = SDL_SCANCODE_B,
C = SDL_SCANCODE_C,
D = SDL_SCANCODE_D,
E = SDL_SCANCODE_E,
F = SDL_SCANCODE_F,
G = SDL_SCANCODE_G,
H = SDL_SCANCODE_H
};

void init();
void update();
void close();

//True if a key is pressed
bool keyDown(int key);
//True if the key was pressed this frame
bool keyPressed(int key);
//True if the key was released this frame
bool keyReleased(int key);
private:
//Length of keyboard arrays (given by SDL_getKeyboardState)
int length;
//State of keyboard last frame
Uint8* keyboardOld;
//State of keyboard this frame
const Uint8* keyboardNew;
};

void Input::init()
{
keyboardNew = SDL_GetKeyboardState(&length);
keyboardOld = new Uint8[length];
}

void Input::update()
{
std::memcpy(keyboardOld, keyboardNew, length);
SDL_PumpEvents();
}

void Input::close()
{
delete[] keyboardOld;
keyboardOld = nullptr;
}

bool Input::keyDown(int key)
{
return keyboardNew[key];
}

bool Input::keyPressed(int key)
{
return (keyboardNew[key] && !keyboardOld[key]);
}

bool Input::keyReleased(int key)
{
return (!keyboardNew[key] && keyboardOld[key]);
}

int main(int argc, char *argv[])
{
SDL_Window* window = nullptr;
SDL_Surface* screenSurface = nullptr;
Uint8 r, g, b;
bool hidden = false;
Input input;

// initialize SDL
if (SDL_Init( SDL_INIT_VIDEO ) < 0)
return 0;
// create window
window = SDL_CreateWindow("SDLTest", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOW);
if (!window)
{
SDL_Quit();
return 0;
}
// initialize our input object
input.init();

// event loop
while (true)
{
// fetch keyboard state
input.update();

// process keyboard state
// -> quit?
if (input.keyPressed(Input::C) || input.keyPressed(SDL_SCANCODE_ESCAPE))
break;
// -> visibility
if (input.keyPressed(Input::H))
hidden = true;
else if (input.keyReleased(Input::H))
hidden = false;
// -> color
if (!hidden)                    r = 0xFF;
else                        r = 0;
if (input.keyDown(Input::G) && !hidden)    g = 0xFF;
else                        g = 0;
if (input.keyDown(Input::B) && !hidden)    b = 0xFF;
else                        b = 0;
// draw a color on screen
screenSurface = SDL_GetWindowSurface(window);
SDL_FillRect(screenSurface, nullptr, SDL_MapRGB(screenSurface->format, r, g, b));
SDL_UpdateWindowSurface(window);
}

// clean up before exit
input.close();
SDL_DestroyWindow(window);
SDL_Quit();

return 0;
}


### #9haegarr  Members

Posted 05 July 2014 - 02:07 AM

Any call to SDL_PumpEvents causes an update of the internal key array where keyboardNew is referring to, be it directly or indirectly by SDL_PollEvent or SDL_WaitEvent. Without knowing whether or not you call it elsewhere … if you do then the array keyboardNew may be updated more frequently then you wish. That means that when you invoke memcpy, in keyboardNew is already a state you never seen before, and especially you have not saved in keyboardOld. So your code is able to detect only those few changes between the most recent accidentally made SDL_PumpEvents and your intended SDL_PumpEvent. As said, we have not enough information to confirm that, but yellowsputnik's test seems to confirm at least that the problem occurs only in a wider context.

To avoid such a kind of problem, you should have two buffers, keyboardOld and also keyboardNew, and fill both by memcpy from keyboardSDL (which was formerly named keyboardNew). Do it like in the following scheme:

Input::update() {
std::memcpy( keyboardOld, keyboardNew, length );
SDL_PumpEvents();
std::memcpy( keyboardNew, keyboardSDL, length );
}


Of course, to avoid the 2nd memcpy, you can do pointer swapping if you wish.

Buckeye's last post goes in a similar direction, but with the solution above you have more control of the state of the buffers.

Edited by haegarr, 05 July 2014 - 02:42 AM.

### #10fredericvanmol  Members

Posted 05 July 2014 - 02:40 AM

Input.cpp
void Input::close(){
delete keyboardOld;
keyboardOld = nullptr;
}


One remark about this: you should use delete[] to release the keyboardOld array, since you also assigned it with the new[] operator.

### #11SmkViper  Members

Posted 05 July 2014 - 08:34 AM

There's one strange thing so far: if I hold both the g and b keys and then press h, the h isn't registered. I have no idea why that is at this point.

This is a common problem with non-mechanical and/or cheap keyboards. Due to the way they're constructed, they can only register a certain number of key presses at once, and pressing certain keys prevents other keys from being registered. The only solution (short of getting a better keyboard) is to allow users to change their keyboard bindings to avoid these issues.

### #12fredericvanmol  Members

Posted 05 July 2014 - 10:31 AM

There's one strange thing so far: if I hold both the g and b keys and then press h, the h isn't registered. I have no idea why that is at this point.

This is a common problem with non-mechanical and/or cheap keyboards. Due to the way they're constructed, they can only register a certain number of key presses at once, and pressing certain keys prevents other keys from being registered. The only solution (short of getting a better keyboard) is to allow users to change their keyboard bindings to avoid these issues.

Yes, I thought it might be something like that. It seems strange that it happens for two very specific combinations (of the ones I've tried), but I guess that may be due to the way the keyboard is wired.

### #13Lactose  GDNet+

Posted 05 July 2014 - 10:42 AM

but I guess that may be due to the way the keyboard is wired.

http://www.microsoft.com/appliedsciences/antighostingexplained.mspx

Hello to all my stalkers.

### #14iceman76  Members

Posted 05 July 2014 - 03:07 PM

Haegarr, you're right- the problem was that I was accidentally calling SDL_PumpEvents.  Right before I check for the input in main program, there's this little snippet to exit when the user presses X:

//Handle events on the queue
while( SDL_PollEvent( &e ) != 0 ){
//User requests quit
if( e.type == SDL_QUIT ){
quit = true;
}
}


I read online several times that SDL_PollEvent calls SDL_PumpEvents internally, but it completely slipped my mind!  Serves me right for purposefully ignoring this from the FAQ:

When posting code (which you should do if the problem is at all code-related), post it verbatim without transcribing or omitted code you believe is relevant as you can easily hide the problem that way.

Anyways, commenting out the above lines worked, so I can detect keyPressed and keyReleased to my heart's content!  Of course, that means that you can't quit my program without resorting to the Windows Task Manager, but that's a feature, right? =P  I've also changed the delete to delete[]

Thanks to everybody for helping me figure this out!

### #15Buckeye  GDNet+

Posted 06 July 2014 - 09:49 AM

you can't quit my program without resorting to the Windows Task Manager, but that's a feature, right? =P

One man's feature is another man's bug. That, IMHO, is a bad "feature." Maybe just me, but I would remove your app (or not download it to be begin with) the moment I discovered that was the only method to terminate it.

Also just my humble opinion, but if an app (particularly a game) isn't dedicated to user convenience as it's first priority, it's not ready for distribution.

Edited by Buckeye, 06 July 2014 - 09:51 AM.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.

### #16haegarr  Members

Posted 06 July 2014 - 09:58 AM

One man's feature is another man's bug. That, IMHO, is a bad "feature." Maybe just me, but I would remove your app (or not download it to be begin with) the moment I discovered that was the only method to terminate it.

Not only that, but letting the "feature" inside is just a time bomb. After a while the developer things of new features based on input … and suddenly keyboard input is again no longer running. Such things should be wiped out at the roots.

### #17iceman76  Members

Posted 06 July 2014 - 01:29 PM

Oh, don't worry, I am most certainly going to fix that.  I was just joking about the same thing Buckeye said, actually- calling it a feature when it is so obviously a bug.  Sorry that I wasn't clear.  Also, this isn't something I'm planning on distributing, this is just me learning how to use SDL2 in the first place.

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.