From f13ca28d5eae95792bb6cdac1e75f05f56d26c2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 4 Jun 2024 10:25:08 +0200 Subject: [PATCH 1/2] Fix performance overhead from ternary state bindable callbacks when selection is changing Closes https://github.com/ppy/osu/issues/28369. The reporter of the issue was incorrect; it's not the beat snap grid that is causing the problem, it's something far stupider than that. When the current selection changes, `EditorSelectionHandler.UpdateTernaryStates()` is supposed to update the state of ternary bindables to reflect the reality of the current selection. This in turn will fire bindable change callbacks for said ternary toggles, which heavily use `EditorBeatmap.PerformOnSelection()`. The thing about that method is that it will attempt to check whether any changes were actually made to avoid producing empty undo states, *but* to do this, it must *serialise out the entire beatmap to a stream* and then *binary equality check that* to determine whether any changes were actually made: https://github.com/ppy/osu/blob/7b14c77e43e4ee96775a9fcb6843324170fa70bb/osu.Game/Screens/Edit/EditorChangeHandler.cs#L65-L69 As goes without saying, this is very expensive and unnecessary, which leads to stuff like keeping a selection box active while a taiko beatmap is playing under it dog slow. So to attempt to mitigate that, add precondition checks to every single ternary callback of this sort to avoid this serialisation overhead. And yes, those precondition checks use linq, and that is *still* faster than not having them. --- .../Edit/TaikoSelectionHandler.cs | 6 ++++++ .../Compose/Components/EditorSelectionHandler.cs | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/osu.Game.Rulesets.Taiko/Edit/TaikoSelectionHandler.cs b/osu.Game.Rulesets.Taiko/Edit/TaikoSelectionHandler.cs index 7ab8a54b02..ae6dced9aa 100644 --- a/osu.Game.Rulesets.Taiko/Edit/TaikoSelectionHandler.cs +++ b/osu.Game.Rulesets.Taiko/Edit/TaikoSelectionHandler.cs @@ -53,6 +53,9 @@ namespace osu.Game.Rulesets.Taiko.Edit public void SetStrongState(bool state) { + if (SelectedItems.OfType().All(h => h.IsStrong == state)) + return; + EditorBeatmap.PerformOnSelection(h => { if (!(h is Hit taikoHit)) return; @@ -67,6 +70,9 @@ namespace osu.Game.Rulesets.Taiko.Edit public void SetRimState(bool state) { + if (SelectedItems.OfType().All(h => h.Type == (state ? HitType.Rim : HitType.Centre))) + return; + EditorBeatmap.PerformOnSelection(h => { if (h is Hit taikoHit) diff --git a/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs b/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs index a73278a61e..7420362e19 100644 --- a/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs +++ b/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs @@ -198,6 +198,9 @@ namespace osu.Game.Screens.Edit.Compose.Components /// The name of the sample bank. public void AddSampleBank(string bankName) { + if (SelectedItems.All(h => h.Samples.All(s => s.Bank == bankName))) + return; + EditorBeatmap.PerformOnSelection(h => { if (h.Samples.All(s => s.Bank == bankName)) @@ -214,6 +217,9 @@ namespace osu.Game.Screens.Edit.Compose.Components /// The name of the hit sample. public void AddHitSample(string sampleName) { + if (SelectedItems.All(h => h.Samples.Any(s => s.Name == sampleName))) + return; + EditorBeatmap.PerformOnSelection(h => { // Make sure there isn't already an existing sample @@ -231,6 +237,9 @@ namespace osu.Game.Screens.Edit.Compose.Components /// The name of the hit sample. public void RemoveHitSample(string sampleName) { + if (SelectedItems.All(h => h.Samples.All(s => s.Name != sampleName))) + return; + EditorBeatmap.PerformOnSelection(h => { h.SamplesBindable.RemoveAll(s => s.Name == sampleName); @@ -245,6 +254,9 @@ namespace osu.Game.Screens.Edit.Compose.Components /// Throws if any selected object doesn't implement public void SetNewCombo(bool state) { + if (SelectedItems.OfType().All(h => h.NewCombo == state)) + return; + EditorBeatmap.PerformOnSelection(h => { var comboInfo = h as IHasComboInformation; From ecfcf7a2c047a589858a532e0c764b98d9fd3550 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 4 Jun 2024 10:36:30 +0200 Subject: [PATCH 2/2] Add xmldoc mention about performance overhead of `PerformOnSelection()` --- osu.Game/Screens/Edit/EditorBeatmap.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs index 7a3ea474fb..6363ed2854 100644 --- a/osu.Game/Screens/Edit/EditorBeatmap.cs +++ b/osu.Game/Screens/Edit/EditorBeatmap.cs @@ -198,6 +198,11 @@ namespace osu.Game.Screens.Edit /// Perform the provided action on every selected hitobject. /// Changes will be grouped as one history action. /// + /// + /// Note that this incurs a full state save, and as such requires the entire beatmap to be encoded, etc. + /// Very frequent use of this method (e.g. once a frame) is most discouraged. + /// If there is need to do so, use local precondition checks to eliminate changes that are known to be no-ops. + /// /// The action to perform. public void PerformOnSelection(Action action) {