-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix incorrectly encoded score IsPerfect value #27668
Conversation
@@ -93,7 +93,7 @@ public void Encode(Stream stream, bool leaveOpen = false) | |||
sw.Write((ushort)(score.ScoreInfo.GetCountMiss() ?? 0)); | |||
sw.Write((int)(score.ScoreInfo.TotalScore)); | |||
sw.Write((ushort)score.ScoreInfo.MaxCombo); | |||
sw.Write(score.ScoreInfo.Combo == score.ScoreInfo.MaxCombo); | |||
sw.Write(score.ScoreInfo.Combo == score.ScoreInfo.GetMaximumAchievableCombo()); |
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 even sure what was happening here? The old implementation looks completely wrong and so does this one for that matter.
Why was this comparing two values from ScoreInfo
to begin with? Why is Combo
in the comparison at all, if it appears to be the current running combo for spectator purposes? Is this even guaranteed to be populated correctly in the database? In mine I have several scores matching Combo != MaxCombo
.
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.
You're right that this should've been MaxCombo
. Though I would hope Combo == MaxCombo
in an IsPerfect == true
scenario, otherwise that sounds like an issue.
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.
You're right that this should've been
MaxCombo
. Though I would hopeCombo == MaxCombo
in anIsPerfect == true
scenario, otherwise that sounds like an issue.
Imagine having an object with (MaxResult = IgnoreHit, MinResult = ComboBreak)
at the end of a map => false negative. I don't think we do this yet though, meaningfully (hold notes do but you can't exploit this on a hold note because the tail still exists).
Aaaaalso... one-object map, miss everything; MaxCombo == Combo == 0
.
What that check basically amounted to is "did you end the map on your longest combo".
I'm not even sure why we're persisting Combo
to db at this point but /shrug
Fixes the score appearing as perfect, as mentioned in ppy/osu-stable-issues#1219
We may want to deploy a new osu-server-spectator with this, but it isn't a huge deal in my opinion (only affects stable afaik).
The test is kinda wishy-washy by manually putting the score into the correct state. I can make it a
Player
-based test if requested, but it felt like too much.