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.
Pull request #11107 introduced changes in osu! performance calculation,
related to a scaling coefficient applied to the speed and aim skills.
The coefficient in question was dependent on the approach rate of
a map. During a post-merge review of that PR, it was spotted that
the scaling coefficient for speed also had a 10x buff applied for AR<8,
which could reach magnitudes as large as 80% on AR0, which seems quite
exorbitant. This change was not discussed or mentioned anywhere in the
review process.
Revert back to the old multiplier of 0.01 rather than 0.1 for AR<8. The
negative slope through AR0 to 8 is retained in its previous form.
`onLoadRequested()` always released the `readyClickOperation` ongoing
operation, without checking whether it actually needs to/should (it
should only do so if the action initiating the operation was starting
the game by the host). This would crash all other consumers, who already
released the operation when their ready-up operation completed server
side.
To resolve, relax the constraint such that the operation can be ended
multiple times in any order. At the end of the day the thing that
matters is that the operation is done and the ready button is unblocked.
Finishing an operation started via
`OngoingOperationTracker.BeginOperation()` was risky in cases where the
operation ended at a callback on another thread (which, in the case of
multiplayer, is *most* cases). In particular, if any consumer registered
a callback that mutates transforms when the operation ends, it would
result in crashes after the framework-side safety checks.
Rework `OngoingOperationTracker` into an always-present component
residing in the drawable hierarchy, and ensure that the
`operationInProgress` bindable is always updated on the update thread.
This way consumers don't have to add local schedules in multiple places.
Fixes a potential crash when moving from main menu to editor after
having previously opened the login settings overlay. Setting the
activity in BDL as done before is unsafe, as that set can trigger value
change callbacks, which in turn can trigger adding transforms, which
should always be done on the update thread.
Semantically it also makes sense, as the user activity should change
once the screen they're moving to has actually loaded and displayed to
the user.
Culls another non-thread-safe mutation of the `Playing` bindable.
It seems to be a weird vestige from an earlier revision of the old
"direct" panel, which relied on `DisposeOnDeathRemoval` to finish track
playback (and then was removed in
6c150c9ed7). The play button is no longer
responsible for managing preview track lifetime anyway;
`PreviewTrackManager`'s method are intended for that.
Revealed by the framework-side transform thread safety checks. `Stopped`
is even annotated as not being thread-safe (but was annotated as such
long after the class's nascence).
