Probably closes https://github.com/ppy/osu/issues/36492.
This is dumb but it's in large part my stupidity.
To begin with, the immediate direct offending call that causes the
observed symptoms is
https://github.com/ppy/osu/blob/a401c7d5e9d6d8b05b2ec293145ad308dfe9d6d0/osu.Game/Screens/Edit/Components/FormSampleSet.cs#L296
The reason why this "invalidation" affects sample volume is that in the
framework implementation, the call [removes the relevant sample factory
from the sample store which is an audio
component](https://github.com/ppy/osu-framework/blob/5b716dcbef6f99e03188a7a7706361fa8445c754/osu.Framework/Audio/Sample/SampleStore.cs#L65-L72).
In the process it also [unbinds audio
adjustments](https://github.com/ppy/osu-framework/blob/5b716dcbef6f99e03188a7a7706361fa8445c754/osu.Framework/Audio/AudioCollectionManager.cs#L37-L38),
which *would* have the effect of resetting the sample volume to 100%,
effectively (and I've pretty much confirmed that that's what happens).
Now for why this call sometimes does the right thing and sometimes
doesn't: Sometimes the call is made in response to an *actual* change to
the beatmap skin, which is correct and expected, if very indirect, but
sometimes it is made *over-eagerly* when there is no reason to recycle
anything yet.
One such circumstance is entering the setup screen, which will still
"invalidate" (read: remove) the samples, but the compose tab hasn't seen
any change to the beatmap skin, so when it is returned to, it has no
reason to retrieve the sample again, and as such it will try to play
samples which are for better or worse in a completely undefined state
because they're not supposed to be *in use* anymore.
Therefore, the right thing here would seem to be to take the
responsibility of invalidation from a random component, and move it to a
place that's *actually* correlated to every other component needing to
recycle samples, e.g. `EditorBeatmapSkin` responding to changes in the
beatmap resources via raising `BeatmapSkinChanged`.
Unfortunately, because of the structure of everything, this recycle
needs to go from targeted to individual samples, to nuking the entire
store. The reason for this is that `RealmBackedResourceStore` does not
provide information as to *what* resources changed, it just says that
*the set of them* did.
For the recycle to be smarter, `EditorBeatmapSkin` would need to know
not only which samples were added or replaced, but also which ones were
*removed*, so that users don't hear phantom samples that no longer exist
in the editor later. That would however be a lot of hassle for nothing,
so I just recycle everything here and hope it won't matter.
As to why I could only reproduce this on this one beatmap - I'm not
super sure. The failure does not seem to be specific to beatmaps, but it
may be exacerbated by certain patterns of accessing samples which means
that beatmaps with high BPM like the one I managed to reproduce this on
may just be more susceptible to this.
As a final note, I do realise that this is not fundamentally improving
the surrounding systems and it's still a pretty rickety thing to do.
It's still on the consumers to know and respond to the sample store
recycle and this is likely to fail if a consumer ever doesn't. That
said, I have no brighter ideas at this point in time that won't involve
me spending a week refactoring audio.
Among others, this features a scary-looking change wherein
`WorkingBeatmap` now exposes a `RealmAccess` via
`IStorageResourceProvider`. This is necessary to make
`RealmBackedResourceStore` actually start firing callbacks when the
edited beatmap's skin is modified by adding new samples to it.
`dotnet format` expects me to put a space between the spread `..` and
the expression after which is UTTERLY STUPID AND UGLY AND WRONG AND
CANNOT BE `.editorconfig`'d BECAUSE [INSERT EXCESSIVELY LONG BLEEP].
God I hate when good features are kneecapped by this sort of stupidity.
The test scene doesn't exercise the custom sample playback, but I hope I
can be forgiven for this as setting up a custom editor beatmap just for
this to work is rather cumbersome.
Bartłomiej Dach ·
Dean Herbert ·