Refactor how drawable carousel items are constructed

osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs

@@ -709,19 +709,20 @@ namespace osu.Game.Tests.Visual.SongSelect
 
         private void loadBeatmaps(List<BeatmapSetInfo> beatmapSets = null, Func<FilterCriteria> initialCriteria = null, Action<BeatmapCarousel> carouselAdjust = null)
         {
-            createCarousel(carouselAdjust);
-
-            if (beatmapSets == null)
-            {
-                beatmapSets = new List<BeatmapSetInfo>();
-
-                for (int i = 1; i <= set_count; i++)
-                    beatmapSets.Add(createTestBeatmapSet(i));
-            }
-
             bool changed = false;
-            AddStep($"Load {(beatmapSets.Count > 0 ? beatmapSets.Count.ToString() : "some")} beatmaps", () =>
+
+            createCarousel(c =>
             {
+                carouselAdjust?.Invoke(c);
+
+                if (beatmapSets == null)
+                {
+                    beatmapSets = new List<BeatmapSetInfo>();
+
+                    for (int i = 1; i <= set_count; i++)
+                        beatmapSets.Add(createTestBeatmapSet(i));
+                }
+
                 carousel.Filter(initialCriteria?.Invoke() ?? new FilterCriteria());
                 carousel.BeatmapSetsChanged = () => changed = true;
                 carousel.BeatmapSets = beatmapSets;
@@ -807,7 +808,7 @@ namespace osu.Game.Tests.Visual.SongSelect
 
         private bool selectedBeatmapVisible()
         {
-            var currentlySelected = carousel.Items.Find(s => s.Item is CarouselBeatmap && s.Item.State.Value == CarouselItemState.Selected);
+            var currentlySelected = carousel.Items.FirstOrDefault(s => s.Item is CarouselBeatmap && s.Item.State.Value == CarouselItemState.Selected);
             if (currentlySelected == null)
                 return true;
 
@@ -908,10 +909,10 @@ namespace osu.Game.Tests.Visual.SongSelect
 
         private class TestBeatmapCarousel : BeatmapCarousel
         {
-            public new List<DrawableCarouselItem> Items => base.Items;
-
             public bool PendingFilterTask => PendingFilter != null;
 
+            public IEnumerable<DrawableCarouselItem> Items => InternalChildren.OfType<DrawableCarouselItem>();
+
             protected override IEnumerable<BeatmapSetInfo> GetLoadableBeatmaps() => Enumerable.Empty<BeatmapSetInfo>();
         }
     }

osu.Game/Screens/Select/BeatmapCarousel.cs

@@ -96,9 +96,6 @@ namespace osu.Game.Screens.Select
 
             beatmapSets.Select(createCarouselSet).Where(g => g != null).ForEach(newRoot.AddChild);
 
-            // preload drawables as the ctor overhead is quite high currently.
-            _ = newRoot.Drawables;
-
             root = newRoot;
             if (selectedBeatmapSet != null && !beatmapSets.Contains(selectedBeatmapSet.BeatmapSet))
                 selectedBeatmapSet = null;
@@ -119,6 +116,8 @@ namespace osu.Game.Screens.Select
         }
 
         private readonly List<float> yPositions = new List<float>();
+        private readonly List<CarouselItem> visibleItems = new List<CarouselItem>();
+
         private readonly Cached itemsCache = new Cached();
         private readonly Cached scrollPositionCache = new Cached();
 
@@ -130,8 +129,6 @@ namespace osu.Game.Screens.Select
         private readonly List<CarouselBeatmapSet> previouslyVisitedRandomSets = new List<CarouselBeatmapSet>();
         private readonly Stack<CarouselBeatmap> randomSelectedBeatmaps = new Stack<CarouselBeatmap>();
 
-        protected List<DrawableCarouselItem> Items = new List<DrawableCarouselItem>();
-
         private CarouselRoot root;
 
         private IBindable<WeakReference<BeatmapSetInfo>> itemUpdated;
@@ -178,7 +175,8 @@ namespace osu.Game.Screens.Select
             itemRestored = beatmaps.BeatmapRestored.GetBoundCopy();
             itemRestored.BindValueChanged(beatmapRestored);
 
-            loadBeatmapSets(GetLoadableBeatmaps());
+            if (!beatmapSets.Any())
+                loadBeatmapSets(GetLoadableBeatmaps());
         }
 
         protected virtual IEnumerable<BeatmapSetInfo> GetLoadableBeatmaps() => beatmaps.GetAllUsableBeatmapSetsEnumerable(IncludedDetails.AllButFiles);
@@ -558,71 +556,78 @@ namespace osu.Game.Screens.Select
         {
             base.Update();
 
+            //todo: this should only refresh items, not everything here
             if (!itemsCache.IsValid)
+            {
                 updateItems();
 
-            // Remove all items that should no longer be on-screen
+                // Remove all items that should no longer be on-screen
-            scrollableContent.RemoveAll(p => p.Y < visibleUpperBound - p.DrawHeight || p.Y > visibleBottomBound || !p.IsPresent);
+                scrollableContent.RemoveAll(p => p.Y < visibleUpperBound - p.DrawHeight || p.Y > visibleBottomBound || !p.IsPresent);
 
-            // Find index range of all items that should be on-screen
+                // Find index range of all items that should be on-screen
-            Trace.Assert(Items.Count == yPositions.Count);
+                int firstIndex = yPositions.BinarySearch(visibleUpperBound - DrawableCarouselItem.MAX_HEIGHT);
+                if (firstIndex < 0) firstIndex = ~firstIndex;
+                int lastIndex = yPositions.BinarySearch(visibleBottomBound);
+                if (lastIndex < 0) lastIndex = ~lastIndex;
 
-            int firstIndex = yPositions.BinarySearch(visibleUpperBound - DrawableCarouselItem.MAX_HEIGHT);
+                scrollableContent.Clear();
-            if (firstIndex < 0) firstIndex = ~firstIndex;
-            int lastIndex = yPositions.BinarySearch(visibleBottomBound);
-            if (lastIndex < 0) lastIndex = ~lastIndex;
 
-            int notVisibleCount = 0;
+                // Add those items within the previously found index range that should be displayed.
-
+                for (int i = firstIndex; i < lastIndex; ++i)
-            // Add those items within the previously found index range that should be displayed.
-            for (int i = firstIndex; i < lastIndex; ++i)
-            {
-                DrawableCarouselItem item = Items[i];
-
-                if (!item.Item.Visible)
                 {
-                    if (!item.IsPresent)
+                    DrawableCarouselItem item = visibleItems[i].CreateDrawableRepresentation();
-                        notVisibleCount++;
-                    continue;
-                }
 
-                float depth = i + (item is DrawableCarouselBeatmapSet ? -Items.Count : 0);
+                    item.Y = yPositions[i];
+                    item.Depth = i;
 
-                // Only add if we're not already part of the content.
+                    scrollableContent.Add(item);
-                if (!scrollableContent.Contains(item))
-                {
-                    // Makes sure headers are always _below_ items,
-                    // and depth flows downward.
-                    item.Depth = depth;
 
-                    switch (item.LoadState)
+                    // if (!item.Item.Visible)
+                    // {
+                    //     if (!item.IsPresent)
+                    //         notVisibleCount++;
+                    //     continue;
+                    // }
+
+                    // Only add if we're not already part of the content.
+                    /*
+                    if (!scrollableContent.Contains(item))
                     {
-                        case LoadState.NotLoaded:
+                        // Makes sure headers are always _below_ items,
-                            LoadComponentAsync(item);
+                        // and depth flows downward.
-                            break;
+                        item.Depth = depth;
 
-                        case LoadState.Loading:
+                        switch (item.LoadState)
-                            break;
+                        {
+                            case LoadState.NotLoaded:
+                                LoadComponentAsync(item);
+                                break;
 
-                        default:
+                            case LoadState.Loading:
-                            scrollableContent.Add(item);
+                                break;
-                            break;
+
+                            default:
+                                scrollableContent.Add(item);
+                                break;
+                        }
                     }
-                }
+                    else
-                else
+                    {
-                {
+                        scrollableContent.ChangeChildDepth(item, depth);
-                    scrollableContent.ChangeChildDepth(item, depth);
+                    }
+                    */
                 }
             }
 
-            // this is not actually useful right now, but once we have groups may well be.
-            if (notVisibleCount > 50)
-                itemsCache.Invalidate();
-
             // Update externally controlled state of currently visible items
             // (e.g. x-offset and opacity).
             foreach (DrawableCarouselItem p in scrollableContent.Children)
+            {
                 updateItem(p);
+
+                // foreach (var pChild in p.ChildItems)
+                //     updateItem(pChild, p);
+            }
         }
 
         protected override void UpdateAfterChildren()
@@ -633,15 +638,6 @@ namespace osu.Game.Screens.Select
                 updateScrollPosition();
         }
 
-        protected override void Dispose(bool isDisposing)
-        {
-            base.Dispose(isDisposing);
-
-            // aggressively dispose "off-screen" items to reduce GC pressure.
-            foreach (var i in Items)
-                i.Dispose();
-        }
-
         private void beatmapRemoved(ValueChangedEvent<WeakReference<BeatmapSetInfo>> weakItem)
         {
             if (weakItem.NewValue.TryGetTarget(out var item))
@@ -704,69 +700,39 @@ namespace osu.Game.Screens.Select
         /// <returns>The Y position of the currently selected item.</returns>
         private void updateItems()
         {
-            Items = root.Drawables.ToList();
-
             yPositions.Clear();
+            visibleItems.Clear();
 
             float currentY = visibleHalfHeight;
-            DrawableCarouselBeatmapSet lastSet = null;
 
             scrollTarget = null;
 
-            foreach (DrawableCarouselItem d in Items)
+            foreach (CarouselItem item in root.Children)
             {
-                if (d.IsPresent)
+                if (item.Filtered.Value)
+                    continue;
+
+                switch (item)
                 {
-                    switch (d)
+                    case CarouselBeatmapSet set:
                     {
-                        case DrawableCarouselBeatmapSet set:
+                        visibleItems.Add(set);
-                        {
+                        yPositions.Add(currentY);
-                            lastSet = set;
+                        //lastSet = set;
 
-                            set.MoveToX(set.Item.State.Value == CarouselItemState.Selected ? -100 : 0, 500, Easing.OutExpo);
+                        // TODO: move this logic to DCBS too.
-                            set.MoveToY(currentY, 750, Easing.OutExpo);
+                        // set.MoveToX(set.Item.State.Value == CarouselItemState.Selected ? -100 : 0, 500, Easing.OutExpo);
-                            break;
+                        // set.MoveToY(currentY, 750, Easing.OutExpo);
-                        }
+                        currentY += set.TotalHeight;
-
+                        break;
-                        case DrawableCarouselBeatmap beatmap:
-                        {
-                            if (beatmap.Item.State.Value == CarouselItemState.Selected)
-                                // scroll position at currentY makes the set panel appear at the very top of the carousel's screen space
-                                // move down by half of visible height (height of the carousel's visible extent, including semi-transparent areas)
-                                // then reapply the top semi-transparent area (because carousel's screen space starts below it)
-                                // and finally add half of the panel's own height to achieve vertical centering of the panel itself
-                                scrollTarget = currentY - visibleHalfHeight + BleedTop + beatmap.DrawHeight / 2;
-
-                            void performMove(float y, float? startY = null)
-                            {
-                                if (startY != null) beatmap.MoveTo(new Vector2(0, startY.Value));
-                                beatmap.MoveToX(beatmap.Item.State.Value == CarouselItemState.Selected ? -50 : 0, 500, Easing.OutExpo);
-                                beatmap.MoveToY(y, 750, Easing.OutExpo);
-                            }
-
-                            Debug.Assert(lastSet != null);
-
-                            float? setY = null;
-                            if (!d.IsLoaded || beatmap.Alpha == 0) // can't use IsPresent due to DrawableCarouselItem override.
-                                setY = lastSet.Y + lastSet.DrawHeight + 5;
-
-                            if (d.IsLoaded)
-                                performMove(currentY, setY);
-                            else
-                            {
-                                float y = currentY;
-                                d.OnLoadComplete += _ => performMove(y, setY);
-                            }
-
-                            break;
-                        }
                     }
+
+                    default:
+                        continue;
+                    //
+                    //     break;
+                    // }
                 }
-
-                yPositions.Add(currentY);
-
-                if (d.Item.Visible)
-                    currentY += d.DrawHeight + 5;
             }
 
             currentY += visibleHalfHeight;
@@ -869,6 +835,7 @@ namespace osu.Game.Screens.Select
             /// </summary>
             public bool UserScrolling { get; private set; }
 
+            // ReSharper disable once OptionalParameterHierarchyMismatch fuck off rider
             protected override void OnUserScroll(float value, bool animated = true, double? distanceDecay = default)
             {
                 UserScrolling = true;

osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs

@@ -10,6 +10,8 @@ namespace osu.Game.Screens.Select.Carousel
 {
     public class CarouselBeatmap : CarouselItem
     {
+        public override float TotalHeight => DrawableCarouselBeatmap.HEIGHT;
+
         public readonly BeatmapInfo Beatmap;
 
         public CarouselBeatmap(BeatmapInfo beatmap)

osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs

@@ -12,7 +12,20 @@ namespace osu.Game.Screens.Select.Carousel
 {
     public class CarouselBeatmapSet : CarouselGroupEagerSelect
     {
-        public float TotalHeight => DrawableCarouselBeatmapSet.HEIGHT + BeatmapSet.Beatmaps.Count * DrawableCarouselBeatmap.HEIGHT;
+        public override float TotalHeight
+        {
+            get
+            {
+                switch (State.Value)
+                {
+                    case CarouselItemState.Selected:
+                        return DrawableCarouselBeatmapSet.HEIGHT + Children.Count * DrawableCarouselBeatmap.HEIGHT;
+
+                    default:
+                        return DrawableCarouselBeatmapSet.HEIGHT;
+                }
+            }
+        }
 
         public IEnumerable<CarouselBeatmap> Beatmaps => InternalChildren.OfType<CarouselBeatmap>();
 

osu.Game/Screens/Select/Carousel/CarouselItem.cs

@@ -7,6 +7,8 @@ namespace osu.Game.Screens.Select.Carousel
 {
     public abstract class CarouselItem
     {
+        public virtual float TotalHeight => 0;
+
         public readonly BindableBool Filtered = new BindableBool();
 
         public readonly Bindable<CarouselItemState> State = new Bindable<CarouselItemState>(CarouselItemState.NotSelected);

osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmap.cs

@@ -31,7 +31,7 @@ namespace osu.Game.Screens.Select.Carousel
 {
     public class DrawableCarouselBeatmap : DrawableCarouselItem, IHasContextMenu
     {
-        public const float HEIGHT = MAX_HEIGHT;
+        public const float HEIGHT = MAX_HEIGHT * 0.6f;
 
         private readonly BeatmapInfo beatmap;
 
@@ -63,7 +63,7 @@ namespace osu.Game.Screens.Select.Carousel
             : base(panel)
         {
             beatmap = panel.Beatmap;
-            Height *= 0.60f;
+            Height = HEIGHT;
         }
 
         [BackgroundDependencyLoader(true)]
@@ -170,6 +170,8 @@ namespace osu.Game.Screens.Select.Carousel
         {
             base.Selected();
 
+            BorderContainer.MoveToX(Item.State.Value == CarouselItemState.Selected ? -50 : 0, 500, Easing.OutExpo);
+
             background.Colour = ColourInfo.GradientVertical(
                 new Color4(20, 43, 51, 255),
                 new Color4(40, 86, 102, 255));

osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs

@@ -43,8 +43,13 @@ namespace osu.Game.Screens.Select.Carousel
         [Resolved(CanBeNull = true)]
         private ManageCollectionsDialog manageCollectionsDialog { get; set; }
 
+        public override IEnumerable<DrawableCarouselItem> ChildItems => beatmapContainer?.Children ?? base.ChildItems;
+
         private readonly BeatmapSetInfo beatmapSet;
 
+        private Container<DrawableCarouselBeatmap> beatmapContainer;
+        private Bindable<CarouselItemState> beatmapSetState;
+
         public DrawableCarouselBeatmapSet(CarouselBeatmapSet set)
             : base(set)
         {
@@ -119,6 +124,44 @@ namespace osu.Game.Screens.Select.Carousel
                     }
                 }
             };
+
+            // TODO: temporary. we probably want to *not* inherit DrawableCarouselItem for this class, but only the above header portion.
+            AddRangeInternal(new Drawable[]
+            {
+                beatmapContainer = new Container<DrawableCarouselBeatmap>
+                {
+                    X = 50,
+                    Y = MAX_HEIGHT,
+                    RelativeSizeAxes = Axes.X,
+                    AutoSizeAxes = Axes.Y,
+                },
+            });
+
+            beatmapSetState = Item.State.GetBoundCopy();
+            beatmapSetState.BindValueChanged(setSelected, true);
+        }
+
+        private void setSelected(ValueChangedEvent<CarouselItemState> obj)
+        {
+            switch (obj.NewValue)
+            {
+                default:
+                    beatmapContainer.Clear();
+                    break;
+
+                case CarouselItemState.Selected:
+
+                    float yPos = 0;
+
+                    foreach (var item in ((CarouselBeatmapSet)Item).Beatmaps.Select(b => b.CreateDrawableRepresentation()).OfType<DrawableCarouselBeatmap>())
+                    {
+                        item.Y = yPos;
+                        beatmapContainer.Add(item);
+                        yPos += item.Item.TotalHeight;
+                    }
+
+                    break;
+            }
         }
 
         private const int maximum_difficulty_icons = 18;

osu.Game/Screens/Select/Carousel/DrawableCarouselItem.cs

@@ -1,6 +1,8 @@
 // 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 System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Audio;
 using osu.Framework.Audio.Sample;
@@ -27,10 +29,13 @@ namespace osu.Game.Screens.Select.Carousel
 
         public readonly CarouselItem Item;
 
-        private Container nestedContainer;
+        public virtual IEnumerable<DrawableCarouselItem> ChildItems => Enumerable.Empty<DrawableCarouselItem>();
-        private Container borderContainer;
 
-        private Box hoverLayer;
+        private readonly Container nestedContainer;
+
+        protected readonly Container BorderContainer;
+
+        private readonly Box hoverLayer;
 
         protected override Container<Drawable> Content => nestedContainer;
 
@@ -41,14 +46,8 @@ namespace osu.Game.Screens.Select.Carousel
             Height = MAX_HEIGHT;
             RelativeSizeAxes = Axes.X;
             Alpha = 0;
-        }
 
-        private SampleChannel sampleHover;
+            InternalChild = BorderContainer = new Container
-
-        [BackgroundDependencyLoader]
-        private void load(AudioManager audio, OsuColour colours)
-        {
-            InternalChild = borderContainer = new Container
             {
                 RelativeSizeAxes = Axes.Both,
                 Masking = true,
@@ -68,7 +67,13 @@ namespace osu.Game.Screens.Select.Carousel
                     },
                 }
             };
+        }
 
+        private SampleChannel sampleHover;
+
+        [BackgroundDependencyLoader]
+        private void load(AudioManager audio, OsuColour colours)
+        {
             sampleHover = audio.Samples.Get($@"SongSelect/song-ping-variation-{RNG.Next(1, 5)}");
             hoverLayer.Colour = colours.Blue.Opacity(0.1f);
         }
@@ -87,7 +92,7 @@ namespace osu.Game.Screens.Select.Carousel
             base.OnHoverLost(e);
         }
 
-        public void SetMultiplicativeAlpha(float alpha) => borderContainer.Alpha = alpha;
+        public void SetMultiplicativeAlpha(float alpha) => BorderContainer.Alpha = alpha;
 
         protected override void LoadComplete()
         {
@@ -123,8 +128,8 @@ namespace osu.Game.Screens.Select.Carousel
         {
             Item.State.Value = CarouselItemState.Selected;
 
-            borderContainer.BorderThickness = 2.5f;
+            BorderContainer.BorderThickness = 2.5f;
-            borderContainer.EdgeEffect = new EdgeEffectParameters
+            BorderContainer.EdgeEffect = new EdgeEffectParameters
             {
                 Type = EdgeEffectType.Glow,
                 Colour = new Color4(130, 204, 255, 150),
@@ -137,8 +142,8 @@ namespace osu.Game.Screens.Select.Carousel
         {
             Item.State.Value = CarouselItemState.NotSelected;
 
-            borderContainer.BorderThickness = 0;
+            BorderContainer.BorderThickness = 0;
-            borderContainer.EdgeEffect = new EdgeEffectParameters
+            BorderContainer.EdgeEffect = new EdgeEffectParameters
             {
                 Type = EdgeEffectType.Shadow,
                 Offset = new Vector2(1),