Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split out beatmap and set panels in beatmap carousel v2 #31652

Merged
merged 6 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ protected void CreateCarousel()

protected void SortBy(FilterCriteria criteria) => AddStep($"sort by {criteria.Sort}", () => Carousel.Filter(criteria));

protected void WaitForDrawablePanels() => AddUntilStep("drawable panels loaded", () => Carousel.ChildrenOfType<BeatmapCarouselPanel>().Count(), () => Is.GreaterThan(0));
protected void WaitForDrawablePanels() => AddUntilStep("drawable panels loaded", () => Carousel.ChildrenOfType<ICarouselPanel>().Count(), () => Is.GreaterThan(0));
protected void WaitForSorting() => AddUntilStep("sorting finished", () => Carousel.IsFiltering, () => Is.False);
protected void WaitForScrolling() => AddUntilStep("scroll finished", () => Scroll.Current, () => Is.EqualTo(Scroll.Target));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,16 @@ public void TestScrollPositionMaintainedOnAddSecondSelected()
WaitForDrawablePanels();

AddStep("select middle beatmap", () => Carousel.CurrentSelection = BeatmapSets.ElementAt(BeatmapSets.Count - 2));
AddStep("scroll to selected item", () => Scroll.ScrollTo(Scroll.ChildrenOfType<BeatmapCarouselPanel>().Single(p => p.Selected.Value)));
AddStep("scroll to selected item", () => Scroll.ScrollTo(Scroll.ChildrenOfType<BeatmapPanel>().Single(p => p.Selected.Value)));

WaitForScrolling();

AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType<BeatmapCarouselPanel>().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad);
AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType<BeatmapPanel>().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad);

RemoveFirstBeatmap();
WaitForSorting();

AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType<BeatmapCarouselPanel>().Single(p => p.Selected.Value).ScreenSpaceDrawQuad,
AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType<BeatmapPanel>().Single(p => p.Selected.Value).ScreenSpaceDrawQuad,
() => Is.EqualTo(positionBefore));
}

Expand All @@ -83,11 +83,11 @@ public void TestScrollPositionMaintainedOnAddLastSelected()

WaitForScrolling();

AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType<BeatmapCarouselPanel>().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad);
AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType<BeatmapPanel>().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad);

RemoveFirstBeatmap();
WaitForSorting();
AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType<BeatmapCarouselPanel>().Single(p => p.Selected.Value).ScreenSpaceDrawQuad,
AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType<BeatmapPanel>().Single(p => p.Selected.Value).ScreenSpaceDrawQuad,
() => Is.EqualTo(positionBefore));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void TestCarouselRemembersSelection()
AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection));
AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection));

BeatmapCarouselPanel? getSelectedPanel() => Carousel.ChildrenOfType<BeatmapCarouselPanel>().SingleOrDefault(p => p.Selected.Value);
BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType<BeatmapPanel>().SingleOrDefault(p => p.Selected.Value);
}

[Test]
Expand Down
20 changes: 17 additions & 3 deletions osu.Game/Screens/SelectV2/BeatmapCarousel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,28 @@ public void Filter(FilterCriteria criteria)

#region Drawable pooling

private readonly DrawablePool<BeatmapCarouselPanel> carouselPanelPool = new DrawablePool<BeatmapCarouselPanel>(100);
private readonly DrawablePool<BeatmapPanel> beatmapPanelPool = new DrawablePool<BeatmapPanel>(100);
private readonly DrawablePool<BeatmapSetPanel> setPanelPool = new DrawablePool<BeatmapSetPanel>(100);

private void setupPools()
{
AddInternal(carouselPanelPool);
AddInternal(beatmapPanelPool);
AddInternal(setPanelPool);
}

protected override Drawable GetDrawableForDisplay(CarouselItem item) => carouselPanelPool.Get();
protected override Drawable GetDrawableForDisplay(CarouselItem item)
{
switch (item.Model)
{
case BeatmapInfo:
return beatmapPanelPool.Get();

case BeatmapSetInfo:
return setPanelPool.Get();
}

throw new InvalidOperationException();
}

#endregion
}
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public async Task<IEnumerable<CarouselItem>> Run(IEnumerable<CarouselItem> items
{
newItems.Add(new CarouselItem(b.BeatmapSet!)
{
DrawHeight = 80,
DrawHeight = BeatmapSetPanel.HEIGHT,
IsGroupSelectionTarget = true
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,25 @@

namespace osu.Game.Screens.SelectV2
{
public partial class BeatmapCarouselPanel : PoolableDrawable, ICarouselPanel
public partial class BeatmapPanel : PoolableDrawable, ICarouselPanel
{
[Resolved]
private BeatmapCarousel carousel { get; set; } = null!;

private Box activationFlash = null!;
private Box background = null!;
private OsuSpriteText text = null!;

[BackgroundDependencyLoader]
private void load()
{
Size = new Vector2(500, CarouselItem.DEFAULT_HEIGHT);
Masking = true;

InternalChildren = new Drawable[]
{
background = new Box
new Box
{
Colour = Color4.Aqua.Darken(5),
Alpha = 0.8f,
RelativeSizeAxes = Axes.Both,
},
Expand Down Expand Up @@ -69,63 +72,40 @@ private void load()
});
}

protected override void FreeAfterUse()
{
base.FreeAfterUse();
Item = null;
Selected.Value = false;
KeyboardSelected.Value = false;
}

protected override void PrepareForUse()
{
base.PrepareForUse();

Debug.Assert(Item != null);
var beatmap = (BeatmapInfo)Item.Model;

DrawYPosition = Item.CarouselYPosition;

Size = new Vector2(500, Item.DrawHeight);
Masking = true;

background.Colour = (Item.Model is BeatmapInfo ? Color4.Aqua : Color4.Yellow).Darken(5);
text.Text = getTextFor(Item.Model);
text.Text = $"Difficulty: {beatmap.DifficultyName} ({beatmap.StarRating:N1}*)";

this.FadeInFromZero(500, Easing.OutQuint);
}

private string getTextFor(object item)
protected override bool OnClick(ClickEvent e)
{
switch (item)
if (carousel.CurrentSelection != Item!.Model)
{
case BeatmapInfo bi:
return $"Difficulty: {bi.DifficultyName} ({bi.StarRating:N1}*)";

case BeatmapSetInfo si:
return $"{si.Metadata}";
carousel.CurrentSelection = Item!.Model;
return true;
}

return "unknown";
}

protected override bool OnClick(ClickEvent e)
{
if (carousel.CurrentSelection == Item!.Model)
carousel.TryActivateSelection();
else
carousel.CurrentSelection = Item!.Model;
carousel.TryActivateSelection();
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()
{
activationFlash.FadeOutFromOne(500, Easing.OutQuint);
}
public void Activated() => activationFlash.FadeOutFromOne(500, Easing.OutQuint);

#endregion
}
}
114 changes: 114 additions & 0 deletions osu.Game/Screens/SelectV2/BeatmapSetPanel.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System;
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.Beatmaps;
using osu.Game.Graphics.Sprites;
using osuTK;
using osuTK.Graphics;

namespace osu.Game.Screens.SelectV2
{
public partial class BeatmapSetPanel : 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.Yellow.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);
});
bdach marked this conversation as resolved.
Show resolved Hide resolved

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);

var beatmapSetInfo = (BeatmapSetInfo)Item.Model;

text.Text = $"{beatmapSetInfo.Metadata}";

this.FadeInFromZero(500, Easing.OutQuint);
}

protected override bool OnClick(ClickEvent e)
{
carousel.CurrentSelection = Item!.Model;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To continue the theme above, this weirds me out. Locally the set panel select itself but then that gets hijacked to beatmap selection at Carousel level? It's a bit strange, is it supposed to be like that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure. How would you see it working? I'm trying to keep the knowlege of "what selects what" all in one place (BeatmapCarousel) and keep these panels as simple as possible. The other direction would be to make them very self aware.

For this specific case I guess it can already do that by setting its first available beatmap, which sounds fine and maybe preferable, until we add something like a recommendation step to the process. Once we have an external entity choosing which beatmap should be selected, I think it's going to be better to have this selection logic in BeatmapCarousel.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno... maybe at least an inline comment or something that this gets magically handled elsewhere?

The other suggestion would be to invert the flow and just give panels a delegate to request selection from the carousel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold that though until I get groups working, because selection is a bit obtuse there and i'm still finding a solution.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get back to this later then... if I remember to I guess 😅

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
}
}
14 changes: 12 additions & 2 deletions osu.Game/Screens/SelectV2/Carousel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -540,11 +540,13 @@ protected override void Update()
{
var c = (ICarouselPanel)panel;

// panel in the process of expiring, ignore it.
if (c.Item == null)
continue;

if (panel.Depth != c.DrawYPosition)
scroll.Panels.ChangeChildDepth(panel, (float)c.DrawYPosition);

Debug.Assert(c.Item != null);

if (c.DrawYPosition != c.Item.CarouselYPosition)
c.DrawYPosition = Interpolation.DampContinuously(c.DrawYPosition, c.Item.CarouselYPosition, 50, Time.Elapsed);

Expand Down Expand Up @@ -631,7 +633,9 @@ private void updateDisplayedRange(DisplayRange range)
if (drawable is not ICarouselPanel carouselPanel)
throw new InvalidOperationException($"Carousel panel drawables must implement {typeof(ICarouselPanel)}");

carouselPanel.DrawYPosition = item.CarouselYPosition;
carouselPanel.Item = item;

scroll.Add(drawable);
}

Expand All @@ -650,6 +654,12 @@ private static void expirePanelImmediately(Drawable panel)
{
panel.FinishTransforms();
panel.Expire();

var carouselPanel = (ICarouselPanel)panel;

carouselPanel.Item = null;
carouselPanel.Selected.Value = false;
carouselPanel.KeyboardSelected.Value = false;
Comment on lines +660 to +662
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the significance of these operations? Should I be worrying that CarouselYPosition isn't reset here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just see it as sane defaults for animation purposes. ie. new panel comes alive, if it gets selected i'd hope it goes from not-selected to selected, rather than starting with selected==true.

Should I be worrying that CarouselYPosition isn't reset here?

I assume you mean DrawYPosition? I don't know what a sane default for that would be. We could set it back to 0 but I think it's kinda up to carousel to manage that (it already transfers when retrieving from pool immediately)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that you moved this logic from BeatmapPanel.FreeAfterUse() which I guess gives this change more of a point. I can agree with it in that light.

}

#endregion
Expand Down
Loading