Fix oversight in osu! pause input handling #29500
Conversation
| { | ||
| bool block = BlockNextPress; | ||
| base.Update(); | ||
| BlockNextPress = false; |
There was a problem hiding this comment.
I feel like this needs local documentation? On its own it looks like a bug.
There was a problem hiding this comment.
I tried adding explanatory note on this one but I realised how hacky it makes the whole fix sound, so I ended up rewriting the fix to make more sense.
| { | ||
| // since the user had to press a button to tap the resume cursor, | ||
| // block that press event from potentially reaching a hit circle that's behind the cursor. | ||
| // we cannot do this from OsuClickToResumeCursor directly since we're in a different input manager tree than the gameplay one, | ||
| // so we rely on a dedicated input blocking component that's implanted in there to do that for us. | ||
| if (inputBlocker != null) | ||
| // note this only matters when the user didn't pause while they were holding the same key that they are resuming with. | ||
| if (inputBlocker != null && !drawableOsuRuleset.AsNonNull().KeyBindingInputManager.PressedActions.Contains(action)) |
There was a problem hiding this comment.
My initial reading of this was "doesn't this mean that if you paused with LeftButton held on a slider, and then want to resume holding by clicking the resume cursor indicator with RightButton, the next RightButton input will get blocked still?" But in testing that... does not appear to be the case for whatever reason? Maybe that has something to do with that entire key up/down syncing/not syncing crap or whatever. Not sure. Any commentary to offer on that?
There was a problem hiding this comment.
I...think I would want to stick to just block next frame only in that case.
There was a problem hiding this comment.
I mean I'd also be fine with "if either left or right button is pressed on the ruleset, don't block the new input" or something.
There was a problem hiding this comment.
Actually no, the next RightButton will not be blocked, what will be blocked is the point at which the user presses the RightButton on the orange cursor.
The point of adding this conditional is to make sure that we're not blocking next press unless we know for a fact that the next press will be generated within this same frame (generated by the user pressing on the orange cursor and triggering a press on gameplay due to that press).
If that sounds bad then I'll just make it block for one frame no questions asked.
There was a problem hiding this comment.
Uhhh... can we just not block either gameplay action if either gameplay action was already pressed before break?
There was a problem hiding this comment.
It's different for sliders because input is tracked by checking
PressedActions. This entire input-blocking concept is made specifically for hit circles which receives input byOnPressedevent instead, it does not apply to sliders and spinners. Because, well, that's how stable does it, apparently?
I don't get this. Does this mean a slider will/"should" start tracking immediately if you start holding a new button before unpausing? Because that sounds wrong to me, ignoring whatever stable may be doing.
Like, as far as I understand what we want is very simple on paper:
- If a key was held before pause, and the key is held after, then it should remain held
- If a key was not held before pause, and is held when unpause occurs, it should not be seen by gameplay
There was a problem hiding this comment.
I think that for sliders specifically, if you're holding a slider with left key, pause, and then want to unpause and continue holding the slider with right key, the input should not get dropped, no? Dropping that input just seems anti-user, they'd have to remember which key they paused with before.
There was a problem hiding this comment.
Maybe it sounds anti-user, but allowing it leads to concerns of using pause buffering as an abuse vector. Also it feels weird that holds are seen but new presses aren't?
There was a problem hiding this comment.
@peppy There's a difference between what you said here and what I'm conveying here. If the user held a key, then unpaused with another key, only the key that was used for unpausing will be seen by gameplay, the other will not be.
As far as I'm aware, you can change keys while holding a slider, so there's no issue here?
There was a problem hiding this comment.
The key explicitly used to trigger the unpause can be seen as a hold only, i guess, sure. It's still a bit haphazard (that it only happens for hold not click) but sure.
|
This is on the verge of my comprehension at this point, but the tests look correct so let's just get this fixed for now. @bdach did you want to look at this any further? |
|
Not really. |
Issue is pretty simple. The user pauses with the key held, then on resume the resume overlay sets
BlockNextPress, but it doesn't get reset until the next press which is when the user uses the key again.The idea of
BlockNextPressis to block any press event in the same frame as the resume operation, so just do that instead (by resetting the flag inUpdate()).