Fix potential issues with medal display #30877
Conversation
Shouldn't really have any functionality changes, just fixing some old code that I can't easily parse these days.
| medalGlow.Texture = textures.Get(@"MedalSplash/medal-glow"); | ||
| description.Colour = colours.BlueLight; | ||
|
|
||
| Logger.Log("loaded"); |
There was a problem hiding this comment.
Pretty non-descript log message.
| @@ -51,7 +50,7 @@ public DrawableMedal(Medal medal) | |||
| Alpha = 0f, | |||
| Children = new Drawable[] | |||
| { | |||
| medalSprite = new Sprite | |||
| new DelayedLoadWrapper(() => new MedalOnlineSprite(medal), 0) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Dunno. Perhaps I'm being dense with the above, so will ask for a second opinion from @bdach
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes I'm aware (that it already does). But I have apprehension due to the latter thing I mentioned.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've adjusted this in line with your proposal on discord, see what you think.
peppy
force-pushed
the
medal-display-fix
branch
from
05cc2e9
to
1e6c04e
Compare
…esources" This reverts commit 8585327.
It's not allowed to call `LoadComponentsAsync()` on a background thread: https://github.com/ppy/osu-framework/blob/fd64f2f0d47f0ee1aaa596bde1e83e527d610340/osu.Framework/Graphics/Containers/CompositeDrawable.cs#L147 and in this case the event that causes the LCA call is dispatched from a websocket client, which is not on the update thread, so scheduling is required.
|
@peppy probably go over my changes as well whenever able. |
|
Hmm, I considered thread safety of LCA but was 100% convinced it's fine from a background thread. TIL. Changes look good, thanks. |
This is mostly a refactor of the component to bring code quality up to 2024 standards. It's still a tad finicky.
Checking the logs, the online texture load never occurred after the preparation call (there's no log of the web request even starting). This suggests the overlay was somehow hidden before the
LoadComponentAsynccould be acted upon. Maybe the overlay was hidden by external forces?Fixes probably #30626 but I haven't been able to repro the underlying failure.
I also took the opportunity to decouple the loading of the medal sprite from the animation. While this was likely not the underlying failure, I'd rather a resource stuck in a loading state didn't leave the game in a weird state. Previously, it could have caused the precise issue in the linked issue (can be tested by adding a sleep in the
load()method).