Revert "Fix O(n²) overhead from individual blueprint additions / removals by batching" - #38506
Merged
Conversation
…vals by batching" Fixes ppy#38483. See attached test case for reproduction. The reason why this fixes things is that delaying the addition of the blueprint was causing its nested `DrawableHitObject` to not have `HitObject` applied to it correctly. One place that depends on that and crashes in its absence is object-to-object snapping. Bad one to let through but short of manually fuzzing editor for however long I'm not sure how I could have seen this one coming. I was hoping tests would catch this sort of thing, which they clearly did not. I suppose this is still karma for attempting to paper over structural shortcomings instead of spending 2 weeks on refactoring everything. I don't see any evidence on sentry that the other change ("Fix O(n²) overhead from timeline blueprints re-sorting on every depth change") needs reverting, but if it is felt like a safer option to do so I shall oblige. Not like there is any perceivable performance increase from it anyhow. Probably worth a hotfix given count of duplicates (ppy#38488, ppy#38489, ppy#38502)
bdach
added
area:editor
type/reliability
peppy
approved these changes
Aug 3, 2026
peppy
pushed a commit
that referenced
this pull request
Aug 4, 2026
…38513) Closes #38509. You might be thinking: "@bdach is dumb and/or incompetent for not just directly reverting #38464 and instead just reverting half of it in #38506". That is certainly what *I* was thinking until I tried reverting and found that it actually doesn't help. This is actually a silent regression from #37485! The chain of failure goes something like this: 1. Start with editor seeked to a start time of one of the existing objects. 2. Begin placing a spinner. 3. Click the object that starts at the current time. 4. On master, `SpinnerPlacementBlueprint` does not consume the left click input when spinner placement has already started. This means that the left click input is allowed to fall through to the blueprint container. 5. When the left click input falls through to the blueprint container, it triggers selection logic to fire and the existing object to become selected. 6. Object selection changing also changes the currently used tool to the "select" tool: https://github.com/ppy/osu/blob/cd27c935d6c15c3c6f2f5b561cc3340130f27407/osu.Game/Rulesets/Edit/HitObjectComposer.cs#L504-L511 7. Changing the currently used tool causes any pending placement to be committed if valid: https://github.com/ppy/osu/blob/abbf6e23492c471d722827c5b8baa935fad19419/osu.Game/Screens/Edit/Compose/Components/ComposeBlueprintContainer.cs#L437-L438 8. Committing the placement causes *the object whose attempt at selection triggered this entire chain of events to begin with* to be removed: https://github.com/ppy/osu/blob/cd27c935d6c15c3c6f2f5b561cc3340130f27407/osu.Game/Rulesets/Edit/HitObjectComposer.cs#L545-L550 And therefore the failure is understandable, because at that point the placement blueprint of the now-removed object *really* does not reside in the container. Prior to #37485 this would not cause an issue because the objects would both exist here. To fix, prevent the issue at point (4) by making the placement blueprint consume left clicks and not allow fall-through to existing objects.
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.
Fixes #38483.
See attached test case for reproduction.
The reason why this fixes things is that delaying the addition of the blueprint was causing its nested
DrawableHitObjectto not haveHitObjectapplied to it correctly. One place that depends on that and crashes in its absence is object-to-object snapping.Bad one to let through but short of manually fuzzing editor for however long I'm not sure how I could have seen this one coming. I was hoping tests would catch this sort of thing, which they clearly did not. I suppose this is still karma for attempting to paper over structural shortcomings instead of spending 2 weeks on refactoring everything.
I don't see any evidence on sentry that the other change ("Fix O(n²) overhead from timeline blueprints re-sorting on every depth change") needs reverting, but if it is felt like a safer option to do so I shall oblige. Not like there is any perceivable performance increase from it anyhow.
@peppy Probably worth a hotfix given count of duplicates (#38488, #38489, #38502)