Merge pull request #29667 from EVAST9919/effect-point-perf-2

Rework kiai handling in `SummaryTimeline`

osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/EffectPointVisualisation.cs

@@ -1,132 +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.Allocation;
-using osu.Framework.Bindables;
-using osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
-using osu.Framework.Graphics.Cursor;
-using osu.Framework.Graphics.Shapes;
-using osu.Framework.Localisation;
-using osu.Game.Beatmaps.ControlPoints;
-using osu.Game.Extensions;
-using osu.Game.Graphics;
-
-namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
-{
-    public partial class EffectPointVisualisation : CompositeDrawable, IControlPointVisualisation
-    {
-        private readonly EffectControlPoint effect;
-        private Bindable<bool> kiai = null!;
-
-        [Resolved]
-        private EditorBeatmap beatmap { get; set; } = null!;
-
-        [Resolved]
-        private OsuColour colours { get; set; } = null!;
-
-        public EffectPointVisualisation(EffectControlPoint point)
-        {
-            RelativePositionAxes = Axes.Both;
-            RelativeSizeAxes = Axes.Y;
-
-            effect = point;
-        }
-
-        [BackgroundDependencyLoader]
-        private void load()
-        {
-            kiai = effect.KiaiModeBindable.GetBoundCopy();
-            kiai.BindValueChanged(_ => refreshDisplay(), true);
-        }
-
-        private EffectControlPoint? nextControlPoint;
-
-        protected override void LoadComplete()
-        {
-            base.LoadComplete();
-
-            // Due to the limitations of ControlPointInfo, it's impossible to know via event flow when the next kiai point has changed.
-            // This is due to the fact that an EffectPoint can be added to an existing group. We would need to bind to ItemAdded on *every*
-            // future group to track this.
-            //
-            // I foresee this being a potential performance issue on beatmaps with many control points, so let's limit how often we check
-            // for changes. ControlPointInfo needs a refactor to make this flow better, but it should do for now.
-            Scheduler.AddDelayed(() =>
-            {
-                EffectControlPoint? next = null;
-
-                for (int i = 0; i < beatmap.ControlPointInfo.EffectPoints.Count; i++)
-                {
-                    var point = beatmap.ControlPointInfo.EffectPoints[i];
-
-                    if (point.Time > effect.Time)
-                    {
-                        next = point;
-                        break;
-                    }
-                }
-
-                if (!ReferenceEquals(nextControlPoint, next))
-                {
-                    nextControlPoint = next;
-                    refreshDisplay();
-                }
-            }, 100, true);
-        }
-
-        private void refreshDisplay()
-        {
-            ClearInternal();
-
-            if (beatmap.BeatmapInfo.Ruleset.CreateInstance().EditorShowScrollSpeed)
-            {
-                AddInternal(new ControlPointVisualisation(effect)
-                {
-                    // importantly, override the x position being set since we do that in the GroupVisualisation parent drawable.
-                    X = 0,
-                });
-            }
-
-            if (!kiai.Value)
-                return;
-
-            // handle kiai duration
-            // eventually this will be simpler when we have control points with durations.
-            if (nextControlPoint != null)
-            {
-                RelativeSizeAxes = Axes.Both;
-                Origin = Anchor.TopLeft;
-
-                Width = (float)(nextControlPoint.Time - effect.Time);
-
-                AddInternal(new KiaiVisualisation(effect.Time, nextControlPoint.Time)
-                {
-                    RelativeSizeAxes = Axes.Both,
-                    Anchor = Anchor.BottomLeft,
-                    Origin = Anchor.CentreLeft,
-                    Height = 0.4f,
-                    Depth = float.MaxValue,
-                    Colour = colours.Purple1,
-                });
-            }
-        }
-
-        private partial class KiaiVisualisation : Circle, IHasTooltip
-        {
-            private readonly double startTime;
-            private readonly double endTime;
-
-            public KiaiVisualisation(double startTime, double endTime)
-            {
-                this.startTime = startTime;
-                this.endTime = endTime;
-            }
-
-            public LocalisableString TooltipText => $"{startTime.ToEditorFormattedString()} - {endTime.ToEditorFormattedString()} kiai time";
-        }
-
-        // kiai sections display duration, so are required to be visualised.
-        public bool IsVisuallyRedundant(ControlPoint other) => other is EffectControlPoint otherEffect && effect.KiaiMode == otherEffect.KiaiMode;
-    }
-}

osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/GroupVisualisation.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System.Linq;
+using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -15,6 +16,8 @@ namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
 
         private readonly IBindableList<ControlPoint> controlPoints = new BindableList<ControlPoint>();
 
+        private bool showScrollSpeed;
+
         public GroupVisualisation(ControlPointGroup group)
         {
             RelativePositionAxes = Axes.X;
@@ -24,8 +27,13 @@ namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
 
             Group = group;
             X = (float)group.Time;
+        }
+
+        [BackgroundDependencyLoader]
+        private void load(EditorBeatmap beatmap)
+        {
+            showScrollSpeed = beatmap.BeatmapInfo.Ruleset.CreateInstance().EditorShowScrollSpeed;
 
-            // Run in constructor so IsRedundant calls can work correctly.
             controlPoints.BindTo(Group.ControlPoints);
             controlPoints.BindCollectionChanged((_, _) =>
             {
@@ -47,8 +55,15 @@ namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
                             });
                             break;
 
-                        case EffectControlPoint effect:
+                        case EffectControlPoint:
-                            AddInternal(new EffectPointVisualisation(effect));
+                            if (!showScrollSpeed)
+                                return;
+
+                            AddInternal(new ControlPointVisualisation(point)
+                            {
+                                // importantly, override the x position being set since we do that above.
+                                X = 0,
+                            });
                             break;
                     }
                 }

osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/KiaiPart.cs

@@ -0,0 +1,123 @@
+// 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.Allocation;
+using osu.Framework.Graphics;
+using osu.Framework.Graphics.Cursor;
+using osu.Framework.Graphics.Pooling;
+using osu.Framework.Graphics.Shapes;
+using osu.Framework.Localisation;
+using osu.Game.Extensions;
+using osu.Game.Graphics;
+
+namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
+{
+    /// <summary>
+    /// The part of the timeline that displays kiai sections in the song.
+    /// </summary>
+    public partial class KiaiPart : TimelinePart
+    {
+        private DrawablePool<KiaiVisualisation> pool = null!;
+
+        [BackgroundDependencyLoader]
+        private void load()
+        {
+            AddInternal(pool = new DrawablePool<KiaiVisualisation>(10));
+        }
+
+        protected override void LoadBeatmap(EditorBeatmap beatmap)
+        {
+            base.LoadBeatmap(beatmap);
+            EditorBeatmap.ControlPointInfo.ControlPointsChanged += updateParts;
+        }
+
+        protected override void LoadComplete()
+        {
+            base.LoadComplete();
+            updateParts();
+        }
+
+        private void updateParts() => Scheduler.AddOnce(() =>
+        {
+            Clear(disposeChildren: false);
+
+            double? startTime = null;
+
+            foreach (var effectPoint in EditorBeatmap.ControlPointInfo.EffectPoints)
+            {
+                if (startTime.HasValue)
+                {
+                    if (effectPoint.KiaiMode)
+                        continue;
+
+                    var section = new KiaiSection
+                    {
+                        StartTime = startTime.Value,
+                        EndTime = effectPoint.Time
+                    };
+
+                    Add(pool.Get(v => v.Section = section));
+
+                    startTime = null;
+                }
+                else
+                {
+                    if (!effectPoint.KiaiMode)
+                        continue;
+
+                    startTime = effectPoint.Time;
+                }
+            }
+
+            // last effect point has kiai enabled, kiai should last until the end of the map
+            if (startTime.HasValue)
+            {
+                Add(pool.Get(v => v.Section = new KiaiSection
+                {
+                    StartTime = startTime.Value,
+                    EndTime = Content.RelativeChildSize.X
+                }));
+            }
+        });
+
+        private partial class KiaiVisualisation : PoolableDrawable, IHasTooltip
+        {
+            private KiaiSection section;
+
+            public KiaiSection Section
+            {
+                set
+                {
+                    section = value;
+
+                    X = (float)value.StartTime;
+                    Width = (float)value.Duration;
+                }
+            }
+
+            [BackgroundDependencyLoader]
+            private void load(OsuColour colours)
+            {
+                RelativePositionAxes = Axes.X;
+                RelativeSizeAxes = Axes.Both;
+                Anchor = Anchor.CentreLeft;
+                Origin = Anchor.CentreLeft;
+                Height = 0.2f;
+                AddInternal(new Circle
+                {
+                    RelativeSizeAxes = Axes.Both,
+                    Colour = colours.Purple1
+                });
+            }
+
+            public LocalisableString TooltipText => $"{section.StartTime.ToEditorFormattedString()} - {section.EndTime.ToEditorFormattedString()} kiai time";
+        }
+
+        private readonly struct KiaiSection
+        {
+            public double StartTime { get; init; }
+            public double EndTime { get; init; }
+            public double Duration => EndTime - StartTime;
+        }
+    }
+}

osu.Game/Screens/Edit/Components/Timelines/Summary/SummaryTimeline.cs

@@ -65,6 +65,12 @@ namespace osu.Game.Screens.Edit.Components.Timelines.Summary
                     Origin = Anchor.Centre,
                     RelativeSizeAxes = Axes.Both,
                 },
+                new KiaiPart
+                {
+                    Anchor = Anchor.Centre,
+                    Origin = Anchor.Centre,
+                    RelativeSizeAxes = Axes.Both,
+                },
                 new ControlPointPart
                 {
                     Anchor = Anchor.Centre,