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

Overhaul device events API and add gamepad support on Windows #804

Merged
merged 39 commits into from
Jun 20, 2019

Conversation

Osspial
Copy link
Contributor

@Osspial Osspial commented Mar 6, 2019

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created an example program if it would help users understand this functionality

This started out as an attempt to merge @francesca64's gamepad integration (❤️) from #624 onto the EventLoop 2.0 branch, but ballooned to a complete overhaul of the device event API since it was the easiest way to expose a clean rumble API and we agreed that it needed to be done anyway.

This is a draft PR because it makes absolutely no attempt to to implement the device event changes on non-Windows platforms, which needs to be done before this can be merged. This also means that it's waiting on X11 and MacOS EL2.0 implementations, since meaningful work to get this working on those backends can't start until those backends work.

Feedback on the API is encouraged, and the API docs are available here. I expect the non-breaking changes to be fairly uncontroversial, and these are my reasons for the breaking changes:

  • Splitting up DeviceEvent into MouseEvent, KeyboardEvent, and GamepadEvent:
    • Previously, the DeviceEvent enum was a bit of a mess; there were several different event sources aggregated under one type, most of which could only be emitted from one specific kind of device. Splitting the API up clarifies which events get emitted by which types of devices and makes it more easily learnable.
  • Removing DeviceEvent::Text variant:
    • No backend implemented it, and its utility compared to WindowEvent::ReceivedCharacter was questionable.
  • Splitting up DeviceId:
    • See reasons for splitting DeviceEvent. Also, I had to split off GamepadHandle so I could add a rumble function, and splitting up MouseId and KeyboardId makes the API more consistent.
  • Removing device IDs from WindowEvent variants:
    • This was only implemented on X11, and all the other platforms just returned stub IDs. This was misleading at best, and I couldn't see a compelling reason to keep it in the face of that.

Closes #750, closes #624, closes #119, closes #212 (?).

francesca64 and others added 30 commits November 4, 2018 15:31
* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input
This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.
@goddessfreya goddessfreya mentioned this pull request May 30, 2019
@aloucks
Copy link
Contributor

aloucks commented May 31, 2019

WIll it be more confusing to have Device events broken out individually while WindowEvents remain bundled under a common enum? This might make it more difficult to understand the subtitles between similar events (e.g. MouseMotion vs CursorMoved).

@Osspial
Copy link
Contributor Author

Osspial commented Jun 3, 2019

@aloucks That's a good point! It's probably worth taking a closer look at WindowEvent and examining it in the broader context of the API, but it wasn't something I considered when working on this PR.

@ghost
Copy link

ghost commented Jun 7, 2019

I finished reading through #695, but I'm still curious as to why it was decided to support gamepads in winit. Regardless of whether people expect winit to support it or not, gilrs already exists for talking to gamepads, and it looks to me like it does this pretty well. It seems like a waste to duplicate this effort. I thought this was previously declared as "out of scope" for winit?

@Osspial
Copy link
Contributor Author

Osspial commented Jun 11, 2019

@aleksijuvani It was indeed declared out of scope previously, but that was by a different person that's no longer actively involved with the project and I personally disagree with it. As for why we'd implement this ourselves instead of just directing people to gilrs: gilrs only supports a polling-based API, which is incompatible with non-game applications. Media center applications, for example, may still want to have responsive controller support while not constantly running a loop at 60 hertz. Besides, I can see no reason ideologically for gamepad input to be out of scope - Winit is an "window creation and input handling" library, and gamepad input is certainly used as an input method for a non-trivial number of applications.

@Osspial Osspial changed the base branch from eventloop-2.0 to master June 13, 2019 05:31
@Osspial
Copy link
Contributor Author

Osspial commented Jun 19, 2019

Once I've rebased this, I'm going to merge this onto the gamepad-device-events branch so new contributors can easily make PRs against it.

@goddessfreya
Copy link
Contributor

Can we merge this instead against https://github.com/rust-windowing/winit/tree/dpi-overhaul? Makes my life easier, as I don't have to maintain two copies of glutin.

@Osspial
Copy link
Contributor Author

Osspial commented Jun 19, 2019

@zegentzy I'd rather not, since that makes it impossible to implement the DPI changes and gamepad support in separate PRs - you'd have to deal with the error messages from both at once, which sounds like a bad experience for new contributors.

Why would you have to maintain additional copies of glutin, anyway? The intent isn't for people to actively use those branches in their applications - it's so we have a centralized place to develop these features, and then merge them onto master. Glutin isn't necessary for either of those processes.

(also I'd be surprised if the breaking changes in this patch break things on Glutin's end)

@Osspial Osspial changed the base branch from master to gamepad-device-events June 20, 2019 02:10
@Osspial Osspial marked this pull request as ready for review June 20, 2019 02:10
@goddessfreya
Copy link
Contributor

goddessfreya commented Jun 20, 2019

People usually (I think?) only want to implement something if they want to use it in their application. Generally, from my experience at least, you port your application to this shiny new unimplemented api, then fiddle with the dependency till it compiles and works. Like, what better way to test if gampad support works then seeing if it works in your application?

Now, if these applications are dependent on glutin, and this PR doesn't break anything in glutin, then I guess it's as simple as a [replace] in the Cargo.toml. However, if this pr does break something, now some poor soul is going to be fiddling with glutin and winit, and they're going to send in those changes, and now I got three copies of glutin, one for each branch (master, gamepad, dpi).

Besides, what if they want to run their application with both the dpi overhaul and gamepad support? I see little joy for the poor soul who was to merge the two branches together (and then demerge them to send in PRs separately), as git is a PITA, nor for the poor soul in the end who has to merge the two branches back, one-by-one, into master, hoping for the changes from each don't conflict with each other.

And, of course, now we got to backport changes from master back to two branches, instead of just one, to avoid them getting too out of sync from master. This is like the eventsloopv2-master split, but with twice the pain.

@Osspial
Copy link
Contributor Author

Osspial commented Jun 20, 2019

Remember, dpi-overhaul and gamepad-device-events aren't intended as long-lived branches that diverge significantly from master: they're small branches oriented around specific features that exist to make it easy for new contributors to make significant improvements to Winit's API even if they aren't familiar with our codebase. Part of the reason dpi-overhaul is a great place for new contributors is that implementing its changes for the most part just involves following several related error messages, and making localized changes around those error messages. Since both this branch and dpi-overhaul break compilation on non-Windows platforms, combining them prematurely will make it impossible for new contributors to focus on a single problem at a time.

Now, if these applications are dependent on glutin, and this PR doesn't break anything in glutin, then I guess it's as simple as a [replace] in the Cargo.toml.

I just tested it, and I can confirm that this PR doesn't break anything in Glutin.

Like, what better way to test if gampad support works then seeing if it works in your application?

I'll agree with that, but see point above.

This is like the eventsloopv2-master split, but with twice the pain.

The difference between these two branches and the EL2 branch is that the changes in these branches are much less fundamental than the changes introduced by the EL2 branch. We aren't in a situation where changes to master are inherently incompatible with changes to these branches - the vast majority of changes will be localized enough that merging will either be completely painless or only involve trivial tweaks. Indeed, I just tested merging dpi-overhaul and el2-win-joy, and git merged them without complaints.

Besides, these branches should get merged much more quickly than than the EL2 branch did, since the changes are much easier to implement and can be implemented without a deep understanding of Winit's structure.

Besides, what if they want to run their application with both the dpi overhaul and gamepad support?

They can either merge the branches themselves, or wait for the changes to land on master. These branches exist primarily to make development easier on our end, not for downstream users to test both changes simultaneously.

I see little joy for the poor soul who was to merge the two branches together (and then demerge them to send in PRs separately), as git is a PITA, nor for the poor soul in the end who has to merge the two branches back, one-by-one, into master, hoping for the changes from each don't conflict with each other.

I'd argue that merging two WIP branches, fixing the issues raised by both branches, than de-merging them is a gross misuse of git as a tool.

@Osspial Osspial merged commit ef34d46 into rust-windowing:gamepad-device-events Jun 20, 2019
Osspial added a commit that referenced this pull request Jun 20, 2019
* Initial implementation

* Corrected RAWINPUT buffer sizing

* Mostly complete XInput implementation

* XInput triggers

* Add preliminary CHANGELOG entry.

* match unix common API to evl 2.0

* wayland: eventloop2.0

* make EventLoopProxy require T: 'static

* Revamp device event API, as well as several misc. fixes on Windows:

* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input

* Add MouseEvent documentation and Device ID debug passthrough

* Improve type safety on get_raw_input_data

* Remove button_id field from MouseEvent::Button in favor of �utton

* Remove regex dependency on Windows

* Remove axis filtering in XInput

* Make gamepads not use lazy_static

* Publicly expose gamepad rumble

* Unstack DeviceEvent and fix examples/tests

* Add HANDLE retrieval method to DeviceExtWindows

* Add distinction between non-joystick axes and joystick axes.

This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.

* Add ability to get gamepad port

* Fix xinput controller hot swapping

* Add functions for enumerating attached devices

* Clamp input to [0.0, 1.0] on gamepad rumble

* Expose gamepad rumble errors

* Add method to check if device is still connected

* Add docs

* Rename AxisHint and ButtonHint to GamepadAxis and GamepadButton

* Add CHANGELOG entry

* Update CHANGELOG.md

* Add HidId and MovedAbsolute

* Fix xinput deprecation warnings

* Add ability to retrieve gamepad battery level

* Fix weird imports in gamepad example

* Update CHANGELOG.md

* Resolve francesca64 comments
Osspial added a commit that referenced this pull request Jun 21, 2019
* Initial implementation

* Corrected RAWINPUT buffer sizing

* Mostly complete XInput implementation

* XInput triggers

* Add preliminary CHANGELOG entry.

* match unix common API to evl 2.0

* wayland: eventloop2.0

* make EventLoopProxy require T: 'static

* Revamp device event API, as well as several misc. fixes on Windows:

* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input

* Add MouseEvent documentation and Device ID debug passthrough

* Improve type safety on get_raw_input_data

* Remove button_id field from MouseEvent::Button in favor of �utton

* Remove regex dependency on Windows

* Remove axis filtering in XInput

* Make gamepads not use lazy_static

* Publicly expose gamepad rumble

* Unstack DeviceEvent and fix examples/tests

* Add HANDLE retrieval method to DeviceExtWindows

* Add distinction between non-joystick axes and joystick axes.

This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.

* Add ability to get gamepad port

* Fix xinput controller hot swapping

* Add functions for enumerating attached devices

* Clamp input to [0.0, 1.0] on gamepad rumble

* Expose gamepad rumble errors

* Add method to check if device is still connected

* Add docs

* Rename AxisHint and ButtonHint to GamepadAxis and GamepadButton

* Add CHANGELOG entry

* Update CHANGELOG.md

* Add HidId and MovedAbsolute

* Fix xinput deprecation warnings

* Add ability to retrieve gamepad battery level

* Fix weird imports in gamepad example

* Update CHANGELOG.md

* Resolve francesca64 comments
Osspial added a commit that referenced this pull request Jun 21, 2019
* Initial implementation

* Corrected RAWINPUT buffer sizing

* Mostly complete XInput implementation

* XInput triggers

* Add preliminary CHANGELOG entry.

* match unix common API to evl 2.0

* wayland: eventloop2.0

* make EventLoopProxy require T: 'static

* Revamp device event API, as well as several misc. fixes on Windows:

* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input

* Add MouseEvent documentation and Device ID debug passthrough

* Improve type safety on get_raw_input_data

* Remove button_id field from MouseEvent::Button in favor of �utton

* Remove regex dependency on Windows

* Remove axis filtering in XInput

* Make gamepads not use lazy_static

* Publicly expose gamepad rumble

* Unstack DeviceEvent and fix examples/tests

* Add HANDLE retrieval method to DeviceExtWindows

* Add distinction between non-joystick axes and joystick axes.

This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.

* Add ability to get gamepad port

* Fix xinput controller hot swapping

* Add functions for enumerating attached devices

* Clamp input to [0.0, 1.0] on gamepad rumble

* Expose gamepad rumble errors

* Add method to check if device is still connected

* Add docs

* Rename AxisHint and ButtonHint to GamepadAxis and GamepadButton

* Add CHANGELOG entry

* Update CHANGELOG.md

* Add HidId and MovedAbsolute

* Fix xinput deprecation warnings

* Add ability to retrieve gamepad battery level

* Fix weird imports in gamepad example

* Update CHANGELOG.md

* Resolve francesca64 comments
Osspial added a commit that referenced this pull request Jun 24, 2019
* Initial implementation

* Corrected RAWINPUT buffer sizing

* Mostly complete XInput implementation

* XInput triggers

* Add preliminary CHANGELOG entry.

* match unix common API to evl 2.0

* wayland: eventloop2.0

* make EventLoopProxy require T: 'static

* Revamp device event API, as well as several misc. fixes on Windows:

* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input

* Add MouseEvent documentation and Device ID debug passthrough

* Improve type safety on get_raw_input_data

* Remove button_id field from MouseEvent::Button in favor of �utton

* Remove regex dependency on Windows

* Remove axis filtering in XInput

* Make gamepads not use lazy_static

* Publicly expose gamepad rumble

* Unstack DeviceEvent and fix examples/tests

* Add HANDLE retrieval method to DeviceExtWindows

* Add distinction between non-joystick axes and joystick axes.

This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.

* Add ability to get gamepad port

* Fix xinput controller hot swapping

* Add functions for enumerating attached devices

* Clamp input to [0.0, 1.0] on gamepad rumble

* Expose gamepad rumble errors

* Add method to check if device is still connected

* Add docs

* Rename AxisHint and ButtonHint to GamepadAxis and GamepadButton

* Add CHANGELOG entry

* Update CHANGELOG.md

* Add HidId and MovedAbsolute

* Fix xinput deprecation warnings

* Add ability to retrieve gamepad battery level

* Fix weird imports in gamepad example

* Update CHANGELOG.md

* Resolve francesca64 comments
Osspial added a commit that referenced this pull request Jun 24, 2019
* Initial implementation

* Corrected RAWINPUT buffer sizing

* Mostly complete XInput implementation

* XInput triggers

* Add preliminary CHANGELOG entry.

* match unix common API to evl 2.0

* wayland: eventloop2.0

* make EventLoopProxy require T: 'static

* Revamp device event API, as well as several misc. fixes on Windows:

* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input

* Add MouseEvent documentation and Device ID debug passthrough

* Improve type safety on get_raw_input_data

* Remove button_id field from MouseEvent::Button in favor of �utton

* Remove regex dependency on Windows

* Remove axis filtering in XInput

* Make gamepads not use lazy_static

* Publicly expose gamepad rumble

* Unstack DeviceEvent and fix examples/tests

* Add HANDLE retrieval method to DeviceExtWindows

* Add distinction between non-joystick axes and joystick axes.

This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.

* Add ability to get gamepad port

* Fix xinput controller hot swapping

* Add functions for enumerating attached devices

* Clamp input to [0.0, 1.0] on gamepad rumble

* Expose gamepad rumble errors

* Add method to check if device is still connected

* Add docs

* Rename AxisHint and ButtonHint to GamepadAxis and GamepadButton

* Add CHANGELOG entry

* Update CHANGELOG.md

* Add HidId and MovedAbsolute

* Fix xinput deprecation warnings

* Add ability to retrieve gamepad battery level

* Fix weird imports in gamepad example

* Update CHANGELOG.md

* Resolve francesca64 comments
Osspial added a commit that referenced this pull request Nov 29, 2019
* Initial implementation

* Corrected RAWINPUT buffer sizing

* Mostly complete XInput implementation

* XInput triggers

* Add preliminary CHANGELOG entry.

* match unix common API to evl 2.0

* wayland: eventloop2.0

* make EventLoopProxy require T: 'static

* Revamp device event API, as well as several misc. fixes on Windows:

* When you have multiple windows, you no longer receive duplicate device
  events
* Mouse Device Events now send X-button input
* Mouse Device Events now send horizontal scroll wheel input

* Add MouseEvent documentation and Device ID debug passthrough

* Improve type safety on get_raw_input_data

* Remove button_id field from MouseEvent::Button in favor of �utton

* Remove regex dependency on Windows

* Remove axis filtering in XInput

* Make gamepads not use lazy_static

* Publicly expose gamepad rumble

* Unstack DeviceEvent and fix examples/tests

* Add HANDLE retrieval method to DeviceExtWindows

* Add distinction between non-joystick axes and joystick axes.

This helps with properly calculating the deadzone for controller
joysticks. One potential issue is that the `Stick` variant isn't used
for *all* joysticks, which could be potentially confusing - for example,
raw input joysticks will never use the `Stick` variant because we don't
understand the semantic meaning of raw input joystick axes.

* Add ability to get gamepad port

* Fix xinput controller hot swapping

* Add functions for enumerating attached devices

* Clamp input to [0.0, 1.0] on gamepad rumble

* Expose gamepad rumble errors

* Add method to check if device is still connected

* Add docs

* Rename AxisHint and ButtonHint to GamepadAxis and GamepadButton

* Add CHANGELOG entry

* Update CHANGELOG.md

* Add HidId and MovedAbsolute

* Fix xinput deprecation warnings

* Add ability to retrieve gamepad battery level

* Fix weird imports in gamepad example

* Update CHANGELOG.md

* Resolve francesca64 comments
@maroider maroider mentioned this pull request Jul 4, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - in progress Implementation is proceeding smoothly C - needs discussion Direction must be ironed out C - waiting on author Waiting for a response or another PR DS - windows S - api Design and usability S - enhancement Wouldn't this be the coolest?
Development

Successfully merging this pull request may close these issues.

5 participants