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

Fix gameplay offset adjustment limits not being enforced #31533

Merged
merged 3 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
87 changes: 87 additions & 0 deletions osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally the change looks fine, but I've got a bit of a problem with these tests - namely that... they don't fail even before the fix? Checking out a4174a3, I get all passes.

What I believe is happening there is that the test is exercising its assertions via realm data, but the realm writes are async:

realmWriteTask = realm.WriteAsync(r =>
{
var setInfo = r.Find<BeatmapSetInfo>(beatmap.Value.BeatmapSetInfo.ID);
if (setInfo == null) // only the case for tests.
return;
// Apply to all difficulties in a beatmap set if they have the same audio
// (they generally always share timing).
foreach (var b in setInfo.Beatmaps)
{
BeatmapUserSettings userSettings = b.UserSettings;
double val = Current.Value;
if (userSettings.Offset != val && b.AudioEquals(beatmap.Value.BeatmapInfo))
userSettings.Offset = val;
}
});

so an AddUntilStep() could unluckily false-pass if it manages to sneak in a run before the write actually happens.

Checking the actual on-screen drawable too fixes that, for whatever that's worth:

diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs
index 326f21ff13..521d097fb9 100644
--- a/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs
+++ b/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs
@@ -41,6 +41,7 @@
 using osu.Game.Screens.OnlinePlay.Playlists;
 using osu.Game.Screens.Play;
 using osu.Game.Screens.Play.HUD;
+using osu.Game.Screens.Play.PlayerSettings;
 using osu.Game.Screens.Ranking;
 using osu.Game.Screens.Select;
 using osu.Game.Screens.Select.Carousel;
@@ -351,8 +352,13 @@ public void TestOffsetAdjustDuringPause()
             AddStep("attempt adjust offset via keyboard", () => InputManager.Key(Key.Minus));
             checkOffset(-1);
 
-            void checkOffset(double offset) => AddUntilStep($"offset is {offset}", () => Game.BeatmapManager.QueryBeatmap(b => b.ID == Game.Beatmap.Value.BeatmapInfo.ID)!.UserSettings.Offset,
-                () => Is.EqualTo(offset));
+            void checkOffset(double offset)
+            {
+                AddUntilStep($"control offset is {offset}", () => this.ChildrenOfType<GameplayOffsetControl>().Single().ChildrenOfType<BeatmapOffsetControl>().Single().Current.Value,
+                    () => Is.EqualTo(offset));
+                AddUntilStep($"database offset is {offset}", () => Game.BeatmapManager.QueryBeatmap(b => b.ID == Game.Beatmap.Value.BeatmapInfo.ID)!.UserSettings.Offset,
+                    () => Is.EqualTo(offset));
+            }
         }
 
         [Test]
@@ -389,8 +395,13 @@ public void TestOffsetAdjustDuringGameplay()
             AddStep("attempt adjust offset via keyboard", () => InputManager.Key(Key.Minus));
             checkOffset(-1);
 
-            void checkOffset(double offset) => AddUntilStep($"offset is {offset}", () => Game.BeatmapManager.QueryBeatmap(b => b.ID == Game.Beatmap.Value.BeatmapInfo.ID)!.UserSettings.Offset,
-                () => Is.EqualTo(offset));
+            void checkOffset(double offset)
+            {
+                AddUntilStep($"control offset is {offset}", () => this.ChildrenOfType<GameplayOffsetControl>().Single().ChildrenOfType<BeatmapOffsetControl>().Single().Current.Value,
+                    () => Is.EqualTo(offset));
+                AddUntilStep($"database offset is {offset}", () => Game.BeatmapManager.QueryBeatmap(b => b.ID == Game.Beatmap.Value.BeatmapInfo.ID)!.UserSettings.Offset,
+                    () => Is.EqualTo(offset));
+            }
         }
 
         [Test]

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I tested this but I guess I never had a case where the database was slow enough for this to occur 🙈. Your fix looks good, applying verbatim.

Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
using osu.Game.Screens.OnlinePlay.Playlists;
using osu.Game.Screens.Play;
using osu.Game.Screens.Play.HUD;
using osu.Game.Screens.Play.PlayerSettings;
using osu.Game.Screens.Ranking;
using osu.Game.Screens.Select;
using osu.Game.Screens.Select.Carousel;
Expand Down Expand Up @@ -317,6 +318,92 @@ public void TestAttemptPlayBeatmapMissingFails()
AddUntilStep("wait for song select", () => songSelect.IsCurrentScreen());
}

[Test]
public void TestOffsetAdjustDuringPause()
{
Player player = null;

Screens.Select.SongSelect songSelect = null;
PushAndConfirm(() => songSelect = new TestPlaySongSelect());
AddUntilStep("wait for song select", () => songSelect.BeatmapSetsLoaded);

AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely());

AddUntilStep("wait for selected", () => !Game.Beatmap.IsDefault);

AddStep("set mods", () => Game.SelectedMods.Value = new Mod[] { new OsuModNoFail() });
AddStep("press enter", () => InputManager.Key(Key.Enter));

AddUntilStep("wait for player", () =>
{
DismissAnyNotifications();
player = Game.ScreenStack.CurrentScreen as Player;
return player?.IsLoaded == true;
});

AddUntilStep("wait for track playing", () => Game.Beatmap.Value.Track.IsRunning);
checkOffset(0);

AddStep("adjust offset via keyboard", () => InputManager.Key(Key.Minus));
checkOffset(-1);

AddStep("pause", () => player.ChildrenOfType<GameplayClockContainer>().First().Stop());
AddUntilStep("wait for pause", () => player.ChildrenOfType<GameplayClockContainer>().First().IsPaused.Value, () => Is.True);
AddStep("attempt adjust offset via keyboard", () => InputManager.Key(Key.Minus));
checkOffset(-1);

void checkOffset(double offset)
{
AddUntilStep($"control offset is {offset}", () => this.ChildrenOfType<GameplayOffsetControl>().Single().ChildrenOfType<BeatmapOffsetControl>().Single().Current.Value,
() => Is.EqualTo(offset));
AddUntilStep($"database offset is {offset}", () => Game.BeatmapManager.QueryBeatmap(b => b.ID == Game.Beatmap.Value.BeatmapInfo.ID)!.UserSettings.Offset,
() => Is.EqualTo(offset));
}
}

[Test]
public void TestOffsetAdjustDuringGameplay()
{
Player player = null;

Screens.Select.SongSelect songSelect = null;
PushAndConfirm(() => songSelect = new TestPlaySongSelect());
AddUntilStep("wait for song select", () => songSelect.BeatmapSetsLoaded);

AddStep("import beatmap", () => BeatmapImportHelper.LoadOszIntoOsu(Game).WaitSafely());

AddUntilStep("wait for selected", () => !Game.Beatmap.IsDefault);

AddStep("set mods", () => Game.SelectedMods.Value = new Mod[] { new OsuModNoFail() });
AddStep("press enter", () => InputManager.Key(Key.Enter));

AddUntilStep("wait for player", () =>
{
DismissAnyNotifications();
player = Game.ScreenStack.CurrentScreen as Player;
return player?.IsLoaded == true;
});

AddUntilStep("wait for track playing", () => Game.Beatmap.Value.Track.IsRunning);
checkOffset(0);

AddStep("adjust offset via keyboard", () => InputManager.Key(Key.Minus));
checkOffset(-1);

AddStep("seek beyond 10 seconds", () => player.ChildrenOfType<GameplayClockContainer>().First().Seek(10500));
AddUntilStep("wait for seek", () => player.ChildrenOfType<GameplayClockContainer>().First().CurrentTime, () => Is.GreaterThan(10600));
AddStep("attempt adjust offset via keyboard", () => InputManager.Key(Key.Minus));
checkOffset(-1);

void checkOffset(double offset)
{
AddUntilStep($"control offset is {offset}", () => this.ChildrenOfType<GameplayOffsetControl>().Single().ChildrenOfType<BeatmapOffsetControl>().Single().Current.Value,
() => Is.EqualTo(offset));
AddUntilStep($"database offset is {offset}", () => Game.BeatmapManager.QueryBeatmap(b => b.ID == Game.Beatmap.Value.BeatmapInfo.ID)!.UserSettings.Offset,
() => Is.EqualTo(offset));
}
}

[Test]
public void TestRetryCountIncrements()
{
Expand Down
1 change: 1 addition & 0 deletions osu.Game/Screens/Play/Player.cs
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ private void load(OsuConfigManager config, OsuGameBase game, CancellationToken c
}

dependencies.CacheAs(DrawableRuleset.FrameStableClock);
dependencies.CacheAs<IGameplayClock>(DrawableRuleset.FrameStableClock);

// add the overlay components as a separate step as they proxy some elements from the above underlay/gameplay components.
// also give the overlays the ruleset skin provider to allow rulesets to potentially override HUD elements (used to disable combo counters etc.)
Expand Down
40 changes: 29 additions & 11 deletions osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -274,20 +274,36 @@ protected override void Dispose(bool isDisposing)
beatmapOffsetSubscription?.Dispose();
}

public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
protected override void Update()
{
base.Update();
Current.Disabled = !allowOffsetAdjust;
}

private bool allowOffsetAdjust
{
// General limitations to ensure players don't do anything too weird.
// These match stable for now.
if (player is SubmittingPlayer)
get
{
// TODO: the blocking conditions should probably display a message.
if (player?.IsBreakTime.Value == false && gameplayClock?.CurrentTime - gameplayClock?.StartTime > 10000)
return false;
// General limitations to ensure players don't do anything too weird.
// These match stable for now.
if (player is SubmittingPlayer)
{
Debug.Assert(gameplayClock != null);

// TODO: the blocking conditions should probably display a message.
if (!player.IsBreakTime.Value && gameplayClock.CurrentTime - gameplayClock.StartTime > 10000)
return false;

if (gameplayClock?.IsPaused.Value == true)
return false;
if (gameplayClock.IsPaused.Value)
return false;
}

return true;
}
}

public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
{
// To match stable, this should adjust by 5 ms, or 1 ms when holding alt.
// But that is hard to make work with global actions due to the operating mode.
// Let's use the more precise as a default for now.
Expand All @@ -296,11 +312,13 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
switch (e.Action)
{
case GlobalAction.IncreaseOffset:
Current.Value += amount;
if (!Current.Disabled)
Current.Value += amount;
return true;

case GlobalAction.DecreaseOffset:
Current.Value -= amount;
if (!Current.Disabled)
Current.Value -= amount;
return true;
}

Expand Down
Loading