[SOLVED: own code bug] Error due concurrency or race condition using events?

Haxe
#1

Hello,

doing a little demo from the Elm examples in OpenFL I found a bug, and I wonder if it is on my code or in the platform.

What I tried to do: I had to print the arrow status in the screen depending on what the user press, with the same formatting as a vector in physics {x:1, y:0}

The problem is that when I press up+right or other key combinations, for some reason I get one value saved permanently, and never removed when the key is released (as it should). I wonder if the problem is on my side and I should implement using an atomic block where the variables are modified.

The code is a bit messy, sorry, I went straight to the OOP way and that was the first idea that came into my mind about how to implement the equivalent elm example.

package;
import openfl.events.KeyboardEvent;
import openfl.text.TextField;
import openfl.display.Sprite;

class Main extends Sprite {
    private var keys_horitzonal:Int = 0;
    private var keys_vertical:Int = 0;
    private var tf:TextField;
	public function new () {
		super();
        tf = new TextField();
        tf.width = 500;
        addChild(tf);
        update_signal(); // init
        stage.addEventListener (KeyboardEvent.KEY_DOWN, key_handle);
        stage.addEventListener (KeyboardEvent.KEY_UP, key_handle);
    }
    private function update_signal() tf.text = Std.string('{ x = ${keys_horitzonal}, y = ${ keys_vertical } }');
    private function key_handle(key:KeyboardEvent) {
        // f is the function modifier determines whether press or release (add or substract)
        var f = switch(key.type) {
            case KeyboardEvent.KEY_DOWN: function(old ,new_status){ trace("down");return  new_status; }; // The event is repeated on long press. Assign only.
            case KeyboardEvent.KEY_UP: function(old, new_status){ trace("up");return old - new_status; }; // Release is sent only once.
            default: return; // any other KeyBoard event doesn't need to be processed here
        }
        // on press left negative, right possitive
        keys_horitzonal = f(keys_horitzonal,if (key.keyCode == 37) - 1 else if (key.keyCode == 39) 1 else 0);
        // on press up negative, right possitive
        keys_vertical = f(keys_vertical, if (key.keyCode == 38) 1 else if (key.keyCode == 40) - 1 else 0);
        update_signal();

        /* I am not really satisfied with this code, long pressing repeats the event and in addition the variable is
           not thread safe because the keys_* variables aren't atoms.
        */
    }
}
#2

Your code is too complicated, and I’d assume that’s what made you miss the errors. Plus, it makes it harder for other people to understand.

Here’s what I’ve deduced:

  1. keys_horizontal is an integer is so it can represent three values: left (-1), right (+1), or not moving (0). Same with keys_vertical.
  • keys_horitzonal is just a typo.
  • You have to use different code for KEY_DOWN and KEY_UP events. Function f lets you choose which of the two possible algorithms to use, without yet knowing where you’ll store the result.
  • If left is pressed, you used to subtract 1 from keys_horizontal, and if right is pressed, you used to add 1. The reason you did this instead of overwriting is so that when both were pressed, the result would be 0. (Same with up and down.)
  • You don’t do #4 anymore because keys repeat if you hold them down. If the user held right, you’d receive lots of the same KEY_DOWN event, and keys_horizontal would keep getting bigger. Instead, you overwrite the value.
  • You still subtract 1 (or subtract -1, as the case may be) when you get a KEY_UP event.
  • You call f for both key_horizontal and key_vertical, even though only one of the two is relevant. You pass 0 to the irrelevant one, so that nothing will be added or subtracted.

Hopefully you can spot the problems now. Take a minute to review your code; it’s better if you find it yourself.

Hint for the first problem: it’s a combination of #5 and #6.
Hint for the second problem: it’s a combination of #5 and #7.

Problem 1: you overwrite when pressing keys, but not when releasing them.

Pressing left gives you -1. Then if you press right, it’ll overwrite the -1 and you get +1. When you release left, it’ll subtract -1, giving you +2.

You’re subtracting -1 because you think the value is either -1 or 0, but it’s not, because the value has already been overwritten.

Problem 2: you think that passing 0 does nothing, but it actually resets the value.

On a KEY_DOWN event, you overwrite the existing value with whatever the “new status” happens to be. If you press right (as in the above example), new_status will be 1 for keys_horizontal and 0 for keys_vertical.

In short, pressing a horizontal key resets your vertical axis, and pressing a vertical key resets your horizontal axis. Pressing the spacebar (or any other key) resets everything.

So how do you fix this? Well, what you really need to do is simplify. Don’t try to combine your key handlers with your up/down logic. Store keys when they’re pressed and released, then later count which keys are pressed.

Feel free to use my class (it’s available under one of the most permissive licenses around). Setup is simple: call Key.init() in your main function.

Once you set it up, you can get a boolean value for each key. The advantage is that you can look at left and right at the same time, rather than trying to store both in one integer. Here’s how I’d do it:

package;

import com.player03.haxeutils.Key;
import openfl.events.Event;
import openfl.text.TextField;
import openfl.display.Sprite;
import openfl.ui.Keyboard;

class Main extends Sprite {
    private var keys_horizontal:Int = 0;
    private var keys_vertical:Int = 0;
    private var tf:TextField;
    
    public function new () {
        super();
        tf = new TextField();
        tf.width = 500;
        addChild(tf);
        update_signal(); // init
        Key.init();
        addEventListener(Event.ENTER_FRAME, frame_handle);
    }
    
    private function update_signal() tf.text = Std.string('{ x = ${keys_horizontal}, y = ${ keys_vertical } }');
    
    private function frame_handle(event:Event) {
        keys_horizontal = 0;
        keys_vertical = 0;
        
        if(Key.isDown(Keyboard.LEFT)) keys_horizontal--;
        if(Key.isDown(Keyboard.RIGHT)) keys_horizontal++;
        if(Key.isDown(Keyboard.UP)) keys_vertical--;
        if(Key.isDown(Keyboard.DOWN)) keys_vertical++;
        
        update_signal();
    }
}
2 Likes
#3

Hello @player_03,

thank you for your answer and the detailed analysis you made; I even didn’t realized that my code was that unreadable and complicated until you exposed it. As you pointed the error was on my side due inconsistencies in the design (adding units and setting a value, all well mixed :sweat_smile: ).

Finally I ended up rewritting everything and following more the elm design and minimizing the side effects to only one single function (creating an array with all keys pressed and then just working on top of the array following a functional approach). The helper still requires the user to add to event listeners vs only one as you have in your helper.

When I look at your code (the Key class) I am learning a lot about how you deal with the event but question comes to my mind: is it not expensive to make those sort of checkings on every frame? I mean, without having enough experience developing games, I suppose that if the frame event handler is overloaded it may cause a performance problem.

Thank you for your answer.

#4

It’s a pretty quick lookup, especially now that I’ve inlined the function. Don’t worry about it until you profile your code.

You’re in luck, because experience isn’t a factor here. Experienced game developers are still terrible at guessing the impact of a piece of code.

That’s the idea behind these “rules of optimization”:

  1. Don’t.
  2. Don’t… yet.
  3. Profile before optimizing.

Read that third one all the way through, get yourself a profiler, and focus on writing maintainable code.

2 Likes