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

New leaderboard score card design implementation #22237

Merged
merged 64 commits into from
Jun 8, 2024

Conversation

mk56-spn
Copy link
Contributor

@mk56-spn mk56-spn commented Jan 16, 2023

Figma reference: https://www.figma.com/file/EjS27IBaZhc2qX97lFBUxN/Song-Select-5?type=design&node-id=1-1010&mode=design&t=WQyZkwmDC4JWz6An-0

More song select stuff.

CleanShot.2024-05-25.at.17.24.00.mp4
old

Couple of things to note when reviewing this:

Something to note while reviewing this, due to the amount of restructuring that will be required to fit these into song select they have been PRed in isolation, a follow up PR, most likely by me if im allowed will go through the necessary refactor to fit these into a new leaderboard, with the appropriate height, shear, etc.

  • I do not recommend comparing the implementation to the old one, since ive split the commits to be reviewed as if this was an entirely new component, since i found it to be probably the easiest way to understand it.

  • As for the overlapping mods when the count exceeds the amount that can be housed inside the right side of the component they will collapse and overlap as per @peppy 's suggestion.

  • And final major thing to note, i opted to specify the widths of the statistics manually, as to avoid them differing in length across different scores, which looked horrible when placed in a leaderboard together.

sssssss

Header should be coming some time in the next few days as well as the new tooltip for this component.

@Joehuu Joehuu self-requested a review January 16, 2023 19:05
@frenzibyte frenzibyte changed the title Leaderboardscore new design implementation New leaderboard score card design implementation Jan 16, 2023
mk56-spn and others added 3 commits January 16, 2023 22:37
Co-authored-by: Joseph Madamba <madamba.joehu@outlook.com>
…aynames.

Make adjustments to statistics to allow them to work with autosizing
@peppy peppy self-requested a review January 23, 2023 05:26
@peppy
Copy link
Member

peppy commented Jan 23, 2023

So what happens for a mania score? Can you add one to the test scene please? I don't think information is going to fit due to more judgements.

When designing tests, please make sure to not just consider the scenario which works well, but intentionally look for the edge cases that need to be supported. Without this, the tests are not that useful.

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Jan 23, 2023

zzz

added to the test, fwiw i had already guarded against this by making the container for the judgements autosize to the size of the judgement names line . although i feel perhaps it would look better if the judgements were centered underneath?

it doesnt look good side by side with osu ruleset but thats not a case that can happen outside of the test scene

@peppy
Copy link
Member

peppy commented Jan 25, 2023

@arflyte are you sure this is the best way to display this? it feels a bit.. half-assed? especially when there's more judgements, it's very hard to visually parse the mapping of judgement names to counts.

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Jan 25, 2023

@arflyte are you sure this is the best way to display this? it feels a bit.. half-assed? especially when there's more judgements, it's very hard to visually parse the mapping of judgement names to counts.

I'm for moving this to the tooltip. It's not information you really need at a first glance ( at least in my experience) and that way the tooltip isn't just an expanded mod panel? As well as displaying them in a much more comprehensible way

@peppy
Copy link
Member

peppy commented Jan 25, 2023

Keep in mind that we already have this in the tooltip on master, but it looks bad. I think we've asked @arflyte for a design for that in the past but haven't gotten anything yet. I think it's supposed to popup a larger view of the score.

@mk56-spn
Copy link
Contributor Author

Keep in mind that we already have this in the tooltip on master, but it looks bad. I think we've asked @arflyte for a design for that in the past but haven't gotten anything yet. I think it's supposed to popup a larger view of the score.

just to clarify im kind of unsure how i should take this, am i meant to pull something out of somewhere and see if it works or await a design from @arflyte

@arflyte
Copy link

arflyte commented Feb 14, 2023

I've been looking into this for the past few weeks; it's part of the song select revision and mod display fix. I still have no final solution to this yet, but it's one of the higher priorities I'm working on right now.

@frenzibyte
Copy link
Member

That is also a viable option as long as we agree we don't need to show more than 4 mods, I thought the point of going up to 7 mods is to resolve some recurring issues we faced with regards to overflow of mods, but I don't know if that's really the case or just an arbitrary change in this PR.

@frenzibyte
Copy link
Member

Upon taking a peek at some of the leaderboards, I think we should at least support showing up to 5 mods for the initial push of the screen, to handle the famous mod combination DT FL HR HD combined with the Classic mod. This is definitely going to show up in a lot of scores and just displaying a text saying "5 mods" feels like it misrepresents the score (as these combinations are quite common), let alone showing them in a contracted state.

Final proposal for the time being:

CleanShot 2024-05-14 at 14 31 28

Keeps design consistent and avoids mods overlapping each other with the "contracted" state (I think contraction is best suited for icon-based mods, which we have seemingly moved away from).

diff
diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneLeaderboardScoreV2.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneLeaderboardScoreV2.cs
index c5f96d1568..321f443a97 100644
--- a/osu.Game.Tests/Visual/SongSelect/TestSceneLeaderboardScoreV2.cs
+++ b/osu.Game.Tests/Visual/SongSelect/TestSceneLeaderboardScoreV2.cs
@@ -154,19 +154,11 @@ private static ScoreInfo[] getTestScores()
                 }
             };
 
-            for (int i = 0; i < LeaderboardScoreV2.MAX_MODS_EXPANDED; i++)
-                scores[0].Mods = scores[0].Mods.Concat(new Mod[] { i % 2 == 0 ? new OsuModHidden() : halfTime }).ToArray();
-
-            for (int i = 0; i < LeaderboardScoreV2.MAX_MODS_EXPANDED + 1; i++)
-                scores[1].Mods = scores[1].Mods.Concat(new Mod[] { i % 2 == 0 ? new OsuModHidden() : new OsuModHalfTime() }).ToArray();
-
-            for (int i = 0; i < LeaderboardScoreV2.MAX_MODS_CONTRACTED; i++)
-                scores[2].Mods = scores[2].Mods.Concat(new Mod[] { i % 2 == 0 ? new OsuModHidden() : halfTime }).ToArray();
-
-            for (int i = 0; i < LeaderboardScoreV2.MAX_MODS_CONTRACTED + 1; i++)
-                scores[3].Mods = scores[3].Mods.Concat(new Mod[] { i % 2 == 0 ? new OsuModHidden() : new OsuModHalfTime() }).ToArray();
-
-            scores[4].Mods = scores[4].BeatmapInfo!.Ruleset.CreateInstance().CreateAllMods().ToArray();
+            scores[0].Mods = new Mod[] { new OsuModDoubleTime(), new OsuModHidden() };
+            scores[1].Mods = new Mod[] { new OsuModDoubleTime(), new OsuModHidden(), new OsuModFlashlight() };
+            scores[2].Mods = new Mod[] { new OsuModDoubleTime(), new OsuModHidden(), new OsuModFlashlight(), new OsuModHardRock() };
+            scores[3].Mods = new Mod[] { new OsuModDoubleTime(), new OsuModHidden(), new OsuModFlashlight(), new OsuModHardRock(), new OsuModClassic() };
+            scores[4].Mods = new Mod[] { new OsuModDoubleTime(), new OsuModHidden(), new OsuModFlashlight(), new OsuModHardRock(), new OsuModClassic(), new OsuModDifficultyAdjust() };
 
             return scores;
         }
diff --git a/osu.Game/Online/Leaderboards/LeaderboardScoreV2.cs b/osu.Game/Online/Leaderboards/LeaderboardScoreV2.cs
index b9ae3bb20e..862eae10e7 100644
--- a/osu.Game/Online/Leaderboards/LeaderboardScoreV2.cs
+++ b/osu.Game/Online/Leaderboards/LeaderboardScoreV2.cs
@@ -41,16 +41,11 @@ namespace osu.Game.Online.Leaderboards
     public partial class LeaderboardScoreV2 : OsuClickableContainer, IHasContextMenu, IHasCustomTooltip<ScoreInfo>
     {
         /// <summary>
-        /// The maximum number of mods when contracted until the mods display width exceeds the <see cref="right_content_width"/>.
+        /// The maximum number of mods until the mods display width exceeds the <see cref="right_content_width"/>.
         /// </summary>
-        public const int MAX_MODS_CONTRACTED = 13;
+        public const int MAX_MODS = 5;
 
-        /// <summary>
-        /// The maximum number of mods when expanded until the mods display width exceeds the <see cref="right_content_width"/>.
-        /// </summary>
-        public const int MAX_MODS_EXPANDED = 4;
-
-        private const float right_content_width = 180;
+        private const float right_content_width = 210;
         private const float grade_width = 40;
         private const float username_min_width = 125;
         private const float statistics_regular_min_width = 175;
@@ -182,8 +177,6 @@ private void load()
             };
 
             innerAvatar.OnLoadComplete += d => d.FadeInFromZero(200);
-
-            modsContainer.Spacing = new Vector2(modsContainer.Children.Count > MAX_MODS_EXPANDED ? -20 : 2, 0);
             modsContainer.Padding = new MarginPadding { Top = modsContainer.Children.Count > 0 ? 4 : 0 };
         }
 
@@ -439,6 +432,7 @@ private void load()
                                         Shear = -shear,
                                         AutoSizeAxes = Axes.Both,
                                         Direction = FillDirection.Horizontal,
+                                        Spacing = new Vector2(2f, 0f),
                                         ChildrenEnumerable = score.Mods.AsOrdered().Select(mod => new ColouredModSwitchTiny(mod) { Scale = new Vector2(0.375f) })
                                     },
                                     modsCounter = new OsuSpriteText
@@ -492,7 +486,7 @@ public override void Show()
 
                     using (BeginDelayedSequence(50))
                     {
-                        Drawable modsDrawable = score.Mods.Length > MAX_MODS_CONTRACTED ? modsCounter : modsContainer;
+                        Drawable modsDrawable = score.Mods.Length > MAX_MODS ? modsCounter : modsContainer;
                         var drawables = new[] { flagBadgeAndDateContainer, modsDrawable }.Concat(statisticsLabels).ToArray();
                         for (int i = 0; i < drawables.Length; i++)
                             drawables[i].FadeIn(100 + i * 50);

Comment on lines 444 to 451
modsCounter = new OsuSpriteText
{
Anchor = Anchor.TopRight,
Origin = Anchor.TopRight,
Shear = -shear,
Text = $"{score.Mods.Length} mods",
Alpha = 0,
}
Copy link
Member

@frenzibyte frenzibyte May 14, 2024

Choose a reason for hiding this comment

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

I have stirred up some discussion on whether this can be replaced by an indicator next to an arbitrary subset of mods, and it was concluded that it's better. For example (design is arbitrary):

image

Currently waiting for a response from @arflyte for a better design of this indicator, unless it's fine to proceed with the current one (I definitely need to rewrite the code because I faked up a Mod to achieve this).

Copy link
Member

Choose a reason for hiding this comment

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

Go with what you have, I think it's fine to start with.

Copy link
Member

Choose a reason for hiding this comment

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

Added in 5955378

@Givikap120
Copy link
Contributor

I think that it's much more readable to use an icons instead of acronyms
Especially considering new players existing

@frenzibyte
Copy link
Member

I've changed the mod display as proposed in #22237 (comment) & #22237 (comment). I've also made the width of the right content change based on scoring mode, to handle marathon beatmaps where scores can go up to 9 digits (e.g. https://osu.ppy.sh/beatmapsets/972#osu/9007).

Preview:

CleanShot.2024-05-25.at.17.24.00.mp4

This looks to be in a good state to me.

frenzibyte
frenzibyte previously approved these changes May 25, 2024
@frenzibyte frenzibyte requested a review from peppy May 25, 2024 14:27
frenzibyte
frenzibyte previously approved these changes May 25, 2024
@Joehuu Joehuu self-requested a review May 26, 2024 06:27
See `ModSelectOverlay` components.
@Joehuu
Copy link
Member

Joehuu commented May 26, 2024

I've pushed a commit that follows precedent to the slanted flow logic (mod overlay components when initializing use Shear = Vector2.Zero while the parent (or fill flow) uses Shear = {SomeComponent}.SHEAR).

When restarting the test (to see the pop in animation), I thought the right content width logic was broken, but it was just changing the scoring mode initially. Not sure how to prevent that step from happening initially.

Recent changes look good to me otherwise.

@Givikap120

This comment was marked as duplicate.

@peppy peppy merged commit daf85c3 into ppy:master Jun 8, 2024
11 of 17 checks passed
@developomp
Copy link

🎉

@Gabixel Gabixel mentioned this pull request Jul 17, 2024
1 task
Joehuu added a commit to Joehuu/osu that referenced this pull request Jul 20, 2024
- No more border gradient (probably same as ppy#22237 (comment))
- No more radius 50 which is maybe a performance issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Mod icons overlapping with accuracy on beatmap leaderboard
9 participants