[PATCH v2 0/1] MR7161: dinput: Clamp FFB effect report value to the field range.
If application sends out-of-range values for FFB effects, they become -1, which is undesireable, when -1 is in the physical range of the field and being sent to the device. This patch fixes it, clamping values to the field range. Clamping was earlier implemented [here](https://gitlab.winehq.org/wine/wine/-/blob/wine-6.19/dlls/dinput/effect_linu...) in 6.19. Windows does this clamping silently, and doesn't fail `SetParameters` call when values is out of range. Affected application is Assetto Corsa EVO, which with default settings sends out ConstantForce magnitude under/over [documented](https://learn.microsoft.com/en-us/previous-versions/ms835644(v=msdn.10)) \[-10000, 10000\]. [Link to the discussion with some investigation on Steam](https://steamcommunity.com/app/3058630/discussions/0/756142145462617887/#c75...). In this submission scaling and clamping were moved to the separate `scale_and_clamp_value` function, which is now called every time from `set_parameter_value`. To avoid regression with sending `-1` for the infinite duration effects, separate function `set_parameter_value_no_scaling` created. Gain values in `hid_joystick_send_device_gain` are also scaled and clamped. -- v2: dinput: Clamp FFB effect report value to the field range. https://gitlab.winehq.org/wine/wine/-/merge_requests/7161
From: Makarenko Oleg <oleg(a)makarenk.ooo> Move scaling and clamping FFB report values to the separate function. Do not scale or clamp report value for infinite duration effects. --- dlls/dinput/joystick_hid.c | 48 +++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 367d6356195..ca144937108 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -749,22 +749,10 @@ static void set_report_value( struct hid_joystick *impl, char *report_buf, { ULONG report_len = impl->caps.OutputReportByteLength; PHIDP_PREPARSED_DATA preparsed = impl->preparsed; - LONG log_min, log_max, phy_min, phy_max; NTSTATUS status; if (!caps) return; - log_min = caps->logical_min; - log_max = caps->logical_max; - phy_min = caps->physical_min; - phy_max = caps->physical_max; - - if (phy_max || phy_min) - { - if (value > phy_max || value < phy_min) value = -1; - else value = log_min + (value - phy_min) * (log_max - log_min) / (phy_max - phy_min); - } - status = HidP_SetUsageValue( HidP_Output, caps->usage_page, caps->link_collection, caps->usage_min, value, preparsed, report_buf, report_len ); if (status != HIDP_STATUS_SUCCESS) WARN( "HidP_SetUsageValue %04x:%04x returned %#lx\n", @@ -837,6 +825,8 @@ static HRESULT hid_joystick_get_property( IDirectInputDevice8W *iface, DWORD pro return DIERR_UNSUPPORTED; } +static LONG scale_and_clamp_value( LONG value, struct hid_value_caps *caps ); + static HRESULT hid_joystick_send_device_gain( IDirectInputDevice8W *iface, LONG device_gain ) { struct hid_joystick *impl = impl_from_IDirectInputDevice8W( iface ); @@ -852,6 +842,7 @@ static HRESULT hid_joystick_send_device_gain( IDirectInputDevice8W *iface, LONG status = HidP_InitializeReportForID( HidP_Output, report->id, impl->preparsed, report_buf, report_len ); if (status != HIDP_STATUS_SUCCESS) return status; + device_gain = scale_and_clamp_value( device_gain, report->device_gain_caps ); set_report_value( impl, report_buf, report->device_gain_caps, device_gain ); if (!WriteFile( impl->device, report_buf, report_len, NULL, NULL )) return DIERR_INPUTLOST; @@ -1141,6 +1132,25 @@ static LONG scale_value( ULONG value, struct object_properties *properties ) return phy_min + MulDiv( tmp - log_min, phy_max - phy_min, log_max - log_min ); } +static LONG scale_and_clamp_value( LONG value, struct hid_value_caps *caps ) +{ + LONG log_min, log_max, phy_min, phy_max; + + if (!caps) return value; + + log_min = caps->logical_min; + log_max = caps->logical_max; + phy_min = caps->physical_min; + phy_max = caps->physical_max; + + if (phy_max || phy_min) + { + value = max(min(value, phy_max), phy_min); + value = log_min + (value - phy_min) * (log_max - log_min) / (phy_max - phy_min); + } + return value; +} + static LONG scale_axis_value( ULONG value, struct object_properties *properties ) { LONG tmp = sign_extend( value, properties ), log_ctr, log_min, log_max, phy_ctr, phy_min, phy_max; @@ -2809,6 +2819,13 @@ static HRESULT WINAPI hid_joystick_effect_GetEffectStatus( IDirectInputEffect *i static void set_parameter_value( struct hid_joystick_effect *impl, char *report_buf, struct hid_value_caps *caps, LONG value ) +{ + value = scale_and_clamp_value( value, caps ); + return set_report_value( impl->joystick, report_buf, caps, value ); +} + +static void set_parameter_value_no_scaling( struct hid_joystick_effect *impl, char *report_buf, + struct hid_value_caps *caps, LONG value ) { return set_report_value( impl->joystick, report_buf, caps, value ); } @@ -2831,7 +2848,12 @@ static void set_parameter_value_us( struct hid_joystick_effect *impl, char *repo LONG exp; if (!caps) return; exp = caps->units_exp; - if (value == INFINITE) value = caps->physical_min - 1; + if (value == INFINITE) + { + value = caps->physical_min - 1; + set_parameter_value_no_scaling( impl, report_buf, caps, value ); + return; + } else if (caps->units != 0x1003) WARN( "unknown time unit caps %#lx\n", caps->units ); else if (exp < -6) while (exp++ < -6) value *= 10; else if (exp > -6) while (exp-- > -6) value /= 10; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7161
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150945 Your paranoid android. === debian11 (32 bit report) === dinput: driver_bus.c:53: Test failed: id 1: force_feedback.c:1704 expect[0]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:1704 expect[0]: 00000000 07 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:1704 expect[0]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:1704 expect[0]: 00000000 07 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:1704 expect[0]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:1704 expect[0]: 00000000 07 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4289 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4289 expect[2]: 00000000 04 01 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4318 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4318 expect[2]: 00000000 04 02 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4526 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4526 expect[2]: 00000000 04 01 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4551 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4551 expect[2]: 00000000 04 01 00 f9 19 d9 00 00 99 === debian11b (64 bit WoW report) === dinput: driver_bus.c:53: Test failed: id 1: force_feedback.c:1704 expect[0]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:1704 expect[0]: 00000000 07 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:1704 expect[0]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:1704 expect[0]: 00000000 07 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:1704 expect[0]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:1704 expect[0]: 00000000 07 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4289 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4289 expect[2]: 00000000 04 01 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4318 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4318 expect[2]: 00000000 04 02 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4526 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4526 expect[2]: 00000000 04 01 00 f9 19 d9 00 00 99 driver_bus.c:53: Test failed: id 1: force_feedback.c:4551 expect[2]: unexpected data: driver_bus.c:62: Test failed: id 1: force_feedback.c:4551 expect[2]: 00000000 04 01 00 f9 19 d9 00 00 99
This seems sensible but as you can see this fails several dinput tests. The tests are using virtual HID PID devices, with hand made HID report descriptors, and check what HID reports are being sent to the device when dinput effects are requested. You will need to make sure the changes pass the existing tests, and eventually add a couple of tests (or adjust existing ones) to match and check the scenario you want to address. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7161#note_92267
As far as i can tell, `dwPositiveSaturation` and `dwNegativeSaturation` behavior is different. I'll check it more thoroughly and will update the patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7161#note_92268
Is it possible that there are some test that would need to be updated, because they were using out-of-bounds values? This would make them fail right now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7161#note_92288
No, existing tests are correct, negative values of `dwPositiveSaturation` and `dwNegativeSaturation` should become maximum field values (`-1`, for the matter), i confirmed it on Windows. I will add tests for magnitude/levels/etc being out-of-bounds after i deal with Saturation fields, and after testing other fields for the same behavior. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7161#note_92289
participants (5)
-
Makarenko Oleg -
Marvin -
Oleg Makarenko (＠JacKeTUs)
-
Rémi Bernon
-
Tomasz Pakuła