Allow beatmap tagging if at least one score is eligible#36992
Allow beatmap tagging if at least one score is eligible#36992imvanni wants to merge 10 commits intoppy:masterfrom
Conversation
bdach
left a comment
There was a problem hiding this comment.
- Tests would go a long way here, see
TestSceneStatisticsPanel - There's also code quality inspections here but I'll let CI handle that one
| var localUserScore = AchievedScore; | ||
|
|
||
| if (localUserScore == null) |
There was a problem hiding this comment.
I'm not sure this makes sense anymore? If the point is to allow tagging if any local score matches, then there's little reason to still prefer the latest set score if the user has already proven themselves capable to tag.
| && (!score.Mods.Any(m => (m.Type == ModType.Conversion) | ||
| || m is not ModClassic)); |
There was a problem hiding this comment.
I'm not even sure this is correctly transcribed from code lower down. m.Type == ModTypeConversion || m is not ModClassic is not the same thing as (m.Type == ModType.Conversion) && !(m is ModClassic).
This should either be && in both places or the first should be correctly negated as per generalized De Morgan's laws.
In general there's kind of little reason why this needs to be duplicated like this. I imagine that you could split a method like getReasonForPreventingTagging(ScoreInfo) and then use it here instead in a getReasonForPreventingTagging(score) == null manner.
|
Oh yeah, apologies for the spam actions. |
|
As for the tests... For the one I was supposed to add here, I repurposed While I was here I added the missing ones I was supposed to add back in #36684. |
| popOutSample = audio.Samples.Get(@"Results/statistics-panel-pop-out"); | ||
| } | ||
|
|
||
| private string? getReasonFromPreventingTagging(ScoreInfo newScore, ScoreInfo? localScore) |
There was a problem hiding this comment.
Why does this method accept two scores? That should not be necessary. localScore seems like it shouldn't be here.
There was a problem hiding this comment.
It was for comparing rulesets. But yeah, realized it's not required. Just passing the expected ruleset OnlineID should be enough, yes?
There was a problem hiding this comment.
I would like to think that even passing ruleset ID explicitly would be gratuitous and all you need is newScore.Beatmap.Ruleset.OnlineID, but I could be wrong on that. Needs checking.
There was a problem hiding this comment.
That'd mean putting the method inside the IEnumerable which renders the existence of said method useless.
If anything I'd convert it to something like
if (localUserScores.Length == 0)
localUserScores.Append(AchievedScore);
foreach (ScoreInfo score in localUserScores)
{
preventTaggingReason = ...;
if (preventTaggingReason == null)
break;
}Except I believe that the first step is both incorrect and unnecessary. Incorrect since it'll take any score, even if it's not local, and unnecessary, because the locally achieved score should've been written to the Realm already, meaning the count isn't zero. Correct me though. It existed while I was writing the tests, because it would always yield a "not played" status, but then figured it's entirely wrong.
Closes #36910.
Instead of picking the top score to see if it's eligible for tagging, find at least one score that matches all criteria for it.
This process is only done when trying to do so on the beatmap list (as in, not after playing) because to be fair I don't think it's necessary, unless we decide otherwise. This really just covers the very edge cases where you may have a better score with a Mirror mod for example.