|
|
Created:
7 years, 10 months ago by teravest Modified:
7 years, 10 months ago 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. |
DescriptionSupport 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 : #
Messages
Total messages: 30 (0 generated)
Thanks for taking this on! Quick high-level review so far. Adding cpu to review the COM-specific bits in content/browser/gamepad/gamepad_platform_data_fetcher_win.cc because I am very COM-unknowledgeable. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:87: vendorstream << std::setw(4) << std::setfill('0') << std::hex << vendorData; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:123: // "global" joydata2 structure from directx. This one is smaller anyway. What? Why? https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:209: SetDirectInputDeadZone(gamepad, 1000); Where did this 1000 come from? https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:234: hr = CoInitialize(NULL); We shouldn't be doing this here. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:242: (void**)&pIWbemLocator); Please fix all the identifiers to match Chromium style. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:423: return; nit: remove https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:570: directinput_create_ = static_cast<DirectInputCreateFunc>( According to http://msdn.microsoft.com/en-us/library/windows/desktop/ee416805(v=vs.85).aspx dinput has shipped and hasn't changed since xpsp2, so there's no reason not to link directly, rather than doing late binding. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_win.h (right): https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.h:29: //struct IDirectInputDevice8; nit: delete these
Thanks for the quick response! https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:87: vendorstream << std::setw(4) << std::setfill('0') << std::hex << vendorData; On 2013/02/13 19:19:07, scottmg wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... Done. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:123: // "global" joydata2 structure from directx. This one is smaller anyway. On 2013/02/13 19:19:07, scottmg wrote: > What? Why? I didn't want to muck around with how we were linking in DirectX, and the linker couldn't resolve c_dfDIJoystick2. It's possible that some gamepads won't have their axes mapped well into that structure anyway, so I think defining our own format here is reasonable. I've updated the comment. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:209: SetDirectInputDeadZone(gamepad, 1000); On 2013/02/13 19:19:07, scottmg wrote: > Where did this 1000 come from? I've added a comment here explaining what this is. Admittedly, 10% is a rough guess at what's a reasonable space for a dead zone on an axis. XInput uses by default 12% on the left stick and 13.3% on the right stick. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:234: hr = CoInitialize(NULL); On 2013/02/13 19:19:07, scottmg wrote: > We shouldn't be doing this here. I've changed this to use a ScopedCOMInitializer. Or are you suggesting this shouldn't happen at all? I think I need to do this to call CoCreateInstance() below. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:242: (void**)&pIWbemLocator); On 2013/02/13 19:19:07, scottmg wrote: > Please fix all the identifiers to match Chromium style. Done. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:423: return; On 2013/02/13 19:19:07, scottmg wrote: > nit: remove Done. https://codereview.chromium.org/12260011/diff/2001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:570: directinput_create_ = static_cast<DirectInputCreateFunc>( On 2013/02/13 19:19:07, scottmg wrote: > According to > http://msdn.microsoft.com/en-us/library/windows/desktop/ee416805%28v=vs.85%29... > dinput has shipped and hasn't changed since xpsp2, so there's no reason not to > link directly, rather than doing late binding. Cool. I'm still trying to figure out how to do this, I haven't been able to link directly yet, but I'll play with the gyp files some more.
The patch now directly links for DirectInput functions.
https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:7: #define DIRECTINPUT_VERSION 0x0800 this is defined twice (also in the header). https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:8: #ifndef _WIN32_DCOM i'm assuming this is here for some reason, but i don't know what it is. could you document? https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:194: wcscpy_s(device.id, WebGamepad::idLengthCap, instance->tszInstanceName); i think this should be tszProductName not tszInstanceName https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:200: device.mapper = GetGamepadStandardMappingFunction(vendor, product); if there's a mapper to standard gamepad style, the .id field needs to include "STANDARD GAMEPAD". https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#remapping https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:210: base::win::ScopedCOMInitializer com_initializer; cpu can confirm, but I think this should only be done once per-thread, not everywhere. we probably don't want it scoped to this function, in any case. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:212: return; return on success? https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:226: base::win::ScopedBstr name_space(L"\\\\.\\root\\cimv2"); what a lovely api they've cooked up here. :P https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:250: unsigned long devices_returned; i think keep this as ULONG since it's primarily interacting with the api https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for (unsigned i = 0; i < devices_returned; i++) { ULONG, or at least match type above https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:275: if (strVid && swscanf(strVid, L"VID_%4X", &dwVid) != 1) should these be <= 0? https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:282: // Add the VID/PID to a linked list not a linked list https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:311: for (unsigned i = 0; i < WebGamepads::itemsLengthCap; ++i) { this approach changes what happens on insert and remove i think. currently xinput guarantees that the indices won't change, but i think if there's 1 xinput and 1 directinput device, and the xinput one is removed, then "player 2" will all of a sudden become "player 1". https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:327: struct EnumDevicesContext ctxt; at least "context" or enum_devices_context if it's not too awkward. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:345: if (pad_state_[pad_state_index].status != DISCONNECTED) { i think it would be clearer to have a local = &pad_state_[pad_state_index] for the assignments below. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:471: while ((hr = gamepad->Acquire()) == DIERR_INPUTLOST) { } this loop worries me. is there any guarantee we won't get stuck here? one possibility: instead call Poll once in Enumerate, and then attempt reacquisition (once) there if the Poll fails? https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:486: // We map the POV (often a D-pad) into the buttons 16-19. this seems like something that should go into the mappers instead, is there a good reason to have it here for all devices? https://codereview.chromium.org/12260011/diff/11001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12260011/diff/11001/content/content_browser.g... content/content_browser.gypi:999: '-lcomctl32.lib', bah, why do these have -l? Could you remove all the '-l's from the list since you're here please?
I was surprised that testing Gamepad support still continued to work, even with the bug at the top of FillXInputDeviceList()! I think we can get rid of the FillXInputDeviceList() business entirely; since we do a PID/VID lookup, we can just restrict ourselves to only provide mappers for controllers that are DirectInput-only. What do you think? On Wed, Feb 13, 2013 at 9:33 PM, <scottmg@chromium.org> wrote: > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc > (right): > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:7: #define > DIRECTINPUT_VERSION 0x0800 > this is defined twice (also in the header). > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:8: #ifndef > _WIN32_DCOM > i'm assuming this is here for some reason, but i don't know what it is. > could you document? > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:194: > wcscpy_s(device.id, WebGamepad::idLengthCap, instance->tszInstanceName); > i think this should be tszProductName not tszInstanceName > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:200: > device.mapper = GetGamepadStandardMappingFunction(vendor, product); > if there's a mapper to standard gamepad style, the .id field needs to > include "STANDARD GAMEPAD". > https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#remapping > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:210: > base::win::ScopedCOMInitializer com_initializer; > cpu can confirm, but I think this should only be done once per-thread, > not everywhere. we probably don't want it scoped to this function, in > any case. > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:212: > return; > return on success? > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:226: > base::win::ScopedBstr name_space(L"\\\\.\\root\\cimv2"); > what a lovely api they've cooked up here. :P > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:250: > unsigned long devices_returned; > i think keep this as ULONG since it's primarily interacting with the api > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for > (unsigned i = 0; i < devices_returned; i++) { > ULONG, or at least match type above > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:275: if > (strVid && swscanf(strVid, L"VID_%4X", &dwVid) != 1) > should these be <= 0? > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:282: // Add > the VID/PID to a linked list > not a linked list > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:311: for > (unsigned i = 0; i < WebGamepads::itemsLengthCap; ++i) { > this approach changes what happens on insert and remove i think. > currently xinput guarantees that the indices won't change, but i think > if there's 1 xinput and 1 directinput device, and the xinput one is > removed, then "player 2" will all of a sudden become "player 1". > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:327: struct > EnumDevicesContext ctxt; > at least "context" or enum_devices_context if it's not too awkward. > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:345: if > (pad_state_[pad_state_index].status != DISCONNECTED) { > i think it would be clearer to have a local = > &pad_state_[pad_state_index] for the assignments below. > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:471: while > ((hr = gamepad->Acquire()) == DIERR_INPUTLOST) { } > this loop worries me. is there any guarantee we won't get stuck here? > > one possibility: instead call Poll once in Enumerate, and then attempt > reacquisition (once) there if the Poll fails? > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:486: // We > map the POV (often a D-pad) into the buttons 16-19. > this seems like something that should go into the mappers instead, is > there a good reason to have it here for all devices? > > https://codereview.chromium.org/12260011/diff/11001/content/content_browser.gypi > File content/content_browser.gypi (right): > > https://codereview.chromium.org/12260011/diff/11001/content/content_browser.g... > content/content_browser.gypi:999: '-lcomctl32.lib', > bah, why do these have -l? Could you remove all the '-l's from the list > since you're here please? > > https://codereview.chromium.org/12260011/
On 2013/02/14 16:32:16, teravest_google.com wrote: > I was surprised that testing Gamepad support still continued to work, even with > the bug at the top of FillXInputDeviceList()! > > I think we can get rid of the FillXInputDeviceList() business entirely; since > we do a PID/VID lookup, we can just restrict ourselves to only provide mappers > for controllers that are DirectInput-only. > > What do you think? it'd be nice to get rid of that code for sure. the logitech devices have a switch to go from XInput <-> DirectInput mode, so as long as it presents different pid/vid when it's switched, that seems like it'd work to me. (i believe it does, but all the logitech gamepads seem to have wandered out of my office) in the process of doing that, if possible, it'd be nice to have another state DIRECTINPUT_UNRECOGNIZED (or whatever) so that we have the ability to add explanatory ui at some point in the future (or maybe even the crowdsourcing thing you mentioned before) > > On Wed, Feb 13, 2013 at 9:33 PM, <mailto:scottmg@chromium.org> wrote: > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc > > (right): > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:7: #define > > DIRECTINPUT_VERSION 0x0800 > > this is defined twice (also in the header). > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:8: #ifndef > > _WIN32_DCOM > > i'm assuming this is here for some reason, but i don't know what it is. > > could you document? > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:194: > > wcscpy_s(device.id, WebGamepad::idLengthCap, instance->tszInstanceName); > > i think this should be tszProductName not tszInstanceName > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:200: > > device.mapper = GetGamepadStandardMappingFunction(vendor, product); > > if there's a mapper to standard gamepad style, the .id field needs to > > include "STANDARD GAMEPAD". > > https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#remapping > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:210: > > base::win::ScopedCOMInitializer com_initializer; > > cpu can confirm, but I think this should only be done once per-thread, > > not everywhere. we probably don't want it scoped to this function, in > > any case. > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:212: > > return; > > return on success? > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:226: > > base::win::ScopedBstr name_space(L"\\\\.\\root\\cimv2"); > > what a lovely api they've cooked up here. :P > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:250: > > unsigned long devices_returned; > > i think keep this as ULONG since it's primarily interacting with the api > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for > > (unsigned i = 0; i < devices_returned; i++) { > > ULONG, or at least match type above > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:275: if > > (strVid && swscanf(strVid, L"VID_%4X", &dwVid) != 1) > > should these be <= 0? > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:282: // Add > > the VID/PID to a linked list > > not a linked list > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:311: for > > (unsigned i = 0; i < WebGamepads::itemsLengthCap; ++i) { > > this approach changes what happens on insert and remove i think. > > currently xinput guarantees that the indices won't change, but i think > > if there's 1 xinput and 1 directinput device, and the xinput one is > > removed, then "player 2" will all of a sudden become "player 1". > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:327: struct > > EnumDevicesContext ctxt; > > at least "context" or enum_devices_context if it's not too awkward. > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:345: if > > (pad_state_[pad_state_index].status != DISCONNECTED) { > > i think it would be clearer to have a local = > > &pad_state_[pad_state_index] for the assignments below. > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:471: while > > ((hr = gamepad->Acquire()) == DIERR_INPUTLOST) { } > > this loop worries me. is there any guarantee we won't get stuck here? > > > > one possibility: instead call Poll once in Enumerate, and then attempt > > reacquisition (once) there if the Poll fails? > > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... > > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:486: // We > > map the POV (often a D-pad) into the buttons 16-19. > > this seems like something that should go into the mappers instead, is > > there a good reason to have it here for all devices? > > > > > https://codereview.chromium.org/12260011/diff/11001/content/content_browser.gypi > > File content/content_browser.gypi (right): > > > > > https://codereview.chromium.org/12260011/diff/11001/content/content_browser.g... > > content/content_browser.gypi:999: '-lcomctl32.lib', > > bah, why do these have -l? Could you remove all the '-l's from the list > > since you're here please? > > > > https://codereview.chromium.org/12260011/
On Thu, Feb 14, 2013 at 9:44 AM, <scottmg@chromium.org> wrote: > On 2013/02/14 16:32:16, teravest_google.com wrote: >> >> I was surprised that testing Gamepad support still continued to work, even > > with >> >> the bug at the top of FillXInputDeviceList()! > > >> I think we can get rid of the FillXInputDeviceList() business entirely; >> since >> we do a PID/VID lookup, we can just restrict ourselves to only provide >> mappers >> for controllers that are DirectInput-only. > > >> What do you think? > > > it'd be nice to get rid of that code for sure. the logitech devices have a > switch to go from XInput <-> DirectInput mode, so as long as it presents > different pid/vid when it's switched, that seems like it'd work to me. (i > believe it does, but all the logitech gamepads seem to have wandered out of > my > office) I've got a Logitech F310 handy. It does switch pid/vid, I just tested this myself. It even reports a different product name. XInput: vendor 046d, product c21d, product_name "Gamepad F310 (Controller)" DirectInput: vendor 046d, product c216, product_name "Logitech Dual Action". I don't know if we can depend on all XInput devices acting this way, but it'd sure be nice. > > in the process of doing that, if possible, it'd be nice to have another > state > DIRECTINPUT_UNRECOGNIZED (or whatever) so that we have the ability to add > explanatory ui at some point in the future (or maybe even the crowdsourcing > thing you mentioned before) Hmm. If we want to do this, I guess we should keep FillXInputDeviceList, then, so we don't mark XInput devices as DIRECTINPUT_UNRECOGNIZED. > > > >> On Wed, Feb 13, 2013 at 9:33 PM, <mailto:scottmg@chromium.org> wrote: >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc >> > (right): >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:7: #define >> > DIRECTINPUT_VERSION 0x0800 >> > this is defined twice (also in the header). >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:8: #ifndef >> > _WIN32_DCOM >> > i'm assuming this is here for some reason, but i don't know what it is. >> > could you document? >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:194: >> > wcscpy_s(device.id, WebGamepad::idLengthCap, instance->tszInstanceName); >> > i think this should be tszProductName not tszInstanceName >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:200: >> > device.mapper = GetGamepadStandardMappingFunction(vendor, product); >> > if there's a mapper to standard gamepad style, the .id field needs to >> > include "STANDARD GAMEPAD". >> > https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#remapping >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:210: >> > base::win::ScopedCOMInitializer com_initializer; >> > cpu can confirm, but I think this should only be done once per-thread, >> > not everywhere. we probably don't want it scoped to this function, in >> > any case. >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:212: >> > return; >> > return on success? >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:226: >> > base::win::ScopedBstr name_space(L"\\\\.\\root\\cimv2"); >> > what a lovely api they've cooked up here. :P >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:250: >> > unsigned long devices_returned; >> > i think keep this as ULONG since it's primarily interacting with the api >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for >> > (unsigned i = 0; i < devices_returned; i++) { >> > ULONG, or at least match type above >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:275: if >> > (strVid && swscanf(strVid, L"VID_%4X", &dwVid) != 1) >> > should these be <= 0? >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:282: // Add >> > the VID/PID to a linked list >> > not a linked list >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:311: for >> > (unsigned i = 0; i < WebGamepads::itemsLengthCap; ++i) { >> > this approach changes what happens on insert and remove i think. >> > currently xinput guarantees that the indices won't change, but i think >> > if there's 1 xinput and 1 directinput device, and the xinput one is >> > removed, then "player 2" will all of a sudden become "player 1". >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:327: struct >> > EnumDevicesContext ctxt; >> > at least "context" or enum_devices_context if it's not too awkward. >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:345: if >> > (pad_state_[pad_state_index].status != DISCONNECTED) { >> > i think it would be clearer to have a local = >> > &pad_state_[pad_state_index] for the assignments below. >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:471: while >> > ((hr = gamepad->Acquire()) == DIERR_INPUTLOST) { } >> > this loop worries me. is there any guarantee we won't get stuck here? >> > >> > one possibility: instead call Poll once in Enumerate, and then attempt >> > reacquisition (once) there if the Poll fails? >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... >> >> > content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:486: // We >> > map the POV (often a D-pad) into the buttons 16-19. >> > this seems like something that should go into the mappers instead, is >> > there a good reason to have it here for all devices? >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/content_browser.gypi >> >> > File content/content_browser.gypi (right): >> > >> > > > > https://codereview.chromium.org/12260011/diff/11001/content/content_browser.g... >> >> > content/content_browser.gypi:999: '-lcomctl32.lib', >> > bah, why do these have -l? Could you remove all the '-l's from the list >> > since you're here please? >> > >> > https://codereview.chromium.org/12260011/ > > > > > https://codereview.chromium.org/12260011/
On 2013/02/14 16:52:01, teravest wrote: > > Hmm. If we want to do this, I guess we should keep > FillXInputDeviceList, then, so we don't mark XInput devices as > DIRECTINPUT_UNRECOGNIZED. Ah, right, never mind then. I was just thinking it might be easy to do now, but I think it better to delete that stuff for now and we can add it later if we want attack the larger problem.
Added support for the F310 in DirectInput mode. Surprisingly, it has the exact same mappings as the DragonRise controller. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:7: #define DIRECTINPUT_VERSION 0x0800 On 2013/02/14 04:33:22, scottmg wrote: > this is defined twice (also in the header). Removed. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:8: #ifndef _WIN32_DCOM On 2013/02/14 04:33:22, scottmg wrote: > i'm assuming this is here for some reason, but i don't know what it is. could > you document? This was present in the DirectX example that FillXInputData() is from. Removing it seems safe, so it's gone now. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:194: wcscpy_s(device.id, WebGamepad::idLengthCap, instance->tszInstanceName); On 2013/02/14 04:33:22, scottmg wrote: > i think this should be tszProductName not tszInstanceName Done. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:200: device.mapper = GetGamepadStandardMappingFunction(vendor, product); On 2013/02/14 04:33:22, scottmg wrote: > if there's a mapper to standard gamepad style, the .id field needs to include > "STANDARD GAMEPAD". > https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#remapping Done. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:210: base::win::ScopedCOMInitializer com_initializer; On 2013/02/14 04:33:22, scottmg wrote: > cpu can confirm, but I think this should only be done once per-thread, not > everywhere. we probably don't want it scoped to this function, in any case. Deleted FillXInputDeviceList(). https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:212: return; On 2013/02/14 04:33:22, scottmg wrote: > return on success? Deleted FillXInputDeviceList(). https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:250: unsigned long devices_returned; On 2013/02/14 04:33:22, scottmg wrote: > i think keep this as ULONG since it's primarily interacting with the api Deleted FillXInputDeviceList(). https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for (unsigned i = 0; i < devices_returned; i++) { On 2013/02/14 04:33:22, scottmg wrote: > ULONG, or at least match type above Deleted FillXInputDeviceList(). https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:275: if (strVid && swscanf(strVid, L"VID_%4X", &dwVid) != 1) On 2013/02/14 04:33:22, scottmg wrote: > should these be <= 0? Deleted FillXInputDeviceList(). https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:282: // Add the VID/PID to a linked list On 2013/02/14 04:33:22, scottmg wrote: > not a linked list Stale comment removed. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:311: for (unsigned i = 0; i < WebGamepads::itemsLengthCap; ++i) { On 2013/02/14 04:33:22, scottmg wrote: > this approach changes what happens on insert and remove i think. currently > xinput guarantees that the indices won't change, but i think if there's 1 xinput > and 1 directinput device, and the xinput one is removed, then "player 2" will > all of a sudden become "player 1". I figured that XInput devices should always take precedence (I think some display the controller number they think they are). In that case, if a DirectInput controller is "Player 1", and someone plugins in a XInput controller, then "Player 1" will suddenly become "Player 2". In fact, it's even worse than that; adding a DirectInput device can change the enumeration order and renumber the controllers too. In light of that, does it make sense to try to make the ordering stable when a controller is removed? https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:327: struct EnumDevicesContext ctxt; On 2013/02/14 04:33:22, scottmg wrote: > at least "context" or enum_devices_context if it's not too awkward. Done. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:345: if (pad_state_[pad_state_index].status != DISCONNECTED) { On 2013/02/14 04:33:22, scottmg wrote: > i think it would be clearer to have a local = &pad_state_[pad_state_index] for > the assignments below. Done. https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:471: while ((hr = gamepad->Acquire()) == DIERR_INPUTLOST) { } On 2013/02/14 04:33:22, scottmg wrote: > this loop worries me. is there any guarantee we won't get stuck here? > > one possibility: instead call Poll once in Enumerate, and then attempt > reacquisition (once) there if the Poll fails? I like that better. Reading through the docs, Acquire() can't return DIERR_INPUTLOST anyway. It's clear I should trust the DirectX samples less than I have been. :( https://codereview.chromium.org/12260011/diff/11001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:486: // We map the POV (often a D-pad) into the buttons 16-19. On 2013/02/14 04:33:22, scottmg wrote: > this seems like something that should go into the mappers instead, is there a > good reason to have it here for all devices? The range of legal values for a POV should always be the same (0..36000). Testing on the F310, it uses the same mappings there. If you'd like, I can put this value as an "axis" in the raw gamepad and do the work in the mappers instead, though. https://codereview.chromium.org/12260011/diff/11001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12260011/diff/11001/content/content_browser.g... content/content_browser.gypi:999: '-lcomctl32.lib', On 2013/02/14 04:33:22, scottmg wrote: > bah, why do these have -l? Could you remove all the '-l's from the list since > you're here please? Removing the -l causes link errors: LINK : fatal error LNK1181: cannot open input file '../../content/comctl32.lib' I don't know a ton about the build system, so I'll leave these as is.
On 2013/02/14 17:52:18, teravest wrote: looking good, glad to not have to squint too hard at that WMI/COM stuff (for now :). removing cpu. > > this approach changes what happens on insert and remove i think. currently > > xinput guarantees that the indices won't change, but i think if there's 1 > xinput > > and 1 directinput device, and the xinput one is removed, then "player 2" will > > all of a sudden become "player 1". > > I figured that XInput devices should always take precedence (I think some > display the controller number they think they are). > > In that case, if a DirectInput controller is "Player 1", and someone plugins in > a XInput controller, then "Player 1" will suddenly become "Player 2". > > In fact, it's even worse than that; adding a DirectInput device can change the > enumeration order and renumber the controllers too. > > In light of that, does it make sense to try to make the ordering stable when a > controller is removed? this is my only concern now, and i think making this behaviour sane is important. the cases you describe are certainly a problem too. ref: https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#widl-Gamepad-index the key point is to not modify the index of a connected device. would it be workable to have connected devices maintain an index while connected?
https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamep... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamep... 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/gamep... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:378: return; i think these failures need to set pad->connected = false, or if you think it's sensible to repeatedly retry, then buttonsLength and axesLength need to be zeroed.
On 2013/02/14 18:44:27, scottmg wrote: > On 2013/02/14 17:52:18, teravest wrote: > > looking good, glad to not have to squint too hard at that WMI/COM stuff (for now > :). removing cpu. > > > > this approach changes what happens on insert and remove i think. currently > > > xinput guarantees that the indices won't change, but i think if there's 1 > > xinput > > > and 1 directinput device, and the xinput one is removed, then "player 2" > will > > > all of a sudden become "player 1". > > > > I figured that XInput devices should always take precedence (I think some > > display the controller number they think they are). > > > > In that case, if a DirectInput controller is "Player 1", and someone plugins > in > > a XInput controller, then "Player 1" will suddenly become "Player 2". > > > > In fact, it's even worse than that; adding a DirectInput device can change the > > enumeration order and renumber the controllers too. > > > > In light of that, does it make sense to try to make the ordering stable when a > > controller is removed? > > this is my only concern now, and i think making this behaviour sane is > important. the cases you describe are certainly a problem too. ref: > https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#widl-Gamepad-index > > the key point is to not modify the index of a connected device. would it be > workable to have connected devices maintain an index while connected? I'll hack something together to do this. One downside is that the ID shown on the XInput device may mismatch the gamepad index, but I guess that's pretty minor.
OK, controller indices are preserved. I've played around plugging and unplugging an XInput and DirectInput controller, and they keep their IDs correctly. This was easier to implement than I thought at first. I'm going to take a longer look at the DirectInput code to make sure those created devices aren't leaked-- it seems like those need to be cleaned up somehow.
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/g... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/17001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for (size_t i = 0; i <= kMaxXInputIndex; ++i) { < XUSER_MAX_COUNT (and remove constant)
I think the logic for releasing references on DirectInput controllers should be good now. https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamep... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/7/content/browser/gamepad/gamep... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:378: return; On 2013/02/14 18:44:34, scottmg wrote: > i think these failures need to set pad->connected = false, or if you think it's > sensible to repeatedly retry, then buttonsLength and axesLength need to be > zeroed. Done. https://codereview.chromium.org/12260011/diff/17001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/17001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:259: for (size_t i = 0; i <= kMaxXInputIndex; ++i) { On 2013/02/14 23:31:59, scottmg wrote: > < XUSER_MAX_COUNT (and remove constant) Done.
Awesome! Just some whiny nits, and lgtm. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:107: EnumDevicesContext* ctxt = (EnumDevicesContext*) context; reinterpret_cast https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:211: (void **)&directinput_interface_, reinterpret_cast<void**>(...) https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:242: const GUID &guid) const { nit; GUID& https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:267: WebGamepad &pad = pads->items[pad_index]; nit; WebGamepad& https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:293: WebGamepad &pad = pads->items[pad_index]; nit; WebGamepad& https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:296: PadState &state = pad_state_[pad_index]; nit; PadState& https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_standard_mappings_win.cc (right): https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_standard_mappings_win.cc:62: { "0079", "0006", MapperDragonRiseGeneric }, // DragonRise Generic USB nit; two spaces after code before comment
https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right): https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:107: EnumDevicesContext* ctxt = (EnumDevicesContext*) context; On 2013/02/19 18:50:52, scottmg wrote: > reinterpret_cast Done. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:211: (void **)&directinput_interface_, On 2013/02/19 18:50:52, scottmg wrote: > reinterpret_cast<void**>(...) Done. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:267: WebGamepad &pad = pads->items[pad_index]; On 2013/02/19 18:50:52, scottmg wrote: > nit; WebGamepad& Done. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:293: WebGamepad &pad = pads->items[pad_index]; On 2013/02/19 18:50:52, scottmg wrote: > nit; WebGamepad& Done. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:296: PadState &state = pad_state_[pad_index]; On 2013/02/19 18:50:52, scottmg wrote: > nit; PadState& Done. https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_standard_mappings_win.cc (right): https://codereview.chromium.org/12260011/diff/20001/content/browser/gamepad/g... content/browser/gamepad/gamepad_standard_mappings_win.cc:62: { "0079", "0006", MapperDragonRiseGeneric }, // DragonRise Generic USB On 2013/02/19 18:50:52, scottmg wrote: > nit; two spaces after code before comment Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/12260011/28002
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 content/browser/gamepad/gamepad_platform_data_fetcher_win.cc Hunk #3 FAILED at 52. 1 out of 3 hunks FAILED -- saving rejects to file content/browser/gamepad/gamepad_platform_data_fetcher_win.cc.rej Patch: content/browser/gamepad/gamepad_platform_data_fetcher_win.cc Index: content/browser/gamepad/gamepad_platform_data_fetcher_win.cc diff --git a/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc b/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc index 838a626d0184b093df64cb1ba6a2269dd9060022..1cee715478a6bf168246d6340354c8e61de50f49 100644 --- a/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc +++ b/content/browser/gamepad/gamepad_platform_data_fetcher_win.cc @@ -4,7 +4,11 @@ #include "content/browser/gamepad/gamepad_platform_data_fetcher_win.h" +#include <dinput.h> +#include <dinputd.h> + #include "base/debug/trace_event.h" +#include "base/stringprintf.h" #include "content/common/gamepad_messages.h" #include "content/common/gamepad_hardware_buffer.h" @@ -28,7 +32,7 @@ static const BYTE kDeviceSubTypeDrumKit = 8; static const BYTE kDeviceSubTypeGuitarBass = 11; static const BYTE kDeviceSubTypeArcadePad = 19; -float NormalizeAxis(SHORT value) { +float NormalizeXInputAxis(SHORT value) { return ((value + 32768.f) / 32767.5f) - 1.f; } @@ -48,105 +52,418 @@ const WebUChar* const GamepadSubTypeName(BYTE sub_type) { } } +bool GetDirectInputVendorProduct(IDirectInputDevice8* gamepad, + std::string* vendor, + std::string* product) { + DIPROPDWORD prop; + prop.diph.dwSize = sizeof(DIPROPDWORD); + prop.diph.dwHeaderSize = sizeof(DIPROPHEADER); + prop.diph.dwObj = 0; + prop.diph.dwHow = DIPH_DEVICE; + + if (FAILED(gamepad->GetProperty(DIPROP_VIDPID, &prop.diph))) + return false; + *vendor = base::StringPrintf("%04x", LOWORD(prop.dwData)); + *product = base::StringPrintf("%04x", HIWORD(prop.dwData)); + return true; +} + +// Sets the deadzone value for all axes of a gamepad. +// deadzone values range from 0 (no deadzone) to 10,000 (entire range +// is dead). +bool SetDirectInputDeadZone(IDirectInputDevice8* gamepad, + int deadzone) { + DIPROPDWORD prop; + prop.diph.dwSize = sizeof(DIPROPDWORD); + prop.diph.dwHeaderSize = sizeof(DIPROPHEADER); + prop.diph.dwObj = 0; + prop.diph.dwHow = DIPH_DEVICE; + prop.dwData = deadzone; + return SUCCEEDED(gamepad->SetProperty(DIPROP_DEADZONE, &prop.diph)); +} + +struct InternalDirectInputDevice { + IDirectInputDevice8* gamepad; + GamepadStandardMappingFunction mapper; + wchar_t id[WebGamepad::idLengthCap]; + GUID guid; +}; + +struct EnumDevicesContext { + IDirectInput8* directinput_interface; + std::vector<InternalDirectInputDevice>* directinput_devices; +}; + +// We define our own data format structure to attempt to get as many +// axes as possible. +struct JoyData { + long axes[10]; + char buttons[24]; + DWORD pov; // Often used for D-pads. +}; + +BOOL CALLBACK DirectInputEnumDevicesCallback(const DIDEVICEINSTANCE* instance, + void* context) { + EnumDevicesContext* ctxt = reinterpret_cast<EnumDevicesContext*>(context); + IDirectInputDevice8* gamepad; + + if (FAILED(ctxt->directinput_interface->CreateDevice(instance->guidInstance, + &gamepad, + NULL))) + return DIENUM_CONTINUE; + + gamepad->Acquire(); + +#define MAKE_AXIS(i) \ + {0, FIELD_OFFSET(JoyData, axes) + 4 * i, \ + DIDFT_AXIS | DIDFT_MAKEINSTANCE(i) | DIDFT_OPTIONAL, 0} +#define MAKE_BUTTON(i) \ + {&GUID_Button, FIELD_OFFSET(JoyData, buttons) + i, \ + DIDFT_BUTTON | DIDFT_MAKEINSTANCE(i) | DIDFT_OPTIONAL, 0} +#define MAKE_POV() \ + {&GUID_POV, FIELD_OFFSET(JoyData, pov), DIDFT_POV | DIDFT_OPTIONAL, 0} + DIOBJECTDATAFORMAT rgodf[] = { + MAKE_AXIS(0), + MAKE_AXIS(1), + MAKE_AXIS(2), + MAKE_AXIS(3), + MAKE_AXIS(4), + MAKE_AXIS(5), + MAKE_AXIS(6), + MAKE_AXIS(7), + MAKE_AXIS(8), + MAKE_AXIS(9), + MAKE_BUTTON(0), + MAKE_BUTTON(1), + MAKE_BUTTON(2), + MAKE_BUTTON(3), + MAKE_BUTTON(4), + MAKE_BUTTON(5), + MAKE_BUTTON(6), + MAKE_BUTTON(7), + MAKE_BUTTON(8), + MAKE_BUTTON(9), + MAKE_BUTTON(10), + MAKE_BUTTON(11), + MAKE_BUTTON(12), + MAKE_BUTTON(13), + MAKE_BUTTON(14), + MAKE_BUTTON(15), + MAKE_BUTTON(16), + MAKE_POV(), + }; +#undef MAKE_AXIS +#undef MAKE_BUTTON +#undef MAKE_POV + + DIDATAFORMAT df = { + sizeof (DIDATAFORMAT), + sizeof (DIOBJECTDATAFORMAT), + DIDF_ABSAXIS, + sizeof (JoyData), + sizeof (rgodf) / sizeof (rgodf[0]), + rgodf + }; + + // If we can't set the data format on the device, don't add it to our + // list, since we won't know how to read data from it. + if (FAILED(gamepad->SetDataFormat(&df))) { + gamepad->Release(); + return DIENUM_CONTINUE; + } + + InternalDirectInputDevice device; + device.guid = instance->guidInstance; + device.gamepad = gamepad; + std::string vendor; + std::string product; + if (!GetDirectInputVendorProduct(gamepad, &vendor, &product)) { + gamepad->Release(); + return DIENUM_CONTINUE; + } + + // Set the dead zone to 10% of the axis length for all axes. This + // gives us a larger space for what's "neutral" so the controls don't + // slowly drift. + SetDirectInputDeadZone(gamepad, 1000); + device.mapper = GetGamepadStandardMappingFunction(vendor, product); + if (device.mapper) { + base::swprintf(device.id, + WebGamepad::idLengthCap, + L"STANDARD GAMEPAD (%ls)", + instance->tszProductName); + ctxt->directinput_devices->push_back(device); + } else { + gamepad->Release(); + } + return DIENUM_CONTINUE; +} + } // namespace GamepadPlatformDataFetcherWin::GamepadPlatformDataFetcherWin() : xinput_dll_(FilePath(FILE_PATH_LITERAL("xinput1_3.dll"))), - xinput_available_(GetXinputDllFunctions()) { + xinput_available_(GetXInputDllFunctions()) { + directinput_available_ = SUCCEEDED(DirectInput8Create( + GetModuleHandle(NULL), + DIRECTINPUT_VERSION, + IID_IDirectInput8, + reinterpret_cast<void**>(&directinput_interface_), + NULL)); + for (size_t i = 0; i < WebGamepads::itemsLengthCap; ++i) + pad_state_[i].status = DISCONNECTED; } GamepadPlatformDataFetcherWin::~GamepadPlatformDataFetcherWin() { + for (size_t i = 0; i < WebGamepads::itemsLengthCap; ++i) { + if (pad_state_[i].status == DIRECTINPUT_CONNECTED) + pad_state_[i].directinput_gamepad->Release(); + } +} + +int GamepadPlatformDataFetcherWin::FirstAvailableGamepadId() const { + for (size_t i = 0; i < WebGamepads::itemsLengthCap; ++i) { + if (pad_state_[i].status == DISCONNECTED) + return i; + } + return -1; +} + +bool GamepadPlatformDataFetcherWin::HasXInputGamepad(int index) const { + for (size_t i = 0; i < WebGamepads::itemsLengthCap; ++i) { + if (pad_state_[i].status == XINPUT_CONNECTED && + pad_state_[i].xinput_index == index) + return true; + } + return false; +} + +bool GamepadPlatformDataFetcherWin::HasDirectInputGamepad( + const GUID& guid) const { + for (size_t i = 0; i < WebGamepads::itemsLengthCap; ++i) { + if (pad_state_[i].status == DIRECTINPUT_CONNECTED && + pad_state_[i].guid == guid) + return true; + } + return false; +} + +void GamepadPlatformDataFetcherWin::EnumerateDevices( + WebGamepads* pads) { + TRACE_EVENT0("GAMEPAD", "EnumerateDevices"); + + // Mark all disconnected pads DISCONNECTED. + for (size_t i = 0; i < WebGamepads::itemsLengthCap; ++i) { + if (!pads->items[i].connected) + pad_state_[i].status = DISCONNECTED; + } + + for (size_t i = 0; i < XUSER_MAX_COUNT; ++i) { + if (HasXInputGamepad(i)) + continue; + int pad_index = FirstAvailableGamepadId(); + if (pad_index == -1) + return; // We can't add any more gamepads. + WebGamepad& pad = pads->items[pad_index]; + if (xinput_available_ && GetXInputPadConnectivity(i, &pad)) { + pad_state_[pad_index].status = XINPUT_CONNECTED; + pad_state_[pad_index].xinput_index = i; + } + } + + if (directinput_available_) { + struct EnumDevicesContext context; + std::vector<InternalDirectInputDevice> directinput_gamepads; + context.directinput_interface = directinput_interface_; + context.directinput_devices = &directinput_gamepads; + + directinput_interface_->EnumDevices( + DI8DEVCLASS_GAMECTRL, + &DirectInputEnumDevicesCallback, + &context, + DIEDFL_ATTACHEDONLY); + for (size_t i = 0; i < directinput_gamepads.size(); ++i) { + if (HasDirectInputGamepad(directinput_gamepads[i].guid)) { + directinput_gamepads[i].gamepad->Release(); + continue; + } + int pad_index = FirstAvailableGamepadId(); + if (pad_index == -1) + return; + WebGamepad& pad = pads->items[pad_index]; + pad.connected = true; + wcscpy_s(pad.id, WebGamepad::idLengthCap, directinput_gamepads[i].id); + PadState& state = pad_state_[pad_index]; + state.status = DIRECTINPUT_CONNECTED; + state.guid = directinput_gamepads[i].guid; + state.directinput_gamepad = directinput_gamepads[i].gamepad; + state.mapper = directinput_gamepads[i].mapper; + } + } } + void GamepadPlatformDataFetcherWin::GetGamepadData(WebGamepads* pads, - bool devices_changed_hint) { + bool devices_changed_hint) { TRACE_EVENT0("GAMEPAD", "GetGamepadData"); - // If there's no XInput DLL on the system, early out so that we don't - // call any other… (message too large)
After rebasing, I've hit something strange where DIDFT_OPTIONAL is now undefined, and I get build errors. I have no idea what changed-- nothing should be different in the DirectX headers. I've been trying to figure this out today, but I'm baffled.
On 2013/02/20 21:46:21, teravest wrote: > After rebasing, I've hit something strange where DIDFT_OPTIONAL is now > undefined, and I get build errors. I have no idea what changed-- nothing should > be different in the DirectX headers. > > I've been trying to figure this out today, but I'm baffled. Hm, http://social.msdn.microsoft.com/forums/en-US/gametechnologiesgeneral/thread/... says it's undocumented. We recently changed from platform sdk7 + dx sdk to a standalone windows 8 sdk, which includes directx. But, looking at the dinput.h from the win8sdk it looks like that undocumented flag has been removed. I'm a little unclear on what it's for. Is it necessary? It looks like SDL just defines it to its previous value if it's unavailable: http://www.libsdl.org/tmp/SDL/src/joystick/windows/SDL_dxjoystick.c
On Wed, Feb 20, 2013 at 2:59 PM, <scottmg@chromium.org> wrote: > On 2013/02/20 21:46:21, teravest wrote: >> >> After rebasing, I've hit something strange where DIDFT_OPTIONAL is now >> undefined, and I get build errors. I have no idea what changed-- nothing > > should >> >> be different in the DirectX headers. > > >> I've been trying to figure this out today, but I'm baffled. > > > Hm, > http://social.msdn.microsoft.com/forums/en-US/gametechnologiesgeneral/thread/... > says it's undocumented. > > We recently changed from platform sdk7 + dx sdk to a standalone windows 8 > sdk, > which includes directx. But, looking at the dinput.h from the win8sdk it > looks > like that undocumented flag has been removed. I'm a little unclear on what > it's > for. Is it necessary? Ah ha! Thanks. I thought I was going crazy! DIDFT_OPTIONAL was necessary when I was building with the "previous" SDK-- if a field isn't optional, it _must_ be there when retrieving the gamepad state. This is problematic when we want to support varying numbers of axes. I'll test to see if the behavior is different with the windows 8 sdk, but I doubt it. > > It looks like SDL just defines it to its previous value if it's unavailable: > http://www.libsdl.org/tmp/SDL/src/joystick/windows/SDL_dxjoystick.c Cool. Should I just define it locally in the .cc file then? > > https://codereview.chromium.org/12260011/
On 2013/02/20 22:23:14, teravest_google.com wrote: > On Wed, Feb 20, 2013 at 2:59 PM, <mailto:scottmg@chromium.org> wrote: > > On 2013/02/20 21:46:21, teravest wrote: > >> > >> After rebasing, I've hit something strange where DIDFT_OPTIONAL is now > >> undefined, and I get build errors. I have no idea what changed-- nothing > > > > should > >> > >> be different in the DirectX headers. > > > > > >> I've been trying to figure this out today, but I'm baffled. > > > > > > Hm, > > > http://social.msdn.microsoft.com/forums/en-US/gametechnologiesgeneral/thread/... > > says it's undocumented. > > > > We recently changed from platform sdk7 + dx sdk to a standalone windows 8 > > sdk, > > which includes directx. But, looking at the dinput.h from the win8sdk it > > looks > > like that undocumented flag has been removed. I'm a little unclear on what > > it's > > for. Is it necessary? > > Ah ha! Thanks. I thought I was going crazy! > > DIDFT_OPTIONAL was necessary when I was building with the "previous" > SDK-- if a field isn't optional, it _must_ be there when retrieving > the gamepad state. This is problematic when we want to support varying > numbers of axes. > > I'll test to see if the behavior is different with the windows 8 sdk, > but I doubt it. > > > > > It looks like SDL just defines it to its previous value if it's unavailable: > > http://www.libsdl.org/tmp/SDL/src/joystick/windows/SDL_dxjoystick.c > > Cool. Should I just define it locally in the .cc file then? That seems fine to me if it's necessary, I guess it's passed straight through to the driver. (?) > > > > > https://codereview.chromium.org/12260011/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/12260011/41001
Presubmit check for 12260011-41001 failed and returned exit status 1. INFO:root:Found 4 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py ** Presubmit ERRORS ** Missing LGTM from an OWNER for files in these directories: content Presubmit checks took 4.9s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Adding brettw for the gypi change in content/.
content gypi lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/12260011/41001
Message was sent while issue was closed.
Change committed as 183689 |