Old Keyboard State in Input Handler

Started by
15 comments, last by iceman76 9 years, 9 months ago

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;
				}
Advertisement

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);

 

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. smile.png 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


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];


@other comments:

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.

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.

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).


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.

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.

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

 

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

[source]

#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;

}

[/source]

 

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.

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.

 

This topic is closed to new replies.

Advertisement