-
Notifications
You must be signed in to change notification settings - Fork 13
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
Allow clients to receive realtime updates for a given playlist #237
Conversation
|
||
for (int i = 0; i < playlistItemIds.Length; ++i) | ||
{ | ||
long[] totalScores = (await connection.QueryAsync<long>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how well this will scale, but I think we can go life with it and deal with that later.
Non-representative tests as we'll likely see a lot more usage than most existing playlists:
select playlist_item_id, count(score_id) from multiplayer_score_links group by playlist_item_id order by
-> count(score_id) desc limit 10;
+------------------+-----------------+
| playlist_item_id | count(score_id) |
+------------------+-----------------+
| 5415805 | 1090 |
| 7091206 | 353 |
| 5415807 | 321 |
| 5827890 | 291 |
| 4350065 | 280 |
| 4296355 | 279 |
| 5415798 | 253 |
| 5344648 | 250 |
| 7091692 | 249 |
| 4296362 | 242 |
+------------------+-----------------+
-- cold query (includes japan-us roundtrip)
select sum(total_score) from multiplayer_score_links join scores on (score_id = scores.id) where playlist_item_id = 5415805;
+------------------+
| sum(total_score) |
+------------------+
| 32986707 |
+------------------+
1 row in set
Time: 0.504s
-- warm query (includes japan-us roundtrip)
select sum(total_score) from multiplayer_score_links join scores on (score_id = scores.id) where playlist_item_id = 5415805;
+------------------+
| sum(total_score) |
+------------------+
| 32986707 |
+------------------+
1 row in set
Time: 0.130s
It shouldn't be a huge reach to cache these values if we need to, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's any solace the intention is that this will only be requested whenever someone enters the daily challenge screen.
Caching can 100% be done even now. It's non-critical data, nobody is going to care about perfect accuracy of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, aware of that. I'm also against caching until we need to.
This pull is intended to be the thing powering recently-added game-side components like ppy/osu#28561 or ppy/osu#28468.
The general design is not intended to explicitly be specific to daily challenge; the API of the feature is supposed to - in theory - apply to any playlist, so if we wanted to we could add these displays to any old playlist room. The one exception is multiplayer; I kinda forget the reason since I wrote the code for this a while back, but I believe it had something to do with the fact that in realtime multi the playlist items are not fixed in time. Now I'm not sure it's an issue anymore but to be honest I don't think realtime rooms are ever going to need this sort of feedback either, so I'm leaving that limitation be there.
Few open questions in my mind remain, which is why I'm PRing this early and not PRing the final piece that integrates this into the game yet
so before all of that is set in stone I'd wanna refrain from getting too deep into it so that I don't have to change things five times over.