[PATCH 3/3] dix: support the transformation matrix for relative devices.

[Peter Hutterer]

On Thu, Jun 02, 2011 at 03:44:15PM +0200, Simon Thum wrote:
> On 05/31/2011 05:57 AM, Peter Hutterer wrote:
> > The transformation matrix we previously stored was a scaled matrix based on
> > the axis ranges of the device. For relative movements, the scaling is not
> > required (or desired).
> Well, also any non-zero translating component will kill the mouse. That
> also should be zeroed out in the relative transform matrix (which I'd
> name after the purpose). In principle that matrix would be 2x2.

Then we need to zero it out on-the-fly since a device may have the same
transformation matrix (for rotation for example) but the coordinates it
sends may be either abs or relative.

> Now for scaling or rotation you'll need sub-pixel processing, or it will
> only work properly with orthogonal angles and integer scale. Also I'd
> say the transform should be applied after acceleration, but arguments
> can be made for any order. 

I think having the acceleration apply after transformation ensures
consistency - the acceleration of the "left" movement of the pointer is
always the same. I can be convinced otherwise though.

also, aiui, if the transformation applies before accel, we can merge this
patch - the issues pointed out below apply to "accel before transform"
only.

> My argument would be that acceleration has
> been built on the assumption that the input "is" integer mickeys more or
> less straight from the device, and the remainder is more or less private
> stuff. Without descending into qualia considerations, there are three
> solutions:

correct me if I'm wrong here but the current accel code seems to also assume
a unified resolution. the higher the device's resolution, the higher the
mickeys (and thus the acceleration) but I didn't see anything in the accel
code that takes this into account. this may be something worth looking into
as well.
 
> 1 ) Base accel on float mickeys/deltas, and choose any order
> 2 ) First accel, then transform valuator + remainder (without setting
> the latter again, i.e. you need one more row of remainders and base your
> events on that)
> 3 ) Make the valuator masks double (or add FP val masks), and expose
> them to drivers and accel and you-name-it.

I believe Daniel has a set of patches to do exactly that but they haven't
been on the list yet.

> OK, 3) is actually 1) on steroids, but it's unavoidable in the long run,
> if you ask me. 2) is the fix for now.
> 
> So the most viable order would be 2-1-3 (too lazy to reshuffle). In any
> case I'd feel better if you didn't push this patch. I think I'm
> available enough these times to at least support that roadmap.

The big advantage we have with properties is that we are not locked into a
single format. We can switch the matrix to support float format in the
future while still supporting an integer matrix as well.

Cheers,
  Peter
 
> > Store two separate matrices, one scaled, one unmodified. Depending on the
> > type of movement, apply the respective matrix.
> > 
> > This breaks the DeviceIntRec ABI
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  dix/devices.c      |    4 ++--
> >  dix/getevents.c    |   26 +++++++++++++++++++++++++-
> >  include/inputstr.h |    5 ++++-
> >  3 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > diff --git a/dix/devices.c b/dix/devices.c
> > index 0ccf252..4e4b702 100644
> > --- a/dix/devices.c
> > +++ b/dix/devices.c
> > @@ -125,14 +125,14 @@ DeviceSetTransform(DeviceIntPtr dev, float *transform)
> >          for (x=0; x<3; x++)
> >              dev->transform.m[y][x] = *transform++;
> >  
> > -    pixman_f_transform_multiply(&dev->transform, &scale, &dev->transform);
> > +    pixman_f_transform_multiply(&dev->scale, &scale, &dev->transform);
> >  
> >      /* scale */
> >      pixman_f_transform_init_scale(&scale, 1.0 / sx, 1.0 / sy);
> >      scale.m[0][2] = -dev->valuator->axes[0].min_value / sx;
> >      scale.m[1][2] = -dev->valuator->axes[1].min_value / sy;
> >  
> > -    pixman_f_transform_multiply(&dev->transform, &dev->transform, &scale);
> > +    pixman_f_transform_multiply(&dev->scale, &dev->scale, &scale);
> >  }
> >  
> >  /**
> > diff --git a/dix/getevents.c b/dix/getevents.c
> > index 1352a81..26c7f5d 100644
> > --- a/dix/getevents.c
> > +++ b/dix/getevents.c
> > @@ -1066,6 +1066,28 @@ transform(struct pixman_f_transform *m, int *x, int *y)
> >      *y = lround(p.v[1]);
> >  }
> >  
> > +
> > +static void
> > +transformRelative(DeviceIntPtr dev, ValuatorMask *mask)
> > +{
> > +    int x, y;
> > +
> > +    x = valuator_mask_isset(mask, 0) ? valuator_mask_get(mask, 0) : 0;
> > +    y = valuator_mask_isset(mask, 1) ? valuator_mask_get(mask, 1) : 0;
> > +
> > +    transform(&dev->transform, &x, &y);
> > +
> > +    if (x)
> > +        valuator_mask_set(mask, 0, x);
> > +    else
> > +        valuator_mask_unset(mask, 0);
> > +
> > +    if (y)
> > +        valuator_mask_set(mask, 1, y);
> > +    else
> > +        valuator_mask_unset(mask, 1);
> > +}
> > +
> >  static void
> >  transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
> >  {
> > @@ -1076,7 +1098,7 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
> >      oy = y = valuator_mask_isset(mask, 1) ? valuator_mask_get(mask, 1) :
> >                                              dev->last.valuators[1];
> >  
> > -    transform(&dev->transform, &x, &y);
> > +    transform(&dev->scale, &x, &y);
> >  
> >      if (valuator_mask_isset(mask, 0) || ox != x)
> >          valuator_mask_set(mask, 0, x);
> > @@ -1200,6 +1222,8 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type, int buttons
> >          transformAbsolute(pDev, &mask);
> >          moveAbsolute(pDev, &x, &y, &mask);
> >      } else {
> > +        transformRelative(pDev, &mask);
> > +
> >          if (flags & POINTER_ACCELERATE) {
> >              accelPointer(pDev, &mask, ms);
> >              /* The pointer acceleration code modifies the fractional part
> > diff --git a/include/inputstr.h b/include/inputstr.h
> > index 00f72c2..b917f6c 100644
> > --- a/include/inputstr.h
> > +++ b/include/inputstr.h
> > @@ -530,8 +530,11 @@ typedef struct _DeviceIntRec {
> >          XIPropertyHandlerPtr handlers; /* NULL-terminated */
> >      } properties;
> >  
> > -    /* coordinate transformation matrix for absolute input devices */
> > +    /* coordinate transformation matrix */
> >      struct pixman_f_transform transform;
> > +    /* scale matrix for absolute devices, this is the combined matrix of
> > +       [1/scale] . [transform] . [scale]. See DeviceSetTransform */
> > +    struct pixman_f_transform scale;
> >  
> >      /* XTest related master device id */
> >      int xtest_master_id;
>