Multi-Backend Input for Silk.NET 3.0

[Perksey]

This enables input to use multiple backends (i.e. so we can have windowing-based input as well as something like OpenXR) and contains other refactorings for 3.0.

Unlike previous 3.0 proposals this proposes only a minor change of the public API.

[Perksey]

This is ready for pre-meeting review.

[HurricanKai]

Please rebase proposal/3.0-input-llapi to not include all the 2.7 changes.

[HurricanKai]

See comments.
Also, please elaborate a bit on how these new types are supposed to work. I'm just massively confused what the interactions are and what the design is here.

[HurricanKai]

Why in a separate package? at this point I'm pretty confident one can ship both in one package.

[Perksey]

I'm not entirely convinced this is possible, they're structured differently and even if we can NuGet will not be able to distinguish dependencies.

[Perksey]

Can I get a follow-up on this and resolve discussions if you think they're fine?

[HurricanKai]

Looking pretty good now. I'm not fully convinced it's good to have Add as a normal method still, I think the InputContext also calling the constructor might be easier to use.

[Perksey]

Few more small tweaks on the way based on Discord conversations and personal gripes...

[Perksey]

I think the InputContext also calling the constructor might be easier to use.

Unfortunately wouldn't be possible as we need to capture the API object and other native handles when creating the backends (won't just be parameterless so we can't use the T : new() constraint)

[Perksey]

@HurricanKai @ThomasMiz are you happy with this? Please approve on GitHub if you are.

[thargy]

Hi @Perksey & @HurricanKai,

The more I look into this the more my spidey-sense is tingling. The source generator requirement breaks KISS and produces non-intuitive behaviour for a consumer of the libraries. Further, it produces a default behaviour for the backends we know about, that isn't reflected for those we don't know about (which is bad design). Every time a solution like this has appeared in the past I've eventually realised there was a much simpler approach.

I get we don't want a dependence from Input -> Backend, for example, when using VR we won't want the Window backend, etc.

The case for why we don't want backends dependent on input is far less clear though. I suspect the main justification is that you might wish to use a separate input system. This is countered by 2 approaches, have the backend implement functionality for input by implementing a thin set of functionality atop of the base classes and interfaces in input, but ensure that initialisation/etc. is optional (classic approach); or have a third package that is dependent on both the Backend and Input (this is overkill).

[Perksey]

One way we have countered this in the past is having an interchange type in Silk.NET.Core. For Windowing, this was INativeWindow - an interface that just contains the handles we need and nothing more. We're trying to avoid using Silk.NET.Core as much as possible, as in 2.X this became a massive dumping ground for anything that makes interoperability with any of our packages better.

In Silk.NET 1.X, we did have a package which literally only existed to bond Windowing and Input, but I don't think that was a good solution. 2.X is structured such that each backend has its own package. We could do this again (i.e. Silk.NET.Input.Windowing and Silk.NET.Input.Common) and reintroduce the Silk.NET.Input "metapackage", but this deviates from the Windowing redesign which mandates that one "reference implementation" is bundled with the main Windowing package (i.e. there is no separate "common"). Personally if we do something for Input in terms of package structure I'd like it to be reflected in Windowing as well for consistency.

EDIT: on the back of this i really don't want to change anything in Windowing, what we have now is really good and if we do package splits we'll have lots of horrible concerns with backend resolution.

[HurricanKai]

I'm not a big fan of using source generators for this either, but it's a bit unclear to me what other options there are? We don't want to use reflection, and we don't want to have the user deal with what backend they use.

[thargy]

I've created this Gist to show how I would build the API, based on our conversation at the last working group about immutability and asynchrony. It compiles but isn't 'tested', it is purely to make some of the ideas more concrete to aid discussion.

I've put a basic (not yet thread-safe, but easy to adjust) implementation of a MouseDevice, and a 'Window' based backend which would appear either in the Windowing NuGet or an Input.Windowing shim NuGet as discussed. The underlying system will support any generic device and is future-proof. Most importantly it has the flexibility to use a polling (state snapshot) approach or an event-based approach depending on the consumer preference.

I've illustrated those use-cases in the example code at the start. In essence, using a mouse event can be as simple as:

// Get a backend that implements IInputBackend
IInputBackend backend = new Window();

// Create a new context from any number of backend(s)
InputContext context = new InputContext(backend);

// Find a connected mouse
MouseDevice mouseDevice = context.FindDevice<MouseDevice)(true);

// Subscribe to events
mouseDevice.MouseMove += 
    e => Console.WriteLine($"Mouse moved {e.Delta.X}, {e.Delta.Y} to {e.Position.X}, {e.Position.Y}");

Though the examples, demonstrate more robust connection handling. Alternatively you can just find a connected device and get its current state, each time round the game loop:

// Will find the first valid, connected, mouse state each time.
MouseState state = context.GetState<MouseState>();
Console.WriteLine($"Mouse moved to {state.X}, {state.Y}");

The idea is that the Input 'common' NuGet would implement the interfaces and abstract classes for common device types (mouse, keyboard, joystick, etc.). The Backend would further extend those device types to call SetState(IEnumerable<KeyValuePair<string, long>> on the base device. This is proper SOC, the Input library should not need to know how a particular backend queries its input devices, and the logical place for a backend to sit is outside the core NuGet so that other backend implementations are treated as first-class citizens in future.

A device state, at its core, is a snapshot dictionary of string->long. From my own work implementing HID devices, this is a good compromise as it supports just about any real data. Where a floating-point type is needed this can be achieved using blitting or similar. How the long is mapped to the actual value is handled by the specific device class.

The beauty of having a common, centralised, state system is that it is significantly easier to monitor all devices concurrently. It is even possible to allow a game developer to bind to an unknown device's inputs by creatively using the friendly names provided.

Devices can extend state functionality to make it more intelligible to end-users, which the MouseDevice illustrates. The input device system tracks the addition/removal of state keys, as well as the change in their values. This allows a keyboard device to indicate a key press by adding/removing the state, but may key the modifier key states around permanently; or, as per the example, a mouse can have an X and Y state permanently available for its state.

By using the abstract device implementations for common devices (like mouse/keyboard), a backend only has to implement the minimum amount of code to call SetState on its devices whenever a state change occurs. This can happen asynchronously to the input system. The state is only exposed to the end-user as a snapshot.

There is no need for code generation, in this model Window would reference Input, or an Input.Window shim can be used; but there is no need to use the Input model if unwanted. Further, the code can be optimised so that event concatenation only occurs where events are subscribed to (this would have complicated the example so isn't implemented yet), which means that it has no overhead if not initialised.

[HurricanKai]

I really like the idea of the device just holding the state, the backend updating that state as needed and then the user just querying that state.
Not sure how I feel about consolidating Window and Input into one package, I mean given we already consolidate extensions and core packages into one package in 3.0, and therefore already rely on .NET 6 linking a lot it could be worthwhile and would simplify stuff a lot.
This idea would also tie in very well into the new ISurface APIs, so that is a plus point.

[Perksey]

Windowing and Input I want to keep separate. I will write up the rest of my opinions later.

[thargy]

Worth noting that, at the moment, the design only uses Context as a convenient Backend grouper. You could drop Context entirely and have the FindDevice/GetState methods implemented as extension methods on a single and/or multiple backends if you're not to bothered about grouping the events. This makes it even easier to use in the common case that you just want a singular backend to supply you with devices.

In terms of keeping input out of windowing, then the normal approach is the shim NuGet approach. This is what logging libraries usually do (amongst many others). It does create NuGet bloat, but it's not a massive deal. That's still preferable to magic code injection (IMHO) which is non-intuitive to a consumer/contributor. Placing Window logic in Input is also worrisome as it's effectively backwards and makes Window input behave differently to a 3rd-party backend.

[Perksey]

In terms of keeping input out of windowing, then the normal approach is the shim NuGet approach. This is what logging libraries usually do (amongst many others). It does create NuGet bloat, but it's not a massive deal. That's still preferable to magic code injection (IMHO) which is non-intuitive to a consumer/contributor. Placing Window logic in Input is also worrisome as it's effectively backwards and makes Window input behave differently to a 3rd-party backend.

Agree, I think a Silk.NET.Input.Abstractions (not using .Common to avoid confusion) and Silk.NET.Input.Windowing (maybe not that name?) split is as good as we can get without source generators, and is something I can deal with as much as I hate introducing packages.

Abridged proposal containing only surface API

// Prototype implementation
// Note - thread-safety and performance are not implemented.
namespace Silk.NET.Input
{
    /// <summary>
    /// Represents an input backend from which input devices can be retrieved.
    /// </summary>
    /// <remarks>
    /// This interface is to be implemented by backend libraries, e.g. Windowing.
    /// </remarks>
    public interface IInputBackend
    {
        /// <summary>
        /// Collection of all devices, supplied by this backend.
        /// </summary>
        IReadOnlyList<InputDevice> Devices { get; }

        /// <summary>
        /// Raised whenever devices are added to the backend.
        /// </summary>
        event Action<IReadOnlyList<InputDevice>>? DevicesAdded;

        /// <summary>
        /// Raised whenever devices are removed from the backend.
        /// </summary>
        event Action<IReadOnlyList<InputDevice>>? DevicesRemoved;
    }

    /// <summary>
    /// Groups together all <see cref="IInputBackend">backends</see> into a single context.
    /// </summary>
    public class InputContext : IDisposable
    {
        public IReadOnlyList<IInputBackend> Backends { get; }

        /// <summary>
        /// Returns all devices from all backends.
        /// </summary>
        public IReadOnlyList<InputDevice> AllDevices { get; }

        /// <summary>
        /// Gets all devices of the given type.
        /// </summary>
        /// <remarks>
        /// The <typeparamref name="T" /> type parameter must be the last interface in the inheritance heirarchy (the "device type")
        /// i.e. it must be a device type like <see cref="IJoystick" /> and not something like <see cref="IInputDevice" />.
        /// The behaviour of using a T that isn't a device type recognised by the backend is undefined.<br />
        /// <br />
        /// This method is not intended for general consumption. Consider <see cref="InputContext" /> instead.
        /// </remarks>
        public IReadOnlyList<T> GetDevices<T>() where T : InputDevice;

        /// <summary>
        /// Find the first device that matches the optional filter criteria.
        /// </summary>
        /// <param name="isConnected">Whether the device must be connected.</param>
        /// <param name="backend">The backend.</param>
        /// <param name="deviceId">The device ID.</param>
        /// <param name="name">The device name.</param>
        /// <param name="comparisonType">The string comparison type to use for device names</param>
        /// <typeparam name="T">The device type.</typeparam>
        /// <returns></returns>
        public T? FindDevice<T>(
            bool? isConnected = null,
            IInputBackend? backend = null,
            int? deviceId = null,
            string? name = null,
            StringComparison comparisonType = StringComparison.Ordinal) where T : InputDevice;

        /// <summary>
        /// Gets the state of the first connected device that matches the optional filter criteria.
        /// </summary>
        /// <param name="backend">The backend.</param>
        /// <param name="deviceId">The device ID.</param>
        /// <param name="deviceType">The device type.</param>
        /// <param name="name">The device name.</param>
        /// <param name="comparisonType">The string comparison type to use for device names</param>
        /// <typeparam name="T">The device type.</typeparam>
        /// <returns></returns>
        public T? GetState<T>(
            IInputBackend? backend = null,
            int? deviceId = null,
            Type? deviceType = null,
            string? name = null,
            StringComparison comparisonType = StringComparison.Ordinal)
            where T : InputDeviceState;

        public InputContext();

        /// <summary>
        /// Initializes a new context with the provided backends.
        /// </summary>
        /// <param name="backends">The backends.</param>
        public InputContext(params IInputBackend[] backends);

        /// <summary>
        /// Initializes a new context with the provided backends.
        /// </summary>
        /// <param name="backends">The backends.</param>
        public InputContext(IEnumerable<IInputBackend> backends);

        /// <summary>
        /// Adds the <paramref name="backends"/> to this context.
        /// </summary>
        /// <param name="backends">The backends to add</param>
        /// <returns><see langword="true"/> if any backends were added; <see langword="false"/> otherwise.</returns>
        public bool Add(params IInputBackend[] backends) => Add((IEnumerable<IInputBackend>) backends);

        /// <summary>
        /// Adds the <paramref name="backends"/> to this context.
        /// </summary>
        /// <param name="backends">The backends to add</param>
        /// <returns><see langword="true"/> if any backends were added; <see langword="false"/> otherwise.</returns>
        public bool Add(IEnumerable<IInputBackend> backends);

        /// <summary>
        /// Remove the <paramref name="backends"/> from this context.
        /// </summary>
        /// <param name="backends">The backends to remove</param>
        /// <returns><see langword="true"/> if any backends were removed; <see langword="false"/> otherwise.</returns>
        public bool Remove(params IInputBackend[] backends);

        /// <summary>
        /// Remove the <paramref name="backends"/> from this context.
        /// </summary>
        /// <param name="backends">The backends to remove</param>
        /// <returns><see langword="true"/> if any backends were removed; <see langword="false"/> otherwise.</returns>
        public bool Remove(IEnumerable<IInputBackend> backends);

        /// <summary>
        /// Removes all backends from the context.
        /// </summary>
        /// <returns><see langword="true"/> if any backends were removed; <see langword="false"/> otherwise.</returns>
        public bool Clear();

        /// <summary>
        /// Attaches to backend events
        /// </summary>
        /// <param name="backends">Enumeration of backends</param>
        protected virtual void Attach(IEnumerable<IInputBackend> backends);

        /// <summary>
        /// Detaches from backend events
        /// </summary>
        /// <param name="backends">Enumeration of backends</param>
        protected virtual void Detach(IEnumerable<IInputBackend> backends);

        /// <summary>
        /// Called whenever devices are added.
        /// </summary>
        /// <param name="devices"></param>
        protected virtual void OnDevicesAdded(IReadOnlyList<InputDevice> devices);

        /// <summary>
        /// Called whenever devices are removed.
        /// </summary>
        /// <param name="devices"></param>
        protected virtual void OnDevicesRemoved(IReadOnlyList<InputDevice> devices);

        /// <summary>
        /// Called whenever a device's state is changed. 
        /// </summary>
        /// <param name="stateChanges"></param>
        protected virtual void OnStateChange(InputDevice device,
            IReadOnlyCollection<InputDeviceStateChange> stateChanges)
            => StateChanges?.Invoke(device, stateChanges);

        /// <summary>
        /// Raised whenever devices are added to the context.
        /// </summary>
        public event Action<IReadOnlyList<InputDevice>>? DevicesAdded;

        /// <summary>
        /// Raised whenever devices are removed from the context.
        /// </summary>
        public event Action<IReadOnlyList<InputDevice>>? DevicesRemoved;

        /// <summary>
        /// Fired whenever any state changes occur.
        /// </summary>
        private event Action<InputDevice, IReadOnlyCollection<InputDeviceStateChange>>? StateChanges;

        /// <summary>
        /// Disposes this context, detaching from all backends.
        /// </summary>
        public void Dispose();
    }

    /// <summary>
    /// Base class for all input devices, is provided by an <see cref="IInputBackend"/> and holds a current state.
    /// </summary>
    /// <remarks>
    /// <para>The device's current state is represented as a dictionary of state keys with long values.</para>
    /// </remarks>
    public abstract class InputDevice
    {
        /// <summary>
        /// Creates a new input device
        /// </summary>
        /// <param name="backend">The backend.</param>
        /// <param name="deviceId">The device ID.</param>
        /// <param name="name">An optional friendly name.</param>
        /// <param name="initialState">The initial state.</param>
        public InputDevice(IInputBackend backend, int deviceId, string? name = null,
            IEnumerable<KeyValuePair<string, long>>? initialState = null);

        /// <summary>
        /// Whether the device is currently connected
        /// </summary>
        /// <remarks>
        /// You should not hold on to a disconnected device.</remarks>
        public bool IsConnected { get; protected set; }

        /// <summary>
        /// The backend that supplies this device
        /// </summary>
        public IInputBackend Backend { get; init; }

        /// <summary>
        /// The backend-specific, device-type-specific identifier for this device.
        /// </summary>
        /// <remarks>
        /// For example, no gamepad may share a DeviceId with another gamepad (same applies for all other device types) on that individual backend.
        /// This property should not be used as a globally unique identifier.
        /// 1 2 3 4 5
        /// 1 3 
        /// 5
        /// </remarks>
        public int DeviceId { get; init; }

        /// <summary>
        /// An optional device friendly name (e.g. "Mouse", "Keyboard", does not need to be unique), this often comes from the USB identifier string.
        /// </summary>
        public string Name { get; init; }

        /// <summary>
        /// Fired whenever any state changes occur.
        /// </summary>
        public event Action<InputDevice, IReadOnlyCollection<InputDeviceStateChange>>? StateChanges;

        /// <summary>
        /// Updates the current state.
        /// </summary>
        /// <param name="state">The new state keys.</param>
        /// <returns><see langword="true"/> if any state change was detected; <see langword="false"/> otherwise.</returns>
        protected IReadOnlyList<InputDeviceStateChange> SetState(IEnumerable<KeyValuePair<string, long>> state);

        protected virtual void OnStateChanges(IReadOnlyList<InputDeviceStateChange> changes);

        /// <summary>
        /// Gets a snapshot of the current device state.
        /// </summary>
        public InputDeviceState State => new InputDeviceState(this, _state);
    }

    /// <summary>
    /// A snapshot of an input device's current state
    /// </summary>
    public class InputDeviceState : IReadOnlyDictionary<string, long>
    {
        public InputDevice Device { get; init; }

        protected internal InputDeviceState(InputDevice device, IReadOnlyDictionary<string, long> state);

        /// <summary>
        /// Whether the device was connected when the state was snapshot. 
        /// </summary>
        public bool WasConnected => _state.TryGetValue(StateKey_Connected, out var value) && value != 0L;
    }

    /// <summary>
    /// A simple generic input device.
    /// </summary>
    /// <remarks>
    /// This is best used for testing purposes, in practice a backend should implement a concrete type that provides
    /// functionality to interpret it's state more readily.
    /// </remarks>
    public class GenericInputDevice : InputDevice
    {
        public GenericInputDevice(IInputBackend backend, int deviceId, string? name = null,
            IEnumerable<KeyValuePair<string, long>>? initialState = null)
            : base(backend, deviceId, name, initialState);

        protected IReadOnlyList<InputDeviceStateChange> Update(IEnumerable<KeyValuePair<string, long>> state)
            => SetState(state);
    }

    /// <summary>
    /// Interface for state changes on a device, lists state keys that have changed.
    /// </summary>
    public readonly struct InputDeviceStateChange
    {
        /// <summary>
        /// The state key that changed.
        /// </summary>
        public readonly string Key;

        /// <summary>
        /// The old state value (if any); otherwise <see langword = "null"/> indicates the state was added.
        /// </summary>
        public readonly long? OldValue;

        /// <summary>
        /// The new state value (if any); otherwise <see langword = "null"/> indicates the state was removed.
        /// </summary>
        public readonly long? NewValue;

        public InputDeviceStateChange(string key, long? oldValue, long? newValue);
    }

    /***
     * OPTIONAL: Input can provide some common 'strongly-typed' overloads for convenience. i.e. mouse, keyboard,
     * joystick, etc.  
     *
     * These would still be implemented by the backend, but would be easier to implement
     *
     * This example shows a generic mouse implementation, with state keys for X,Y and buttons.
     *
     * A keyboard would have states for modifiers, and would add/remove state keys for each key pressed. (The state
     * system tracks addition/removal of keys too, so keys can be 'permanent' or 'transient').
     *
     * Note that transitions to/from states are supported so it is possible to see the current state as well
     * as use events to detect transitions.  Handling key auto-repeats however is more complex, but can still be done
     * by the device implementation for a generic keyboard by triggering an additional event every x-ms whilst a
     * key is held down. 
     ***/
    
    /// <summary>
    /// Generic mouse device implementation
    /// </summary>
    public abstract class MouseDevice : InputDevice
    {
        protected MouseDevice(
            IInputBackend backend,
            int deviceId,
            string? name = null,
            IEnumerable<KeyValuePair<string, long>>? initialState = null)
            : base(backend, deviceId, name ?? "Generic mouse", initialState);

        protected override void OnStateChanges(IReadOnlyList<InputDeviceStateChange> changes);

        /// <summary>
        /// Get's the latest mouse state.
        /// </summary>
        public new MouseState State => new MouseState(this, base.State);

        public event Action<MouseMoveEvent> MouseMove;
        public event Action<MouseButtonEvent> ButtonsChanged;
    }

    public class MouseState : InputDeviceState
    {
        public MouseDevice Device { get; init; }

        // State encapsulation field
        private IReadOnlyDictionary<string, long> _state;

        public MouseState(MouseDevice device, IReadOnlyDictionary<string, long> state)
            : base(device, state)
        {
        }

        #region Mouse State helpers

        public long X => TryGetValue(StateKey_X, out var value) ? value : 0L;

        public long Y => TryGetValue(StateKey_Y, out var value) ? value : 0L;

        public bool LeftClicked => TryGetValue(StateKey_Left_Click, out var value) && value != 0L;

        public bool RightClicked => TryGetValue(StateKey_Right_Click, out var value) && value != 0L;

        public bool MiddleClicked => TryGetValue(StateKey_Middle_Click, out var value) && value != 0L;

        public bool ButtonClicked(int number) =>
            TryGetValue($"{StateKey_Button_Prefix} {number}", out var value) && value != 0L;

        #endregion
    }

    public readonly struct MouseMoveEvent
    {
        public readonly MouseDevice Device;
        public readonly Vector2 Position;
        public readonly Vector2 Delta;

        public MouseMoveEvent(MouseDevice device, InputDeviceStateChange? xChange, InputDeviceStateChange? yChange)
        {
            Device = device;
            Position = new Vector2(xChange?.NewValue ?? 0L, yChange?.NewValue ?? 0L);
            Delta = new Vector2(Position.X - (xChange?.OldValue ?? 0L),
                Position.Y - (yChange?.OldValue ?? 0L));
        }
    }

    public readonly struct MouseButtonEvent
    {
        public readonly MouseDevice Device;

        /// <summary>
        /// Button indices that changed. <see langword="true"/> if button was pressed; <see langword="false"/>
        /// otherwise.
        /// </summary>
        private readonly IReadOnlyDictionary<int, bool> _buttonChanges;

        public MouseButtonEvent(MouseDevice device, IEnumerable<InputDeviceStateChange> buttonChanges)
        {
            Device = device;
            var changes = new Dictionary<int, bool>();
            foreach (var change in buttonChanges)
            {
                // Get the button number from the key
                if (!int.TryParse(change.Key[MouseState.StateKey_Button_Prefix.Length..], out var index))
                    continue;

                var oldValue = change.OldValue ?? 0L;
                var newValue = change.NewValue ?? 0L;

                // Sanity check
                if (oldValue == newValue) continue;

                changes[index] = newValue != 0L;
            }

            _buttonChanges = changes;
        }

        public bool LeftClicked => _buttonChanges.TryGetValue(MouseState.Button_Left, out var value) && value;

        public bool RightClicked => _buttonChanges.TryGetValue(MouseState.Button_Right, out var value) && value;

        public bool MiddleClicked => _buttonChanges.TryGetValue(MouseState.Button_Middle, out var value) && value;
        public bool LeftReleased => _buttonChanges.TryGetValue(MouseState.Button_Left, out var value) && !value;

        public bool RightReleased => _buttonChanges.TryGetValue(MouseState.Button_Right, out var value) && !value;

        public bool MiddleReleased => _buttonChanges.TryGetValue(MouseState.Button_Middle, out var value) && !value;
        
        public bool ButtonClicked(int number) => _buttonChanges.TryGetValue(number, out var value) && value;
        public bool ButtonReleased(int number) => _buttonChanges.TryGetValue(number, out var value) && !value;
    }
    
    /*
     * Example backend implementation, this is dumb and just creates a single mouse device.
     * 
     */
    public class Window : IInputBackend
    {
        private readonly List<InputDevice> _devices;

        public Window()
        {
            _devices = new List<InputDevice> {new WindowMouseDevice(this, 0)};
        }

        public IReadOnlyList<InputDevice> Devices => _devices;
        public event Action<IReadOnlyList<InputDevice>>? DevicesAdded;
        public event Action<IReadOnlyList<InputDevice>>? DevicesRemoved;
    }
}

}

Most of my irks were with the implementation rather than the API, so I've ignored that in the name of the proposal.

• I agree with the philosophy of the proposal, as well as the surface API of the state, device, and context classes, except:
  • Why all the dictionary stuff? We don't need this, we can just add more fields to the structs - no need to abstract away the state storage. This is thanks to (from the SDP):

  Users are expected to keep all of the versions of all Silk.NET packages they are using in-sync.

  • Where are MouseDown and friends? Can we replace the generic event on InputContext with those specialized events? Also I'm not sure about the whole generic event thing, maybe something like bool InputDevice.Subscribe<TState>(Action<TState> subscriber); and a matching Unsubscribe method would make sense so we can receive MouseButtonEvent (from the Enhanced Input Events proposal) directly from the backend.
  • I don't think InputContext should be abstract, in fact my gut feel is that it should be sealed.
  • But yeah TLDR I love this, just the "state" and "events" are a bit too abstracted away
• Naming is top notch as it definitely conveys that state represents the mouse at any point in time

[thargy]

Most of my irks were with the implementation rather than the API, so I've ignored that in the name of the proposal.

Yeah, this was thrown together to make the API compile and demonstrate the concepts a little more readily. It's not production ready.

• Why all the dictionary stuff? We don't need this, we can just add more fields to the structs - no need to abstract away the state storage. This is thanks to (from the SDP):

By having state represented as a simple Dictionary it is using a simple property-bag model - this is something I've come across repeatedly and it has a number of advantages over using unknowable extensible structs. This allows the code in the abstract device base class to handle state change logic (effectively diff'ing), event propagation and thread-synchronisation logic (not shown in the example), for all backends without backends needing to re-invent the wheel. Further, diffing is faster (you can do it with unknown structs using reflection, but that has a start-up performance at least and requires a lot of code to create compilable lambdas). It allows that without the input engine having any idea what a backend actually does.

• Where are MouseDown and friends?

MouseDevice.ButtonsChanged is fired whenever the mouse button states changes. The MouseButtonEvent contains all the buttons that changed state. Unlike the other API, this API is heavily focussed on batching events for performance, whenever possible. As multiple buttons can technically change state at the same time (due to the way USB, or a polling backend may work, and due to a single SetState call) then we only want to throw one event when the state changes. For example, if someone clicks both Left and Right mouse buttons, and releases Button 5 (on a multi-button mouse) then MouseButtonEvent._buttonChanges (which should probably be exposed through a public read-only property) contains ([1,true],[2,true],[5,false]), as such calling LeftClicked, RightClicked and ButtonReleased(5) will all return true. The properties can be renamed LeftDown, LeftUp, etc. if you prefer. Importantly, there is no limit to how many buttons the mouse can expose, but we name the most common one for convenience (under the hood they are numbered though).

Can we replace the generic event on InputContext with those specialized events?

The generic event is called for all state changes because we know about all state changes at the base. This is convenient for logging and debugging as it allows you to see changes being raised by devices you're learning about, also useful during backend development, as you can quickly throw something together before adding all the extra event and state wrappers. However, descendent Devices can (and should) add more specialised events that are more relevant, at no additional performance cost (I won't go into it now, but you can reduce the performance impact by only doing stuff when events are listened to, I didn't implement that for clarity). The MouseDevice demonstrates this with its two additional example events:

        public event Action<MouseMoveEvent> MouseMove;
        public event Action<MouseButtonEvent> ButtonsChanged;

This approach gives you the best of both worlds IMHO.

Also I'm not sure about the whole generic event thing, maybe something like bool InputDevice.Subscribe<TState>(Action<TState> subscriber); and a matching Unsubscribe method would make sense so we can receive MouseButtonEvent (from the Enhanced Input Events proposal) directly from the backend.

Not quite sure where you're going with that one (look a bit RX like). Under this proposal the backend only needs to override one of the standard abstract devices (like MouseDevice, KeyboardDevice) and implement calls to SetState and be done, removing the majority of boilerplate. Where a backend wants to introduce an entirely new kind of device, then they have the ability to do that, but it won't be that common, as the end consumer would need to also understand that new device type to get the most benefit.

Another reason for having the base StateChanges event is that it allows a backend to expose other state changes beyond the 'norm'. For example, let's say we have a special gaming keyboard that allows you to set its backlight RGB. We might create a backend that knows about it and extends the KeyboardDevice from the Input Nuget (not shown yet!) with a BacklitKeyboardDevice, which has the additional members:

	public class BacklitKeyboardDevice : KeyboardDevice
	{
		...
        public event Action<BackLightEvent> BacklightChanged;
		public void SetBacklight(Vector3 color) { ... }
	}


    public class BacklitKeyboardState : KeyboardDeviceState
    {
        #region State Keys used by Device

        public const string StateKey_BacklightColor = "BacklightRGB";

        #endregion

        public BacklitKeyboardDevice Device { get; init; }

...

        #region Mouse State helpers

        public Vector3 Color => TryGetValue(StateKey_BacklightColor, out var value) ? new Vector3(/* Convert long */...) : Vector3.Zero;

        #endregion
    }

	public readonly struct BackLightEvent
	{
            public readonly BacklitKeyboardDevice Device;
            public readonly Vector3 OldColor;
            public readonly Vector2 NewColor;

			...
	}

The beauty of this is that any code written to expect just a KeyboardDevice will continue to work as normal. And if you have Debug code writing out events from the keyboard you will see state changes referring to StateKey_BacklightColor whenever the colour changes (or you grab a state snapsot). However, you can also then add code to make use of the actual BacklitKeyboardDevice which not only now exposes a specific event for backlight changes, but gives you a method to call to actually change the backlight. Note that calling the method would send a message to the keyboard to make the change, and then, at a later time, the state would change to reflect the new color (easy asynchrony). This was something we discussed in the working group - with a MouseDevice (being able to 'set' the cursor position), though I didn't show that in the example implementation for brevity. (In general, methods that can change a device should always be method calls, and we should resist them as this is an 'input' library).

• I don't think InputContext should be abstract, in fact my gut feel is that it should be sealed.

Yeah, that was a toss-up, I made it abstract as it's somewhat limited usefulness at the moment, it merely groups together backends. However, being a backend grouper then it's also hard to see the use case for a backend extending it...

[Perksey]

By having state represented as a simple Dictionary it is using a simple property-bag model - this is something I've come across repeatedly and it has a number of advantages over using unknowable extensible structs.

This is just a struct wrapper around a dictionary though, which I don't think is needed. Because it's a struct, it isn't really any more extensible than if they were regular fields (and I'm not saying make it more extensible, I'm saying we don't need this).

Dictionaries will lead user code to have to do roundabout hash table lookup in use, something which is an undesirable expense given this will likely be used on a hot path. The best summary of my train of thought is that I just don't want a bag, I want pigeon holes where everything has its place fixed. The dictionaries make me uncomfortable.

For diffing I think we should do this manually or if this proves to be too much of a small make a private (i.e. not seen by the end user) source generator which does this for us. If we really wanted to get performance nuts about this, more theoretical performance is possible by doing it this way if we wanted to use intrinsics in the diffs (but I doubt we'd need to)

Unlike the other API, this API is heavily focussed on batching events for performance, whenever possible.

This will decrease the number of callvirts but I think the performance difference overall will be immeasurable or worse. If we needed to batch events, which I don't think is a bad idea, we can investigate altering Enhanced Input Events such that the actual button data is a Span<T> where T is button-specific data, the event structures ref structs, and the delegates named to get around the ref struct generics restrictions.

MouseDevice.ButtonsChanged is fired whenever the mouse button states changes.

Unified mouse events were rejected in the initial drafts of Enhanced Input Events, so I'd rather not do that. It's going out of our way to remove all familiarity for little benefit, and it makes it awkward to use. I'd imagine most of our users don't even care about changing the state, they just want to know when things happen and having dedicated events with the event structs proposed in Enhanced Input Events exposes this in a clean way.

Yeah, that was a toss-up, I made it abstract as it's somewhat limited usefulness at the moment, it merely groups together backends. However, being a backend grouper then it's also hard to see the use case for a backend extending it...

Eeeeeeeehhhhhhhhhhhhhh come back to me.

Not quite sure where you're going with that one (look a bit RX like)

The idea is that the Input Context subscribes to a specific kind of state change individually, and routes this to the events it exposes.

[Perksey]

I think the main thing is the new events are too fluid. I would rather have rigid events with a fluid way of subscribing to them rather than fluid events with a rigid way of subscribing to them. This would what my proposed Subscribe and Unsubscribe would effectively do. The state should be rigidly structured and rigidly defined.

[thargy]

I've gone down the rabbit hole before of building type differs. In fact, I've gone back and forth between dictionaries and type members (using reflection or T4 source gen) so many times, I've lost count (check out the horrifying decade long history on the core libraries, which I wrote most of between 2007-2018, for just a few examples of the back and forth).

It has therefore been my bitter experience that, as the adage goes, performance is an engineering problem, not a design one. That is you shouldn't optimise it unless you prove you have too. I have used dictionaries on hot paths of very large scale applications, and they are fast (being hashtable lookups). But I accept it involves heap allocations, etc. Either way, using source gen or reflection both require a lot more work, so you really should establish the problem exists before exploring that path. At least that's been my experience.

In a similar way, event batching just makes sense with a batch state setter, and though event invocation has improved over the years (compared to the bad ol' days) it's still best minimised. Ultimately, events are multicast delegates and are therefore slower than simple method invocations.

In terms of rejecting unified mouse events, I think it's more than that. A mouse, like a keyboard, is capable of reporting multiple buttons being pressed/released at the same time, having them grouped makes it much easier to code for detecting, for example, left & right mouse buttons pressed together; just as you would do with multiple keys (which I know was a criticism levelled against the old API).

However, I'm not precious, I wouldn't use the event interface in practice anyway, preferring to grab the state on each game loop, but it's there for those who prevent event-driven programming. But, the more events exposed means the more checks and invocations required every time a state changes, I see that as a bigger overhead than dictionary lookups.

I'm really not sure what you're getting at with the Subscribe/Unsubscribe proposal. It looks, exactly like a slower re-implementation of multicast delegates/events by another name? As for adding concrete events (like MouseMove) to the Context, that blocks extensibility and future-proofing as you can't add additional ones in future when implementing a new Device (like the BaclitKeyboard example I gave).

I'd try to implement what you're suggesting, but I don't think I've followed you properly, could you work up a practical example?

[Perksey]

performance is an engineering problem, not a design one.

Right but I don't think we should allow ourselves to potentially be bottlenecked by a design which, if approved now, we'd be locked into until 2023 when Silk.NET 4.0 releases. By the time we've proven that this is a problem, there's nothing we can do about it because the dictionary stuff is part of the public API. At least if it were an implementation detail we can choose not to use it. Moreover it's our job to think of performance as much as possible, as Silk.NET can never afford to be even remotely a bottleneck.

they are fast (being hashtable lookups)

Not as fast as regular field retrievals.

Ultimately, events are multicast delegates and are therefore slower than simple method invocations.

They're not that much slower on a hot path. Most modern CPUs will cache the VTable resolved function's instructions if called lots (within reason), and this is why our indirect calls don't prove to be that slow in practice either.

I'm really not sure what you're getting at with the Subscribe/Unsubscribe proposal. It looks, exactly like a slower re-implementation of multicast delegates/events by another name?

Yes it's literally just to get around that we can't do

public event Action<T> StateChanged<T>;

I agree it's not great, but IInputBackend was never intended for general consumption - InputContext remains the main user-facing API.

I'd try to implement what you're suggesting, but I don't think I've followed you properly, could you work up a practical example?

Will have a play this afternoon.

As for adding concrete events (like MouseMove) to the Context, that blocks extensibility and future-proofing as you can't add additional ones in future when implementing a new Device (like the BaclitKeyboard example I gave).

Why would this be the case? We want to be able to change the Context class however frequently we want without affecting the backend.

[thargy]

performance is an engineering problem, not a design one.

Right but I don't think we should allow ourselves to potentially be bottlenecked by a design which, if approved now, we'd be locked into until 2023 when Silk.NET 4.0 releases. By the time we've proven that this is a problem, there's nothing we can do about it because the dictionary stuff is part of the public API. At least if it were an implementation detail we can choose not to use it. Moreover it's our job to think of performance as much as possible, as Silk.NET can never afford to be even remotely a bottleneck.

Agreed, sorry I meant that, for performance-critical scenarios, sometimes we need to prototype options and benchmark prior to settling the design, not that we develop and release a less performant version and wait for the next major milestone. You see this approach a lot in the dotnet team's own approach when debating designs of performance-critical code (I've had to submit BenchmarkDotNet samples, to show relative performance in terms of speed and allocs).

they are fast (being hashtable lookups)

Not as fast as regular field retrievals.

Yes, obviously not! However, the real-world impact still comes down to how often you do those ~5ns dictionary lookups each game loop compared to everything else you do, and what you gain/lose in return.

Ultimately, events are multicast delegates and are therefore slower than simple method invocations.

They're not that much slower on a hot path. Most modern CPUs will cache the VTable resolved function's instructions if called lots (within reason), and this is why our indirect calls don't prove to be that slow in practice either.

Agreed, this is the problem with performance during design is that these slower/not slower things are hard to predict the impact of without an actual real-world benchmark. I only said they were slower, not by how much. Indeed, even after years of writing performant code, I get surprised when something I thought would be slower is actually faster, etc. That's particularly true as the compiler team continue to optimise (cf the new record struct performance compared to plain old structs, they add cool functionality and are faster). For example, we used to have to avoid Enum.HasFlag like the plague, until the compiler team made it as fast (in some cases faster) than hand-coding the logic, and then we had to go back and rewrite.

I'm really not sure what you're getting at with the Subscribe/Unsubscribe proposal. It looks, exactly like a slower re-implementation of multicast delegates/events by another name?

Yes it's literally just to get around that we can't do

public event Action<T> StateChanged<T>;

I agree it's not great, but IInputBackend was never intended for general consumption - InputContext remains the main user-facing API.

I'd try to implement what you're suggesting, but I don't think I've followed you properly, could you work up a practical example?

Will have a play this afternoon.

As for adding concrete events (like MouseMove) to the Context, that blocks extensibility and future-proofing as you can't add additional ones in future when implementing a new Device (like the BaclitKeyboard example I gave).

Why would this be the case? We want to be able to change the Context class however frequently we want without affecting the backend.

Ah! 💡

So in my proposal the separation of concerns is this:

Class	Role	Location
IInputBackend	Requires a backend to provide a collection of devices and notify changes	Input
InputContext	Groups backends for accessing all backends and devices together, grouping device attach/detach events, and all device events, if desired, which is useful for logging.	Input
InputDevice	Base class of all devices, hold device state in a generic way, detects changes and raises an event indicating what has changed and how	Input
InputDeviceState	Base class of all device state snapshots giving 'friendly' access to state	Input
[Mouse|Keyboard|Gamepad|Joystick|...]Device	Extends InputDevice and provides a common surface area for an abstract version of a device type, but doesn't actually implement the device fully.	Input
[Mouse|Keyboard|Gamepad|Joystick|...]State	Extends InputDeviceState and provides relevant state accessors for that abstract device type	Input
[Mouse|Keyboard|Gamepad|Joystick|...]Event	Creates event implementations for that abstract device type	Input
*Device	Concrete device implementation that can extend InputDevice directly if a unique device type, or one of the provided [Mouse|Keyboard|Gamepad|Joystick|...]Device. It's main purpose is to call SetState on the base classes whenever a device update occurs, and/or implement any abstract base methods that are for setting device state (rare as discussed above). Where no new functionality is added these may be internal, leaving only the abstract common base class exposed (e.g. MouseDevice)	Backend
*State	Optional concrete device state implementation extends InputDeviceState and provides relevant state accessors for that device type (normally only needed for a unique device type)	Backend
*Event	Optional concrete event implementations for that device type (normally only needed for a unique device type)	Backend
*Backend	Concrete backend implementation provides it's specific devices	Backend

In the proposal:

• A 'device' is a provider of device-specific functionality (a mouse, a keyboard, etc.), only a mouse device would expose a MouseMove event, which makes no sense for a keyboard, etc. The full implementation belongs in a backend (which may be 3rd-party and unknown to us). MouseDevice (et al) are partial implementations to provide a common surface area so that any consumer of our library can make use of mouse-like devices without necessarily caring or worrying about the device's 'full' functionality as provided by the backend. The BaclkitKeyboardDevice example illustrates this, you can drop in the backend that supports it and existing code will continue to treat it like a KeyboardDevice, however, if you care about the extra functionality you can check for the BaclkitKeyboardDevice and use it explicitly. NB The input library itself has no concrete implementations of any device.
• A 'backend' is a provider of supported devices (e.g. a 'Windowing' backend may supply mouse, keyboard, gamepad and joystick implementations), it provides an implementation of IInputBackend, and concrete implementations for the devices it supports.
• A 'context' groups backends together, allowing it to expose all devices from all backends in one place. This makes finding devices easier.

To use the system you add backends to a context then query the context for the device you want. You can ask for a MouseDevice and you will get any concrete implementation that extends a MouseDevice, or you can ask for a specific concrete implementation like BacklitKeyboardDevice, etc. Once you have a device, you can access its unique events (like MouseMove on a MouseDevice).

When I say this design is future proof, consider the following example. A 3rd party consumer wants to make use of our library, but has a custom device they want to work with; that device is a gaming keyboard with a backlight. They don't want to re-implement the wheel for gaming keyboards, and in fact, want their gaming keyboard to be useable by existing games with minimal effort. As such, they implement a new backend (IInputBackend) that provides a new BacklitKeyboardDevice that extends KeyboardDevice. They only need to add the extra functionality for controlling and monitoring the state of the backlight (see example above), which is trivial, and providing the state information for their keyboard (i.e. which keys/modifiers are pressed) via SetState. In their own code, they request a BacklitKeyboardDevice and can make use of the existing and new functionality.

They don't have to worry about the state thread-safety, or change tracking and event propagation, the InputDevice base class has done all that work for them already. Nor do they have to request we add something to InputContext to make it work. For this reason, a concrete event like MouseMove or BacklightChanged can't be exposed directly on InputContext as to do so would require all supported devices to be known when we build InputContext (the opposite of future proof).

Your 'Subscribe''Unsubscribe' model, however, does allow you a way to get close whilst remaining future-proof. If a device, instead of exposing events, calls a generic base method on InputDevice with a generic 'event payload', then InputDevice can raise those 'events' using the Subscribe\Unsubscribe methodology, these can then be subscribed to by the InputContext and exposed there. However, that's exactly what IObservable<T> is for, along with Rx (Reactive Extensions). It is what I use in my own HID library for that reason, but if you're going for hyper-performance (Rx has overhead, which is something I'm intimately familiar with), then implementing a Subsribe\Unsubscribe model will require dictionaries and lookups, etc.

Based on SOLID, the 'context' should not contain events like MouseMove, only a Mouse device should. The StateChanges event I exposed there is SOLID as a context can know that a device has changed state (by subscribing to it) even if it doesn't understand the state change itself, it is generic (like your proposal) and abstract, and so is primarily only useful for logging/debugging. It is low overhead being an event. I don't think we should be worried about the 'inconvenience' of getting the kind of device you want and then getting its events.

However, if you are concerned about, for example, getting all keyboard events from multiple keyboards, then this is where my proposal is that we forget about 'context' and instead think more about device grouping. My current proposal treats context as a device group of all devices provided by a group of backends, that concept can be extended to a group by type (all keyboards from all backends), etc. There are complexities though, about how you group together state (effectively overwriting in specified priority order). But that's a point for a broader discussion and breaks KISS.

Usually, a consumer wants to keep separate events from individual devices (e.g. gamepads) not group them all together. As such, the proposed design of finding a connected device instance from a group of backends and then querying it about its state (or subscribing to its events) seems like the right approach. Indeed I have no objection to dropping the StateChanges event from InputContext entirely if its objectionable, it's only there because it's essentially free and readily available (and useful for debugging/logging as mentioned).