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

Fix top rank display not showing up on beatmaps with many difficulties #30579

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Nov 11, 2024

Closes #30553. Also addresses concerns from the original padding addition (see #26701 (comment)) by undoing it.

Note that this doesn't fix the delayed drawables not showing underneath the footer. I have another incoming change to fix that (with design adjustments).

I tested this using

diff --git a/osu.Game/Screens/Select/Carousel/TopLocalRank.cs b/osu.Game/Screens/Select/Carousel/TopLocalRank.cs
index da9661f702..f18fe23607 100644
--- a/osu.Game/Screens/Select/Carousel/TopLocalRank.cs
+++ b/osu.Game/Screens/Select/Carousel/TopLocalRank.cs
@@ -75,7 +75,10 @@ void localScoresChanged(IRealmCollection<ScoreInfo> sender, ChangeSet? changes)
                 if (changes?.HasCollectionChanges() == false)
                     return;
 
-                ScoreInfo? topScore = sender.MaxBy(info => (info.TotalScore, -info.Date.UtcDateTime.Ticks));
+                ScoreInfo? topScore = sender.MaxBy(info => (info.TotalScore, -info.Date.UtcDateTime.Ticks)) ?? new ScoreInfo
+                {
+                    Rank = ScoreRank.A,
+                };
                 updateable.Rank = topScore?.Rank;
                 updateable.Alpha = topScore != null ? 1 : 0;
             }

with https://osu.ppy.sh/beatmapsets/2167008#osu/4572759

peppy added a commit to peppy/osu that referenced this pull request Nov 11, 2024
I made these changes while working on
ppy#30579. Basically, it's hard to fix the
ranks not loading while underneath the footer, and the transaprency both
looks bad, and is going away in the redesign.

I've chosen values here that are moving *in the direction* of the new
design without overhauling everything.

- I know that there's still some transparency. I did this because it
helps keep all current elements / colours contrasting without too much
effort.
- I completely removed the transaprency adjustments on the beatmap
panels. This always looked bad due to being applied per-layer, and I
don't think it added much.
peppy added a commit to peppy/osu that referenced this pull request Nov 11, 2024
I made these changes while working on
ppy#30579. Basically, it's hard to fix the
ranks not loading while underneath the footer, and the transparency both
looks bad, and is going away in the redesign.

I've chosen values here that are moving *in the direction* of the new
design without overhauling everything.

- I know that there's still some transparency. I did this because it
helps keep all current elements / colours contrasting without too much
effort.
- I completely removed the transparency adjustments on the beatmap
panels. This always looked bad due to being applied per-layer, and I
don't think it added much.
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Nov 11, 2024
@@ -26,7 +26,7 @@
<ItemGroup Label="Package References">
<PackageReference Include="System.IO.Packaging" Version="8.0.1" />
<PackageReference Include="DiscordRichPresence" Version="1.2.1.24" />
<PackageReference Include="Velopack" Version="0.0.630-g9c52e40" />
<PackageReference Include="Velopack" Version="0.0.869" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This bump makes me INCREDIBLY nervous, to the point where I'm hesitant approving this PR without doing an installer check on all platforms even if everything else looks fine (and it does). Should I be doing one?

Copy link
Member Author

@peppy peppy Nov 12, 2024

Choose a reason for hiding this comment

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

I'd hope we wouldn't need to check each velopack update.. but can understand the concern. Should I just revert for now?

((considering the next release is going to also turn on sdl3 it might be fine to just include this and test/fix quickly at the point of dropping the release))

Copy link
Collaborator

Choose a reason for hiding this comment

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

considering the next release is going to also turn on sdl3 it might be fine to just include this and test/fix quickly at the point of dropping the release

I suppose, although installer failures, if any, tend to have the most... unmitigable (?) consequences, contrary to sdl3 deaths, which we mitigate just by reverting to having it off by default.

I'd looked at velopack changelogs and didn't see much of note other than @smoogipoo's appimage changes and reverts thereof, so maybe fine to just proceed...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the changelog looked tame enough that I was just thinking "let's get off the pre-releases we're currently on".

Copy link
Contributor

@smoogipoo smoogipoo Nov 12, 2024

Choose a reason for hiding this comment

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

The 755 build (which is used by osu-deploy) is a prerelease build that already contains both the appimage change + revert. So there should be no issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On a brief check on linux with osu-deploy it seems like things work, so I'm gonna cross fingers and hope for the best...

@bdach bdach merged commit c25215d into ppy:master Nov 12, 2024
9 of 10 checks passed
@peppy peppy deleted the fix-rank-display-song-select branch November 15, 2024 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:song-select next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TopLocalScore icon gets loaded too late
3 participants