[PATCH 0/2] MR4029: winmm: Reimplement joystick axis mapping.
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/winmm/joystick.c | 74 ++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/dlls/winmm/joystick.c b/dlls/winmm/joystick.c index 0285074bd40..56ff61582ba 100644 --- a/dlls/winmm/joystick.c +++ b/dlls/winmm/joystick.c @@ -23,6 +23,7 @@ */ #include <stdarg.h> +#include <stdbool.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> @@ -175,14 +176,48 @@ void joystick_unload(void) IDirectInput8_Release( dinput ); } +static bool acquire_joystick( DWORD index ) +{ + const DIDEVICEINSTANCEW *instance = &instances[index]; + struct joystick *joystick = &joysticks[index]; + IDirectInputDevice8W *device; + HANDLE event; + HRESULT hr; + + if (FAILED(hr = IDirectInput8_CreateDevice( dinput, &instance->guidInstance, &device, NULL ))) + { + WARN( "failed to create dinput device %s, hr %#lx\n", debugstr_guid( &instance->guidInstance ), hr ); + return false; + } + + if (!(event = CreateEventW( NULL, FALSE, FALSE, NULL ))) + ERR( "failed to create event, error %lu\n", GetLastError() ); + if (FAILED(hr = IDirectInputDevice8_SetEventNotification( device, event ))) + ERR( "failed to set event notification, hr %#lx\n", hr ); + if (FAILED(hr = IDirectInputDevice8_SetCooperativeLevel( device, NULL, DISCL_NONEXCLUSIVE | DISCL_BACKGROUND ))) + ERR( "failed to set cooperative level, hr %#lx\n", hr ); + if (FAILED(hr = IDirectInputDevice8_SetDataFormat( device, &data_format ))) + ERR( "failed to set data format, hr %#lx\n", hr ); + if (FAILED(hr = IDirectInputDevice8_Acquire( device ))) + { + WARN( "failed to acquire, hr %#lx\n", hr ); + IDirectInputDevice8_Release( device ); + CloseHandle( event ); + return false; + } + + memset( joystick, 0, sizeof(*joystick) ); + joystick->instance = *instance; + joystick->device = device; + joystick->event = event; + return true; +} + static void find_joysticks(void) { static INIT_ONCE init_once = INIT_ONCE_STATIC_INIT; - IDirectInputDevice8W *device; - HANDLE event; DWORD index; - HRESULT hr; InitOnceExecuteOnce( &init_once, joystick_load_once, NULL, NULL ); @@ -203,35 +238,16 @@ static void find_joysticks(void) CloseHandle( joysticks[index].event ); } - if (!(event = CreateEventW( NULL, FALSE, FALSE, NULL ))) - WARN( "could not event for device, error %lu\n", GetLastError() ); - else if (FAILED(hr = IDirectInput8_CreateDevice( dinput, &instances[index].guidInstance, &device, NULL ))) - WARN( "could not create device %s instance, hr %#lx\n", - debugstr_guid( &instances[index].guidInstance ), hr ); - else if (FAILED(hr = IDirectInputDevice8_SetEventNotification( device, event ))) - WARN( "SetEventNotification device %p hr %#lx\n", device, hr ); - else if (FAILED(hr = IDirectInputDevice8_SetCooperativeLevel( device, NULL, DISCL_NONEXCLUSIVE|DISCL_BACKGROUND ))) - WARN( "SetCooperativeLevel device %p hr %#lx\n", device, hr ); - else if (FAILED(hr = IDirectInputDevice8_SetDataFormat( device, &data_format ))) - WARN( "SetDataFormat device %p hr %#lx\n", device, hr ); - else if (FAILED(hr = IDirectInputDevice8_Acquire( device ))) - WARN( "Acquire device %p hr %#lx\n", device, hr ); + if (acquire_joystick( index )) + { + TRACE( "opened device %s\n", debugstr_guid( &instances[index].guidInstance )); + } else { - TRACE( "opened device %p event %p\n", device, event ); - - memset( &joysticks[index], 0, sizeof(struct joystick) ); - joysticks[index].instance = instances[index]; - joysticks[index].device = device; - joysticks[index].event = event; - continue; + memmove( joysticks + index, joysticks + index + 1, + (ARRAY_SIZE(joysticks) - index - 1) * sizeof(struct joystick) ); + memset( &joysticks[ARRAY_SIZE(joysticks) - 1], 0, sizeof(struct joystick) ); } - - CloseHandle( event ); - if (device) IDirectInputDevice8_Release( device ); - memmove( joysticks + index, joysticks + index + 1, - (ARRAY_SIZE(joysticks) - index - 1) * sizeof(struct joystick) ); - memset( &joysticks[ARRAY_SIZE(joysticks) - 1], 0, sizeof(struct joystick) ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4029
From: Zebediah Figura <zfigura(a)codeweavers.com> This should more closely match the algorithm used by native, discovered through extensive manual testing. This does not include any automated tests, both because I could not get winmm to recognize a second plugged joystick, and (as I eventually discovered while testing) the number of tests that would be necessary to conclusively prove this algorithm turned out to be impractical. -- I discovered that a certain joystick, an XBox clone (Logitech F310) was failing to report all of its axes to a winmm game (Super Smash Flash 2). The joystick reports axes X, Y, Z, RX, RY, RZ, but currently winmm does not map RY. Checking Windows's joy.cpl implied that our HID usages were correct, so I instead started checking whether our mapping algorithm was correct for a synthetic joystick with the same usages. It turned out that it was not. Native winmm appears to work in a way that is relatively intuitive to understand: for each axis, it runs through a list of candidate usages. If the candidate usage is present on the joystick, and wasn't already mapped to a different axis, then the axis is mapped. --- dlls/winmm/joystick.c | 166 ++++++++++++++++++++++++------------------ 1 file changed, 96 insertions(+), 70 deletions(-) diff --git a/dlls/winmm/joystick.c b/dlls/winmm/joystick.c index 56ff61582ba..6f0c1208f47 100644 --- a/dlls/winmm/joystick.c +++ b/dlls/winmm/joystick.c @@ -34,6 +34,7 @@ #include "winbase.h" #include "mmsystem.h" +#include "hidusage.h" #include "initguid.h" #include "dinput.h" @@ -52,69 +53,21 @@ static CRITICAL_SECTION_DEBUG critsect_debug = }; static CRITICAL_SECTION joystick_cs = { &critsect_debug, -1, 0, 0, 0, 0 }; -struct joystick_state +enum axis { - LONG x; - LONG y; - LONG z; - LONG u; - LONG v; - LONG r; - LONG pov; - BYTE buttons[32]; + AXIS_X, + AXIS_Y, + AXIS_Z, + AXIS_R, + AXIS_U, + AXIS_V, }; -static const DIOBJECTDATAFORMAT object_formats[] = -{ - { &GUID_XAxis, offsetof(struct joystick_state, x), DIDFT_OPTIONAL|DIDFT_AXIS|DIDFT_ANYINSTANCE, DIDOI_ASPECTPOSITION }, - { &GUID_YAxis, offsetof(struct joystick_state, y), DIDFT_OPTIONAL|DIDFT_AXIS|DIDFT_ANYINSTANCE, DIDOI_ASPECTPOSITION }, - { &GUID_ZAxis, offsetof(struct joystick_state, z), DIDFT_OPTIONAL|DIDFT_AXIS|DIDFT_ANYINSTANCE, DIDOI_ASPECTPOSITION }, - { &GUID_RzAxis, offsetof(struct joystick_state, r), DIDFT_OPTIONAL|DIDFT_AXIS|DIDFT_ANYINSTANCE, DIDOI_ASPECTPOSITION }, - { &GUID_Slider, offsetof(struct joystick_state, u), DIDFT_OPTIONAL|DIDFT_AXIS|DIDFT_ANYINSTANCE, DIDOI_ASPECTPOSITION }, - { &GUID_RxAxis, offsetof(struct joystick_state, v), DIDFT_OPTIONAL|DIDFT_AXIS|DIDFT_ANYINSTANCE, DIDOI_ASPECTPOSITION }, - { &GUID_POV, offsetof(struct joystick_state, pov), DIDFT_OPTIONAL|DIDFT_POV|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[0]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[1]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[2]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[3]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[4]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[5]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[6]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[7]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[8]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[9]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[10]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[11]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[12]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[13]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[14]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[15]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[16]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[17]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[18]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[19]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[20]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[21]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[22]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[23]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[24]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[25]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[26]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[27]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[28]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[29]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[30]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, - { NULL, offsetof(struct joystick_state, buttons[31]), DIDFT_OPTIONAL|DIDFT_BUTTON|DIDFT_ANYINSTANCE, 0 }, -}; - -static const DIDATAFORMAT data_format = +struct joystick_state { - .dwSize = sizeof(DIDATAFORMAT), - .dwObjSize = sizeof(DIOBJECTDATAFORMAT), - .dwFlags = DIDF_ABSAXIS, - .dwDataSize = sizeof(struct joystick_state), - .dwNumObjs = ARRAY_SIZE(object_formats), - .rgodf = (DIOBJECTDATAFORMAT *)object_formats, + LONG axes[6]; + LONG pov; + BYTE buttons[32]; }; #define JOY_PERIOD_MIN (10) /* min Capture time period */ @@ -176,20 +129,94 @@ void joystick_unload(void) IDirectInput8_Release( dinput ); } +static bool is_already_mapped( const DIOBJECTDATAFORMAT *object_formats, DWORD count, + const DIDEVICEOBJECTINSTANCEW *instance ) +{ + DWORD i; + + for (i = 0; i < count; ++i) + { + if (object_formats[i].dwType == instance->dwType) + return true; + } + + return false; +} + static bool acquire_joystick( DWORD index ) { + DIOBJECTDATAFORMAT object_formats[6 + 1 + 32] = {{0}}; /* 6 axes, 1 hat switch, 32 buttons */ const DIDEVICEINSTANCEW *instance = &instances[index]; struct joystick *joystick = &joysticks[index]; + DIOBJECTDATAFORMAT *object_format; + DIDATAFORMAT data_format = {0}; IDirectInputDevice8W *device; + unsigned int i, j; HANDLE event; HRESULT hr; + static const struct + { + WORD usages[5]; + } + usage_mappings[6] = + { + [AXIS_X] = {{HID_USAGE_GENERIC_X, HID_USAGE_GENERIC_RY}}, + [AXIS_Y] = {{HID_USAGE_GENERIC_Y, HID_USAGE_GENERIC_RX}}, + [AXIS_Z] = {{HID_USAGE_GENERIC_WHEEL, HID_USAGE_GENERIC_Z, HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL}}, + [AXIS_R] = {{HID_USAGE_GENERIC_RZ, HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL, HID_USAGE_GENERIC_RY, HID_USAGE_GENERIC_RX}}, + [AXIS_U] = {{HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL, HID_USAGE_GENERIC_RY, HID_USAGE_GENERIC_RX}}, + [AXIS_V] = {{HID_USAGE_GENERIC_RX}}, + }; + + memset( joystick, 0, sizeof(*joystick) ); + if (FAILED(hr = IDirectInput8_CreateDevice( dinput, &instance->guidInstance, &device, NULL ))) { WARN( "failed to create dinput device %s, hr %#lx\n", debugstr_guid( &instance->guidInstance ), hr ); return false; } + for (i = 0; i < 6; ++i) + { + for (j = 0; j < ARRAY_SIZE(usage_mappings[i].usages) && usage_mappings[i].usages[j]; ++j) + { + DIDEVICEOBJECTINSTANCEW instance; + + instance.dwSize = sizeof(DIDEVICEOBJECTINSTANCEW); + if (FAILED(IDirectInputDevice8_GetObjectInfo( device, &instance, + DIMAKEUSAGEDWORD(HID_USAGE_PAGE_GENERIC, usage_mappings[i].usages[j]), DIPH_BYUSAGE ))) + continue; + if (!(instance.dwFlags & DIDOI_ASPECTPOSITION)) + continue; + if (is_already_mapped( object_formats, data_format.dwNumObjs, &instance )) + continue; + + object_format = &object_formats[data_format.dwNumObjs++]; + object_format->dwOfs = offsetof(struct joystick_state, axes[i]); + object_format->dwType = instance.dwType; + break; + } + } + + object_format = &object_formats[data_format.dwNumObjs++]; + object_format->pguid = &GUID_POV; + object_format->dwOfs = offsetof(struct joystick_state, pov); + object_format->dwType = DIDFT_OPTIONAL | DIDFT_POV | DIDFT_ANYINSTANCE; + + for (i = 0; i < 32; ++i) + { + object_format = &object_formats[data_format.dwNumObjs++]; + object_format->dwOfs = offsetof(struct joystick_state, buttons[i]); + object_format->dwType = DIDFT_OPTIONAL | DIDFT_BUTTON | DIDFT_ANYINSTANCE; + } + + data_format.dwSize = sizeof(DIDATAFORMAT); + data_format.dwObjSize = sizeof(DIOBJECTDATAFORMAT); + data_format.dwFlags = DIDF_ABSAXIS; + data_format.dwDataSize = sizeof(struct joystick_state); + data_format.rgodf = object_formats; + if (!(event = CreateEventW( NULL, FALSE, FALSE, NULL ))) ERR( "failed to create event, error %lu\n", GetLastError() ); if (FAILED(hr = IDirectInputDevice8_SetEventNotification( device, event ))) @@ -206,7 +233,6 @@ static bool acquire_joystick( DWORD index ) return false; } - memset( joystick, 0, sizeof(*joystick) ); joystick->instance = *instance; joystick->device = device; joystick->event = event; @@ -401,13 +427,13 @@ MMRESULT WINAPI DECLSPEC_HOTPATCH joyGetDevCapsW( UINT_PTR id, JOYCAPSW *caps, U caps->wNumAxes = min( dicaps.dwAxes, caps->wMaxAxes ); caps->wMaxButtons = 32; - hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, z), DIPH_BYOFFSET ); + hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, axes[AXIS_Z]), DIPH_BYOFFSET ); if (SUCCEEDED(hr)) caps->wCaps |= JOYCAPS_HASZ; - hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, r), DIPH_BYOFFSET ); + hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, axes[AXIS_R]), DIPH_BYOFFSET ); if (SUCCEEDED(hr)) caps->wCaps |= JOYCAPS_HASR; - hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, u), DIPH_BYOFFSET ); + hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, axes[AXIS_U]), DIPH_BYOFFSET ); if (SUCCEEDED(hr)) caps->wCaps |= JOYCAPS_HASU; - hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, v), DIPH_BYOFFSET ); + hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, axes[AXIS_V]), DIPH_BYOFFSET ); if (SUCCEEDED(hr)) caps->wCaps |= JOYCAPS_HASV; hr = IDirectInputDevice8_GetObjectInfo( device, &instance, offsetof(struct joystick_state, pov), DIPH_BYOFFSET ); if (SUCCEEDED(hr)) caps->wCaps |= JOYCAPS_HASPOV|JOYCAPS_POV4DIR; @@ -509,12 +535,12 @@ MMRESULT WINAPI DECLSPEC_HOTPATCH joyGetPosEx( UINT id, JOYINFOEX *info ) } else { - if (info->dwFlags & JOY_RETURNX) info->dwXpos = state.x; - if (info->dwFlags & JOY_RETURNY) info->dwYpos = state.y; - if (info->dwFlags & JOY_RETURNZ) info->dwZpos = state.z; - if (info->dwFlags & JOY_RETURNR) info->dwRpos = state.r; - if (info->dwFlags & JOY_RETURNU) info->dwUpos = state.u; - if (info->dwFlags & JOY_RETURNV) info->dwVpos = state.v; + if (info->dwFlags & JOY_RETURNX) info->dwXpos = state.axes[AXIS_X]; + if (info->dwFlags & JOY_RETURNY) info->dwYpos = state.axes[AXIS_Y]; + if (info->dwFlags & JOY_RETURNZ) info->dwZpos = state.axes[AXIS_Z]; + if (info->dwFlags & JOY_RETURNR) info->dwRpos = state.axes[AXIS_R]; + if (info->dwFlags & JOY_RETURNU) info->dwUpos = state.axes[AXIS_U]; + if (info->dwFlags & JOY_RETURNV) info->dwVpos = state.axes[AXIS_V]; if (info->dwFlags & JOY_RETURNPOV) { if (state.pov == ~0) info->dwPOV = 0xffff; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4029
Rémi Bernon (@rbernon) commented about dlls/winmm/joystick.c:
*/
#include <stdarg.h> +#include <stdbool.h> We use BOOL everywhere else in the file, why use stdbool instead?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_47822
Rémi Bernon (@rbernon) commented about dlls/winmm/joystick.c:
+ + if (!(event = CreateEventW( NULL, FALSE, FALSE, NULL ))) + ERR( "failed to create event, error %lu\n", GetLastError() ); + if (FAILED(hr = IDirectInputDevice8_SetEventNotification( device, event ))) + ERR( "failed to set event notification, hr %#lx\n", hr ); + if (FAILED(hr = IDirectInputDevice8_SetCooperativeLevel( device, NULL, DISCL_NONEXCLUSIVE | DISCL_BACKGROUND ))) + ERR( "failed to set cooperative level, hr %#lx\n", hr ); + if (FAILED(hr = IDirectInputDevice8_SetDataFormat( device, &data_format ))) + ERR( "failed to set data format, hr %#lx\n", hr ); + if (FAILED(hr = IDirectInputDevice8_Acquire( device ))) + { + WARN( "failed to acquire, hr %#lx\n", hr ); + IDirectInputDevice8_Release( device ); + CloseHandle( event ); + return false; + } Why change the logic? I don't think we want to add a joystick if any of these calls fail.
Same question with the error levels. I don't know precisely what we consider to deserve ERR vs WARN but I had the impression that ERR is reserved for critical failures that will prevent Wine from working. Most if not all of these failures aren't and are possibly completely acceptable. Fwiw looking at the next commit, I think you could instead just have a `set_joystick_data_format` helper replacing the `IDirectInputDevice8_SetDataFormat` call. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_47823
Rémi Bernon (@rbernon) commented about dlls/winmm/joystick.c:
struct joystick *joystick = &joysticks[index]; + DIOBJECTDATAFORMAT *object_format; + DIDATAFORMAT data_format = {0}; IDirectInputDevice8W *device; + unsigned int i, j; HANDLE event; HRESULT hr;
+ static const struct + { + WORD usages[5]; + } + usage_mappings[6] = + { + [AXIS_X] = {{HID_USAGE_GENERIC_X, HID_USAGE_GENERIC_RY}}, + [AXIS_Y] = {{HID_USAGE_GENERIC_Y, HID_USAGE_GENERIC_RX}},
The secondary axes usage seem inverted? Fwiw a simple test, failing with the current mapping, and fixed with this would be appreciated. For instance something that reproduces the usages of the device you want to fix. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_47824
Rémi Bernon (@rbernon) commented about dlls/winmm/joystick.c:
+ [AXIS_Y] = {{HID_USAGE_GENERIC_Y, HID_USAGE_GENERIC_RX}}, + [AXIS_Z] = {{HID_USAGE_GENERIC_WHEEL, HID_USAGE_GENERIC_Z, HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL}}, + [AXIS_R] = {{HID_USAGE_GENERIC_RZ, HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL, HID_USAGE_GENERIC_RY, HID_USAGE_GENERIC_RX}}, + [AXIS_U] = {{HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL, HID_USAGE_GENERIC_RY, HID_USAGE_GENERIC_RX}}, + [AXIS_V] = {{HID_USAGE_GENERIC_RX}}, + }; + + memset( joystick, 0, sizeof(*joystick) ); + if (FAILED(hr = IDirectInput8_CreateDevice( dinput, &instance->guidInstance, &device, NULL ))) { WARN( "failed to create dinput device %s, hr %#lx\n", debugstr_guid( &instance->guidInstance ), hr ); return false; }
+ for (i = 0; i < 6; ++i)
for (i = 0; i < ARRAY_SIZE(usage_mappings); ++i)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_47825
Rémi Bernon (@rbernon) commented about dlls/winmm/joystick.c:
+ if (is_already_mapped( object_formats, data_format.dwNumObjs, &instance )) + continue; + + object_format = &object_formats[data_format.dwNumObjs++]; + object_format->dwOfs = offsetof(struct joystick_state, axes[i]); + object_format->dwType = instance.dwType; + break; + } + } + + object_format = &object_formats[data_format.dwNumObjs++]; + object_format->pguid = &GUID_POV; + object_format->dwOfs = offsetof(struct joystick_state, pov); + object_format->dwType = DIDFT_OPTIONAL | DIDFT_POV | DIDFT_ANYINSTANCE; + + for (i = 0; i < 32; ++i)
for (i = 0; i < ARRAY_SIZE(((struct joystick_state *)0)->buttons); ++i)
(or a local joystick_state variable) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_47826
Rémi Bernon (@rbernon) commented about dlls/winmm/joystick.c:
+ static const struct + { + WORD usages[5]; + } + usage_mappings[6] = + { + [AXIS_X] = {{HID_USAGE_GENERIC_X, HID_USAGE_GENERIC_RY}}, + [AXIS_Y] = {{HID_USAGE_GENERIC_Y, HID_USAGE_GENERIC_RX}}, + [AXIS_Z] = {{HID_USAGE_GENERIC_WHEEL, HID_USAGE_GENERIC_Z, HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL}}, + [AXIS_R] = {{HID_USAGE_GENERIC_RZ, HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL, HID_USAGE_GENERIC_RY, HID_USAGE_GENERIC_RX}}, + [AXIS_U] = {{HID_USAGE_GENERIC_SLIDER, HID_USAGE_GENERIC_DIAL, HID_USAGE_GENERIC_RY, HID_USAGE_GENERIC_RX}}, + [AXIS_V] = {{HID_USAGE_GENERIC_RX}}, + }; + + memset( joystick, 0, sizeof(*joystick) );
Why move this here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_47827
On Fri Oct 6 07:28:23 2023 +0000, Rémi Bernon wrote:
Why move this here? Leftover code from some intermediate changes; I'll revert it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48437
On Fri Oct 6 07:28:22 2023 +0000, Rémi Bernon wrote:
We use BOOL everywhere else in the file, why use stdbool instead? bool is more explicit, it's standard C, and it may also be nicer to the compiler. [Also, you can do things like assign to bool and get an implicit !!, although the value of that is a bit dubious]. For these reasons I've been incrementally adopting it elsewhere. I'm happy to remove it if it's a sticking point, though.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48438
On Fri Oct 6 07:28:22 2023 +0000, Rémi Bernon wrote:
Why change the logic? I don't think we want to add a joystick if any of these calls fail. Same question with the error levels. I don't know precisely what we consider to deserve ERR vs WARN but I had the impression that ERR is reserved for critical failures that will prevent Wine from working. Most if not all of these failures aren't and are possibly completely acceptable. Fwiw looking at the next commit, I think you could instead just have a `set_joystick_data_format` helper replacing the `IDirectInputDevice8_SetDataFormat` call. My (re)interpretation of error levels has been that ERR should be for internal errors that a well-behaved application doesn't trigger, whereas WARN is for errors that might happen naturally but might also be symptomatic of a failure. Wine code doesn't always follow this interpretation (although I think most of the more well-maintained subsystems do), but I think it broadly makes sense given what's printed by default. (And usually where Wine code doesn't follow this, it errs on the side of being *more* verbose rather than less.)
In this case the motivation for the change is that almost none of those calls should fail if we're passing valid parameters (and not fatally out of memory). If they do fail, it's an internal error and we should be noisy about it; I'd think an assert would not be unwarranted even. This is also the motivation for changing the control flow, because it's really just not worth the extra code to handle the failure. The original motivation for this is that the if/else model doesn't work well if we need to do something nontrivial between two method calls. As you say, though, a set-data-format helper would also achieve this, so I can revert this patch and do that instead; I don't feel that strongly about it. (I do still feel that ERR is more appropriate than WARN, though.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48439
On Fri Oct 6 07:28:22 2023 +0000, Rémi Bernon wrote:
The secondary axes usage seem inverted? Fwiw a simple test, failing with the current mapping, and fixed with this would be appreciated. For instance something that reproduces the usages of the device you want to fix. They look inverted but they're correct; I can add a /* [sic] */ if that'd help.
The attached diff should be such a test. Unfortunately, as mentioned, I couldn't figure out a way to add even one more test to the existing winmm tests. [scratch.diff](/uploads/4c92990debf6c8d7f429568ece24a230/scratch.diff) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48440
On Thu Oct 12 04:24:03 2023 +0000, Zebediah Figura wrote:
bool is more explicit, it's standard C, and it may also be nicer to the compiler. [Also, you can do things like assign to bool and get an implicit !!, although the value of that is a bit dubious]. For these reasons I've been incrementally adopting it elsewhere. I'm happy to remove it if it's a sticking point, though. This seems unrelated and I don't see any plan to globally change this, so I'd prefer to leave it out until then, as it otherwise only makes the code more inconsistent.
Regarding such a plan, as many of the Win32 API functions we implement use BOOL, I think using `bool` for internal functions may make things inconsistent too, and the eventual advantages don't seem really worth it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48646
On Thu Oct 12 04:24:03 2023 +0000, Zebediah Figura wrote:
My (re)interpretation of error levels has been that ERR should be for internal errors that a well-behaved application doesn't trigger, whereas WARN is for errors that might happen naturally but might also be symptomatic of a failure. Wine code doesn't always follow this interpretation (although I think most of the more well-maintained subsystems do), but I think it broadly makes sense given what's printed by default. (And usually where Wine code doesn't follow this, it errs on the side of being *more* verbose rather than less.) In this case the motivation for the change is that almost none of those calls should fail if we're passing valid parameters (and not fatally out of memory). If they do fail, it's an internal error and we should be noisy about it; I'd think an assert would not be unwarranted even. This is also the motivation for changing the control flow, because it's really just not worth the extra code to handle the failure. The original motivation for this is that the if/else model doesn't work well if we need to do something nontrivial between two method calls. As you say, though, a set-data-format helper would also achieve this, so I can revert this patch and do that instead; I don't feel that strongly about it. (I do still feel that ERR is more appropriate than WARN, though.) Okay, I'm fine with changing WARN to ERR with this rationale, though it should be a separate change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48647
On Thu Oct 12 04:24:03 2023 +0000, Zebediah Figura wrote:
They look inverted but they're correct; I can add a /* [sic] */ if that'd help. The attached diff should be such a test. Unfortunately, as mentioned, I couldn't figure out a way to add even one more test to the existing winmm tests. [scratch.diff](/uploads/4c92990debf6c8d7f429568ece24a230/scratch.diff) Something like that would be good I think. You'll also then notice that Z axis mapping does not seem to prefer Z over WHEEL.
[0001-dinput-tests-Use-exotic-axes-ordering-for-winmm-test.patch](/uploads/583a33f7e512dae40aa80c29b767f757/0001-dinput-tests-Use-exotic-axes-ordering-for-winmm-test.patch) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48648
On Fri Oct 13 13:43:45 2023 +0000, Rémi Bernon wrote:
Something like that would be good I think. You'll also then notice that Z axis mapping does not seem to prefer Z over WHEEL. [0001-dinput-tests-Use-exotic-axes-ordering-for-winmm-test.patch](/uploads/583a33f7e512dae40aa80c29b767f757/0001-dinput-tests-Use-exotic-axes-ordering-for-winmm-test.patch) That's annoying, it means we can't use GetObjectInfo().
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4029#note_48709
participants (3)
-
Rémi Bernon -
Zebediah Figura
-
Zebediah Figura (＠zfigura)