-
Notifications
You must be signed in to change notification settings - Fork 627
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
GLFW Input. #928
GLFW Input. #928
Conversation
Just deferring this until I've checked over the others as it's quite large! |
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.
Looks good so far. Only significant change is removing our key enum and just using GLFW directly.
Opinions?
src/Windowing/OpenToolkit.Windowing.Common/Events/TextInputEventArgs.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Interfaces/IWindowInput.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Interfaces/IWindowInput.cs
Outdated
Show resolved
Hide resolved
Going back and forth on how best to avoid the mismatch issue between GLFW keys and OpenTK.Input.Keys: Current plan is to update our Key/Mouse/whatever enums to match the GLFW values directly. We use those inside GLFW directly (instead of having a second GLFW key enum and trying to map between them). This way we get all of the benefits of 1:1 mapping without the hard dependency on GLFW on other platforms. |
Also improve docs a tiny bit.
@Vassalware / @jvbsl - Would really appreciate a review on this if you guys can! |
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.
Except for KeyboardState I found nothing major and looks good so far. I did not test it though
src/Windowing/OpenToolkit.Windowing.Common/Events/FrameEventArgs.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Events/KeyboardKeyEventArgs.cs
Show resolved
Hide resolved
/// <summary> | ||
/// Gets the string representation of the input Unicode code point. | ||
/// </summary> | ||
public string String => char.ConvertFromUtf32(Unicode); |
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.
I think UnicodeString or something like that would be better. Just naming it String seems wrong to me because System.String is a type
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.
I'm open to a better name (AsString
maybe?), but UnicodeString
or something is kind of redundant since strings are always Unicode.
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
Int seems to produce better code inside the .NET Core JIT.
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.
Great work @PJB3005. This is super thorough and well written.
Only required change is the unsafe on the public API. The rest are all nice-to-haves.
src/Windowing/OpenToolkit.Windowing.Common/Events/MouseMoveEventArgs.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Input/KeyboardState.cs
Outdated
Show resolved
Hide resolved
src/Windowing/OpenToolkit.Windowing.Common/Events/MouseMoveEventArgs.cs
Outdated
Show resolved
Hide resolved
Kickass work and thanks again, @PJB3005 . |
Thanks to @jvbsl for the help on the review as well! |
Purpose of this PR
Implements input on GLFW. Keyboard input and mouse input works. Gamepad/joystick is not.
I had to fix GLFW window instancing too to actually test this, so if these changes are out of scope I can move them to a separate PR.
There's still a lot to do here but this is already a pretty big PR I would say.
Testing status
I have tested this with the wonderful world of
Console.WriteLine
and manual main loops.