Reload scrolling hitobject composer on control point changes#28444
Merged
peppy merged 3 commits intoppy:masterfrom Jun 20, 2024
Merged
Reload scrolling hitobject composer on control point changes#28444peppy merged 3 commits intoppy:masterfrom
peppy merged 3 commits intoppy:masterfrom
Conversation
peppy
approved these changes
Jun 20, 2024
bdach
added a commit
to bdach/osu
that referenced
this pull request
Mar 5, 2026
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 two of the same delegate in the invocation list. 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#L145 https://github.com/ppy/osu/blob/64a29313a852d50095ae4b7ea8f22fde23aa634f/osu.Game/Screens/Edit/Editor.cs#L1137-L1145 This flow is my "fault"; see ppy#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.
peppy
pushed a commit
that referenced
this pull request
Mar 5, 2026
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#L145 https://github.com/ppy/osu/blob/64a29313a852d50095ae4b7ea8f22fde23aa634f/osu.Game/Screens/Edit/Editor.cs#L1137-L1145 This flow is my "fault"; see #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 #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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC.
Simplest viable implementation that uses the core idea of a single event on
ControlPointInfoon any and all changes.Note that
ControlPointInfodoes not encompass sample / difficulty points anymore, which is actually okay for editor purposes because those don't influence anything on scrolling rulesets as far as I'm aware.