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.

Sign up for GitHub

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

Jump to bottom

Fix oversight in osu! pause input handling #29500

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Fix oversight in osu! pause input handling #29500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e962ce
Add failing test case
frenzibyte Aug 19, 2024
62dec1c
Fix oversight in input blocking from osu! gameplay resume
frenzibyte Aug 19, 2024
86d0079
Rewrite the fix to look less hacky and direct to the point
frenzibyte Aug 19, 2024
2a49167
Remove flac whatever
frenzibyte Aug 19, 2024
2ecf5ec
Add further test coverage
frenzibyte Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions osu.Game.Rulesets.Osu/UI/OsuResumeOverlay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using osu.Framework.Allocation;
using osu.Framework.Extensions.ObjectExtensions;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Graphics.Cursor;
Expand Down Expand Up @@ -35,23 +36,26 @@ private void load()
{
OsuResumeOverlayInputBlocker? inputBlocker = null;

if (drawableRuleset != null)
var drawableOsuRuleset = (DrawableOsuRuleset?)drawableRuleset;

if (drawableOsuRuleset != null)
{
var osuPlayfield = (OsuPlayfield)drawableRuleset.Playfield;
var osuPlayfield = drawableOsuRuleset.Playfield;
osuPlayfield.AttachResumeOverlayInputBlocker(inputBlocker = new OsuResumeOverlayInputBlocker());
}

Add(cursorScaleContainer = new Container
{
Child = clickToResumeCursor = new OsuClickToResumeCursor
{
ResumeRequested = () =>
ResumeRequested = action =>
{
// 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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I...think I would want to stick to just block next frame only in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@frenzibyte frenzibyte Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhhh... can we just not block either gameplay action if either gameplay action was already pressed before break?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 by OnPressed event 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

Copy link
Collaborator

@bdach bdach Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

inputBlocker.BlockNextPress = true;

Resume();
Expand Down Expand Up @@ -94,7 +98,7 @@ public partial class OsuClickToResumeCursor : OsuCursor, IKeyBindingHandler<OsuA
{
public override bool HandlePositionalInput => true;

public Action? ResumeRequested;
public Action<OsuAction>? ResumeRequested;
private Container scaleTransitionContainer = null!;

public OsuClickToResumeCursor()
Expand Down Expand Up @@ -136,7 +140,7 @@ public bool OnPressed(KeyBindingPressEvent<OsuAction> e)
return false;

scaleTransitionContainer.ScaleTo(2, TRANSITION_TIME, Easing.OutQuint);
ResumeRequested?.Invoke();
ResumeRequested?.Invoke(e.Action);
return true;
}

Expand Down
79 changes: 76 additions & 3 deletions osu.Game.Tests/Visual/Gameplay/TestScenePauseInputHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ public partial class TestScenePauseInputHandling : PlayerTestScene
{
Position = OsuPlayfield.BASE_SIZE / 2,
StartTime = 5000,
},
new HitCircle
{
Position = OsuPlayfield.BASE_SIZE / 2,
StartTime = 10000,
},
new HitCircle
{
Position = OsuPlayfield.BASE_SIZE / 2,
StartTime = 15000,
}
}
};
Expand Down Expand Up @@ -256,7 +266,7 @@ public void TestManiaHeldInputRemainHeldAfterResume()
}

[Test]
public void TestOsuRegisterInputFromPressingOrangeCursorButPressIsBlocked()
public void TestOsuHitCircleNotReceivingInputOnResume()
{
KeyCounter counter = null!;

Expand All @@ -281,19 +291,82 @@ public void TestOsuRegisterInputFromPressingOrangeCursorButPressIsBlocked()
AddAssert("button is released in kbc", () => !Player.DrawableRuleset.Playfield.FindClosestParent<OsuInputManager>()!.PressedActions.Any());
}

[Test]
public void TestOsuHitCircleNotReceivingInputOnResume_PauseWhileHoldingSameKey()
{
KeyCounter counter = null!;

loadPlayer(() => new OsuRuleset());
AddStep("get key counter", () => counter = this.ChildrenOfType<KeyCounter>().Single(k => k.Trigger is KeyCounterActionTrigger<OsuAction> actionTrigger && actionTrigger.Action == OsuAction.LeftButton));

AddStep("press Z", () => InputManager.PressKey(Key.Z));
AddAssert("circle hit", () => Player.ScoreProcessor.HighestCombo.Value, () => Is.EqualTo(1));

AddStep("pause", () => Player.Pause());
AddStep("release Z", () => InputManager.ReleaseKey(Key.Z));

AddStep("resume", () => Player.Resume());
AddStep("go to resume cursor", () => InputManager.MoveMouseTo(this.ChildrenOfType<OsuResumeOverlay.OsuClickToResumeCursor>().Single()));
AddStep("press Z to resume", () => InputManager.PressKey(Key.Z));
AddStep("release Z", () => InputManager.ReleaseKey(Key.Z));

checkKey(() => counter, 1, false);

seekTo(5000);

AddStep("press Z", () => InputManager.PressKey(Key.Z));

checkKey(() => counter, 2, true);
AddAssert("circle hit", () => Player.ScoreProcessor.HighestCombo.Value, () => Is.EqualTo(2));

AddStep("release Z", () => InputManager.ReleaseKey(Key.Z));
checkKey(() => counter, 2, false);
}

[Test]
public void TestOsuHitCircleNotReceivingInputOnResume_PauseWhileHoldingOtherKey()
{
loadPlayer(() => new OsuRuleset());

AddStep("press X", () => InputManager.PressKey(Key.X));
AddAssert("circle hit", () => Player.ScoreProcessor.HighestCombo.Value, () => Is.EqualTo(1));

seekTo(5000);

AddStep("pause", () => Player.Pause());
AddStep("release X", () => InputManager.ReleaseKey(Key.X));

AddStep("resume", () => Player.Resume());
AddStep("go to resume cursor", () => InputManager.MoveMouseTo(this.ChildrenOfType<OsuResumeOverlay.OsuClickToResumeCursor>().Single()));
AddStep("press Z to resume", () => InputManager.PressKey(Key.Z));
AddStep("release Z", () => InputManager.ReleaseKey(Key.Z));

AddAssert("circle not hit", () => Player.ScoreProcessor.HighestCombo.Value, () => Is.EqualTo(1));

AddStep("press X", () => InputManager.PressKey(Key.X));
AddStep("release X", () => InputManager.ReleaseKey(Key.X));

AddAssert("circle hit", () => Player.ScoreProcessor.HighestCombo.Value, () => Is.EqualTo(2));
}

private void loadPlayer(Func<Ruleset> createRuleset)
{
AddStep("set ruleset", () => currentRuleset = createRuleset());
AddStep("load player", LoadPlayer);
AddUntilStep("player loaded", () => Player.IsLoaded && Player.Alpha == 1);
AddUntilStep("wait for hud", () => Player.HUDOverlay.ChildrenOfType<SkinComponentsContainer>().All(s => s.ComponentsLoaded));

AddStep("seek to gameplay", () => Player.GameplayClockContainer.Seek(0));
AddUntilStep("wait for seek to finish", () => Player.DrawableRuleset.FrameStableClock.CurrentTime, () => Is.EqualTo(0).Within(500));
seekTo(0);
AddAssert("not in break", () => !Player.IsBreakTime.Value);
AddStep("move cursor to center", () => InputManager.MoveMouseTo(Player.DrawableRuleset.Playfield));
}

private void seekTo(double time)
{
AddStep($"seek to {time}ms", () => Player.GameplayClockContainer.Seek(time));
AddUntilStep("wait for seek to finish", () => Player.DrawableRuleset.FrameStableClock.CurrentTime, () => Is.EqualTo(time).Within(500));
}

private void checkKey(Func<KeyCounter> counter, int count, bool active)
{
AddAssert($"key count = {count}", () => counter().CountPresses.Value, () => Is.EqualTo(count));
Expand Down
Loading