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

Add Gamepad API #223

Merged
merged 8 commits into from
May 28, 2018
Merged

Add Gamepad API #223

merged 8 commits into from
May 28, 2018

Conversation

coryshrmn
Copy link
Contributor

This PR fully wraps the current Gamepad API spec.

The Gamepad API is important for Rust browser games. It is supported on all major browsers.

GamepadEvent and GamepadButtonMapping are unit-tested. The rest of the API is not testable, because Gamepad and GamepadButton are not constructible.

In lieu of tests, I added a Gamepad example (deployed here). It demonstrates the entire API (except GamepadButton::touched(), which is not yet supported in Chrome). I verified the example works perfectly on Firefox, Chrome, and Safari.

Please let me know if there is anything else I can do.
Thanks!

The example listens for gamepad connections, and constantly displays the full state of all gamepads.

Works on Firefox 60.0.1.

Not updating properly on Chrome 65.0.3325.181.
The gamepad state is correct when first connected, but never updates.
We can only test `GamepadEvent` and `GamepadButtonMapping`, because `Gamepad` and `GamepadButton` are not constructible.

In lieu of tests, the Gamepad example is a fairly exhaustive demo of the full Gamepad API.
It only lacks `GamepadButton::touched()`, because Chrome does not support it.
Copy link
Owner

@koute koute left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! So far looks pretty great!

/// Retrieve all connected gamepads, in an array indexed by each gamepad's `index` member.
///
/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/getGamepads)
// https://w3c.github.io/gamepad/#dom-gamepad-axes
Copy link
Owner

Choose a reason for hiding this comment

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

Wrong specs link. (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks :)

///
/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/getGamepads)
// https://w3c.github.io/gamepad/#dom-gamepad-axes
pub fn get_gamepads() -> Vec<Option<Gamepad>> {
Copy link
Owner

Choose a reason for hiding this comment

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

This would be better as Gamepad::get_all or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec is navigator.getGamepads(), I wanted to keep the original name so it is obvious which JS function is being called. getGamepads() has some browser quirks, notably Chrome only updates Gamepad state when that function is called.

How does Gamepad::get_gamepads() sound?

Copy link
Owner

Choose a reason for hiding this comment

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

If we absolutely want to adhere to the specs we could add a navigator method (just as we do for e.g. document) and then define get_gamepads on it. Then it would be obvious that it's a 1-to-1 call.

In extra convenience methods like this one we usually just pick a name that's more Rust-y, and in this case since the call will be prefixed with Gamepad:: there is no point in repeating it.

That said, it's not that much of a big deal, so I leave this decision to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Gamepad::get_all() does sound nicer. I renamed to Gamepad::get_all(), and documented it should be called every frame.

#[reference(instance_of = "Gamepad")]
pub struct Gamepad( Reference );

impl IGamepad for Gamepad {}
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm... since those are the only impementations we probably should get rid of the IGamepadButton and IGamepad interfaces and just define all of those methods in a normal impl block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. done!

1. correct spec link for get_gamepads()
2. remove IGamepad and IGamepadButton.
    impl Gamepad and GamepadButton directly.
3. move `get_gamepads()` to `Gamepad::get_gamepads()`.
@koute
Copy link
Owner

koute commented May 28, 2018

LGTM! Thanks a lot!

@koute koute merged commit 8c2d499 into koute:master May 28, 2018
coryshrmn added a commit to coryshrmn/stdweb that referenced this pull request May 29, 2018
koute#223 was merged,
so we are now in sync with koute/stdweb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants