Replacing `BeatmapDifficultyCache` with realm stored values

[peppy]

In trying to update star ratings after an editor save (as part of https://github.com/ppy/osu/issues/18665), I ran into some issues which need further discussion before moving forward, to make sure everyone is on the same page.

The primary concern is that BeatmapDifficultyCache has no method of invalidation. Adding one is non-trivial as the cache is done on a struct with Ruleset:Mod:Beatmap combinations. When saving a beatmap we would want to invalidate all entries on a beatmap level.

Looking for a solution to this, I was looking to move this cache to be at a realm BeatmapInfo level. I think this is something we all generally can agree on, but let me know if that's not the case. It would look something like this:

diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
index 69488277f1..657db1fc74 100644
--- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
+++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
@@ -347,6 +347,8 @@ public DifficultyCacheLookup([NotNull] BeatmapInfo beatmapInfo, [CanBeNull] Rule
                 OrderedMods = mods?.OrderBy(m => m.Acronym).Select(mod => mod.DeepClone()).ToArray() ?? Array.Empty<Mod>();
             }

+            public string LookupString => $"{Ruleset.ShortName}:{string.Join(',', OrderedMods.Select(m => m.Acronym))}";
+
             public bool Equals(DifficultyCacheLookup other)
                 => BeatmapInfo.Equals(other.BeatmapInfo)
                    && Ruleset.Equals(other.Ruleset)
diff --git a/osu.Game/Beatmaps/BeatmapInfo.cs b/osu.Game/Beatmaps/BeatmapInfo.cs
index 346bf86818..ef494128b6 100644
--- a/osu.Game/Beatmaps/BeatmapInfo.cs
+++ b/osu.Game/Beatmaps/BeatmapInfo.cs
@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.

 using System;
+using System.Collections.Generic;
 using System.Linq;
 using JetBrains.Annotations;
 using Newtonsoft.Json;
@@ -55,6 +56,7 @@ public BeatmapInfo(RulesetInfo? ruleset = null, BeatmapDifficulty? difficulty =
             Difficulty = difficulty ?? new BeatmapDifficulty();
             Metadata = metadata ?? new BeatmapMetadata();
             UserSettings = new BeatmapUserSettings();
+            StarRatings = new Dictionary<string, double>();
         }

         [UsedImplicitly]
@@ -86,7 +88,10 @@ public BeatmapOnlineStatus Status

         public string Hash { get; set; } = string.Empty;

-        public double StarRating { get; set; }
+        /// <summary>
+        /// Mapping of ruleset/mod combinations to star ratings.
+        /// </summary>
+        public IDictionary<string, double> StarRatings { get; set; } = null!;

         [Indexed]
         public string MD5Hash { get; set; } = string.Empty;

We would not be able to cache every settings combination, so I'd propose that caching for default settings is a good place to be.

But this may still need to be accompanied by a stored value for the current mod/setting/ruleset combination to handle multiple components potentially requesting the difficulty at song select etc.

Potentially the current star rating could be cached in WorkingBeatmap to handle this case (as a bindable, replacing the GetBindableDifficulty flow in BeatmapDifficultyCache.

---

If more context is required for the above, please have a read through my WIP branch for updating beatmaps after save.

[smoogipoo]

I'm not entirely sure what sort of feedback you want on this, if any, but storing default mod settings is fine.

I don't think we'll ever be able to store every mod combination, so we'll need some sort of process that enables both use of/caching to the database (for default settings) and inline/in-memory computation (for song select filtering on SR with non-default settings).

[peppy]

I think that filtering / sorting should use default settings as best effort, at least for now. For display purposes, I propose the WorkingBeatmap level single-value cache for current settings.

Whether we will ever be able to sort or filter on settings adjusted values is something for a far future discussion I think.

Does that sounds okay?

[smoogipoo]

Song select will need an async process as I mentioned in the past, but the idea is more that you should build in that direction/keep that in mind.

Not sure on the WorkingBeatmap thing. WorkingBeatmaps aren't tied to mods right now, and some components like the details graph in song select need two values (the default and the mod-applied one).

[peppy]

Put another way: I think we should only be caching values for the current settings/mod (for non-defaults), rather than persisting each settings changed value to memory cache.

Whether it’s stored in WorkingBeatmap or somewhere else is something I’ll figure out, that just fits best with the invalidation flow I have at the moment.

[smoogipoo]

I think we should only be caching values for the current settings/mod (for non-defaults), rather than persisting each settings changed value to memory cache.

I agree, only the defaults should be persisted anywhere (memory or otherwise).

[peppy]

I managed to add a method of invalidation via https://github.com/ppy/osu/pull/18835. I still believe we should be caching less to memory (non-default combinations, as agreed upon above) and more to realm, but this has become a lower priority issue so I'm removing from the roadmap.