Merge pull request #30377 from bdach/very-bad-completely-no-good-comparer

Fix timeline blueprints sometimes causing crashes due to current placement blueprint becoming unsorted

osu.Game.Rulesets.Taiko.Tests/Editor/TestSceneEditorPlacement.cs

@@ -0,0 +1,37 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Graphics.UserInterface;
+using osu.Framework.Testing;
+using osu.Game.Graphics.UserInterface;
+using osu.Game.Rulesets.Taiko.Objects;
+using osu.Game.Rulesets.Taiko.Objects.Drawables;
+using osu.Game.Tests.Visual;
+using osuTK.Input;
+
+namespace osu.Game.Rulesets.Taiko.Tests.Editor
+{
+    public partial class TestSceneEditorPlacement : EditorTestScene
+    {
+        protected override Ruleset CreateEditorRuleset() => new TaikoRuleset();
+
+        [Test]
+        public void TestPlacementBlueprintDoesNotCauseCrashes()
+        {
+            AddStep("clear objects", () => EditorBeatmap.Clear());
+            AddStep("add two objects", () =>
+            {
+                EditorBeatmap.Add(new Hit { StartTime = 1818 });
+                EditorBeatmap.Add(new Hit { StartTime = 1584 });
+            });
+            AddStep("seek back", () => EditorClock.Seek(1584));
+            AddStep("choose hit placement tool", () => InputManager.Key(Key.Number2));
+            AddStep("hover over first hit", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<DrawableHit>().ElementAt(1)));
+            AddStep("hover over second hit", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<DrawableHit>().ElementAt(0)));
+            AddStep("right click", () => InputManager.Click(MouseButton.Right));
+            AddUntilStep("context menu open", () => Editor.ChildrenOfType<OsuContextMenu>().Any(menu => menu.State == MenuState.Open));
+        }
+    }
+}

osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs

@@ -32,7 +32,12 @@ namespace osu.Game.Screens.Edit.Compose.Components
     {
         protected DragBox DragBox { get; private set; }
 
-        public Container<SelectionBlueprint<T>> SelectionBlueprints { get; private set; }
+        public SelectionBlueprintContainer SelectionBlueprints { get; private set; }
+
+        public partial class SelectionBlueprintContainer : Container<SelectionBlueprint<T>>
+        {
+            public new virtual void ChangeChildDepth(SelectionBlueprint<T> child, float newDepth) => base.ChangeChildDepth(child, newDepth);
+        }
 
         public SelectionHandler<T> SelectionHandler { get; private set; }
 
@@ -95,7 +100,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
             });
         }
 
-        protected virtual Container<SelectionBlueprint<T>> CreateSelectionBlueprintContainer() => new Container<SelectionBlueprint<T>> { RelativeSizeAxes = Axes.Both };
+        protected virtual SelectionBlueprintContainer CreateSelectionBlueprintContainer() => new SelectionBlueprintContainer { RelativeSizeAxes = Axes.Both };
 
         /// <summary>
         /// Creates a <see cref="Components.SelectionHandler{T}"/> which outlines items and handles movement of selections.

osu.Game/Screens/Edit/Compose/Components/EditorBlueprintContainer.cs

@@ -9,7 +9,6 @@ using System.Collections.Specialized;
 using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
 using osu.Framework.Input;
 using osu.Framework.Input.Events;
 using osu.Game.Rulesets.Edit;
@@ -136,7 +135,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
             base.ApplySelectionOrder(blueprints)
                 .OrderBy(b => Math.Min(Math.Abs(EditorClock.CurrentTime - b.Item.GetEndTime()), Math.Abs(EditorClock.CurrentTime - b.Item.StartTime)));
 
-        protected override Container<SelectionBlueprint<HitObject>> CreateSelectionBlueprintContainer() => new HitObjectOrderedSelectionContainer { RelativeSizeAxes = Axes.Both };
+        protected override SelectionBlueprintContainer CreateSelectionBlueprintContainer() => new HitObjectOrderedSelectionContainer { RelativeSizeAxes = Axes.Both };
 
         protected override SelectionHandler<HitObject> CreateSelectionHandler() => new EditorSelectionHandler();
 

osu.Game/Screens/Edit/Compose/Components/HitObjectOrderedSelectionContainer.cs

@@ -4,7 +4,6 @@
 using osu.Framework.Allocation;
 using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
 using osu.Game.Rulesets.Edit;
 using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Objects.Types;
@@ -14,7 +13,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
     /// <summary>
     /// A container for <see cref="SelectionBlueprint{HitObject}"/> ordered by their <see cref="HitObject"/> start times.
     /// </summary>
-    public sealed partial class HitObjectOrderedSelectionContainer : Container<SelectionBlueprint<HitObject>>
+    public sealed partial class HitObjectOrderedSelectionContainer : BlueprintContainer<HitObject>.SelectionBlueprintContainer
     {
         [Resolved]
         private EditorBeatmap editorBeatmap { get; set; } = null!;
@@ -28,16 +27,18 @@ namespace osu.Game.Screens.Edit.Compose.Components
 
         public override void Add(SelectionBlueprint<HitObject> drawable)
         {
-            SortInternal();
+            Sort();
             base.Add(drawable);
         }
 
         public override bool Remove(SelectionBlueprint<HitObject> drawable, bool disposeImmediately)
         {
-            SortInternal();
+            Sort();
             return base.Remove(drawable, disposeImmediately);
         }
 
+        internal void Sort() => SortInternal();
+
         protected override int Compare(Drawable x, Drawable y)
         {
             var xObj = ((SelectionBlueprint<HitObject>)x).Item;

osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineBlueprintContainer.cs

@@ -91,7 +91,7 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline
             }
         }
 
-        protected override Container<SelectionBlueprint<HitObject>> CreateSelectionBlueprintContainer() => new TimelineSelectionBlueprintContainer { RelativeSizeAxes = Axes.Both };
+        protected override SelectionBlueprintContainer CreateSelectionBlueprintContainer() => new TimelineSelectionBlueprintContainer { RelativeSizeAxes = Axes.Both };
 
         protected override bool OnDragStart(DragStartEvent e)
         {
@@ -287,14 +287,27 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline
             }
         }
 
-        protected partial class TimelineSelectionBlueprintContainer : Container<SelectionBlueprint<HitObject>>
+        protected partial class TimelineSelectionBlueprintContainer : SelectionBlueprintContainer
         {
-            protected override Container<SelectionBlueprint<HitObject>> Content { get; }
+            protected override HitObjectOrderedSelectionContainer Content { get; }
 
             public TimelineSelectionBlueprintContainer()
             {
                 AddInternal(new TimelinePart<SelectionBlueprint<HitObject>>(Content = new HitObjectOrderedSelectionContainer { RelativeSizeAxes = Axes.Both }) { RelativeSizeAxes = Axes.Both });
             }
+
+            public override void ChangeChildDepth(SelectionBlueprint<HitObject> child, float newDepth)
+            {
+                // timeline blueprint container also contains a blueprint for current placement, if present
+                // (see `placementChanged()` callback above).
+                // because the current placement hitobject is generally going to be mutated during the placement,
+                // it is possible for `Content`'s children to become unsorted when the user moves the placement around,
+                // which can culminate in a critical failure when attempting to binary-search children here
+                // using `HitObjectOrderedSelectionContainer`'s custom comparer.
+                // thus, always force a re-sort of objects before attempting to change child depth to avoid this scenario.
+                Content.Sort();
+                base.ChangeChildDepth(child, newDepth);
+            }
         }
     }
 }