Reduce allocation overhead for keyboard / binding / button handling #6126
Conversation
| } | ||
|
|
||
| // 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)); |
There was a problem hiding this comment.
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.
| pressedBindings.Remove(binding); | ||
| var binding = pressedBindings[i]; | ||
|
|
||
| if (binding.KeyCombination.IsPressed(pressedCombination, KeyCombinationMatchingMode.Any)) continue; |
There was a problem hiding this comment.
There's a behavioural change here that breaks tests...
The previous code had an explicit pressedInputKeys.Count == 0 check. That would basically "short circuit" the IsPressed() checking by just assuming that in such a case all pressed combinations must be released. But now this transformation breaks things because due to the lack of that special check, IsPressed is given KeyCombination.none as the pressedCombination here and dies.
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.
I don't get it.
My thinking is that if pressedBindings is empty, there's nothing to release. What am I missing?
There was a problem hiding this comment.
The previous code was filtering pressedInputBindings basing on the value of pressedKeys. Significant difference.
Which then, with the current code, dies at
There was a problem hiding this comment.
Right. Got confused with variables to some degree. I think your solution is okay.
Overall gains are pretty substantial. All testing done with 13 keystrokes. Ignore overall allocations as the runtime of tests was significantly different.
94b4b24 Remove allocation overhead in button propagation flow
d16d899 Remove allocation overhead from
KeyBindingContainerkey handling7641f70 Reduce allocation overhead of
KeyCombinationconstructor