Skip to content

Add song select beatmap leaderboard display #32844

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

Merged
merged 24 commits into from
Apr 30, 2025

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Apr 18, 2025

Preview:

CleanShot.2025-04-18.at.07.00.44.mp4

Notes:

  • @peppy Seeing how BeatmapLeaderboard implements score fetching, I've attempted to follow suit and do the exact same to have full functionality support in de9146c. Feel free to revert / force-push this commit away if you still think it shouldn't be done here.

  • If a score card goes partially off-screen, then resizing the width back and fourth doesn't work correctly. This is happening due to the parent of rankLabelStandalone getting masked away completely, and therefore not processing transforms to that component. I have no idea how to fix this one because the parent here is a cell container of GridContainer, somehow GridContainer.CellContainer needs to have IsMaskedAway never set to false.

    CleanShot.2025-04-18.at.07.04.04.mp4
  • Total leaderboard scores count that was added in Include score count in beatmap scores response osu-web#12018 has not been implemented yet, as it requires minor changes to LeaderboardManager to carry the value across to consumers. To be done in a follow-up.

@frenzibyte frenzibyte requested a review from peppy April 18, 2025 11:17
@frenzibyte frenzibyte force-pushed the song-select-v2-wedges-leaderboard branch 2 times, most recently from de9146c to 21d670d Compare April 18, 2025 12:51
@frenzibyte frenzibyte force-pushed the song-select-v2-wedges-leaderboard branch from 21d670d to 81d54a9 Compare April 18, 2025 12:54
@peppy
Copy link
Member

peppy commented Apr 22, 2025

@peppy Seeing how BeatmapLeaderboard implements score fetching, I've attempted to follow suit and do the exact same to have full functionality support in de9146c. Feel free to revert / force-push this commit away if you still think it shouldn't be done here.

You copied a bug and missed a test, see #32816. Fixed via peppy@7a18a77.

peppy added 2 commits April 22, 2025 18:01
We'll need to figure out what to do in multiplayer cases in the future,
but the hope is that it can be done without further subclassing if
possible.
@peppy
Copy link
Member

peppy commented Apr 22, 2025

Cases like 26f2703 make me very wary that you haven't run through all added tests on a visual level.

peppy added 2 commits April 22, 2025 18:30
This removes a lot of movement, but honestly it didn't feel good in the
first place. If anything I'll come back with a second-pass animation
pass on this.
@frenzibyte
Copy link
Member Author

frenzibyte commented Apr 22, 2025

Cases like 26f2703 make me very wary that you haven't run through all added tests on a visual level.

That's a test from master branch and doesn't look like one I need to carefully pay attention to, it displays non-sheared version while I'm working completely on sheared version. I'm not sure I have to be responsible for that.

I have tested each and every test case that matters to this PR chain and song select.

Leaderboard regression fix came in an unfortunate time as I've already prepared it for PR. Good find is all I can say.

peppy added 2 commits April 23, 2025 18:30
Also handles some display edge cases where scores may overlap
placeholder or do other weird things.
@peppy
Copy link
Member

peppy commented Apr 23, 2025

@frenzibyte we have four mod tooltips now. the one in the beatmap score is only used, even though it could probably be applied to all mod switch types (and maybe all mods icons?):

JetBrains Rider 2025-04-23 at 09 56 36

what's the forward direction here?


Put another way: rather than adding the mod tooltip locally in a subclass of ModSwitchTiny, can it not be added to all mod switches?

}

private sealed partial class MoreModSwitchTiny : CompositeDrawable
private sealed partial class MoreModSwitchTiny : CompositeDrawable, IHasPopover
Copy link
Member

Choose a reason for hiding this comment

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

A button called a switch, with no sounds attached.

Copy link
Member Author

Choose a reason for hiding this comment

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

Has been removed as part of moving from tiny mod icons to the old full ones.

@peppy
Copy link
Member

peppy commented Apr 23, 2025

BeatmapLeaderboardScore is, nothing short of a pile of turd.

Yes, I get that it's not your original code, but it also feels like you've just turned a blind eye to it while also refactoring a good portion of the class.

As one example, it has a heap of local animations which are only activated in the TestSceneBeatmapLeaderboardScore (and look completely broken), but never seen outside of this. I'd have a hope that when you bring classes like this online – even in a design pass – you'd catch on to these things and amend them.

@peppy peppy self-requested a review April 24, 2025 11:32
@frenzibyte
Copy link
Member Author

@frenzibyte we have four mod tooltips now. the one in the beatmap score is only used, even though it could probably be applied to all mod switch types (and maybe all mods icons?):

JetBrains Rider 2025-04-23 at 09 56 36

what's the forward direction here?

Put another way: rather than adding the mod tooltip locally in a subclass of ModSwitchTiny, can it not be added to all mod switches?

I have responded to this in discord as there needs to be further discussion before settling on a direction and it tends to stray away from this PR (none of the changes in this PR play around with mod tooltips).

@peppy
Copy link
Member

peppy commented Apr 28, 2025

would it not make more sense to have the score popover show all mods with all details rather than having a local clickable expand thing just for mods for some arbitrary reason?

@frenzibyte frenzibyte force-pushed the song-select-v2-wedges-leaderboard branch from cdbd3c9 to 23a9e34 Compare April 29, 2025 03:25
@frenzibyte frenzibyte force-pushed the song-select-v2-wedges-leaderboard branch from 23a9e34 to ca11f33 Compare April 29, 2025 03:25
frenzibyte and others added 2 commits April 30, 2025 07:20
I'm not sure why this is a thing but let's not do it without proper
rationale.
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

50% confidence

@peppy peppy merged commit d48bfd1 into ppy:master Apr 30, 2025
7 of 9 checks passed
@frenzibyte frenzibyte deleted the song-select-v2-wedges-leaderboard branch April 30, 2025 05:45
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.

3 participants