Extract leaderboard fetch logic from song select beatmap leaderboard drawable #31881
Conversation
There was a problem hiding this comment.
I guess I'm not really convinced by the abstraction model here. As I mention in the comments below it feels like this is more of a "manager" class functionality-wise, but that means it's doing quite a lot of logic...
- Exposes mutable beatmap/ruleset/mods. Why should player/results ever be concerned about any other mods?
- Exposes mutable scope. On its own this is fine, but I don't see why we wouldn't instead pass this bindable on its own through player/results.
- Exposes
ModFilterActive. At surface level this may be fine, but is this how player should behave as well? For example, I would argue the more common scenario is a player enables this filter while in song select but still wants to see all scores during gameplay.
Additionally, this means that any change to the filtering here is going to take effect in song select as well. I'm not sure this is bad, but let me propose a hypothetical: I have the global leaderboard selected, play through a map and go to results, and just click through tabs because I want to see how I compare to others, but I don't want to remain on the "friends" tab - I want to be put back on the global tab when I'm at song select. One-directional is fine but bi-directionality is what I'm not sure about.
Finally, I'll propose another hypothetical. What if the skinning component could let you choose which leaderboard to display, so that you could display two leaderboards (e.g. country + global) during play? It feels like that would be made impossible with this setup.
It feels like this component is just coupling everything together more and its purpose is not fully fleshed out. Are we trying to cache scores? Are we trying to tie absolutely everything together like this? I'm not sure what the primary goal is.
| RefetchScores(); | ||
| }; | ||
| } | ||
| public StateTrackingLeaderboardProvider? LeaderboardProvider { get; private set; } |
There was a problem hiding this comment.
We cannot have this:
- An inner component creating another component, which is extracted out by a parenting component to be passed into a separate component (i.e. the changes in
PlaySongSelect). - Bindables bound to from within one component ferried through like this (
SongSelect.Beatmap->DetailArea.BeatmapInfo->BeatmapLeaderboard.BeatmapInfo-> back into song select and passed into another component). - Said inner component disposing what has now become an external component.
Is there a reason to not pass this top-down from SongSelect instead? If this relates to the "Leaderboard abomination", then that will need to be fixed first, as this is a blocker for me.
There was a problem hiding this comment.
I'm not exactly sure having this thing at SongSelect level is in our best interests (because that class is too huge) but sure I'll try.
There was a problem hiding this comment.
Also just to be clear, of all the commentary I provided this one is the only blocker for me. The rest is only at the level of "i feel like we're going to have to reconsider this component's place again in the (near) future".
Would very much like this to be top-down unless worlds have to be moved apart to do so.
| /// <see langword="null"/> if fetch is in progress. | ||
| /// Updates to this bindable may not be delivered on the update thread. Consumers are expected to schedule locally as required. | ||
| /// </summary> | ||
| public Bindable<(IEnumerable<ScoreInfo> best, ScoreInfo? userScore)?> Scores => scores; |
There was a problem hiding this comment.
Should be an IBindable<>.
| /// Raised when fetching scores fails. | ||
| /// This event may not be invoked on the update thread. Consumers are expected to schedule locally as required. | ||
| /// </summary> | ||
| public event Action<Exception>? RetrievalFailed; |
There was a problem hiding this comment.
I don't have a better suggestion right now, but I'm not sure about this in the context of being around a BIndable. It works around web requests or Tasks in my mind, but this provider appears to be wanting to be a transparent class that just does everything correctly - that is it's trying to be more akin to a "manager".
There was a problem hiding this comment.
Not sure how to resolve this
| public IEnumerable<ScoreInfo> GetLocalScoresFor(BeatmapInfo beatmap, RulesetInfo ruleset) => realm.Run(r => getLocalScoresFor(r, beatmap, ruleset)).AsEnumerable(); | ||
|
|
||
| public IDisposable SubscribeToLocalScores(BeatmapInfo beatmap, RulesetInfo ruleset, NotificationCallbackDelegate<ScoreInfo> onChange) |
There was a problem hiding this comment.
Is there intent behind exposing these?
There was a problem hiding this comment.
SubscribeToLocalScore() is used. GetLocalScoresFor() isn't and can probably removed, but again, I'm not sure what you're asking for here.
Part of the reason why there's a two-level split of this out from the tracking leaderboard provider thing is mentioned in the OP (e.g. not wanting realm subscriptions to be tied to particular components that may or may not be alive/updating, which breaks scheduling flows, and thus breaks most of anything async).
I don't understand this concern.
My assumption is that the gameplay leaderboard's state should match the song select leaderboard 1:1. To do this, there are more pieces of state to be passed around. Thus, having one component collect all pieces of state and passing that seemed like a better idea than passing five disparate pieces of state.
I have not heard anyone say this, or seen this mentioned in any discussion around this area. This PR was not designed with this in mind.
I have not seen a single results screen design where there are "tabs", so this PR was not designed with this in mind.
I don't want to discuss the in-gameplay part in this PR because it is a disparate thing that will have a disparate solution. This PR is almost orthogonal to it in a sense. To not be 100% evasive, the sketch is that there will be an |
I brought it up because I assumed that's the way it worked in stable. But actually trying it out it's retaining the selected mods, so I suppose there's no issue here.
This is the latest design afaik https://www.figma.com/design/cbYyzJWrG68hQw3MincySk/Client---Result-Screen, and I took the bottom part to represent tabs given that one of them appears to be in a selected state: 
But maybe that's just so that it corresponds to the progress bar below it. Dunno. |
i don't think i've ever seen this figma. the github project that was supposed to stay up to date and be the source of truth on this links to this figma which i have seen and doesn't appear to feature a leaderboard selector. @arflyte please advise |
|
Coincidentally, that's the first time I've seen that design (was going by the figma library in internal #design) 😅 Well, alright then... |
|
While it's the latest design (https://www.figma.com/design/f7Gj5AHR79C9QctfS0HHkK/Result-Screen-8?node-id=0-1&p=f&t=59oCJzAW0PDKi7zv-0), it's pending for review and update; it's far from complete. It will be the next focus after Song Select. This (https://www.figma.com/design/cbYyzJWrG68hQw3MincySk/Client---Result-Screen) works for the current implemented design. |
somehow this explanation has made me even more confused so let me try and restate the question: is either figma supposed to be used for anything at all? |
|
I'm not sure what exactly is trying to be implemented here. If you're looking for a way to quickly implement leaderboard tabs, then yes, you may use this for now: https://www.figma.com/design/cbYyzJWrG68hQw3MincySk/Client---Result-Screen This works for the currently implemented Result Screen. This https://www.figma.com/design/f7Gj5AHR79C9QctfS0HHkK/Result-Screen-8?node-id=0-1&p=f&t=59oCJzAW0PDKi7zv-0, is still being worked on, and shouldn't be used until it's fully reviewed. |
|
I'm still getting through reviewing this PR, but in the mean time:
I didn't see these as tabs (the design cues make them look like static elements), but let's take a step back for a moment. Designs and designers shouldn't be part of this conversation. Design is driven by actual behavioural requirements and not vice-versa. From a functional perspective, giving users the ability to view different leaderboard categories from the results screen may be nice and I'm sure some users will ask for it, but it also means we need full filter controls available as users will ask for every leaderboard style available. To keep scope in check, let's say that this can't be done for now, and results screen just shows the same leaderboard that was selected at song select. I would still see this selection being game-wide. If structured correctly, this would mean that in a future we could allow the current displayed leaderboard at results to be changed (and it would update the global selection), which may be something to keep in mind when making implementation decisions. Keep it simple, one state object defining the leaderboard shown game wide. |
sure, but i'd also say that because designs should be driven by requirements, looking at designs can be a contextual cue for me to deduce behavioural requirements that may have been laid down in conversations i was not party to. establishing whether we want leaderboard filter controls on the results screen at all is one key factor in this entire series that i would like to be clarified before i proceed any further, because it may invalidate core assumptions behind this PR. |
|
The only way I could surmise that affecting this change is whether the state is mutable or not post-results-screen-load, would that be correct? or put another way, should the results screen be listening to a bindable, or receive a non-mutable state at construction? |
The way I foresee this series proceeding and have it prepared, unless a major shift in direction occurs, is that the collection of scores to display on the leaderboards and then in results is going to be more or less frozen at the state it was in song select at the point of entering player, which would be closer to the latter than the former. If that ceases to be true, further work on results screens might be required. |
|
As discussed, I think a starting point would be to have a global component which manages the leaderboard scores, something like this: class LeaderboardCache // i think this most closely matches other cache classes in usage?
{
readonly Bindable<LeaderboardScope> GlobalScope; // either lives here or in song select or OsuGame, doens't really matter just needs to be somewhere.
readonly ILeaderboardContext GetScores(LeaderboardScope scope);
private class Context : ILeaderboardContext
{
// scores list updated by cache
// disposal used to allow leaderboard cache to clean up after all usages are gone.
}
}
class ILeaderboardContext : IDisposable
{
IBindableList<ScoreInfo> Scores;
void Invalidate(); // alternative: Refetch();
}
record LeaderboardScope
{
BeatmapLeaderboardScope Scope;
BeatmapInfo Beatmap;
RulesetInfo Ruleset;
Mod[]? RequiredMods;
}I've probably missed things here so please apply salt. |
|
@peppy @smoogipoo can I get both of you to take 15 minutes reading through https://github.com/ppy/osu/compare/master...bdach:osu:yet-another-leaderboard-rewrite-attempt?expand=1 and telling me if I'm going in the right direction? I wrote it to mostly follow the API proposal in #31881 (comment) and it sort of appears to function but I'm not convinced it's going to alleviate all of your concerns.
Not sure where to take this further. If it helps we can voice at some point. |
|
My biggest problem with that changeset is the following:
Resolving both a manager and a provider sounds extremely flimsy to me. You say "pulling all of the leaderboard filtering state out to SongSelect" but I don't understand why this is required... What logic does SongSelect need for leaderboards? The whole point of the last discussion we had is that there was one global manager that linked things together, no? That is, it would be Player/Results/this leaderboard, all resolving in something that is shared through the global manager. I still cannot come to accept this structure as is... sorry. Is it a case of me not understanding the complexities involved? Other than that, it looks a little bit more tenable than this branch. |
I could make all the leaderboard state game global and pull it out to the leaderboard manager class or whatever, but that's also going to change the API shape enough to no longer match @peppy's proposal (and go even farther away from the "cache" idea)... |
|
I think it might be easiest for me to hold off from reviewing further, because it feels like I'm being a thorn. I'll let @peppy review things for now and see whether he has a better view of the path you're heading in. |
The reasoning for making it "cache" to some degree is to cover the scenario of gameplay and results screen use the same content as song select. With the way you've structured this, I believe the idea is to cache the "resultant" If so, this sounds fine until we potentially want to switch the source or criteria at the results screen, but I think that can be dealt with separately. |
|
As for the concerns smoogi raises about the song select bindable, I tend to agree. In my API, the |
…drawable RFC. Another attempt at this. - Supersedes ppy#31881 - Supersedes / closes ppy#31355 - Closes ppy#29861 This is a weird diff because I am feeling rather boxed in by all the constraints, namely that: - Leaderboard state should be global state - But the global state is essentially managed by song select and namely `BeatmapLeaderboard` itself. That's because trying to e.g. not have `BeatmapLeaderboard` pass the beatmap and the ruleset to the global leaderboard manager is worse, as it essentially introduces two parallel paths of execution that need to be somehow merged into one (as in I'd have to somehow sync `LeaderboardManager` responding to beatmap/ruleset changes with `BeatmapLeaderboard` which is inheritance hell) - Also local leaderboard fetching is data-push (as in the scores can change under the leaderboard manager), and online leaderboard fetching is data-pull (as in the scores do not change unless the leaderboard manager does something). Also online leaderboard fetching can fail. Which is why I need to still have the weird setup wherein there's a `FetchWithCriteriaAsync()` (because I need to be able to respond to online requests taking time, or failing), but also the `BeatmapLeaderboard` only uses the public `Scores` bindable to actually read the scores (because it needs to respond to new local scores arriving). - Another thing to think about here is what happens when a retrieval fails because e.g. the user requested friend leaderboards without having supporter. With how this diff is written, that special condition is handled to `BeatmapLeaderboard`, and `LeaderboardManager`'s state will remain as whatever it was before that scope change was requested, which may be considered good or it may not (I imagine it's better to show scores in gameplay than not in this case, but maybe I'm wrong?)
|
Superseded by #32494 |
This is an RFC. It appears to be fully functional and pass tests, but I'm not super sure on the direction here, so opening as draft.
The key points:
The core logic of online and local leaderboard fetches has been moved out to
LeaderboardProvider- a component atOsuGameBaselevel. The reason for pulling it up there is to avoid many stupid issues related to presence, schedules not running, etc., that would result from putting that component in one piece inSongSelect.SongSelecthas a second component -StateTrackingLeaderboardProvider- that is responsible for tracking the user filter preferences and calling appropriateLeaderboardProvidermethods. This component is passed around toPlayerimplementations which fixes Leaderboards do not show when a user goes into gameplay immediately #29861, and that is later intended to daisy-chain further intoResultsScreenimplementations, which is prospective towards fixing Results screen should change based on the leaderboard you have selected in song select #26331. Watching a replay does not show leaderboard #27609 is another related issue.In Remove fetching logic from leaderboard #31355 I knocked the idea of doing state tracking, but I concede that having a thing that does it is easier than not, because you can pass this stuff forward without needing to pass forward the entirety of song select's leaderboard scope state.
However, contrary to that pull, there are no weird generics here and no overfitted structures. Everything is written straight, for the specific use case of leaderboards in song select which I consider disparate and not 100% compatible with leaderboards in gameplay (more on that later down the chain of PRs, if this one passes review scrutiny).
I admit out of the gate that the hookup of
StateTrackingLeaderboardProvidertoBeatmapLeaderboardis shoddy at best - theStateTrackingLeaderboardProvideris reconstructed more than it needs to. That is becauseBeatmapLeaderboardand its baseLeaderboardabomination are bad and should be nuked from orbit. I am hoping this can happen with the song select leaderboard, which is why I'm tagging @frenzibyte in here to examine the structure. The hope is that the new implementation can focus on display logic and rely on the provider to provide scores, rather than having all of thatRefetch()stuff.StateTrackingLeaderboardProviderkeeps it head out of all of the special error logic (showing the messages about the beatmap not having online leaderboards, not having supporter, etc.) I was in two minds about this, and in the end I think this is better, but it's not a strong opinion and I am open to reconsideration.