Add API endpoints for ranked play rankings#12940
Conversation
| if (is_api_request()) { | ||
| return [ | ||
| ...cursor_for_response( | ||
| empty($scores) || $page >= $maxPages ? null : ['page' => $page + 1], | ||
| ), | ||
| 'ranking' => json_collection($scores, new MatchmakingUserStatsTransformer(), MatchmakingUserStatsTransformer::RANKING_INCLUDES), | ||
| 'total' => $maxResults, | ||
| ]; |
There was a problem hiding this comment.
API should stop using offset-limit and use cursors for pagination; see the stuff with WithDbCursorHelper for reference
There was a problem hiding this comment.
Looking at how the cursor helper works, I'm not sure if it's going to work here. If I understand things correctly, it needs some index that's strictly monotonic across all rows so that the where clause can properly sort rows and do its filtering.
The problem is that several users can have the same rating, which will cause the cursor to skip any records with the same rating that happen to be on the next "page". withRank probably won't work either since it gives the same rank for equal ratings.
Also would this allow fetching an arbitrary page of the rankings? Currently lazer only shows the first page, and it'd be great to bring it to parity with web in that regard.
There was a problem hiding this comment.
you can use a tiebreaker column like
osu-web/app/Models/Multiplayer/UserScoreAggregate.php
Lines 32 to 37 in 23dad9d
Although, if it's the same format as web's rankings page listing, then it should probably stick with offset-limit pagination, but it still needs a tie-breakers or it'll show equal values in whatever arbitrary order the db decides on when querying.
There was a problem hiding this comment.
yeah to my understanding the order of those is still undetermined in the current rankings as they are on prod
i'll need to think it through
i initially wanted to order by username but realized it's a pain to pull in other tables for this purpose
There was a problem hiding this comment.
I wonder if elo_data['approximate_posterior']['sig'] (as seen in #12979) would be an appropriate tiebreaker here, given that it's meant to automatically decay the longer a user has not played. This way we could effectively order people with the same rating so that the most active ones are at the top, which seems instinctively correct.
Mainly I'm wondering if using a field pulled out of JSON for ordering would impact performance negatively. just realized it's already doing that for rating anyway
There was a problem hiding this comment.
I've made it use the sigma value as a tiebreaker, hopefully it's going to work fine. I'm guessing it also needs the indexes adjusted but I'm not confident in that myslelf.
|
|
||
| Route::get('rankings/kudosu', 'RankingController@kudosu'); | ||
| Route::get('rankings/{mode}/ranked-play', 'Ranking\MatchmakingController@index')->name('rankings.matchmaking.index'); | ||
| Route::get('rankings/{mode}/ranked-play/{pool}', 'Ranking\MatchmakingController@show')->name('rankings.matchmaking.show'); |
There was a problem hiding this comment.
this don't match the web routes
There was a problem hiding this comment.
client expects ranking routes to be in a rankings/<ruleset>/<type> format, just like the already existing RankingController route a line below
i could override that on a case by case basis if that's how it should be, but in my view it is the web routes that are inconsistent (some are <ruleset>/<type> while others are <type>/<ruleset>)
There was a problem hiding this comment.
those are from before other type of rankings are added (and to minimise url change during refactor), and some of the newer ones don't even have ruleset so the scheme already fell apart
| $scoresQuery = $pool | ||
| ->allUserStats() | ||
| ->with('user.team') | ||
| ->with(['user', 'user.country', 'user.team']) |
There was a problem hiding this comment.
country shouldn't be preloaded (the relation isn't directly used anymore)
4 participants
This is needed to get ranked play leaderboards in lazer.
Retrieving matchmaking pools and actual rankings is split across two separate endpoints because there's no reason to resend them on every ranking fetch.
Also reworked the controller to use cursors for pagination like
RankingController. I noticed a TODO saying that these should be converted to offset/limit, but I'm not sure how up to date that is, and also it probably makes sense for all rankings to have consistent implementation for pagination, and then be eventually refactored all at once.Also I wonder if we should limit the amount of pages on this, since currently on production there's already >570 pages compared to the global ranking's 200.