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

[Matteo Bruni]

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...
> 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.

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.

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

[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. 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

[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.

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

[Bruno Jesus]

On Mon, Sep 19, 2016 at 9:11 PM, Matteo Bruni  wrote:
> > On Mon, Sep 19, 2016 at 8:12 PM, Matteo Bruni  wrote:
> > 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...

Yes, I agree that if we can make it better it should be better. I just
lack the math skills sometimes for things that may look obvious for
other people, I even considered opening questions at
http://math.stackexchange.com to help improve some equations.