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

[Matteo Bruni]

2016-09-20 1:34 GMT+02:00 Bruno Jesus :
> On Mon, Sep 19, 2016 at 8:12 PM, Matteo Bruni  wrote:
> > 2016-09-14 7:25 GMT+02:00 Bruno Jesus :
> > > Based on idea by Elias Vanderstuyft.
> > > Signed-off-by: Bruno Jesus
> > >
> > > 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.