Move unnecessary properties from BeatmapInfo / realm to IBeatmap

[bdach]

RFC. PRing for early feedback.

• Closes Editor does not read properties ignored on BeatmapInfo correctly #20883
• Supersedes / closes Move Ignored properties into IBeatmap #23418
• Needs more testing probably, I've only checked that tests pass

This is basically #23418 but hopefully better (because it moves out more stuff, and hopefully contains less naming discussions). This removes everything that was feasible to remove from BeatmapInfo, therefore bypassing the reference handling nightmare that causes #20883.

I'm necroing that because it came up in recent discussions about persisting the new grid settings somewhere (see discord, cc @OliBomby).

Some things still remain:

• BeatmapInfo.BeatDivisor - can't really be moved out of realm because it's used for filtering in song select
• BeatmapInfo.EditorTimestamp could go live in the .osu file but I didn't want to make that change here as I would rather keep this diff strictly menial.

I'm not very happy about how much this makes IBeatmap grow but I don't see much wiggle room. Any effort to constrain interfaces any more to specific usages (diffcalc, editor) is basically killed by WorkingBeatmap / conversion and requires at least introduction of generic messes and multiple scoped overloads so I decided I'd rather not.

[smoogipoo]

I suppose that in principle I'm not super against this, but it does blurry the definition for what should and should not be put in BeatmapInfo a bit.

You could say that BeatmapInfo is everything to be databased, in which case I'd probably expect the epilepsy warning to be put there, since it's being used in a context where it's expected to be available relatively "free-of-cost"?

[smoogipoo]

This is kinda weird...

[bdach]

So is everything else being populated with empty objects in this class. I was just following precedent, if you will.

[smoogipoo]

My problem here is two-fold. The first is that I'm struggling to follow the code to even ensure this is being written to right now, because it doesn't look like it is? The second is I think this goes above the "everything else" for me.

One thing I thought about but hesitated from posting is to split the properties out into a BeatmapProperties or maybe even into BeatmapMetadata. It feels wrong to be passing Beatmap around which historically has been used very close to gameplay...

To go further, both BeatmapInfo and Beatmap look wrong to me.

• If we're not going to have a general "class of info" that we're passing around, then I think EpilepsyWarning/WidescreenStoryboard should be moved into Storyboard.
• The remaining references to BeatmapInfo need to be removed - it looks like there's two:
  • ReplacesBackground (BeatmapInfo.Metadata.BackgroundFile) - probably BackgroundFile should be duplicated as a property here.
  • GetStoragePathFromStoryboardPath the directionality looks inverted. It should be DrawableStoryboard doing the lookups on resource stores, not what is effectively a model class otherwise.

Perhaps this would be best as a pre-pass.

[bdach]

One thing I thought about but hesitated from posting is to split the properties out into a BeatmapProperties or maybe even into BeatmapMetadata

That's what #23418 tried to do but got stunlocked on naming. If we can agree on a name or a place I don't see why not.

[peppy]

One thing I thought about but hesitated from posting is to split the properties out into a BeatmapProperties or maybe even into BeatmapMetadata. It feels wrong to be passing Beatmap around which historically has been used very close to gameplay...

I get this, but is there an actually requirement for the split? In stable, for instance, we have an .osu file. And we have a Beatmap. And they are 1:1. Should we not just stick with that for simplicity? Am I missing a case where passing "too much" in one class is an issue?

Also I'm still confused. I can't find any write usages of Storyboard.Beatmap or its properties (except one test), even though it's read from. @bdach can you clarify here?

[peppy]

Judging from master code, it looks like the Beatmap would need to be set around here to work correctly:

osu/osu.Game/Beatmaps/WorkingBeatmap.cs

Line 65 in 3978d4b

protected virtual Storyboard GetStoryboard() => new Storyboard { BeatmapInfo = BeatmapInfo };

[bdach]

Also I'm still confused. I can't find any write usages of Storyboard.Beatmap or its properties (except one test), even though it's read from.

Yeah that's just a bug. Good spot, although I did try to preempt this PR by saying it's basically a draft of the general shape with no testing other than checking that unit tests pass.

Judging from master code, it looks like the Beatmap would need to be set around here to work correctly:

Looks close, but the reality is uglier than that; see 1d4d806. tl;dr is that storyboards go through a complete separate decoding path:

osu/osu.Game/Beatmaps/WorkingBeatmapCache.cs

Lines 257 to 312 in 40b171f

	protected override Storyboard GetStoryboard()
	{
	Storyboard storyboard;
	if (BeatmapInfo.Path == null)
	return new Storyboard();
	try
	{
	string fileStorePath = BeatmapSetInfo.GetPathForFile(BeatmapInfo.Path);
	var beatmapFileStream = GetStream(fileStorePath);
	if (beatmapFileStream == null)
	{
	Logger.Log($"Beatmap failed to load (file {BeatmapInfo.Path} not found on disk at expected location {fileStorePath})", level: LogLevel.Error);
	return new Storyboard();
	}
	using (var reader = new LineBufferedReader(beatmapFileStream))
	{
	var decoder = Decoder.GetDecoder<Storyboard>(reader);
	Stream storyboardFileStream = null;
	string mainStoryboardFilename = getMainStoryboardFilename(BeatmapSetInfo.Metadata);
	if (BeatmapSetInfo?.Files.FirstOrDefault(f => f.Filename.Equals(mainStoryboardFilename, StringComparison.OrdinalIgnoreCase))?.Filename is string
	storyboardFilename)
	{
	string storyboardFileStorePath = BeatmapSetInfo?.GetPathForFile(storyboardFilename);
	storyboardFileStream = GetStream(storyboardFileStorePath);
	if (storyboardFileStream == null)
	Logger.Log($"Storyboard failed to load (file {storyboardFilename} not found on disk at expected location {storyboardFileStorePath})", level: LogLevel.Error);
	}
	if (storyboardFileStream != null)
	{
	// Stand-alone storyboard was found, so parse in addition to the beatmap's local storyboard.
	using (var secondaryReader = new LineBufferedReader(storyboardFileStream))
	storyboard = decoder.Decode(reader, secondaryReader);
	}
	else
	storyboard = decoder.Decode(reader);
	}
	}
	catch (Exception e)
	{
	Logger.Error(e, "Storyboard failed to load");
	storyboard = new Storyboard();
	}
	storyboard.BeatmapInfo = BeatmapInfo;
	return storyboard;
	}

and thus all IBeatmap properties that Storyboard needs to read correctly must be populated by LegacyStoryboardDecoder.

I guess this is a relevant point of discussion to the general direction here, and I'm not sure what to make of that revelation. In a way this may be more correct because this separate decoding path reads both the .osb (if present) and the .osu as a fallback, so the resultant behaviour may be more correct here (needs stable cross-check) - but that's also a weak edge case that probably has never been an issue until now

The fact that decoding would need to be split and/or duplicated based on what beatmaps care about and storyboards care about could be seen as bad. Although that is arguably already a problem because that exists:

osu/osu.Game/Beatmaps/Formats/LegacyStoryboardDecoder.cs

Lines 72 to 74 in 40b171f

	case "UseSkinSprites":
	storyboard.UseSkinSprites = pair.Value == "1";
	break;

[smoogipoo]

Have you tested whether this is blocking on the beatmap load? This looks kinda scary to me.

[bdach]

I haven't tested this until now (see mention in OP that this probably needs more testing). I was interested in getting the shape of this right first and foremost, and was only going to go over specifics later. The extent of what is to be moved out is negotiable.

As far as the answer goes, I believe it to be "generally it will not be", because the game-wide async load process will have already completed by now:

osu/osu.Game/OsuGame.cs

Lines 799 to 803 in 57688c2

	private void beatmapChanged(ValueChangedEvent<WorkingBeatmap> beatmap)
	{
	beatmap.OldValue?.CancelAsyncLoad();
	beatmap.NewValue?.BeginAsyncLoad();
	}

But sure, I can agree with not moving this out. I was in two minds about doing this myself, but am prepared to revert this part. It just seemed to me like it wasn't impossible to move it so I did it, to cover absolutely everything that can be moved without significant changes.

[bdach]

Generally I'll just bring up that while this is a draft I'd like to see movement on this or an alternative solution since the underlying issue is currently blocking editor improvements that users want to see too (like bookmarks which don't work because they get dropped by the dumb BeatmapInfo reference juggling logic).

[OliBomby]

I'll try to put in my 2 cents.

If the BeatmapInfo is for realm databased properties, I suggest renaming it to BeatmapDatabasedInfo or BeatmapRealmInfo. That will help convey its purpose and explain which properties you want in that class.

For the properties which should not be databased, I suggest putting them in classes like BeatmapGeneral for general properties and BeatmapEditor for editor properties. This mirrors the sections that exist in the .osu format and follows the precedent set by the BeatmapMetadata and BeatmapDifficulty classes.

[peppy]

Generally I'll just bring up that while this is a draft I'd like to see movement on this or an alternative solution since the underlying issue is currently blocking editor improvements that users want to see too (like bookmarks which don't work because they get dropped by the dumb BeatmapInfo reference juggling logic).

I have this on my review queue, if you need some assurance that it hasn't been forgotten.

[peppy]

I guess my biggest concern is requiring these in the IBeatmap interface (and the delegation requirement of implementations). Which is probably not a huge concern at all.

I'm neutral on moving stuff into a separate class, will require further discussion to ascertain what we're looking for here, as touched on in other comment thread.