Clear pre-existing bindings of same key combination to single action #25152
Conversation
…the same combination This actually seems to be the case in catch (dash is bound to shift twice). This is annoying but harmless, so let's work around it for now to avoid ruining users' configs.
| } | ||
|
|
||
| [Test] | ||
| public void TestDuplicateBindingsAllowedIfBoundToSameAction() |
There was a problem hiding this comment.
This test could seem like it's exercising an edge case, except for the part where catch actually would get caught on this:
osu/osu.Game.Rulesets.Catch/CatchRuleset.cs
Lines 56 to 57 in 75011db
which would cause default dash bindings to become spontaneously completely unbound.
There was a problem hiding this comment.
I do wonder if the default for catch should be InputKey.None for the second (to create an extra binding but have no default)...
There was a problem hiding this comment.
Sure, but I believe such a change would not apply retroactively to existing configs (unless I specifically include it in the migration I guess).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
This mirrors stable. Although the way stable does this is pretty dodgy, see: https://github.com/peppy/osu-stable-reference/blob/master/osu!/GameModes/Play/Rulesets/Fruits/RulesetFruits.cs#L274-L275
This PR enforces the rules set in #25105 backwards to users' existing configs. If duplicate bindings are detected in the user's config, they will receive the following notification once the migration has completed:
Additionally, to curb funny business like somebody downloading realm studio to edit the db directly, ruleset key binding containers do the key combination uniqueness check separately, and will silently refuse to use key combinations bound to multiple actions. The precedent in behaviour here is #12933.
I am not bothering to do this for
GlobalActionContainerbecause it doesn't feel as important. I will bother to do it if it is deemed important.