[PATCH] dix: the input matrix must work on pixels, not device ranges

[Peter Hutterer]

On Wed, Apr 24, 2013 at 02:09:03PM -0700, Keith Packard wrote:
> Peter Hutterer <peter.hutterer at who-t.net> writes:
> 
> > The screen width is for most devices smaller than the device range.
> > Previously, we applied the matrix to the device coordinate range, then
> > converting into screen (subpixel) coordinates. The half-way point of a
> > device is likely somewhere inside a subpixel, just to the left of the pixel
> > we want (in a 50:50 configuration). Thus, the mapping is off by one pixel.
> > In a 50/50 mapping with a 50% offset this means that we hit the right-most
> > pixel of the left screen, even when we should only be bound to the right
> > screen.
> 
> Can you provide some concrete numbers so that we can see the effect of
> the different operation ordering? From a brief reading of the code, it
> seems like you're just changing the order of operations from:

ok, here's the long writeup, but doing this one made me realise why I ended
up with the complicated solution instead of the simple one which is now
here:
http://patchwork.freedesktop.org/patch/13584/

the problem was: devices give us an inclusive axis width, the screen is
exclusive. that coupled with a pixel size that's bigger than a device unit
leads to bad results.

The Wacom Intuos4 6x9 has an x coordinate range of [0..44704]. Let's assume
a screen with [0,1024[ pixels.

Now with a 50% offset and a 50% mapping (i.e. the one you'd chose for the
right monitor), you'd think that the half of the tablet is mapped to
half the screen, except:
        input x = 0
        after transform = 44704/2 = 22352

        22352 * 1023/44704 = 511.5

so the device will be mapped just half a pixel off the center. the crux here
is simply the 1023 since that is the last reachable coordinate. but this
would ignore the width of a pixel.

an alternative solution is to make both exclusive:
        22352 * 1024/(44704 + 1) = 511.988
Close, but still wrong.

The original patch was the result of trying to calculate it into the right
range. 0 was first mapped into screen coordinates, then the matrix
was applied, giving us 512. That was mapped back into device coordinates for
XI events.

complicated, but not nice and it effectively gave us a small dead area on
the tablet (smaller than half a pixel translated, but still).


Writing this up did make me realise that there was one combination I missed:
the reason why the above didn't work is because matrix too needs to be set
up for [min,max[. With that done, the coordinate work out asx:

        input x = 0
        after transform = (44704 + 1)/2 = 22352.5
        22352.5 * 1024/(44704 + 1) = 512

And that appears to be the sensible thing to do.

Cheers,
   Peter



>     device -> transformed_device -> clipped_device -> desktop -> screen
> 
> to
> 
>     device -> desktop -> screen
> 
> I'm obviously missing something important here; is there a float->int
> truncation that I skipped?
> 
> > -    sx = dev->valuator->axes[0].max_value - dev->valuator->axes[0].min_value;
> > -    sy = dev->valuator->axes[1].max_value - dev->valuator->axes[1].min_value;
> > +    sx = screenInfo.width - screenInfo.x;
> > +    sy = screenInfo.height - screenInfo.y;
> 
> Shouldn't the scale just be the overall screen size, not the screen size
> minus the upper left corner?
> 
> And, given that you want screen coordinates coming out of this function,
> shouldn't the translation be screenInfo.x and screenInfo.y for the
> invscale matrix?
> 
> >  
> >      /* invscale */
> >      pixman_f_transform_init_scale(&scale, sx, sy);
> 
> > @@ -129,8 +129,8 @@ DeviceSetTransform(DeviceIntPtr dev, float *transform_data)
> >  
> >      /* 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;
> > +    scale.m[0][2] = -screenInfo.x / sx;
> > +    scale.m[1][2] = -screenInfo.y / sy;
> 
> This looks correct -- get the screen origin out of the coordinate after
> scaling it.
> 
> > +        clipAbsolute(pDev, &mask); /* set to axis boundaries */
> > +        if ((flags & POINTER_NORAW) == 0)
> 
> Do we need to clip the screen coordinates at any point in this
> computation? They were implicitly clipped before as they were taken from
> the clipped device coordinates.
> 
> -- 
> keith.packard at intel.com