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

Fix score breakdown tooltips appearing in other feeds #29159

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

frenzibyte
Copy link
Member

All feeds are set to be always present so that they can remain updated while in the background.

@peppy
Copy link
Sponsor Member

peppy commented Jul 29, 2024

One thing I was wondering about is whether we should be doing the update while visible. For some of the displays, I think it would be cool if they played a catch-up animation after becoming visible again.

But that will likely require extra thought (per display) so this is probably a sane starting point.

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Fixes what it says it does.

Comment on lines +30 to +31
// we're always present so that we can update while hidden, but we don't want tooltips to be displayed, therefore directly use alpha comparison here.
public override bool PropagatePositionalInputSubTree => base.PropagatePositionalInputSubTree && Alpha > 0;
Copy link
Collaborator

@bdach bdach Jul 29, 2024

Choose a reason for hiding this comment

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

I think this solution is short-sighted and generally incorrect (even if it does what it says). Why? Because if some other ever-present component that handles input gets added to the carousel, or an existing one starts to handle it, you'll have to add this hack local to that other component too.

I'd say this should be fixed at the carousel level if at all possible and not here. Via overriding ShouldBeConsideredForInput(). Something like so (quick and dirty):

diff --git a/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs b/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs
index a9f9a5cd78..bf8e403913 100644
--- a/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs
+++ b/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs
@@ -18,19 +18,18 @@ public partial class DailyChallengeCarousel : Container
     {
         private const int switch_interval = 20_500;
 
-        private readonly Container content;
+        private readonly CarouselContainer content;
         private readonly FillFlowContainer<NavigationDot> navigationFlow;
 
         protected override Container<Drawable> Content => content;
 
         private double clockStartTime;
-        private int lastDisplayed = -1;
 
         public DailyChallengeCarousel()
         {
             InternalChildren = new Drawable[]
             {
-                content = new Container
+                content = new CarouselContainer
                 {
                     RelativeSizeAxes = Axes.Both,
                     Padding = new MarginPadding { Bottom = 40 },
@@ -81,7 +80,7 @@ protected override void Update()
 
             if (content.Count == 0)
             {
-                lastDisplayed = -1;
+                content.LastDisplayed = -1;
                 return;
             }
 
@@ -95,18 +94,18 @@ protected override void Update()
             if (content.Count > 1)
                 navigationFlow[currentDisplay].Progress = (float)displayProgress;
 
-            if (currentDisplay == lastDisplayed)
+            if (currentDisplay == content.LastDisplayed)
                 return;
 
-            if (lastDisplayed >= 0)
+            if (content.LastDisplayed >= 0)
             {
-                content[lastDisplayed].FadeOutFromOne(250, Easing.OutQuint);
-                navigationFlow[lastDisplayed].Active.Value = false;
+                content[content.LastDisplayed].FadeOutFromOne(250, Easing.OutQuint);
+                navigationFlow[content.LastDisplayed].Active.Value = false;
             }
 
             content[currentDisplay].Delay(250).Then().FadeInFromZero(250, Easing.OutQuint);
 
-            lastDisplayed = currentDisplay;
+            content.LastDisplayed = currentDisplay;
         }
 
         private void onManualNavigation(NavigationDot dot)
@@ -230,5 +229,15 @@ protected override bool OnClick(ClickEvent e)
                 return true;
             }
         }
+
+        private partial class CarouselContainer : Container
+        {
+            public int LastDisplayed { get; set; } = -1;
+
+            protected override bool ShouldBeConsideredForInput(Drawable child)
+            {
+                return base.ShouldBeConsideredForInput(child) && IndexOf(child) == LastDisplayed;
+            }
+        }
     }
 }

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

At the point of doing this I'd probably explore making the displays update nicely when coming into view as I proposed.

I think it's optimal if they should some animation when they come back on-screen (if there's something they can animate since last time).

Copy link
Collaborator

@bdach bdach Jul 29, 2024

Choose a reason for hiding this comment

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

At the point of doing this I'd probably explore making the displays update nicely when coming into view as I proposed.

Not sure I get where this is coming from? It's not like the above is some horrible eldritch hack no? Certainly no more than what this PR currently is.

it would be cool if they played a catch-up animation after becoming visible again

I dunno if I agree with that either. Not sure how you do it with something like the breakdown without it just spamming texts.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

As in, if we can avoid AlwaysPresent altogether none of this needs to exist.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Not sure how you do it with something like the breakdown without it just spamming texts.

Can show the last n scores with a delay between each.

Copy link
Member Author

@frenzibyte frenzibyte Jul 30, 2024

Choose a reason for hiding this comment

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

Your concern is valid. I tend to avoid overcomplicating things whenever possible, but I admit I was too lenient in this case. I would've agreed on going with the diff above as alternative to the solution in my PR, but I realise now that this whole situation has already been addressed in #29187.

If we want to bring back the concept of updating screen while hidden, we can also consider turning all feeds to VisibilityContainers as an alternative option to the diff above, as that disables input handling based on Visibility state rather than the direct drawable presence. Something for later.

@bdach
Copy link
Collaborator

bdach commented Jul 29, 2024

I still disagree with this but am not willing to go through the struggle session of convincing everyone so am just gonna merge and move on with my day. I'm not sure I'd be wanting / willing to do anything to existing components either to remove the requirement of AlwaysPresent either.

@bdach bdach merged commit efd1919 into ppy:master Jul 29, 2024
13 checks passed
@frenzibyte frenzibyte deleted the fix-daily-challenge-hover branch July 29, 2024 23:51
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.

Game shows amount of plays in daily challenges other menus
3 participants