-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix gameplay offset adjustment limits not being enforced #31533
Conversation
There was a problem hiding this comment.
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:
osu/osu.Game/Screens/Play/PlayerSettings/BeatmapOffsetControl.cs
Lines 161 to 178 in 471180d
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]
There was a problem hiding this comment.
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.
An incorrect DI type lookup meant the limitations were never being applied. I've added test coverage for these and they should now be working.
I change the flow through which the limitations were applied to account for the control potentially being visible (where the user could adjust the slider bar). Now that the bindable is disabled, this would also not work during the disallowed scenarios.