feat: per-mod personal best score on leaderboard

[g1-1-1]

Describe your changes

previously, the query was hardcoded to only find the personal best without accounting for the different leaderboards -- now, it works similarly to bancho.

video

Related Issues / Projects

closes #237

Checklist

• I've manually tested my code
• The changes pass pre-commit checks (make lint)
• The changes follow coding style

[g1-1-1]

lgtm 👍

[7ez]

lgtm 👍

cus u made it

[tsunyoku]

this doesn't account for the fact that the user may have multiple submitted scores for a mod combo, you need to add a 1 score per user constraint to the query

[g1-1-1]

this doesn't account for the fact that the user may have multiple submitted scores for a mod combo, you need to add a 1 score per user constraint to the query

i've ran this in my production server for about 2 months now, every case where i've submitted a score with a different mod combination (which would be the best for that combo) seems to work just fine.. if you still think it is a problem i could add what you've requested but it seems kinda redundant

[tsunyoku]

it's not redundant, you're just lucky.

[g1-1-1]

how can one be lucky so many times? i'll look into it regardless

[tsunyoku]

just think about the issue i handed you. it needs to be handled no matter how many cases you show me of it seeming to work correctly

[skrungly]

how can one be lucky so many times? i'll look into it regardless

i am willing to guess (though i will also test to check once i get time) that the client is generous and automatically filters for the best personal score for each person who is in the leaderboard list given by the server. however, our server is limiting the search to 50 rows, several of which may be from the same person. once the client has filtered this out, there will not necessarily be 50 rows remaining, and your top 50 is no longer a top 50. if that is the case upon testing then i'll let you know so you can refine your patch! :D

i've implemented a slightly different solution on my own server using some SQL inner join mess, but i don't think it should be particularly taxing on performance? that being said, i haven't been able to thoroughly test it on a large user base. the relevant commit is skrungly@a2b1cf2

[tsunyoku]

i am willing to guess (though i will also test to check once i get time) that the client is generous and automatically filters for the best personal score for each person who is in the leaderboard list given by the server. however, our server is limiting the search to 50 rows, several of which may be from the same person. once the client has filtered this out, there will not necessarily be 50 rows remaining, and your top 50 is no longer a top 50.

yeah, this is what is happening. didn't state it explicitly as i wanted them to think about the problem, but it's okay. your solution works, but it's not very comfortably readable 🤔 (namely the query.insert choice)

[tsunyoku]

i'm also generally concerned by some of the choices you made with that query. selecting all submitted scores in every case is an unnecessary performance harm, it is only in the mod leaderboard case that we should be considering multiple scores + filtering one per user. we should preferably use status = 2 for all other leaderboard types, with status != 0 being kept for mod leaderboards only. the extra join is also an unnecessary performance harm outside of the mod leaderboard scenario

[skrungly]

yeah, this is what is happening. didn't state it explicitly as i wanted them to think about the problem, but it's okay.

i know :D it's just that it had been a week without any updates to the PR or the discussion, so i figured i might give some pointers.

your solution works, but it's not very comfortably readable thinking (namely the query.insert choice)

i completely agree. in fact, the query.append is totally redundant alongside the query.insert, so the append shouldn't even be there. even still, it would be more readable if i had just used a COALESCE of some sorts instead. thanks for reminding me to think about this, i had meant to make it more readable before making the commit but was too tired to bother.

i'm also generally concerned by some of the choices you made with that query. selecting all submitted scores in every case is an unnecessary performance harm, it is only in the mod leaderboard case that we should be considering multiple scores + filtering one per user. we should preferably use status = 2 for all other leaderboard types, with status != 0 being kept for mod leaderboards only. the extra join is also an unnecessary performance harm outside of the mod leaderboard scenario

in my own server, this behaviour is intentional since it mostly matches what you get on the ppy bancho leaderboards, though that's just based on my own and my players' preferences. i didn't intend for my patch to be a drop-in solution by any means! instead, refer to the discussion in #237, where there hasn't really been a firm decision on what the general behaviour should be for bancho.py.

regardless of this, i understand that the query may not be very optimal! i'm just not very well-practised in the way of sql just yet. i'm working on it though hehe

[tsunyoku]

heh that's no problem! just wanted to give some feedback :p

i'll let this pr sit for a bit given the discussion and see if @g1-1-1 decides to make any changes. if not, i'll close it and keep it as an open issue for now

[g1-1-1]

i'll work on this in a few, you can limit 1 per user using raw sql but it is tricky to pull off

[cmyui]

I'll close this PR for now -- I've created an issue to track this feature request: #560