w3c / gamepad

Gamepad
https://w3c.github.io/gamepad/
Other
141 stars 48 forks source link

Add vibrationActuator to the main spec #190

Closed gabrielsanbrito closed 8 months ago

gabrielsanbrito commented 10 months ago

Closes #173

The following tasks have been completed:

Implementation commitment:


Preview | Diff

gabrielsanbrito commented 10 months ago

@nondebug, @marcoscaceres Just a few questions I had while editing:

nondebug commented 10 months ago

Should we remove the GamepadHapticActuatorType from the spec

Yes, let's remove it. It was never really part of the spec so it's not even officially deprecated.

Should we create a pull request to add the visibility changed steps to the HTML spec

Yes, let's do that.

marcoscaceres commented 10 months ago

Yeah, @gabrielsanbrito, it seems you've accidentally changed the whitespace of the file: https://github.com/w3c/gamepad/pull/190/files?diff=unified&w=1

Can you see about changing it back?

Maybe try:

git rebase --whitespace=fix gh-pages
marcoscaceres commented 8 months ago

File Gecko and WebKit bugs... WebKit already implements vibratorActuator, and so does Chrome. But it would be good to make sure we are all interoperable.

In any case, it meets criteria for inclusion in the spec.

marcoscaceres commented 8 months ago

@gabrielsanbrito, I merged in my suggestions and rebased.

The last remaining thing is just adding the DOMHighResTimestamp suggestion. Do you want me to do that?

gabrielsanbrito commented 8 months ago

@gabrielsanbrito, I merged in my suggestions and rebased.

The last remaining thing is just adding the DOMHighResTimestamp suggestion. Do you want me to do that?

Thanks for doing that, @marcoscaceres! I was looking up on how to do the DOMHighResTimestamp. I was thinking on something like this:

<li>Let {{GamepadHapticActuator/[[playingEffectPromise]]}} be [=a
new promise=].
<li>Let |playEffectTimestamp:DOMHighResTimestamp| be the [=current high resolution time=] given the |document|'s [=relevant global object=].
</li>
<li>Do the following steps [=in parallel=]:
  <ol>
    <li>[=Issue a haptic effect=] to the actuator with |type|, |params|, and the 
     |playEffectTimestamp|.
    </li>
[...]

Then, on the issue a haptic effect, I was thinking of adding:

To <dfn>issue a haptic effect</dfn> on an actuator, the [=user agent=] MUST send a command to the device to render an effect of |type:GamepadHapticEffectType| and try to make it use the provided |params:GamepadEffectParameters|. The [=user agent=] SHOULD use the provided |playEffectTimestamp:DOMHighResTimestamp| for more precise playback timing when |params|.{{startDelay}} is not `0.0`.

What do you think? Please feel free to add it directly if it is too far from ideal.

gabrielsanbrito commented 8 months ago

File Gecko and WebKit bugs... WebKit already implements vibratorActuator, and so does Chrome. But it would be good to make sure we are all interoperable.

In any case, it meets criteria for inclusion in the spec.

It looks like both WebKit (https://bugs.webkit.org/show_bug.cgi?id=267523) and Gecko (https://bugzilla.mozilla.org/show_bug.cgi?id=1824217) already have bugs filed for the vibrationActuator addition. I have just added a small update comment on the Gecko one to let them know that vibrationActuator is being added to the Gamepad API spec.

marcoscaceres commented 8 months ago

@gabrielsanbrito, the proposed text for the timestamp sounds great!

marcoscaceres commented 8 months ago

Ok, cool. I've updated the OP to link to the existing Gecko bug. Thanks also for commenting there @gabrielsanbrito.

marcoscaceres commented 8 months ago

I wonder if we should do a little "test fest" on our next call. Would be nice to just run through the algorithms manually and check for any introp issues.

gabrielsanbrito commented 8 months ago

@marcoscaceres had to remove the "switch" class from the dl tag. It was messing the numbers of the ordered list. With the class set, I was having: Item 1: Given the value of {{GamepadHapticEffectType}} |type:GamepadHapticEffectType|, switch on:  "dual-rumble": If params does not describe a valid dual-rumble effect, return false. Item 3: Return true.

Just removing the class made "Return true." become item 2 again - so I have removed it. I don't know if I am missing on something here or if there is something wrong with the "switch" class. Please let me know if this looks good for you. Now it is looking like this: Item 1: Given the value of {{GamepadHapticEffectType}} |type:GamepadHapticEffectType|, switch on:  "dual-rumble": If params does not describe a valid dual-rumble effect, return false. Item 2: Return true.

marcoscaceres commented 8 months ago

Yeah, that's fine. And don't worry, we will eventually get rid of all the <dl>s in algorithms... they are annoying.

marcoscaceres commented 8 months ago

@gabrielsanbrito, just waiting on Matt. Spoke to him and he's hoping to do the final review soon. Expecting this to land hopefully early next week.

gabrielsanbrito commented 8 months ago

Thank you all for the valuable feedback!

@marcoscaceres may I merge the pull request?

marcoscaceres commented 8 months ago

Merged 🎉

razvanphp commented 2 months ago

This link in the OP is missing: https://issues.chromium.org/issues/40834175