From 3143224e5b5b51f939c6130dd026de24bb3e96db Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Oct 2020 14:23:18 +0900 Subject: [PATCH] Refactor how drawable carousel items are constructed --- .../SongSelect/TestSceneBeatmapCarousel.cs | 29 +-- osu.Game/Screens/Select/BeatmapCarousel.cs | 187 ++++++++---------- .../Select/Carousel/CarouselBeatmap.cs | 2 + .../Select/Carousel/CarouselBeatmapSet.cs | 15 +- .../Screens/Select/Carousel/CarouselItem.cs | 2 + .../Carousel/DrawableCarouselBeatmap.cs | 6 +- .../Carousel/DrawableCarouselBeatmapSet.cs | 43 ++++ .../Select/Carousel/DrawableCarouselItem.cs | 35 ++-- 8 files changed, 177 insertions(+), 142 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 3aff390a47..680928b331 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -709,19 +709,20 @@ namespace osu.Game.Tests.Visual.SongSelect private void loadBeatmaps(List beatmapSets = null, Func initialCriteria = null, Action carouselAdjust = null) { - createCarousel(carouselAdjust); - - if (beatmapSets == null) - { - beatmapSets = new List(); - - 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(); + + 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 Items => base.Items; - public bool PendingFilterTask => PendingFilter != null; + public IEnumerable Items => InternalChildren.OfType(); + protected override IEnumerable GetLoadableBeatmaps() => Enumerable.Empty(); } } diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 5f6f859d66..ef533a1456 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/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 yPositions = new List(); + private readonly List visibleItems = new List(); + private readonly Cached itemsCache = new Cached(); private readonly Cached scrollPositionCache = new Cached(); @@ -130,8 +129,6 @@ namespace osu.Game.Screens.Select private readonly List previouslyVisitedRandomSets = new List(); private readonly Stack randomSelectedBeatmaps = new Stack(); - protected List Items = new List(); - private CarouselRoot root; private IBindable> 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 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 - scrollableContent.RemoveAll(p => p.Y < visibleUpperBound - p.DrawHeight || p.Y > visibleBottomBound || !p.IsPresent); + // Remove all items that should no longer be on-screen + scrollableContent.RemoveAll(p => p.Y < visibleUpperBound - p.DrawHeight || p.Y > visibleBottomBound || !p.IsPresent); - // Find index range of all items that should be on-screen - Trace.Assert(Items.Count == yPositions.Count); + // Find index range of all items that should be on-screen + 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); - if (firstIndex < 0) firstIndex = ~firstIndex; - int lastIndex = yPositions.BinarySearch(visibleBottomBound); - if (lastIndex < 0) lastIndex = ~lastIndex; + scrollableContent.Clear(); - int notVisibleCount = 0; - - // 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) + // Add those items within the previously found index range that should be displayed. + for (int i = firstIndex; i < lastIndex; ++i) { - if (!item.IsPresent) - notVisibleCount++; - continue; - } + DrawableCarouselItem item = visibleItems[i].CreateDrawableRepresentation(); - 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. - if (!scrollableContent.Contains(item)) - { - // Makes sure headers are always _below_ items, - // and depth flows downward. - item.Depth = depth; + scrollableContent.Add(item); - 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: - LoadComponentAsync(item); - break; + // Makes sure headers are always _below_ items, + // and depth flows downward. + item.Depth = depth; - case LoadState.Loading: - break; + switch (item.LoadState) + { + case LoadState.NotLoaded: + LoadComponentAsync(item); + break; - default: - scrollableContent.Add(item); - break; + case LoadState.Loading: + break; + + default: + scrollableContent.Add(item); + break; + } } - } - else - { - scrollableContent.ChangeChildDepth(item, depth); + else + { + 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> weakItem) { if (weakItem.NewValue.TryGetTarget(out var item)) @@ -704,69 +700,39 @@ namespace osu.Game.Screens.Select /// The Y position of the currently selected item. 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: - { - lastSet = set; + visibleItems.Add(set); + yPositions.Add(currentY); + //lastSet = set; - set.MoveToX(set.Item.State.Value == CarouselItemState.Selected ? -100 : 0, 500, Easing.OutExpo); - set.MoveToY(currentY, 750, Easing.OutExpo); - 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; - } + // TODO: move this logic to DCBS too. + // set.MoveToX(set.Item.State.Value == CarouselItemState.Selected ? -100 : 0, 500, Easing.OutExpo); + // set.MoveToY(currentY, 750, Easing.OutExpo); + currentY += set.TotalHeight; + 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 /// 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; diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index 6a18e2d6a3..dce4028f17 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs +++ b/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) diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs index 44b8e72d51..e34710f71d 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs +++ b/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 Beatmaps => InternalChildren.OfType(); diff --git a/osu.Game/Screens/Select/Carousel/CarouselItem.cs b/osu.Game/Screens/Select/Carousel/CarouselItem.cs index 555c32c041..004d9779ef 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselItem.cs +++ b/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 State = new Bindable(CarouselItemState.NotSelected); diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmap.cs index c8f9507b91..7d69251265 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmap.cs +++ b/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)); diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs index 8332093a3c..aaa925c6f8 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs +++ b/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 ChildItems => beatmapContainer?.Children ?? base.ChildItems; + private readonly BeatmapSetInfo beatmapSet; + private Container beatmapContainer; + private Bindable 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 + { + X = 50, + Y = MAX_HEIGHT, + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y, + }, + }); + + beatmapSetState = Item.State.GetBoundCopy(); + beatmapSetState.BindValueChanged(setSelected, true); + } + + private void setSelected(ValueChangedEvent 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()) + { + item.Y = yPos; + beatmapContainer.Add(item); + yPos += item.Item.TotalHeight; + } + + break; + } } private const int maximum_difficulty_icons = 18; diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselItem.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselItem.cs index 121491d6ca..c0405b373d 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselItem.cs +++ b/osu.Game/Screens/Select/Carousel/DrawableCarouselItem.cs @@ -1,6 +1,8 @@ // Copyright (c) ppy Pty Ltd . 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; - private Container borderContainer; + public virtual IEnumerable ChildItems => Enumerable.Empty(); - private Box hoverLayer; + private readonly Container nestedContainer; + + protected readonly Container BorderContainer; + + private readonly Box hoverLayer; protected override Container Content => nestedContainer; @@ -41,14 +46,8 @@ namespace osu.Game.Screens.Select.Carousel Height = MAX_HEIGHT; RelativeSizeAxes = Axes.X; Alpha = 0; - } - private SampleChannel sampleHover; - - [BackgroundDependencyLoader] - private void load(AudioManager audio, OsuColour colours) - { - InternalChild = borderContainer = new Container + 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.EdgeEffect = new EdgeEffectParameters + BorderContainer.BorderThickness = 2.5f; + 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.EdgeEffect = new EdgeEffectParameters + BorderContainer.BorderThickness = 0; + BorderContainer.EdgeEffect = new EdgeEffectParameters { Type = EdgeEffectType.Shadow, Offset = new Vector2(1),