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

Added support for gamepad #164

Merged
merged 1 commit into from Jan 10, 2019
Merged

Added support for gamepad #164

merged 1 commit into from Jan 10, 2019

Conversation

tario
Copy link
Contributor

@tario tario commented Jan 8, 2019

Features:

  • Configuration of gamepad button from controls modal
  • Added icons for keyboard and gamepad according to the setup
  • Support for two gamepad devices connected at the same time (for player 1 and player 2) regardless of whether they are both the same model or not
  • Storage of gamepad button config linked to gamepad id (e.g. "USB Gamepad (STANDARD GAMEPAD Vendor: 0079 Product: 0011)")
  • Storage of gamepad id linked to players (e.g. player 1 use "USB Gamepad (STANDARD GAMEPAD Vendor: 0079 Product: 0011)" and player 2 use "usb gamepad (Vendor: 0810 Product: e501)")
  • Avoiding at all the use of gamepad "indexes" from API, because these can change if the gamepads are reconnected on different order
  • Avoiding hardcode of gamepad button configurations (any gamepad supported by gamepad API can be used)

Please let me know if this need fixes. I will work on them as soon as possible

Appendix:

  • Sample of localStorage item generated for gamepadConfig
    gamepadConfig.txt

  • Screenshot of ControlsModal

controlsmodal1
controlsmodal2

@tario tario changed the title Added support for gamepad Added support for gamepad #19 Jan 8, 2019
@tario tario changed the title Added support for gamepad #19 Added support for gamepad Jan 8, 2019
Copy link
Owner

@bfirsh bfirsh left a comment

Choose a reason for hiding this comment

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

Nice, thanks! I haven't got a gamepad so I trust this works. :)

A couple of minor comments. I think this code could also generally do with some refactoring, which is already noted in #149.

.playerGamepadId || [null, null];
this.state.gamepadConfig.configs = this.state.gamepadConfig.configs || {};

this.state.controllerIcon = this.state.gamepadConfig.playerGamepadId.map(
Copy link
Owner

Choose a reason for hiding this comment

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

This stuff seems to be calculated based on gamepadConfig. Perhaps it makes more sense to just be a function or something instead of being on state.

Copy link
Contributor Author

@tario tario Jan 9, 2019

Choose a reason for hiding this comment

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

I am sorry, I don't get it, it is only a default initialization to ensure gamepadConfig starts in a not null state. Show me a code example if you want

Copy link
Owner

Choose a reason for hiding this comment

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

It's only a minor thing, no worries. From a cursory look at the code, it seems like controllerIcon is just computed from gamepadConfig. So, it's not really state, but just a function which would take gamepadConfig as an argument.

I'll get this merged and then make some follow-up issues. :)

</th>
<th>
Player 2
<img
Copy link
Owner

Choose a reason for hiding this comment

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

Have you tested that this works in the built production bundle? I think you might need to import the images to make sure the calculated paths are correct: https://facebook.github.io/create-react-app/docs/adding-images-fonts-and-files

Copy link
Contributor Author

@tario tario Jan 9, 2019

Choose a reason for hiding this comment

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

Great! I alredy tested it using developer run (yarn start) and production build on my local machine. I will test it again on the netlify deploy for this PR (https://deploy-preview-164--youthful-kepler-d6ba79.netlify.com/), only to be sure

Do you need a list of covered cases ?

Copy link
Owner

Choose a reason for hiding this comment

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

Nice okay. Sounds like it works then. :)

@bfirsh
Copy link
Owner

bfirsh commented Jan 9, 2019

Come to think of it, perhaps some basic tests are a good idea. I'm never going to test this manually, so it will probably regress.

@bfirsh bfirsh merged commit a1c0c8f into bfirsh:master Jan 10, 2019
@bfirsh
Copy link
Owner

bfirsh commented Jan 10, 2019

Thanks @tario! ✨

@bfirsh
Copy link
Owner

bfirsh commented Jan 10, 2019

See #165.

This was referenced Jan 10, 2019
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.

None yet

2 participants