Ensure ranks are properly populated in spectated scores #231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Spectator server dies at runtime without this.
This begins to matter after ppy/osu#28058, as with that pull the rank is encoded to the replay, so it must be correctly set on the server side. Otherwise spectator server will default to storing D rank, which the client will happily read and use raw without checking. This is purposefully computed server-side rather than sent in replay headers for the reasons of simplicity and backwards compatibility. You could add score rank to the replay headers, but to ensure that replays recorded with old clients don't have D rank set, recomputing server-side would have to be done at least for the old clients. And you could argue recomputing ranks always server-side removes a tamper vector too (although that's not a very strong argument).
|
@bdach i'm quite confident it is, but just to confirm, this is fine to deploy immediately yes? |
peppy
approved these changes
May 7, 2024
|
As far as I'm aware yes, this change is basically inert until the next game package bump. |
|
Inert assuming no funny business, sure. I ask because I was planning a new spectator deploy today, will include this 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning
This - or an alternative PR fixing this same issue - must be merged before the next bump of game packages in spectator server. Without fixing this issue one way or another, all online replays after next game bump & deploy thereof will have rank D stored to the replay and read client-side.
RFC.
This begins to matter after ppy/osu#28058, as with that pull the rank is encoded to the replay, so it must be correctly set on the server side. Otherwise spectator server will default to storing D rank, which the client will happily read and use raw without checking.
This is purposefully computed server-side rather than sent in replay headers for the reasons of simplicity and backwards compatibility. You could add score rank to the replay headers, but to ensure that replays recorded with old clients don't have D rank set, recomputing server-side would have to be done at least for the old clients. And you could argue recomputing ranks always server-side removes a tamper vector too (although that's not a very strong argument).
Also includes a sentry package bump in e160320 - without this the server dies at runtime with newest game packages.