Support undo/redo for control points in the Editor

osu.Game/Beatmaps/Formats/LegacyDecoder.cs

@@ -143,7 +143,7 @@ protected KeyValuePair<string, string> SplitKeyVal(string line, char separator =
 
         protected string CleanFilename(string path) => path.Trim('"').ToStandardisedPath();
 
-        protected enum Section
+        public enum Section
         {
             General,
             Editor,

osu.Game/Screens/Edit/EditorBeatmap.cs

@@ -71,31 +71,7 @@ public class EditorBeatmap : TransactionalCommitComponent, IBeatmap, IBeatSnapPr
         public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null, BeatmapInfo beatmapInfo = null)
         {
             PlayableBeatmap = playableBeatmap;
-
-            // ensure we are not working with legacy control points.
-            // if we leave the legacy points around they will be applied over any local changes on
-            // ApplyDefaults calls. this should eventually be removed once the default logic is moved to the decoder/converter.
-            if (PlayableBeatmap.ControlPointInfo is LegacyControlPointInfo)
-            {
-                var newControlPoints = new ControlPointInfo();
-
-                foreach (var controlPoint in PlayableBeatmap.ControlPointInfo.AllControlPoints)
-                {
-                    switch (controlPoint)
-                    {
-                        case DifficultyControlPoint _:
-                        case SampleControlPoint _:
-                            // skip legacy types.
-                            continue;
-
-                        default:
-                            newControlPoints.Add(controlPoint.Time, controlPoint);
-                            break;
-                    }
-                }
-
-                playableBeatmap.ControlPointInfo = newControlPoints;
-            }
+            PlayableBeatmap.ControlPointInfo = ConvertControlPoints(PlayableBeatmap.ControlPointInfo);
 
             this.beatmapInfo = beatmapInfo ?? playableBeatmap.BeatmapInfo;
 
@@ -108,6 +84,39 @@ public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null, Beatmap
                 trackStartTime(obj);
         }
 
+        /// <summary>
+        /// Converts a <see cref="ControlPointInfo"/> such that the resultant <see cref="ControlPointInfo"/> is non-legacy.
+        /// </summary>
+        /// <param name="incoming">The <see cref="ControlPointInfo"/> to convert.</param>
+        /// <returns>The non-legacy <see cref="ControlPointInfo"/>. <paramref name="incoming"/> is returned if already non-legacy.</returns>
+        public static ControlPointInfo ConvertControlPoints(ControlPointInfo incoming)
+        {
+            // ensure we are not working with legacy control points.
+            // if we leave the legacy points around they will be applied over any local changes on
+            // ApplyDefaults calls. this should eventually be removed once the default logic is moved to the decoder/converter.
+            if (!(incoming is LegacyControlPointInfo))
+                return incoming;
+
+            var newControlPoints = new ControlPointInfo();
+
+            foreach (var controlPoint in incoming.AllControlPoints)
+            {
+                switch (controlPoint)
+                {
+                    case DifficultyControlPoint _:
+                    case SampleControlPoint _:
+                        // skip legacy types.
+                        continue;
+
+                    default:
+                        newControlPoints.Add(controlPoint.Time, controlPoint);
+                        break;
+                }
+            }
+
+            return newControlPoints;
+        }
+
         public BeatmapInfo BeatmapInfo
         {
             get => beatmapInfo;

osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs

@@ -7,9 +7,11 @@
 using System.IO;
 using System.Text;
 using DiffPlex;
+using DiffPlex.Model;
 using osu.Framework.Audio.Track;
 using osu.Framework.Graphics.Textures;
 using osu.Game.Beatmaps;
+using osu.Game.Beatmaps.Formats;
 using osu.Game.IO;
 using osu.Game.Skinning;
 using Decoder = osu.Game.Beatmaps.Formats.Decoder;
@@ -32,61 +34,99 @@ public void Patch(byte[] currentState, byte[] newState)
         {
             // Diff the beatmaps
             var result = new Differ().CreateLineDiffs(readString(currentState), readString(newState), true, false);
+            IBeatmap newBeatmap = null;
+
+            editorBeatmap.BeginChange();
+            processHitObjects(result, () => newBeatmap ??= readBeatmap(newState));
+            processTimingPoints(result, () => newBeatmap ??= readBeatmap(newState));
+            editorBeatmap.EndChange();
+        }
+
+        private void processTimingPoints(DiffResult result, Func<IBeatmap> getNewBeatmap)
+        {
+            findChangedIndices(result, LegacyDecoder<Beatmap>.Section.TimingPoints, out var removedIndices, out var addedIndices);
+
+            if (removedIndices.Count == 0 && addedIndices.Count == 0)
+                return;
+
+            // Due to conversion from legacy to non-legacy control points, it becomes difficult to diff control points correctly.
+            // So instead _all_ control points are reloaded if _any_ control point is changed.
+
+            var newControlPoints = EditorBeatmap.ConvertControlPoints(getNewBeatmap().ControlPointInfo);
+
+            editorBeatmap.ControlPointInfo.Clear();
+            foreach (var point in newControlPoints.AllControlPoints)
+                editorBeatmap.ControlPointInfo.Add(point.Time, point);
+        }
+
+        private void processHitObjects(DiffResult result, Func<IBeatmap> getNewBeatmap)
+        {
+            findChangedIndices(result, LegacyDecoder<Beatmap>.Section.HitObjects, out var removedIndices, out var addedIndices);
+
+            foreach (int removed in removedIndices)
+                editorBeatmap.RemoveAt(removed);
+
+            if (addedIndices.Count > 0)
+            {
+                var newBeatmap = getNewBeatmap();
+
+                foreach (int i in addedIndices)
+                    editorBeatmap.Insert(i, newBeatmap.HitObjects[i]);
+            }
+        }
+
+        private void findChangedIndices(DiffResult result, LegacyDecoder<Beatmap>.Section section, out List<int> removedIndices, out List<int> addedIndices)
+        {
+            removedIndices = new List<int>();
+            addedIndices = new List<int>();
 
             // Find the index of [HitObject] sections. Lines changed prior to this index are ignored.
-            int oldHitObjectsIndex = Array.IndexOf(result.PiecesOld, "[HitObjects]");
-            int newHitObjectsIndex = Array.IndexOf(result.PiecesNew, "[HitObjects]");
+            int oldSectionStartIndex = Array.IndexOf(result.PiecesOld, $"[{section}]");
+            int oldSectionEndIndex = Array.FindIndex(result.PiecesOld, oldSectionStartIndex + 1, s => s.StartsWith(@"[", StringComparison.Ordinal));
+
+            if (oldSectionEndIndex == -1)
+                oldSectionEndIndex = result.PiecesOld.Length;
+
+            int newSectionStartIndex = Array.IndexOf(result.PiecesNew, $"[{section}]");
+            int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith(@"[", StringComparison.Ordinal));
 
-            Debug.Assert(oldHitObjectsIndex >= 0);
-            Debug.Assert(newHitObjectsIndex >= 0);
+            if (newSectionEndIndex == -1)
+                newSectionEndIndex = result.PiecesOld.Length;
 
-            var toRemove = new List<int>();
-            var toAdd = new List<int>();
+            Debug.Assert(oldSectionStartIndex >= 0);
+            Debug.Assert(newSectionStartIndex >= 0);
 
             foreach (var block in result.DiffBlocks)
             {
-                // Removed hitobjects
+                // Removed indices
                 for (int i = 0; i < block.DeleteCountA; i++)
                 {
-                    int hoIndex = block.DeleteStartA + i - oldHitObjectsIndex - 1;
+                    int objectIndex = block.DeleteStartA + i;
 
-                    if (hoIndex < 0)
+                    if (objectIndex <= oldSectionStartIndex || objectIndex >= oldSectionEndIndex)
                         continue;
 
-                    toRemove.Add(hoIndex);
+                    removedIndices.Add(objectIndex - oldSectionStartIndex - 1);
                 }
 
-                // Added hitobjects
+                // Added indices
                 for (int i = 0; i < block.InsertCountB; i++)
                 {
-                    int hoIndex = block.InsertStartB + i - newHitObjectsIndex - 1;
+                    int objectIndex = block.InsertStartB + i;
 
-                    if (hoIndex < 0)
+                    if (objectIndex <= newSectionStartIndex || objectIndex >= newSectionEndIndex)
                         continue;
 
-                    toAdd.Add(hoIndex);
+                    addedIndices.Add(objectIndex - newSectionStartIndex - 1);
                 }
             }
 
             // Sort the indices to ensure that removal + insertion indices don't get jumbled up post-removal or post-insertion.
             // This isn't strictly required, but the differ makes no guarantees about order.
-            toRemove.Sort();
-            toAdd.Sort();
+            removedIndices.Sort();
+            addedIndices.Sort();
 
-            editorBeatmap.BeginChange();
-
-            // Apply the changes.
-            for (int i = toRemove.Count - 1; i >= 0; i--)
-                editorBeatmap.RemoveAt(toRemove[i]);
-
-            if (toAdd.Count > 0)
-            {
-                IBeatmap newBeatmap = readBeatmap(newState);
-                foreach (int i in toAdd)
-                    editorBeatmap.Insert(i, newBeatmap.HitObjects[i]);
-            }
-
-            editorBeatmap.EndChange();
+            removedIndices.Reverse();
         }
 
         private string readString(byte[] state) => Encoding.UTF8.GetString(state);

osu.Game/Screens/Edit/Timing/TimingScreen.cs

@@ -137,8 +137,12 @@ protected override void LoadComplete()
                 controlPointGroups.BindTo(Beatmap.ControlPointInfo.Groups);
                 controlPointGroups.BindCollectionChanged((sender, args) =>
                 {
-                    table.ControlGroups = controlPointGroups;
-                    changeHandler?.SaveState();
+                    // This AddOnce() works around performance issues from the LegacyEditorBeatmapPatcher re-initialising all control points every undo & redo.
+                    Scheduler.AddOnce(() =>
+                    {
+                        table.ControlGroups = controlPointGroups;
+                        changeHandler?.SaveState();
+                    });
                 }, true);
             }