ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
14.64k stars 2.16k forks source link

Move unnecessary properties from `BeatmapInfo` / realm to `IBeatmap` #28473

Open bdach opened 2 weeks ago

bdach commented 2 weeks ago

RFC. PRing for early feedback.

This is basically https://github.com/ppy/osu/pull/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 https://github.com/ppy/osu/issues/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:

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.

bdach commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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.