Merge pull request #8914 from smoogipoo/fix-overlapping-object-undoredo

Fix undo/redo behaving poorly with simultaneous objects

osu.Game.Tests/Editing/LegacyEditorBeatmapPatcherTest.cs

@@ -304,6 +304,31 @@ namespace osu.Game.Tests.Editing
             runTest(patch);
         }
 
+        [Test]
+        public void TestChangeHitObjectAtSameTime()
+        {
+            current.AddRange(new[]
+            {
+                new HitCircle { StartTime = 500, Position = new Vector2(50) },
+                new HitCircle { StartTime = 500, Position = new Vector2(100) },
+                new HitCircle { StartTime = 500, Position = new Vector2(150) },
+                new HitCircle { StartTime = 500, Position = new Vector2(200) },
+            });
+
+            var patch = new OsuBeatmap
+            {
+                HitObjects =
+                {
+                    new HitCircle { StartTime = 500, Position = new Vector2(150) },
+                    new HitCircle { StartTime = 500, Position = new Vector2(100) },
+                    new HitCircle { StartTime = 500, Position = new Vector2(50) },
+                    new HitCircle { StartTime = 500, Position = new Vector2(200) },
+                }
+            };
+
+            runTest(patch);
+        }
+
         private void runTest(IBeatmap patch)
         {
             // Due to the method of testing, "patch" comes in without having been decoded via a beatmap decoder.

osu.Game/Screens/Edit/EditorBeatmap.cs

@@ -136,14 +136,26 @@ namespace osu.Game.Screens.Edit
         /// <param name="hitObject">The <see cref="HitObject"/> to add.</param>
         public void Add(HitObject hitObject)
         {
-            trackStartTime(hitObject);
-
             // Preserve existing sorting order in the beatmap
             var insertionIndex = findInsertionIndex(PlayableBeatmap.HitObjects, hitObject.StartTime);
-            mutableHitObjects.Insert(insertionIndex + 1, hitObject);
+            Insert(insertionIndex + 1, hitObject);
+        }
+
+        /// <summary>
+        /// Inserts a <see cref="HitObject"/> into this <see cref="EditorBeatmap"/>.
+        /// </summary>
+        /// <remarks>
+        /// It is the invoker's responsibility to make sure that <see cref="HitObject"/> sorting order is maintained.
+        /// </remarks>
+        /// <param name="index">The index to insert the <see cref="HitObject"/> at.</param>
+        /// <param name="hitObject">The <see cref="HitObject"/> to insert.</param>
+        public void Insert(int index, HitObject hitObject)
+        {
+            trackStartTime(hitObject);
+
+            mutableHitObjects.Insert(index, hitObject);
 
             HitObjectAdded?.Invoke(hitObject);
-
             updateHitObject(hitObject, true);
         }
 

osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs

@@ -63,8 +63,10 @@ namespace osu.Game.Screens.Edit
                 }
             }
 
-            // Make the removal indices are sorted so that iteration order doesn't get messed up post-removal.
+            // 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();
 
             // Apply the changes.
             for (int i = toRemove.Count - 1; i >= 0; i--)
@@ -74,7 +76,7 @@ namespace osu.Game.Screens.Edit
             {
                 IBeatmap newBeatmap = readBeatmap(newState);
                 foreach (var i in toAdd)
-                    editorBeatmap.Add(newBeatmap.HitObjects[i]);
+                    editorBeatmap.Insert(i, newBeatmap.HitObjects[i]);
             }
         }