Escape menu cannot properly show pause menu (it is shown and then closes)

[neoh4x0r]

The escape key can be bound under Settings->Controls->Pause (this is the default)

However escape is internally bound to menu exit (to go back to the last menu) -- this is not exposed through the control scheme.

When escape is pressed the pause will will appear and then immediately disappear.

Pause can be bound to any keyboard key/gamepad button other than escape -- and the issue does not occur.

The default binding for pause should be set to something other then escape.

[LunaMoo]

The only bad thing about pause button I can find is that when user removes all mapping for it, they can't exit the game(on platforms other than windows which can do it from windows menu/hotkey) without closing ppsspp until they map some button again.

Nothing bad happens with esc being mapped as default here and since there's no "back" functionality that could be triggered from in-game and cause instant return from "pause" the situation as described in here can't really happen, unless you hold the button instead of pressing it or have some hardware problems which doesn't pass the input correctly. Possibly caused by third party software messing with input as well.

Either way remapping the defaults for everyone that got used to it and works properly for is really not an acceptable solution for one user issue.

[unknownbrackets]

I think we should wait for a release of Esc before returning back to the game, ideally, to require two presses. Holding down ideally shouldn't swap back and forth.

Maybe it's some weird repeat rate issue here.

-[Unknown]

[neoh4x0r]

@LunaMoo @unknownbrackets I am pressing the button and not holding it down and again ESC is the only button that causes this issue (if I remap pause to another button it works correctly) -- so I'm not sure if waiting for two button presses will make any difference.

"the situation as described in here can't really happen". I'm sorry but that statement does not appear to be valid in this case -- if it were impossible for it to happen then there would have been no issue to report.

I doubt that I am the first user to experience this issue (running this on Debian/Stretch 9 amd64) just that I am the first to report it.

[unknownbrackets]

SDL or Qt? Maybe this is a Linux only problem, then.

-[Unknown]

[LunaMoo]

Google for weird behaviour of escape key on debian first result brings "ESC key causes a small delay in terminal due to its Alt+ behavior" so basically when you press an esc key on that system it might be equivalent of holding that key on most other platforms. (to clarify what I ment by "most" ~ when it comes to keyboard aside from windows only tried some ubuntu;p /sdl build)

[neoh4x0r]

@LunaMoo Those issues only affect the terminal/vim and would not affect ppsspp.

I am using sdl and it may only affect linux users.

[LunaMoo]

From my personal experience including sdl build on ubuntu there are no problems like that in ppsspp. You're on desktop or laptop? Some of the latter might use esc as a switch for function keys which I'm guessing would also "keep it pressed". As that's really the only way I can reproduce such problem, by intentionally holding the key.

[neoh4x0r]

@LunaMoo I'm on a desktop and no other programs are having issues with proper escape key handling (just this one).

@unknownbrackets

The problem stems from the escape key being set as a hardcoded cancel key in Common/KeyMap.cpp::void UpdateNativeMenuKeys()

Notice this part: const KeyDef hardcodedCancelKeys[] = { KeyDef(DEVICE_ID_KEYBOARD, NKCODE_ESCAPE),

I removed this entry from the array and the problem went away.

Honestly I don't see the need to have these keys hard coded because properly setting up the key bindings on the control page should be enough.

Any key that is hard coded here cannot be overridden and will conflict with any duplicate key binding if they should be used.

At the very least -- there should be a check that disables native menu key functionality while a game is running.

---

Common/KeyMap.cpp::void UpdateNativeMenuKeys()

// TODO: This is such a mess...
void UpdateNativeMenuKeys() {
    std::vector<KeyDef> confirmKeys, cancelKeys;
    std::vector<KeyDef> tabLeft, tabRight;
    std::vector<KeyDef> upKeys, downKeys, leftKeys, rightKeys;

    int confirmKey = g_Config.iButtonPreference == PSP_SYSTEMPARAM_BUTTON_CROSS ? CTRL_CROSS : CTRL_CIRCLE;
    int cancelKey = g_Config.iButtonPreference == PSP_SYSTEMPARAM_BUTTON_CROSS ? CTRL_CIRCLE : CTRL_CROSS;

    KeyFromPspButton(confirmKey, &confirmKeys);
    KeyFromPspButton(cancelKey, &cancelKeys);
    KeyFromPspButton(CTRL_LTRIGGER, &tabLeft);
    KeyFromPspButton(CTRL_RTRIGGER, &tabRight);
    KeyFromPspButton(CTRL_UP, &upKeys);
    KeyFromPspButton(CTRL_DOWN, &downKeys);
    KeyFromPspButton(CTRL_LEFT, &leftKeys);
    KeyFromPspButton(CTRL_RIGHT, &rightKeys);

#ifdef __ANDROID__
    // Hardcode DPAD on Android
    upKeys.push_back(KeyDef(DEVICE_ID_ANY, NKCODE_DPAD_UP));
    downKeys.push_back(KeyDef(DEVICE_ID_ANY, NKCODE_DPAD_DOWN));
    leftKeys.push_back(KeyDef(DEVICE_ID_ANY, NKCODE_DPAD_LEFT));
    rightKeys.push_back(KeyDef(DEVICE_ID_ANY, NKCODE_DPAD_RIGHT));
#endif

    // Push several hard-coded keys before submitting to native.
    const KeyDef hardcodedConfirmKeys[] = {
        KeyDef(DEVICE_ID_KEYBOARD, NKCODE_SPACE),
        KeyDef(DEVICE_ID_KEYBOARD, NKCODE_ENTER),
        KeyDef(DEVICE_ID_ANY, NKCODE_BUTTON_A),
    };

    // If they're not already bound, add them in.
    for (size_t i = 0; i < ARRAY_SIZE(hardcodedConfirmKeys); i++) {
        if (std::find(confirmKeys.begin(), confirmKeys.end(), hardcodedConfirmKeys[i]) == confirmKeys.end())
            confirmKeys.push_back(hardcodedConfirmKeys[i]);
    }

    const KeyDef hardcodedCancelKeys[] = {
        KeyDef(DEVICE_ID_KEYBOARD, NKCODE_ESCAPE),
        KeyDef(DEVICE_ID_ANY, NKCODE_BACK),
        KeyDef(DEVICE_ID_ANY, NKCODE_BUTTON_B),
    };

    for (size_t i = 0; i < ARRAY_SIZE(hardcodedCancelKeys); i++) {
        if (std::find(cancelKeys.begin(), cancelKeys.end(), hardcodedCancelKeys[i]) == cancelKeys.end())
            cancelKeys.push_back(hardcodedCancelKeys[i]);
    }

    SetDPadKeys(upKeys, downKeys, leftKeys, rightKeys);
    SetConfirmCancelKeys(confirmKeys, cancelKeys);
    SetTabLeftRightKeys(tabLeft, tabRight);
}

[unknownbrackets]

The reason that it's hardcoded there is to theoretically prevent you from "trapping yourself" when configuring the mapping. It's been proposed to remove this, but right now, that's why it's there.

But, that's not actually your problem. See, what's supposed to happen is:

1. You tap Esc.
2. EmuScreen detects this Back button or Pause keypress.
3. releaseButtons() is called, in case you're holding any other buttons down. Maybe this is part of the problem.
4. The pause screen is shown.
5. If and when you tap Esc a second time (after releasing it), you leave the dialog. All dialogs, including the pause screen, behave this way.

So, what's happening for you is that key() is being triggered on the pause screen as if you pressed Esc again. To me, it looks like releaseButtons() is likely the problem here. Or not, seems like it doesn't actually affect the pressed/not state.

Does the change you posted work if you make the cancel key and the pause screen trigger button the same (i.e. both Esc)? Because that is supposed to work. If that doesn't work, there's still a bug, no matter what other workaround is applied.

It'd be great if you could log here (just add printf("KEYDOWN: %d\n", k);): https://github.com/hrydgard/ppsspp/blob/701b97487639b550b9a391441facc4bc329b1b68/ext/native/base/PCMain.cpp#L746

And see if Esc is sending multiple SDL_KEYDOWN events, as if it were pressed more than once.

-[Unknown]

[neoh4x0r]

@unknownbrackets Yes it seems to record two button down events....I'm looking at ways to eliminate the double key down event. I added this to /PCMain.cpp#L736

int k = event.key.keysym.sym;                   
printf("SDL_KEYDOWN: %d (repeat: %i)\n", k, event.key.repeat);

And I get this when pressing escape: SDL_KEYDOWN: 27 (repeat: 0) SDL_KEYDOWN: 27 (repeat: 1) SDL_KEYUP: 27 (repeat: 0)

Maybe we should just ignore any key down events where repeat is greater than 0.

case SDL_KEYDOWN:
if (event.key.repeat > 0) { break;}

Adding this check to prevent duplicate key_down events, make the pause menu work without changing anything else.

This also ensures that other keys work as intended -- such as toggling full screen. If two keydown events the windows goes full screen and then immediately back to non-fullscreen, but by ignoring the repeated event it works.

This is the method I would recommend as is fixes the whole issue and not just the pause menu.

[unknownbrackets]

Nicely investigated. Hmm, what we could do is (PCMain.cpp):

key.flags = KEY_DOWN | (event.key.repeat ? KEY_IS_REPEAT : 0);

And then maybe change this part (ui_screen.cpp):

if (!retval && (key.flags & KEY_DOWN) && UI::IsEscapeKey(key)) {

To perhaps:

if (!retval && (key.flags & (KEY_DOWN | KEY_IS_REPEAT)) == KEY_DOWN && UI::IsEscapeKey(key)) {

Which would ensure that repeats are ignored for leaving dialogs (this seems to make more sense to me anyway.)

The main place we might want repeats is when typing into fields - there aren't many, but there are a couple in settings, mostly related to network. But maybe indeed it might be better to just ignore repeats, especially if sometimes it sends them very soon.

-[Unknown]

[neoh4x0r]

@unknownbrackets But like I mentioned only doing this for the ESC key means that things like fullscreen toggle will not work and any button press will be duplicated.

I really don't see this affecting text input, since once SDL_TEXTINPUT is sent a flag could be set and any SDL_KEYDOWN / SDL_KEYUP events can be ignored by checking if textinput is active.

Also the text input for the networking configuration page really does not allow you type manually type anything -- the only thing that asks for input is 'Change PRO ad hoc sever ip address' and you must enter the text by using the on-screen buttons (no manual text entry) -- so this should not interfere. That's with v 1.3-871-g8f5f15455.

[unknownbrackets]

You're right, we should just ignore the repeats. Want to PR?

Hmm, the missing text input sounds like an oversight. Looks like the issue is here: https://github.com/hrydgard/ppsspp/blob/b9a9a509ae2dacee42251b5efc4983214a2b174b/UI/GameSettingsScreen.cpp#L587 https://github.com/hrydgard/ppsspp/blob/b9a9a509ae2dacee42251b5efc4983214a2b174b/UI/GameSettingsScreen.cpp#L728

Which should probably better be #if !defined(MOBILE_DEVICE) && !defined(USING_QT_UI), since apparently Qt doesn't support KEY_CHAR either right now. I guess that'd be better for Windows Mobile too, @hrydgard - assuming that sets MOBILE_DEVICE.

-[Unknown]

[neoh4x0r]

@unknownbrackets Yeah PR'ing this sounds good

[neoh4x0r]

@unknownbrackets

Here's the git diff:

diff --git a/ext/native/base/PCMain.cpp b/ext/native/base/PCMain.cpp
index a7a676f05..256bcd1b2 100644
--- a/ext/native/base/PCMain.cpp
+++ b/ext/native/base/PCMain.cpp
@@ -734,6 +734,7 @@ int main(int argc, char *argv[]) {
 #endif
                        case SDL_KEYDOWN:
                                {
+                                       if (event.key.repeat > 0) { break;}
                                        int k = event.key.keysym.sym;
                                        KeyInput key;
                                        key.flags = KEY_DOWN;
@@ -753,6 +754,7 @@ int main(int argc, char *argv[]) {
                                }
                        case SDL_KEYUP:
                                {
+                                       if (event.key.repeat > 0) { break;}
                                        int k = event.key.keysym.sym;
                                        KeyInput key;
                                        key.flags = KEY_UP;

[hrydgard]

@neoh4x0r just PR it yourself :)

I'll fix the other issue with the inputbox.

[neoh4x0r]

@hrydgard How do I PR this when I don't have a branch to pull the change from ? All I have is a patch/diff...

[hrydgard]

What? You should clone, make a branch, do the change, then make a pull request. That's how it's done.

[unknownbrackets]

Yep, go here: https://github.com/hrydgard/ppsspp/fork

Then you can clone:

git clone git@github.com:neoh4x0r/ppsspp.git
git remote add upstream https://github.com/hrydgard/ppsspp.git
git fetch upstream

git checkout -b sdl-repeat upstream/master

# Or your editor of choice.
vim ext/native/base/PCMain.cpp

git add -p
git commit

git push origin sdl-repeat

And then go to https://github.com/hrydgard/ppsspp and open a pull request.

-[Unknown]

[neoh4x0r]

@unknownbrackets @hrydgard I just created the branch and pull request.

When dealing with github I almost never needed to create a fork and pull request -- most of the time providing a patch in a issue was enough -- but now I know how to do it....thanks.

[hrydgard]

Great! People will be happier when you provide pull requests instead of patches - much faster to merge and easier to review with inline comments.

Thanks!

[hrydgard]

So did that take care of all the issues here?

Bumping to 1.6.0 anyway.

[neoh4x0r]

@hrydgard You said you would look into the other issue with the inputbox. That PR resolved the double key_down events (the issue with the ESC key pausing and closing the menu)...so I guess this issue can be closed.

[unknownbrackets]

I think that was done in 35e2a5a7d1db4ae83c8d80187efdb0bfaf8ab7bb, wasn't it?

-[Unknown]