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

Reduce allocation overhead for keyboard / binding / button handling #6126

Merged
merged 8 commits into from
Jan 12, 2024
38 changes: 27 additions & 11 deletions osu.Framework/Input/Bindings/KeyBindingContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
Copy link
Collaborator

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.


pressedBindings.AddRange(newlyPressed);

Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it.

My thinking is that if pressedBindings is empty, there's nothing to release. What am I missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous code was filtering pressedInputBindings basing on the value of pressedKeys. Significant difference.

Which then, with the current code, dies at

https://github.com/ppy/osu-framework/blob/master/osu.Framework%2FInput%2FBindings%2FKeyCombination.cs#L75

Copy link
Member Author

Choose a reason for hiding this comment

The 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();
}
Expand Down
28 changes: 21 additions & 7 deletions osu.Framework/Input/Bindings/KeyCombination.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,25 @@ namespace osu.Framework.Input.Bindings
/// Construct a new instance.
/// </summary>
/// <param name="keys">The keys.</param>
/// <remarks>This constructor is not optimized. Hot paths are assumed to use <see cref="FromInputState(InputState, Vector2?)"/>.</remarks>
public KeyCombination(IEnumerable<InputKey>? keys)
public KeyCombination(ICollection<InputKey>? keys)
{
Keys = keys?.Any() == true ? keys.Distinct().OrderBy(k => (int)k).ToImmutableArray() : none;
bdach marked this conversation as resolved.
Show resolved Hide resolved
if (keys == null || !keys.Any())
{
Keys = none;
return;
}

var keyBuilder = ImmutableArray.CreateBuilder<InputKey>(keys.Count);

foreach (var key in keys)
{
if (!keyBuilder.Contains(key))
keyBuilder.Add(key);
}

keyBuilder.Sort();

Keys = keyBuilder.MoveToImmutable();
}

/// <summary>
Expand All @@ -41,7 +56,7 @@ public KeyCombination(IEnumerable<InputKey>? keys)
/// <param name="keys">The keys.</param>
/// <remarks>This constructor is not optimized. Hot paths are assumed to use <see cref="FromInputState(InputState, Vector2?)"/>.</remarks>
public KeyCombination(params InputKey[] keys)
: this(keys.AsEnumerable())
: this((ICollection<InputKey>)keys)
{
}

Expand All @@ -51,7 +66,7 @@ public KeyCombination(params InputKey[] keys)
/// <param name="keys">A comma-separated (KeyCode in integer) string representation of the keys.</param>
/// <remarks>This constructor is not optimized. Hot paths are assumed to use <see cref="FromInputState(InputState, Vector2?)"/>.</remarks>
public KeyCombination(string keys)
: this(keys.Split(',', StringSplitOptions.RemoveEmptyEntries).Select(s => (InputKey)int.Parse(s)))
: this(keys.Split(',', StringSplitOptions.RemoveEmptyEntries).Select(s => (InputKey)int.Parse(s)).ToArray())
{
}

Expand Down Expand Up @@ -643,8 +658,7 @@ public static KeyCombination FromInputState(InputState state, Vector2? scrollDel
}

Debug.Assert(!keys.Contains(InputKey.None)); // Having None in pressed keys will break IsPressed
keys.Sort();
return new KeyCombination(keys.ToImmutable());
return new KeyCombination(keys);
}
}

Expand Down
11 changes: 10 additions & 1 deletion osu.Framework/Input/ButtonEventManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,16 @@ private void handleButtonUp(InputState state)
/// <returns>The drawable which handled the event or null if none.</returns>
protected Drawable? PropagateButtonEvent(IEnumerable<Drawable> drawables, UIEvent e)
{
var handledBy = drawables.FirstOrDefault(target => target.TriggerEvent(e));
Drawable? handledBy = null;

foreach (Drawable target in drawables)
{
if (target.TriggerEvent(e))
{
handledBy = target;
break;
}
}

if (handledBy != null)
Logger.Log($"{e} handled by {handledBy}.", LoggingTarget.Runtime, LogLevel.Debug);
Expand Down
Loading