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

Missing keyboard input functionality #399

Closed
ThomasMiz opened this issue Jan 4, 2021 · 20 comments
Closed

Missing keyboard input functionality #399

ThomasMiz opened this issue Jan 4, 2021 · 20 comments
Labels
area-Input enhancement New feature or request

Comments

@ThomasMiz
Copy link
Contributor

There currently doesn't appear to be a way to properly detect some keys for keyboard typing input.

The Keyboard.KeyChar provides the characters typed, and keys such as Enter, Backspace, Tab, Delete can be detected via KeyDown. However, if the user is pressing down on the backspace key, the only thing we can register with this system is a single backspace press, and no repeats.

Looking at the GLFW input guide, there aren't just "key down" and "key up" actions, but there's also a "key repeat" action which we're probably ignoring.

We should try to present all these parameters in our KeyChar, KeyDown and KeyUp events.

@ThomasMiz ThomasMiz added the enhancement New feature or request label Jan 4, 2021
@SanFlan
Copy link

SanFlan commented Jan 4, 2021

I'm not that familiarized with SILK but it seems that the event is being purposely ignored in GlfwKeyboard.cs, line 49

        _handle = events.Handle;
        events.Char += _char = (_, c) => KeyChar?.Invoke(this, (char) c);
        events.Key += _key = (_, key, code, action, mods) =>
            (action switch
            {
                InputAction.Press => KeyDown,
                InputAction.Release => KeyUp,
                InputAction.Repeat => null,
                _ => null
            })?.Invoke(this, ConvertKey(key), code);`

@ThomasMiz
Copy link
Contributor Author

I think it's a good idea to propose some changes to the input events so we can pass all these parameters to the user of the library.

We'll have to add more parameters to the KeyDown & other events, which will mean everyone will have to add them to their event handlers as well. It might be a good idea to do as OpenTK and aggregate all these parameters into a single struct called KeyEventArgs or something like that, which will be the sole parameter of these events:

public readonly struct KeyEventArgs
{
    public Key Key { get; }
    public int KeyCode { get; }
    public bool IsRepeat { get; }
    public ModifierKeys ModifiersKeys { get; }
    // And any other param which we might want to add now or in the future
}

This way we can also add more parameters in the future without making breaking changes.

We can invoke the "repeated" keys with the KeyDown event.

The other option would be to replace KeyDown and KeyUp with a single key event which reports all three up down and repeat, in which case instead of a bool IsRepeat the struct would have a KeyAction Action { get; }, but I like the previous approach more.

@HurricanKai
Copy link
Member

Sounds like a good idea to me - but not something we can do right now, this would have to be 3.0 work, which is faaar off.
For now, we could simply expose a new event Repeat that triggers when a repeat is triggered.
This isn't ideal from the user code side, but at least works.

@Perksey
Copy link
Member

Perksey commented Jan 5, 2021

We could just have a catch-all Key event.

event Action<KeyEvent> Key;

This would be non-breaking and could be accepted into 2.X.

@ThomasMiz
Copy link
Contributor Author

Just adding the Key event is an easy solution, but not a good solution.

What I suggest is a breaking change but previous code will be easy to adapt- All you have to do is change the event handler's parameters to a single one and then get the parameters from there. And it solves all these problems.

I think we should make these changes, if not for Silk 2.X then for Silk 3.0 and optionally, until then, we can add the Key event Perksey suggested.

@Perksey
Copy link
Member

Perksey commented Jan 5, 2021

We can't do breaking changes for 2.X so it'll have to be 3.0.

Silk.NET 3.0 has an official release date of December 31st 9999 but realistically won't be any time before 2022 (I'm not dedicating any time to new massive features for 3.0, only minor improvements, bug fixes, and the monthly updates).

@FrostByteGER
Copy link

We can't do breaking changes for 2.X so it'll have to be 3.0.

Silk.NET 3.0 has an official release date of December 31st 9999 but realistically won't be any time before 2022 (I'm not dedicating any time to new massive features for 3.0, only minor improvements, bug fixes, and the monthly updates).

So 3.0 will be more like a big maintenance release?

@ThomasMiz
Copy link
Contributor Author

I created a proposal for these changes. You can find it here.

@Perksey
Copy link
Member

Perksey commented Jan 6, 2021

So 3.0 will be more like a big maintenance release?

@FrostByteGER 3.0 has a tracker here: #209. It'll be added to as we further realise 3.0 but we haven't even started thinking about 3.0 yet. We're not in as much of a rush to get 3.0 out as we were 2.0.

@Perksey
Copy link
Member

Perksey commented Jan 12, 2021

@ThomasMiz if you can pull request this in like #397 we'll review it next working group meeting.

@MatijaBrown
Copy link
Contributor

There currently doesn't appear to be a way to properly detect some keys for keyboard typing input.

The Keyboard.KeyChar provides the characters typed, and keys such as Enter, Backspace, Tab, Delete can be detected via KeyDown. However, if the user is pressing down on the backspace key, the only thing we can register with this system is a single backspace press, and no repeats.

Looking at the GLFW input guide, there aren't just "key down" and "key up" actions, but there's also a "key repeat" action which we're probably ignoring.

We should try to present all these parameters in our KeyChar, KeyDown and KeyUp events.

If you want to do text input, you should use the char callback not the keydown event, according to GLFW. This also automatically handles key modifiers like AltGr+Q=@ and so on. Doesn't fix the backspace problem though.

@Perksey Perksey added this to the Future milestone Mar 5, 2021
@Perksey
Copy link
Member

Perksey commented Jan 10, 2023

This has been resolved in the 3.0 proposals #539

@Perksey Perksey closed this as completed Jan 10, 2023
@Pannoniae
Copy link

btw this is still a problem

@Beyley
Copy link
Contributor

Beyley commented Aug 5, 2024

btw this is still a problem

if you have an issue with the solution provided by the 3.0 proposals, please open a separate issue regarding any missing features from there

@Pannoniae
Copy link

no, I mean it's an issue right now, not in 3.0 when trying to use the library:) I'm literally repackaging Silk.NET.Input to add this functionality rn

@Perksey
Copy link
Member

Perksey commented Aug 5, 2024

Yes we’re aware it’s an issue right now, however the work specific to this issue is deemed complete for 3.0. Everything else will be tackled as part of normal 3.0 development. Very similar to how we treat issues in 2.X where we don’t close the issue until the work specific to the issue is in a core branch. We don’t keep it open until shipment because that would create a lot of maintenance overhead.

@Pannoniae
Copy link

Pannoniae commented Aug 5, 2024

Is it not possible to just add a keyrepeat event as a kludge, like right now? That won't break anything in terms of backward compatibility.

@Perksey
Copy link
Member

Perksey commented Aug 5, 2024

Any modification of the API contract will be breaking.

@Pannoniae
Copy link

How does adding an event break the API? I don't understand

@Perksey
Copy link
Member

Perksey commented Aug 5, 2024

Any third party implementation of IKeyboard would be incompatible with the update unless it also adjusts its code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Input enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants