Add ability to create new difficulties from within the editor

[bdach]

Attempt no. 2 at #12035. Now with infinity% more realm. The general UI/UX hasn't changed since #14799 so I'm not going to bother including a video again.

I'll preface this by saying that while this works - as far as I've tested, at least - I'm not tremendously sure that this diff will hold up to scrutiny. I've had a weirdly difficult time arriving at this version and it was only after some substantial nudging in the current direction via discord. I'm mostly getting this out today for the sake of my sanity, because I've been tweaking with this for days and still feel no more confident with the state of these changes any way I try.

The main issue is that the editor flow is a little particular about what should be detached and what shouldn't. In this diff things appear to just-work, but that's guaranteed by one silent assumption - namely, that the objects incoming to BeatmapModelManager.AddDifficultyToBeatmapSet() are detached (as writes to any properties of either model instance would fail if they were managed, not to mention potential attempts to take out nested write usages which will not succeed if there is an outer transaction open). This may be considered dodgy but I'll note that as things stand, the existing Save() method also silently assumes its arguments are detached.

Due to the above I've considered delaying this and trying to figure out how editor could work in fully-managed mode, but there are a lot of unknowns there; I can't just hold a transaction open for the entire duration of the editing session, so where should changes go? how to do reverts? etc. etc. - so even if I were working on that day-to-day (which I 99% won't be), the timescale of that would likely be weeks-to-months. And as this is a pretty large missing piece for the editor, I figure I may as well PR what I have and see if it's passable enough. If not then I'll try the long way around I guess.

Note that you can't create a copy of a difficulty yet. My plan was to address that separately as a follow-up.

[peppy]

I've been doing some further thinking on how we are going to deal with this in a better way going forward, in the scenario that we do want to keep it in a managed state. My current best solution is to first create a deep clone of the beatmap that is being edited and add it to realm (with a flag that implies it is in a "dirty" state), then decide whether to delete or replace at the end of the edit session.

Which is to say, as we are already detaching in all scenarios, it would only require a method of updating GUIDs to ensure we can create a distinct clone. Maybe a similar extension method to Detach which handles this specifically.

Adding it to realm means that if there is a crash, it will still remain and can potentially be recovered by a user, so I think this makes the most sense. Care will still need to be made as the .osu file is not in realm, though.

---

That said, as I may have mentioned on discord, and touching on what you say above, this is likely going to take quite a bit of consideration and further refactoring. I think we should hold off on this until we run into issues that call for it. We have plenty of other work to be done on the editor and if the solution in this PR is enough to keep things working into the future we should stick with it.

[peppy]

I'm not confident on us implementing these manually all over the place. One of the things I wanted to look at is replacing IDeepCloneable with automapper. Is that something you could get behind?

That said, I think this is fine for the time being to get things working.

[bdach]

Given the fact that I've messed up writing these clones no less than twice in this pull alone, you won't hear me protest against using automapper for these for sure.

[peppy]

Just to confirm, the clones are to avoid reference issues at the detached side correct? And have nothing to do with realm.

[bdach]

The metadata clone was originally to ensure 1:1 BeatmapInfo to BeatmapMetadata correspondence as you requested in an earlier version of this. It will not be actually necessary anymore for that original purpose, but probably safest to have for sanity to avoid reference issues as you say.

[peppy]

If it's not required I'd argue we omit it to keep things as simple as possible... not sure.

[bdach]

I'll also note that the metadata directly associated with the BeatmapInfo here will become managed upon the beatmap being added, which is another reason to use a temporary clone for that.

I'll try removing but I feel somewhat safer with it.

[peppy]

This Realm.Run usage doesn't seem to be required, nor is it adding any kind of transactional safety. Oversight maybe?

[bdach]

Looks like a vestige that I forgot to rebase out. I'll double check for sure though.

[peppy]

I've made some changes, feel free to cherry pick as you wish (https://github.com/ppy/osu/compare/master...peppy:ndc-amends?expand=1)

67d306c Remove unnecessary clones

Not really intended to be cherry-picked, but all tests still pass without these. I'd probably add test coverage at the very least. The potential fail case is the returned WorkingBeatmap being immediately used while the original is still in use and not invalidated, right?

bef0a2d Remove return type from AddDifficultyToBeatmapSet

Also removes a pointless realm encapsulation. Neither look required.

ad47649 Make BeatmapModelManager.Save non-virtual

No brainer I think.

[bdach]

Thanks, I've pulled the last two commits. Not sure about the first one still:

The potential fail case is the returned WorkingBeatmap being immediately used while the original is still in use and not invalidated, right?

Correct. The worry is that without the clones, the "reference beatmap" fetched in BeatmapManager.CreateNewBlankDifficulty() would share a metadata reference with the new beatmap being added to realm. At the point of realm.Add(), the metadata reference would become managed and therefore unsafe for writes (or even reads, if something recycles contexts).

Now sure, BeatmapManager.CreateNewBlankDifficulty() purges the cache for the entire set, but if some component happens to access the working beatmap within that timeframe (between the realm add and the working beatmap cache purge), it will henceforth hold a reference to a WorkingBeatmap with a BeatmapInfo inside which would have that same unsafe metadata reference. I think that's slightly too dangerous to let live myself and feel better with the clone there all things considered.

[peppy]

Yeah, agree for now.

One other thing I noticed is that you are invalidating the WorkingBeatmapCache twice (it's already done in Save but you're also doing so in the AddDifficulty method) - is that just for safety?

[bdach]

Save() invalidates the particular BeatmapInfo, CreateNewBlankDifficulty() invalidates all beatmaps from the entire set. Which matters due to the circular references between them (the beatmaps contain backreferences to the set, which may not be updated with the newly created difficulty without that call).

[peppy]

One other thing I'll mention is that I really dislike this line but am okay with it for now. I thought it was here for some realm issue, but no, it's just because of the convoluted path the Save method takes. I propose we change Save to take in the BeatmapSetInfo as the first parameter in this PR or a future code quality pass, because it's weird at the moment.

[bdach]

I agree this is not great. The only counterargument I could immediately come up with is potential misuse of Save() by accident by passing in a BeatmapInfo and a BeatmapSetInfo that aren't supposed to be associated. But you could do that anyway by writing into the BeatmapSet property so that's not really an argument.

I'm OK with applying that change here as it's just two immediate usages to update.

[peppy]

See how you go, if it's a simple one (should be?) then this PR is fine I think.

[bdach]

Unfortunately there is a second place that was relying on the BeatmapInfo -> BeatmapSetInfo link, and that is GetWorkingBeatmap() itself, which will return a dummy beatmap instead if the link isn't populated:

osu/osu.Game/Beatmaps/WorkingBeatmapCache.cs

Lines 77 to 78 in dbf2a11

	if (beatmapInfo?.BeatmapSet == null)
	return DefaultBeatmap;

Touching GetWorkingBeatmap() in this PR seems... unwise, so I'm keeping my hands off for now.

However in the process of typing up the suggested change I did notice that the BeatmapModelManager method wasn't really doing much anymore and so I merged it back into the BeatmapManager method in 6dc0f3f. Seems less convoluted maybe? Dunno.

[frenzibyte]

The test case states that creating a new beatmap fails with blank difficulty name, but I think this is missing an Editor.Save() step here to confirm that it would fail? (or was the test case intended to ensure CreateNewDifficulty doesn't affect the databased beatmap set when Save is not performed?)