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

Prevent invalid high scores from being saved #898

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

thomeval
Copy link

This PR adds validation to Band and Solo high scores, meaning that they will be considered invalid and thus not saved when certain conditions are met. This also includes a "Band score not saved" message on the Result screen. Unfortunately, it is not possible to filter out already existing invalid high scores, so the validation rules will only apply to scores set after this PR is merged.

Currently, a band high score will be considered invalid if the Song Speed is less than 100% :
Screenshot 2024-10-08 142841

Or if any bots are active:

Screenshot 2024-10-10 141616

It will be fairly simple to add more rules later by extending the ScoreContainer.IsSoloScoreValid() and ScoreContainer.IsBandScoreValid() methods.

Known Limitation: In some circumstances, it is possible for a Band Score to be invalid, but a player's Solo Score to still be valid (for example, when playing with bots). In such cases, we should save the player score, but not the band score. Assuming there are no blockers preventing it, this will be addressed in the Solo High Score overhaul PR.

Copy link
Member

@EliteAsian123 EliteAsian123 left a comment

Choose a reason for hiding this comment

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

Hello! Sorry for the late review again.

After some changes below, this should be good to go!

@EliteAsian123
Copy link
Member

In some circumstances, it is possible for a Band Score to be invalid, but a player's Solo Score to still be valid (for example, when playing with bots).

For future proofing, it's probably better for this not to be a thing whenever band multipliers and stuff come into play.

@thomeval
Copy link
Author

Thanks for the feedback, I've pushed a change that should resolve everything.

Copy link
Member

@EliteAsian123 EliteAsian123 left a comment

Choose a reason for hiding this comment

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

The code is all good! I'm away right now so once I get back I'll test this in-game and this should be good to merge (if one of the other maintainers could do that, that could work too)

@EliteAsian123 EliteAsian123 merged commit a06b7dc into YARC-Official:dev Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants