[PATCH libinput 1/2] Replace output screen size callback with transform helpers

[Peter Hutterer]

On Fri, Jan 31, 2014 at 09:46:00AM +0100, Jonas Ådahl wrote:
> On Fri, Jan 31, 2014 at 09:12:17AM +1000, Peter Hutterer wrote:
> > On Thu, Jan 30, 2014 at 08:38:02AM +0100, Jonas Ådahl wrote:
> > > On Thu, Jan 30, 2014 at 01:02:15PM +1000, Peter Hutterer wrote:
> > > > On Wed, Jan 29, 2014 at 09:33:11PM +0100, Jonas Ådahl wrote:
> > > > > Instead of automatically transforming absolute coordinates of touch and
> > > > > pointer events to screen coordinates, the user now uses the corresponding
> > > > > transform helper function. This means the coordinates returned by
> > > > > libinput_event_pointer_get_absolute_x(),
> > > > > libinput_event_pointer_get_absolute_y(), libinput_touch_get_x() and
> > > > > libinput_touch_get_y() has changed from being in output screen coordinate
> > > > > space to being in device specific coordinate space.
> > > > > 
> > > > > For example, where one before would call libinput_event_touch_get_x(event),
> > > > > one now calls libinput_event_touch_get_x_transformed(event, output_width).
> > > > > 
> > > > > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > > > > ---
> > > > >  src/evdev.c    |  54 ++++++++++--------------
> > > > >  src/evdev.h    |  10 +++++
> > > > >  src/libinput.c |  44 ++++++++++++++++++++
> > > > >  src/libinput.h | 128 +++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > >  test/litest.c  |  11 -----
> > > > >  5 files changed, 186 insertions(+), 61 deletions(-)
> > > > > 
> > > > > diff --git a/src/evdev.c b/src/evdev.c
> > > > > index 46bd35a..cb83a1f 100644
> > > > > --- a/src/evdev.c
> > > > > +++ b/src/evdev.c
> > > > > @@ -86,6 +86,24 @@ transform_absolute(struct evdev_device *device, int32_t *x, int32_t *y)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +li_fixed_t
> > > > > +evdev_device_transform_x(struct evdev_device *device,
> > > > > +			 li_fixed_t x,
> > > > > +			 uint32_t width)
> > > > > +{
> > > > > +	return (x - device->abs.min_x) * width /
> > > > > +		(device->abs.max_x - device->abs.min_x);
> > > > > +}
> > > > > +
> > > > > +li_fixed_t
> > > > > +evdev_device_transform_y(struct evdev_device *device,
> > > > > +			 li_fixed_t y,
> > > > > +			 uint32_t height)
> > > > > +{
> > > > > +	return (y - device->abs.min_y) * height /
> > > > > +		(device->abs.max_y - device->abs.min_y);
> > > > 
> > > > you're mixing coordinate systems here, x and y are in fixed_t but
> > > > abs.min/max is in normal integers. that breaks if you have a non-zero min.
> > > > You'll need to convert the rest to li_fixed_t too if you want to keep the
> > > > integer division.
> > > 
> > > Yea, missed the wl_fixed_from_int here (and in _x), and they were all 0
> > > so didn't notice it either. For multiplication, one of the factors cannot be
> > > li_fixed_t though. Same goes for division where the denominator needs to
> > > be a normal int even if the numerator is li_fixed_t.
> > 
> > I agree, but "cannot" should read "does not need to be". the complete formula is:
> > 
> >   scaled = (x - xmin) * (screen_max_x - screen_min_x)/(xmax - xmin) + screen_min_x
> > 
> > fixed_t is essentially (foo * 256), so if we assume x is in fixed_t and we
> > convert everything to fixed, we have
> > 
> >   = (x - xmin * 256) * (screen_max_x * 256 - screen_min_x * 256)/(xmax * 256 - xmin * 256) + screen_min_x * 256
> >   = (x - xmin * 256) * (screen_max_x - screen_min_x) * 256/((xmax - xmin) * 256) + screen_min_x  * 256
> >   = (x - xmin * 256) * (screen_max_x - screen_min_x)/(xmax - xmin) + screen_min_x * 256
> > 
> > and because we have an offset of 0, and thus screen_max_x == width, we end
> > up with
> > 
> >   = (x - xmin * 256) * width/(xmax - xmin)
> > 
> > so yes, you only need to convert xmin to li_fixed_t, but that only
> > applies because we expect a 0 screen offset.
> > 
> > and that concludes today's math tutorial.
> > (which I mainly did because I wasn't 100% sure on this either ;)
> > 
> > It'd probably be worth noting this somewhere, or at least writing down the
> > base formula so that if there are ever patches that change this at least the
> > base formula is clear. We've messed up missing out on (+ screen_min_x) a few
> > times in the X stack over the years.
> > 
> > Also, there is one problem with the formula. the screen dimensions are
> > exclusive [0,width[, the device coordinates are inclusive [min, max]. so the
> > correct scaling should be (xmax - xmin + 1).
> 
> 
> What I meant was just that. If you have a multiplication, you don't want
> to multiply two li_fixed_t because you'll get 256 * 256. Same for
> division, if you devide a li_fixed_t with a li_fixed_t you get 256 / 256
> == 1.
> 
> So in other words, the following should be correct (change (one of the)
> min_y to li_fixed_t, and the missing +1):
> 
> 	return (y - li_fixed_from_int(device->abs.min_y)) * height /
> 		(device->abs.max_y - device->abs.min_y + 1);
> 
> As a side note, this formula (except the missing fixed min_x/y
> conversion) is just the one in weston moved around a couple of times,
> so if the + 1 is important and urgent, it should be fixed there as well.

yeah, it should be fixed in both. I had some weird issue in X where some
screen crossing didn't work right because of that missing unit.
 
> > > > also, should we add a non-zero min for width and height to scale to a screen
> > > > not the top/left-most? The compositor can just add it afterwards, but 
> > > > it would have to convert to fixed_t as well:
> > > > 
> > > >     x = libinput_event_touch_get_x_transformed(event, screen_width);
> > > >     x += li_fixed_from_int(screen_offset);
> > > > 
> > > > which is more error prone than something like:
> > > > 
> > > >     x = libinput_event_touch_get_x_transformed(event, screen_offset_x, screen_width);
> > > >
> > > 
> > > That transform wouldn't be enough. We'd have to rotate etc as well. See
> > > http://cgit.freedesktop.org/wayland/weston/tree/src/compositor.c#n3408 .
> > > Given that, I think its easiest to let libinput do the device specific
> > > transform (device coords -> output coords) and then have the user
> > > translate, and do other transformations.
> > 
> > fair enough. There is an argument to be made for libinput to do these
> > things, or provide helper functions to avoid callers writing potentially
> > buggy code. but not this time :)
> > 
> > > > also, is it likely that the caller always has the screen dimensions handy
> > > > when it comes to processing events? or would an config-style approach work
> > > > better:
> > > 
> > > Where I implemented this (weston), this is indeed the case. See
> > > https://github.com/jadahl/weston/commit/9230416e82929dd2a545b176934e38538ea19e11
> > > 
> > > > 
> > > >    libinput_device_set_output_dimensions(device, xoff, yoff, width, height);
> > > >    ...
> > > >    x = libinput_event_touch_get_x_transformed(event);
> > > >    y = libinput_event_touch_get_y_transformed(event);
> > > 
> > > I don't think we gain anything by duplicating the state in libinput, as
> > > the user most likely will always have it. Instead we'd have to add extra
> > > hooks in the user's code to update the output dimensions, that would be
> > > wrong anyway if an output gets disconnected, and then we'd need to have
> > > a unset_output_dimensions() or set to 0, 0, 0, 0, which is a bit
> > > awkward.
> > 
> > hmm, how is the last bit different to not having a device where you get the
> > width/height from?
> 
> Its not very different, it will just potentially complicate things for
> the user for no obvious gain. We'd also get rid of a getter that depends
> on a previously set state.

Ok, I don't have a particularly big preference either way, so leave it
as-is then.

Cheers,
   Peter