[PATCH 0/2] MR2573: dinput: Set guidType = GUID_Unknown for HID collections.
I'm not sure if I'm using the dinput HID test infrastructure correctly. Also, an existing test could be extended instead of adding a whole new test function, saving a couple of source code lines and test execution time, but I wasn't sure which one to extend - feel free to suggest one and I will update the test. Also, maybe the suggested fix is not a great idea. Maybe there are actually some collection objects that are supposed to have a guidType that is not GUID_Unknown? I couldn't find any in the limited number of Windows environments I can access, so I wanted to put it out there for discussion. The issue I'm trying to solve is that Zusi 3 does not appear to check the DIDFT_NODATA bit in dwType, so it just adds most of the objects listed by EnumObjects() to the SetDataFormat() call, maybe filtering out only those that have guidType = GUID_Unknown, or maybe there's an allow-list of GUIDs to add, and that list includes GUID_ZAxis. I haven't seen complaints about joysticks not working in Zusi 3 on Windows, so I guess collections (and DIDFT_NODATA objects in general) rarely/never have a non-Unknown GUID, or maybe Windows simply accepts these collections in SetDataFormat() and that only fails in Wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2573
From: Florian Will <florian.will(a)gmail.com> --- dlls/dinput/tests/joystick8.c | 83 +++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/dlls/dinput/tests/joystick8.c b/dlls/dinput/tests/joystick8.c index 3ab6f3c26d4..9bac8fdf668 100644 --- a/dlls/dinput/tests/joystick8.c +++ b/dlls/dinput/tests/joystick8.c @@ -4767,6 +4767,88 @@ done: return device != NULL; } +static void test_collection_guid(void) +{ +#include "psh_hid_macros.h" + static const unsigned char report_desc[] = + { + USAGE_PAGE(1, HID_USAGE_PAGE_GENERIC), + USAGE(1, HID_USAGE_GENERIC_JOYSTICK), + COLLECTION(1, Application), + USAGE(1, HID_USAGE_GENERIC_Z), + COLLECTION(1, Physical), + END_COLLECTION, + END_COLLECTION, + }; + C_ASSERT(sizeof(report_desc) < MAX_HID_DESCRIPTOR_LEN); +#include "pop_hid_macros.h" + + struct hid_device_desc desc = + { + .attributes = default_attributes, + }; + const DIDEVICEOBJECTINSTANCEW expect_objects[] = + { + { + .dwSize = sizeof(DIDEVICEOBJECTINSTANCEW), + .guidType = GUID_Unknown, + .dwType = DIDFT_COLLECTION|DIDFT_NODATA|DIDFT_MAKEINSTANCE(0), + .tszName = L"Collection 0 - Joystick", + .wUsagePage = HID_USAGE_PAGE_GENERIC, + .wUsage = HID_USAGE_GENERIC_JOYSTICK, + }, + { + .dwSize = sizeof(DIDEVICEOBJECTINSTANCEW), + .guidType = GUID_Unknown, + .dwType = DIDFT_COLLECTION|DIDFT_NODATA|DIDFT_MAKEINSTANCE(1), + .tszName = L"Collection 1 - Z Axis", + .wUsagePage = HID_USAGE_PAGE_GENERIC, + .wUsage = HID_USAGE_GENERIC_Z, + }, + }; + struct check_object_todo todo_objects[ARRAY_SIZE(expect_objects)] = + { + {0}, + { .guid = TRUE }, + }; + struct check_objects_params check_objects_params = + { + .version = DIRECTINPUT_VERSION, + .expect_count = ARRAY_SIZE(expect_objects), + .expect_objs = expect_objects, + .todo_objs = todo_objects, + }; + + DIDEVICEINSTANCEW devinst = {0}; + IDirectInputDevice8W *device; + HRESULT hr; + ULONG ref; + + cleanup_registry_keys(); + + desc.report_descriptor_len = sizeof(report_desc); + memcpy( desc.report_descriptor_buf, report_desc, sizeof(report_desc) ); + fill_context( desc.context, ARRAY_SIZE(desc.context) ); + + if (!hid_device_start( &desc, 1 )) goto done; + if (FAILED(hr = dinput_test_create_device( DIRECTINPUT_VERSION, &devinst, &device ))) goto done; + + check_dinput_devices( DIRECTINPUT_VERSION, &devinst ); + + hr = IDirectInputDevice8_EnumObjects( device, check_objects, &check_objects_params, DIDFT_ALL ); + ok( hr == DI_OK, "EnumObjects returned %#lx\n", hr ); + ok( check_objects_params.index >= check_objects_params.expect_count, "missing %u objects\n", + check_objects_params.expect_count - check_objects_params.index ); + + ref = IDirectInputDevice8_Release( device ); + ok( ref == 0, "Release returned %ld\n", ref ); + +done: + hid_device_stop( &desc, 1 ); + cleanup_registry_keys(); + winetest_pop_context(); +} + #define check_interface( a, b, c ) check_interface_( __LINE__, a, b, c ) static void check_interface_( unsigned int line, void *iface_ptr, REFIID iid, BOOL supported ) { @@ -5471,6 +5553,7 @@ START_TEST( joystick8 ) test_driving_wheel_axes(); test_rawinput(); test_windows_gaming_input(); + test_collection_guid(); } done: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2573
From: Florian Will <florian.will(a)gmail.com> Fixes an issue in Zusi 3 where DIDFT_COLLECTION objects would be passed to SetDataFormat. --- dlls/dinput/joystick_hid.c | 2 +- dlls/dinput/tests/joystick8.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 42f58c52202..3320ac92a1c 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -728,7 +728,7 @@ static BOOL enum_objects( struct hid_joystick *impl, const DIPROPHEADER *filter, instance.dwFlags = 0; instance.wUsagePage = node->usage_page; instance.wUsage = node->usage; - instance.guidType = *object_usage_to_guid( instance.wUsagePage, instance.wUsage ); + instance.guidType = GUID_Unknown; instance.wReportId = 0; instance.wCollectionNumber = node->parent; instance.dwDimension = 0; diff --git a/dlls/dinput/tests/joystick8.c b/dlls/dinput/tests/joystick8.c index 9bac8fdf668..12198107cf6 100644 --- a/dlls/dinput/tests/joystick8.c +++ b/dlls/dinput/tests/joystick8.c @@ -4809,7 +4809,7 @@ static void test_collection_guid(void) struct check_object_todo todo_objects[ARRAY_SIZE(expect_objects)] = { {0}, - { .guid = TRUE }, + {0}, }; struct check_objects_params check_objects_params = { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2573
The fix seems to make sense, I don't think any test showed that collections are supposed to have an actual GUID, and it was set with `object_usage_to_guid` mostly because the other objects are. Regarding the test I think you can add an empty collection in `test_simple_joystick` instead, though you might have to adjust the expected descs accordingly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2573#note_28809
Florian Will (@w-flo) commented about dlls/dinput/tests/joystick8.c:
+ if (FAILED(hr = dinput_test_create_device( DIRECTINPUT_VERSION, &devinst, &device ))) goto done; + + check_dinput_devices( DIRECTINPUT_VERSION, &devinst ); + + hr = IDirectInputDevice8_EnumObjects( device, check_objects, &check_objects_params, DIDFT_ALL ); + ok( hr == DI_OK, "EnumObjects returned %#lx\n", hr ); + ok( check_objects_params.index >= check_objects_params.expect_count, "missing %u objects\n", + check_objects_params.expect_count - check_objects_params.index ); + + ref = IDirectInputDevice8_Release( device ); + ok( ref == 0, "Release returned %ld\n", ref ); + +done: + hid_device_stop( &desc, 1 ); + cleanup_registry_keys(); + winetest_pop_context(); I just noticed this: I copied the test_driving_wheel_axes() function for my test, and I believe both test_driving_wheel_axes() and my new test should drop the winetest_pop_context() because they don't push anything. Or maybe they should push something at the start of the test? Same with the test_many_axes_joystick() test and maybe others. It seems off-topic for this MR though.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2573#note_28810
On Tue Apr 4 07:10:17 2023 +0000, Florian Will wrote:
I just noticed this: I copied the test_driving_wheel_axes() function for my test, and I believe both test_driving_wheel_axes() and my new test should drop the winetest_pop_context() because they don't push anything. Or maybe they should push something at the start of the test? Same with the test_many_axes_joystick() test and maybe others. It seems off-topic for this MR though. Indeed, feel free to squeeze a change to remove them in the MR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2573#note_28811
participants (3)
-
Florian Will -
Florian Will (＠w-flo)
-
Rémi Bernon