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

Gamepad trigger-rumble and associated updates #34442

Merged
merged 8 commits into from
Jul 24, 2024

Conversation

chrisdavidmills
Copy link
Contributor

@chrisdavidmills chrisdavidmills commented Jun 27, 2024

Description

The Gamepad API "trigger-rumble" haptic effect has been supported since Chrome 126 (see the ChromeStatus entry).

This PR adds/updates content covering related features:

  • Bring GamepadHapticActuator.type page up-to-date.
  • Add GamepadHapticActuator.effects page.
  • Add GamepadHapticActuator.reset() page.
  • Add "trigger-rumble" as an effect type that can be used in the GampadHapticActuator.effects property.
  • Add "trigger-rumble" as an effect type that can be used in the GampadHapticActuator.playEffect() method.
  • Add leftTrigger and rightTrigger as params that can be used in the GampadHapticActuator.playEffect() method.

Motivation

Additional details

Related issues and pull requests

@chrisdavidmills chrisdavidmills requested review from a team as code owners June 27, 2024 07:37
@chrisdavidmills chrisdavidmills requested review from wbamberg and pepelsbey and removed request for a team June 27, 2024 07:37
@chrisdavidmills chrisdavidmills marked this pull request as draft June 27, 2024 07:37
@github-actions github-actions bot added Content:WebAPI Web API docs size/s [PR only] 6-50 LoC changed labels Jun 27, 2024
@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Jun 27, 2024
@chrisdavidmills chrisdavidmills marked this pull request as ready for review June 27, 2024 11:50
@@ -3,22 +3,34 @@ title: "GamepadHapticActuator: type property"
short-title: type
slug: Web/API/GamepadHapticActuator/type
page-type: web-api-instance-property
status:
- deprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

These days the source of truth for deprecated is BCD, which auto-fills this value and auto-adds macro calls. So unless the BCD for this property also marks it deprecated, this will get un-fixed again next time the script runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good call. I've just done that.

- `weakMagnitude`
- : Rumble intensity of the high-frequency (weak) rumble motors, normalized to the range between 0.0 and 1.0.
- `duration` {{optional_inline}}
- : The duration of the effect in milliseconds. If omitted, `duration` defaults to `0`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

highly subjective, but I think this is overly repetitive, and in this situation I just say:

Suggested change
- : The duration of the effect in milliseconds. If omitted, `duration` defaults to `0`.
- : The duration of the effect in milliseconds.
Defaults to `0`.

I wish we had a consistent way to write default values for optional items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, mine is wordier, but it is also a bit more intuitive. Saying that, I don't think it really adds much, so I've updated it to your suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, mine is wordier, but it is also a bit more intuitive.

It is, and if this were the only place we document a default I would prefer it. But there are (obviously) thousands of places we document defaults, and if every one says "If omitted, thing defaults to value" then it is very repetitive: "if omitted" essentially repeats the definition of "default" every time. IMO MDN really misses a structured consistent way to represent "default", that's not just in prose at all.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jul 13, 2024
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Jul 14, 2024
Copy link
Contributor

@tomayac tomayac left a comment

Choose a reason for hiding this comment

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

Non-owner LGTM. Left one suggestion, which could also be applied to playEffect(). It's a bit unclear what's the best to suggest in both cases, as the value of the promise rarely will be needed. Feel free to ignore.

strongMagnitude: 1.0,
});

gamepad.vibrationActuator.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you want to await this and show the returned value? Maybe to make it crystal-clear, you could wrap the call in a setTimeout() with a delay <200ms, so you can show the "preempted" return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @tomayac! I wasn't 100% sure what you meant, but I've tried to update both pages as suggested. Let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

In the reset() example, you await playing the effect before scheduling the timeout, so it doesn't show the effect you mean to show as per the comment. You need to flip the order. I would probably rather work with then() in both cases, resetting and playing the effect. Awaiting something in a realtime game scenario feels like a weird pattern.

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've swapped the order as you suggested, and converted both examples to use then(). Thanks again.

Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks :)

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

👍 thank you Chris!

@wbamberg wbamberg merged commit 874fc07 into mdn:main Jul 24, 2024
9 checks passed
@chrisdavidmills chrisdavidmills deleted the gamepad-trigger-rumble branch August 2, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants