From cc341b4119bd9a1aa8cb5aafe936088ff1e0f857 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 25 Jan 2024 15:21:19 +0900 Subject: [PATCH 01/14] Adjust beatmap carousel padding to avoid scrollbar disappearing underneath logo --- osu.Game/Screens/Select/BeatmapCarousel.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 70ecde3858..32a1b5cb58 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -215,6 +215,12 @@ namespace osu.Game.Screens.Select InternalChild = new OsuContextMenuContainer { RelativeSizeAxes = Axes.Both, + Padding = new MarginPadding + { + // Avoid clash between scrollbar and osu! logo. + Top = 10, + Bottom = 100, + }, Children = new Drawable[] { setPool, From 5c049feca1e855daca7499b2e6bfb6482c99fe8a Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sun, 25 Feb 2024 21:18:15 +0300 Subject: [PATCH 02/14] Fix advanced stats in beatmap info overlay showing "key count" on non-mania beatmaps --- osu.Game/Overlays/BeatmapSet/Details.cs | 30 ++++++++++++------- .../Screens/Select/Details/AdvancedStats.cs | 29 ++++++------------ osu.Game/Screens/Select/SongSelect.cs | 7 ++++- 3 files changed, 34 insertions(+), 32 deletions(-) diff --git a/osu.Game/Overlays/BeatmapSet/Details.cs b/osu.Game/Overlays/BeatmapSet/Details.cs index cf78f605aa..d656a6b14b 100644 --- a/osu.Game/Overlays/BeatmapSet/Details.cs +++ b/osu.Game/Overlays/BeatmapSet/Details.cs @@ -10,6 +10,7 @@ using osu.Framework.Graphics.Shapes; using osu.Game.Beatmaps; using osu.Game.Online.API.Requests.Responses; using osu.Game.Overlays.BeatmapSet.Buttons; +using osu.Game.Rulesets; using osu.Game.Screens.Select.Details; using osuTK; @@ -33,10 +34,10 @@ namespace osu.Game.Overlays.BeatmapSet { if (value == beatmapSet) return; - beatmapSet = value; + basic.BeatmapSet = preview.BeatmapSet = beatmapSet = value; - basic.BeatmapSet = preview.BeatmapSet = BeatmapSet; - updateDisplay(); + if (IsLoaded) + updateDisplay(); } } @@ -50,13 +51,10 @@ namespace osu.Game.Overlays.BeatmapSet if (value == beatmapInfo) return; basic.BeatmapInfo = advanced.BeatmapInfo = beatmapInfo = value; - } - } - private void updateDisplay() - { - Ratings.Ratings = BeatmapSet?.Ratings; - ratingBox.Alpha = BeatmapSet?.Status > 0 ? 1 : 0; + if (IsLoaded) + updateDisplay(); + } } public Details() @@ -101,12 +99,22 @@ namespace osu.Game.Overlays.BeatmapSet }; } - [BackgroundDependencyLoader] - private void load() + [Resolved] + private RulesetStore rulesets { get; set; } + + protected override void LoadComplete() { + base.LoadComplete(); updateDisplay(); } + private void updateDisplay() + { + Ratings.Ratings = BeatmapSet?.Ratings; + ratingBox.Alpha = BeatmapSet?.Status > 0 ? 1 : 0; + advanced.Ruleset.Value = rulesets.GetRuleset(beatmapInfo?.Ruleset.OnlineID ?? 0); + } + private partial class DetailBox : Container { private readonly Container content; diff --git a/osu.Game/Screens/Select/Details/AdvancedStats.cs b/osu.Game/Screens/Select/Details/AdvancedStats.cs index 0d68a0ec3c..74276795d2 100644 --- a/osu.Game/Screens/Select/Details/AdvancedStats.cs +++ b/osu.Game/Screens/Select/Details/AdvancedStats.cs @@ -38,11 +38,6 @@ namespace osu.Game.Screens.Select.Details [Resolved] private IBindable> mods { get; set; } - [Resolved] - private OsuGameBase game { get; set; } - - private IBindable gameRuleset; - protected readonly StatisticRow FirstValue, HpDrain, Accuracy, ApproachRate; private readonly StatisticRow starDifficulty; @@ -64,6 +59,8 @@ namespace osu.Game.Screens.Select.Details } } + public Bindable Ruleset { get; } = new Bindable(); + public AdvancedStats(int columns = 1) { switch (columns) @@ -137,12 +134,7 @@ namespace osu.Game.Screens.Select.Details { base.LoadComplete(); - // the cached ruleset bindable might be a decoupled bindable provided by SongSelect, - // which we can't rely on in combination with the game-wide selected mods list, - // since mods could be updated to the new ruleset instances while the decoupled bindable is held behind, - // therefore resulting in performing difficulty calculation with invalid states. - gameRuleset = game.Ruleset.GetBoundCopy(); - gameRuleset.BindValueChanged(_ => updateStatistics()); + Ruleset.BindValueChanged(_ => updateStatistics()); mods.BindValueChanged(modsChanged, true); } @@ -169,8 +161,6 @@ namespace osu.Game.Screens.Select.Details IBeatmapDifficultyInfo baseDifficulty = BeatmapInfo?.Difficulty; BeatmapDifficulty adjustedDifficulty = null; - IRulesetInfo ruleset = gameRuleset?.Value ?? beatmapInfo.Ruleset; - if (baseDifficulty != null) { BeatmapDifficulty originalDifficulty = new BeatmapDifficulty(baseDifficulty); @@ -180,24 +170,24 @@ namespace osu.Game.Screens.Select.Details adjustedDifficulty = originalDifficulty; - if (gameRuleset != null) + if (Ruleset.Value != null) { double rate = 1; foreach (var mod in mods.Value.OfType()) rate = mod.ApplyToRate(0, rate); - adjustedDifficulty = ruleset.CreateInstance().GetRateAdjustedDisplayDifficulty(originalDifficulty, rate); + adjustedDifficulty = Ruleset.Value.CreateInstance().GetRateAdjustedDisplayDifficulty(originalDifficulty, rate); TooltipContent = new AdjustedAttributesTooltip.Data(originalDifficulty, adjustedDifficulty); } } - switch (ruleset.OnlineID) + switch (Ruleset.Value?.OnlineID) { case 3: // Account for mania differences locally for now. // Eventually this should be handled in a more modular way, allowing rulesets to return arbitrary difficulty attributes. - ILegacyRuleset legacyRuleset = (ILegacyRuleset)ruleset.CreateInstance(); + ILegacyRuleset legacyRuleset = (ILegacyRuleset)Ruleset.Value.CreateInstance(); // For the time being, the key count is static no matter what, because: // a) The method doesn't have knowledge of the active keymods. Doing so may require considerations for filtering. @@ -206,7 +196,6 @@ namespace osu.Game.Screens.Select.Details FirstValue.Title = BeatmapsetsStrings.ShowStatsCsMania; FirstValue.Value = (keyCount, keyCount); - break; default: @@ -240,8 +229,8 @@ namespace osu.Game.Screens.Select.Details starDifficultyCancellationSource = new CancellationTokenSource(); - var normalStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, gameRuleset.Value, null, starDifficultyCancellationSource.Token); - var moddedStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, gameRuleset.Value, mods.Value, starDifficultyCancellationSource.Token); + var normalStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, Ruleset.Value, null, starDifficultyCancellationSource.Token); + var moddedStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, Ruleset.Value, mods.Value, starDifficultyCancellationSource.Token); Task.WhenAll(normalStarDifficultyTask, moddedStarDifficultyTask).ContinueWith(_ => Schedule(() => { diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index a603934a9d..15469fad5b 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -286,7 +286,7 @@ namespace osu.Game.Screens.Select AutoSizeAxes = Axes.Y, Anchor = Anchor.Centre, Origin = Anchor.Centre, - Padding = new MarginPadding(10) + Padding = new MarginPadding(10), }, } }, @@ -585,6 +585,11 @@ namespace osu.Game.Screens.Select beatmapInfoPrevious = beatmap; } + // we can't run this in the debounced run due to the selected mods bindable not being debounced, + // since mods could be updated to the new ruleset instances while the decoupled bindable is held behind, + // therefore resulting in performing difficulty calculation with invalid states. + advancedStats.Ruleset.Value = ruleset; + void run() { // clear pending task immediately to track any potential nested debounce operation. From b30a6d522462331e20043f0bada10186a3315196 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sun, 25 Feb 2024 21:23:09 +0300 Subject: [PATCH 03/14] Adjust existing test coverage --- .../SongSelect/TestSceneAdvancedStats.cs | 49 ++----------------- 1 file changed, 5 insertions(+), 44 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneAdvancedStats.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneAdvancedStats.cs index 4bb2b557ff..3b89c70a63 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneAdvancedStats.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneAdvancedStats.cs @@ -37,15 +37,9 @@ namespace osu.Game.Tests.Visual.SongSelect [SetUp] public void Setup() => Schedule(() => Child = advancedStats = new TestAdvancedStats { - Width = 500 + Width = 500, }); - [SetUpSteps] - public void SetUpSteps() - { - AddStep("reset game ruleset", () => Ruleset.Value = new OsuRuleset().RulesetInfo); - } - private BeatmapInfo exampleBeatmapInfo => new BeatmapInfo { Ruleset = rulesets.AvailableRulesets.First(), @@ -74,45 +68,12 @@ namespace osu.Game.Tests.Visual.SongSelect } [Test] - public void TestManiaFirstBarTextManiaBeatmap() + public void TestFirstBarText() { - AddStep("set game ruleset to mania", () => Ruleset.Value = new ManiaRuleset().RulesetInfo); - - AddStep("set beatmap", () => advancedStats.BeatmapInfo = new BeatmapInfo - { - Ruleset = rulesets.GetRuleset(3) ?? throw new InvalidOperationException("osu!mania ruleset not found"), - Difficulty = new BeatmapDifficulty - { - CircleSize = 5, - DrainRate = 4.3f, - OverallDifficulty = 4.5f, - ApproachRate = 3.1f - }, - StarRating = 8 - }); - - AddAssert("first bar text is correct", () => advancedStats.ChildrenOfType().First().Text == BeatmapsetsStrings.ShowStatsCsMania); - } - - [Test] - public void TestManiaFirstBarTextConvert() - { - AddStep("set game ruleset to mania", () => Ruleset.Value = new ManiaRuleset().RulesetInfo); - - AddStep("set beatmap", () => advancedStats.BeatmapInfo = new BeatmapInfo - { - Ruleset = new OsuRuleset().RulesetInfo, - Difficulty = new BeatmapDifficulty - { - CircleSize = 5, - DrainRate = 4.3f, - OverallDifficulty = 4.5f, - ApproachRate = 3.1f - }, - StarRating = 8 - }); - + AddStep("set ruleset to mania", () => advancedStats.Ruleset.Value = new ManiaRuleset().RulesetInfo); AddAssert("first bar text is correct", () => advancedStats.ChildrenOfType().First().Text == BeatmapsetsStrings.ShowStatsCsMania); + AddStep("set ruleset to osu", () => advancedStats.Ruleset.Value = new OsuRuleset().RulesetInfo); + AddAssert("first bar text is correct", () => advancedStats.ChildrenOfType().First().Text == BeatmapsetsStrings.ShowStatsCs); } [Test] From 8e336610d090b2d1f94239620c336a7e0854fc34 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 26 Feb 2024 18:31:36 +0800 Subject: [PATCH 04/14] Add xmldoc explaining `Ruleset` bindable's usage --- osu.Game/Screens/Select/Details/AdvancedStats.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/osu.Game/Screens/Select/Details/AdvancedStats.cs b/osu.Game/Screens/Select/Details/AdvancedStats.cs index 74276795d2..1aba977f44 100644 --- a/osu.Game/Screens/Select/Details/AdvancedStats.cs +++ b/osu.Game/Screens/Select/Details/AdvancedStats.cs @@ -59,6 +59,13 @@ namespace osu.Game.Screens.Select.Details } } + /// + /// Ruleset to be used for certain elements of display. + /// When set, this will override the set 's own ruleset. + /// + /// + /// No checks are done as to whether the ruleset specified is valid for the currently . + /// public Bindable Ruleset { get; } = new Bindable(); public AdvancedStats(int columns = 1) From bbdd85020cc755ee75d8227468bc27c06bb67505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 27 Feb 2024 11:45:03 +0100 Subject: [PATCH 05/14] Fix slider tails sometimes not dimming correctly Originally noticed during review of another change: https://github.com/ppy/osu/pull/27369#issuecomment-1966140198. `DrawableOsuHitObject` tries to solve the initial dimming of objects by applying transform to a list of dimmable parts. For plain drawables this is safe, but if one of the parts is a DHO, it is not safe, because drawable transforms can be cleared at will. In particular, on first use of a drawable slider, `UpdateInitialTransforms()` would fire via `LoadComplete()` on the `DrawableSlider`, but *then*, also via `LoadComplete()`, the `DrawableSliderTail` would update its own state and by doing so inadvertently clear the dim transform just added by the slider. To fix, ensure dim transforms are applied to DHOs via `ApplyCustomUpdateState`. --- .../Objects/Drawables/DrawableOsuHitObject.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index 5271c03e08..35cab6459b 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -80,6 +80,17 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables base.UpdateInitialTransforms(); foreach (var piece in DimmablePieces) + { + // if the specified dimmable piece is a DHO, it is generally not safe to tack transforms onto it directly + // as they may be cleared via the `updateState()` DHO flow, + // so use `ApplyCustomUpdateState` instead. which does not have this pitfall. + if (piece is DrawableHitObject drawableObjectPiece) + drawableObjectPiece.ApplyCustomUpdateState += (dho, _) => applyDim(dho); + else + applyDim(piece); + } + + void applyDim(Drawable piece) { piece.FadeColour(new Color4(195, 195, 195, 255)); using (piece.BeginDelayedSequence(InitialLifetimeOffset - OsuHitWindows.MISS_WINDOW)) From 351160f94e34712103cc7d16d2ee21ee007660a6 Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Tue, 27 Feb 2024 22:41:49 -0800 Subject: [PATCH 06/14] Move back/quit button from bottom left to fail overlay when spectating --- osu.Game/Screens/Play/FailOverlay.cs | 17 +---------------- osu.Game/Screens/Play/GameplayMenuOverlay.cs | 10 ++++++++++ osu.Game/Screens/Play/PauseOverlay.cs | 10 +--------- osu.Game/Screens/Play/Player.cs | 4 ++-- osu.Game/Screens/Play/SpectatorPlayer.cs | 2 -- 5 files changed, 14 insertions(+), 29 deletions(-) diff --git a/osu.Game/Screens/Play/FailOverlay.cs b/osu.Game/Screens/Play/FailOverlay.cs index 210ae5ceb6..f14cdfc213 100644 --- a/osu.Game/Screens/Play/FailOverlay.cs +++ b/osu.Game/Screens/Play/FailOverlay.cs @@ -6,10 +6,8 @@ using System; using System.Threading.Tasks; using osu.Game.Scoring; -using osu.Game.Graphics; using osu.Game.Graphics.UserInterface; using osuTK; -using osuTK.Graphics; using osu.Framework.Allocation; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; @@ -26,22 +24,9 @@ namespace osu.Game.Screens.Play public override LocalisableString Header => GameplayMenuOverlayStrings.FailedHeader; - private readonly bool showButtons; - - public FailOverlay(bool showButtons = true) - { - this.showButtons = showButtons; - } - [BackgroundDependencyLoader] - private void load(OsuColour colours) + private void load() { - if (showButtons) - { - AddButton(GameplayMenuOverlayStrings.Retry, colours.YellowDark, () => OnRetry?.Invoke()); - AddButton(GameplayMenuOverlayStrings.Quit, new Color4(170, 27, 39, 255), () => OnQuit?.Invoke()); - } - // from #10339 maybe this is a better visual effect Add(new Container { diff --git a/osu.Game/Screens/Play/GameplayMenuOverlay.cs b/osu.Game/Screens/Play/GameplayMenuOverlay.cs index 440b8d37b9..be4b8ff9fe 100644 --- a/osu.Game/Screens/Play/GameplayMenuOverlay.cs +++ b/osu.Game/Screens/Play/GameplayMenuOverlay.cs @@ -38,6 +38,7 @@ namespace osu.Game.Screens.Play public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => true; + public Action? OnResume; public Action? OnRetry; public Action? OnQuit; @@ -129,6 +130,15 @@ namespace osu.Game.Screens.Play }, }; + if (OnResume != null) + AddButton(GameplayMenuOverlayStrings.Continue, colours.Green, () => OnResume.Invoke()); + + if (OnRetry != null) + AddButton(GameplayMenuOverlayStrings.Retry, colours.YellowDark, () => OnRetry.Invoke()); + + if (OnQuit != null) + AddButton(GameplayMenuOverlayStrings.Quit, new Color4(170, 27, 39, 255), () => OnQuit.Invoke()); + State.ValueChanged += _ => InternalButtons.Deselect(); updateInfoText(); diff --git a/osu.Game/Screens/Play/PauseOverlay.cs b/osu.Game/Screens/Play/PauseOverlay.cs index 2aa2793fd4..c8e8275641 100644 --- a/osu.Game/Screens/Play/PauseOverlay.cs +++ b/osu.Game/Screens/Play/PauseOverlay.cs @@ -11,18 +11,14 @@ using osu.Framework.Graphics; using osu.Framework.Input.Events; using osu.Framework.Localisation; using osu.Game.Audio; -using osu.Game.Graphics; using osu.Game.Input.Bindings; using osu.Game.Localisation; using osu.Game.Skinning; -using osuTK.Graphics; namespace osu.Game.Screens.Play { public partial class PauseOverlay : GameplayMenuOverlay { - public Action OnResume; - public override bool IsPresent => base.IsPresent || pauseLoop.IsPlaying; public override LocalisableString Header => GameplayMenuOverlayStrings.PausedHeader; @@ -38,12 +34,8 @@ namespace osu.Game.Screens.Play }; [BackgroundDependencyLoader] - private void load(OsuColour colours) + private void load() { - AddButton(GameplayMenuOverlayStrings.Continue, colours.Green, () => OnResume?.Invoke()); - AddButton(GameplayMenuOverlayStrings.Retry, colours.YellowDark, () => OnRetry?.Invoke()); - AddButton(GameplayMenuOverlayStrings.Quit, new Color4(170, 27, 39, 255), () => OnQuit?.Invoke()); - AddInternal(pauseLoop = new SkinnableSound(new SampleInfo("Gameplay/pause-loop")) { Looping = true, diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index 11ff5fdbef..88f6ae9e71 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -278,10 +278,10 @@ namespace osu.Game.Screens.Play createGameplayComponents(Beatmap.Value) } }, - FailOverlay = new FailOverlay(Configuration.AllowUserInteraction) + FailOverlay = new FailOverlay { SaveReplay = async () => await prepareAndImportScoreAsync(true).ConfigureAwait(false), - OnRetry = () => Restart(), + OnRetry = Configuration.AllowUserInteraction ? () => Restart() : null, OnQuit = () => PerformExit(true), }, new HotkeyExitOverlay diff --git a/osu.Game/Screens/Play/SpectatorPlayer.cs b/osu.Game/Screens/Play/SpectatorPlayer.cs index 2faead0ee1..d1404ac184 100644 --- a/osu.Game/Screens/Play/SpectatorPlayer.cs +++ b/osu.Game/Screens/Play/SpectatorPlayer.cs @@ -25,8 +25,6 @@ namespace osu.Game.Screens.Play private readonly Score score; - public override bool AllowBackButton => true; - protected override bool CheckModsAllowFailure() { if (!allowFail) From dee57c7e72275fb87fabacbbc802775418e95185 Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Tue, 27 Feb 2024 22:42:36 -0800 Subject: [PATCH 07/14] Refactor test to only allow init of actions --- .../Gameplay/TestSceneGameplayMenuOverlay.cs | 43 ++++++------------- osu.Game/Screens/Play/GameplayMenuOverlay.cs | 6 +-- 2 files changed, 16 insertions(+), 33 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs index aa5e5985c3..73028e6df9 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs @@ -3,7 +3,6 @@ #nullable disable -using System; using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -26,6 +25,8 @@ namespace osu.Game.Tests.Visual.Gameplay private GlobalActionContainer globalActionContainer; + private bool triggeredRetryButton; + [BackgroundDependencyLoader] private void load(OsuGameBase game) { @@ -35,12 +36,18 @@ namespace osu.Game.Tests.Visual.Gameplay [SetUp] public void SetUp() => Schedule(() => { + triggeredRetryButton = false; + globalActionContainer.Children = new Drawable[] { pauseOverlay = new PauseOverlay { OnResume = () => Logger.Log(@"Resume"), - OnRetry = () => Logger.Log(@"Retry"), + OnRetry = () => + { + Logger.Log(@"Retry"); + triggeredRetryButton = true; + }, OnQuit = () => Logger.Log(@"Quit"), }, failOverlay = new FailOverlay @@ -224,17 +231,9 @@ namespace osu.Game.Tests.Visual.Gameplay { showOverlay(); - bool triggered = false; - AddStep("Click retry button", () => - { - var lastAction = pauseOverlay.OnRetry; - pauseOverlay.OnRetry = () => triggered = true; + AddStep("Click retry button", () => getButton(1).TriggerClick()); - getButton(1).TriggerClick(); - pauseOverlay.OnRetry = lastAction; - }); - - AddAssert("Action was triggered", () => triggered); + AddAssert("Retry was triggered", () => triggeredRetryButton); AddAssert("Overlay is closed", () => pauseOverlay.State.Value == Visibility.Hidden); } @@ -252,25 +251,9 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.Key(Key.Down); }); - bool triggered = false; - Action lastAction = null; - AddStep("Press enter", () => - { - lastAction = pauseOverlay.OnRetry; - pauseOverlay.OnRetry = () => triggered = true; - InputManager.Key(Key.Enter); - }); + AddStep("Press enter", () => InputManager.Key(Key.Enter)); - AddAssert("Action was triggered", () => - { - if (lastAction != null) - { - pauseOverlay.OnRetry = lastAction; - lastAction = null; - } - - return triggered; - }); + AddAssert("Retry was triggered", () => triggeredRetryButton); AddAssert("Overlay is closed", () => pauseOverlay.State.Value == Visibility.Hidden); } diff --git a/osu.Game/Screens/Play/GameplayMenuOverlay.cs b/osu.Game/Screens/Play/GameplayMenuOverlay.cs index be4b8ff9fe..da239d585e 100644 --- a/osu.Game/Screens/Play/GameplayMenuOverlay.cs +++ b/osu.Game/Screens/Play/GameplayMenuOverlay.cs @@ -38,9 +38,9 @@ namespace osu.Game.Screens.Play public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => true; - public Action? OnResume; - public Action? OnRetry; - public Action? OnQuit; + public Action? OnResume { get; init; } + public Action? OnRetry { get; init; } + public Action? OnQuit { get; init; } /// /// Action that is invoked when is triggered. From e8a106177771cc23112f8f42e6cc9adb725736cc Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Tue, 27 Feb 2024 23:16:20 -0800 Subject: [PATCH 08/14] Add test for spectator player exit --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 1c7ede2b19..caf3a05518 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -25,6 +25,7 @@ using osu.Game.Tests.Gameplay; using osu.Game.Tests.Visual.Multiplayer; using osu.Game.Tests.Visual.Spectator; using osuTK; +using osuTK.Input; namespace osu.Game.Tests.Visual.Gameplay { @@ -214,6 +215,9 @@ namespace osu.Game.Tests.Visual.Gameplay checkPaused(false); // Should continue playing until out of frames checkPaused(true); // And eventually stop after running out of frames and fail. // Todo: Should check for + display a failed message. + + AddStep("exit player", () => InputManager.Key(Key.Escape)); + AddAssert("player exited", () => Stack.CurrentScreen is SoloSpectatorScreen); } [Test] From 8e462fbb38e4ffb41f3cf7cf9e7775978d03464b Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Tue, 27 Feb 2024 23:24:04 -0800 Subject: [PATCH 09/14] Apply NRT to touched files --- .../Gameplay/TestSceneGameplayMenuOverlay.cs | 8 +++----- .../Visual/Gameplay/TestSceneSpectator.cs | 20 +++++++++---------- osu.Game/Screens/Play/FailOverlay.cs | 4 +--- osu.Game/Screens/Play/PauseOverlay.cs | 4 +--- .../Screens/Play/SaveFailedScoreButton.cs | 17 ++++++++++------ osu.Game/Screens/Play/SoloSpectatorPlayer.cs | 3 +-- osu.Game/Screens/Play/SpectatorPlayer.cs | 11 ++++------ 7 files changed, 30 insertions(+), 37 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs index 73028e6df9..3501c1a521 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayMenuOverlay.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -20,10 +18,10 @@ namespace osu.Game.Tests.Visual.Gameplay [Description("player pause/fail screens")] public partial class TestSceneGameplayMenuOverlay : OsuManualInputManagerTestScene { - private FailOverlay failOverlay; - private PauseOverlay pauseOverlay; + private FailOverlay failOverlay = null!; + private PauseOverlay pauseOverlay = null!; - private GlobalActionContainer globalActionContainer; + private GlobalActionContainer globalActionContainer = null!; private bool triggeredRetryButton; diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index caf3a05518..c8356cd191 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -40,13 +38,13 @@ namespace osu.Game.Tests.Visual.Gameplay protected override bool UseOnlineAPI => true; [Resolved] - private OsuGameBase game { get; set; } + private OsuGameBase game { get; set; } = null!; private TestSpectatorClient spectatorClient => dependenciesScreen.SpectatorClient; - private DependenciesScreen dependenciesScreen; - private SoloSpectatorScreen spectatorScreen; + private DependenciesScreen dependenciesScreen = null!; + private SoloSpectatorScreen spectatorScreen = null!; - private BeatmapSetInfo importedBeatmap; + private BeatmapSetInfo importedBeatmap = null!; private int importedBeatmapId; [SetUpSteps] @@ -189,7 +187,7 @@ namespace osu.Game.Tests.Visual.Gameplay waitForPlayerCurrent(); - Player lastPlayer = null; + Player lastPlayer = null!; AddStep("store first player", () => lastPlayer = player); start(); @@ -282,7 +280,7 @@ namespace osu.Game.Tests.Visual.Gameplay [Test] public void TestFinalFrameInBundleHasHeader() { - FrameDataBundle lastBundle = null; + FrameDataBundle? lastBundle = null; AddStep("bind to client", () => spectatorClient.OnNewFrames += (_, bundle) => lastBundle = bundle); @@ -291,8 +289,8 @@ namespace osu.Game.Tests.Visual.Gameplay finish(); AddUntilStep("bundle received", () => lastBundle != null); - AddAssert("first frame does not have header", () => lastBundle.Frames[0].Header == null); - AddAssert("last frame has header", () => lastBundle.Frames[^1].Header != null); + AddAssert("first frame does not have header", () => lastBundle?.Frames[0].Header == null); + AddAssert("last frame has header", () => lastBundle?.Frames[^1].Header != null); } [Test] @@ -387,7 +385,7 @@ namespace osu.Game.Tests.Visual.Gameplay } private OsuFramedReplayInputHandler replayHandler => - (OsuFramedReplayInputHandler)Stack.ChildrenOfType().First().ReplayInputHandler; + (OsuFramedReplayInputHandler)Stack.ChildrenOfType().First().ReplayInputHandler!; private Player player => this.ChildrenOfType().Single(); diff --git a/osu.Game/Screens/Play/FailOverlay.cs b/osu.Game/Screens/Play/FailOverlay.cs index f14cdfc213..4a0a6f573c 100644 --- a/osu.Game/Screens/Play/FailOverlay.cs +++ b/osu.Game/Screens/Play/FailOverlay.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Threading.Tasks; using osu.Game.Scoring; @@ -20,7 +18,7 @@ namespace osu.Game.Screens.Play { public partial class FailOverlay : GameplayMenuOverlay { - public Func> SaveReplay; + public Func>? SaveReplay; public override LocalisableString Header => GameplayMenuOverlayStrings.FailedHeader; diff --git a/osu.Game/Screens/Play/PauseOverlay.cs b/osu.Game/Screens/Play/PauseOverlay.cs index c8e8275641..3a471acba4 100644 --- a/osu.Game/Screens/Play/PauseOverlay.cs +++ b/osu.Game/Screens/Play/PauseOverlay.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Linq; using osu.Framework.Allocation; @@ -23,7 +21,7 @@ namespace osu.Game.Screens.Play public override LocalisableString Header => GameplayMenuOverlayStrings.PausedHeader; - private SkinnableSound pauseLoop; + private SkinnableSound pauseLoop = null!; protected override Action BackAction => () => { diff --git a/osu.Game/Screens/Play/SaveFailedScoreButton.cs b/osu.Game/Screens/Play/SaveFailedScoreButton.cs index b97c140250..ef27aac1b9 100644 --- a/osu.Game/Screens/Play/SaveFailedScoreButton.cs +++ b/osu.Game/Screens/Play/SaveFailedScoreButton.cs @@ -30,13 +30,13 @@ namespace osu.Game.Screens.Play private readonly Bindable state = new Bindable(); - private readonly Func> importFailedScore; + private readonly Func>? importFailedScore; private ScoreInfo? importedScore; private DownloadButton button = null!; - public SaveFailedScoreButton(Func> importFailedScore) + public SaveFailedScoreButton(Func>? importFailedScore) { Size = new Vector2(50, 30); @@ -60,11 +60,16 @@ namespace osu.Game.Screens.Play case DownloadState.NotDownloaded: state.Value = DownloadState.Importing; - Task.Run(importFailedScore).ContinueWith(t => + + if (importFailedScore != null) { - importedScore = realm.Run(r => r.Find(t.GetResultSafely().ID)?.Detach()); - Schedule(() => state.Value = importedScore != null ? DownloadState.LocallyAvailable : DownloadState.NotDownloaded); - }).FireAndForget(); + Task.Run(importFailedScore).ContinueWith(t => + { + importedScore = realm.Run(r => r.Find(t.GetResultSafely().ID)?.Detach()); + Schedule(() => state.Value = importedScore != null ? DownloadState.LocallyAvailable : DownloadState.NotDownloaded); + }).FireAndForget(); + } + break; } } diff --git a/osu.Game/Screens/Play/SoloSpectatorPlayer.cs b/osu.Game/Screens/Play/SoloSpectatorPlayer.cs index 8d25a0148d..3dafd5f752 100644 --- a/osu.Game/Screens/Play/SoloSpectatorPlayer.cs +++ b/osu.Game/Screens/Play/SoloSpectatorPlayer.cs @@ -50,8 +50,7 @@ namespace osu.Game.Screens.Play { base.Dispose(isDisposing); - if (SpectatorClient != null) - SpectatorClient.OnUserBeganPlaying -= userBeganPlaying; + SpectatorClient.OnUserBeganPlaying -= userBeganPlaying; } } } diff --git a/osu.Game/Screens/Play/SpectatorPlayer.cs b/osu.Game/Screens/Play/SpectatorPlayer.cs index d1404ac184..520fb43445 100644 --- a/osu.Game/Screens/Play/SpectatorPlayer.cs +++ b/osu.Game/Screens/Play/SpectatorPlayer.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System.Diagnostics; using osu.Framework.Allocation; using osu.Framework.Graphics; @@ -21,7 +19,7 @@ namespace osu.Game.Screens.Play public abstract partial class SpectatorPlayer : Player { [Resolved] - protected SpectatorClient SpectatorClient { get; private set; } + protected SpectatorClient SpectatorClient { get; private set; } = null!; private readonly Score score; @@ -35,7 +33,7 @@ namespace osu.Game.Screens.Play private bool allowFail; - protected SpectatorPlayer(Score score, PlayerConfiguration configuration = null) + protected SpectatorPlayer(Score score, PlayerConfiguration? configuration = null) : base(configuration) { this.score = score; @@ -98,7 +96,7 @@ namespace osu.Game.Screens.Play foreach (var frame in bundle.Frames) { - IConvertibleReplayFrame convertibleFrame = GameplayState.Ruleset.CreateConvertibleReplayFrame(); + IConvertibleReplayFrame convertibleFrame = GameplayState.Ruleset.CreateConvertibleReplayFrame()!; Debug.Assert(convertibleFrame != null); convertibleFrame.FromLegacy(frame, GameplayState.Beatmap); @@ -134,8 +132,7 @@ namespace osu.Game.Screens.Play { base.Dispose(isDisposing); - if (SpectatorClient != null) - SpectatorClient.OnNewFrames -= userSentFrames; + SpectatorClient.OnNewFrames -= userSentFrames; } } } From 6e03384c6bffba55b403c20cb566bf88c7252b12 Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Wed, 28 Feb 2024 00:01:16 -0800 Subject: [PATCH 10/14] Apply NRT to `SoloSpectatorPlayer` --- osu.Game/Screens/Play/SoloSpectatorPlayer.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Screens/Play/SoloSpectatorPlayer.cs b/osu.Game/Screens/Play/SoloSpectatorPlayer.cs index 3dafd5f752..2cc1c4a368 100644 --- a/osu.Game/Screens/Play/SoloSpectatorPlayer.cs +++ b/osu.Game/Screens/Play/SoloSpectatorPlayer.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using osu.Framework.Allocation; using osu.Framework.Screens; using osu.Game.Online.Spectator; From b5ce2642aacc33cd47879d72a18e51714936e11b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 28 Feb 2024 13:20:41 +0100 Subject: [PATCH 11/14] Fix subscribing to `ApplyCustomUpdateState` too much --- .../Objects/Drawables/DrawableOsuHitObject.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index 35cab6459b..5f5deca1ba 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -85,7 +85,12 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables // as they may be cleared via the `updateState()` DHO flow, // so use `ApplyCustomUpdateState` instead. which does not have this pitfall. if (piece is DrawableHitObject drawableObjectPiece) - drawableObjectPiece.ApplyCustomUpdateState += (dho, _) => applyDim(dho); + { + // this method can be called multiple times, and we don't want to subscribe to the event more than once, + // so this is what it is going to have to be... + drawableObjectPiece.ApplyCustomUpdateState -= applyDimToDrawableHitObject; + drawableObjectPiece.ApplyCustomUpdateState += applyDimToDrawableHitObject; + } else applyDim(piece); } @@ -96,6 +101,8 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables using (piece.BeginDelayedSequence(InitialLifetimeOffset - OsuHitWindows.MISS_WINDOW)) piece.FadeColour(Color4.White, 100); } + + void applyDimToDrawableHitObject(DrawableHitObject dho, ArmedState _) => applyDim(dho); } protected sealed override double InitialLifetimeOffset => HitObject.TimePreempt; From c10ba6ece9aea6d4325482563ae64fbc8261a65e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 28 Feb 2024 15:59:22 +0100 Subject: [PATCH 12/14] Fix right mouse scroll clamping not going along well with padding Co-authored-by: Joseph Madamba --- osu.Game/Graphics/Containers/OsuScrollContainer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Graphics/Containers/OsuScrollContainer.cs b/osu.Game/Graphics/Containers/OsuScrollContainer.cs index 124becc35a..ffd28957ef 100644 --- a/osu.Game/Graphics/Containers/OsuScrollContainer.cs +++ b/osu.Game/Graphics/Containers/OsuScrollContainer.cs @@ -127,7 +127,7 @@ namespace osu.Game.Graphics.Containers } protected virtual void ScrollFromMouseEvent(MouseEvent e) => - ScrollTo(Clamp(ToLocalSpace(e.ScreenSpaceMousePosition)[ScrollDim] / DrawSize[ScrollDim]) * Content.DrawSize[ScrollDim], true, DistanceDecayOnRightMouseScrollbar); + ScrollTo(Clamp(ToLocalSpace(e.ScreenSpaceMousePosition)[ScrollDim] / DrawSize[ScrollDim] * Content.DrawSize[ScrollDim]), true, DistanceDecayOnRightMouseScrollbar); protected override ScrollbarContainer CreateScrollbar(Direction direction) => new OsuScrollbar(direction); From 5ca6e8c68a54c0341fc1e0debac5c6e3ebb9739e Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Wed, 28 Feb 2024 11:03:09 -0800 Subject: [PATCH 13/14] Fix some NRT changes --- osu.Game/Screens/Play/SoloSpectatorPlayer.cs | 4 +++- osu.Game/Screens/Play/SpectatorPlayer.cs | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/Play/SoloSpectatorPlayer.cs b/osu.Game/Screens/Play/SoloSpectatorPlayer.cs index 2cc1c4a368..be83a4c6b5 100644 --- a/osu.Game/Screens/Play/SoloSpectatorPlayer.cs +++ b/osu.Game/Screens/Play/SoloSpectatorPlayer.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using osu.Framework.Allocation; +using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Screens; using osu.Game.Online.Spectator; using osu.Game.Scoring; @@ -48,7 +49,8 @@ namespace osu.Game.Screens.Play { base.Dispose(isDisposing); - SpectatorClient.OnUserBeganPlaying -= userBeganPlaying; + if (SpectatorClient.IsNotNull()) + SpectatorClient.OnUserBeganPlaying -= userBeganPlaying; } } } diff --git a/osu.Game/Screens/Play/SpectatorPlayer.cs b/osu.Game/Screens/Play/SpectatorPlayer.cs index 520fb43445..b2ac946642 100644 --- a/osu.Game/Screens/Play/SpectatorPlayer.cs +++ b/osu.Game/Screens/Play/SpectatorPlayer.cs @@ -1,8 +1,8 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System.Diagnostics; using osu.Framework.Allocation; +using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Screens; using osu.Game.Beatmaps; @@ -97,7 +97,6 @@ namespace osu.Game.Screens.Play foreach (var frame in bundle.Frames) { IConvertibleReplayFrame convertibleFrame = GameplayState.Ruleset.CreateConvertibleReplayFrame()!; - Debug.Assert(convertibleFrame != null); convertibleFrame.FromLegacy(frame, GameplayState.Beatmap); var convertedFrame = (ReplayFrame)convertibleFrame; @@ -132,7 +131,8 @@ namespace osu.Game.Screens.Play { base.Dispose(isDisposing); - SpectatorClient.OnNewFrames -= userSentFrames; + if (SpectatorClient.IsNotNull()) + SpectatorClient.OnNewFrames -= userSentFrames; } } } From 232ca5778fc7fb86144f7bb74ee93ee2ac462549 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 29 Feb 2024 00:09:48 +0300 Subject: [PATCH 14/14] Improve spectator fail test --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index c8356cd191..0de2b6a980 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -10,6 +10,7 @@ using osu.Framework.Screens; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Database; +using osu.Game.Localisation; using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Spectator; using osu.Game.Rulesets.Osu; @@ -23,7 +24,6 @@ using osu.Game.Tests.Gameplay; using osu.Game.Tests.Visual.Multiplayer; using osu.Game.Tests.Visual.Spectator; using osuTK; -using osuTK.Input; namespace osu.Game.Tests.Visual.Gameplay { @@ -214,7 +214,9 @@ namespace osu.Game.Tests.Visual.Gameplay checkPaused(true); // And eventually stop after running out of frames and fail. // Todo: Should check for + display a failed message. - AddStep("exit player", () => InputManager.Key(Key.Escape)); + AddAssert("fail overlay present", () => player.ChildrenOfType().Single().IsPresent); + AddAssert("overlay can only quit", () => player.ChildrenOfType().Single().Buttons.Single().Text == GameplayMenuOverlayStrings.Quit); + AddStep("press quit button", () => player.ChildrenOfType().Single().Buttons.Single().TriggerClick()); AddAssert("player exited", () => Stack.CurrentScreen is SoloSpectatorScreen); }