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.

Sign up for GitHub

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

Jump to bottom

Fix InputManager including non-loaded direct children in input queue #6101

Merged
merged 4 commits into from Jan 15, 2024
Merged

Fix InputManager including non-loaded direct children in input queue #6101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ceeb177
Add failing test case
frenzibyte Dec 31, 2023
13d5831
Fix `InputManager` including non-loaded direct children in input queue
frenzibyte Jan 1, 2024
4b19aa3
Fix input manager's positional input queue not checking against `Shou…
frenzibyte Jan 12, 2024
95aafc1
Merge branch 'master' into fix-unexpected-double-key-press-2
bdach Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 75 additions & 2 deletions osu.Framework.Tests/Visual/Input/TestSceneKeyBindingContainer.cs
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
using NUnit.Framework;
using osu.Framework.Graphics;
using osu.Framework.Graphics.UserInterface;
using osu.Framework.Input;
using osu.Framework.Input.Bindings;
using osu.Framework.Input.Events;
using osu.Framework.Testing;
Expand Down Expand Up @@ -48,7 +49,7 @@ public void TestTriggerWithNoKeyBindings()
}

[Test]
public void TestPressKeyBeforeKeyBindingContainerAdded()
public void TestPressHalfCombinationBeforeKeyBindingContainerAdded()
{
List<TestAction> pressedActions = new List<TestAction>();
List<TestAction> releasedActions = new List<TestAction>();
Expand Down Expand Up @@ -81,6 +82,77 @@ public void TestPressKeyBeforeKeyBindingContainerAdded()
AddAssert("ActionA released", () => releasedActions[0], () => Is.EqualTo(TestAction.ActionA));
}

[Test]
public void TestPressKeyBeforeKeyBindingContainerAdded()
{
List<TestAction> pressedActions = new List<TestAction>();
List<TestAction> releasedActions = new List<TestAction>();

AddStep("press enter", () => InputManager.PressKey(Key.Enter));

AddStep("add container", () =>
{
pressedActions.Clear();
releasedActions.Clear();

Child = new TestKeyBindingContainer(mode: SimultaneousBindingMode.All)
{
Child = new TestKeyBindingReceptor
{
Pressed = a => pressedActions.Add(a),
Released = a => releasedActions.Add(a)
}
};
});

AddStep("press key A", () => InputManager.PressKey(Key.A));
AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
AddAssert("ActionA triggered", () => pressedActions[0], () => Is.EqualTo(TestAction.ActionA));
AddAssert("no actions released", () => releasedActions, () => Is.Empty);

AddStep("release key A", () => InputManager.ReleaseKey(Key.A));
AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
AddAssert("only one action released", () => releasedActions, () => Has.Count.EqualTo(1));
AddAssert("ActionA released", () => releasedActions[0], () => Is.EqualTo(TestAction.ActionA));
}

[Test]
public void TestPressKeyBeforeKeyBindingContainerAdded_WithPassThroughInputManager()
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing that I wondered with this test is what happens once Enter is released after the key binding container is loaded / added. And the answer to that is nothing happens, i.e. the input manager does not receive the release of the Enter key. Which is probably a good thing, as it's better to send nothing than half a press, but it still has me worried about the implications on game usages.

So this is probably okay, but I'd appreciate assurances that nothing falls over game-side with this change applied, i.e. running game-side tests and making sure nothing fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that I wondered with this test is what happens once Enter is released after the key binding container is loaded / added. And the answer to that is nothing happens, i.e. the input manager does not receive the release of the Enter key. Which is probably a good thing, as it's better to send nothing than half a press, but it still has me worried about the implications on game usages.

If by "input manager" you mean the key binding container, then that is correct. I had run game-side tests beforehand and ensured nothing broke, but I'll check again to confirm.

{
List<TestAction> pressedActions = new List<TestAction>();
List<TestAction> releasedActions = new List<TestAction>();

AddStep("press enter", () => InputManager.PressKey(Key.Enter));

AddStep("add container", () =>
{
pressedActions.Clear();
releasedActions.Clear();

Child = new PassThroughInputManager
{
Child = new TestKeyBindingContainer(mode: SimultaneousBindingMode.All)
{
Child = new TestKeyBindingReceptor
{
Pressed = a => pressedActions.Add(a),
Released = a => releasedActions.Add(a)
},
}
};
});

AddStep("press key A", () => InputManager.PressKey(Key.A));
AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
AddAssert("ActionA triggered", () => pressedActions[0], () => Is.EqualTo(TestAction.ActionA));
AddAssert("no actions released", () => releasedActions, () => Is.Empty);

AddStep("release key A", () => InputManager.ReleaseKey(Key.A));
AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
AddAssert("only one action released", () => releasedActions, () => Has.Count.EqualTo(1));
AddAssert("ActionA released", () => releasedActions[0], () => Is.EqualTo(TestAction.ActionA));
}

[Test]
public void TestKeyHandledByOtherDrawableDoesNotTrigger()
{
Expand Down Expand Up @@ -419,7 +491,8 @@ private partial class TestKeyBindingContainer : KeyBindingContainer<TestAction>,

public Func<TestAction, bool>? Pressed;

public TestKeyBindingContainer(bool prioritised = false)
public TestKeyBindingContainer(bool prioritised = false, SimultaneousBindingMode mode = SimultaneousBindingMode.None)
: base(mode)
{
Prioritised = prioritised;
}
Expand Down
12 changes: 10 additions & 2 deletions osu.Framework/Input/InputManager.cs
View file
Open in desktop
Expand Up @@ -593,8 +593,12 @@ private SlimReadOnlyListWrapper<Drawable> buildNonPositionalInputQueue()
FrameStatistics.Increment(StatisticsCounterType.InputQueue);

var children = AliveInternalChildren;

for (int i = 0; i < children.Count; i++)
children[i].BuildNonPositionalInputQueue(inputQueue);
{
if (ShouldBeConsideredForInput(children[i]))
children[i].BuildNonPositionalInputQueue(inputQueue);
bdach marked this conversation as resolved.
Show resolved Hide resolved
}

if (!unfocusIfNoLongerValid())
{
Expand All @@ -620,8 +624,12 @@ private SlimReadOnlyListWrapper<Drawable> buildPositionalInputQueue(Vector2 scre
FrameStatistics.Increment(StatisticsCounterType.PositionalIQ);

var children = AliveInternalChildren;

for (int i = 0; i < children.Count; i++)
children[i].BuildPositionalInputQueue(screenSpacePos, positionalInputQueue);
{
if (ShouldBeConsideredForInput(children[i]))
children[i].BuildPositionalInputQueue(screenSpacePos, positionalInputQueue);
}

positionalInputQueue.Reverse();
return positionalInputQueue.AsSlimReadOnly();
Expand Down