Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keys held down are no longer listened to when another key is pressed #502

Closed
jedeen opened this issue Aug 18, 2015 · 20 comments
Closed

keys held down are no longer listened to when another key is pressed #502

jedeen opened this issue Aug 18, 2015 · 20 comments
Assignees
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Milestone

Comments

@jedeen
Copy link
Member

jedeen commented Aug 18, 2015

Given an custom actor.update() :

public update(engine, delta) {
   super.update(engine, delta);
   // move
   if (engine.input.keyboard.isKeyDown(ex.Input.Keys.Up)) {
      this.dy = -200;
   }
   if (engine.input.keyboard.isKeyDown(ex.Input.Keys.Down) {
      this.dy = 200;
   }

   // stop moving
   if (engine.input.keyboard.isKeyUp(ex.Input.Keys.Up)) {
      this.dy = 0;
   }
   if (engine.input.keyboard.isKeyUp(ex.Input.Keys.Down) {
      this.dy = 0;
   }
}

key-input-bug

example: If you hold down 'up', and then tap 'down', while still holding 'up', the 'up' key is ignored (and vice versa).

@jedeen jedeen added the bug This issue describes undesirable, incorrect, or unexpected behavior label Aug 18, 2015
@jedeen jedeen added this to the 0.7 Release milestone Aug 18, 2015
@kamranayub
Copy link
Member

oops 😭

@jedeen jedeen modified the milestones: 0.6 Release, 0.7 Release Aug 19, 2015
@eonarheim eonarheim self-assigned this Aug 19, 2015
@eonarheim
Copy link
Member

So, it looks like we confused ourselves.

The function we actually wanted above was isKeyPressed the difference between isKeyDown and isKeyPressed is that isKeyDown measure the instantaneous moment the key was down and not the state of the key (which is what isKeyPressed does).

I posit, this is mega confusing 😞

How about this instead:

  • keyboard.isPressed(key) - Tells you if a key was just pressed
  • keyboard.isReleased(key) - Tells you if a key was just released
  • keyboard.isDown(key) - Tells you if the key is currently in the down state

@eonarheim
Copy link
Member

This also has implications for the event based api as well

keyboard.on('pressed', function(){...}) vs keyboard.on('down', function(){...})

@kamranayub
Copy link
Member

Yah that makes more sense I think...

On Tue, Aug 18, 2015 at 10:09 PM Erik Onarheim notifications@github.com
wrote:

This also has implications for the event based api as well

keyboard.on('pressed', function(){...}) vs keyboard.on('down',
function(){...})


Reply to this email directly or view it on GitHub
#502 (comment)
.

@kamranayub
Copy link
Member

Might be easier to walk through an event sequence:

User presses and releases "H" key

Frame | State
0 down
1 pressed (technically may not occur if fast enough)
2 up

User holds key "H" down and then releases

0 down
1 pressed
2 pressed
3 pressed
4 up

Given this, the original API does make technical sense (only one keydown vs. many keypresses) but it's easy to be confused, since I had to write this out to explain it. keydown is entering the pressed state while keypress is the pressed state.

If we wanted to make it clearer we would need different terminology than the usual JS "keypress" and "keydown" terms because colloquially down and pressed mean the same thing.

We could do something like:

  • keyboard.isDown/isPressed(key) - Tells if you a key is being held down (multiple frames)
  • keyboard.justDown/justPressed(key) - Tells you if a key was previously up and is now pressed (one frame only)
  • keyboard.isUp/isReleased(key) - Tells you if a key is in the up state (one frame only)

So in example 1:

Frame  State  isDown  justDown  isUp
-----  -----  ------  --------  ----
0      down   true    true      false
1      press  true    false     false
2      up     false   false     true

And example 2:

Frame  State  isDown  justDown  isUp
-----  -----  ------  --------  ----
0      down   true    true      false
1      press  true    false     false
2      press  true    false     false
3      press  true    false     false
4      up     false   false     true

@alanag13
Copy link
Member

WentUp/wentDown/isHeld? Similarly on (up/down/hold)? I feel like went is more appropriate than is here because it captures the state transition (you could argue or expect that every key on a keyboard that is not in use is up), and that down/hold is a clearer destinction than down/pressed or down/justdown.

@alanag13
Copy link
Member

I would say something like becameUp but that seems way too long

@alanag13
Copy link
Member

If we change this you could also still alias the typical js event names for this for people that are used to that

@alanag13
Copy link
Member

After thinking about it a little more, i think i like wasPressed/wasReleased/isHeld, and on press/release/hold

@kamranayub
Copy link
Member

@alanag13 I like those too I think.

@eonarheim
Copy link
Member

@alanag13 @kamranayub I like the was- prefix on this because it feels like something happened.What do you guys think of wasJustPressed and wasJustReleased to make things more clear that the event just happened? Or is that just too verbose...

Is isHeld more clear than isDown or maybe something like getState(key), thoughts? I do like the conciseness of isHeld.

Also, does this mean we want to rename our event based api to reflect this? or create aliases to the existing api? or is the existing event based api clear?

keyboard.on('down', function(){...}) vs. keyboard.on('pressed', function(){...}) and so on

@alanag13
Copy link
Member

the names of the state checks and event apis should optimally align, imo. This would make it more clear which event causes which state. Therefore we would have
keyboard.wasPressed(key), keyboard.wasReleased(key), keyboard.isHeld(key)

and

keyboard.on('press' || 'release' || 'hold', function(){...})

Terminology should be consistent. For events it should be the name of the action being performed (i.e. on press/ on release/ on hold not on pressed/on released/on held, similar to how we would expect onclick, not onclicked), while for state checks it should be the name of the state for the current frame ( wasPressed/wasReleased/isHeld this frame).

I don't have strong opinions about the aliasing of the old api. I think this is much clearer from a simple choice of words standpoint (and think for this reason we should make these the "main" methods either way), but I can also see how a seasoned js developer might try these events based on the more commonplace keydown, keypress, and keyup events.

@alanag13
Copy link
Member

I think the addition of Just is probably unnecessary, personally the only the only difference between "was" and "was Just" I can think of would mean that "was" actually means "was EVER" (i.e. this was pressed just now VS this was pressed at some point in the past, but now may or may not be).

I think its probably safe to assume that no one using the api would think wasPressed was meant to indicate whether or not a given key was EVER pressed.

@eonarheim
Copy link
Member

I definitely agree the pressed/released/held is a more clear way of dealing with this. The only sticker is the seasoned js programmer trying the up/down/pressed because of familiarity, but I'm willing to sacrifice that experience if we feel aliasing pressed/down, released/up, pressed/held in the event api is too bad.

Yah, the Just is probably too verbose. I'll push a commit with some proposed changes soon.

@kamranayub
Copy link
Member

Have to remember we aren't necessarily targeting seasoned JS developers--furthermore, those event names make sense for browser key handling but in a game they don't as much--e.g. keydown and keypress are both triggered continously as long as a key is held down in the browser--some browsers don't do keypress/keydown correctly or aren't consistent either.

Also I feel it's okay to deviate from "common" JS events if it makes sense in our context or if it's plain confusing--which it was.

@kamranayub
Copy link
Member

If it helps here are unity's methods:

GetButton   Returns true while the virtual button identified by buttonName is held down.
GetButtonDown   Returns true during the frame the user pressed down the virtual button identified by buttonName.
GetButtonUp Returns true the first frame the user releases the virtual button identified by buttonName.

GetKey  Returns true while the user holds down the key identified by name. Think auto fire.
GetKeyDown  Returns true during the frame the user starts pressing down the key identified by name.
GetKeyUp    Returns true during the frame the user releases the key identified by name.

GetMouseButton  Returns whether the given mouse button is held down.
GetMouseButtonDown  Returns true during the frame the user pressed the given mouse button.
GetMouseButtonUp

I don't mind that API personally.

@alanag13
Copy link
Member

Doesnt that reinstate the problem of it being difficult to distinguish between down and press? Its super easy to think getbuttondown would be true when held down here, especially since the method that does do that is just called getbutton. I would never expect getNoun to return a boolean....

@alanag13
Copy link
Member

And again, i might expect getkeyup to be true for all keys by default. I think wasReleased makes it clear that it is only true for that small moment in time.

eonarheim added a commit that referenced this issue Sep 18, 2015
@eonarheim
Copy link
Member

Please review the changes, i actually like doing things this way.

#524

@eonarheim
Copy link
Member

Fixed with #524

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Development

No branches or pull requests

4 participants