Merge branch 'master' into beatmap-track-rework

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))
+            using (BeginAbsoluteSequence(spinner.StartTime - spinner.TimeFadeIn / 2, true))
-                this.FadeInFromZero(spinner.TimePreempt / 2);
+                this.FadeInFromZero(spinner.TimeFadeIn / 2);
 
             fixedMiddle.FadeColour(Color4.White);
             using (BeginAbsoluteSequence(spinner.StartTime, true))

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))
+            using (BeginAbsoluteSequence(spinner.StartTime - spinner.TimeFadeIn / 2, true))
-                this.FadeInFromZero(spinner.TimePreempt / 2);
+                this.FadeInFromZero(spinner.TimeFadeIn / 2);
         }
 
         protected override void Update()

osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs

@@ -46,25 +46,37 @@ namespace osu.Game.Tests.Visual.Gameplay
         /// </summary>
         /// <param name="interactive">If the test player should behave like the production one.</param>
         /// <param name="beforeLoadAction">An action to run before player load but after bindable leases are returned.</param>
-        /// <param name="afterLoadAction">An action to run after container load.</param>
+        public void ResetPlayer(bool interactive, Action beforeLoadAction = null)
-        public void ResetPlayer(bool interactive, Action beforeLoadAction = null, Action afterLoadAction = null)
         {
+            player = null;
+
             audioManager.Volume.SetDefault();
 
             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<IApplicableToTrack>())
                 mod.ApplyToTrack(MusicController.CurrentTrack);
 
-            InputManager.Child = container = new TestPlayerLoaderContainer(
+            InputManager.Child = container;
-                loader = new TestPlayerLoader(() =>
+        }
-                {
+
-                    afterLoadAction?.Invoke();
+        [Test]
-                    return player = new TestPlayer(interactive, interactive);
+        public void TestEarlyExitBeforePlayerConstruction()
-                }));
+        {
+            AddStep("load dummy beatmap", () => ResetPlayer(false, () => SelectedMods.Value = new[] { new OsuModNightcore() }));
+            AddUntilStep("wait for current", () => loader.IsCurrentScreen());
+            AddAssert("mod rate applied", () => MusicController.CurrentTrack.Rate != 1);
+            AddStep("exit loader", () => loader.Exit());
+            AddUntilStep("wait for not current", () => !loader.IsCurrentScreen());
+            AddAssert("player did not load", () => player == null);
+            AddUntilStep("player disposed", () => loader.DisposalTask == null);
+            AddAssert("mod rate still applied", () => MusicController.CurrentTrack.Rate != 1);
         }
 
         /// <summary>
@@ -73,11 +85,12 @@ namespace osu.Game.Tests.Visual.Gameplay
         /// speed adjustments were undone too late, causing cross-screen pollution.
         /// </summary>
         [Test]
-        public void TestEarlyExit()
+        public void TestEarlyExitAfterPlayerConstruction()
         {
             AddStep("load dummy beatmap", () => ResetPlayer(false, () => SelectedMods.Value = new[] { new OsuModNightcore() }));
             AddUntilStep("wait for current", () => loader.IsCurrentScreen());
             AddAssert("mod rate applied", () => MusicController.CurrentTrack.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);
@@ -94,7 +107,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);
 
@@ -195,19 +208,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);
         }
 
         /// <remarks>
@@ -215,14 +228,13 @@ namespace osu.Game.Tests.Visual.Gameplay
         /// </remarks>
         /// <param name="volumeName">What part of the volume system is checked</param>
         /// <param name="beforeLoad">The action to be invoked to set the volume before loading</param>
-        /// <param name="afterLoad">The action to be invoked to set the volume after loading</param>
         /// <param name="assert">The function to be invoked and checked</param>
-        private void addVolumeSteps(string volumeName, Action beforeLoad, Action afterLoad, Func<bool> assert)
+        private void addVolumeSteps(string volumeName, Action beforeLoad, Func<bool> assert)
         {
             AddStep("reset notification lock", () => sessionStatics.GetBindable<bool>(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);
+            AddUntilStep("wait for player", () => player?.LoadState == LoadState.Ready);
 
             AddAssert("check for notification", () => container.NotificationOverlay.UnreadCount.Value == 1);
             AddStep("click notification", () =>

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);
         }
 
+        /// <summary>
+        /// Tests that the same <see cref="Mod"/> instances are not shared between two playlist items.
+        /// </summary>
+        [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;

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", () =>

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
         /// <summary>
         /// Creates a copy of this <see cref="Mod"/> initialised to a default state.
         /// </summary>
-        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<T>.Value property.
+                var valueProperty = origBindable.GetType().GetProperty(nameof(Bindable<object>.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();
     }

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.
@@ -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()

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();
         }
     }
 }