• Advertisement
Sign in to follow this  

Old Keyboard State in Input Handler

This topic is 1385 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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

Share this post


Link to post
Share on other sites
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);

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

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]

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites

 

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.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement