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 InputManager including non-loaded direct children in input queue #6101

Merged
merged 4 commits into from Jan 15, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Jan 1, 2024

KeyBindingContainer gets into a weird state when handling input while it's not loaded. Since it's not loaded, KeyBindings would be null since it only gets populated in LoadComplete, so no press/release events are propagated to children for the first key. However, the key gets stored in pressedInputKeys, and later on, when the next key is pressed, the old key incorrectly becomes processed as a new binding as well, causing the double-press issue.

KeyBindingContainer was not supposed to enter the input queue if it's not fully loaded in the first place, but turns out InputManager was incorrectly adding it (by bypassing ShouldBeConsideredForInput).

osu.Framework/Input/InputManager.cs Show resolved Hide resolved
}

[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.

@bdach bdach enabled auto-merge January 15, 2024 20:44
@bdach bdach merged commit eaec94e into ppy:master Jan 15, 2024
13 checks passed
@frenzibyte frenzibyte deleted the fix-unexpected-double-key-press-2 branch January 16, 2024 03:39
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.

Holding keys before gameplay starts can result in incorrect input handling
2 participants