Illegal keys are incorrectly released

[AlmasB]

Discussed in #1312

^{Originally posted by LegionaryCohort October 26, 2023}
Hi,
First of all, great game engine, having a lot of fun playing around with it so far!

Context for my question:
I have a semi-static map (top-down view) and I'm trying to implement input handling that allows the player to move the viewport.
Nothing fancy, just WASD-keys to move the view around. This works great so far.
I'm now trying to add the ability to move the view around faster when pressing the Shift key as well.

So far I've tried the following:

1. Add seperate UserActions for the keys with and without the Shift modifier.
   The UserActions simply set the viewport speed, which in turn is used in onUpdate to move the viewport around.
   This mostly works as expected, however with one caveat. When I press the A key, and then press the Shift key (while still holding the A key down), it does not switch to the fast movement. Vice versa, if I start with Shift-A and then release the Shift key (while still holding the A key), the movement stops entirely for a short moment, then resumes in slow mode.
   The cause appears to be that as long as the A-key remains pressed, the Shift key is not registered. Or in the second scenario, the Shift-A combination is registered and then stops and it takes the engine a short moment to register that the A-key is still being pressed.

private double speed = 0;

@Override
protected void initInput() {
	var moveLeft = new UserAction("Move Camera Left (Slow)") {
		@Override
		protected void onActionBegin() {
			speed = -600;
		}
		@Override
		protected void onActionEnd() {
			speed = 0;
		}
	};
	var moveLeftFast = new UserAction("Move Camera Left (Fast)") {
		@Override
		protected void onActionBegin() {
			speed = -1200;
		}
		@Override
		protected void onActionEnd() {
			speed = 0;
		}
	};
	FXGL.getInput().addAction(moveLeft, KeyCode.A);
	FXGL.getInput().addAction(moveLeftFast, KeyCode.A, InputModifier.SHIFT);
}

@Override
protected void onUpdate(double tpf) {
	FXGL.getGameScene().getViewport().setX(viewport.getX() + speed * tpf);
}

2. Add separate UserActions for the WASD keys and the shift key.
   The WASD keys again set the speed, the shift key sets a factor. In onUpdate the viewport is moved according to speed*factor.
   This simply does not work, as the engine throws an IllegalArgumentException when trying to bind an action to the Shift key.
   If I bind toggleSpeed to a different (valid) key however, this gives me exactly the result I would like to achieve, just with the wrong key.

private double speed = 0;
private double factor = 1;

@Override
protected void initInput() {
	var toggleSpeed = new UserAction("Toggle Camera Speed") {
		@Override
		protected void onActionBegin() {
			factor = 2;
		}
		@Override
		protected void onActionEnd() {
			speed = 1;
		}
	};
	var moveLeft = new UserAction("Move Camera Left") {
		@Override
		protected void onActionBegin() {
			speed = -600;
		}
		@Override
		protected void onActionEnd() {
			speed = 0;
		}
	};
	FXGL.getInput().addAction(toggleSpeed, KeyCode.SHIFT);
	FXGL.getInput().addAction(moveLeft, KeyCode.A);
}

@Override
protected void onUpdate(double tpf) {
	FXGL.getGameScene().getViewport().setX(viewport.getX() + speed * factor * tpf);
}

3. Add a TriggerListener that handles the Shift-logic internally
   The TriggerListener checks for the WASD-keys being pressed. If detected, it will check if the Shift-key is also pressed and set the viewport speed accordingly.
   This does not work, keyTrigger.getModifier() always returns NONE, regardless of whether the Shift key is pressed or not.
   The regular movement works just fine though, the Shift key just seems to never be registered.
   (On that note, this does feel like a bug, although I'm not entirely certain it isn't just me doing something wrong).

private double speed = 0;
private double factor = 1;

@Override
protected void initInput() {
	FXGL.getInput().addTriggerListener(
		new TriggerListener() {
			@Override
			protected void onAction(Trigger trigger) {
				if (trigger.isKey()) {
					var keyTrigger = (KeyTrigger) trigger;
					if (keyTrigger.getKey() == KeyCode.A) {
						speed = -600;
						factor = (keyTrigger.getModifier() == InputModifier.SHIFT) ? 2 : 1;
					}
				}
			}

			@Override
			protected void onActionEnd(Trigger trigger) {
				if (trigger.isKey()) {
					var keyTrigger = (KeyTrigger) trigger;
					if (keyTrigger.getKey() == KeyCode.A) {
						speed = 0;
						factor = 1;
					}
				}
			}
		}
	);
}

@Override
protected void onUpdate(double tpf) {
	FXGL.getGameScene().getViewport().setX(viewport.getX() + speed * factor * tpf);
}

So, my question:
How do I implement the behaviour that I want, i.e. smooth switching between regular and fast movement, without having to release the movement keys in between?
Note that I also tried using the onAction method, rather than the onActionBegin (figuring it might simply be that the single starting tick was the problem), but this made no difference.
I feel like I'm probably missing something obvious or I'm just misusing the API, so if you could give me any pointers on this, that would be great!
(And sorry for the long post, I'm hoping the code samples clarify what I'm doing, or trying to do)

[LegionaryCohort]

For the design of the relevant test-cases there is still the open questions of the specific desired behaviour.
It's currently unclear (at least to me) what this should look like with regards to TriggerListeners and/or UserActions.

I'm assuming that there are two UserActions defined:
A and Shift-A, assigned to the respective KeyCode-InputModifier combinations.
I propose the following desired behaviors for these scenarios:

1. Press & hold the A key. Then press & release the Shift key.
   → A action, then Shift-A action, then A action
2. Press & hold the A key. Then press & hold the Shift key. Then release the A key.
   → A action, then Shift-A action, then no action
3. Press & hold the Shift key. Then press & release the A key.
   → no action, then Shift-A action, then no action
4. Press & hold the Shift key. Then press & hold the A key. Then release the Shift key.
   → no action, then Shift-A action, then A action

This handling model also covers more complex combinations with additional keys involved, assuming each of the non-modifier keys are handled separately. It probably still makes sense to add in one or two additional tests for more complex cases.

What I'm not sure about is if this could cause any unwanted side-effects, as this is fairly strongly enforcing the coupling of modifiers and keys. Specifically this prohibits any modifier keys from realistically being used as standalone keys, potentially complicating TriggerListeners that listen for these keys in isolation.

What are your thoughts on this?

[DeathPhoenix22]

Hi,

I had to do something similar with my Viewport handler. Basically, the user may press (and hold) the, e.g., up arrow so it will scroll up, but if the user starts to press the left arrow, it must now scroll to the left. In my case, there was a "glitch" with the Key Spam calls as they were not at the same time, making the Viewport scroll up, then a fraction of a second scroll left. I therefor had to ensure that both the up and left scrolls were performed Synched!

To easily fix this, I would have required that you can check if a Key is currently pressed, so at each onUpdate I do what the keys are indicating, but I haven't found how to do so.

What I did instead was also fairly simple:

    ...
    private boolean left = false;
    private boolean up = false;
    ...
    @Override
    public void onInit() {
        onKey(KeyCode.UP, () -> {
            if(canVerticalScroll()) {  // Just checking that were not already scrolling DOWN
                up = true;
                down = false;
                upTimer.capture();
            }
        });
        onKey(KeyCode.LEFT, () -> {
            if(canHorizontalScroll()) { // Just checking that were not already scrolling RIGHT
                left = true;
                right = false;
                leftTimer.capture();
            }
        });
        ...
    }
...
@Override
    public void onGameUpdate(double tpf) {
        if(scrolledTimer.elapsed(SCROLLING_LIMITATOR_DURATION)) {  // Limit redraws. Viewport change = full redraw :(
            boolean moved = false;
            if (left) {
                move(Direction.LEFT);
                left = false;
            }
            if (up) {
                move(Direction.UP);
                up = false;
            }
            ...
        }
    }

So for you (something like):

    ...
    private double speed = 0;
    private double factor = 1;
    ...
    @Override
    public void onInit() {
        var toggleSpeed = new UserAction("Toggle Camera Speed") {
		@Override
		protected void onActionBegin() {
			factor = 2;
		}
		@Override
		protected void onActionEnd() {
			speed = 1;
		}
	};
	FXGL.getInput().addAction(toggleSpeed, KeyCode.SHIFT);
        onKey(KeyCode.A, () -> {
            speed = -600;
        });
        ...
    }
...
@Override
    public void onGameUpdate(double tpf) {
        if(speed < 0 || speed > 0) {
            FXGL.getGameScene().getViewport().setX(viewport.getX() + speed * factor * tpf);
            speed = 0;
        }
    }

If you don't want to "Over Draw", you should apply a LocalTimer to avoid FullScreen redraw every frames. For me, it killed the FPS after 1 second of pressing.

[AlmasB]

@LegionaryCohort apologies for the delayed reply, your proposal regarding the 4 combinations seems reasonable.

I am not particularly worried about TriggerListener behaviour because they are effectively just emulating raw inputs, whereas UserActions provide a higher-level API for action bindings. So if we get the more difficult part right, I hope the other part will be straightforward.

[AlmasB]

@LegionaryCohort this specific issue should now be fixed, I haven't considered the above combinations, as it looks like we now have a straightforward way to handle cases where augmented WASD movements are needed.

Here's a standalone sample:

import com.almasb.fxgl.app.GameApplication;
import com.almasb.fxgl.app.GameSettings;
import com.almasb.fxgl.entity.Entity;
import com.almasb.fxgl.input.KeyTrigger;
import com.almasb.fxgl.input.TriggerListener;
import javafx.scene.input.KeyCode;
import javafx.scene.paint.Color;
import javafx.scene.shape.Rectangle;

import static com.almasb.fxgl.dsl.FXGL.*;

/**
 *
 */
public class InputModifierSample extends GameApplication {

    private Entity e;
    private double speedMult = 1.0;

    @Override
    protected void initSettings(GameSettings settings) {
        settings.setWidth(1280);
        settings.setHeight(720);
    }

    @Override
    protected void initInput() {
        getInput().addTriggerListener(new TriggerListener() {
            @Override
            protected void onKeyBegin(KeyTrigger keyTrigger) {
                if (keyTrigger.getKey() == KeyCode.SHIFT) {
                    speedMult = 3.0;
                }
            }

            @Override
            protected void onKey(KeyTrigger keyTrigger) {
                switch (keyTrigger.getKey()) {
                    case W -> { e.translateY(-speedMult * 1); }
                    case S -> { e.translateY(speedMult * 1); }
                    case A -> { e.translateX(-speedMult * 1); }
                    case D -> { e.translateX(speedMult * 1); }
                }
            }

            @Override
            protected void onKeyEnd(KeyTrigger keyTrigger) {
                if (keyTrigger.getKey() == KeyCode.SHIFT) {
                    speedMult = 1.0;
                }
            }
        });
    }

    @Override
    protected void initGame() {
        e = entityBuilder()
                .at(150, 150)
                .view(new Rectangle(40, 40, Color.BLUE))
                .buildAndAttach();
    }

    public static void main(String[] args) {
        launch(args);
    }
}

To get the latest build, you can use this:

<dependencies>
    <dependency>
        <groupId>com.github.almasb</groupId>
        <artifactId>fxgl</artifactId>
        <version>21+dev-SNAPSHOT</version>
    </dependency>
</dependencies>

<repositories>
    <repository>
        <id>oss.sonatype.org-snapshot</id>
        <url>https://oss.sonatype.org/content/repositories/snapshots</url>
    </repository>
</repositories>