Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hitobjects' samples getting in bad state when changing selection between objects #30881

Merged
merged 3 commits into from
Nov 26, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Nov 26, 2024

This was causing state pollution in the new selection. I can't see why this needs to happen when a selection changes to another.

This fixes #30839 and also the same issue happening for the new combo toggle.

Of note, this issue only occurs due to this code having Schedule calls, causing the **reset from a Clear() call to be run after the subsequent Add(newSelection):

private void onSelectedItemsChanged(object? sender, NotifyCollectionChangedEventArgs e)
{
// Reset the ternary states when the selection is cleared.
if (e.OldStartingIndex >= 0 && e.NewStartingIndex < 0)

Removing the schedules is also a viable solution:

diff --git a/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs b/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs
index 78cee2c1cf..b7652af23d 100644
--- a/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs
+++ b/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs
@@ -258,9 +258,6 @@ private void createStateBindables()
 
         private void resetTernaryStates()
         {
-            if (SelectedItems.Count > 0)
-                return;
-
             SelectionNewComboState.Value = TernaryState.False;
             AutoSelectionBankEnabled.Value = true;
             SelectionAdditionBanksEnabled.Value = true;
@@ -301,9 +298,9 @@ private void onSelectedItemsChanged(object? sender, NotifyCollectionChangedEvent
         {
             // Reset the ternary states when the selection is cleared.
             if (e.OldStartingIndex >= 0 && e.NewStartingIndex < 0)
-                Scheduler.AddOnce(resetTernaryStates);
+                resetTernaryStates();
             else
-                Scheduler.AddOnce(UpdateTernaryStates);
+                UpdateTernaryStates();
         }
 
         private IEnumerable<IList<HitSampleInfo>> enumerateAllSamples(HitObject hitObject)

…editor

This was causing state pollution in the new selection. I can't see why
this needs to happen when a selection changes to another.

This fixes ppy#30839 and also the same
issue happening for the new combo toggle.

Tests all seem to pass, and I can't immediately find anything broken,
but YMMV.
@peppy peppy changed the title Don't reset state when changing from one selection to another in the editor Fix hitobject's samples getting in bad state when changing selection between objects Nov 26, 2024
@peppy peppy changed the title Fix hitobject's samples getting in bad state when changing selection between objects Fix hitobjects' samples getting in bad state when changing selection between objects Nov 26, 2024
@cloudrac3r
Copy link
Contributor

I've been using this code change for the last couple of hours and it's been working well for me. I haven't seen any negative side effects. Thank you for the fix!

@bdach
Copy link
Collaborator

bdach commented Nov 26, 2024

Removing the schedules is also a viable solution

Schedules date back to ab308d2 so I think I'd rather not touch them.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems ok if CI passes

@bdach bdach merged commit 33c2eb1 into ppy:master Nov 26, 2024
9 of 10 checks passed
@peppy peppy deleted the fix-editor-state-leaking branch November 27, 2024 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor forgets hitsound addition choice when moving between objects
3 participants