60 Commits

• Fix race condition in beatmap difficulty cache invalidation flow (#37178)

  Exposed by CI failures
  ([example](https://github.com/ppy/osu/actions/runs/23888446400#user-content-r0s0)).
  
  The race occurs when a consumer calls `GetBindableDifficulty()` for the
  first time and then a cache invalidation is triggered. The sequence of
  events triggering the failure is as follows:
  
  1. Consumer calls `GetBindableDifficulty()` to get a difficulty bindable
  for a given beatmap tracking the game-global ruleset / mods. This
  triggers difficulty calculation A.
  2. In the meantime, another process requests a cache invalidation for
  the same beatmap as the one supplied by the consumer in step (1). This
  incurs a cache purge and triggers difficulty calculation B, but never
  cancels difficulty calculation A.
  3. Difficulty calculation B concludes and writes the correct, latest
  difficulty value to the bindable.
  4. Difficulty calculation A concludes and writes an incorrect, stale
  difficulty value to the bindable.
  
  See below for patch that reproduces this behaviour on my machine 100%
  reliably:
  
  ```diff
  diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
  index https://github.com/ppy/osu/commit/d6b40639161e26af223f03761b3826b0cd7f4a87..c9604e0e58 100644
  --- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
  +++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
  @@ -252,17 +252,17 @@ private void updateBindable(BindableStarDifficulty bindable, IRulesetInfo? rules
               GetDifficultyAsync(bindable.BeatmapInfo, rulesetInfo, mods, cancellationToken, computationDelay)
                   .ContinueWith(task =>
                   {
  +                    StarDifficulty? starDifficulty = task.GetResultSafely();
  +
                       // We're on a threadpool thread, but we should exit back to the update thread so consumers can safely handle value-changed events.
  -                    Schedule(() =>
  +                    Scheduler.AddDelayed(() =>
                       {
                           if (cancellationToken.IsCancellationRequested)
                               return;
  
  -                        StarDifficulty? starDifficulty = task.GetResultSafely();
  -
                           if (starDifficulty != null)
                               bindable.Value = starDifficulty.Value;
  -                    });
  +                    }, starDifficulty?.Stars > 0 ? 400 : 0);
                   }, cancellationToken);
           }
  
  ```
  
  The goal of the patch is to reorder the write to the bindable in order
  to trigger the scenario described above.
  
  Due to the invasiveness of the patch it is not suitable to add as a
  test, and chances are that the schedule delay may need to be tweaked if
  anyone else wants to check that patch.
  
  Anyway, the solution here is to use the same pattern of creating a
  linked cancellation token even on the first retrieval of a bindable
  difficulty, and registering it in the list of cancellation tokens that
  already existed to service the ruleset- / mod-tracking flow.
  
  Some extra rearranging in
  https://github.com/ppy/osu/commit/9184299239a8b5e82957d46db33f1d26bab238fd
  is needed to ensure the linked tokens created to do this don't stay
  behind after they are no longer needed for anything.

  Bartłomiej Dach · 2026-04-03 21:57:07 +09:00
  ce8406613a
• Improve song select audio transitions (#37022)

  Based on feedback I've heard more than once ([most
  recently](https://github.com/ppy/osu/discussions/36933)), there's too
  much overlap compared to stable. And it can sound a bit "jumbled" as a
  result, especially when transitioning between two songs with loud and
  "complex" audio.
  
  This also fixes noticeable audio glitching that some users may have been
  experiencing when switching tracks.
  
  ---
  
  Unless there's any very loud objections, I'd ask that review is on the
  code not the audible change. This is the kind of change that I tweak to
  my personal spec and then push out to hear the general reaction.
  
  ### b8d0dfe2ed Better default preview time
  based on global fade
  
  This is an oversight. The preview point wasn't being offset to account
  for the fade in. This is something we do when generating web-side
  previews, which allows for the actual preview point to be audible,
  rather than mid-fade.
  
  ### 8de323b68c Adjust fade in and fade out
  during track transitions to reduce overlap
  
  Yes, I'm aware that the delay is not accounted for in the constant. This
  is intentional because it sounds better. I want this to be an internal
  adjustment which is not to be considered by any external usages of those
  constants. Just sounds better.
  
  ### aab01b0fe5 Remove weird/dated schedule
  usage
  
  This causes audible artifacts in playback. The track is not set to zero
  volume early enough. Original change was made in [this ancient
  PR](https://github.com/ppy/osu/pull/9792) (see
  [commit](https://github.com/ppy/osu/commit/688e4479506645bdacee7cdc7ae9ee9deaa5f1b4)).
  The original failure does not occur. Tests still pass. I'd argue that if
  we encounter this again, it's an issue at the call site, not this code.
  
  This is a drive-by fix, as I noticed this glitching while working on the
  main issue here. Arguably should be it's own PR 🤷 .

  Dean Herbert · 2026-03-18 10:58:05 +01:00
  978e380087
• Fix star rating no longer updating from mod setting changes after re-entering song select (#36601)
  Bartłomiej Dach · 2026-02-06 19:00:20 +09:00
  5e01dda411
• Fix BeatmapDifficultyCache's bindable tracking not correctly updating BeatmapInfo metadata on invalidation
  Dean Herbert · 2026-01-29 01:47:13 +09:00
  b705313d3f
• Avoid triggering bindable updates if mods haven't actually changed
  Dean Herbert · 2026-01-29 00:28:58 +09:00
  10651512a5
• More aggressively cancel debounced tracked bindable updates

  If we've run an update since we can cancel the scheduled debounce run.

  Dean Herbert · 2026-01-29 00:28:58 +09:00
  4c2ffa0f79
• Make BeatmapDifficultyCache's returned bindable non-nullable
  Dean Herbert · 2025-05-25 22:27:37 +09:00
  6500be3f2d
• Start with known star rating for no-mod-no-convert rather than zero

  This makes displays look better, even if it's incorrect momentarily.

  Dean Herbert · 2025-05-19 16:18:54 +09:00
  01edee123f
• Add debounce to difficulty calculation operations in churn scenarios
  Dean Herbert · 2025-05-19 16:16:33 +09:00
  192a1642d4
• Simplify implementation
  Dan Balasescu · 2024-10-17 20:00:17 +09:00
  bfcf6693ca
• Compute maximum performance along with difficulty
  Dan Balasescu · 2024-10-17 19:26:19 +09:00
  d195a69447
• Partial everything
  Dan Balasescu · 2022-11-27 00:00:27 +09:00
  7bc8908ca9
• Merge branch 'master' into beatmap-update-flow
  Dean Herbert · 2022-06-30 16:44:17 +09:00
  98938821e5
• Guard against NaN star difficulty results
  Dean Herbert · 2022-06-29 21:02:29 +09:00
  18d465eff7
• Difficulty cache invalidation flow
  Dean Herbert · 2022-06-24 21:02:38 +09:00
  30b3973c9f
• Remove unused bindable retrieval flow
  Dean Herbert · 2022-06-24 17:09:56 +09:00
  c179127670
• Use AddOnce for update calls
  Dean Herbert · 2022-06-24 17:09:56 +09:00
  0755430006
• Move GetDifficultyRating helper method to StarDifficulty
  Dean Herbert · 2022-06-23 19:51:58 +09:00
  ef258122d2
• Enable NRT on BeatmapDiffiultyCache
  Dean Herbert · 2022-06-23 19:51:58 +09:00
  b068df2149
• Automated #nullable processing
  Dan Balasescu · 2022-06-17 16:37:17 +09:00
  f8830c6850
• Remove unnecessary Ruleset null check in BeatmapDifficultyCache
  Dean Herbert · 2022-01-13 13:19:49 +09:00
  86b2ac3217
• Fix BeatmapDifficultyCache not working with detached beatmaps
  Dean Herbert · 2022-01-12 17:00:17 +09:00
  76670a8faa
• Update usages to new naming
  Dean Herbert · 2022-01-06 22:54:43 +09:00
  00177a3ae1
• Update continuation usages to use GetCompletedResult
  Dean Herbert · 2022-01-06 22:53:07 +09:00
  3ea7588a91
• Replace usage of .Result with .WaitSafelyForResult
  Dean Herbert · 2022-01-04 11:51:41 +09:00
  73b40e6833
• Merge branch 'master' into primary-key-consistency
  Dean Herbert · 2021-12-08 21:34:38 +09:00
  8d9c37a825
• Update all actual usages of RulesetInfo.ID to use OnlineID instead
  Dean Herbert · 2021-11-24 15:50:26 +09:00
  bbd3ea5b77
• Update cases where equality can be used instead of primary key equality
  Dean Herbert · 2021-11-24 12:49:57 +09:00
  729f681938
• Log all non-cancellation errors in difficulty cache
  Bartłomiej Dach · 2021-11-20 17:32:40 +01:00
  a7e45a9098
• Change difficulty cache storage type to nullable

  The recent changes related to adding support for working beatmap load
  cancellation exposed a flaw in the beatmap difficulty cache. With the
  way the difficulty computation logic was written, any error in the
  calculation process (including beatmap load timeout, or cancellation)
  would result in a 0.00 star rating being permanently cached in memory
  for the given beatmap.
  
  To resolve, change the difficulty cache's return type to nullable.
  In failure scenarios, `null` is returned, rather than
  `default(StarDifficulty)` as done previously.

  Bartłomiej Dach · 2021-11-20 17:00:50 +01:00
  15feb17da8
• Replace existing cases with new helper method
  Dean Herbert · 2021-11-19 22:15:41 +09:00
  83b4625bd5
• Merge branch 'master' into beatmap-cancellation-token
  Bartłomiej Dach · 2021-11-17 21:52:30 +01:00
  8b134914cf
• Update all simple cases of switching to IWorkingBeatmap
  Dean Herbert · 2021-11-17 20:56:57 +09:00
  7c2e79f911
• Standardise naming of CancellationToken parameters
  Dean Herbert · 2021-11-16 14:45:51 +09:00
  6cca657a2d
• Merge branch 'master' into beatmap-cancellation-token
  Dean Herbert · 2021-11-16 14:43:13 +09:00
  53c0682a08
• Rename BeatmapInfo.OnlineBeatmapID to OnlineID to match interface
  Dean Herbert · 2021-11-12 17:46:24 +09:00
  6a098a8634
• Add cancellation token support for beatmap difficulty calculation.
  Tollii · 2021-11-06 00:21:29 +01:00
  eb7d04bc77
• Fix incorrect check for local beatmap in BeatmapDifficultyCache

  This was correct in the WIP branch I have, but pulled out alone (where
  usages of `ToBeatmapInfo` still exist) it was not enough.

  Dean Herbert · 2021-11-03 03:12:52 +09:00
  08b0ffad50
• Refactor BeatmapDifficultyCache to work with IBeatmapInfo
  Dean Herbert · 2021-10-29 16:45:10 +09:00
  7583435901
• Avoid returning a live IEnumerable
  Dean Herbert · 2021-10-05 17:01:07 +09:00
  599d82e383
• Further asyncify load process
  smoogipoo · 2021-10-05 11:26:13 +09:00
  593da79bbc
• Rename all remaining cases
  Dean Herbert · 2021-10-03 00:55:29 +09:00
  ec61c3c5ee
• Fix difficulty cache lookups sharing underlying mod instances

  `DifficultyCacheLookup`s were storing raw `Mod` instances into their
  `OrderedMods` field. This could cause the cache lookups to wrongly
  succeed in cases of mods with settings. The particular case that
  triggered this fix was Difficulty Adjust.
  
  Because the difficulty cache is backed by a dictionary, there are two
  stages to the lookup; first `GetHashCode()` is used to find the
  appropriate hash bucket to look in, and then items from that hash bucket
  are compared against the key being searched for via the implementation
  of `Equals()`.
  
  As it turns out, the first hashing step ended up being the saving grace
  in most cases, as the hash computation included the values of the mod
  settings. But the Difficulty Adjust failure case was triggered by the
  quirk that `GetHashCode(0) == GetHashCode(null) == 0`.
  
  In such a case, the `Equals()` fallback was used. But as it turns out,
  because the `Mod` instance stored to lookups was not cloned and
  therefore potentially externally mutable, it could be polluted after
  being stored to the dictionary, and therefore breaking the equality
  check. Even though all of the setting values were compared, the hash
  bucket didn't match the actual contents of the lookup anymore (because
  they were mutated externally, e.g. by the user changing the mod setting
  values in the mod settings overlay).
  
  To resolve, clone out the mod structure before creating all difficulty
  lookups.

  Bartłomiej Dach · 2021-08-21 15:50:33 +02:00
  995338029c
• Track changes to mod settings in beatmap difficulty cache with 100ms debouncing
  Salman Ahmed · 2021-08-17 05:46:05 +03:00
  32ba525555
• Consider mod equality in difficulty cache lookup rather than acronym
  Salman Ahmed · 2021-08-17 04:27:43 +03:00
  0291cd5ae2
• Make BeatmapDifficultyCache.GetDifficultyAsync virtual
  Salman Ahmed · 2021-05-14 15:52:36 +03:00
  e0728a6e19
• Update xmldoc
  Dean Herbert · 2021-02-25 16:22:40 +09:00
  5fa9bf61b6
• Allow determining a BeatmapDifficultyCache's bindable return's completion state via nullability
  Dean Herbert · 2021-02-25 16:19:01 +09:00
  03771ce8ec
• Fix deadlock scenario when calculating fallback difficulty

  The previous code would run a calcaulation for the beatmap's own ruleset
  if the current one failed. While this does make sense, with the current
  way we use this component (and the implementation flow) it is quite unsafe.
  
  The to the call on `.Result` in the `catch` block, this would 100%
  deadlock due to the thread concurrency of the `ThreadedTaskScheduler`
  being 1. Even if the nested run could be run inline (it should be), the
  task scheduler won't even get to the point of checking whether this is
  feasible due to it being saturated by the already running task.
  
  I'm not sure if we still need this fallback lookup logic. After removing
  it, it's feasible that 0 stars will be returned during the scenario that
  previously caused a deadlock, but I don't necessarily think this is
  incorrect. There may be another reason for this needing to exist which
  I'm not aware of (diffcalc?) but if that's the case we may want to move
  the try-catch handling to the point of usage.
  
  To reproduce the deadlock scenario with 100% success (the repro
  instructions in the linked issue aren't that simple and require some
  patience and good timing), the main portion of the lookup can be changed
  to randomly trigger a nested lookup:
  
  ```
  if (RNG.NextSingle() > 0.5f)
      return GetAsync(new
  DifficultyCacheLookup(key.Beatmap, key.Beatmap.Ruleset,
  key.OrderedMods)).Result;
  else
      return new StarDifficulty(attributes);
  ```
  
  After switching beatmap once or twice, pausing debug and viewing the
  state of threads should show exactly what is going on.

  Dean Herbert · 2021-01-14 18:25:34 +09:00
  8a0b975d71
• Add explicit lock object and some xmldoc for clarity
  Dean Herbert · 2020-11-10 14:31:52 +09:00
  6cc0bf17a9