Add dynamic computation of beatmap difficulty

[smoogipoo]

This introduces the new BeatmapDifficultyManager class. I imagine this to take on the responsibility of databased background calculation in the future.

You can retrieve the beatmap difficulty in four ways via BeatmapDifficultyManager:

// Via an "untracked" bindable. Does not follow the user's current ruleset/mods.
IBindable<StarDifficulty> GetBindableDifficulty(BeatmapInfo, RulesetInfo, IEnumerable<Mod>);

// Via a "tracked" bindable. Follows the user's current ruleset/mods.
IBindable<StarDifficulty> GetBindableDifficulty(BeatmapInfo);

// Asynchronously.
async Task<StarDifficulty> GetDifficultyAsync(...);

// Synchronously. May be used for quick prototyping/testing.
StarDifficulty GetDifficulty(...);

Tracking of the bindable versions stops either when the given CancellationToken is cancelled, or when references are lost. I've chosen to explicitly cancel the token in SongSelect and AdvancedStats even though it's not required, just so that calculation doesn't continue after exiting to main menu with references still alive.

The async version can also be cancelled.

All of these will, in some way, give a StarDifficulty object. I imagine this object to contain more than just the star rating in the future - for example the current BeatmapInfo.DifficultyRating could maybe be moved in there, along with the attributes provided by the difficulty calculator (to display graphs in advanced stats, etc).

As for quirky implementation details, I've chosen to permanently keep the difficulties in the cache. I originally had it clear after 60s (although that didn't do much since it's a dictionary), however upon profiling I've found that 230 difficulty values take up ~13KB space anyway, which I'd say is insignificant for the time being.

[peppy]

move outside of the if/else

[peppy]

Just to confirm, this is only showing in the expanded panels' star displays? So the tooltips are using previous databased values for the time being intentionally, correct?

[smoogipoo]

Difficulty icons/tooltips are interesting since they:

1. Specify a beatmap + ruleset so it wouldn't make sense to follow the currently-selected ones - it probably needs a special ctor for that.
2. Appear for when the beatmap isn't local, but then should update when the beatmap does become locally-available.

I have a WIP branch for it, but I'd like to do that in a separate PR.

[peppy]

Rather than tracked/untracked, how about making it a bit more explicit?

CreateBindableForCurrent()
CreateBindableFor(BeatmapInfo...)

Get or Fetch can be used in place of Create if you prefer.

[smoogipoo]

Are you saying to use the current beatmap rather than to pass in a beatmap? That's kind-of what it implies to me...

Maybe if it were named:

CreateBindableWithCurrentRuleset(BeatmapInfo)
CreateBindable(BeatmapInfo, RulesetInfo, Mods)

Instead? Though I do like that "tracked" implies it changes along with something (the ruleset and mods), whereas even this still can imply it's a one-time thing.

[peppy]

by "current" i meant current ruleset/mods, so yeah as you said above.

or maybe even combine the methods, and mention that if ruleset / mods is null, the game-wide settings are used and tracked?

[peppy]

I haven't gone through this PR completely but "why would i want even want a bindable if it's untracked?" is a question I still have to figure out. Would be good if the naming can convey the meaning a bit better.

Also since there's GetDifficulty method, how about we call the bindable ones GetBindableDifficulty?

[smoogipoo]

or maybe even combine the methods

I think I'd be fine with having to specify the ruleset and mods every time.

why would i want even want a bindable if it's untracked

Three reasons off the top of my head:

1. You don't receive the difficulty immediately since it's asynchronous. Handling the async operation is quite difficult - start task, schedule continuation, check cancellation token inside scheduler callback (basically everything inside updateBindable right now).
2. Eventually the untracked bindable will respond to changes in the beatmap - e.g. if the beatmap was imported or updated. I believe this to be expected in 100% of cases. I wouldn't want to bind to beatmapManager.ItemUpdated everywhere in the game to handle this properly.
3. Maybe you have a component that can house both tracked and untracked bindables. I believe DifficultyIcon is going to do this, where in the carousel it'll use tracked bindables and in, say, multiplayer playlist panels it'll use untracked bindables.

I think a better question is when someone would use the async version. Perhaps that could be an internal usage - for example maybe the async version should be invoked on import.

And yeah, GetDifficulty is a thing, which as I said is mostly for prototyping, where you don't care about tracking/untracking, asynchronous callbacks, or cancellations, and you just want the difficulty. I don't expect that method to be used in production code.

how about we call the bindable ones GetBindableDifficulty

I like that.

[bdach]

Some notes from reading source. (Mostly nits.)

Not sure whether I have something to say right now when it comes to the discussion on whether tracked/untracked/async makes sense. I am of the opinion that both untracked and async have potential for being used but I'm bad at future-proofing.

[bdach]

I don't think I can fabricate a scenario to confirm my suspicions for sure right now, but this seems unsafe. If I'm compiling in my head correctly, default is {BeatmapId = 0, RulesetId = 0, Mods = null}; callers of this method use the value of key without checks for default, so there's a potential for garbage stores to the cache - which in itself wouldn't be a problem, but the garbage reads that would be possible later on (if I'm not misreading) due to reusing that same default key is.

The returned key should be probably nullable or something.

[smoogipoo]

This method is private, so I'm not concerned about callers potentially doing something like:

tryGetExisting(... out key)

// Assume the key hasn't existed (ignore return value)
cache[key] = ...

[bdach]

Hmm, I suppose. I still would have probably restructured this so that this mistake is impossible to make type-wise but I guess as long is it private...

[smoogipoo]

@peppy I've changed to GetBindableDifficulty, see if it fits nicer for you.

[peppy]

Reason for ourSource local? Doesn't seem like it could be thread-unsafe to just use the field.

[smoogipoo]

Old code, whoops.