From 749704344c5fbb0d46b153d98e60798e331a3965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 30 Jan 2025 13:11:05 +0100 Subject: [PATCH 01/20] Move implicit slider path segment handling logic to Bezier converter The logic in `LegacyBeatmapEncoder` that was supposed to handle the lazer-exclusive feature of supporting multiple slider segment types in a single slider was interfering rather badly with the Bezier converter. Generally it was a bit difficult to follow, too. The nice thing about `BezierConverter` is that it is *guaranteed* to only output Bezier control points. In light of this, the same double-up- -the-control-point logic that was supposed to make multiple slider segment types backwards-compatible with stable can be placed in the Bezier conversion logic, and be *much* more understandable, too. --- .../Beatmaps/Formats/LegacyBeatmapEncoder.cs | 59 +++++-------------- osu.Game/Rulesets/Objects/BezierConverter.cs | 4 ++ 2 files changed, 19 insertions(+), 44 deletions(-) diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs index 6c855e1346..07e88ab956 100644 --- a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs @@ -447,60 +447,31 @@ namespace osu.Game.Beatmaps.Formats private void addPathData(TextWriter writer, IHasPath pathData, Vector2 position) { - PathType? lastType = null; - for (int i = 0; i < pathData.Path.ControlPoints.Count; i++) { PathControlPoint point = pathData.Path.ControlPoints[i]; + // Note that lazer's encoding format supports specifying multiple curve types for a slider path, which is not supported by stable. + // Backwards compatibility with stable is handled by `LegacyBeatmapExporter` and `BezierConverter.ConvertToModernBezier()`. if (point.Type != null) { - // We've reached a new (explicit) segment! - - // Explicit segments have a new format in which the type is injected into the middle of the control point string. - // To preserve compatibility with osu-stable as much as possible, explicit segments with the same type are converted to use implicit segments by duplicating the control point. - // One exception are consecutive perfect curves, which aren't supported in osu!stable and can lead to decoding issues if encoded as implicit segments - bool needsExplicitSegment = point.Type != lastType || point.Type == PathType.PERFECT_CURVE || i == pathData.Path.ControlPoints.Count - 1; - - // Another exception to this is when the last two control points of the last segment were duplicated. This is not a scenario supported by osu!stable. - // Lazer does not add implicit segments for the last two control points of _any_ explicit segment, so an explicit segment is forced in order to maintain consistency with the decoder. - if (i > 1) + switch (point.Type?.Type) { - // We need to use the absolute control point position to determine equality, otherwise floating point issues may arise. - Vector2 p1 = position + pathData.Path.ControlPoints[i - 1].Position; - Vector2 p2 = position + pathData.Path.ControlPoints[i - 2].Position; + case SplineType.BSpline: + writer.Write(point.Type.Value.Degree > 0 ? $"B{point.Type.Value.Degree}|" : "B|"); + break; - if ((int)p1.X == (int)p2.X && (int)p1.Y == (int)p2.Y) - needsExplicitSegment = true; - } + case SplineType.Catmull: + writer.Write("C|"); + break; - if (needsExplicitSegment) - { - switch (point.Type?.Type) - { - case SplineType.BSpline: - writer.Write(point.Type.Value.Degree > 0 ? $"B{point.Type.Value.Degree}|" : "B|"); - break; + case SplineType.PerfectCurve: + writer.Write("P|"); + break; - case SplineType.Catmull: - writer.Write("C|"); - break; - - case SplineType.PerfectCurve: - writer.Write("P|"); - break; - - case SplineType.Linear: - writer.Write("L|"); - break; - } - - lastType = point.Type; - } - else - { - // New segment with the same type - duplicate the control point - writer.Write(FormattableString.Invariant($"{position.X + point.Position.X}:{position.Y + point.Position.Y}|")); + case SplineType.Linear: + writer.Write("L|"); + break; } } diff --git a/osu.Game/Rulesets/Objects/BezierConverter.cs b/osu.Game/Rulesets/Objects/BezierConverter.cs index 638975630e..384c686167 100644 --- a/osu.Game/Rulesets/Objects/BezierConverter.cs +++ b/osu.Game/Rulesets/Objects/BezierConverter.cs @@ -136,6 +136,7 @@ namespace osu.Game.Rulesets.Objects { for (int j = 0; j < segment.Length - 1; j++) { + if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(segment[j])); result.Add(new PathControlPoint(segment[j], j == 0 ? PathType.BEZIER : null)); } } @@ -147,6 +148,7 @@ namespace osu.Game.Rulesets.Objects { for (int j = 0; j < segment.Length - 1; j++) { + if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(segment[j])); result.Add(new PathControlPoint(segment[j], j == 0 ? PathType.BEZIER : null)); } } @@ -158,6 +160,7 @@ namespace osu.Game.Rulesets.Objects for (int j = 0; j < circleResult.Length - 1; j++) { + if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(circleResult[j])); result.Add(new PathControlPoint(circleResult[j], j == 0 ? PathType.BEZIER : null)); } @@ -170,6 +173,7 @@ namespace osu.Game.Rulesets.Objects for (int j = 0; j < bSplineResult.Length - 1; j++) { + if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(bSplineResult[j])); result.Add(new PathControlPoint(bSplineResult[j], j == 0 ? PathType.BEZIER : null)); } From b4f63da048e16c9f0fd0d339ea13f33637dade9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 30 Jan 2025 15:23:22 +0100 Subject: [PATCH 02/20] Move control point double-up logic to `LegacyBeatmapExporter` Done for two reasons: - During review it was requested for the logic to be moved out of `BezierConverter` as `BezierConverter` was intended to produce "lazer style" sliders with per-control-point curve types, as a future usability / code layering concern. - It is also relevant for encode-decode stability. With how the logic was structured between the Bezier converter and the legacy beatmap encoder, the encoder would leave behind per-control-point Bezier curve specs that stable ignored, but subsequent encodes and decodes in lazer would end up multiplying the doubled-up control points ad nauseam. Instead, it is sufficient to only specify the curve type for the head control point as Bezier, not specify any further curve types later on, and instead just keep the double-up-control-point for new implicit segment logic which is enough to make stable cooperate (and also as close to outputting the slider exactly as stable would have produced it as we've ever been) --- osu.Game/Database/LegacyBeatmapExporter.cs | 32 ++++++++++++++------ osu.Game/Rulesets/Objects/BezierConverter.cs | 4 --- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/osu.Game/Database/LegacyBeatmapExporter.cs b/osu.Game/Database/LegacyBeatmapExporter.cs index 24e752da31..9bb90ab461 100644 --- a/osu.Game/Database/LegacyBeatmapExporter.cs +++ b/osu.Game/Database/LegacyBeatmapExporter.cs @@ -120,18 +120,30 @@ namespace osu.Game.Database if (BezierConverter.CountSegments(hasPath.Path.ControlPoints) <= 1 && hasPath.Path.ControlPoints[0].Type!.Value.Degree == null) continue; - var newControlPoints = BezierConverter.ConvertToModernBezier(hasPath.Path.ControlPoints); - - // Truncate control points to integer positions - foreach (var pathControlPoint in newControlPoints) - { - pathControlPoint.Position = new Vector2( - (float)Math.Floor(pathControlPoint.Position.X), - (float)Math.Floor(pathControlPoint.Position.Y)); - } + var convertedToBezier = BezierConverter.ConvertToModernBezier(hasPath.Path.ControlPoints); hasPath.Path.ControlPoints.Clear(); - hasPath.Path.ControlPoints.AddRange(newControlPoints); + + for (int i = 0; i < convertedToBezier.Count; i++) + { + var convertedPoint = convertedToBezier[i]; + + // Truncate control points to integer positions + var position = new Vector2( + (float)Math.Floor(convertedPoint.Position.X), + (float)Math.Floor(convertedPoint.Position.Y)); + + // stable only supports a single curve type specification per slider. + // we exploit the fact that the converted-to-Bézier path only has Bézier segments, + // and thus we specify the Bézier curve type once ever at the start of the slider. + hasPath.Path.ControlPoints.Add(new PathControlPoint(position, i == 0 ? PathType.BEZIER : null)); + + // however, the Bézier path as output by the converter has multiple segments. + // `LegacyBeatmapEncoder` will attempt to encode this by emitting per-control-point curve type specs which don't do anything for stable. + // instead, stable expects control points that start a segment to be present in the path twice in succession. + if (convertedPoint.Type == PathType.BEZIER) + hasPath.Path.ControlPoints.Add(new PathControlPoint(position)); + } } // Encode to legacy format diff --git a/osu.Game/Rulesets/Objects/BezierConverter.cs b/osu.Game/Rulesets/Objects/BezierConverter.cs index 384c686167..638975630e 100644 --- a/osu.Game/Rulesets/Objects/BezierConverter.cs +++ b/osu.Game/Rulesets/Objects/BezierConverter.cs @@ -136,7 +136,6 @@ namespace osu.Game.Rulesets.Objects { for (int j = 0; j < segment.Length - 1; j++) { - if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(segment[j])); result.Add(new PathControlPoint(segment[j], j == 0 ? PathType.BEZIER : null)); } } @@ -148,7 +147,6 @@ namespace osu.Game.Rulesets.Objects { for (int j = 0; j < segment.Length - 1; j++) { - if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(segment[j])); result.Add(new PathControlPoint(segment[j], j == 0 ? PathType.BEZIER : null)); } } @@ -160,7 +158,6 @@ namespace osu.Game.Rulesets.Objects for (int j = 0; j < circleResult.Length - 1; j++) { - if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(circleResult[j])); result.Add(new PathControlPoint(circleResult[j], j == 0 ? PathType.BEZIER : null)); } @@ -173,7 +170,6 @@ namespace osu.Game.Rulesets.Objects for (int j = 0; j < bSplineResult.Length - 1; j++) { - if (result.Count > 0 && j == 0) result.Add(new PathControlPoint(bSplineResult[j])); result.Add(new PathControlPoint(bSplineResult[j], j == 0 ? PathType.BEZIER : null)); } From 20280cd1959d0ceecff45f1e11a7aff3cedd5768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 31 Jan 2025 09:01:42 +0100 Subject: [PATCH 03/20] Do not double up first control point of path --- osu.Game/Database/LegacyBeatmapExporter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Database/LegacyBeatmapExporter.cs b/osu.Game/Database/LegacyBeatmapExporter.cs index 9bb90ab461..8f94fc9e63 100644 --- a/osu.Game/Database/LegacyBeatmapExporter.cs +++ b/osu.Game/Database/LegacyBeatmapExporter.cs @@ -141,7 +141,7 @@ namespace osu.Game.Database // however, the Bézier path as output by the converter has multiple segments. // `LegacyBeatmapEncoder` will attempt to encode this by emitting per-control-point curve type specs which don't do anything for stable. // instead, stable expects control points that start a segment to be present in the path twice in succession. - if (convertedPoint.Type == PathType.BEZIER) + if (convertedPoint.Type == PathType.BEZIER && i > 0) hasPath.Path.ControlPoints.Add(new PathControlPoint(position)); } } From 3cde11ab773f705e4132d7f837150e1b1232c11b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 24 Jan 2025 19:28:39 +0900 Subject: [PATCH 04/20] Re-enable masking by default --- .../Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs | 7 +++++++ osu.Game/Screens/SelectV2/Carousel.cs | 1 - 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs index 3a516ea762..0e72ee4f8c 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs @@ -32,6 +32,13 @@ namespace osu.Game.Tests.Visual.SongSelect RemoveAllBeatmaps(); } + [Test] + public void TestOffScreenLoading() + { + AddStep("disable masking", () => Scroll.Masking = false); + AddStep("enable masking", () => Scroll.Masking = true); + } + [Test] public void TestAddRemoveOneByOne() { diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 648c2d090a..811bb120e1 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -205,7 +205,6 @@ namespace osu.Game.Screens.SelectV2 InternalChild = scroll = new CarouselScrollContainer { RelativeSizeAxes = Axes.Both, - Masking = false, }; Items.BindCollectionChanged((_, _) => FilterAsync()); From d5dc55149d93cd534e3106a5997be2262d18be17 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 24 Jan 2025 19:29:14 +0900 Subject: [PATCH 05/20] Add initial difficulty grouping support --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 59 ++++++--- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 36 +++++- osu.Game/Screens/SelectV2/GroupPanel.cs | 113 ++++++++++++++++++ 3 files changed, 191 insertions(+), 17 deletions(-) create mode 100644 osu.Game/Screens/SelectV2/GroupPanel.cs diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index bb13c7449d..9a87fba140 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -92,34 +92,56 @@ namespace osu.Game.Screens.SelectV2 #region Selection handling + private GroupDefinition? lastSelectedGroup; + private BeatmapInfo? lastSelectedBeatmap; + protected override void HandleItemSelected(object? model) { base.HandleItemSelected(model); - // Selecting a set isn't valid – let's re-select the first difficulty. - if (model is BeatmapSetInfo setInfo) + switch (model) { - CurrentSelection = setInfo.Beatmaps.First(); - return; - } + case GroupDefinition group: + if (lastSelectedGroup != null) + setVisibilityOfGroupItems(lastSelectedGroup, false); + lastSelectedGroup = group; - if (model is BeatmapInfo beatmapInfo) - setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); + setVisibilityOfGroupItems(group, true); + + // In stable, you can kinda select a group (expand without changing selection) + // For simplicity, let's not do that for now and handle similar to a beatmap set header. + CurrentSelection = grouping.GroupItems[group].First().Model; + return; + + case BeatmapSetInfo setInfo: + // Selecting a set isn't valid – let's re-select the first difficulty. + CurrentSelection = setInfo.Beatmaps.First(); + return; + + case BeatmapInfo beatmapInfo: + if (lastSelectedBeatmap != null) + setVisibilityOfSetItems(lastSelectedBeatmap.BeatmapSet!, false); + lastSelectedBeatmap = beatmapInfo; + + setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); + break; + } } - protected override void HandleItemDeselected(object? model) + private void setVisibilityOfGroupItems(GroupDefinition group, bool visible) { - base.HandleItemDeselected(model); - - if (model is BeatmapInfo beatmapInfo) - setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, false); + if (grouping.GroupItems.TryGetValue(group, out var items)) + { + foreach (var i in items) + i.IsVisible = visible; + } } private void setVisibilityOfSetItems(BeatmapSetInfo set, bool visible) { - if (grouping.SetItems.TryGetValue(set, out var group)) + if (grouping.SetItems.TryGetValue(set, out var items)) { - foreach (var i in group) + foreach (var i in items) i.IsVisible = visible; } } @@ -143,9 +165,11 @@ namespace osu.Game.Screens.SelectV2 private readonly DrawablePool beatmapPanelPool = new DrawablePool(100); private readonly DrawablePool setPanelPool = new DrawablePool(100); + private readonly DrawablePool groupPanelPool = new DrawablePool(100); private void setupPools() { + AddInternal(groupPanelPool); AddInternal(beatmapPanelPool); AddInternal(setPanelPool); } @@ -154,7 +178,12 @@ namespace osu.Game.Screens.SelectV2 { switch (item.Model) { + case GroupDefinition: + return groupPanelPool.Get(); + case BeatmapInfo: + // TODO: if beatmap is a group selection target, it needs to be a different drawable + // with more information attached. return beatmapPanelPool.Get(); case BeatmapSetInfo: @@ -166,4 +195,6 @@ namespace osu.Game.Screens.SelectV2 #endregion } + + public record GroupDefinition(string Title); } diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index 0658263a8c..e8384a8a2d 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -18,7 +18,13 @@ namespace osu.Game.Screens.SelectV2 /// public IDictionary> SetItems => setItems; + /// + /// Groups contain children which are group-selectable. This dictionary holds the relationships between groups-panels to allow expanding them on selection. + /// + public IDictionary> GroupItems => groupItems; + private readonly Dictionary> setItems = new Dictionary>(); + private readonly Dictionary> groupItems = new Dictionary>(); private readonly Func getCriteria; @@ -31,15 +37,40 @@ namespace osu.Game.Screens.SelectV2 { var criteria = getCriteria(); + int starGroup = int.MinValue; + if (criteria.SplitOutDifficulties) { + var diffItems = new List(items.Count()); + + GroupDefinition? group = null; + foreach (var item in items) { - item.IsVisible = true; + var b = (BeatmapInfo)item.Model; + + if (b.StarRating > starGroup) + { + starGroup = (int)Math.Floor(b.StarRating); + group = new GroupDefinition($"{starGroup} - {++starGroup} *"); + diffItems.Add(new CarouselItem(group) + { + DrawHeight = GroupPanel.HEIGHT, + IsGroupSelectionTarget = true + }); + } + + if (!groupItems.TryGetValue(group!, out var related)) + groupItems[group!] = related = new HashSet(); + related.Add(item); + + diffItems.Add(item); + + item.IsVisible = false; item.IsGroupSelectionTarget = true; } - return items; + return diffItems; } CarouselItem? lastItem = null; @@ -64,7 +95,6 @@ namespace osu.Game.Screens.SelectV2 if (!setItems.TryGetValue(b.BeatmapSet!, out var related)) setItems[b.BeatmapSet!] = related = new HashSet(); - related.Add(item); } diff --git a/osu.Game/Screens/SelectV2/GroupPanel.cs b/osu.Game/Screens/SelectV2/GroupPanel.cs new file mode 100644 index 0000000000..e837d8a32f --- /dev/null +++ b/osu.Game/Screens/SelectV2/GroupPanel.cs @@ -0,0 +1,113 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using System.Diagnostics; +using osu.Framework.Allocation; +using osu.Framework.Bindables; +using osu.Framework.Extensions.Color4Extensions; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Pooling; +using osu.Framework.Graphics.Shapes; +using osu.Framework.Input.Events; +using osu.Game.Graphics.Sprites; +using osuTK; +using osuTK.Graphics; + +namespace osu.Game.Screens.SelectV2 +{ + public partial class GroupPanel : PoolableDrawable, ICarouselPanel + { + public const float HEIGHT = CarouselItem.DEFAULT_HEIGHT * 2; + + [Resolved] + private BeatmapCarousel carousel { get; set; } = null!; + + private Box activationFlash = null!; + private OsuSpriteText text = null!; + + [BackgroundDependencyLoader] + private void load() + { + Size = new Vector2(500, HEIGHT); + Masking = true; + + InternalChildren = new Drawable[] + { + new Box + { + Colour = Color4.DarkBlue.Darken(5), + Alpha = 0.8f, + RelativeSizeAxes = Axes.Both, + }, + activationFlash = new Box + { + Colour = Color4.White, + Blending = BlendingParameters.Additive, + Alpha = 0, + RelativeSizeAxes = Axes.Both, + }, + text = new OsuSpriteText + { + Padding = new MarginPadding(5), + Anchor = Anchor.CentreLeft, + Origin = Anchor.CentreLeft, + } + }; + + Selected.BindValueChanged(value => + { + activationFlash.FadeTo(value.NewValue ? 0.2f : 0, 500, Easing.OutQuint); + }); + + KeyboardSelected.BindValueChanged(value => + { + if (value.NewValue) + { + BorderThickness = 5; + BorderColour = Color4.Pink; + } + else + { + BorderThickness = 0; + } + }); + } + + protected override void PrepareForUse() + { + base.PrepareForUse(); + + Debug.Assert(Item != null); + Debug.Assert(Item.IsGroupSelectionTarget); + + GroupDefinition group = (GroupDefinition)Item.Model; + + text.Text = group.Title; + + this.FadeInFromZero(500, Easing.OutQuint); + } + + protected override bool OnClick(ClickEvent e) + { + carousel.CurrentSelection = Item!.Model; + return true; + } + + #region ICarouselPanel + + public CarouselItem? Item { get; set; } + public BindableBool Selected { get; } = new BindableBool(); + public BindableBool KeyboardSelected { get; } = new BindableBool(); + + public double DrawYPosition { get; set; } + + public void Activated() + { + // sets should never be activated. + throw new InvalidOperationException(); + } + + #endregion + } +} From 764f799dcb3aeb33cb905888d811a91e5a37640f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 28 Jan 2025 22:53:17 +0900 Subject: [PATCH 06/20] Improve selection flow using early exit and invalidation --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 31 +++++++++++--- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 3 ++ osu.Game/Screens/SelectV2/Carousel.cs | 41 ++++++++++--------- 3 files changed, 50 insertions(+), 25 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 9a87fba140..0a7ca5a6bb 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -95,7 +95,7 @@ namespace osu.Game.Screens.SelectV2 private GroupDefinition? lastSelectedGroup; private BeatmapInfo? lastSelectedBeatmap; - protected override void HandleItemSelected(object? model) + protected override bool HandleItemSelected(object? model) { base.HandleItemSelected(model); @@ -104,6 +104,14 @@ namespace osu.Game.Screens.SelectV2 case GroupDefinition group: if (lastSelectedGroup != null) setVisibilityOfGroupItems(lastSelectedGroup, false); + + // Collapsing an open group. + if (lastSelectedGroup == group) + { + lastSelectedGroup = null; + return false; + } + lastSelectedGroup = group; setVisibilityOfGroupItems(group, true); @@ -111,21 +119,34 @@ namespace osu.Game.Screens.SelectV2 // In stable, you can kinda select a group (expand without changing selection) // For simplicity, let's not do that for now and handle similar to a beatmap set header. CurrentSelection = grouping.GroupItems[group].First().Model; - return; + return false; case BeatmapSetInfo setInfo: // Selecting a set isn't valid – let's re-select the first difficulty. CurrentSelection = setInfo.Beatmaps.First(); - return; + return false; case BeatmapInfo beatmapInfo: if (lastSelectedBeatmap != null) setVisibilityOfSetItems(lastSelectedBeatmap.BeatmapSet!, false); lastSelectedBeatmap = beatmapInfo; - setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); - break; + // If we have groups, we need to account for them. + if (grouping.GroupItems.Count > 0) + { + // Find the containing group. There should never be too many groups so iterating is efficient enough. + var group = grouping.GroupItems.Single(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; + setVisibilityOfGroupItems(group, true); + } + else + setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); + + // Ensure the group containing this beatmap is also visible. + // TODO: need to update visibility of correct group? + return true; } + + return true; } private void setVisibilityOfGroupItems(GroupDefinition group, bool visible) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index e8384a8a2d..9ecf735980 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -35,6 +35,9 @@ namespace osu.Game.Screens.SelectV2 public async Task> Run(IEnumerable items, CancellationToken cancellationToken) => await Task.Run(() => { + setItems.Clear(); + groupItems.Clear(); + var criteria = getCriteria(); int starGroup = int.MinValue; diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 811bb120e1..7184aaa866 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -8,6 +8,7 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; using osu.Framework.Bindables; +using osu.Framework.Caching; using osu.Framework.Extensions.TypeExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -170,9 +171,8 @@ namespace osu.Game.Screens.SelectV2 /// /// Called when an item is "selected". /// - protected virtual void HandleItemSelected(object? model) - { - } + /// Whether the item should be selected. + protected virtual bool HandleItemSelected(object? model) => true; /// /// Called when an item is "deselected". @@ -410,6 +410,8 @@ namespace osu.Game.Screens.SelectV2 #region Selection handling + private readonly Cached selectionValid = new Cached(); + private Selection currentKeyboardSelection = new Selection(); private Selection currentSelection = new Selection(); @@ -418,29 +420,21 @@ namespace osu.Game.Screens.SelectV2 if (currentSelection.Model == model) return; - var previousSelection = currentSelection; + if (HandleItemSelected(model)) + { + if (currentSelection.Model != null) + HandleItemDeselected(currentSelection.Model); - if (previousSelection.Model != null) - HandleItemDeselected(previousSelection.Model); - - currentSelection = currentKeyboardSelection = new Selection(model); - HandleItemSelected(currentSelection.Model); - - // `HandleItemSelected` can alter `CurrentSelection`, which will recursively call `setSelection()` again. - // if that happens, the rest of this method should be a no-op. - if (currentSelection.Model != model) - return; - - refreshAfterSelection(); - scrollToSelection(); + currentKeyboardSelection = new Selection(model); + currentSelection = currentKeyboardSelection; + selectionValid.Invalidate(); + } } private void setKeyboardSelection(object? model) { currentKeyboardSelection = new Selection(model); - - refreshAfterSelection(); - scrollToSelection(); + selectionValid.Invalidate(); } /// @@ -525,6 +519,13 @@ namespace osu.Game.Screens.SelectV2 if (carouselItems == null) return; + if (!selectionValid.IsValid) + { + refreshAfterSelection(); + scrollToSelection(); + selectionValid.Validate(); + } + var range = getDisplayRange(); if (range != displayedRange) From d74939e6e983267a5bc8be37d94108d46581b02f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 31 Jan 2025 20:58:32 +0900 Subject: [PATCH 07/20] Fix backwards traversal of groupings and allow toggling groups without updating selection --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 64 +++++++++++++------ .../SelectV2/BeatmapCarouselFilterGrouping.cs | 9 +-- osu.Game/Screens/SelectV2/BeatmapSetPanel.cs | 1 - osu.Game/Screens/SelectV2/Carousel.cs | 18 +++++- osu.Game/Screens/SelectV2/CarouselItem.cs | 5 -- osu.Game/Screens/SelectV2/GroupPanel.cs | 1 - 6 files changed, 60 insertions(+), 38 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 0a7ca5a6bb..10bc069cfc 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -102,23 +102,15 @@ namespace osu.Game.Screens.SelectV2 switch (model) { case GroupDefinition group: - if (lastSelectedGroup != null) - setVisibilityOfGroupItems(lastSelectedGroup, false); - - // Collapsing an open group. + // Special case – collapsing an open group. if (lastSelectedGroup == group) { + setVisibilityOfGroupItems(lastSelectedGroup, false); lastSelectedGroup = null; return false; } - lastSelectedGroup = group; - - setVisibilityOfGroupItems(group, true); - - // In stable, you can kinda select a group (expand without changing selection) - // For simplicity, let's not do that for now and handle similar to a beatmap set header. - CurrentSelection = grouping.GroupItems[group].First().Model; + setVisibleGroup(group); return false; case BeatmapSetInfo setInfo: @@ -127,28 +119,52 @@ namespace osu.Game.Screens.SelectV2 return false; case BeatmapInfo beatmapInfo: - if (lastSelectedBeatmap != null) - setVisibilityOfSetItems(lastSelectedBeatmap.BeatmapSet!, false); - lastSelectedBeatmap = beatmapInfo; // If we have groups, we need to account for them. - if (grouping.GroupItems.Count > 0) + if (Criteria.SplitOutDifficulties) { // Find the containing group. There should never be too many groups so iterating is efficient enough. - var group = grouping.GroupItems.Single(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; - setVisibilityOfGroupItems(group, true); + GroupDefinition group = grouping.GroupItems.Single(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; + + setVisibleGroup(group); } else - setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); + { + setVisibleSet(beatmapInfo); + } - // Ensure the group containing this beatmap is also visible. - // TODO: need to update visibility of correct group? return true; } return true; } + protected override bool CheckValidForGroupSelection(CarouselItem item) + { + switch (item.Model) + { + case BeatmapSetInfo: + return true; + + case BeatmapInfo: + return Criteria.SplitOutDifficulties; + + case GroupDefinition: + return false; + + default: + throw new ArgumentException($"Unsupported model type {item.Model}"); + } + } + + private void setVisibleGroup(GroupDefinition group) + { + if (lastSelectedGroup != null) + setVisibilityOfGroupItems(lastSelectedGroup, false); + lastSelectedGroup = group; + setVisibilityOfGroupItems(group, true); + } + private void setVisibilityOfGroupItems(GroupDefinition group, bool visible) { if (grouping.GroupItems.TryGetValue(group, out var items)) @@ -158,6 +174,14 @@ namespace osu.Game.Screens.SelectV2 } } + private void setVisibleSet(BeatmapInfo beatmapInfo) + { + if (lastSelectedBeatmap != null) + setVisibilityOfSetItems(lastSelectedBeatmap.BeatmapSet!, false); + lastSelectedBeatmap = beatmapInfo; + setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); + } + private void setVisibilityOfSetItems(BeatmapSetInfo set, bool visible) { if (grouping.SetItems.TryGetValue(set, out var items)) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index 9ecf735980..951b010564 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -56,11 +56,7 @@ namespace osu.Game.Screens.SelectV2 { starGroup = (int)Math.Floor(b.StarRating); group = new GroupDefinition($"{starGroup} - {++starGroup} *"); - diffItems.Add(new CarouselItem(group) - { - DrawHeight = GroupPanel.HEIGHT, - IsGroupSelectionTarget = true - }); + diffItems.Add(new CarouselItem(group) { DrawHeight = GroupPanel.HEIGHT }); } if (!groupItems.TryGetValue(group!, out var related)) @@ -70,7 +66,6 @@ namespace osu.Game.Screens.SelectV2 diffItems.Add(item); item.IsVisible = false; - item.IsGroupSelectionTarget = true; } return diffItems; @@ -92,7 +87,6 @@ namespace osu.Game.Screens.SelectV2 newItems.Add(new CarouselItem(b.BeatmapSet!) { DrawHeight = BeatmapSetPanel.HEIGHT, - IsGroupSelectionTarget = true }); } @@ -104,7 +98,6 @@ namespace osu.Game.Screens.SelectV2 newItems.Add(item); lastItem = item; - item.IsGroupSelectionTarget = false; item.IsVisible = false; } diff --git a/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs b/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs index 37e8b88f71..06e3ad3426 100644 --- a/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs @@ -67,7 +67,6 @@ namespace osu.Game.Screens.SelectV2 base.PrepareForUse(); Debug.Assert(Item != null); - Debug.Assert(Item.IsGroupSelectionTarget); var beatmapSetInfo = (BeatmapSetInfo)Item.Model; diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 7184aaa866..a76b6efee9 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -168,6 +168,13 @@ namespace osu.Game.Screens.SelectV2 protected Drawable? GetMaterialisedDrawableForItem(CarouselItem item) => scroll.Panels.SingleOrDefault(p => ((ICarouselPanel)p).Item == item); + /// + /// When a user is traversing the carousel via group selection keys, assert whether the item provided is a valid target. + /// + /// The candidate item. + /// Whether the provided item is a valid group target. If false, more panels will be checked in the user's requested direction until a valid target is found. + protected virtual bool CheckValidForGroupSelection(CarouselItem item) => true; + /// /// Called when an item is "selected". /// @@ -373,7 +380,7 @@ namespace osu.Game.Screens.SelectV2 // make sure to go back to the group header this item belongs to, so that the block below doesn't find it and stop too early. if (isGroupSelection && direction < 0) { - while (!carouselItems[selectionIndex].IsGroupSelectionTarget) + while (!CheckValidForGroupSelection(carouselItems[selectionIndex])) selectionIndex--; } @@ -394,7 +401,11 @@ namespace osu.Game.Screens.SelectV2 bool attemptSelection(CarouselItem item) { - if (!item.IsVisible || (isGroupSelection && !item.IsGroupSelectionTarget)) + // Keyboard (non-group) selection should only consider visible items. + if (!isGroupSelection && !item.IsVisible) + return false; + + if (isGroupSelection && !CheckValidForGroupSelection(item)) return false; if (isGroupSelection) @@ -427,8 +438,9 @@ namespace osu.Game.Screens.SelectV2 currentKeyboardSelection = new Selection(model); currentSelection = currentKeyboardSelection; - selectionValid.Invalidate(); } + + selectionValid.Invalidate(); } private void setKeyboardSelection(object? model) diff --git a/osu.Game/Screens/SelectV2/CarouselItem.cs b/osu.Game/Screens/SelectV2/CarouselItem.cs index 2cb96a3d7f..13d5c840cf 100644 --- a/osu.Game/Screens/SelectV2/CarouselItem.cs +++ b/osu.Game/Screens/SelectV2/CarouselItem.cs @@ -29,11 +29,6 @@ namespace osu.Game.Screens.SelectV2 /// public float DrawHeight { get; set; } = DEFAULT_HEIGHT; - /// - /// Whether this item should be a valid target for user group selection hotkeys. - /// - public bool IsGroupSelectionTarget { get; set; } - /// /// Whether this item is visible or collapsed (hidden). /// diff --git a/osu.Game/Screens/SelectV2/GroupPanel.cs b/osu.Game/Screens/SelectV2/GroupPanel.cs index e837d8a32f..882d77cb8d 100644 --- a/osu.Game/Screens/SelectV2/GroupPanel.cs +++ b/osu.Game/Screens/SelectV2/GroupPanel.cs @@ -79,7 +79,6 @@ namespace osu.Game.Screens.SelectV2 base.PrepareForUse(); Debug.Assert(Item != null); - Debug.Assert(Item.IsGroupSelectionTarget); GroupDefinition group = (GroupDefinition)Item.Model; From 645c26ca19a16e9c5b33fb66125011c806ca2d78 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 1 Feb 2025 11:18:45 +0900 Subject: [PATCH 08/20] Simplify keyboard traversal logic --- osu.Game/Screens/SelectV2/Carousel.cs | 149 +++++++++++++------------- 1 file changed, 73 insertions(+), 76 deletions(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index a76b6efee9..312dbc1bd9 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -309,19 +309,19 @@ namespace osu.Game.Screens.SelectV2 return true; case GlobalAction.SelectNext: - selectNext(1, isGroupSelection: false); - return true; - - case GlobalAction.SelectNextGroup: - selectNext(1, isGroupSelection: true); + traverseKeyboardSelection(1); return true; case GlobalAction.SelectPrevious: - selectNext(-1, isGroupSelection: false); + traverseKeyboardSelection(-1); + return true; + + case GlobalAction.SelectNextGroup: + traverseGroupSelection(1); return true; case GlobalAction.SelectPreviousGroup: - selectNext(-1, isGroupSelection: true); + traverseGroupSelection(-1); return true; } @@ -332,89 +332,86 @@ namespace osu.Game.Screens.SelectV2 { } - /// - /// Select the next valid selection relative to a current selection. - /// This is generally for keyboard based traversal. - /// - /// Positive for downwards, negative for upwards. - /// Whether the selection should traverse groups. Group selection updates the actual selection immediately, while non-group selection will only prepare a future keyboard selection. - /// Whether selection was possible. - private bool selectNext(int direction, bool isGroupSelection) + private void traverseKeyboardSelection(int direction) { - // Ensure sanity - Debug.Assert(direction != 0); - direction = direction > 0 ? 1 : -1; + if (carouselItems == null || carouselItems.Count == 0) return; - if (carouselItems == null || carouselItems.Count == 0) - return false; + int originalIndex; - // If the user has a different keyboard selection and requests - // group selection, first transfer the keyboard selection to actual selection. - if (isGroupSelection && currentSelection.CarouselItem != currentKeyboardSelection.CarouselItem) - { - TryActivateSelection(); - return true; - } + if (currentKeyboardSelection.Index != null) + originalIndex = currentKeyboardSelection.Index.Value; + else if (direction > 0) + originalIndex = carouselItems.Count - 1; + else + originalIndex = 0; - CarouselItem? selectionItem = currentKeyboardSelection.CarouselItem; - int selectionIndex = currentKeyboardSelection.Index ?? -1; - - // To keep things simple, let's first handle the cases where there's no selection yet. - if (selectionItem == null || selectionIndex < 0) - { - // Start by selecting the first item. - selectionItem = carouselItems.First(); - selectionIndex = 0; - - // In the forwards case, immediately attempt selection of this panel. - // If selection fails, continue with standard logic to find the next valid selection. - if (direction > 0 && attemptSelection(selectionItem)) - return true; - - // In the backwards direction we can just allow the selection logic to go ahead and loop around to the last valid. - } - - Debug.Assert(selectionItem != null); - - // As a second special case, if we're group selecting backwards and the current selection isn't a group, - // make sure to go back to the group header this item belongs to, so that the block below doesn't find it and stop too early. - if (isGroupSelection && direction < 0) - { - while (!CheckValidForGroupSelection(carouselItems[selectionIndex])) - selectionIndex--; - } - - CarouselItem? newItem; + int newIndex = originalIndex; // Iterate over every item back to the current selection, finding the first valid item. // The fail condition is when we reach the selection after a cyclic loop over every item. do { - selectionIndex += direction; - newItem = carouselItems[(selectionIndex + carouselItems.Count) % carouselItems.Count]; + newIndex = (newIndex + direction + carouselItems.Count) % carouselItems.Count; + var newItem = carouselItems[newIndex]; - if (attemptSelection(newItem)) - return true; - } while (newItem != selectionItem); + if (newItem.IsVisible) + { + setKeyboardSelection(newItem.Model); + return; + } + } while (newIndex != originalIndex); + } - return false; + /// + /// Select the next valid selection relative to a current selection. + /// This is generally for keyboard based traversal. + /// + /// Positive for downwards, negative for upwards. + /// Whether selection was possible. + private void traverseGroupSelection(int direction) + { + if (carouselItems == null || carouselItems.Count == 0) return; - bool attemptSelection(CarouselItem item) + // If the user has a different keyboard selection and requests + // group selection, first transfer the keyboard selection to actual selection. + if (currentSelection.CarouselItem != currentKeyboardSelection.CarouselItem) { - // Keyboard (non-group) selection should only consider visible items. - if (!isGroupSelection && !item.IsVisible) - return false; - - if (isGroupSelection && !CheckValidForGroupSelection(item)) - return false; - - if (isGroupSelection) - setSelection(item.Model); - else - setKeyboardSelection(item.Model); - - return true; + TryActivateSelection(); + return; } + + int originalIndex; + + if (currentKeyboardSelection.Index != null) + originalIndex = currentKeyboardSelection.Index.Value; + else if (direction > 0) + originalIndex = carouselItems.Count - 1; + else + originalIndex = 0; + + int newIndex = originalIndex; + + // As a second special case, if we're group selecting backwards and the current selection isn't a group, + // make sure to go back to the group header this item belongs to, so that the block below doesn't find it and stop too early. + if (direction < 0) + { + while (!CheckValidForGroupSelection(carouselItems[newIndex])) + newIndex--; + } + + // Iterate over every item back to the current selection, finding the first valid item. + // The fail condition is when we reach the selection after a cyclic loop over every item. + do + { + newIndex = (newIndex + direction + carouselItems.Count) % carouselItems.Count; + var newItem = carouselItems[newIndex]; + + if (CheckValidForGroupSelection(newItem)) + { + setSelection(newItem.Model); + return; + } + } while (newIndex != originalIndex); } #endregion From 9c34819ff4a533f8a39879dd8a5053676bff415a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 1 Feb 2025 14:55:48 +0900 Subject: [PATCH 09/20] Add test coverage for grouped selection --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 50 +++++++- ...estSceneBeatmapCarouselV2GroupSelection.cs | 121 ++++++++++++++++++ .../TestSceneBeatmapCarouselV2Selection.cs | 112 +++++++--------- osu.Game/Screens/SelectV2/Carousel.cs | 2 +- 4 files changed, 217 insertions(+), 68 deletions(-) create mode 100644 osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 281be924a1..5143d681a6 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -21,6 +21,7 @@ using osu.Game.Screens.SelectV2; using osu.Game.Tests.Beatmaps; using osu.Game.Tests.Resources; using osuTK.Graphics; +using osuTK.Input; using BeatmapCarousel = osu.Game.Screens.SelectV2.BeatmapCarousel; namespace osu.Game.Tests.Visual.SongSelect @@ -53,7 +54,7 @@ namespace osu.Game.Tests.Visual.SongSelect } [SetUpSteps] - public void SetUpSteps() + public virtual void SetUpSteps() { RemoveAllBeatmaps(); @@ -135,6 +136,53 @@ namespace osu.Game.Tests.Visual.SongSelect protected void WaitForSorting() => AddUntilStep("sorting finished", () => Carousel.IsFiltering, () => Is.False); protected void WaitForScrolling() => AddUntilStep("scroll finished", () => Scroll.Current, () => Is.EqualTo(Scroll.Target)); + protected void SelectNextPanel() => AddStep("select next panel", () => InputManager.Key(Key.Down)); + protected void SelectPrevPanel() => AddStep("select prev panel", () => InputManager.Key(Key.Up)); + protected void SelectNextGroup() => AddStep("select next group", () => InputManager.Key(Key.Right)); + protected void SelectPrevGroup() => AddStep("select prev group", () => InputManager.Key(Key.Left)); + + protected void Select() => AddStep("select", () => InputManager.Key(Key.Enter)); + + protected void CheckNoSelection() => AddAssert("has no selection", () => Carousel.CurrentSelection, () => Is.Null); + protected void CheckHasSelection() => AddAssert("has selection", () => Carousel.CurrentSelection, () => Is.Not.Null); + + protected void WaitForGroupSelection(int group, int panel) + { + AddUntilStep($"selected is group{group} panel{panel}", () => + { + var groupingFilter = Carousel.Filters.OfType().Single(); + + GroupDefinition g = groupingFilter.GroupItems.Keys.ElementAt(group); + CarouselItem item = groupingFilter.GroupItems[g].ElementAt(panel); + + return ReferenceEquals(Carousel.CurrentSelection, item.Model); + }); + } + + protected void WaitForSelection(int set, int? diff = null) + { + AddUntilStep($"selected is set{set}{(diff.HasValue ? $" diff{diff.Value}" : "")}", () => + { + if (diff != null) + return ReferenceEquals(Carousel.CurrentSelection, BeatmapSets[set].Beatmaps[diff.Value]); + + return BeatmapSets[set].Beatmaps.Contains(Carousel.CurrentSelection); + }); + } + + protected void ClickVisiblePanel(int index) + where T : Drawable + { + AddStep($"click panel at index {index}", () => + { + Carousel.ChildrenOfType() + .Where(p => ((ICarouselPanel)p).Item?.IsVisible == true) + .Reverse() + .ElementAt(index) + .TriggerClick(); + }); + } + /// /// Add requested beatmap sets count to list. /// diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs new file mode 100644 index 0000000000..bcb609500f --- /dev/null +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs @@ -0,0 +1,121 @@ +// Copyright (c) ppy Pty Ltd . 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.Testing; +using osu.Game.Beatmaps; +using osu.Game.Screens.Select; +using osu.Game.Screens.Select.Filter; +using osu.Game.Screens.SelectV2; + +namespace osu.Game.Tests.Visual.SongSelect +{ + [TestFixture] + public partial class TestSceneBeatmapCarouselV2GroupSelection : BeatmapCarouselV2TestScene + { + public override void SetUpSteps() + { + RemoveAllBeatmaps(); + + CreateCarousel(); + + SortBy(new FilterCriteria { Sort = SortMode.Difficulty }); + } + + [Test] + public void TestOpenCloseGroupWithNoSelection() + { + AddBeatmaps(10, 5); + WaitForDrawablePanels(); + + AddAssert("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + + ClickVisiblePanel(0); + AddUntilStep("some beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); + CheckNoSelection(); + + ClickVisiblePanel(0); + AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + } + + [Test] + public void TestCarouselRemembersSelection() + { + AddBeatmaps(10); + WaitForDrawablePanels(); + + SelectNextGroup(); + + object? selection = null; + + AddStep("store drawable selection", () => selection = getSelectedPanel()?.Item?.Model); + + CheckHasSelection(); + AddAssert("drawable selection non-null", () => selection, () => Is.Not.Null); + AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); + + RemoveAllBeatmaps(); + AddUntilStep("no drawable selection", getSelectedPanel, () => Is.Null); + + AddBeatmaps(10); + WaitForDrawablePanels(); + + CheckHasSelection(); + AddAssert("no drawable selection", getSelectedPanel, () => Is.Null); + + AddStep("add previous selection", () => BeatmapSets.Add(((BeatmapInfo)selection!).BeatmapSet!)); + + AddAssert("selection matches original carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); + AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); + AddAssert("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); + + ClickVisiblePanel(0); + AddUntilStep("carousel item not visible", getSelectedPanel, () => Is.Null); + + ClickVisiblePanel(0); + AddUntilStep("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); + + BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); + } + + [Test] + public void TestKeyboardSelection() + { + AddBeatmaps(10, 3); + WaitForDrawablePanels(); + + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + CheckNoSelection(); + + // open first group + Select(); + CheckNoSelection(); + AddUntilStep("some beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); + + SelectNextPanel(); + Select(); + WaitForGroupSelection(0, 0); + + SelectNextGroup(); + WaitForGroupSelection(0, 1); + + SelectNextGroup(); + WaitForGroupSelection(0, 2); + + SelectPrevGroup(); + WaitForGroupSelection(0, 1); + + SelectPrevGroup(); + WaitForGroupSelection(0, 0); + + SelectPrevGroup(); + WaitForGroupSelection(2, 9); + } + } +} diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs index 3c42969d8c..50395cf1ff 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs @@ -22,10 +22,10 @@ namespace osu.Game.Tests.Visual.SongSelect { AddBeatmaps(10); WaitForDrawablePanels(); - checkNoSelection(); + CheckNoSelection(); - select(); - checkNoSelection(); + Select(); + CheckNoSelection(); AddStep("press down arrow", () => InputManager.PressKey(Key.Down)); checkSelectionIterating(false); @@ -39,8 +39,8 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep("release up arrow", () => InputManager.ReleaseKey(Key.Up)); checkSelectionIterating(false); - select(); - checkHasSelection(); + Select(); + CheckHasSelection(); } /// @@ -52,7 +52,7 @@ namespace osu.Game.Tests.Visual.SongSelect { AddBeatmaps(10); WaitForDrawablePanels(); - checkNoSelection(); + CheckNoSelection(); AddStep("press right arrow", () => InputManager.PressKey(Key.Right)); checkSelectionIterating(true); @@ -73,13 +73,13 @@ namespace osu.Game.Tests.Visual.SongSelect AddBeatmaps(10); WaitForDrawablePanels(); - selectNextGroup(); + SelectNextGroup(); object? selection = null; AddStep("store drawable selection", () => selection = getSelectedPanel()?.Item?.Model); - checkHasSelection(); + CheckHasSelection(); AddAssert("drawable selection non-null", () => selection, () => Is.Not.Null); AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); @@ -89,13 +89,14 @@ namespace osu.Game.Tests.Visual.SongSelect AddBeatmaps(10); WaitForDrawablePanels(); - checkHasSelection(); + CheckHasSelection(); AddAssert("no drawable selection", getSelectedPanel, () => Is.Null); AddStep("add previous selection", () => BeatmapSets.Add(((BeatmapInfo)selection!).BeatmapSet!)); + AddAssert("selection matches original carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); - AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); + AddAssert("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); } @@ -108,10 +109,10 @@ namespace osu.Game.Tests.Visual.SongSelect AddBeatmaps(total_set_count); WaitForDrawablePanels(); - selectNextGroup(); - waitForSelection(0, 0); - selectPrevGroup(); - waitForSelection(total_set_count - 1, 0); + SelectNextGroup(); + WaitForSelection(0, 0); + SelectPrevGroup(); + WaitForSelection(total_set_count - 1, 0); } [Test] @@ -122,10 +123,10 @@ namespace osu.Game.Tests.Visual.SongSelect AddBeatmaps(total_set_count); WaitForDrawablePanels(); - selectPrevGroup(); - waitForSelection(total_set_count - 1, 0); - selectNextGroup(); - waitForSelection(0, 0); + SelectPrevGroup(); + WaitForSelection(total_set_count - 1, 0); + SelectNextGroup(); + WaitForSelection(0, 0); } [Test] @@ -134,71 +135,50 @@ namespace osu.Game.Tests.Visual.SongSelect AddBeatmaps(10, 3); WaitForDrawablePanels(); - selectNextPanel(); - selectNextPanel(); - selectNextPanel(); - selectNextPanel(); - checkNoSelection(); + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + CheckNoSelection(); - select(); - waitForSelection(3, 0); + Select(); + WaitForSelection(3, 0); - selectNextPanel(); - waitForSelection(3, 0); + SelectNextPanel(); + WaitForSelection(3, 0); - select(); - waitForSelection(3, 1); + Select(); + WaitForSelection(3, 1); - selectNextPanel(); - waitForSelection(3, 1); + SelectNextPanel(); + WaitForSelection(3, 1); - select(); - waitForSelection(3, 2); + Select(); + WaitForSelection(3, 2); - selectNextPanel(); - waitForSelection(3, 2); + SelectNextPanel(); + WaitForSelection(3, 2); - select(); - waitForSelection(4, 0); + Select(); + WaitForSelection(4, 0); } [Test] public void TestEmptyTraversal() { - selectNextPanel(); - checkNoSelection(); + SelectNextPanel(); + CheckNoSelection(); - selectNextGroup(); - checkNoSelection(); + SelectNextGroup(); + CheckNoSelection(); - selectPrevPanel(); - checkNoSelection(); + SelectPrevPanel(); + CheckNoSelection(); - selectPrevGroup(); - checkNoSelection(); + SelectPrevGroup(); + CheckNoSelection(); } - private void waitForSelection(int set, int? diff = null) - { - AddUntilStep($"selected is set{set}{(diff.HasValue ? $" diff{diff.Value}" : "")}", () => - { - if (diff != null) - return ReferenceEquals(Carousel.CurrentSelection, BeatmapSets[set].Beatmaps[diff.Value]); - - return BeatmapSets[set].Beatmaps.Contains(Carousel.CurrentSelection); - }); - } - - private void selectNextPanel() => AddStep("select next panel", () => InputManager.Key(Key.Down)); - private void selectPrevPanel() => AddStep("select prev panel", () => InputManager.Key(Key.Up)); - private void selectNextGroup() => AddStep("select next group", () => InputManager.Key(Key.Right)); - private void selectPrevGroup() => AddStep("select prev group", () => InputManager.Key(Key.Left)); - - private void select() => AddStep("select", () => InputManager.Key(Key.Enter)); - - private void checkNoSelection() => AddAssert("has no selection", () => Carousel.CurrentSelection, () => Is.Null); - private void checkHasSelection() => AddAssert("has selection", () => Carousel.CurrentSelection, () => Is.Not.Null); - private void checkSelectionIterating(bool isIterating) { object? selection = null; diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 312dbc1bd9..0da9cb5c19 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -131,7 +131,7 @@ namespace osu.Game.Screens.SelectV2 /// /// A filter may add, mutate or remove items. /// - protected IEnumerable Filters { get; init; } = Enumerable.Empty(); + public IEnumerable Filters { get; init; } = Enumerable.Empty(); /// /// All items which are to be considered for display in this carousel. From 6a18d18feb0ada227cb85fdb9144439196b3cef7 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 2 Feb 2025 13:28:31 +0900 Subject: [PATCH 10/20] Fix null handling when no items are populated but a selection is made --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 10bc069cfc..858888c517 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -124,9 +124,10 @@ namespace osu.Game.Screens.SelectV2 if (Criteria.SplitOutDifficulties) { // Find the containing group. There should never be too many groups so iterating is efficient enough. - GroupDefinition group = grouping.GroupItems.Single(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; + GroupDefinition? group = grouping.GroupItems.SingleOrDefault(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; - setVisibleGroup(group); + if (group != null) + setVisibleGroup(group); } else { From c587958f387db1287218801292a1ed9480d8edef Mon Sep 17 00:00:00 2001 From: Salman Alshamrani Date: Sat, 1 Feb 2025 03:01:47 -0500 Subject: [PATCH 11/20] Apply depth ordering relative to selected item --- osu.Game/Screens/SelectV2/Carousel.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 648c2d090a..f41154b878 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -544,8 +544,8 @@ namespace osu.Game.Screens.SelectV2 if (c.Item == null) continue; - if (panel.Depth != c.DrawYPosition) - scroll.Panels.ChangeChildDepth(panel, (float)c.DrawYPosition); + double selectedYPos = currentSelection?.CarouselItem?.CarouselYPosition ?? 0; + scroll.Panels.ChangeChildDepth(panel, (float)Math.Abs(c.DrawYPosition - selectedYPos)); if (c.DrawYPosition != c.Item.CarouselYPosition) c.DrawYPosition = Interpolation.DampContinuously(c.DrawYPosition, c.Item.CarouselYPosition, 50, Time.Elapsed); From c7780c9fdca97525d2f20920bc44951b652e4854 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 3 Feb 2025 19:53:46 +0900 Subject: [PATCH 12/20] Refactor how grouping is performed --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 2 +- .../TestSceneBeatmapCarouselV2Basics.cs | 2 +- ...estSceneBeatmapCarouselV2GroupSelection.cs | 2 +- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 133 +++++++++++------- 4 files changed, 85 insertions(+), 54 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 5143d681a6..0a9719423c 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -130,7 +130,7 @@ namespace osu.Game.Tests.Visual.SongSelect }); } - protected void SortBy(FilterCriteria criteria) => AddStep($"sort by {criteria.Sort}", () => Carousel.Filter(criteria)); + protected void SortBy(FilterCriteria criteria) => AddStep($"sort {criteria.Sort} group {criteria.Group}", () => Carousel.Filter(criteria)); protected void WaitForDrawablePanels() => AddUntilStep("drawable panels loaded", () => Carousel.ChildrenOfType().Count(), () => Is.GreaterThan(0)); protected void WaitForSorting() => AddUntilStep("sorting finished", () => Carousel.IsFiltering, () => Is.False); diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs index 0e72ee4f8c..8ffb51b995 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs @@ -50,7 +50,7 @@ namespace osu.Game.Tests.Visual.SongSelect public void TestSorting() { AddBeatmaps(10); - SortBy(new FilterCriteria { Sort = SortMode.Difficulty }); + SortBy(new FilterCriteria { Group = GroupMode.Difficulty, Sort = SortMode.Difficulty }); SortBy(new FilterCriteria { Sort = SortMode.Artist }); } diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs index bcb609500f..5728583507 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs @@ -20,7 +20,7 @@ namespace osu.Game.Tests.Visual.SongSelect CreateCarousel(); - SortBy(new FilterCriteria { Sort = SortMode.Difficulty }); + SortBy(new FilterCriteria { Group = GroupMode.Difficulty, Sort = SortMode.Difficulty }); } [Test] diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index 951b010564..34fbfdbaa6 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -8,6 +8,7 @@ using System.Threading; using System.Threading.Tasks; using osu.Game.Beatmaps; using osu.Game.Screens.Select; +using osu.Game.Screens.Select.Filter; namespace osu.Game.Screens.SelectV2 { @@ -35,70 +36,100 @@ namespace osu.Game.Screens.SelectV2 public async Task> Run(IEnumerable items, CancellationToken cancellationToken) => await Task.Run(() => { + bool groupSetsTogether; + setItems.Clear(); groupItems.Clear(); var criteria = getCriteria(); - - int starGroup = int.MinValue; - - if (criteria.SplitOutDifficulties) - { - var diffItems = new List(items.Count()); - - GroupDefinition? group = null; - - foreach (var item in items) - { - var b = (BeatmapInfo)item.Model; - - if (b.StarRating > starGroup) - { - starGroup = (int)Math.Floor(b.StarRating); - group = new GroupDefinition($"{starGroup} - {++starGroup} *"); - diffItems.Add(new CarouselItem(group) { DrawHeight = GroupPanel.HEIGHT }); - } - - if (!groupItems.TryGetValue(group!, out var related)) - groupItems[group!] = related = new HashSet(); - related.Add(item); - - diffItems.Add(item); - - item.IsVisible = false; - } - - return diffItems; - } - - CarouselItem? lastItem = null; - var newItems = new List(items.Count()); - foreach (var item in items) + // Add criteria groups. + switch (criteria.Group) + { + default: + groupSetsTogether = true; + newItems.AddRange(items); + break; + + case GroupMode.Difficulty: + groupSetsTogether = false; + int starGroup = int.MinValue; + + foreach (var item in items) + { + cancellationToken.ThrowIfCancellationRequested(); + + var b = (BeatmapInfo)item.Model; + + if (b.StarRating > starGroup) + { + starGroup = (int)Math.Floor(b.StarRating); + newItems.Add(new CarouselItem(new GroupDefinition($"{starGroup} - {++starGroup} *")) { DrawHeight = GroupPanel.HEIGHT }); + } + + newItems.Add(item); + } + + break; + } + + // Add set headers wherever required. + CarouselItem? lastItem = null; + + if (groupSetsTogether) + { + for (int i = 0; i < newItems.Count; i++) + { + cancellationToken.ThrowIfCancellationRequested(); + + var item = newItems[i]; + + if (item.Model is BeatmapInfo beatmap) + { + if (groupSetsTogether) + { + bool newBeatmapSet = lastItem == null || (lastItem.Model is BeatmapInfo lastBeatmap && lastBeatmap.BeatmapSet!.ID != beatmap.BeatmapSet!.ID); + + if (newBeatmapSet) + { + newItems.Insert(i, new CarouselItem(beatmap.BeatmapSet!) { DrawHeight = BeatmapSetPanel.HEIGHT }); + i++; + } + + if (!setItems.TryGetValue(beatmap.BeatmapSet!, out var related)) + setItems[beatmap.BeatmapSet!] = related = new HashSet(); + + related.Add(item); + item.IsVisible = false; + } + } + + lastItem = item; + } + } + + // Link group items to their headers. + GroupDefinition? lastGroup = null; + + foreach (var item in newItems) { cancellationToken.ThrowIfCancellationRequested(); - if (item.Model is BeatmapInfo b) + if (item.Model is GroupDefinition group) { - // Add set header - if (lastItem == null || (lastItem.Model is BeatmapInfo b2 && b2.BeatmapSet!.OnlineID != b.BeatmapSet!.OnlineID)) - { - newItems.Add(new CarouselItem(b.BeatmapSet!) - { - DrawHeight = BeatmapSetPanel.HEIGHT, - }); - } - - if (!setItems.TryGetValue(b.BeatmapSet!, out var related)) - setItems[b.BeatmapSet!] = related = new HashSet(); - related.Add(item); + lastGroup = group; + continue; } - newItems.Add(item); - lastItem = item; + if (lastGroup != null) + { + if (!groupItems.TryGetValue(lastGroup, out var groupRelated)) + groupItems[lastGroup] = groupRelated = new HashSet(); + groupRelated.Add(item); - item.IsVisible = false; + item.IsVisible = false; + } } return newItems; From b433eef1389ae8a07627ee6a9597bebe336d61c6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 01:51:43 +0900 Subject: [PATCH 13/20] Remove redundant conditional check --- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index 34fbfdbaa6..ea737d8b7f 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -87,22 +87,19 @@ namespace osu.Game.Screens.SelectV2 if (item.Model is BeatmapInfo beatmap) { - if (groupSetsTogether) + bool newBeatmapSet = lastItem == null || (lastItem.Model is BeatmapInfo lastBeatmap && lastBeatmap.BeatmapSet!.ID != beatmap.BeatmapSet!.ID); + + if (newBeatmapSet) { - bool newBeatmapSet = lastItem == null || (lastItem.Model is BeatmapInfo lastBeatmap && lastBeatmap.BeatmapSet!.ID != beatmap.BeatmapSet!.ID); - - if (newBeatmapSet) - { - newItems.Insert(i, new CarouselItem(beatmap.BeatmapSet!) { DrawHeight = BeatmapSetPanel.HEIGHT }); - i++; - } - - if (!setItems.TryGetValue(beatmap.BeatmapSet!, out var related)) - setItems[beatmap.BeatmapSet!] = related = new HashSet(); - - related.Add(item); - item.IsVisible = false; + newItems.Insert(i, new CarouselItem(beatmap.BeatmapSet!) { DrawHeight = BeatmapSetPanel.HEIGHT }); + i++; } + + if (!setItems.TryGetValue(beatmap.BeatmapSet!, out var related)) + setItems[beatmap.BeatmapSet!] = related = new HashSet(); + + related.Add(item); + item.IsVisible = false; } lastItem = item; From b5c4e3bc147e0c4f085de754ed8019dc18ead270 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 02:41:56 +0900 Subject: [PATCH 14/20] Add failing tests for traversal on group headers --- .../TestSceneBeatmapCarouselV2GroupSelection.cs | 14 ++++++++++++++ .../TestSceneBeatmapCarouselV2Selection.cs | 15 +++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs index 5728583507..04ca0a9085 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs @@ -81,6 +81,20 @@ namespace osu.Game.Tests.Visual.SongSelect BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); } + [Test] + public void TestGroupSelectionOnHeader() + { + AddBeatmaps(10, 3); + WaitForDrawablePanels(); + + SelectNextGroup(); + WaitForGroupSelection(0, 0); + + SelectPrevPanel(); + SelectPrevGroup(); + WaitForGroupSelection(2, 9); + } + [Test] public void TestKeyboardSelection() { diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs index 50395cf1ff..b087c252e4 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs @@ -129,6 +129,21 @@ namespace osu.Game.Tests.Visual.SongSelect WaitForSelection(0, 0); } + [Test] + public void TestGroupSelectionOnHeader() + { + AddBeatmaps(10, 3); + WaitForDrawablePanels(); + + SelectNextGroup(); + SelectNextGroup(); + WaitForSelection(1, 0); + + SelectPrevPanel(); + SelectPrevGroup(); + WaitForSelection(0, 0); + } + [Test] public void TestKeyboardSelection() { From e454fa558cb5891ac6614dd9c626fa21834c168f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 02:55:57 +0900 Subject: [PATCH 15/20] Adjust group traversal logic to handle cases where keyboard selection redirects --- osu.Game/Screens/SelectV2/Carousel.cs | 35 +++++++++++++++------------ 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 0da9cb5c19..a13de0e26d 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -377,26 +377,31 @@ namespace osu.Game.Screens.SelectV2 if (currentSelection.CarouselItem != currentKeyboardSelection.CarouselItem) { TryActivateSelection(); - return; + + // There's a chance this couldn't resolve, at which point continue with standard traversal. + if (currentSelection.CarouselItem == currentKeyboardSelection.CarouselItem) + return; } int originalIndex; + int newIndex; - if (currentKeyboardSelection.Index != null) - originalIndex = currentKeyboardSelection.Index.Value; - else if (direction > 0) - originalIndex = carouselItems.Count - 1; - else - originalIndex = 0; - - int newIndex = originalIndex; - - // As a second special case, if we're group selecting backwards and the current selection isn't a group, - // make sure to go back to the group header this item belongs to, so that the block below doesn't find it and stop too early. - if (direction < 0) + if (currentSelection.Index == null) { - while (!CheckValidForGroupSelection(carouselItems[newIndex])) - newIndex--; + // If there's no current selection, start from either end of the full list. + newIndex = originalIndex = direction > 0 ? carouselItems.Count - 1 : 0; + } + else + { + newIndex = originalIndex = currentSelection.Index.Value; + + // As a second special case, if we're group selecting backwards and the current selection isn't a group, + // make sure to go back to the group header this item belongs to, so that the block below doesn't find it and stop too early. + if (direction < 0) + { + while (!CheckValidForGroupSelection(carouselItems[newIndex])) + newIndex--; + } } // Iterate over every item back to the current selection, finding the first valid item. From 38933039880b3b50eaef5557290a9c806dd79f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 17 Oct 2024 11:59:27 +0200 Subject: [PATCH 16/20] Implement "form button" control --- .../UserInterface/TestSceneFormControls.cs | 166 ++++++++------- .../Graphics/UserInterfaceV2/FormButton.cs | 189 ++++++++++++++++++ 2 files changed, 280 insertions(+), 75 deletions(-) create mode 100644 osu.Game/Graphics/UserInterfaceV2/FormButton.cs diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs index 118fbca97b..2003f5de83 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs @@ -6,6 +6,7 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Cursor; using osu.Game.Beatmaps; +using osu.Game.Graphics.Containers; using osu.Game.Graphics.Cursor; using osu.Game.Graphics.UserInterfaceV2; using osu.Game.Localisation; @@ -27,87 +28,102 @@ namespace osu.Game.Tests.Visual.UserInterface Child = new PopoverContainer { RelativeSizeAxes = Axes.Both, - Child = new FillFlowContainer + Child = new OsuScrollContainer { - RelativeSizeAxes = Axes.Y, - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - Width = 400, - Direction = FillDirection.Vertical, - Spacing = new Vector2(5), - Padding = new MarginPadding(10), - Children = new Drawable[] + RelativeSizeAxes = Axes.Both, + Child = new FillFlowContainer { - new FormTextBox + AutoSizeAxes = Axes.Y, + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Width = 400, + Direction = FillDirection.Vertical, + Spacing = new Vector2(5), + Padding = new MarginPadding(10), + Children = new Drawable[] { - Caption = "Artist", - HintText = "Poot artist here!", - PlaceholderText = "Here is an artist", - TabbableContentContainer = this, - }, - new FormTextBox - { - Caption = "Artist", - HintText = "Poot artist here!", - PlaceholderText = "Here is an artist", - Current = { Disabled = true }, - TabbableContentContainer = this, - }, - new FormNumberBox(allowDecimals: true) - { - Caption = "Number", - HintText = "Insert your favourite number", - PlaceholderText = "Mine is 42!", - TabbableContentContainer = this, - }, - new FormCheckBox - { - Caption = EditorSetupStrings.LetterboxDuringBreaks, - HintText = EditorSetupStrings.LetterboxDuringBreaksDescription, - }, - new FormCheckBox - { - Caption = EditorSetupStrings.LetterboxDuringBreaks, - HintText = EditorSetupStrings.LetterboxDuringBreaksDescription, - Current = { Disabled = true }, - }, - new FormSliderBar - { - Caption = "Slider", - Current = new BindableFloat + new FormTextBox { - MinValue = 0, - MaxValue = 10, - Value = 5, - Precision = 0.1f, + Caption = "Artist", + HintText = "Poot artist here!", + PlaceholderText = "Here is an artist", + TabbableContentContainer = this, }, - TabbableContentContainer = this, - }, - new FormEnumDropdown - { - Caption = EditorSetupStrings.EnableCountdown, - HintText = EditorSetupStrings.CountdownDescription, - }, - new FormFileSelector - { - Caption = "File selector", - PlaceholderText = "Select a file", - }, - new FormBeatmapFileSelector(true) - { - Caption = "File selector with intermediate choice dialog", - PlaceholderText = "Select a file", - }, - new FormColourPalette - { - Caption = "Combo colours", - Colours = + new FormTextBox { - Colour4.Red, - Colour4.Green, - Colour4.Blue, - Colour4.Yellow, - } + Caption = "Artist", + HintText = "Poot artist here!", + PlaceholderText = "Here is an artist", + Current = { Disabled = true }, + TabbableContentContainer = this, + }, + new FormNumberBox(allowDecimals: true) + { + Caption = "Number", + HintText = "Insert your favourite number", + PlaceholderText = "Mine is 42!", + TabbableContentContainer = this, + }, + new FormCheckBox + { + Caption = EditorSetupStrings.LetterboxDuringBreaks, + HintText = EditorSetupStrings.LetterboxDuringBreaksDescription, + }, + new FormCheckBox + { + Caption = EditorSetupStrings.LetterboxDuringBreaks, + HintText = EditorSetupStrings.LetterboxDuringBreaksDescription, + Current = { Disabled = true }, + }, + new FormSliderBar + { + Caption = "Slider", + Current = new BindableFloat + { + MinValue = 0, + MaxValue = 10, + Value = 5, + Precision = 0.1f, + }, + TabbableContentContainer = this, + }, + new FormEnumDropdown + { + Caption = EditorSetupStrings.EnableCountdown, + HintText = EditorSetupStrings.CountdownDescription, + }, + new FormFileSelector + { + Caption = "File selector", + PlaceholderText = "Select a file", + }, + new FormBeatmapFileSelector(true) + { + Caption = "File selector with intermediate choice dialog", + PlaceholderText = "Select a file", + }, + new FormColourPalette + { + Caption = "Combo colours", + Colours = + { + Colour4.Red, + Colour4.Green, + Colour4.Blue, + Colour4.Yellow, + } + }, + new FormButton + { + Caption = "No text in button", + Action = () => { }, + }, + new FormButton + { + Caption = "Text in button which is pretty long and is very likely to wrap", + ButtonText = "Foo the bar", + Action = () => { }, + }, }, }, }, diff --git a/osu.Game/Graphics/UserInterfaceV2/FormButton.cs b/osu.Game/Graphics/UserInterfaceV2/FormButton.cs new file mode 100644 index 0000000000..fec855153b --- /dev/null +++ b/osu.Game/Graphics/UserInterfaceV2/FormButton.cs @@ -0,0 +1,189 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using System.Diagnostics; +using osu.Framework.Allocation; +using osu.Framework.Extensions.Color4Extensions; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Colour; +using osu.Framework.Graphics.Containers; +using osu.Framework.Graphics.Shapes; +using osu.Framework.Graphics.Sprites; +using osu.Framework.Input.Events; +using osu.Framework.Localisation; +using osu.Game.Graphics.Backgrounds; +using osu.Game.Graphics.Containers; +using osu.Game.Graphics.UserInterface; +using osu.Game.Overlays; +using osuTK; +using osuTK.Graphics; + +namespace osu.Game.Graphics.UserInterfaceV2 +{ + public partial class FormButton : CompositeDrawable + { + /// + /// Caption describing this button, displayed on the left of it. + /// + public LocalisableString Caption { get; init; } + + public LocalisableString ButtonText { get; init; } + + public Action? Action { get; init; } + + [Resolved] + private OverlayColourProvider colourProvider { get; set; } = null!; + + [BackgroundDependencyLoader] + private void load() + { + RelativeSizeAxes = Axes.X; + Height = 50; + + Masking = true; + CornerRadius = 5; + CornerExponent = 2.5f; + + InternalChildren = new Drawable[] + { + new Box + { + RelativeSizeAxes = Axes.Both, + Colour = colourProvider.Background5, + }, + new Container + { + RelativeSizeAxes = Axes.Both, + Padding = new MarginPadding + { + Left = 9, + Right = 5, + Vertical = 5, + }, + Children = new Drawable[] + { + new OsuTextFlowContainer + { + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y, + Width = 0.45f, + Anchor = Anchor.CentreLeft, + Origin = Anchor.CentreLeft, + Text = Caption, + }, + new Button + { + Action = Action, + Text = ButtonText, + RelativeSizeAxes = ButtonText == default ? Axes.None : Axes.X, + Width = ButtonText == default ? 90 : 0.45f, + Anchor = Anchor.CentreRight, + Origin = Anchor.CentreRight, + } + }, + }, + }; + } + + protected override bool OnHover(HoverEvent e) + { + updateState(); + return true; + } + + protected override void OnHoverLost(HoverLostEvent e) + { + base.OnHoverLost(e); + updateState(); + } + + private void updateState() + { + BorderThickness = IsHovered ? 2 : 0; + + if (IsHovered) + BorderColour = colourProvider.Light4; + } + + public partial class Button : OsuButton + { + private TrianglesV2? triangles { get; set; } + + protected override float HoverLayerFinalAlpha => 0; + + private Color4? triangleGradientSecondColour; + + public override Color4 BackgroundColour + { + get => base.BackgroundColour; + set + { + base.BackgroundColour = value; + triangleGradientSecondColour = BackgroundColour.Lighten(0.2f); + updateColours(); + } + } + + [BackgroundDependencyLoader] + private void load(OverlayColourProvider overlayColourProvider) + { + DefaultBackgroundColour = overlayColourProvider.Colour3; + triangleGradientSecondColour ??= overlayColourProvider.Colour1; + + if (Text == default) + { + Add(new SpriteIcon + { + Icon = FontAwesome.Solid.ChevronRight, + Size = new Vector2(16), + Shadow = true, + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + }); + } + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + Content.CornerRadius = 2; + + Add(triangles = new TrianglesV2 + { + Thickness = 0.02f, + SpawnRatio = 0.6f, + RelativeSizeAxes = Axes.Both, + Depth = float.MaxValue, + }); + + updateColours(); + } + + private void updateColours() + { + if (triangles == null) + return; + + Debug.Assert(triangleGradientSecondColour != null); + + triangles.Colour = ColourInfo.GradientVertical(triangleGradientSecondColour.Value, BackgroundColour); + } + + protected override bool OnHover(HoverEvent e) + { + Debug.Assert(triangleGradientSecondColour != null); + + Background.FadeColour(triangleGradientSecondColour.Value, 300, Easing.OutQuint); + return base.OnHover(e); + } + + protected override void OnHoverLost(HoverLostEvent e) + { + Background.FadeColour(BackgroundColour, 300, Easing.OutQuint); + base.OnHoverLost(e); + } + } + } +} From 2f2dc158e0353aa5ba27108980a1bed1466a2f36 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 17:44:59 +0900 Subject: [PATCH 17/20] Ensure test step doesn't consider pooled instances of drawables --- osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 0a9719423c..2e67e625f9 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -175,7 +175,8 @@ namespace osu.Game.Tests.Visual.SongSelect { AddStep($"click panel at index {index}", () => { - Carousel.ChildrenOfType() + Carousel.ChildrenOfType().Single() + .ChildrenOfType() .Where(p => ((ICarouselPanel)p).Item?.IsVisible == true) .Reverse() .ElementAt(index) From ccdb6e4c4870ef64b3a2e549716c4bf7b412b646 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 17:50:14 +0900 Subject: [PATCH 18/20] Fix carousel tests failing due to dependency on depth ordering --- osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 2e67e625f9..f7be5f12e8 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -178,7 +178,7 @@ namespace osu.Game.Tests.Visual.SongSelect Carousel.ChildrenOfType().Single() .ChildrenOfType() .Where(p => ((ICarouselPanel)p).Item?.IsVisible == true) - .Reverse() + .OrderBy(p => p.Y) .ElementAt(index) .TriggerClick(); }); From 58560f8acfe0259795358e969ddee6ca0600d2ad Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 17:11:09 +0900 Subject: [PATCH 19/20] Add tracking of expansion states for groups and sets --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 3 +- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 38 ++++++++++++------- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 20 +++++----- osu.Game/Screens/SelectV2/CarouselItem.cs | 7 +++- 4 files changed, 42 insertions(+), 26 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index f7be5f12e8..72c9611fdb 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -153,7 +153,8 @@ namespace osu.Game.Tests.Visual.SongSelect var groupingFilter = Carousel.Filters.OfType().Single(); GroupDefinition g = groupingFilter.GroupItems.Keys.ElementAt(group); - CarouselItem item = groupingFilter.GroupItems[g].ElementAt(panel); + // offset by one because the group itself is included in the items list. + CarouselItem item = groupingFilter.GroupItems[g].ElementAt(panel + 1); return ReferenceEquals(Carousel.CurrentSelection, item.Model); }); diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 858888c517..9f62780dda 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -105,12 +105,12 @@ namespace osu.Game.Screens.SelectV2 // Special case – collapsing an open group. if (lastSelectedGroup == group) { - setVisibilityOfGroupItems(lastSelectedGroup, false); + setExpansionStateOfGroup(lastSelectedGroup, false); lastSelectedGroup = null; return false; } - setVisibleGroup(group); + setExpandedGroup(group); return false; case BeatmapSetInfo setInfo: @@ -127,11 +127,11 @@ namespace osu.Game.Screens.SelectV2 GroupDefinition? group = grouping.GroupItems.SingleOrDefault(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; if (group != null) - setVisibleGroup(group); + setExpandedGroup(group); } else { - setVisibleSet(beatmapInfo); + setExpandedSet(beatmapInfo); } return true; @@ -158,37 +158,47 @@ namespace osu.Game.Screens.SelectV2 } } - private void setVisibleGroup(GroupDefinition group) + private void setExpandedGroup(GroupDefinition group) { if (lastSelectedGroup != null) - setVisibilityOfGroupItems(lastSelectedGroup, false); + setExpansionStateOfGroup(lastSelectedGroup, false); lastSelectedGroup = group; - setVisibilityOfGroupItems(group, true); + setExpansionStateOfGroup(group, true); } - private void setVisibilityOfGroupItems(GroupDefinition group, bool visible) + private void setExpansionStateOfGroup(GroupDefinition group, bool expanded) { if (grouping.GroupItems.TryGetValue(group, out var items)) { foreach (var i in items) - i.IsVisible = visible; + { + if (i.Model is GroupDefinition) + i.IsExpanded = expanded; + else + i.IsVisible = expanded; + } } } - private void setVisibleSet(BeatmapInfo beatmapInfo) + private void setExpandedSet(BeatmapInfo beatmapInfo) { if (lastSelectedBeatmap != null) - setVisibilityOfSetItems(lastSelectedBeatmap.BeatmapSet!, false); + setExpansionStateOfSetItems(lastSelectedBeatmap.BeatmapSet!, false); lastSelectedBeatmap = beatmapInfo; - setVisibilityOfSetItems(beatmapInfo.BeatmapSet!, true); + setExpansionStateOfSetItems(beatmapInfo.BeatmapSet!, true); } - private void setVisibilityOfSetItems(BeatmapSetInfo set, bool visible) + private void setExpansionStateOfSetItems(BeatmapSetInfo set, bool expanded) { if (grouping.SetItems.TryGetValue(set, out var items)) { foreach (var i in items) - i.IsVisible = visible; + { + if (i.Model is BeatmapSetInfo) + i.IsExpanded = expanded; + else + i.IsVisible = expanded; + } } } diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index ea737d8b7f..e4160cc0fa 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -65,7 +65,11 @@ namespace osu.Game.Screens.SelectV2 if (b.StarRating > starGroup) { starGroup = (int)Math.Floor(b.StarRating); - newItems.Add(new CarouselItem(new GroupDefinition($"{starGroup} - {++starGroup} *")) { DrawHeight = GroupPanel.HEIGHT }); + var groupDefinition = new GroupDefinition($"{starGroup} - {++starGroup} *"); + var groupItem = new CarouselItem(groupDefinition) { DrawHeight = GroupPanel.HEIGHT }; + + newItems.Add(groupItem); + groupItems[groupDefinition] = new HashSet { groupItem }; } newItems.Add(item); @@ -91,14 +95,13 @@ namespace osu.Game.Screens.SelectV2 if (newBeatmapSet) { - newItems.Insert(i, new CarouselItem(beatmap.BeatmapSet!) { DrawHeight = BeatmapSetPanel.HEIGHT }); + var setItem = new CarouselItem(beatmap.BeatmapSet!) { DrawHeight = BeatmapSetPanel.HEIGHT }; + setItems[beatmap.BeatmapSet!] = new HashSet { setItem }; + newItems.Insert(i, setItem); i++; } - if (!setItems.TryGetValue(beatmap.BeatmapSet!, out var related)) - setItems[beatmap.BeatmapSet!] = related = new HashSet(); - - related.Add(item); + setItems[beatmap.BeatmapSet!].Add(item); item.IsVisible = false; } @@ -121,10 +124,7 @@ namespace osu.Game.Screens.SelectV2 if (lastGroup != null) { - if (!groupItems.TryGetValue(lastGroup, out var groupRelated)) - groupItems[lastGroup] = groupRelated = new HashSet(); - groupRelated.Add(item); - + groupItems[lastGroup].Add(item); item.IsVisible = false; } } diff --git a/osu.Game/Screens/SelectV2/CarouselItem.cs b/osu.Game/Screens/SelectV2/CarouselItem.cs index 13d5c840cf..32be33e99a 100644 --- a/osu.Game/Screens/SelectV2/CarouselItem.cs +++ b/osu.Game/Screens/SelectV2/CarouselItem.cs @@ -30,10 +30,15 @@ namespace osu.Game.Screens.SelectV2 public float DrawHeight { get; set; } = DEFAULT_HEIGHT; /// - /// Whether this item is visible or collapsed (hidden). + /// Whether this item is visible or hidden. /// public bool IsVisible { get; set; } = true; + /// + /// Whether this item is expanded or not. Should only be used for headers of groups. + /// + public bool IsExpanded { get; set; } + public CarouselItem(object model) { Model = model; From 599b59cb1447467048bda41105956bd0c532863e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 17:16:36 +0900 Subject: [PATCH 20/20] Add expanded state to sample drawable representations --- ...estSceneBeatmapCarouselV2GroupSelection.cs | 25 ++++++++++++++++++- osu.Game/Screens/SelectV2/BeatmapPanel.cs | 1 + osu.Game/Screens/SelectV2/BeatmapSetPanel.cs | 9 ++++++- osu.Game/Screens/SelectV2/Carousel.cs | 2 ++ osu.Game/Screens/SelectV2/GroupPanel.cs | 10 +++++++- osu.Game/Screens/SelectV2/ICarouselPanel.cs | 7 +++++- 6 files changed, 50 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs index 04ca0a9085..f4d97be5a5 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs @@ -24,7 +24,7 @@ namespace osu.Game.Tests.Visual.SongSelect } [Test] - public void TestOpenCloseGroupWithNoSelection() + public void TestOpenCloseGroupWithNoSelectionMouse() { AddBeatmaps(10, 5); WaitForDrawablePanels(); @@ -41,6 +41,29 @@ namespace osu.Game.Tests.Visual.SongSelect CheckNoSelection(); } + [Test] + public void TestOpenCloseGroupWithNoSelectionKeyboard() + { + AddBeatmaps(10, 5); + WaitForDrawablePanels(); + + AddAssert("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + + SelectNextPanel(); + Select(); + AddUntilStep("some beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); + AddAssert("keyboard selected is expanded", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); + CheckNoSelection(); + + Select(); + AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddAssert("keyboard selected is collapsed", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); + CheckNoSelection(); + + GroupPanel? getKeyboardSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.KeyboardSelected.Value); + } + [Test] public void TestCarouselRemembersSelection() { diff --git a/osu.Game/Screens/SelectV2/BeatmapPanel.cs b/osu.Game/Screens/SelectV2/BeatmapPanel.cs index 4a9e406def..3edfd4203b 100644 --- a/osu.Game/Screens/SelectV2/BeatmapPanel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapPanel.cs @@ -100,6 +100,7 @@ namespace osu.Game.Screens.SelectV2 public CarouselItem? Item { get; set; } public BindableBool Selected { get; } = new BindableBool(); + public BindableBool Expanded { get; } = new BindableBool(); public BindableBool KeyboardSelected { get; } = new BindableBool(); public double DrawYPosition { get; set; } diff --git a/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs b/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs index 06e3ad3426..79ffe0f68a 100644 --- a/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs @@ -25,6 +25,7 @@ namespace osu.Game.Screens.SelectV2 private BeatmapCarousel carousel { get; set; } = null!; private OsuSpriteText text = null!; + private Box box = null!; [BackgroundDependencyLoader] private void load() @@ -34,7 +35,7 @@ namespace osu.Game.Screens.SelectV2 InternalChildren = new Drawable[] { - new Box + box = new Box { Colour = Color4.Yellow.Darken(5), Alpha = 0.8f, @@ -48,6 +49,11 @@ namespace osu.Game.Screens.SelectV2 } }; + Expanded.BindValueChanged(value => + { + box.FadeColour(value.NewValue ? Color4.Yellow.Darken(2) : Color4.Yellow.Darken(5), 500, Easing.OutQuint); + }); + KeyboardSelected.BindValueChanged(value => { if (value.NewValue) @@ -85,6 +91,7 @@ namespace osu.Game.Screens.SelectV2 public CarouselItem? Item { get; set; } public BindableBool Selected { get; } = new BindableBool(); + public BindableBool Expanded { get; } = new BindableBool(); public BindableBool KeyboardSelected { get; } = new BindableBool(); public double DrawYPosition { get; set; } diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index a1bafac620..608ef207d9 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -571,6 +571,7 @@ namespace osu.Game.Screens.SelectV2 c.Selected.Value = c.Item == currentSelection?.CarouselItem; c.KeyboardSelected.Value = c.Item == currentKeyboardSelection?.CarouselItem; + c.Expanded.Value = c.Item.IsExpanded; } } @@ -674,6 +675,7 @@ namespace osu.Game.Screens.SelectV2 carouselPanel.Item = null; carouselPanel.Selected.Value = false; carouselPanel.KeyboardSelected.Value = false; + carouselPanel.Expanded.Value = false; } #endregion diff --git a/osu.Game/Screens/SelectV2/GroupPanel.cs b/osu.Game/Screens/SelectV2/GroupPanel.cs index 882d77cb8d..7ed256ca6a 100644 --- a/osu.Game/Screens/SelectV2/GroupPanel.cs +++ b/osu.Game/Screens/SelectV2/GroupPanel.cs @@ -26,6 +26,8 @@ namespace osu.Game.Screens.SelectV2 private Box activationFlash = null!; private OsuSpriteText text = null!; + private Box box = null!; + [BackgroundDependencyLoader] private void load() { @@ -34,7 +36,7 @@ namespace osu.Game.Screens.SelectV2 InternalChildren = new Drawable[] { - new Box + box = new Box { Colour = Color4.DarkBlue.Darken(5), Alpha = 0.8f, @@ -60,6 +62,11 @@ namespace osu.Game.Screens.SelectV2 activationFlash.FadeTo(value.NewValue ? 0.2f : 0, 500, Easing.OutQuint); }); + Expanded.BindValueChanged(value => + { + box.FadeColour(value.NewValue ? Color4.SkyBlue : Color4.DarkBlue.Darken(5), 500, Easing.OutQuint); + }); + KeyboardSelected.BindValueChanged(value => { if (value.NewValue) @@ -97,6 +104,7 @@ namespace osu.Game.Screens.SelectV2 public CarouselItem? Item { get; set; } public BindableBool Selected { get; } = new BindableBool(); + public BindableBool Expanded { get; } = new BindableBool(); public BindableBool KeyboardSelected { get; } = new BindableBool(); public double DrawYPosition { get; set; } diff --git a/osu.Game/Screens/SelectV2/ICarouselPanel.cs b/osu.Game/Screens/SelectV2/ICarouselPanel.cs index a956bb22a3..4fba0d2827 100644 --- a/osu.Game/Screens/SelectV2/ICarouselPanel.cs +++ b/osu.Game/Screens/SelectV2/ICarouselPanel.cs @@ -14,10 +14,15 @@ namespace osu.Game.Screens.SelectV2 public interface ICarouselPanel { /// - /// Whether this item has selection. Should be read from to update the visual state. + /// Whether this item has selection (see ). Should be read from to update the visual state. /// BindableBool Selected { get; } + /// + /// Whether this item is expanded (see ). Should be read from to update the visual state. + /// + BindableBool Expanded { get; } + /// /// Whether this item has keyboard selection. Should be read from to update the visual state. ///