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.

Sign up for GitHub

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

Jump to bottom

Fix potential issues with medal display #30877

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Fix potential issues with medal display #30877

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8585327
Ensure `DrawableMedal` loading doesn't ever block on online resources
peppy Nov 26, 2024
d057dc9
Refactor `MedalOverlay` to be more readable
peppy Nov 26, 2024
672dbe6
Better control of show/hide of overlay
peppy Nov 26, 2024
e8fae85
Fix hidden dissmissing logic
peppy Nov 26, 2024
1e6c04e
Remove debug logging
peppy Nov 26, 2024
98044c1
Revert "Ensure `DrawableMedal` loading doesn't ever block on online r…
peppy Nov 26, 2024
71294c3
Change point of queueing to avoid loading-from-in-queue
peppy Nov 26, 2024
bf29e3a
Simplify hide code by moving to common method
peppy Nov 26, 2024
b0958c8
Attempt to fix test failures
bdach Nov 28, 2024
c14fe21
Fix LCA call crashing in actual usage
bdach Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions osu.Game/Overlays/MedalAnimation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,18 +245,19 @@ protected override void PopOut()
this.FadeOut(200);
}

public void Dismiss()
public bool Dismiss()
{
if (drawableMedal != null && drawableMedal.State != DisplayState.Full)
{
// if we haven't yet, play out the animation fully
drawableMedal.State = DisplayState.Full;
FinishTransforms(true);
return;
return false;
}

Hide();
Expire();
return true;
}

private partial class BackgroundStrip : Container
Expand Down
80 changes: 56 additions & 24 deletions osu.Game/Overlays/MedalOverlay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public partial class MedalOverlay : OsuFocusedOverlayContainer
private IAPIProvider api { get; set; } = null!;

private Container<Drawable> medalContainer = null!;
private MedalAnimation? lastAnimation;
private MedalAnimation? currentMedalDisplay;

[BackgroundDependencyLoader]
private void load()
Expand All @@ -54,11 +54,12 @@ protected override void LoadComplete()
{
base.LoadComplete();

OverlayActivationMode.BindValueChanged(val =>
{
if (val.NewValue == OverlayActivation.All && (queuedMedals.Any() || medalContainer.Any() || lastAnimation?.IsLoaded == false))
Show();
}, true);
OverlayActivationMode.BindValueChanged(_ => displayIfReady(), true);
}

public override void Hide()
{
// don't allow hiding the overlay via any method other than our own.
}

private void handleMedalMessages(SocketMessage obj)
Expand Down Expand Up @@ -86,43 +87,74 @@ private void handleMedalMessages(SocketMessage obj)
queuedMedals.Enqueue(medalAnimation);
Logger.Log($"Queueing medal unlock for \"{medal.Name}\" ({queuedMedals.Count} to display)");

if (OverlayActivationMode.Value == OverlayActivation.All)
Scheduler.AddOnce(Show);
Schedule(displayIfReady);
}

protected override void Update()
protected override bool OnClick(ClickEvent e)
{
base.Update();
progressDisplayByUser();
return true;
}

public override bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
{
if (e.Action == GlobalAction.Back)
{
progressDisplayByUser();
return true;
}

return base.OnPressed(e);
}

if (medalContainer.Any() || lastAnimation?.IsLoaded == false)
private void progressDisplayByUser()
{
// For now, we want to make sure that medals are definitely seen by the user.
// So we block exiting the overlay until the load of the active medal completes.
if (currentMedalDisplay?.IsLoaded == false)
return;

// Dismissing may sometimes play out the medal animation rather than immediately dismissing.
if (currentMedalDisplay?.Dismiss() == false)
return;

if (!queuedMedals.TryDequeue(out lastAnimation))
currentMedalDisplay = null;

if (!queuedMedals.Any())
{
Logger.Log("All queued medals have been displayed!");
Hide();
Logger.Log("All queued medals have been displayed, hiding overlay!");
base.Hide();
return;
}

Logger.Log($"Preparing to display \"{lastAnimation.Medal.Name}\"");
LoadComponentAsync(lastAnimation, medalContainer.Add);
showNextMedal();
}

protected override bool OnClick(ClickEvent e)
private void displayIfReady()
{
lastAnimation?.Dismiss();
return true;
if (OverlayActivationMode.Value != OverlayActivation.All)
return;

if (currentMedalDisplay != null || queuedMedals.Any())
showNextMedal();
}

public override bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
private void showNextMedal()
{
if (e.Action == GlobalAction.Back)
// A medal is already loading / loaded, so just ensure the overlay is visible.
if (currentMedalDisplay != null)
{
lastAnimation?.Dismiss();
return true;
Show();
return;
}

return base.OnPressed(e);
if (queuedMedals.TryDequeue(out currentMedalDisplay))
{
Logger.Log($"Preparing to display \"{currentMedalDisplay.Medal.Name}\"");

Show();
LoadComponentAsync(currentMedalDisplay, m => medalContainer.Add(m));
}
}

protected override void Dispose(bool isDisposing)
Expand Down
37 changes: 31 additions & 6 deletions osu.Game/Overlays/MedalSplash/DrawableMedal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using osu.Framework.Graphics.Containers;
using osu.Framework.Graphics.Sprites;
using osu.Framework.Graphics.Textures;
using osu.Framework.Logging;
using osu.Game.Graphics;
using osu.Game.Graphics.Containers;
using osu.Game.Graphics.Sprites;
Expand All @@ -28,16 +29,14 @@ public partial class DrawableMedal : Container, IStateful<DisplayState>
[CanBeNull]
public event Action<DisplayState> StateChanged;

private readonly Medal medal;
private readonly Container medalContainer;
private readonly Sprite medalSprite, medalGlow;
private readonly Sprite medalGlow;
private readonly OsuSpriteText unlocked, name;
private readonly TextFlowContainer description;
private DisplayState state;

public DrawableMedal(Medal medal)
{
this.medal = medal;
Position = new Vector2(0f, MedalAnimation.DISC_SIZE / 2);

FillFlowContainer infoFlow;
Expand All @@ -51,7 +50,7 @@ public DrawableMedal(Medal medal)
Alpha = 0f,
Children = new Drawable[]
{
medalSprite = new Sprite
new DelayedLoadWrapper(() => new MedalOnlineSprite(medal), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really what we want? Doesn't this mean the medal will display without a texture on slow internet? I would rather it were delayed (by LCA) as it was previously. The LCA and whole "preparing to display" stuff in MedalOverlay looks unnecessary now because it's not doing anything that would take a long time to load?

Copy link
Member Author

Choose a reason for hiding this comment

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

It displays with just the border, along with description etc.

I think this is better than having the whole overlay just sit there loading. You can test by adding a short delay, I don't think it feels too bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

The LCA and whole "preparing to display" stuff in MedalOverlay looks unnecessary now because it's not doing anything that would take a long time to load?

It's still loading a sample and texture. If a stutter from that is not of concern, then I guess it could be removed, but I'd prefer having it there?

Copy link
Contributor

@smoogipoo smoogipoo Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand why this is better than having the whole overlay sit there loading? I would rather have the overlay look good than to have it display immediately as soon as a medal is awarded, no?

Master:

2024-11-26.16-27-03.mp4

This PR:

2024-11-26.16-25-42.mp4

If a stutter from that is not of concern

Have you actually noticed a stutter there...? I'd be highly concerned if that caused a stutter because we do that pattern everywhere else in the game without LCA.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dunno. Perhaps I'm being dense with the above, so will ask for a second opinion from @bdach

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, potentially that works. Just have to make sure the load actually still works without the thing loading being present. It would be the refactor I'd look at doing.

I'll give that direction a try and see how it goes. It kind of freaks me out that right now, a loading texture can block medals from ever displaying again (I'm not sure if we have an established "load failed" path for drawables?).

Copy link
Contributor

@smoogipoo smoogipoo Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that will work because the scheduler (LCA callback) is only updated if the overlay is present. But there's like 1001 different ways to get around that, not the least being to use the scheduler callback on LCA, set AlwaysPresent, or override IsPresent like we do everywhere else.

Edit: In fact, IsPresent is already overridden in this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I'm aware (that it already does). But I have apprehension due to the latter thing I mentioned.

Copy link
Contributor

@smoogipoo smoogipoo Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest if that's so hard to handle and DLW is used as a shot-in-the-dark way to handle that, then I'm even less confident about this change. I'm not interested in quick merging this as it was reported once ever, so I would rather it be fixed properly as long as we agree that my dislike of DLW is not insane.

Also I'm not exactly sure what can "fail" here, OnlineStore try-catches everything and returns null, and so will TextureStore. I understand you haven't reproed it (and I haven't either - tested prior to this PR), but that feels like a weird thing to hinge on.

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've adjusted this in line with your proposal on discord, see what you think.

{
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Expand Down Expand Up @@ -122,11 +121,12 @@ public DrawableMedal(Medal medal)
}

[BackgroundDependencyLoader]
private void load(OsuColour colours, TextureStore textures, LargeTextureStore largeTextures)
private void load(OsuColour colours, TextureStore textures)
{
medalSprite.Texture = largeTextures.Get(medal.ImageUrl);
medalGlow.Texture = textures.Get(@"MedalSplash/medal-glow");
description.Colour = colours.BlueLight;

Logger.Log("loaded");
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty non-descript log message.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, was debug

}

protected override void LoadComplete()
Expand Down Expand Up @@ -191,6 +191,31 @@ private void updateState()
break;
}
}

private partial class MedalOnlineSprite : Sprite
{
private readonly Medal medal;

public MedalOnlineSprite(Medal medal)
{
this.medal = medal;
}

[BackgroundDependencyLoader]
private void load(OsuColour colours, TextureStore textures, LargeTextureStore largeTextures)
{
Texture = largeTextures.Get(medal.ImageUrl);

Anchor = Anchor.Centre;
Origin = Anchor.Centre;
}

protected override void LoadComplete()
{
base.LoadComplete();
this.FadeInFromZero(150, Easing.OutQuint);
}
}
}

public enum DisplayState
Expand Down
Loading