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

Implement new components for song select wedge redesign #28063

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented May 2, 2024

Where's the individual component PRs?

Considered PRing individual components (and replacing old ones) more effort because:

  • you would have to remove the old component with the new one that's potentially in a completely different spot
  • new and old wedge components have to coexist short-term (song select wedge will be wonky until everything is completed, and can't promise everything will be done before next release)
  • probably conflicts along the way if I were to multiple component PRs all at once, and probably slow if I leave at most 1 open at a time

My original proposal for the process was just implementing the component and not replacing the old one, but also may be harder to review as it'll just be a single component (content) in a test with no context.

The goal of this PR is preparation for just replacing the old wedge. It does the bare minimum: no skinning (e.g. collapsing), no new density graph, old details tab w/ metadata and online stuff is still a thing (newest design iteration may not have it displayed by default). Also had beatmap info overlay / online in mind when I did this long before, so there are APIBeatmap cases with some testing.

Code follows the Content terminology like results screen components when the component contains text and has no background. The LOC is probably daunting, but If you don't count the tests / header and usings, it is probably under 1000 with some reusing of old components' code.

The interim design is following the very first figma iteration for the difficulty name / length and bpm content, and for object count / difficulty settings content, it follows the current layout but has three columns because the left side expanded. The header is not implemented as I believe that takes too much height room (it's still in multi though). Also ignores the latest figma design linked above that removes bars entirely.

The old-to-new wedge replacement diff is below. Didn't apply it for now to ease LOC.

There are some TODOs, and I wrote this after midnight, so opening as draft for general feedback for now:

TODOs (some todos in code not listed):
  • Right of wedge looks wrong at first song select start (was an issue before this PR)
    Screenshot 2024-05-02 at 12 32 49 AM
  • object count bars' max values are hardcoded to 1000 (should be hardest difficulty iirc)
Before After
osu_2024-05-02_00-06-19 osu_2024-05-02_00-07-21
diff --git a/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs b/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs
index 20dd7c55d8..e95fa4f40d 100644
--- a/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs
+++ b/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs
@@ -200,6 +200,8 @@ protected override void LoadComplete()
             }, true);
 
             beatmap.BindValueChanged(_ => updateDisplay(), true);
+
+            Show();
         }
 
         private const double animation_duration = 600;
diff --git a/osu.Game/Screens/Select/FilterControl.cs b/osu.Game/Screens/Select/FilterControl.cs
index 73c122dda6..6416cdcaf5 100644
--- a/osu.Game/Screens/Select/FilterControl.cs
+++ b/osu.Game/Screens/Select/FilterControl.cs
@@ -10,7 +10,9 @@
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
+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.Input;
@@ -97,8 +99,7 @@ private void load(OsuColour colours, OsuConfigManager config)
             {
                 new Box
                 {
-                    Colour = Color4.Black,
-                    Alpha = 0.8f,
+                    Colour = ColourInfo.GradientVertical(Color4.Black, Color4.Black.Opacity(0)),
                     Width = 2,
                     RelativeSizeAxes = Axes.Both,
                 },
@@ -106,7 +107,7 @@ private void load(OsuColour colours, OsuConfigManager config)
                 {
                     Padding = new MarginPadding(side_margin),
                     RelativeSizeAxes = Axes.Both,
-                    Width = 0.5f,
+                    Width = 0.4f,
                     Anchor = Anchor.TopRight,
                     Origin = Anchor.TopRight,
                     // Reverse ChildID so that dropdowns in the top section appear on top of the bottom section.
diff --git a/osu.Game/Screens/Select/PlaySongSelect.cs b/osu.Game/Screens/Select/PlaySongSelect.cs
index 52f49ba56a..aec6aeee82 100644
--- a/osu.Game/Screens/Select/PlaySongSelect.cs
+++ b/osu.Game/Screens/Select/PlaySongSelect.cs
@@ -28,6 +28,9 @@ namespace osu.Game.Screens.Select
 {
     public partial class PlaySongSelect : SongSelect
     {
+        [Cached]
+        private readonly OverlayColourProvider colourProvider = new OverlayColourProvider(OverlayColourScheme.Aquamarine);
+
         private OsuScreen? playerLoader;
 
         [Resolved]
diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs
index 2580f4fd7a..de3f774eed 100644
--- a/osu.Game/Screens/Select/SongSelect.cs
+++ b/osu.Game/Screens/Select/SongSelect.cs
@@ -13,7 +13,6 @@
 using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
-using osu.Framework.Graphics.Shapes;
 using osu.Framework.Graphics.Sprites;
 using osu.Framework.Graphics.UserInterface;
 using osu.Framework.Input.Bindings;
@@ -36,7 +35,6 @@
 using osu.Game.Screens.Edit;
 using osu.Game.Screens.Menu;
 using osu.Game.Screens.Play;
-using osu.Game.Screens.Select.Details;
 using osu.Game.Screens.Select.Options;
 using osu.Game.Skinning;
 using osuTK;
@@ -108,9 +106,7 @@ public abstract partial class SongSelect : ScreenWithBeatmapBackground, IKeyBind
 
         private ParallaxContainer wedgeBackground = null!;
 
-        protected Container LeftArea { get; private set; } = null!;
-
-        private BeatmapInfoWedge beatmapInfoWedge = null!;
+        protected GridContainer LeftArea { get; private set; } = null!;
 
         [Resolved]
         private IDialogOverlay? dialogOverlay { get; set; }
@@ -137,8 +133,6 @@ public abstract partial class SongSelect : ScreenWithBeatmapBackground, IKeyBind
 
         private IDisposable? modSelectOverlayRegistration;
 
-        private AdvancedStats advancedStats = null!;
-
         [Resolved]
         private MusicController music { get; set; } = null!;
 
@@ -147,6 +141,12 @@ public abstract partial class SongSelect : ScreenWithBeatmapBackground, IKeyBind
 
         private Bindable<bool> configBackgroundBlur = null!;
 
+        [Cached(typeof(IBindable<IBeatmapInfo?>))]
+        private readonly Bindable<IBeatmapInfo?> beatmapInfo = new Bindable<IBeatmapInfo?>();
+
+        [Cached(typeof(IBindable<IBeatmapSetInfo?>))]
+        private readonly Bindable<IBeatmapSetInfo?> beatmapSetInfo = new Bindable<IBeatmapSetInfo?>();
+
         [BackgroundDependencyLoader(true)]
         private void load(AudioManager audio, OsuColour colours, ManageCollectionsDialog? manageCollectionsDialog, DifficultyRecommender? recommender, OsuConfigManager config)
         {
@@ -188,7 +188,7 @@ private void load(AudioManager audio, OsuColour colours, ManageCollectionsDialog
                             ColumnDimensions = new[]
                             {
                                 new Dimension(),
-                                new Dimension(GridSizeMode.Relative, 0.5f, maxSize: 850),
+                                new Dimension(GridSizeMode.Relative, 0.4f, maxSize: 650),
                             },
                             Content = new[]
                             {
@@ -231,86 +231,54 @@ private void load(AudioManager audio, OsuColour colours, ManageCollectionsDialog
                             RelativeSizeAxes = Axes.Both,
                             ColumnDimensions = new[]
                             {
-                                new Dimension(GridSizeMode.Relative, 0.5f, maxSize: 650),
+                                new Dimension(GridSizeMode.Relative, 0.6f, maxSize: 850),
                             },
                             Content = new[]
                             {
                                 new Drawable[]
                                 {
-                                    LeftArea = new Container
+                                    LeftArea = new LeftSideInteractionContainer(() => Carousel.ScrollToSelected())
                                     {
                                         Origin = Anchor.BottomLeft,
                                         Anchor = Anchor.BottomLeft,
                                         RelativeSizeAxes = Axes.Both,
-                                        Padding = new MarginPadding { Top = 5 },
-                                        Children = new Drawable[]
+                                        RowDimensions = new[]
                                         {
-                                            new LeftSideInteractionContainer(() => Carousel.ScrollToSelected())
-                                            {
-                                                RelativeSizeAxes = Axes.Both,
-                                            },
-                                            beatmapInfoWedge = new BeatmapInfoWedge
+                                            new Dimension(GridSizeMode.AutoSize),
+                                        },
+                                        Content = new[]
+                                        {
+                                            new Drawable[]
                                             {
-                                                Height = WEDGE_HEIGHT,
-                                                RelativeSizeAxes = Axes.X,
-                                                Margin = new MarginPadding
+                                                new Container
                                                 {
-                                                    Right = left_area_padding,
-                                                    Left = -BeatmapInfoWedge.BORDER_THICKNESS, // Hide the left border
+                                                    RelativeSizeAxes = Axes.X,
+                                                    AutoSizeAxes = Axes.Y,
+                                                    Padding = new MarginPadding { Top = 10 },
+                                                    Child = new BeatmapInfoWedgeV2
+                                                    {
+                                                        RelativeSizeAxes = Axes.X,
+                                                    },
                                                 },
                                             },
-                                            new Container
+                                            new Drawable[]
                                             {
-                                                RelativeSizeAxes = Axes.X,
-                                                Height = 90,
-                                                Padding = new MarginPadding(10)
-                                                {
-                                                    Left = left_area_padding,
-                                                    Right = left_area_padding * 2 + 5,
-                                                },
-                                                Y = WEDGE_HEIGHT,
-                                                Children = new Drawable[]
+                                                new Container
                                                 {
-                                                    new Container
+                                                    RelativeSizeAxes = Axes.Both,
+                                                    Padding = new MarginPadding
                                                     {
-                                                        RelativeSizeAxes = Axes.Both,
-                                                        Masking = true,
-                                                        CornerRadius = 10,
-                                                        Children = new Drawable[]
-                                                        {
-                                                            new Box
-                                                            {
-                                                                RelativeSizeAxes = Axes.Both,
-                                                                Colour = Colour4.Black.Opacity(0.3f),
-                                                            },
-                                                            advancedStats = new AdvancedStats(2)
-                                                            {
-                                                                RelativeSizeAxes = Axes.X,
-                                                                AutoSizeAxes = Axes.Y,
-                                                                Anchor = Anchor.Centre,
-                                                                Origin = Anchor.Centre,
-                                                                Padding = new MarginPadding(10),
-                                                            },
-                                                        }
+                                                        Bottom = Footer.HEIGHT,
+                                                        Left = left_area_padding,
+                                                        Right = left_area_padding * 2,
                                                     },
-                                                }
-                                            },
-                                            new Container
-                                            {
-                                                RelativeSizeAxes = Axes.Both,
-                                                Padding = new MarginPadding
-                                                {
-                                                    Bottom = Footer.HEIGHT,
-                                                    Top = WEDGE_HEIGHT + 70,
-                                                    Left = left_area_padding,
-                                                    Right = left_area_padding * 2,
+                                                    Child = BeatmapDetails = CreateBeatmapDetailArea().With(d =>
+                                                    {
+                                                        d.RelativeSizeAxes = Axes.Both;
+                                                        d.Padding = new MarginPadding { Top = 10, Right = 5 };
+                                                    })
                                                 },
-                                                Child = BeatmapDetails = CreateBeatmapDetailArea().With(d =>
-                                                {
-                                                    d.RelativeSizeAxes = Axes.Both;
-                                                    d.Padding = new MarginPadding { Top = 10, Right = 5 };
-                                                })
-                                            },
+                                            }
                                         }
                                     },
                                 },
@@ -589,11 +557,6 @@ private void performUpdateSelected()
                 beatmapInfoPrevious = beatmap;
             }
 
-            // we can't run this in the debounced run due to the selected mods bindable not being debounced,
-            // since mods could be updated to the new ruleset instances while the decoupled bindable is held behind,
-            // therefore resulting in performing difficulty calculation with invalid states.
-            advancedStats.Ruleset.Value = ruleset;
-
             void run()
             {
                 // clear pending task immediately to track any potential nested debounce operation.
@@ -844,14 +807,10 @@ private void updateComponentFromBeatmap(WorkingBeatmap beatmap)
                 });
             }
 
-            beatmapInfoWedge.Beatmap = beatmap;
-
             BeatmapDetails.Beatmap = beatmap;
 
             ModSelect.Beatmap = beatmap;
 
-            advancedStats.BeatmapInfo = beatmap.BeatmapInfo;
-
             bool beatmapSelected = beatmap is not DummyWorkingBeatmap;
 
             if (beatmapSelected)
@@ -961,7 +920,13 @@ private void bindBindables()
             };
             decoupledRuleset.DisabledChanged += r => Ruleset.Disabled = r;
 
-            Beatmap.BindValueChanged(updateCarouselSelection);
+            Beatmap.BindValueChanged(b =>
+            {
+                updateCarouselSelection();
+
+                beatmapInfo.Value = b.NewValue.BeatmapInfo;
+                beatmapSetInfo.Value = b.NewValue.BeatmapSetInfo;
+            }, true);
 
             boundLocalBindables = true;
         }
@@ -1075,7 +1040,7 @@ public VerticalMaskingContainer()
         /// <summary>
         /// Handles mouse interactions required when moving away from the carousel.
         /// </summary>
-        internal partial class LeftSideInteractionContainer : Container
+        internal partial class LeftSideInteractionContainer : GridContainer
         {
             private readonly Action? resetCarouselPosition;
 

@bdach
Copy link
Collaborator

bdach commented May 2, 2024

Considered PRing individual components (and replacing old ones) more effort because

I wanna stop reading already at this point honestly.

Do you realise what reviewing of a 1.7k line diffstat feels like? Do you realise the bikeshed fiasco you're opening yourself up here for by getting this entire code blob stunlocked on minute design changes?

I dunno, this may be "less work" for you, but it is more work for me / other reviewers, and I see there being a 95% chance of this getting stuck in review hell again.

This is especially important for me because I would like to pay very close attention to how individual components are structured to avoid similar fiascos that plague the current song select and make implementing stuff like correct beatmap invalidation basically infeasible.

Please take a look at how I replaced the mod overlay for this. It was not a giant 5k line blob.

@peppy
Copy link
Sponsor Member

peppy commented May 2, 2024

Maybe I'm biased by wanting to make quick forward progress, but even with this many lines diff, if it's mostly drawable code and hardly any modified lines (all new lines) I'm willing to take it on – at least an initial pass to see how things read and feel.

Will loop back after doing a pass.

@bdach
Copy link
Collaborator

bdach commented May 2, 2024

Sure if you want to try and stomach it I guess. But I will want to have a look too before merge even if you end up being okay with it.

An important code design constraint is that every drawable component in here must support the beatmap changing under it for the invalidation flow. Preferably with a way to force a refresh manually too. If that is not the case, then I'm gonna have a problem with accepting this.

@peppy
Copy link
Sponsor Member

peppy commented May 2, 2024

@Joehuu what is the significance of the diff in the OP? It doesn't look to cleanly apply.

@peppy peppy self-requested a review May 2, 2024 14:44
@peppy
Copy link
Sponsor Member

peppy commented May 2, 2024

@arflyte how much of this change is going to be thrown away in the "next" iteration?

@Joehuu
Copy link
Member Author

Joehuu commented May 2, 2024

@Joehuu what is the significance of the diff in the OP? It doesn't look to cleanly apply.

Seemed to be broken. Have fixed.

If this does get stuck in review hell, I structured the commits here well enough for cherry-picking for individual components PRs.

Skimming at the diff again, I did leave trivial code quality issues not caught by CI and some unnecessary cached fields in a test, but will fix them if the initial evaluation of this approach is tolerable.

@peppy
Copy link
Sponsor Member

peppy commented May 3, 2024

Layout feels really weird at non-widescreen resolutions. The left half gets too much space:

2024-05-03 22 47 19@2x

Spacing is a bit weird here:

2024-05-03 22 55 13@2x

Having this display jump all over the place in string length makes it very hard to visually parse:

2024-05-03 22 56 13

Also all the stats are just mind boggling weird layout-wise. But I guess that's something for us to fix as the designs are made without knowledge of how the information is actually used by the average player.

@Joehuu
Copy link
Member Author

Joehuu commented May 7, 2024

Waiting for @arflyte response to #28063 (comment) before working on this again.

As for the concerns, first one is from a non-pushed commit (wasn't meant to be a talking point, probably when it's actually replaced), second probably just needs a horizontal divider (like those vertical ones on figma), and third i don't really know as it's right aligned (can remove "mostly". there's this discussion, but the right align is probably a good reason).

Last concern is I was just copying the layout we had (at least for advanced stats). Can probably revert back to two columns and have the left area not take up more space).

@Joehuu Joehuu self-assigned this May 11, 2024
@peppy
Copy link
Sponsor Member

peppy commented May 12, 2024

I don't think we should be waiting on flyte responses here, especially considering that this isn't immediately going to be live on any interface. I'd just push all your remaining changes so we can fast-track this to an initial merged state.

@frenzibyte
Copy link
Member

@Joehuu is this ready for a full code review and/or should it be taken out of draft?

@Joehuu
Copy link
Member Author

Joehuu commented May 13, 2024

This is ready for review, yes. Forgot to take this out of draft.

@Joehuu Joehuu marked this pull request as ready for review May 13, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants