[PATCH 1/2] dinput: Fix phase conversion on periodic effects

[Matteo Bruni]

2016-09-20 1:34 GMT+02:00 Bruno Jesus <00cpxxx at gmail.com>:
> On Mon, Sep 19, 2016 at 8:12 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
>> 2016-09-14 7:25 GMT+02:00 Bruno Jesus <00cpxxx at gmail.com>:
>>> Based on idea by Elias Vanderstuyft.
>>>
>>> Signed-off-by: Bruno Jesus <00cpxxx at gmail.com>
>>> ---
>>>  dlls/dinput/effect_linuxinput.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> Thanks for giving dinput and force feedback some love and care lately.
>> I'm going to be a nag though...
>
> Ciao, Italian friend.

;)

>>> diff --git a/dlls/dinput/effect_linuxinput.c b/dlls/dinput/effect_linuxinput.c
>>> index 9205c9c..bb45792 100644
>>> --- a/dlls/dinput/effect_linuxinput.c
>>> +++ b/dlls/dinput/effect_linuxinput.c
>>> @@ -632,7 +632,8 @@ static HRESULT WINAPI LinuxInputEffectImpl_SetParameters(
>>>
>>>              This->effect.u.periodic.magnitude = (tsp->dwMagnitude / 10) * 32;
>>>              This->effect.u.periodic.offset = (tsp->lOffset / 10) * 32;
>>> -            This->effect.u.periodic.phase = (tsp->dwPhase / 9) * 8; /* == (/ 36 * 32) */
>>> +            /* phase ranges from 0 - 35999 in dinput and 0 - 65535 on linux */
>>> +            This->effect.u.periodic.phase = (tsp->dwPhase / 36) * 65;
>>
>> Probably I'm missing something but... Why don't you actually use 35999
>> and 65535 in the expression?
>> Relatedly, why not do the multiplication before the division instead
>> of throwing bits of precision away? Maybe phase can go over 35999? If
>> that's the only reason, I'd adjust the phase before doing this
>> computation instead.
>> I guess these points apply to the other effect parameters too.
>
> I have been talking a lot to Elias Vanderstuyft recently, we are aware
> of precision problems and are working to fix it a in better way. For
> now I'm sticking the simple changes that I'm able to test in my real
> hardware. The limit of phase is 35999 because it is related to degrees
> / 100, more than that would wrap. You are probably right that we need
> to clamp to max value before calculating.

I don't know if we need to clamp or not. My guess is that it just
repeats periodically after that, if such values are allowed at all
(SetParameter might fail). I.e. I would have used something like
(phase % 36000) instead of clamping.
That probably means some more tests are required...

> It is important to note that
> although we are losing precision you can't really notice it in a
> standard gamepad.

Sure but it still seems like a good idea to do things right when reasonable.
Notice that I don't mean to blame you or the patch, I had a bit of a
look at the existing code after all...

>> BTW, you didn't change the corresponding piece of code in
>> GetParameters (which, for some reason, uses 33 and 36 as scaling
>> constants...) so that's probably broken now. Or at least more broken
>> than it was before.
>
> I'm planning to throw away the GetParameters code and instead save a
> copy of SetParameters value and return it instead.

Yeah, that sounds like a good idea for sure.