Re: [w3c/gamepad] Move vibrationActuator to the main spec (PR #190)

@nondebug requested changes on this pull request.



> +          <td>
+            An empty [=list=]
+          </td>
+          <td>
+            A [=list=] containing the maximum logical value for each button
+          </td>
+        </tr>
+        <tr>
+          <td>
+            <dfn data-dfn-for="Gamepad">[[\vibrationActuator]]</dfn>
+          </td>
+          <td>
+            null
+          </td>
+          <td>
+            A {{GamepadHapticActuator}} object capable of generating a haptic effect that vibrates the entire gamepad.

A {{GamepadHapticActuator}} object capable of generating a haptic effect that vibrates the entire gamepad, or null if the gamepad has no such capability.

> +            RECOMMENDED that buttons appear in decreasing importance such that
+            the primary button, secondary button, tertiary button, and so on
+            appear as elements 0, 1, 2, ... in the buttons array. The same
+            object MUST be returned until the [=user agent=] needs to return
+            different values (or values in a different order).
+          </p>
+          <p>
+            The {{Gamepad/buttons}} getter steps are:
+          </p>
+          <ol>
+            <li>Return [=this=].{{Gamepad/[[buttons]]}}.
+            </li>
+          </ol>
+        </dd>
+        <dt>
+          <dfn>vibrationActuator</dfn> attribute

We should define algorithmic steps for the vibrationActuator attribute getter like we do for other attributes.

```
<p>
  The {{Gamepad/vibrationActuator}} getter steps are:
</p>
<ol>
  <li>Return [=this=].{{Gamepad/[[vibrationActuator]]}}.
  </li>
</ol>
```

I think the paragraph below is no longer needed since we now define steps for initializing the internal slot in [=initialize the vibration actuator=].

> +        </dd>
+      </dl>
+    </section>
+    <section data-dfn-for="GamepadHapticActuator" data-link-for="GamepadHapticActuator">
+      <h2>
+        <dfn>GamepadHapticActuator</dfn> Interface
+      </h2>
+      <p>
+        A {{GamepadHapticActuator}} corresponds to a configuration of motors or
+        other actuators that can apply a force for the purposes of haptic
+        feedback.
+      </p>
+      <pre class="idl">
+        [Exposed=Window]
+        interface GamepadHapticActuator {
+          readonly attribute GamepadHapticActuatorType type;

Let's remove `GamepadHapticActuatorType type`, it is not needed now that we have `canPlayEffectType`.

> +      </dl>
+    </section>
+    <section data-dfn-for="GamepadHapticActuator" data-link-for="GamepadHapticActuator">
+      <h2>
+        <dfn>GamepadHapticActuator</dfn> Interface
+      </h2>
+      <p>
+        A {{GamepadHapticActuator}} corresponds to a configuration of motors or
+        other actuators that can apply a force for the purposes of haptic
+        feedback.
+      </p>
+      <pre class="idl">
+        [Exposed=Window]
+        interface GamepadHapticActuator {
+          readonly attribute GamepadHapticActuatorType type;
+          boolean canPlayEffectType(GamepadHapticEffectType type);

In an earlier review [Marcos suggested](https://github.com/w3c/gamepad/pull/163#pullrequestreview-1000657433) some modifications to `canPlayEffectType`. I think there are no implementations yet so we are unconstrained.

suggestion 1: rename to "canPlay" or "supports"
suggestion 2: change the parameter to a DOMString so it's not an error to pass a new effect type to an old browser that doesn't recognize it
suggestion 3: replace "canPlayEffectType" with an "effects" attribute that returns a FrozenArray of supported effect types

Suggestion 3 seems best to me.

> +                    with {{GamepadHapticsResult/"complete"}}.
+                    </li>
+                    <li>Set
+                    [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}
+                    to `undefined`.
+                    </li>
+                  </ol>
+                </li>
+              </ol>
+            </li>
+            <li>Return {{GamepadHapticActuator/[[playingEffectPromise]]}}.
+            </li>
+          </ol>
+        </dd>
+        <dt>
+          <dfn>pulse()</dfn> method

We should define steps for `pulse`.

> +        <dfn>Gamepad</dfn> interface
+      </h2>
+      <p>
+        This interface defines an individual gamepad device.
+      </p>
+      <pre class="idl" data-cite="HR-TIME">
+        [Exposed=Window, SecureContext]
+        interface Gamepad {
+          readonly attribute DOMString id;
+          readonly attribute long index;
+          readonly attribute boolean connected;
+          readonly attribute DOMHighResTimeStamp timestamp;
+          readonly attribute GamepadMappingType mapping;
+          readonly attribute FrozenArray&lt;double&gt; axes;
+          readonly attribute FrozenArray&lt;GamepadButton&gt; buttons;
+          [SameObject] readonly attribute GamepadHapticActuator? vibrationActuator;

To support `pulse()` we probably need to bring back the `hapticActuators` array:

```
readonly attribute FrozenArray&lt;GamepadHapticActuator> hapticActuators;
```

With `pulse()` it's expected that a gamepad could have multiple haptic actuators and each actuator can be controlled independently. With `playEffect()` the `vibrationActuator` member is expected to control multiple actuators. This means there should be overlap between the actuators in `hapticActuators` and `vibrationActuator`. The spec should explain how to handle pre-empting `pulse()` with `playEffect()` and vice versa.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/gamepad/pull/190#pullrequestreview-1730967486
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/gamepad/pull/190/review/1730967486@github.com>