Skip to content
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

Remove Passed flag from score classes #28057

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frenzibyte
Copy link
Member

I'm not sure how to go about doing the above myself so I'm leaving it as a task for others to tick off.

In favour of `ScoreRank` now having an F rank to define that.
@peppy
Copy link
Member

peppy commented May 2, 2024

Check all consumers of SoloScoreInfo/MultiplayerScore no longer rely on the now-removed passed flag

I don't really see this as a prereq. That can be handled as required. Except MutiplayerScore due to serialisation, needs checking.

@bdach
Copy link
Collaborator

bdach commented May 2, 2024

One thing I haven't seen discussed clearly anywhere is that this only works if we hard commit that all failed scores have F rank and that all F ranks are failures. I dunno how that's gonna pan out with stuff like multiplayer wherein people want the "recover from failure" feature to return at some point - like are we going to allow them to recover but keep F rank (i.e. current behaviour)? Or is there going to be something more convoluted here going on (like allow them to get a higher rank but then also mark the score as unranked via some means, which would probably be most easily achieved by keeping rank and passed separate and setting rank to non-F while keeping passed = false?)

I'm not sure how to go about doing the above myself so I'm leaving it as a task for others to tick off.

Also this is maybe a personal problem but this sorta thing really ticks me off personally. Review is review yes but just offloading part of the testing that doesn't really even require any special permissions or access to do is kinda bad taste imo. I'd like to say that I'd personally never actually do that.

@frenzibyte
Copy link
Member Author

frenzibyte commented May 2, 2024

I understand your concern but I'm not well-versed with how this is used everywhere due to either language (e.g. osu-web) or not knowing all the repositories that use these models (all I remember is osu-queue-score-statistics), so I'm keeping this task for someone who knows well about this and can execute it off cleanly.

If it turns out that there is the slightest of a problem or concern from this change then I'm fine with closing this PR and leaving it until later, I don't want to stir any attention.

Regarding the "recover from failure", I would rather say rank should be meaningless when a score has failed at least once, because it's not going to add to anything for the user's statistics, and won't be submitted anywhere as if it's actually an "S" score for example.

If anything the rank can be displayed based off the accuracy somewhere on the side of the results screen while keeping F as the effective rank.

@peppy
Copy link
Member

peppy commented May 2, 2024

I'd hope we don't have two passed states. And if we need something for multiplayer ("imminent" passing state) I'd really hope that was scoped locally to multiplayer as a Passing variable.

Comment on lines -30 to -31
[JsonProperty("passed")]
public bool Passed { get; set; }
Copy link
Collaborator

@bdach bdach May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well for one thing I can certainly tell you that this cannot fly.

https://github.com/ppy/osu-web/blob/f73ea11049f333731c09ec9b1b6c1141a65a8320/app/Models/Solo/Score.php#L85-L90
https://github.com/ppy/osu-web/blob/f73ea11049f333731c09ec9b1b6c1141a65a8320/app/Models/Solo/Score.php#L123

Removing this flag will likely break things to the point wherein no submitted score will have preserved set. If this diff is to proceed we will need to wean off web from the flag first and deploy that change.

(That does not really require osu-web knowledge to test either by the way. It just requires a local environment.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to say I miswrote the PR's description, I meant to say all consumers of SoloScoreInfo should be changed to no longer use the property...totally my bad though.

I saw those usages, and that's actually exactly why I didn't pursue this any further and left this as a task for someone more knowledgable in osu-web to change things around there (replace passed with rank != F).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScoreInfo.Passed should be deprecated in favour of F rank
3 participants