From 27cd9e119aa27be6ce39ee7989f30c5ead5437db Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 13 Aug 2020 12:04:32 +0900 Subject: [PATCH 1/8] Delay beatmap load until after transition has finished Previously the beatmap would begin loading at the same time the `PlayerLoader` class was. This can cause a horribly visible series of stutters, especially when a storyboard is involved. Obviously we should be aiming to reduce the stutters via changes to the beatmap load process (such as incremental storyboard loading, `DrawableHitObject` pooling, etc.) but this improves user experience tenfold in the mean time. --- osu.Game/Screens/Play/PlayerLoader.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/osu.Game/Screens/Play/PlayerLoader.cs b/osu.Game/Screens/Play/PlayerLoader.cs index 93a734589c..d32fae1b90 100644 --- a/osu.Game/Screens/Play/PlayerLoader.cs +++ b/osu.Game/Screens/Play/PlayerLoader.cs @@ -153,8 +153,6 @@ namespace osu.Game.Screens.Play { base.OnEntering(last); - prepareNewPlayer(); - content.ScaleTo(0.7f); Background?.FadeColour(Color4.White, 800, Easing.OutQuint); @@ -172,11 +170,6 @@ namespace osu.Game.Screens.Play contentIn(); - MetadataInfo.Loading = true; - - // we will only be resumed if the player has requested a re-run (see restartRequested). - prepareNewPlayer(); - this.Delay(400).Schedule(pushWhenLoaded); } @@ -257,6 +250,9 @@ namespace osu.Game.Screens.Play private void prepareNewPlayer() { + if (!this.IsCurrentScreen()) + return; + var restartCount = player?.RestartCount + 1 ?? 0; player = createPlayer(); @@ -274,8 +270,10 @@ namespace osu.Game.Screens.Play private void contentIn() { - content.ScaleTo(1, 650, Easing.OutQuint); + MetadataInfo.Loading = true; + content.FadeInFromZero(400); + content.ScaleTo(1, 650, Easing.OutQuint).Then().Schedule(prepareNewPlayer); } private void contentOut() From 99bea6b8e9e5d5a586179c385d7a14c246c522f4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 13 Aug 2020 12:52:35 +0900 Subject: [PATCH 2/8] Add missing null check (player construction is potentially delayed now) --- osu.Game/Screens/Play/PlayerLoader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Play/PlayerLoader.cs b/osu.Game/Screens/Play/PlayerLoader.cs index d32fae1b90..dcf84a8821 100644 --- a/osu.Game/Screens/Play/PlayerLoader.cs +++ b/osu.Game/Screens/Play/PlayerLoader.cs @@ -68,7 +68,7 @@ namespace osu.Game.Screens.Play private bool readyForPush => // don't push unless the player is completely loaded - player.LoadState == LoadState.Ready + player?.LoadState == LoadState.Ready // don't push if the user is hovering one of the panes, unless they are idle. && (IsHovered || idleTracker.IsIdle.Value) // don't push if the user is dragging a slider or otherwise. From 5b536aebe71a49f2c4eab4edd508985d00bdcdf3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 13 Aug 2020 12:53:37 +0900 Subject: [PATCH 3/8] Add missing null checks and avoid cross-test pollution --- osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs index 4c73065087..c34b523c97 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs @@ -49,6 +49,8 @@ namespace osu.Game.Tests.Visual.Gameplay /// An action to run after container load. public void ResetPlayer(bool interactive, Action beforeLoadAction = null, Action afterLoadAction = null) { + player = null; + audioManager.Volume.SetDefault(); InputManager.Clear(); @@ -80,7 +82,7 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("mod rate applied", () => Beatmap.Value.Track.Rate != 1); AddStep("exit loader", () => loader.Exit()); AddUntilStep("wait for not current", () => !loader.IsCurrentScreen()); - AddAssert("player did not load", () => !player.IsLoaded); + AddAssert("player did not load", () => player?.IsLoaded != true); AddUntilStep("player disposed", () => loader.DisposalTask?.IsCompleted == true); AddAssert("mod rate still applied", () => Beatmap.Value.Track.Rate != 1); } @@ -94,7 +96,7 @@ namespace osu.Game.Tests.Visual.Gameplay AddUntilStep("wait for load ready", () => { moveMouse(); - return player.LoadState == LoadState.Ready; + return player?.LoadState == LoadState.Ready; }); AddRepeatStep("move mouse", moveMouse, 20); @@ -222,7 +224,7 @@ namespace osu.Game.Tests.Visual.Gameplay AddStep("reset notification lock", () => sessionStatics.GetBindable(Static.MutedAudioNotificationShownOnce).Value = false); AddStep("load player", () => ResetPlayer(false, beforeLoad, afterLoad)); - AddUntilStep("wait for player", () => player.LoadState == LoadState.Ready); + AddUntilStep("wait for player", () => player?.LoadState == LoadState.Ready); AddAssert("check for notification", () => container.NotificationOverlay.UnreadCount.Value == 1); AddStep("click notification", () => From fd7bf70b7d4302c1536a6b0e1489ac0c9ff5a1ff Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 13 Aug 2020 12:59:00 +0900 Subject: [PATCH 4/8] Remove weird "after load" action This was pretty pointless anyway and from its usages, doesn't look to need to exist. --- .../Visual/Gameplay/TestScenePlayerLoader.cs | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs index c34b523c97..d6742a27c2 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs @@ -46,8 +46,7 @@ namespace osu.Game.Tests.Visual.Gameplay /// /// If the test player should behave like the production one. /// An action to run before player load but after bindable leases are returned. - /// An action to run after container load. - public void ResetPlayer(bool interactive, Action beforeLoadAction = null, Action afterLoadAction = null) + public void ResetPlayer(bool interactive, Action beforeLoadAction = null) { player = null; @@ -55,18 +54,16 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.Clear(); + container = new TestPlayerLoaderContainer(loader = new TestPlayerLoader(() => player = new TestPlayer(interactive, interactive))); + beforeLoadAction?.Invoke(); + Beatmap.Value = CreateWorkingBeatmap(new OsuRuleset().RulesetInfo); foreach (var mod in SelectedMods.Value.OfType()) mod.ApplyToTrack(Beatmap.Value.Track); - InputManager.Child = container = new TestPlayerLoaderContainer( - loader = new TestPlayerLoader(() => - { - afterLoadAction?.Invoke(); - return player = new TestPlayer(interactive, interactive); - })); + InputManager.Child = container; } /// @@ -197,19 +194,19 @@ namespace osu.Game.Tests.Visual.Gameplay [Test] public void TestMutedNotificationMasterVolume() { - addVolumeSteps("master volume", () => audioManager.Volume.Value = 0, null, () => audioManager.Volume.IsDefault); + addVolumeSteps("master volume", () => audioManager.Volume.Value = 0, () => audioManager.Volume.IsDefault); } [Test] public void TestMutedNotificationTrackVolume() { - addVolumeSteps("music volume", () => audioManager.VolumeTrack.Value = 0, null, () => audioManager.VolumeTrack.IsDefault); + addVolumeSteps("music volume", () => audioManager.VolumeTrack.Value = 0, () => audioManager.VolumeTrack.IsDefault); } [Test] public void TestMutedNotificationMuteButton() { - addVolumeSteps("mute button", null, () => container.VolumeOverlay.IsMuted.Value = true, () => !container.VolumeOverlay.IsMuted.Value); + addVolumeSteps("mute button", () => container.VolumeOverlay.IsMuted.Value = true, () => !container.VolumeOverlay.IsMuted.Value); } /// @@ -217,13 +214,12 @@ namespace osu.Game.Tests.Visual.Gameplay /// /// What part of the volume system is checked /// The action to be invoked to set the volume before loading - /// The action to be invoked to set the volume after loading /// The function to be invoked and checked - private void addVolumeSteps(string volumeName, Action beforeLoad, Action afterLoad, Func assert) + private void addVolumeSteps(string volumeName, Action beforeLoad, Func assert) { AddStep("reset notification lock", () => sessionStatics.GetBindable(Static.MutedAudioNotificationShownOnce).Value = false); - AddStep("load player", () => ResetPlayer(false, beforeLoad, afterLoad)); + AddStep("load player", () => ResetPlayer(false, beforeLoad)); AddUntilStep("wait for player", () => player?.LoadState == LoadState.Ready); AddAssert("check for notification", () => container.NotificationOverlay.UnreadCount.Value == 1); From cf9bda6c199bddcbf957033191285814c531b04a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 13 Aug 2020 13:05:00 +0900 Subject: [PATCH 5/8] Add coverage of early exit with null and non-null player --- .../Visual/Gameplay/TestScenePlayerLoader.cs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs index d6742a27c2..e698d31176 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs @@ -66,20 +66,34 @@ namespace osu.Game.Tests.Visual.Gameplay InputManager.Child = container; } - /// - /// When exits early, it has to wait for the player load task - /// to complete before running disposal on player. This previously caused an issue where mod - /// speed adjustments were undone too late, causing cross-screen pollution. - /// [Test] - public void TestEarlyExit() + public void TestEarlyExitBeforePlayerConstruction() { AddStep("load dummy beatmap", () => ResetPlayer(false, () => SelectedMods.Value = new[] { new OsuModNightcore() })); AddUntilStep("wait for current", () => loader.IsCurrentScreen()); AddAssert("mod rate applied", () => Beatmap.Value.Track.Rate != 1); AddStep("exit loader", () => loader.Exit()); AddUntilStep("wait for not current", () => !loader.IsCurrentScreen()); - AddAssert("player did not load", () => player?.IsLoaded != true); + AddAssert("player did not load", () => player == null); + AddUntilStep("player disposed", () => loader.DisposalTask == null); + AddAssert("mod rate still applied", () => Beatmap.Value.Track.Rate != 1); + } + + /// + /// When exits early, it has to wait for the player load task + /// to complete before running disposal on player. This previously caused an issue where mod + /// speed adjustments were undone too late, causing cross-screen pollution. + /// + [Test] + public void TestEarlyExitAfterPlayerConstruction() + { + AddStep("load dummy beatmap", () => ResetPlayer(false, () => SelectedMods.Value = new[] { new OsuModNightcore() })); + AddUntilStep("wait for current", () => loader.IsCurrentScreen()); + AddAssert("mod rate applied", () => Beatmap.Value.Track.Rate != 1); + AddUntilStep("wait for non-null player", () => player != null); + AddStep("exit loader", () => loader.Exit()); + AddUntilStep("wait for not current", () => !loader.IsCurrentScreen()); + AddAssert("player did not load", () => !player.IsLoaded); AddUntilStep("player disposed", () => loader.DisposalTask?.IsCompleted == true); AddAssert("mod rate still applied", () => Beatmap.Value.Track.Rate != 1); } From 662281d727561b70572f76d40174a1bc75d67604 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 13 Aug 2020 18:20:45 +0900 Subject: [PATCH 6/8] Adjust legacy spinners to fade in later Matches stable 1:1 for legacy skins. I've left lazer default as it is because changing to use the shorter apperance looks bad. This will probably change as we proceed with the redesign of the default skin. --- osu.Game.Rulesets.Osu/Skinning/LegacyNewStyleSpinner.cs | 4 ++-- osu.Game.Rulesets.Osu/Skinning/LegacyOldStyleSpinner.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/LegacyNewStyleSpinner.cs b/osu.Game.Rulesets.Osu/Skinning/LegacyNewStyleSpinner.cs index 72bc3ddc9a..739c87e037 100644 --- a/osu.Game.Rulesets.Osu/Skinning/LegacyNewStyleSpinner.cs +++ b/osu.Game.Rulesets.Osu/Skinning/LegacyNewStyleSpinner.cs @@ -79,8 +79,8 @@ namespace osu.Game.Rulesets.Osu.Skinning { var spinner = (Spinner)drawableSpinner.HitObject; - using (BeginAbsoluteSequence(spinner.StartTime - spinner.TimePreempt / 2, true)) - this.FadeInFromZero(spinner.TimePreempt / 2); + using (BeginAbsoluteSequence(spinner.StartTime - spinner.TimeFadeIn / 2, true)) + this.FadeInFromZero(spinner.TimeFadeIn / 2); fixedMiddle.FadeColour(Color4.White); using (BeginAbsoluteSequence(spinner.StartTime, true)) diff --git a/osu.Game.Rulesets.Osu/Skinning/LegacyOldStyleSpinner.cs b/osu.Game.Rulesets.Osu/Skinning/LegacyOldStyleSpinner.cs index 0ae1d8f683..81a0df5ea5 100644 --- a/osu.Game.Rulesets.Osu/Skinning/LegacyOldStyleSpinner.cs +++ b/osu.Game.Rulesets.Osu/Skinning/LegacyOldStyleSpinner.cs @@ -85,8 +85,8 @@ namespace osu.Game.Rulesets.Osu.Skinning { var spinner = drawableSpinner.HitObject; - using (BeginAbsoluteSequence(spinner.StartTime - spinner.TimePreempt / 2, true)) - this.FadeInFromZero(spinner.TimePreempt / 2); + using (BeginAbsoluteSequence(spinner.StartTime - spinner.TimeFadeIn / 2, true)) + this.FadeInFromZero(spinner.TimeFadeIn / 2); } protected override void Update() From 3cb22fad82d6d1f3b0bc07f8bb025acabb090cd5 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Thu, 13 Aug 2020 19:48:31 +0900 Subject: [PATCH 7/8] Fix mods sharing bindable instances --- .../UserInterface/TestSceneModSettings.cs | 20 ++++++++++++++++++ osu.Game/Rulesets/Mods/Mod.cs | 21 ++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSettings.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSettings.cs index 7ff463361a..c5ce3751ef 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSettings.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSettings.cs @@ -8,6 +8,7 @@ using NUnit.Framework; using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; +using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Configuration; using osu.Game.Graphics.UserInterface; @@ -15,6 +16,7 @@ using osu.Game.Overlays.Mods; using osu.Game.Rulesets; using osu.Game.Rulesets.Difficulty; using osu.Game.Rulesets.Mods; +using osu.Game.Rulesets.Osu.Mods; using osu.Game.Rulesets.UI; namespace osu.Game.Tests.Visual.UserInterface @@ -75,6 +77,24 @@ namespace osu.Game.Tests.Visual.UserInterface AddAssert("Customisation closed", () => modSelect.ModSettingsContainer.Alpha == 0); } + [Test] + public void TestModSettingsUnboundWhenCopied() + { + OsuModDoubleTime original = null; + OsuModDoubleTime copy = null; + + AddStep("create mods", () => + { + original = new OsuModDoubleTime(); + copy = (OsuModDoubleTime)original.CreateCopy(); + }); + + AddStep("change property", () => original.SpeedChange.Value = 2); + + AddAssert("original has new value", () => Precision.AlmostEquals(2.0, original.SpeedChange.Value)); + AddAssert("copy has original value", () => Precision.AlmostEquals(1.5, copy.SpeedChange.Value)); + } + private void createModSelect() { AddStep("create mod select", () => diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 0e5fe3fc9c..52ffa0ad2a 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Reflection; using Newtonsoft.Json; @@ -126,7 +127,25 @@ namespace osu.Game.Rulesets.Mods /// /// Creates a copy of this initialised to a default state. /// - public virtual Mod CreateCopy() => (Mod)MemberwiseClone(); + public virtual Mod CreateCopy() + { + var copy = (Mod)Activator.CreateInstance(GetType()); + + // Copy bindable values across + foreach (var (_, prop) in this.GetSettingsSourceProperties()) + { + var origBindable = prop.GetValue(this); + var copyBindable = prop.GetValue(copy); + + // The bindables themselves are readonly, so the value must be transferred through the Bindable.Value property. + var valueProperty = origBindable.GetType().GetProperty(nameof(Bindable.Value), BindingFlags.Public | BindingFlags.Instance); + Debug.Assert(valueProperty != null); + + valueProperty.SetValue(copyBindable, valueProperty.GetValue(origBindable)); + } + + return copy; + } public bool Equals(IMod other) => GetType() == other?.GetType(); } From 0500d82b5bed73153b1bcee54374c557a4408ab4 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Thu, 13 Aug 2020 19:48:41 +0900 Subject: [PATCH 8/8] Fix playlist items sharing mod instances --- .../Multiplayer/TestSceneMatchSongSelect.cs | 17 +++++++++++++++++ osu.Game/Screens/Select/MatchSongSelect.cs | 2 ++ 2 files changed, 19 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMatchSongSelect.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMatchSongSelect.cs index c62479faa0..3d225aa0a9 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMatchSongSelect.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMatchSongSelect.cs @@ -16,7 +16,9 @@ using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Online.Multiplayer; using osu.Game.Rulesets; +using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Osu; +using osu.Game.Rulesets.Osu.Mods; using osu.Game.Screens.Multi.Components; using osu.Game.Screens.Select; @@ -145,6 +147,21 @@ namespace osu.Game.Tests.Visual.Multiplayer AddAssert("new item has id 2", () => Room.Playlist.Last().ID == 2); } + /// + /// Tests that the same instances are not shared between two playlist items. + /// + [Test] + public void TestNewItemHasNewModInstances() + { + AddStep("set dt mod", () => SelectedMods.Value = new[] { new OsuModDoubleTime() }); + AddStep("create item", () => songSelect.BeatmapDetails.CreateNewItem()); + AddStep("change mod rate", () => ((OsuModDoubleTime)SelectedMods.Value[0]).SpeedChange.Value = 2); + AddStep("create item", () => songSelect.BeatmapDetails.CreateNewItem()); + + AddAssert("item 1 has rate 1.5", () => Precision.AlmostEquals(1.5, ((OsuModDoubleTime)Room.Playlist.First().RequiredMods[0]).SpeedChange.Value)); + AddAssert("item 2 has rate 2", () => Precision.AlmostEquals(2, ((OsuModDoubleTime)Room.Playlist.Last().RequiredMods[0]).SpeedChange.Value)); + } + private class TestMatchSongSelect : MatchSongSelect { public new MatchBeatmapDetailArea BeatmapDetails => (MatchBeatmapDetailArea)base.BeatmapDetails; diff --git a/osu.Game/Screens/Select/MatchSongSelect.cs b/osu.Game/Screens/Select/MatchSongSelect.cs index 2f3674642e..96a48fa3ac 100644 --- a/osu.Game/Screens/Select/MatchSongSelect.cs +++ b/osu.Game/Screens/Select/MatchSongSelect.cs @@ -77,6 +77,8 @@ namespace osu.Game.Screens.Select item.RequiredMods.Clear(); item.RequiredMods.AddRange(Mods.Value); + + Mods.Value = Mods.Value.Select(m => m.CreateCopy()).ToArray(); } } }