The reproduction scenario for the subscription leak is as follows:
1. Switch to a scrolling ruleset (anything but osu! from the standard
ones).
2. Select a beatmap to edit.
3. Load the composer.
4. Go to timing tab.
5. Change a timing point.
6. Go back to the composer.
At this point, `EditorChangeHandler.OnStateChange` will have multiple of
the same delegate in the invocation list.
<img width="691" height="311" alt="Screenshot 2026-03-05 at 11 15 55"
src="https://github.com/user-attachments/assets/57788341-9573-48f1-b360-f21036891081"
/>
That in turn is caused by the fact that changing a timing point *does*
incur a full reload of the composer via the following flow:
https://github.com/ppy/osu/blob/15b6e28ebe888b1a87574891be1a0db3b04093b7/osu.Game/Rulesets/Edit/ScrollingHitObjectComposer.cs#L145https://github.com/ppy/osu/blob/64a29313a852d50095ae4b7ea8f22fde23aa634f/osu.Game/Screens/Edit/Editor.cs#L1137-L1145
This flow is my "fault"; see https://github.com/ppy/osu/pull/28444. The
reason why a full composer reload is used is not clear to my
recollection at this time, but it is likely because it's just the least
likely to fail. A smarter solution that wouldn't require a full reload
would also entail checking that there exists a safe insertion point that
allows replacing timing points in a way that will reflect everywhere it
must. Including all of the `IScrollingAlgorithm` machinery and such.
In general it is not uncommon in the codebase to not bother to clean up
some event callbacks if it is implicitly or explicitly guaranteed that
both objects bound by the callback will always get disposed in tandem at
the same time. This *was* true with this particular flow to a point,
which was until that full composer reload was implemented.
<details>
<summary>To address the elephant in the room</summary>
Someone will inevitably notice https://github.com/ppy/osu/pull/36824
which was a clanked pull request pointing out this leak. And then
someone will inevitably call this "AI discrimination"! *Gasp!*
So first of all, let me stop you right there. Yes, as far as I am
_personally_ concerned, it is "AI discrimination". I invoke the full
force of the Butlerian Jihad.
The clank army's goal is to eradicate my job and make me work in an
Amazon warehouse instead. Or, if not that, at least my job is to be rid
of all remnants of fun I still get from it and for me to be reduced to
that one guy from the meme "i guess we're doin circles now". You know
the one.
I resent this. You attack me directly. I do not perceive the need to
meet you halfway or be civil.
That said, I have too much respect for the users of this software to
leave reports of potentially real issues unchecked. So I did check, and
it was real. And you know what? Good job to the clanker. It did what it
was designed to do: it parsed a code file, recognised a hole in a
pattern it was designed to recognise, and invoked forms of language
given to it to communicate this to the meatbag that opened that PR.
And here's the thing: my primary issue is with that meatbag that opened
that PR. That meatbag served no functional purpose in any of this. The
meatbag took a hose that spews 90% water and 10% raw sewage at random
intervals and pointed it at my house directly, claiming that they just
want to clean it. At no point did the meatbag appear to have the common
decency to pull out a container, pour some magic liquid out, check if
there's sewage in it, and filter it out if there is any. But no, that
would take *effort* and *thought*, would it not? The *effort* and
*thought* that is required of *me* to *review* the clanker's work?
The PR had no reproduction scenario, and had testing checkboxes that
were presumably meant for *me* to check off. Why is it *my* job to
figure all of this out rather than the submitter meatbag's?
I do *not* have obligations towards spew-hose-pointing meatbags. Point
that hose at your own backyard at your peril.
If you *actually manage* to get the clanker to filter out *all* of the
spew without fail itself, my only win condition is gone. But it is not
yet that time. So at least have the decency to check for the spew
yourself, rather than telling the clanker to put checkboxes in the PR
descriptions telling *me* to check for it.
</details>
- [x] Depends on https://github.com/ppy/osu/pull/36741 for merge
conflict avoidance
RFC, cc @OliBomby
## [Adjust behaviour of automatic bank assignment during
placement](https://github.com/ppy/osu/commit/547f55e9b3ded668fe6e1c8865a2d625e64a2f45)
Diatribe time!
This is fallout of the discussion about auto bank in
https://github.com/ppy/osu/issues/36705.
Auto bank in lazer as written before this commit is confused. On stable,
auto bank is closer to "no bank", as in "go look up the current sample
timing point, get the bank of that, and use that". lazer has no timing
points anymore, but people still want auto bank. So what do?
Auto bank for normal samples is somewhat sane still. It only works
during placement, and will just copy the normal bank of the previous
object - if one exists. That said, one *might not* exist, but the
resulting object will still have its normal sample created with
`editorAutoBank: true`. That is largely cosmetic and without
consequences, but this commit fixes that.
Auto bank for *addition* samples, however... Hoo boy.
- For placed objects, auto bank means "take the normal sample, read its
bank, and use that". Simple enough, right?
- Hoooooowever. During placement, auto bank before this commit used to
mean "look at the *previous object*, check if it has an addition sound
and then use its bank, if not use *the previous object's* normal sample
and then use its bank" which is a completely different thing with its
own implications. Like, say, what happens if the previous object uses
the auto addition bank too? What should be copied over? Should it be the
notion of "auto bank" in that the addition bank should match the normal
bank, or should it be the literal bank that the previous object is
using?
This change attempts to define this unambiguously. "Auto additions bank"
means "the same bank as the normal bank of this object", full stop.
## [Do not touch sample toggle state if there are no selected
objects](https://github.com/ppy/osu/commit/052cde5987e48800ec68ab2528c7e0ce3140e6e0)
Fixes issue described in
https://github.com/ppy/osu/issues/36705#issuecomment-3953917163 wherein
opening a sample popover will disable addition bank toggles and toggle
off all addition samples.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
- Closes https://github.com/ppy/osu/issues/30293
- Fixes https://osu.ppy.sh/community/forums/topics/2179339?n=1
Aside from fixing the off-by-one error that I mentioned in
https://github.com/ppy/osu/issues/30293#issuecomment-2413801663, this
also:
- Brings back the behaviour wherein if timing points are arranged very
weird and nightcore would play e.g. two first beats in a timing point
back-to-back, the second timing point is silent.
- Brings back the behaviour wherein the finish sample only plays if
`OmitFirstBarLine` on the timing point is disabled.
However:
- This does not bring back the behaviour wherein hat samples only play
if the slider tick rate is even because that only kind of makes sense in
common time, and if common time is mixed with waltz time or other time
signatures, it just gets weird.
- Also stable has zero attempt for compensating for waltz time anyway,
lazer's behaviour is bespoke, so that is not going to match any way you
cut it.
My testing procedure essentially consisted of getting stable to log when
it was playing nightcore samples and cross-checking the first 30sec or
so of https://osu.ppy.sh/beatmapsets/534385#osu/1131956 (check out the
timing of that beatmap, for something ranked it is DEEPLY messed up).
I guess I can add test cases if deemed required but I already wasted
much more time than I would have liked here...
Unsure about this one, but I find the preceding commit to be very
lacking in explaining to the user why the editor don't work. Shining
some things red may help aid understanding.
* Add failing test coverage for layered hit samples not playing in mania when beatmap is converted
Adding the `osu.Game.Rulesets.Osu` reference to the mania test project
is required so that `HitObjectSampleTest` base logic doesn't die on
https://github.com/ppy/osu/blob/f0aeeeea966f06add12cf2bca3dd48dac8573e82/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs#L88-L91
* Fix layered hit sounds not playing on converted beatmaps in mania
Compare
https://github.com/peppy/osu-stable-reference/blob/f9e58b4864a10f801393199e7652b2192c7342c3/osu!/GameplayElements/HitObjects/HitObject.cs#L476-L477.
In case of converted beatmaps, the last condition there
(`BeatmapManager.Current.PlayMode != PlayModes.OsuMania`) fails,
and thus layered hitsounds are allowed to play.
* Add failing test coverage for mania beatmap conversion assigning wrong samples to spinners
* Fix mania beatmap conversion assigning wrong samples to spinners
A spinner is never `IHasRepeats`. It was a dead condition, leading to
the hitobject generating fallback `NodeSamples`, which in particular
feature a silent tail which stable doesn't do.
Noticeably, stable also appears to force the head of the generated hold
note to have no addition sounds:
https://github.com/peppy/osu-stable-reference/blob/f9e58b4864a10f801393199e7652b2192c7342c3/osu!/GameplayElements/HitObjects/Mania/SpinnerMania.cs#L86-L89
* Add failing test coverage for file hit sample not falling back to plain samples if file missing
* Allow `FileHitSampleInfo` to fall back to standard samples if the file is not found (or not allowed to be looked up)
I'm honestly not 100% as to how closely this matches stable because I
reached the point wherein I'd rather not look at stable code anymore, so
as long as this passes tests I'm fine to wait for someone else to report
new breakage.
* Use alternative workaround for lack of osu! ruleset assembly in mania test project
* Fix encode stability test failures
This is a set of model changes which is supposed to facilitate support
for custom sample sets to the beatmap editor that is on par with stable.
It is the minimal set of changes. Because of this, it can probably be
considered "ugly" or however else you want to put it - but before you
say that, I want to try and pre-empt that criticism by explaining where
the problems lie.
Problem #1: duality in sample models
---
There is currently a weird duality of what a `HitObject`'s samples will
be.
- If an object has just been placed in the editor, and not saved /
decoded yet, it will use `HitSampleInfo`.
- If an object has already been encoded to the beatmap at least once, it
will use `ConvertHitObjectParser.LegacyHitSampleInfo`.
As long as that state of affairs remains, `HitSampleInfo` must be able
to represent anything that `LegacyHitSampleInfo` can, if feature parity
is to be achieved.
Problem 2: The 0 & 1 sample banks
---
Custom sample banks of 2 and above are a pretty clean affair. They map to
a suffix on the sample filename, and said samples are allowed to be
looked up from the beatmap skin. `Suffix` already exists in
`HitSampleInfo`.
However, the 1 custom sample bank is evil. It uses *non-suffixed*
samples, *allows lookups from the beatmap skins*, contrary to no bank /
bank 0, which *also* uses non-suffixed samples, but *doesn't* allow them
to be looked up from the beatmap skin.
This is why `HitSampleInfo.UseBeatmapSamples` has been called to
existence - without it there is no way to represent the ability of using
or not using the beatmap skin assets.
As has been stated previously in discussions about this feature, it's
both a *mapping* and a *skinning* concern.
There are many things you could do about either of these problems, but I
am pretty sure tackling either one is going to take *many* more lines of
code than this commit does. Which is why this is the starting point of
negotiation.
Previously CompositeDrawable.CheckChildrenLife() would be run before lifetimeManager.Update() which lead to the new drawables being inserted into the container but not being made alive immediately, leading to the drawable not becoming visibile until the next update loop.