Merge pull request #13499 from ekrctb/fix-mania-editor-crash

Rewrite mania selection blueprint to use only `HitObject` (no `DrawableHitObject`) for layout

osu.Game.Rulesets.Mania.Tests/Editor/ManiaSelectionBlueprintTestScene.cs

@@ -1,31 +1,54 @@
 // 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.Collections.Generic;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
-using osu.Framework.Timing;
+using osu.Framework.Graphics.Containers;
+using osu.Game.Rulesets.Mania.Beatmaps;
 using osu.Game.Rulesets.Mania.UI;
+using osu.Game.Rulesets.UI;
+using osu.Game.Rulesets.UI.Scrolling;
 using osu.Game.Tests.Visual;
-using osuTK.Graphics;
 
 namespace osu.Game.Rulesets.Mania.Tests.Editor
 {
     public abstract class ManiaSelectionBlueprintTestScene : SelectionBlueprintTestScene
     {
-        [Cached(Type = typeof(IAdjustableClock))]
+        protected override Container<Drawable> Content => blueprints ?? base.Content;
-        private readonly IAdjustableClock clock = new StopwatchClock();
 
-        protected ManiaSelectionBlueprintTestScene()
+        private readonly Container blueprints;
+
+        [Cached(typeof(Playfield))]
+        public Playfield Playfield { get; }
+
+        private readonly ScrollingTestContainer scrollingTestContainer;
+
+        protected ScrollingDirection Direction
         {
-            Add(new Column(0)
+            set => scrollingTestContainer.Direction = value;
-            {
-                Anchor = Anchor.Centre,
-                Origin = Anchor.Centre,
-                AccentColour = Color4.OrangeRed,
-                Clock = new FramedClock(new StopwatchClock()), // No scroll
-            });
         }
 
-        public ManiaPlayfield Playfield => null;
+        protected ManiaSelectionBlueprintTestScene(int columns)
+        {
+            var stageDefinitions = new List<StageDefinition> { new StageDefinition { Columns = columns } };
+            base.Content.Child = scrollingTestContainer = new ScrollingTestContainer(ScrollingDirection.Up)
+            {
+                RelativeSizeAxes = Axes.Both,
+                Children = new Drawable[]
+                {
+                    Playfield = new ManiaPlayfield(stageDefinitions)
+                    {
+                        RelativeSizeAxes = Axes.Both,
+                    },
+                    blueprints = new Container
+                    {
+                        RelativeSizeAxes = Axes.Both
+                    }
+                }
+            };
+
+            AddToggleStep("Downward scroll", b => Direction = b ? ScrollingDirection.Down : ScrollingDirection.Up);
+        }
     }
 }

osu.Game.Rulesets.Mania.Tests/Editor/TestSceneHoldNoteSelectionBlueprint.cs

@@ -1,55 +1,32 @@
 // 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 osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
-using osu.Game.Graphics;
 using osu.Game.Rulesets.Mania.Edit.Blueprints;
 using osu.Game.Rulesets.Mania.Objects;
 using osu.Game.Rulesets.Mania.Objects.Drawables;
-using osu.Game.Rulesets.UI.Scrolling;
-using osu.Game.Tests.Visual;
 
 namespace osu.Game.Rulesets.Mania.Tests.Editor
 {
     public class TestSceneHoldNoteSelectionBlueprint : ManiaSelectionBlueprintTestScene
     {
-        private readonly DrawableHoldNote drawableObject;
-
-        protected override Container<Drawable> Content => content ?? base.Content;
-        private readonly Container content;
-
         public TestSceneHoldNoteSelectionBlueprint()
+            : base(4)
         {
-            var holdNote = new HoldNote { Column = 0, Duration = 1000 };
+            for (int i = 0; i < 4; i++)
-            holdNote.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty());
-
-            base.Content.Child = content = new ScrollingTestContainer(ScrollingDirection.Down)
             {
-                Anchor = Anchor.Centre,
+                var holdNote = new HoldNote
-                Origin = Anchor.Centre,
-                AutoSizeAxes = Axes.Y,
-                Width = 50,
-                Child = drawableObject = new DrawableHoldNote(holdNote)
                 {
-                    Height = 300,
+                    Column = i,
-                    AccentColour = { Value = OsuColour.Gray(0.3f) }
+                    StartTime = i * 100,
-                }
+                    Duration = 500
-            };
+                };
+                holdNote.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty());
 
-            AddBlueprint(new HoldNoteSelectionBlueprint(holdNote), drawableObject);
+                var drawableHitObject = new DrawableHoldNote(holdNote);
-        }
+                Playfield.Add(drawableHitObject);
-
+                AddBlueprint(new HoldNoteSelectionBlueprint(holdNote), drawableHitObject);
-        protected override void Update()
-        {
-            base.Update();
-
-            foreach (var nested in drawableObject.NestedHitObjects)
-            {
-                double finalPosition = (nested.HitObject.StartTime - drawableObject.HitObject.StartTime) / drawableObject.HitObject.Duration;
-                nested.Y = (float)(-finalPosition * content.DrawHeight);
             }
         }
     }

osu.Game.Rulesets.Mania.Tests/Editor/TestSceneManiaHitObjectComposer.cs

@@ -12,7 +12,7 @@ using osu.Framework.Utils;
 using osu.Game.Rulesets.Edit;
 using osu.Game.Rulesets.Mania.Beatmaps;
 using osu.Game.Rulesets.Mania.Edit;
-using osu.Game.Rulesets.Mania.Edit.Blueprints;
+using osu.Game.Rulesets.Mania.Edit.Blueprints.Components;
 using osu.Game.Rulesets.Mania.Objects;
 using osu.Game.Rulesets.Mania.Objects.Drawables;
 using osu.Game.Rulesets.Mania.Skinning.Default;
@@ -184,8 +184,8 @@ namespace osu.Game.Rulesets.Mania.Tests.Editor
             AddAssert("head note positioned correctly", () => Precision.AlmostEquals(holdNote.ScreenSpaceDrawQuad.BottomLeft, holdNote.Head.ScreenSpaceDrawQuad.BottomLeft));
             AddAssert("tail note positioned correctly", () => Precision.AlmostEquals(holdNote.ScreenSpaceDrawQuad.TopLeft, holdNote.Tail.ScreenSpaceDrawQuad.BottomLeft));
 
-            AddAssert("head blueprint positioned correctly", () => this.ChildrenOfType<HoldNoteNoteOverlay>().ElementAt(0).DrawPosition == holdNote.Head.DrawPosition);
+            AddAssert("head blueprint positioned correctly", () => this.ChildrenOfType<EditNotePiece>().ElementAt(0).DrawPosition == holdNote.Head.DrawPosition);
-            AddAssert("tail blueprint positioned correctly", () => this.ChildrenOfType<HoldNoteNoteOverlay>().ElementAt(1).DrawPosition == holdNote.Tail.DrawPosition);
+            AddAssert("tail blueprint positioned correctly", () => this.ChildrenOfType<EditNotePiece>().ElementAt(1).DrawPosition == holdNote.Tail.DrawPosition);
         }
 
         private void setScrollStep(ScrollingDirection direction)

osu.Game.Rulesets.Mania.Tests/Editor/TestSceneNoteSelectionBlueprint.cs

@@ -1,40 +1,32 @@
 // 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 osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Rulesets.Mania.Edit.Blueprints;
 using osu.Game.Rulesets.Mania.Objects;
 using osu.Game.Rulesets.Mania.Objects.Drawables;
-using osu.Game.Rulesets.UI.Scrolling;
-using osu.Game.Tests.Visual;
-using osuTK;
 
 namespace osu.Game.Rulesets.Mania.Tests.Editor
 {
     public class TestSceneNoteSelectionBlueprint : ManiaSelectionBlueprintTestScene
     {
-        protected override Container<Drawable> Content => content ?? base.Content;
-        private readonly Container content;
-
         public TestSceneNoteSelectionBlueprint()
+            : base(4)
         {
-            var note = new Note { Column = 0 };
+            for (int i = 0; i < 4; i++)
-            note.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty());
-
-            DrawableNote drawableObject;
-
-            base.Content.Child = content = new ScrollingTestContainer(ScrollingDirection.Down)
             {
-                Anchor = Anchor.Centre,
+                var note = new Note
-                Origin = Anchor.Centre,
+                {
-                Size = new Vector2(50, 20),
+                    Column = i,
-                Child = drawableObject = new DrawableNote(note)
+                    StartTime = i * 200,
-            };
+                };
+                note.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty());
 
-            AddBlueprint(new NoteSelectionBlueprint(note), drawableObject);
+                var drawableHitObject = new DrawableNote(note);
+                Playfield.Add(drawableHitObject);
+                AddBlueprint(new NoteSelectionBlueprint(note), drawableHitObject);
+            }
         }
     }
 }

osu.Game.Rulesets.Mania/Edit/Blueprints/HoldNoteNoteOverlay.cs

@@ -1,43 +0,0 @@
-// 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 osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
-using osu.Game.Rulesets.Mania.Edit.Blueprints.Components;
-using osu.Game.Rulesets.Mania.Objects.Drawables;
-
-namespace osu.Game.Rulesets.Mania.Edit.Blueprints
-{
-    public class HoldNoteNoteOverlay : CompositeDrawable
-    {
-        private readonly HoldNoteSelectionBlueprint holdNoteBlueprint;
-        private readonly HoldNotePosition position;
-
-        public HoldNoteNoteOverlay(HoldNoteSelectionBlueprint holdNoteBlueprint, HoldNotePosition position)
-        {
-            this.holdNoteBlueprint = holdNoteBlueprint;
-            this.position = position;
-
-            InternalChild = new EditNotePiece { RelativeSizeAxes = Axes.X };
-        }
-
-        protected override void Update()
-        {
-            base.Update();
-
-            var drawableObject = holdNoteBlueprint.DrawableObject;
-
-            // Todo: This shouldn't exist, mania should not reference the drawable hitobject directly.
-            if (drawableObject.IsLoaded)
-            {
-                DrawableNote note = position == HoldNotePosition.Start ? (DrawableNote)drawableObject.Head : drawableObject.Tail;
-
-                Anchor = note.Anchor;
-                Origin = note.Origin;
-
-                Size = note.DrawSize;
-                Position = note.DrawPosition;
-            }
-        }
-    }
-}

osu.Game.Rulesets.Mania/Edit/Blueprints/HoldNotePosition.cs

@@ -1,11 +0,0 @@
-// 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.
-
-namespace osu.Game.Rulesets.Mania.Edit.Blueprints
-{
-    public enum HoldNotePosition
-    {
-        Start,
-        End
-    }
-}

osu.Game.Rulesets.Mania/Edit/Blueprints/HoldNoteSelectionBlueprint.cs

@@ -2,14 +2,13 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using osu.Framework.Allocation;
-using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Primitives;
 using osu.Framework.Graphics.Shapes;
 using osu.Game.Graphics;
+using osu.Game.Rulesets.Mania.Edit.Blueprints.Components;
 using osu.Game.Rulesets.Mania.Objects;
-using osu.Game.Rulesets.Mania.Objects.Drawables;
 using osu.Game.Rulesets.UI.Scrolling;
 using osuTK;
 
@@ -17,13 +16,12 @@ namespace osu.Game.Rulesets.Mania.Edit.Blueprints
 {
     public class HoldNoteSelectionBlueprint : ManiaSelectionBlueprint<HoldNote>
     {
-        public new DrawableHoldNote DrawableObject => (DrawableHoldNote)base.DrawableObject;
-
-        private readonly IBindable<ScrollingDirection> direction = new Bindable<ScrollingDirection>();
-
         [Resolved]
         private OsuColour colours { get; set; }
 
+        private EditNotePiece head;
+        private EditNotePiece tail;
+
         public HoldNoteSelectionBlueprint(HoldNote hold)
             : base(hold)
         {
@@ -32,12 +30,10 @@ namespace osu.Game.Rulesets.Mania.Edit.Blueprints
         [BackgroundDependencyLoader]
         private void load(IScrollingInfo scrollingInfo)
         {
-            direction.BindTo(scrollingInfo.Direction);
-
             InternalChildren = new Drawable[]
             {
-                new HoldNoteNoteOverlay(this, HoldNotePosition.Start),
+                head = new EditNotePiece { RelativeSizeAxes = Axes.X },
-                new HoldNoteNoteOverlay(this, HoldNotePosition.End),
+                tail = new EditNotePiece { RelativeSizeAxes = Axes.X },
                 new Container
                 {
                     RelativeSizeAxes = Axes.Both,
@@ -58,21 +54,13 @@ namespace osu.Game.Rulesets.Mania.Edit.Blueprints
         {
             base.Update();
 
-            // Todo: This shouldn't exist, mania should not reference the drawable hitobject directly.
+            head.Y = HitObjectContainer.PositionAtTime(HitObject.Head.StartTime, HitObject.StartTime);
-            if (DrawableObject.IsLoaded)
+            tail.Y = HitObjectContainer.PositionAtTime(HitObject.Tail.StartTime, HitObject.StartTime);
-            {
+            Height = HitObjectContainer.LengthAtTime(HitObject.StartTime, HitObject.EndTime) + tail.DrawHeight;
-                Size = DrawableObject.DrawSize + new Vector2(0, DrawableObject.Tail.DrawHeight);
-
-                // This is a side-effect of not matching the hitobject's anchors/origins, which is kinda hard to do
-                // When scrolling upwards our origin is already at the top of the head note (which is the intended location),
-                // but when scrolling downwards our origin is at the _bottom_ of the tail note (where we need to be at the _top_ of the tail note)
-                if (direction.Value == ScrollingDirection.Down)
-                    Y -= DrawableObject.Tail.DrawHeight;
-            }
         }
 
         public override Quad SelectionQuad => ScreenSpaceDrawQuad;
 
-        public override Vector2 ScreenSpaceSelectionPoint => DrawableObject.Head.ScreenSpaceDrawQuad.Centre;
+        public override Vector2 ScreenSpaceSelectionPoint => head.ScreenSpaceDrawQuad.Centre;
     }
 }

osu.Game.Rulesets.Mania/Edit/Blueprints/ManiaSelectionBlueprint.cs

@@ -5,20 +5,23 @@ using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Game.Rulesets.Edit;
 using osu.Game.Rulesets.Mania.Objects;
-using osu.Game.Rulesets.Mania.Objects.Drawables;
+using osu.Game.Rulesets.Mania.UI;
+using osu.Game.Rulesets.UI;
 using osu.Game.Rulesets.UI.Scrolling;
-using osuTK;
 
 namespace osu.Game.Rulesets.Mania.Edit.Blueprints
 {
     public abstract class ManiaSelectionBlueprint<T> : HitObjectSelectionBlueprint<T>
         where T : ManiaHitObject
     {
-        public new DrawableManiaHitObject DrawableObject => (DrawableManiaHitObject)base.DrawableObject;
+        [Resolved]
+        private Playfield playfield { get; set; }
 
         [Resolved]
         private IScrollingInfo scrollingInfo { get; set; }
 
+        protected ScrollingHitObjectContainer HitObjectContainer => ((ManiaPlayfield)playfield).GetColumn(HitObject.Column).HitObjectContainer;
+
         protected ManiaSelectionBlueprint(T hitObject)
             : base(hitObject)
         {
@@ -29,19 +32,13 @@ namespace osu.Game.Rulesets.Mania.Edit.Blueprints
         {
             base.Update();
 
-            Position = Parent.ToLocalSpace(DrawableObject.ToScreenSpace(Vector2.Zero));
+            var anchor = scrollingInfo.Direction.Value == ScrollingDirection.Up ? Anchor.TopCentre : Anchor.BottomCentre;
-        }
+            Anchor = Origin = anchor;
+            foreach (var child in InternalChildren)
+                child.Anchor = child.Origin = anchor;
 
-        public override void Show()
+            Position = Parent.ToLocalSpace(HitObjectContainer.ScreenSpacePositionAtTime(HitObject.StartTime)) - AnchorPosition;
-        {
+            Width = HitObjectContainer.DrawWidth;
-            DrawableObject.AlwaysAlive = true;
-            base.Show();
-        }
-
-        public override void Hide()
-        {
-            DrawableObject.AlwaysAlive = false;
-            base.Hide();
         }
     }
 }

osu.Game.Rulesets.Mania/Edit/Blueprints/NoteSelectionBlueprint.cs

@@ -14,14 +14,5 @@ namespace osu.Game.Rulesets.Mania.Edit.Blueprints
         {
             AddInternal(new EditNotePiece { RelativeSizeAxes = Axes.X });
         }
-
-        protected override void Update()
-        {
-            base.Update();
-
-            // Todo: This shouldn't exist, mania should not reference the drawable hitobject directly.
-            if (DrawableObject.IsLoaded)
-                Size = DrawableObject.DrawSize;
-        }
     }
 }

osu.Game.Rulesets.Mania/Objects/Drawables/DrawableManiaHitObject.cs

@@ -85,63 +85,6 @@ namespace osu.Game.Rulesets.Mania.Objects.Drawables
                 AccentColour.UnbindFrom(ParentHitObject.AccentColour);
         }
 
-        private double computedLifetimeStart;
-
-        public override double LifetimeStart
-        {
-            get => base.LifetimeStart;
-            set
-            {
-                computedLifetimeStart = value;
-
-                if (!AlwaysAlive)
-                    base.LifetimeStart = value;
-            }
-        }
-
-        private double computedLifetimeEnd;
-
-        public override double LifetimeEnd
-        {
-            get => base.LifetimeEnd;
-            set
-            {
-                computedLifetimeEnd = value;
-
-                if (!AlwaysAlive)
-                    base.LifetimeEnd = value;
-            }
-        }
-
-        private bool alwaysAlive;
-
-        /// <summary>
-        /// Whether this <see cref="DrawableManiaHitObject"/> should always remain alive.
-        /// </summary>
-        internal bool AlwaysAlive
-        {
-            get => alwaysAlive;
-            set
-            {
-                if (alwaysAlive == value)
-                    return;
-
-                alwaysAlive = value;
-
-                if (value)
-                {
-                    // Set the base lifetimes directly, to avoid mangling the computed lifetimes
-                    base.LifetimeStart = double.MinValue;
-                    base.LifetimeEnd = double.MaxValue;
-                }
-                else
-                {
-                    LifetimeStart = computedLifetimeStart;
-                    LifetimeEnd = computedLifetimeEnd;
-                }
-            }
-        }
-
         protected virtual void OnDirectionChanged(ValueChangedEvent<ScrollingDirection> e)
         {
             Anchor = Origin = e.NewValue == ScrollingDirection.Up ? Anchor.TopCentre : Anchor.BottomCentre;

osu.Game/Rulesets/Edit/HitObjectComposer.cs

@@ -43,6 +43,9 @@ namespace osu.Game.Rulesets.Edit
 
         protected readonly Ruleset Ruleset;
 
+        // Provides `Playfield`
+        private DependencyContainer dependencies;
+
         [Resolved]
         protected EditorClock EditorClock { get; private set; }
 
@@ -69,6 +72,9 @@ namespace osu.Game.Rulesets.Edit
             Ruleset = ruleset;
         }
 
+        protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) =>
+            dependencies = new DependencyContainer(base.CreateChildDependencies(parent));
+
         [BackgroundDependencyLoader]
         private void load()
         {
@@ -88,6 +94,8 @@ namespace osu.Game.Rulesets.Edit
                 return;
             }
 
+            dependencies.CacheAs(Playfield);
+
             const float toolbar_width = 200;
 
             InternalChildren = new Drawable[]