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 KeyCombination.IsPressed() tripping assertions when empty #6304

Merged
merged 2 commits into from
May 29, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented May 27, 2024

Found this while checking something else.

After #6229, KeyCombination.IsPressed() can trip on an assertion:

System.AggregateException : One or more errors occurred. (: )
  ----> NUnit.Framework.AssertionException : :
   at osu.Framework.Testing.TestScene.checkForErrors() in /home/dachb/Documents/opensource/osu-framework/osu.Framework/Testing/TestScene.cs:line 502
   at osu.Framework.Testing.TestScene.UseTestSceneRunnerAttribute.AfterTest(ITest test) in
/home/dachb/Documents/opensource/osu-framework/osu.Framework/Testing/TestScene.cs:line 563
   at NUnit.Framework.Internal.Commands.TestActionCommand.<>c__DisplayClass0_0.<.ctor>b__1(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.BeforeAndAfterTestCommand.<>c__DisplayClass1_0.<Execute>b__1()
   at NUnit.Framework.Internal.Commands.DelegatingTestCommand.RunTestMethodInThreadAbortSafeZone(TestExecutionContext context, Action action)
--AssertionException
   at osu.Framework.Logging.ThrowingTraceListener.Fail(String message1, String message2) in
/home/dachb/Documents/opensource/osu-framework/osu.Framework/Logging/ThrowingTraceListener.cs:line 27
   at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
   at System.Diagnostics.Debug.Fail(String message, String detailMessage)
   at osu.Framework.Input.Bindings.KeyCombination.IsPressed(ImmutableArray`1 pressedPhysicalKeys, InputKey candidateKey) in
/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyCombination.cs:line 206
   at osu.Framework.Input.Bindings.KeyCombination.IsPressed(KeyCombination pressedKeys, InputState inputState, KeyCombinationMatchingMode matchingMode) in
/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyCombination.cs:line 105
   at osu.Framework.Input.Bindings.KeyBindingContainer`1.handleNewPressed(InputState state, InputKey newKey, Nullable`1 scrollDelta, Boolean isPrecise) in
/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyBindingContainer.cs:line 227
   at osu.Framework.Input.Bindings.KeyBindingContainer`1.Handle(UIEvent e) in
/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyBindingContainer.cs:line 125
   at osu.Framework.Graphics.Drawable.OnMouseDown(MouseDownEvent e) in /home/dachb/Documents/opensource/osu-framework/osu.Framework/Graphics/Drawable.cs:line 2157
   at osu.Framework.Graphics.Drawable.TriggerEvent(UIEvent e) in /home/dachb/Documents/opensource/osu-framework/osu.Framework/Graphics/Drawable.cs:line 2033

This is happening game-side when a keybinding is vacated, at which point it is represented by KeyCombination.none or equivalent, i.e. a KeyCombination with its only key being InputKey.None.

InputKey.None is defined to be neither physical or virtual, which trips the aforementioned assertion. To bypass that entire debacle altogether, add an early return path that just returns false if the key combination being tested for being pressed is equivalent to none.

(It cannot be a reference equality check, I checked against game. Sequence equality is required. Something in game is probably instantiating anew. Probably something to do with how keybindings are materialised from realm.)

bdach added 2 commits May 27, 2024 12:43
Found this while checking something else.

After ppy#6229,
`KeyCombination.IsPressed()` can trip on an assertion:

	System.AggregateException : One or more errors occurred. (: )
	  ----> NUnit.Framework.AssertionException : :
	   at osu.Framework.Testing.TestScene.checkForErrors() in /home/dachb/Documents/opensource/osu-framework/osu.Framework/Testing/TestScene.cs:line 502
	   at osu.Framework.Testing.TestScene.UseTestSceneRunnerAttribute.AfterTest(ITest test) in
	/home/dachb/Documents/opensource/osu-framework/osu.Framework/Testing/TestScene.cs:line 563
	   at NUnit.Framework.Internal.Commands.TestActionCommand.<>c__DisplayClass0_0.<.ctor>b__1(TestExecutionContext context)
	   at NUnit.Framework.Internal.Commands.BeforeAndAfterTestCommand.<>c__DisplayClass1_0.<Execute>b__1()
	   at NUnit.Framework.Internal.Commands.DelegatingTestCommand.RunTestMethodInThreadAbortSafeZone(TestExecutionContext context, Action action)
	--AssertionException
	   at osu.Framework.Logging.ThrowingTraceListener.Fail(String message1, String message2) in
	/home/dachb/Documents/opensource/osu-framework/osu.Framework/Logging/ThrowingTraceListener.cs:line 27
	   at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
	   at System.Diagnostics.Debug.Fail(String message, String detailMessage)
	   at osu.Framework.Input.Bindings.KeyCombination.IsPressed(ImmutableArray`1 pressedPhysicalKeys, InputKey candidateKey) in
	/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyCombination.cs:line 206
	   at osu.Framework.Input.Bindings.KeyCombination.IsPressed(KeyCombination pressedKeys, InputState inputState, KeyCombinationMatchingMode matchingMode) in
	/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyCombination.cs:line 105
	   at osu.Framework.Input.Bindings.KeyBindingContainer`1.handleNewPressed(InputState state, InputKey newKey, Nullable`1 scrollDelta, Boolean isPrecise) in
	/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyBindingContainer.cs:line 227
	   at osu.Framework.Input.Bindings.KeyBindingContainer`1.Handle(UIEvent e) in
	/home/dachb/Documents/opensource/osu-framework/osu.Framework/Input/Bindings/KeyBindingContainer.cs:line 125
	   at osu.Framework.Graphics.Drawable.OnMouseDown(MouseDownEvent e) in /home/dachb/Documents/opensource/osu-framework/osu.Framework/Graphics/Drawable.cs:line 2157
	   at osu.Framework.Graphics.Drawable.TriggerEvent(UIEvent e) in /home/dachb/Documents/opensource/osu-framework/osu.Framework/Graphics/Drawable.cs:line 2033

This is happening game-side when a keybinding is vacated, at which point
it is represented by `KeyCombination.none` or equivalent, i.e. a
`KeyCombination` with its only key being `InputKey.None`.

`InputKey.None` is defined to be neither physical or virtual, which
trips the aforementioned assertion. To bypass that entire debacle
altogether, add an early return path that just returns false if the key
combination being tested for being pressed is equivalent to `none`.

(It cannot be a reference equality check, I checked against game.
Sequence equality is required. Something in game is probably
instantiating anew. Probably something to do with how keybindings are
materialised from realm.)
Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

The assertion that failed is still kinda wrong, but removing it also feels wrong. Probably out of scope for this PR.

diff --git a/osu.Framework/Input/Bindings/KeyCombination.cs b/osu.Framework/Input/Bindings/KeyCombination.cs
index 72c659fb3..e672119ef 100644
--- a/osu.Framework/Input/Bindings/KeyCombination.cs
+++ b/osu.Framework/Input/Bindings/KeyCombination.cs
@@ -203,8 +203,10 @@ internal static bool IsPressed(ImmutableArray<InputKey> pressedPhysicalKeys, Inp
             if (candidateKey.IsPhysical())
                 return pressedPhysicalKeys.Contains(candidateKey);
 
-            Debug.Assert(candidateKey.IsVirtual());
-            return pressedPhysicalKeys.Any(k => getVirtualKey(k) == candidateKey);
+            if (candidateKey.IsVirtual())
+                return pressedPhysicalKeys.Any(k => getVirtualKey(k) == candidateKey);
+
+            return false;
         }
 
         public bool Equals(KeyCombination other) => Keys.SequenceEqual(other.Keys);

@bdach bdach requested a review from smoogipoo May 29, 2024 08:38
@smoogipoo smoogipoo merged commit ddf4a2a into ppy:master May 29, 2024
21 checks passed
@bdach bdach deleted the empty-key-combinations-crash branch May 29, 2024 08:52
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.

None yet

3 participants