Skin component "DefaultRankDisplay" and "LegacyRankDisplay" blink too frequently when getting SH rank

fuyukiSmkw commented 4 months ago

Type

Cosmetic

Bug description

When getting an S rank with mod Hidden (which gives you the rank SH), DefaultRankDisplay and LegacyRankDisplay blinks every time a note is hit, which is not expected compared to the normal behavior that it should only blink when the rank changes. This issue seems to only happen to SH rank, and other ranks such as XH (SS with Hidden), X (SS without Hidden), S (S without Hidden), etc. seem to be not affected. This issue seems to happen to all mod combinations that include Hidden.

Screenshots or videos

https://github.com/ppy/osu/assets/33680223/bada7b23-4050-42c1-b958-d935108745f4

Version

2024.625.0-lazer

Logs

compressed-logs.zip

fuyukiSmkw commented 4 months ago

Add: This also seems to happen to taiko, ctb and mania.

cdwcgt commented 4 months ago

The problem is in

https://github.com/ppy/osu/blob/b4cefe0cc2fda0ab4b5af6138ee158bd32262f9a/osu.Game/Rulesets/Scoring/ScoreProcessor.cs#L384-L386

code here update the rank twice.

so we have

Only update rank once by @PercyDan54


diff --git a/osu.Game/Rulesets/Scoring/ScoreProcessor.cs b/osu.Game/Rulesets/Scoring/ScoreProcessor.cs
index 0b20f1089a..d5ce50e395 100644
--- a/osu.Game/Rulesets/Scoring/ScoreProcessor.cs
+++ b/osu.Game/Rulesets/Scoring/ScoreProcessor.cs
@@ -381,9 +381,11 @@ private void updateRank()
         if (rank.Value == ScoreRank.F)
             return;

rank.Value = RankFromScore(Accuracy.Value, ScoreResultCounts);
ScoreRank newRank = RankFromScore(Accuracy.Value, ScoreResultCounts); foreach (var mod in Mods.Value.OfType())
rank.Value = mod.AdjustRank(Rank.Value, Accuracy.Value);
newRank = mod.AdjustRank(newRank, Accuracy.Value);

rank.Value = newRank; }

 protected virtual double ComputeTotalScore(double comboProgress, double accuracyProgress, double bonusPortion)

update rank in Scheduler

diff --git a/osu.Game/Screens/Play/HUD/DefaultRankDisplay.cs b/osu.Game/Screens/Play/HUD/DefaultRankDisplay.cs
index 09ab7d156c..2c2b0fce82 100644
--- a/osu.Game/Screens/Play/HUD/DefaultRankDisplay.cs
+++ b/osu.Game/Screens/Play/HUD/DefaultRankDisplay.cs
@@ -4,6 +4,7 @@
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
+using osu.Framework.Threading;
 using osu.Game.Online.Leaderboards;
 using osu.Game.Rulesets.Scoring;
 using osu.Game.Skinning;
@@ -20,6 +21,8 @@ public partial class DefaultRankDisplay : Container, ISerialisableDrawable

         private readonly UpdateableRank rank;

+        private ScheduledDelegate? rankUpdateTask;
+
         public DefaultRankDisplay()
         {
             Size = new Vector2(70, 35);
@@ -39,7 +42,15 @@ protected override void LoadComplete()

             rank.Rank = scoreProcessor.Rank.Value;

-            scoreProcessor.Rank.BindValueChanged(v => rank.Rank = v.NewValue);
+            scoreProcessor.Rank.BindValueChanged(v =>
+            {
+                rankUpdateTask?.Cancel();
+
+                rankUpdateTask = Scheduler.Add(() =>
+                {
+                    rank.Rank = v.NewValue;
+                });
+            });
         }
     }
 }

I may prefer first one, because this solved the problem fundamentally.

bdach commented 4 months ago

@cdwcgt when having two solutions and one involves a schedule but the other doesn't, in 99% of cases choose the one without a schedule. I think the former is what we want (generally no subscriber to the bindable should be seeing the transient values). Should be fine to PR that.

That said I'm not sure why OP is saying "this only happens with SH". If the issue is where you're saying it is, it should also be happening with XH.

PercyDan54 commented 4 months ago

@cdwcgt when having two solutions and one involves a schedule but the other doesn't, in 99% of cases choose the one without a schedule. I think the former is what we want (generally no subscriber to the bindable should be seeing the transient values). Should be fine to PR that.

That said I'm not sure why OP is saying "this only happens with SH". If the issue is where you're saying it is, it should also be happening with XH.

updateRank() is called on Accuracy change which doesn't happen with XH (accuracy = 100%)

bdach commented 4 months ago

Sure, makes sense.

Feel free to PR that first patch.

ppy / osu