-
Notifications
You must be signed in to change notification settings - Fork 423
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
Reduce allocation overhead for keyboard / binding / button handling #6126
Changes from 6 commits
94b4b24
d16d899
7641f70
f80c387
a83fd0f
a38af9e
d3c895a
da75cb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,6 +202,8 @@ private bool handleRepeat(InputState state) | |
return drawables.FirstOrDefault(d => triggerKeyBindingEvent(d, pressEvent)) != null; | ||
} | ||
|
||
private readonly List<IKeyBinding> newlyPressed = new List<IKeyBinding>(); | ||
|
||
private bool handleNewPressed(InputState state, InputKey newKey, Vector2? scrollDelta = null, bool isPrecise = false) | ||
{ | ||
pressedInputKeys.Add(newKey); | ||
|
@@ -210,21 +212,36 @@ private bool handleNewPressed(InputState state, InputKey newKey, Vector2? scroll | |
var pressedCombination = new KeyCombination(pressedInputKeys); | ||
|
||
bool handled = false; | ||
var bindings = KeyBindings?.Except(pressedBindings) ?? Enumerable.Empty<IKeyBinding>(); | ||
var newlyPressed = bindings.Where(m => | ||
m.KeyCombination.IsPressed(pressedCombination, matchingMode)); | ||
|
||
newlyPressed.Clear(); | ||
|
||
if (KeyBindings != null) | ||
{ | ||
foreach (IKeyBinding binding in KeyBindings) | ||
{ | ||
if (pressedBindings.Contains(binding)) | ||
continue; | ||
|
||
if (binding.KeyCombination.IsPressed(pressedCombination, matchingMode)) | ||
newlyPressed.Add(binding); | ||
} | ||
} | ||
|
||
if (KeyCombination.IsModifierKey(newKey)) | ||
{ | ||
// if the current key pressed was a modifier, only handle modifier-only bindings. | ||
// lambda expression is used so that the delegate is cached (see: https://github.com/dotnet/roslyn/issues/5835) | ||
// TODO: remove when we switch to .NET 7. | ||
// ReSharper disable once ConvertClosureToMethodGroup | ||
newlyPressed = newlyPressed.Where(b => b.KeyCombination.Keys.All(key => KeyCombination.IsModifierKey(key))); | ||
for (int i = 0; i < newlyPressed.Count; i++) | ||
{ | ||
if (!newlyPressed[i].KeyCombination.Keys.All(key => KeyCombination.IsModifierKey(key))) | ||
newlyPressed.RemoveAt(i--); | ||
} | ||
} | ||
|
||
// we want to always handle bindings with more keys before bindings with less. | ||
newlyPressed = newlyPressed.OrderByDescending(b => b.KeyCombination.Keys.Length).ToList(); | ||
newlyPressed.Sort(static (a, b) => b.KeyCombination.Keys.Length.CompareTo(a.KeyCombination.Keys.Length)); | ||
|
||
pressedBindings.AddRange(newlyPressed); | ||
|
||
|
@@ -342,14 +359,13 @@ private void handleNewReleased(InputState state, InputKey releasedKey) | |
// we don't want to consider exact matching here as we are dealing with bindings, not actions. | ||
var pressedCombination = new KeyCombination(pressedInputKeys); | ||
|
||
var newlyReleased = pressedInputKeys.Count == 0 | ||
? pressedBindings.ToList() | ||
: pressedBindings.Where(b => !b.KeyCombination.IsPressed(pressedCombination, KeyCombinationMatchingMode.Any)).ToList(); | ||
|
||
foreach (var binding in newlyReleased) | ||
for (int i = 0; i < pressedBindings.Count; i++) | ||
{ | ||
pressedBindings.Remove(binding); | ||
var binding = pressedBindings[i]; | ||
|
||
if (binding.KeyCombination.IsPressed(pressedCombination, KeyCombinationMatchingMode.Any)) continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a behavioural change here that breaks tests... The previous code had an explicit Something like this fixes: diff --git a/osu.Framework/Input/Bindings/KeyBindingContainer.cs b/osu.Framework/Input/Bindings/KeyBindingContainer.cs
index 6fcb2ba06..4e7908212 100644
--- a/osu.Framework/Input/Bindings/KeyBindingContainer.cs
+++ b/osu.Framework/Input/Bindings/KeyBindingContainer.cs
@@ -363,11 +363,12 @@ private void handleNewReleased(InputState state, InputKey releasedKey)
{
var binding = pressedBindings[i];
- if (binding.KeyCombination.IsPressed(pressedCombination, KeyCombinationMatchingMode.Any)) continue;
-
- pressedBindings.RemoveAt(i--);
- PropagateReleased(getInputQueue(binding).Where(d => d.IsRootedAt(this)), state, binding.GetAction<T>());
- keyBindingQueues[binding].Clear();
+ if (pressedInputKeys.Count == 0 || !binding.KeyCombination.IsPressed(pressedCombination, KeyCombinationMatchingMode.Any))
+ {
+ pressedBindings.RemoveAt(i--);
+ PropagateReleased(getInputQueue(binding).Where(d => d.IsRootedAt(this)), state, binding.GetAction<T>());
+ keyBindingQueues[binding].Clear();
+ }
}
}
but also feels immensely dodgy... Not sure what else to do, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get it. My thinking is that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous code was filtering Which then, with the current code, dies at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. Got confused with variables to some degree. I think your solution is okay. |
||
|
||
pressedBindings.RemoveAt(i--); | ||
PropagateReleased(getInputQueue(binding).Where(d => d.IsRootedAt(this)), state, binding.GetAction<T>()); | ||
keyBindingQueues[binding].Clear(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically a change in behaviour as
.Sort()
uses an unstable algorithm while.OrderByDescending()
uses a stable one. I don't realistically think it matters though.