Fetch missing beatmap when importing replay

[cdwcgt]

• Depends on Fix MemoryStreamArchiveReader.GetStream() failing in some cases #24846

Reimplementation for

• Add screen for downloading missing beatmap when importing score #14044
• Don't fetch beatmapset that already PendingDelete when import score #22741

Discussed in

• Download missing beatmap when importing scores #14029

Redesigned based on peppy's comment

I'm a little confused about the adjustment of anchor and origin, so the design may not look good, so please let me know if you are not satisfied

which this pr do:

1. Don't fetch delete pending beatmapset which use by realm filter
2. Post a progress notification that will show beatmap card image and some metadata with a download button.
3. detected if there are already a beatmapset match online id but no beatmap match the score hash, which will happen with outdated beatmapset.
4. re-import score if beatmap which match the score imported.

If you think the design is okay, just add localization then done.

osu.development.2023-09-04.16-06-26_output_x264.mp4

outdated

Tested importing from Stable, but it's annoying when missing beatmap keep appearing.

I cannot use DI to get IPerformFromScreenRunner in ScoreImporter, so I pass it by ScoreManager, I don't know if there is a better solution.

Some rulesets may need beatmap information when parsing the score, like mania need key count, so I choose don't to parse the score in advance

Current behavior:

1. Import failed due to missing beatmap.
2. Save score stream into memory for reimport.
3. Fetch from online by md5 in score, if no beatmap provided, stream will dispose and no difference from the original.
4. Push ReplayMissingBeatmapScreen.
5. If AutomaticallyDownloadWhenSpectating is true, then auto download.
6. When beatmap downloaded complete, import the score again.

after score imported or ReplayMissingBeatmapScreen dispose will dispose the stream.

osu.2023-08-02.17-41-46_x264.mp4

[bdach]

I don't know how to feel about this sort of thing. First of all, even if we're to do this it should be next to the other ScoreManager attachment thingies (so after line 853). Secondly, that's three things being jammed from OsuGame into ScoreManager in this janky way. I'm almost at the point where I think it'd be worth making all of these managers Components so that they can leverage DI properly or something. Not sure.

[peppy]

Looking at the video alone, the UX of this is still pretty shocking. Notifications popping up all over the place, a fullscreen view which doesn't let the user know what's going on and hijacks the full game. I feel like the only way this is going to work well is using a custom notification which allows downloading the beatmap inline.

You do not have the required beatmap for this replay

[ some kind of beatmap panel / donwnload interface ]

The notification would not dismiss while the download is happening, and after the local availability changes, it would retry the import in the same notification.

I'm not even sure it's worth reviewing this code without getting the UX correct first.

[cdwcgt]

I'm not even sure it's worth reviewing this code without getting the UX correct first.

Then I think we can start with custom notifications, but it may lack design.
Especially it seems that the notification may not fit that much content
If so, I will close this pr, or just simply show the corresponding beatmap for the score. I do my best
decision is yours

[peppy]

Just based on the videos, this is looking much much better than what you had.

I'd hope we can make the inline beatmap card display a bit nicer, but the flow looks decent.

[bdach]

Ok, well, I tried to do something with this branch...

Of note:

• Visual appearance redesigned to be somewhat palatable
• UX redesigned to be somewhat tolerable
• Fixed autodownload setting parasiting off spectator autodownload and instead made one common autodownload setting, with appropriate copy and automatic migration

Note that this is still like 50% jank because of the amount of notifications fired. In an ideal world you'd probably want to eliminate both the "score failed" notification and the "importing" notification. But neither is trivial as those notifications are nestled deep into RealmArchiveModelImporter and attempting to yank that out would probably either take an eldritch abomination the likes of which shall never be witnessed, or a 1k line rewrite of central classes. So I'll just leave it halfway jank for now, I've already spent too much time on this in comparison to how much time I actually wanted to put in.

@peppy Leaving this for you to check UI/UX. Not sure you'll like my decisions, so go ham.

self-resolved i guess

[peppy]

@bdach on an initial pass my main question would be why you can't click the card to bring up the overlay. Is it a mobile concern?

[bdach]

Which overlay are you referring to specifically? It's a bit unclear, sorry.

[peppy]

When clicking the new nano beatmap card inside the notification, it always triggers download (via this override) rather than open the beatmap overlay. Feels a bit odd to me.

[bdach]

Well the general thinking was that the card is too small for multiple actions and tiny buttons that master has there, so it should just have one action always. And if it's gonna have one always, it should be the most convenient one, i.e. "download the map now", rather than "show the beatmap set overlay, then download the map".

Dunno if valid, I guess there's no real reason not to have it open the overlay first. But that does beg the question where would download go on the card (if anywhere at all).

[peppy]

But that does beg the question where would download go on the card (if anywhere at all).

Hmm, but the weird thing is the download button is still there (and even more weirdly, it's the only part of the card that doesn't have a "Download" tooltip):

Judging from your last reply, was the button still being there unintentional?

I think it's likely fine to have users click the download button specifically to download the beatmap, as it is an established UX from cards everywhere else.

[bdach]

I'll take another look tomorrow. The tooltip thing is not intentional but the icon generally was supposed to be where it is (it was no longer meant to really be a button itself, more like an indication what is gonna happen when the card is clicked anywhere).

I'll see if I can instead have just the expanded right part be the download button, and have the left part open the beatmap overlay, since that seems to be the UX you're angling for.

[bdach]

@peppy I think things should be closer to expectations with the latest commit. If they aren't let me know, or feel free to take over, I guess. Whichever's easier.

[peppy]

Isn't this going to get overwritten by the Text= in LoadComplete?

[bdach]

Yeah good spot. Should be addressed by ed9039f.

[peppy]

In addition to the one issue I haven't fixed, please check my new commits to make sure you agree with them.

[bdach]

Other changes look good, thanks 👍

[cdwcgt]

maybe we need to not auto download if there are a beatmapset match the online id?
It may a local change or something player need to keep, auto download will override them.

[bdach]

@cdwcgt I'm pretty sure your original version had that too so ¯\_(ツ)_/¯

I think it's fine as is.