5
\$\begingroup\$

I recently started developing a simple 2D game in Java using JavaFX and no proper game library (libgdx etc.) and among the things I did was to make an InputManager to handle every input from the player.

In the InputManager I'm using a Hash Map, called inputMap, using a String as the key and a Boolean as the value. When the player presses a movement key, e.g. the UP arrow key, InputManager sets the value of the "UP" key of the map to "true". The game loop which also has a Hash Map, copies the map from the InputManager (via a getInputMap() method) to it's own map every frame and then using if statements the player moves.

My questions is, is this an acceptable way of doing this? The movement and every action in general.

Is there a better way of doing it without the use of Hash Maps or if Hash Maps are the way to go is there a better, more appropriate, way to implement them?

Below is the InputManager class:

class InputManager {

    private Scene scene;
    private Map<String, Boolean> inputMap = new HashMap<String, Boolean>();

    InputManager(Scene scene) {
        this.scene = scene;
        populateInputMap();
        getInput();
    }

    private void populateInputMap() {
        inputMap.put("UP", false);
        inputMap.put("DOWN", false);
        inputMap.put("LEFT", false);
        inputMap.put("RIGHT", false);
        inputMap.put("RESTART", false);
    }

    private void getInput() {

        scene.setOnKeyPressed(new EventHandler<KeyEvent>() {
            @Override
            public void handle(KeyEvent event) {

                if (event.getCode() == KeyCode.UP) {
                    inputMap.put("UP", true);
                }
                if (event.getCode() == KeyCode.DOWN) {
                    inputMap.put("DOWN", true);
                }
                if (event.getCode() == KeyCode.LEFT) {
                    inputMap.put("LEFT", true);
                }
                if (event.getCode() == KeyCode.RIGHT) {
                    inputMap.put("RIGHT", true);
                }
                if (event.getCode() == KeyCode.R) {
                    System.out.println("RESTART");
                }
            }
        });

        scene.setOnKeyReleased(new EventHandler<KeyEvent>() {
            @Override
            public void handle(KeyEvent event) {
                if (event.getCode() == KeyCode.UP) {
                    inputMap.put("UP", false);
                }
                if (event.getCode() == KeyCode.DOWN) {
                    inputMap.put("DOWN", false);
                }
                if (event.getCode() == KeyCode.LEFT) {
                    inputMap.put("LEFT", false);
                }
                if (event.getCode() == KeyCode.RIGHT) {
                    inputMap.put("RIGHT", false);
                }
            }
        });
    }

    public Map getInputMap() {
        return inputMap;
    }
}

And this is the part of the Game Loop that the map gets copied:

private void updateGame() {
    frameCount++;
    inputMap.putAll(inputManager.getInputMap());

    if (inputMap.get("UP").booleanValue()) {
        y -= 5;
        player.setLayoutY(y);
    }

    if (inputMap.get("DOWN").booleanValue()) {
        y += 5;
        player.setLayoutY(y);
    }

    if (inputMap.get("LEFT").booleanValue()) {
        x -= 5;
        player.setLayoutX(x);
    }

    if (inputMap.get("RIGHT").booleanValue()) {
        x += 5;
        player.setLayoutX(x);
    }
    inputMap.clear();
}

I am aware that the movement part is not right.

\$\endgroup\$
  • 1
    \$\begingroup\$ If you don't know what all your possible inputs are, but you know they're booleans, use a Set, and its the methods add, addAll, remove, removeAll and contains. Also, your code calls for an enum. \$\endgroup\$ – Olivier Grégoire Jul 4 '17 at 7:37
  • \$\begingroup\$ Why would you copy it each frame? That is unnecessary work... \$\endgroup\$ – Milney Jul 4 '17 at 9:47
2
\$\begingroup\$

What do you really need to do here? You need to check for the presence of an action. While your current solution will get you there, it's fairly heavy for a presence/non-presence check.

First, using String constants typically isn't a good idea when designing a framework. It's easy and expandable, but more often than not you want to use an enum:

enum Command{
    UP, DOWN, LEFT, RIGHT;
}

This lets you easily add and remove commands down the line. If you needed to add a fire button, for instance, you could add FIRE to the enum, and you'd be set.

Second, while a Map to Boolean seems like the logical choice, if you're just checking two states, you can use a Set and use contains(Object) to reduce the overhead. Because you're now using an enum to store your states, you can use the blazingly fast EnumSet to do this:

private EnumSet<Command> inputKeys = EnumSet.noneOf(Command.class);

As a side note: if you're comparing a single value to a bunch of other values, switch/case is the way to go.

The simplified code:

import java.util.EnumSet;

import javafx.scene.Node;
import javafx.scene.Scene;

class InputManager{
    private EnumSet<Command> inputKeys = EnumSet.noneOf(Command.class);

    InputManager(Scene scene) {
        scene.setOnKeyPressed(event->{
            switch(event.getCode()){
                case UP:
                    inputKeys.add(Command.UP);
                    break;
                case DOWN:
                    inputKeys.add(Command.DOWN);
                    break;
                case LEFT:
                    inputKeys.add(Command.LEFT);
                    break;
                case RIGHT:
                    inputKeys.add(Command.RIGHT);
                    break;
                case R:
                    System.out.println("RESTART");
                    break;
            }
        });
        scene.setOnKeyReleased(event->{
            switch(event.getCode()){
                case UP:
                    inputKeys.remove(Command.UP);
                    break;
                case DOWN:
                    inputKeys.remove(Command.DOWN);
                    break;
                case LEFT:
                    inputKeys.remove(Command.LEFT);
                    break;
                case RIGHT:
                    inputKeys.remove(Command.RIGHT);
                    break;
            }
        });
    }

    public EnumSet<Command> getInputKeys(){
        return inputKeys;
    }

    enum Command{
        UP, DOWN, LEFT, RIGHT;
    }
}

class GameLogic{
    // Dummy values
    volatile int frameCount = 0, x = 0, y = 0;
    InputManager inputManager = null;
    Node player = null;

    private void updateGame() {
        frameCount++;

        EnumSet<InputManager.Command> keys = inputManager.getInputKeys();

        if(keys.contains(InputManager.Command.UP)){
            y -= 5;
            player.setLayoutY(y);
        }
        if(keys.contains(InputManager.Command.DOWN)){
            y += 5;
            player.setLayoutY(y);
        }
        if(keys.contains(InputManager.Command.LEFT)){
            x -= 5;
            player.setLayoutX(x);
        }
        if(keys.contains(InputManager.Command.RIGHT)){
            x += 5;
            player.setLayoutX(x);
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I like your answer. I'm implementing it now and I'll report back how it went. \$\endgroup\$ – Nick Jul 4 '17 at 17:12
  • 2
    \$\begingroup\$ It's working nicely. Now I'm no expert in optimization but i feel like this one, performance wise, is much better than copying the hash maps. I made one small change and gave Enums their own file just so i wont have to call them through the InputManager. \$\endgroup\$ – Nick Jul 4 '17 at 17:48
  • 1
    \$\begingroup\$ Glad to hear it's working well! Good luck with your game! \$\endgroup\$ – ndm13 Jul 4 '17 at 17:50
5
\$\begingroup\$

Hashmaps are okay. If you believe you are going to get a performance bottleneck on input, then you are doing something critically wrong, or you're pre-maturely optimizing.

Unlike the other answer, with hashmaps you don't have to worry about array bounds or things going wrong, you just put it in and get it out when you need it.

I use my own custom enumerations with hashsets/maps, and the footprint is so small that if I take it out (as in, drop polling input) it's not even noticeable. Spending time converting to arrays and then always making sure I don't do something dumb forever afterwards with array bounds is not a good tradeoff IMO.

While putting in an "UP"/"DOWN"/"LEFT"/"RIGHT" probably means you should be using an enumeration, you can leave it for right now and revisit it later if you want.

\$\endgroup\$
  • \$\begingroup\$ I wasn't concerned as much about a performance bottleneck on input as a general performance bottleneck because I am not aware of how taxing copying the hash maps will be. I like this approach as it feels "clear" to me and easily adjustable. I will be using enums, thought it as well but at that moment i was experimenting to see if the hash tables would work hence the strings. \$\endgroup\$ – Nick Jul 4 '17 at 1:33
3
\$\begingroup\$

Copying hash maps is horribly expensive, in the context of an operation that needs to be executed millions of times a second.

You're doing this up to 120 times a second. Performance-wise it's perfectly acceptable. That's not saying I'd do it the same way, but you have a decent approach that's going to work, so stick with it.

\$\endgroup\$
  • \$\begingroup\$ I will stick with it, but what other approaches exist? \$\endgroup\$ – Nick Jul 4 '17 at 1:59
  • \$\begingroup\$ @Nick That would be quite a long answer, which wouldn't be about your original question. A common approach is to use an interface instead of a hash map so the implementation is easier to change later, to e.g. a list, which is cheaper to copy if you keep the list from one frame to the next, and cheaper to look up if it contains less than 10 elements. A possible major change is to use a list of timestamped events instead of key-press state to properly handle double-presses at low framerates. \$\endgroup\$ – Peter Jul 4 '17 at 6:08
  • \$\begingroup\$ @Peter Given that there's a fixed amount of keys being tracked, a simple array will always be the fastest option if you keep any list of all states (rather than e.g. only storing the keys actually pressed). Just directly index into the array using the ID of the key (e.g. "left" == 1, "right" == 2, easy with enums), no need for searching or hash codes. Having the implementation hidden behind an interface makes it very easy to optimise in the way you need, including said change from "all states" to "only true states", as well as your event model (which would also extend the interface). \$\endgroup\$ – Luaan Jul 4 '17 at 9:04
  • 1
    \$\begingroup\$ @Luaan Careful when using "always" and "fastest" in the same sentence. But I'll resist the temptation to derail this :) \$\endgroup\$ – Peter Jul 4 '17 at 10:08
  • \$\begingroup\$ @Peter The closer I get to the comment length limit, the more likely I'm to change "under most reasonable conditions" to "always" :P In this case, it's close enough to "always" - you'd have to really go out of your way to make an array slower. Even bit-wise manipulation can easily be slower than such a small array (though I concede that I'm being modern-x86-centric - the situation might be very different on e.g. an ARM CPU). \$\endgroup\$ – Luaan Jul 4 '17 at 10:34
3
\$\begingroup\$

It seems like you will know what types of inputs there will be ahead of time, so using a HashMap seems over-complicated to me. You want to pass around a bunch of booleans, so why not just pass around a bunch of booleans like this?

class Input {
    public boolean up = false;
    public boolean down = false;
    public boolean left = false;
    public boolean right = false;
}

Instead of having to call two methods to get a boolean you can just check the value directly, like so:

private void updateGame() {
    frameCount++;
    Input input = inputManager.getInput();

    if (input.up) {
        y -= 5;
        player.setLayoutY(y);
    }

    if (input.down) {
        y += 5;
        player.setLayoutY(y);
    }

    if (input.left) {
        x -= 5;
        player.setLayoutX(x);
    }

    if (input.right) {
        x += 5;
        player.setLayoutX(x);
    }

}
\$\endgroup\$
0
\$\begingroup\$

Hashmap seems like overkill. I personally use an array for buffering (if you even need it), a flag-style enum for my inputs, and a currentInput variable that gets set with these flags.

int UP = 1;
int DOWN = 2;
int RIGHT = 4;
int LEFT = 8;

// The current input as the user entered
int currentInput = 0;

void handle(KeyEvent event)
{
    if (event.GetKeyCode(Keys.Up))
    {
        currentInput |= UP;
    }
    // etc...
}

Later in your updateGame() method:

void updateGame()
{
    if ((currentInput&UP) == UP)
    {
        // set velocity, pos, etc.
        // Reset currentInput
        currentInput ˆ= UP;
    }
    // ...
}

EDIT: Excuse me for my slow edits, I'm on my tablet.

\$\endgroup\$
  • \$\begingroup\$ Can you elaborate? I get the reason for enum but what do you have in your array? The reason i went for hash maps is because i want the movement to make use of Booleans (in order to avoid the OS "lag" of the first key stroke). \$\endgroup\$ – Nick Jul 3 '17 at 21:09
  • \$\begingroup\$ I edited my post with some more info. Basically you can use a single int as a bitmask for Boolean variables later. In my instance, you could do in your updateGame() method something like if ((currentEvent&UP) == UP) { // take action } \$\endgroup\$ – tayoung Jul 3 '17 at 21:13
  • \$\begingroup\$ The array I use would only be if you need to buffer inputs for more than 1 frame for whatever reason and would store the value of "currentInput" for the amount of frames needed. What I wrote now should be an efficient way to suit your needs without the overhead of a hashmap. \$\endgroup\$ – tayoung Jul 3 '17 at 21:28
  • \$\begingroup\$ Ok i get the idea. I could be using ints like in your example and based on the number act accordingly. Performance wise this is definitely faster than copying the whole hash map but i believe there are some problems with this implementation. First, i will need an int for every single action, right now i only have movement but if/when more things are added this will be a bit of a chaos. I could use an array for that and have all the values there and just use a temporary int to pass around the value of the key pressed. The second problem is, can this way handle multiple inputs simultaneously? \$\endgroup\$ – Nick Jul 3 '17 at 21:28
  • \$\begingroup\$ I would just use an int for every standard button press and use either the currentInput or an input buffer array to check for any input commands. Yes, this will handle multiple inputs since it's flags-based. \$\endgroup\$ – tayoung Jul 3 '17 at 21:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.