Ensure that OpenVR only adds placeholder buttons when needed.

The current implementation of the OpenVRGamepadHelper always adds the
optional grip and secondary axes buttons; however, if those buttons are
missing and no additional buttons need to be supported, they should not
be included. A prime example of this is the vive controller, which has
a trigger, a grip, and a touchpad, but no secondary axis button.  This
is essentially the controller that the new TestGamepadOptionalData test
builds.

Bug: 964026
Change-Id: I1de93b5bd7bd0d9e75013cf33b8f333e5d70270f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1627914Reviewed-by: Bill Orr <billorr@chromium.org>
Commit-Queue: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662843}

chrome/browser/vr/webxr_vr_input_browser_test.cc

@@ -441,6 +441,16 @@ void TestGamepadMinimumDataImpl(WebXrVrBrowserTestBase* t) {
       t->GetFileUrlForHtmlTestFile("test_webxr_gamepad_support"));
   t->EnterSessionWithUserGestureOrFail();
 
+  // We only actually connect the data for the two buttons, but WMR expects
+  // the WMR controller (which has all of the required and optional buttons)
+  // and so adds dummy/placeholder buttons regardless of what data we send up.
+  std::string button_count = "2";
+  if (t->GetRuntimeType() == XrBrowserTestBase::RuntimeType::RUNTIME_WMR)
+    button_count = "4";
+
+  t->PollJavaScriptBooleanOrFail("isButtonCountEqualTo(" + button_count + ")",
+                                 WebXrVrBrowserTestBase::kPollTimeoutShort);
+
   // Press the trigger and set the axis to a non-zero amount, so we can ensure
   // we aren't getting just default gamepad data.
   my_mock.TogglePrimaryTrigger(controller_index);
@@ -478,8 +488,7 @@ IN_PROC_BROWSER_TEST_F(WebXrVrBrowserTestWMR, TestGamepadMinimumData) {
 void TestGamepadCompleteDataImpl(WebXrVrBrowserTestBase* t) {
   WebXrControllerInputMock my_mock;
 
-  // Create a controller that supports all reserved buttons.  Note that per
-  // third_party/openvr/src/headers/openvr.h, SteamVR_Trigger is Axis1.
+  // Create a controller that supports all reserved buttons.
   uint64_t supported_buttons =
       device::XrButtonMaskFromId(device::XrButtonId::kAxisTrigger) |
       device::XrButtonMaskFromId(device::XrButtonId::kAxisPrimary) |
@@ -519,6 +528,10 @@ void TestGamepadCompleteDataImpl(WebXrVrBrowserTestBase* t) {
   t->PollJavaScriptBooleanOrFail("isMappingEqualTo('xr-standard')",
                                  WebVrBrowserTestBase::kPollTimeoutShort);
 
+  // Controller should have all required and optional xr-standard buttons
+  t->PollJavaScriptBooleanOrFail("isButtonCountEqualTo(4)",
+                                 WebXrVrBrowserTestBase::kPollTimeoutShort);
+
   // The secondary set of axes should be set appropriately.
   t->PollJavaScriptBooleanOrFail("areAxesValuesEqualTo(1, 0.25, -0.25)",
                                  WebVrBrowserTestBase::kPollTimeoutShort);
@@ -583,6 +596,7 @@ IN_PROC_BROWSER_TEST_F(WebXrVrBrowserTestStandard, TestGamepadReservedData) {
   // pressed, and our "extra" button should be index 4 and should be pressed.
   PollJavaScriptBooleanOrFail("isMappingEqualTo('xr-standard')",
                               kPollTimeoutShort);
+  PollJavaScriptBooleanOrFail("isButtonCountEqualTo(5)", kPollTimeoutShort);
   PollJavaScriptBooleanOrFail("isButtonPressedEqualTo(0, true)",
                               kPollTimeoutShort);
   PollJavaScriptBooleanOrFail("isButtonPressedEqualTo(1, true)",
@@ -598,6 +612,44 @@ IN_PROC_BROWSER_TEST_F(WebXrVrBrowserTestStandard, TestGamepadReservedData) {
   EndTest();
 }
 
+// Ensure that if a gamepad has a grip, but not any extra buttons or a secondary
+// axis, that no trailing placeholder button is added.  This is a slight
+// variation on TestGamepadMinimalData, but won't re-test whether or not buttons
+// get sent up.  Note that since WMR always builds the WMR controller which
+// supports all required and optional buttons specified by the xr-standard
+// mapping, this test is OpenVR-only.
+IN_PROC_BROWSER_TEST_F(WebXrVrBrowserTestStandard, TestGamepadOptionalData) {
+  WebXrControllerInputMock my_mock;
+
+  // Create a controller that supports the trigger, primary axis, and grip
+  uint64_t supported_buttons =
+      device::XrButtonMaskFromId(device::XrButtonId::kAxisTrigger) |
+      device::XrButtonMaskFromId(device::XrButtonId::kAxisPrimary) |
+      device::XrButtonMaskFromId(device::XrButtonId::kGrip);
+
+  std::map<device::XrButtonId, unsigned int> axis_types = {
+      {device::XrButtonId::kAxisPrimary, GetPrimaryAxisType()},
+      {device::XrButtonId::kAxisTrigger, device::XrAxisType::kTrigger},
+  };
+
+  my_mock.CreateAndConnectController(
+      device::ControllerRole::kControllerRoleRight, axis_types,
+      supported_buttons);
+
+  LoadUrlAndAwaitInitialization(
+      GetFileUrlForHtmlTestFile("test_webxr_gamepad_support"));
+  EnterSessionWithUserGestureOrFail();
+
+  // There should be enough buttons for an xr-standard mapping, and it should
+  // have one optional button, but not the other.
+  PollJavaScriptBooleanOrFail("isMappingEqualTo('xr-standard')",
+                              kPollTimeoutShort);
+  PollJavaScriptBooleanOrFail("isButtonCountEqualTo(3)", kPollTimeoutShort);
+
+  RunJavaScriptOrFail("done()");
+  EndTest();
+}
+
 void TestControllerInputRegisteredImpl(WebXrVrBrowserTestBase* t) {
   WebXrControllerInputMock my_mock;
 

device/vr/openvr/openvr_gamepad_helper.cc

@@ -244,7 +244,7 @@ class OpenVRGamepadBuilder : public GamepadBuilder {
  public:
   enum class AxesRequirement {
     kOptional = 0,
-    kRequired = 1,
+    kRequireBoth = 1,
   };
 
   OpenVRGamepadBuilder(vr::IVRSystem* vr_system,
@@ -264,13 +264,13 @@ class OpenVRGamepadBuilder : public GamepadBuilder {
 
   ~OpenVRGamepadBuilder() override = default;
 
-  bool TryAddAxesButton(
+  bool TryAddAxesOrTriggerButton(
       vr::EVRButtonId button_id,
       AxesRequirement requirement = AxesRequirement::kOptional) {
     if (!IsInAxesData(button_id))
       return false;
 
-    bool require_axes = (requirement == AxesRequirement::kRequired);
+    bool require_axes = (requirement == AxesRequirement::kRequireBoth);
     if (require_axes && !axes_data_[button_id].has_both_axes)
       return false;
 
@@ -280,13 +280,13 @@ class OpenVRGamepadBuilder : public GamepadBuilder {
     return true;
   }
 
-  bool TryAddNextUnusedAxesButton() {
+  bool TryAddNextUnusedButtonWithAxes() {
     for (const auto& axes_data_pair : axes_data_) {
       vr::EVRButtonId button_id = axes_data_pair.first;
       if (IsUsed(button_id))
         continue;
 
-      if (TryAddAxesButton(button_id, AxesRequirement::kRequired))
+      if (TryAddAxesOrTriggerButton(button_id, AxesRequirement::kRequireBoth))
         return true;
     }
 
@@ -305,11 +305,17 @@ class OpenVRGamepadBuilder : public GamepadBuilder {
   }
 
   // This will add any remaining unused values from axes_data to the gamepad.
-  void AddRemainingAxes() {
+  // Returns a bool indicating whether any additional axes were added.
+  bool AddRemainingTriggersAndAxes() {
+    bool added_axes = false;
     for (const auto& axes_data_pair : axes_data_) {
-      if (!IsUsed(axes_data_pair.first))
+      if (!IsUsed(axes_data_pair.first)) {
+        added_axes = true;
         AddButton(axes_data_pair.second);
+      }
     }
+
+    return added_axes;
   }
 
  private:
@@ -365,30 +371,57 @@ base::Optional<Gamepad> OpenVRGamepadHelper::GetXRGamepad(
   OpenVRGamepadBuilder builder(vr_system, controller_id, controller_state,
                                handedness);
 
-  if (!builder.TryAddAxesButton(vr::k_EButton_SteamVR_Trigger))
+  if (!builder.TryAddAxesOrTriggerButton(vr::k_EButton_SteamVR_Trigger))
     return base::nullopt;
 
-  if (!builder.TryAddNextUnusedAxesButton())
+  if (!builder.TryAddNextUnusedButtonWithAxes())
     return base::nullopt;
 
-  if (!builder.TryAddButton(vr::k_EButton_Grip))
+  bool added_placeholder_grip = false;
+  if (!builder.TryAddButton(vr::k_EButton_Grip)) {
+    added_placeholder_grip = true;
     builder.AddPlaceholderButton();
+  }
 
   // If we can't find any secondary button with an x and y axis, add a fake
   // button.  Note that we're not worried about ensuring that the axes data gets
   // added, because if there were any other axes to add, we would've added them.
-  if (!builder.TryAddNextUnusedAxesButton())
+  bool added_placeholder_axes = false;
+  if (!builder.TryAddNextUnusedButtonWithAxes()) {
+    added_placeholder_axes = true;
     builder.AddPlaceholderButton();
+  }
 
   // Now that all of the xr-standard reserved buttons have been filled in, we
   // add the rest of the buttons in order of decreasing importance.
   // First add regular buttons
+  bool added_optional_buttons = false;
   for (const auto& button : kWebXRButtonOrder) {
-    builder.TryAddButton(button);
+    added_optional_buttons =
+        builder.TryAddButton(button) || added_optional_buttons;
   }
 
   // Finally, add any remaining axis buttons (triggers/josysticks/touchpads)
-  builder.AddRemainingAxes();
+  bool added_optional_axes = builder.AddRemainingTriggersAndAxes();
+
+  // If we didn't add any optional buttons, we need to remove our placeholder
+  // buttons.
+  if (!(added_optional_buttons || added_optional_axes)) {
+    // If we didn't add any optional buttons, see if we need to remove the most
+    // recent placeholder (the secondary axes).
+    // Note that if we added a placeholder axes, the only optional axes that
+    // should have been added are triggers, and so we don't need to worry about
+    // the order
+    if (added_placeholder_axes) {
+      builder.RemovePlaceholderButton();
+
+      // Only if the axes button was a placeholder can we remove the grip
+      // if it was also a placeholder.
+      if (added_placeholder_grip) {
+        builder.RemovePlaceholderButton();
+      }
+    }
+  }
 
   return builder.GetGamepad();
 }

device/vr/util/gamepad_builder.cc

@@ -118,6 +118,18 @@ void GamepadBuilder::AddPlaceholderButton() {
   AddButton(GamepadButton());
 }
 
+void GamepadBuilder::RemovePlaceholderButton() {
+  // Since this is a member array, it actually is full of default constructed
+  // buttons, so all we have to do to remove a button is decrement the length
+  // variable.  However, we should check before we do so that we actually have
+  // a length and that there's not any data that's been set in the alleged
+  // placeholder button.
+  DCHECK_GT(gamepad_.buttons_length, 0u);
+  GamepadButton button = gamepad_.buttons[gamepad_.buttons_length - 1];
+  DCHECK(!button.pressed && !button.touched && button.value == 0);
+  gamepad_.buttons_length--;
+}
+
 double GamepadBuilder::ApplyAxisDeadzoneToValue(double value) const {
   return std::fabs(value) < axis_deadzone_ ? 0 : value;
 }

device/vr/util/gamepad_builder.h

@@ -41,6 +41,7 @@ class GamepadBuilder {
   void AddButton(const ButtonData& data);
   void AddAxis(double value);
   void AddPlaceholderButton();
+  void RemovePlaceholderButton();
 
  protected:
   void AddAxes(const ButtonData& data);