Merge pull request #9843 from smoogipoo/fix-playlist-mods

osu.Game.Tests/Visual/Multiplayer/TestSceneMatchSongSelect.cs

@@ -16,7 +16,9 @@
 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 @@ public void TestAddItemAfterRearrangement()
             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 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.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 @@ public void TestCustomisationMenuVisibility()
             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 @@ public virtual string SettingDescription
         /// <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/Select/MatchSongSelect.cs

@@ -77,6 +77,8 @@ private void populateItemFromCurrent(PlaylistItem item)
 
             item.RequiredMods.Clear();
             item.RequiredMods.AddRange(Mods.Value);
+
+            Mods.Value = Mods.Value.Select(m => m.CreateCopy()).ToArray();
         }
     }
 }