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.

Sign up for GitHub

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

Jump to bottom

Android: option to vibrate on button press #580

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Android: option to vibrate on button press #580

wants to merge 3 commits into from

Conversation

BigRobCoder
Copy link
Contributor

I haven't made a OSS contribution in a while, and web stuff is not my expertise, so any feedback is appreciated!

BigRobCoder
BigRobCoder commented Oct 12, 2022
View reviewed changes
runtimes/web/src/ui/app.ts Outdated
@@ -76,6 +83,11 @@ export class App extends LitElement {

private readonly diskPrefix: string;

private vibrateLevelIdx = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure where to store the vibration level. I tried in virtual-gamepad.ts at first, but didn't see how menu-overlay.ts could access it, so it ended up here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm defaulting to "OFF" for now, but if feedback is positive I recommend we default to "MEDIUM", I think the vibration really makes it feel more tactile.

BigRobCoder
BigRobCoder commented Oct 12, 2022
View reviewed changes
runtimes/web/src/ui/menu-overlay.ts
@@ -115,6 +118,7 @@ export class MenuOverlay extends LitElement {

@state() private selectedIdx = 0;
@state() private netplaySummary: { playerIdx: number, ping: number }[] = [];
@state() private vibrateLevel = "";
Copy link
Contributor Author

@BigRobCoder BigRobCoder Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I tried just referencing app.vibrateLevel, but wasn't seeing render updates immediately. Is this the correct pattern? Mirror the setting in a local @state() variable?

BigRobCoder
BigRobCoder commented Oct 12, 2022
View reviewed changes
runtimes/web/src/ui/menu-overlay.ts Outdated
@@ -243,11 +258,16 @@ export class MenuOverlay extends LitElement {
}

render () {
if (!this.vibrateLevel) {
this.vibrateLevel = this.app.vibrateLevel[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried setting this in the constructor, but looks like app is not yet ready, so moved it to here.

@BigRobCoder BigRobCoder marked this pull request as ready for review October 12, 2022 05:43
@BigRobCoder
Copy link
Contributor Author

Hmm, after some further testing it seems like the vibration stops working after a while. Maybe chrome blocks website vibrations if they are triggered too frequently?

@BigRobCoder
Copy link
Contributor Author

Was testing this for a while tonight on a Samsung Galaxy S21, no issues this time. Tried testing on an iPhone 13 as well, as intended there was no "vibrate" menu option.

@aduros
Copy link
Owner

aduros commented Oct 22, 2022

Thanks so much for the PR! It looks very clean, and this is a great feature.

Some feedback/questions after testing:

  • On my device (Android Moto) all the different vibration levels (LOW, MEDIUM, HIGH) feel the same. Is it worth having multiple levels?
  • What do you think about only vibrating on touchdown instead of both touchdown and touchup? I don't have a strong opinion here but wondering if that might be more ergonomic?
  • Let's avoid calling navigator.vibrate() at all if the setting is off, instead of passing it zero.
  • In a future PR we could move the vibration setting to a settings submenu, and also persist settings between sessions 😃

@BigRobCoder
Copy link
Contributor Author

Thanks so much for the PR! It looks very clean, and this is a great feature.

Some feedback/questions after testing:

  • On my device (Android Moto) all the different vibration levels (LOW, MEDIUM, HIGH) feel the same. Is it worth having multiple levels?

Very interesting. On my Samsung Galaxy S21, the different vibration do feel different, but it probably depends on the device. The way I have it now, "LOW" should be 1 millisecond of vibration, "MEDIUM" 2, and "HIGH" 4. Maybe I should have more like 10 different levels of vibration, from 1ms to 1024ms?

  • What do you think about only vibrating on touchdown instead of both touchdown and touchup? I don't have a strong opinion here but wondering if that might be more ergonomic?

Yeah, I started out just vibrating on touchdown, but after playing a bunch of games, it just didn't feel right to me, and vibrate both felt more right.

  • Let's avoid calling navigator.vibrate() at all if the setting is off, instead of passing it zero.

Sure.

  • In a future PR we could move the vibration setting to a settings submenu, and also persist settings between sessions 😃

Yeah, that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BigRobCoder @aduros