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 <jad...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel