Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(147)

Issue 12260011: Support DirectInput gamepads on Windows. (Closed)

Created:
7 years, 10 months ago by teravest
Modified:
7 years, 10 months ago
Reviewers:
brettw, scottmg
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dmichael (off chromium)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Support DirectInput gamepads on Windows. This change adds generic support for gamepads which support DirectInput but do not support XInput. It only adds support for recognized controllers to prevent gamepads from only partially working. This change only adds explicit support for the "DragonRise Generic USB" controller but it should be easy to add follow-on patches for different controllers (as is done for Linux and Mac). Tested: I tested this change with a DirectInput controller, with and without an XInput controller also connected, and ensured that the XInput controller's desired ID took precedence. All buttons and axes appear to work fine. BUG=crbug.com/110013 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183689

Patch Set 1 #

Patch Set 2 : #

Total comments: 15

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 33

Patch Set 6 : #

Total comments: 3

Patch Set 7 : Preserve controller index #

Total comments: 2

Patch Set 8 : #

Total comments: 13

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : DO NOT SUBMIT, weird build break after rebase #

Patch Set 13 : Build fix: defined DIDFT_OPTIONAL #

Patch Set 14 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+545 lines, -70 lines) Patch
M content/browser/gamepad/gamepad_platform_data_fetcher_win.h View 1 2 3 4 5 6 5 chunks +44 lines, -1 line 0 comments Download
M content/browser/gamepad/gamepad_platform_data_fetcher_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +393 lines, -69 lines 0 comments Download
A content/browser/gamepad/gamepad_standard_mappings_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +105 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (0 generated)
teravest
7 years, 10 months ago (2013-02-13 18:42:19 UTC) #1
scottmg
Thanks for taking this on! Quick high-level review so far. Adding cpu to review the ...
7 years, 10 months ago (2013-02-13 19:19:07 UTC) #2
teravest
Thanks for the quick response! https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc#newcode87 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:87: vendorstream << std::setw(4) << ...
7 years, 10 months ago (2013-02-13 21:59:21 UTC) #3
teravest
The patch now directly links for DirectInput functions.
7 years, 10 months ago (2013-02-13 22:45:41 UTC) #4
scottmg
https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc#newcode7 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:7: #define DIRECTINPUT_VERSION 0x0800 this is defined twice (also in ...
7 years, 10 months ago (2013-02-14 04:33:22 UTC) #5
teravest1
I was surprised that testing Gamepad support still continued to work, even with the bug ...
7 years, 10 months ago (2013-02-14 16:32:16 UTC) #6
scottmg
On 2013/02/14 16:32:16, teravest_google.com wrote: > I was surprised that testing Gamepad support still continued ...
7 years, 10 months ago (2013-02-14 16:44:27 UTC) #7
teravest
On Thu, Feb 14, 2013 at 9:44 AM, <scottmg@chromium.org> wrote: > On 2013/02/14 16:32:16, teravest_google.com ...
7 years, 10 months ago (2013-02-14 16:52:01 UTC) #8
scottmg
On 2013/02/14 16:52:01, teravest wrote: > > Hmm. If we want to do this, I ...
7 years, 10 months ago (2013-02-14 16:57:04 UTC) #9
teravest
Added support for the F310 in DirectInput mode. Surprisingly, it has the exact same mappings ...
7 years, 10 months ago (2013-02-14 17:52:18 UTC) #10
scottmg
On 2013/02/14 17:52:18, teravest wrote: looking good, glad to not have to squint too hard ...
7 years, 10 months ago (2013-02-14 18:44:27 UTC) #11
scottmg
https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc#newcode206 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:206: DIRECTINPUT_VERSION, nit, parameter alignment https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc#newcode378 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:378: return; i think ...
7 years, 10 months ago (2013-02-14 18:44:33 UTC) #12
teravest
On 2013/02/14 18:44:27, scottmg wrote: > On 2013/02/14 17:52:18, teravest wrote: > > looking good, ...
7 years, 10 months ago (2013-02-14 21:09:38 UTC) #13
teravest
OK, controller indices are preserved. I've played around plugging and unplugging an XInput and DirectInput ...
7 years, 10 months ago (2013-02-14 23:26:43 UTC) #14
scottmg
super! let me know when you're happy and i'll do a final pass. https://codereview.chromium.org/12260011/diff/17001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc File ...
7 years, 10 months ago (2013-02-14 23:31:58 UTC) #15
teravest
I think the logic for releasing references on DirectInput controllers should be good now. https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc ...
7 years, 10 months ago (2013-02-19 17:17:24 UTC) #16
scottmg
Awesome! Just some whiny nits, and lgtm. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc#newcode107 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:107: EnumDevicesContext* ctxt ...
7 years, 10 months ago (2013-02-19 18:50:51 UTC) #17
teravest
https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc#newcode107 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:107: EnumDevicesContext* ctxt = (EnumDevicesContext*) context; On 2013/02/19 18:50:52, scottmg ...
7 years, 10 months ago (2013-02-19 20:53:37 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/12260011/28002
7 years, 10 months ago (2013-02-20 14:45:24 UTC) #19
commit-bot: I haz the power
Failed to apply patch for content/browser/gamepad/gamepad_platform_data_fetcher_win.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 10 months ago (2013-02-20 14:45:26 UTC) #20
teravest
After rebasing, I've hit something strange where DIDFT_OPTIONAL is now undefined, and I get build ...
7 years, 10 months ago (2013-02-20 21:46:21 UTC) #21
scottmg
On 2013/02/20 21:46:21, teravest wrote: > After rebasing, I've hit something strange where DIDFT_OPTIONAL is ...
7 years, 10 months ago (2013-02-20 21:59:34 UTC) #22
teravest1
On Wed, Feb 20, 2013 at 2:59 PM, <scottmg@chromium.org> wrote: > On 2013/02/20 21:46:21, teravest ...
7 years, 10 months ago (2013-02-20 22:23:14 UTC) #23
scottmg
On 2013/02/20 22:23:14, teravest_google.com wrote: > On Wed, Feb 20, 2013 at 2:59 PM, <mailto:scottmg@chromium.org> ...
7 years, 10 months ago (2013-02-20 22:31:23 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/12260011/41001
7 years, 10 months ago (2013-02-20 23:00:09 UTC) #25
commit-bot: I haz the power
Presubmit check for 12260011-41001 failed and returned exit status 1. INFO:root:Found 4 file(s). Running presubmit ...
7 years, 10 months ago (2013-02-20 23:00:16 UTC) #26
teravest
Adding brettw for the gypi change in content/.
7 years, 10 months ago (2013-02-20 23:01:58 UTC) #27
brettw
content gypi lgtm
7 years, 10 months ago (2013-02-20 23:04:42 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/12260011/41001
7 years, 10 months ago (2013-02-20 23:06:32 UTC) #29
commit-bot: I haz the power
7 years, 10 months ago (2013-02-21 01:09:27 UTC) #30
Message was sent while issue was closed.
Change committed as 183689

Powered by Google App Engine
This is Rietveld 408576698