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

Clear pre-existing bindings of same key combination to single action #25152

Merged
merged 9 commits into from
Oct 17, 2023
2 changes: 1 addition & 1 deletion osu.Game.Rulesets.Catch/CatchRuleset.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class CatchRuleset : Ruleset, ILegacyRuleset
new KeyBinding(InputKey.X, CatchAction.MoveRight),
new KeyBinding(InputKey.Right, CatchAction.MoveRight),
new KeyBinding(InputKey.Shift, CatchAction.Dash),
new KeyBinding(InputKey.Shift, CatchAction.Dash),
new KeyBinding(InputKey.MouseLeft, CatchAction.Dash),
};

public override IEnumerable<Mod> ConvertFromLegacyMods(LegacyMods mods)
Expand Down
117 changes: 117 additions & 0 deletions osu.Game.Tests/Input/RealmKeyBindingStoreTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Collections.Generic;
using NUnit.Framework;
using osu.Framework.Input.Bindings;
using osu.Game.Input;
using osu.Game.Input.Bindings;
using osuTK.Input;

namespace osu.Game.Tests.Input
{
[TestFixture]
public class RealmKeyBindingStoreTest
{
[Test]
public void TestBindingsWithoutDuplicatesAreNotModified()
{
var bindings = new List<RealmKeyBinding>
{
new RealmKeyBinding(GlobalAction.Back, KeyCombination.FromKey(Key.Escape)),
new RealmKeyBinding(GlobalAction.Back, KeyCombination.FromMouseButton(MouseButton.Button1)),
new RealmKeyBinding(GlobalAction.MusicPrev, KeyCombination.FromKey(Key.F1)),
new RealmKeyBinding(GlobalAction.MusicNext, KeyCombination.FromKey(Key.F5))
};

int countCleared = RealmKeyBindingStore.ClearDuplicateBindings(bindings);

Assert.Multiple(() =>
{
Assert.That(countCleared, Is.Zero);

Assert.That(bindings[0].Action, Is.EqualTo((int)GlobalAction.Back));
Assert.That(bindings[0].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.Escape)));

Assert.That(bindings[1].Action, Is.EqualTo((int)GlobalAction.Back));
Assert.That(bindings[1].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.ExtraMouseButton1)));

Assert.That(bindings[2].Action, Is.EqualTo((int)GlobalAction.MusicPrev));
Assert.That(bindings[2].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.F1)));

Assert.That(bindings[3].Action, Is.EqualTo((int)GlobalAction.MusicNext));
Assert.That(bindings[3].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.F5)));
});
}

[Test]
public void TestDuplicateBindingsAreCleared()
{
var bindings = new List<RealmKeyBinding>
{
new RealmKeyBinding(GlobalAction.Back, KeyCombination.FromKey(Key.Escape)),
new RealmKeyBinding(GlobalAction.Back, KeyCombination.FromMouseButton(MouseButton.Button1)),
new RealmKeyBinding(GlobalAction.MusicPrev, KeyCombination.FromKey(Key.F1)),
new RealmKeyBinding(GlobalAction.IncreaseVolume, KeyCombination.FromKey(Key.Escape)),
new RealmKeyBinding(GlobalAction.MusicNext, KeyCombination.FromKey(Key.F5)),
new RealmKeyBinding(GlobalAction.ExportReplay, KeyCombination.FromKey(Key.F1)),
new RealmKeyBinding(GlobalAction.TakeScreenshot, KeyCombination.FromKey(Key.PrintScreen)),
};

int countCleared = RealmKeyBindingStore.ClearDuplicateBindings(bindings);

Assert.Multiple(() =>
{
Assert.That(countCleared, Is.EqualTo(4));

Assert.That(bindings[0].Action, Is.EqualTo((int)GlobalAction.Back));
Assert.That(bindings[0].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.None)));

Assert.That(bindings[1].Action, Is.EqualTo((int)GlobalAction.Back));
Assert.That(bindings[1].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.ExtraMouseButton1)));

Assert.That(bindings[2].Action, Is.EqualTo((int)GlobalAction.MusicPrev));
Assert.That(bindings[2].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.None)));

Assert.That(bindings[3].Action, Is.EqualTo((int)GlobalAction.IncreaseVolume));
Assert.That(bindings[3].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.None)));

Assert.That(bindings[4].Action, Is.EqualTo((int)GlobalAction.MusicNext));
Assert.That(bindings[4].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.F5)));

Assert.That(bindings[5].Action, Is.EqualTo((int)GlobalAction.ExportReplay));
Assert.That(bindings[5].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.None)));

Assert.That(bindings[6].Action, Is.EqualTo((int)GlobalAction.TakeScreenshot));
Assert.That(bindings[6].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.PrintScreen)));
});
}

[Test]
public void TestDuplicateBindingsAllowedIfBoundToSameAction()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test could seem like it's exercising an edge case, except for the part where catch actually would get caught on this:

new KeyBinding(InputKey.Shift, CatchAction.Dash),
new KeyBinding(InputKey.Shift, CatchAction.Dash),

which would cause default dash bindings to become spontaneously completely unbound.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I do wonder if the default for catch should be InputKey.None for the second (to create an extra binding but have no default)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but I believe such a change would not apply retroactively to existing configs (unless I specifically include it in the migration I guess).

Copy link
Sponsor Member

@peppy peppy Oct 17, 2023

Choose a reason for hiding this comment

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

This does seem to work on a quick test. Shall we change it while we're here?

diff --git a/osu.Game.Rulesets.Catch/CatchRuleset.cs b/osu.Game.Rulesets.Catch/CatchRuleset.cs
index efd4b46782..63dc01d0d0 100644
--- a/osu.Game.Rulesets.Catch/CatchRuleset.cs
+++ b/osu.Game.Rulesets.Catch/CatchRuleset.cs
@@ -54,7 +54,7 @@ public class CatchRuleset : Ruleset, ILegacyRuleset
             new KeyBinding(InputKey.X, CatchAction.MoveRight),
             new KeyBinding(InputKey.Right, CatchAction.MoveRight),
             new KeyBinding(InputKey.Shift, CatchAction.Dash),
-            new KeyBinding(InputKey.Shift, CatchAction.Dash),
+            new KeyBinding(InputKey.None, CatchAction.Dash),
         };
 
         public override IEnumerable<Mod> ConvertFromLegacyMods(LegacyMods mods)

Or alternatively,

diff --git a/osu.Game.Rulesets.Catch/CatchRuleset.cs b/osu.Game.Rulesets.Catch/CatchRuleset.cs
index efd4b46782..ee2d3ec2ea 100644
--- a/osu.Game.Rulesets.Catch/CatchRuleset.cs
+++ b/osu.Game.Rulesets.Catch/CatchRuleset.cs
@@ -53,8 +53,8 @@ public class CatchRuleset : Ruleset, ILegacyRuleset
             new KeyBinding(InputKey.Left, CatchAction.MoveLeft),
             new KeyBinding(InputKey.X, CatchAction.MoveRight),
             new KeyBinding(InputKey.Right, CatchAction.MoveRight),
-            new KeyBinding(InputKey.Shift, CatchAction.Dash),
-            new KeyBinding(InputKey.Shift, CatchAction.Dash),
+            new KeyBinding(InputKey.LShift, CatchAction.Dash),
+            new KeyBinding(InputKey.RShift, CatchAction.Dash),
         };
 
         public override IEnumerable<Mod> ConvertFromLegacyMods(LegacyMods mods)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

As pointed out on twitch chat, we should likely have the defaults match stable, which would make it:

diff --git a/osu.Game.Rulesets.Catch/CatchRuleset.cs b/osu.Game.Rulesets.Catch/CatchRuleset.cs
index efd4b46782..9ceb78893e 100644
--- a/osu.Game.Rulesets.Catch/CatchRuleset.cs
+++ b/osu.Game.Rulesets.Catch/CatchRuleset.cs
@@ -54,7 +54,7 @@ public class CatchRuleset : Ruleset, ILegacyRuleset
             new KeyBinding(InputKey.X, CatchAction.MoveRight),
             new KeyBinding(InputKey.Right, CatchAction.MoveRight),
             new KeyBinding(InputKey.Shift, CatchAction.Dash),
-            new KeyBinding(InputKey.Shift, CatchAction.Dash),
+            new KeyBinding(InputKey.MouseLeft, CatchAction.Dash),
         };
 
         public override IEnumerable<Mod> ConvertFromLegacyMods(LegacyMods mods)

I think it's fine to leave your test and logic in place as it's probably not going to harm anything. Up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've applied the last diff, and also added backwards migration on top (4cfc95c). I initially wasn't gonna bother but not doing so breaks revert-to-default for reasons I'd rather not have to fix so I decided that I might as well migrate over.

{
var bindings = new List<RealmKeyBinding>
{
new RealmKeyBinding(GlobalAction.Back, KeyCombination.FromKey(Key.Escape)),
new RealmKeyBinding(GlobalAction.Back, KeyCombination.FromKey(Key.Escape)),
new RealmKeyBinding(GlobalAction.MusicPrev, KeyCombination.FromKey(Key.F1)),
};

int countCleared = RealmKeyBindingStore.ClearDuplicateBindings(bindings);

Assert.Multiple(() =>
{
Assert.That(countCleared, Is.EqualTo(0));

Assert.That(bindings[0].Action, Is.EqualTo((int)GlobalAction.Back));
Assert.That(bindings[0].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.Escape)));

Assert.That(bindings[1].Action, Is.EqualTo((int)GlobalAction.Back));
Assert.That(bindings[1].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.Escape)));

Assert.That(bindings[2].Action, Is.EqualTo((int)GlobalAction.MusicPrev));
Assert.That(bindings[2].KeyCombination, Is.EqualTo(new KeyCombination(InputKey.F1)));
});
}
}
}
46 changes: 45 additions & 1 deletion osu.Game/Database/RealmAccess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
using osu.Game.Beatmaps.Legacy;
using osu.Game.Configuration;
using osu.Game.Extensions;
using osu.Game.Input;
using osu.Game.Input.Bindings;
using osu.Game.Models;
using osu.Game.Online.API;
Expand All @@ -34,6 +35,7 @@
using osu.Game.Scoring;
using osu.Game.Scoring.Legacy;
using osu.Game.Skinning;
using osuTK.Input;
using Realms;
using Realms.Exceptions;

Expand Down Expand Up @@ -84,8 +86,9 @@ public class RealmAccess : IDisposable
/// 32 2023-07-09 Populate legacy scores with the ScoreV2 mod (and restore TotalScore to the legacy total for such scores) using replay files.
/// 33 2023-08-16 Reset default chat toggle key binding to avoid conflict with newly added leaderboard toggle key binding.
/// 34 2023-08-21 Add BackgroundReprocessingFailed flag to ScoreInfo to track upgrade failures.
/// 35 2023-10-16 Clear key combinations of keybindings that are assigned to more than one action in a given settings section.
/// </summary>
private const int schema_version = 34;
private const int schema_version = 35;

/// <summary>
/// Lock object which is held during <see cref="BlockAllOperations"/> sections, blocking realm retrieval during blocking periods.
Expand Down Expand Up @@ -1031,6 +1034,47 @@ private void applyMigrationsForVersion(Migration migration, ulong targetVersion)

break;
}

case 35:
{
// catch used `Shift` twice as a default key combination for dash, which generally was bothersome and causes issues elsewhere.
// the duplicate binding logic below had to account for it, it could also break keybinding conflict resolution on revert-to-default.
// as such, detect this situation and fix it before proceeding further.
var catchDashBindings = migration.NewRealm.All<RealmKeyBinding>()
.Where(kb => kb.RulesetName == @"fruits" && kb.ActionInt == 2)
.ToList();

if (catchDashBindings.All(kb => kb.KeyCombination.Equals(new KeyCombination(InputKey.Shift))))
{
Debug.Assert(catchDashBindings.Count == 2);
catchDashBindings.Last().KeyCombination = KeyCombination.FromMouseButton(MouseButton.Left);
}

// with the catch case dealt with, de-duplicate the remaining bindings.
int countCleared = 0;

var globalBindings = migration.NewRealm.All<RealmKeyBinding>().Where(kb => kb.RulesetName == null).ToList();

foreach (var category in Enum.GetValues<GlobalActionCategory>())
{
var categoryActions = GlobalActionContainer.GetGlobalActionsFor(category).Cast<int>().ToHashSet();
var categoryBindings = globalBindings.Where(kb => categoryActions.Contains(kb.ActionInt));
countCleared += RealmKeyBindingStore.ClearDuplicateBindings(categoryBindings);
}

var rulesetBindings = migration.NewRealm.All<RealmKeyBinding>().Where(kb => kb.RulesetName != null).ToList();

foreach (var variantGroup in rulesetBindings.GroupBy(kb => (kb.RulesetName, kb.Variant)))
countCleared += RealmKeyBindingStore.ClearDuplicateBindings(variantGroup);

if (countCleared > 0)
{
Logger.Log($"{countCleared} of your keybinding(s) have been cleared due to being bound to multiple actions. "
+ "Please choose new unique ones in the settings panel.", level: LogLevel.Important);
Comment on lines +1072 to +1073
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LogLevel.Important is a bit of a cheap way to post a notification via the OsuGame logger new entry flow without introducing a dependency on NotificationOverlay.

}

break;
}
}

Logger.Log($"Migration completed in {stopwatch.ElapsedMilliseconds}ms");
Expand Down
26 changes: 26 additions & 0 deletions osu.Game/Input/RealmKeyBindingStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -130,5 +130,31 @@ public static bool CheckValidForGameplay(KeyCombination combination)

return true;
}

/// <summary>
/// Clears all <see cref="RealmKeyBinding.KeyCombination"/>s from the provided <paramref name="keyBindings"/>
/// which are assigned to more than one binding.
/// </summary>
/// <param name="keyBindings">The <see cref="RealmKeyBinding"/>s to de-duplicate.</param>
/// <returns>Number of bindings cleared.</returns>
public static int ClearDuplicateBindings(IEnumerable<IKeyBinding> keyBindings)
{
int countRemoved = 0;

var lookup = keyBindings.ToLookup(kb => kb.KeyCombination);

foreach (var group in lookup)
{
if (group.Select(kb => kb.Action).Distinct().Count() <= 1)
continue;

foreach (var binding in group)
binding.KeyCombination = new KeyCombination(InputKey.None);

countRemoved += group.Count();
}

return countRemoved;
}
}
}
1 change: 1 addition & 0 deletions osu.Game/Rulesets/UI/RulesetInputManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ protected override void ReloadMappings(IQueryable<RealmKeyBinding> realmKeyBindi
base.ReloadMappings(realmKeyBindings);

KeyBindings = KeyBindings.Where(b => RealmKeyBindingStore.CheckValidForGameplay(b.KeyCombination)).ToList();
RealmKeyBindingStore.ClearDuplicateBindings(KeyBindings);
}
}
}
Expand Down