Show results in results screen based on leaderboard selected in song select #27861
Conversation
| using osu.Game.Scoring; | ||
|
|
||
| namespace osu.Game.Screens.Ranking | ||
| { | ||
| public partial class SoloResultsScreen : ResultsScreen | ||
| { | ||
| private GetScoresRequest? getScoreRequest; | ||
| public readonly IBindableList<ScoreInfo> Scores = new BindableList<ScoreInfo>(); |
There was a problem hiding this comment.
So looking at the actual code, my primary question here would be - does this need to be bindable?
Once you enter results, the scores are not going to change until you exit back to song select. So is the complexity incurred from handling having this change worth it?
Additionally, there's also #27609 which is still an issue even with this bindable, the score retrieval still strongly depends on song select (which it shouldn't).
Dunno. All this is making me question whether further review of this diff is worth the time.
There was a problem hiding this comment.
The only difference is that if the results are delayed, then the results wouldn't show. One of the tests tested delayed fetching, so I didn't wanted to break that. This is highly unluckily and I wrote my concerns about it in my initial comment.
I also agree that the whole score retrival should be refactored. I can do that but you probably don't want me to. I decided to take scores in the same fashion as SoloGameplayLeaderboard, so the eventual refactoring might be easier.
| AllowRetry = true, | ||
| ShowUserStatistics = true, |
There was a problem hiding this comment.
Why are these removed from here? I would hope they could stay here. Otherwise this is going to take significant checking that every single derived implementation sets these correctly.
There was a problem hiding this comment.
I mean sure, but only EditorPlayer would not override this in the current form.
| protected override void Update() | ||
| { | ||
| base.Update(); | ||
|
|
||
| if (lastFetchCompleted) | ||
| { | ||
| APIRequest nextPageRequest = null; | ||
|
|
||
| if (ScorePanelList.IsScrolledToStart) | ||
| nextPageRequest = FetchNextPage(-1, fetchScoresCallback); | ||
| else if (ScorePanelList.IsScrolledToEnd) | ||
| nextPageRequest = FetchNextPage(1, fetchScoresCallback); | ||
|
|
||
| if (nextPageRequest != null) | ||
| { | ||
| lastFetchCompleted = false; | ||
| api.Queue(nextPageRequest); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
What is happening here? Why is this in update? Why is it not being handled like every single score request flow in every other results screen implementation?
There was a problem hiding this comment.
I moved this from the previous implementation of ResultsScreen, it allowed fetching next pages of scores. However, it only really made sense for PlaylistResultsScreen, other implementations of FetchNextPage only returned null. Thus, I moved it.
Alternatively, I can make MultiPageResultsScreen, which implement this.
|
I'm personally deprioritising further review of this as it was discussed during today's catch-up meeting that the future of the leaderboard on the results screen is not certain (it may even be that the results screen won't fetch other results at all anymore). |
Resolves #26331, resolves #9201.
Now
SoloResultsScreengets scores fromLeaderboardScoresinPlayer, similarly toSoloGameplayLeaderboard.I'm not so sure about how I implemented
PopulateScorePanelListinSoloResultsScreen. The idea was that, if the scores are already loaded, thenhasLoadedprevents eventual changes that might happen later but still allow delayed fetching of scores (if they somehow didn't have time to load during gameplay and in song select?).Maybe that can be improved/changed though.
Other thing that changed because of that is the
SpectatorResultsScreen, similarly to theGameplayLeaderboard, doesn't show any other scores, because of lack ofLeaderboardScoresinSoloSpectatorPlayeredit:
Tests seem to fail due to changing of
testResults, even though on my local machine dotnet test gives 0 fails.Are the tests performed parallelly and in normal situation it wouldn't occur, or just logic in
SoloResultsScreenis not thread safe?