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

[Bruno Jesus]

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. It is important to note that
although we are losing precision you can't really notice it in a
standard gamepad.

> 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. This will solve the
issues about linux -> dinput reconversion, this seems to work but
testing takes longer time. Specially because I could not find any game
that actually uses this, only effect editing tools.

Best wishes,
Bruno