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. > > > > 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. > > > > I also suspect that the device-specific dimensions will be rather useless > > > if > > > we don't have a call to get the min/max from each device. which I should > > > be > > > focusing on real soon :) > > > > Yea, I didn't add them because I wouldn't use them anywhere yet. I did > > consider just removing _get_x/y() but didn't. > > fwiw, I'll need get_x/y() in the xorg drivers because the server scales > everything for me. > > Cheers, > Peter > > > > > static void > > > > evdev_flush_pending_event(struct evdev_device *device, uint32_t time) > > > > { > > > > @@ -242,16 +260,6 @@ evdev_process_touch(struct evdev_device *device, > > > > struct input_event *e, > > > > uint32_t time) > > > > { > > > > - struct libinput *libinput = device->base.seat->libinput; > > > > - int screen_width; > > > > - int screen_height; > > > > - > > > > - libinput->interface->get_current_screen_dimensions( > > > > - &device->base, > > > > - &screen_width, > > > > - &screen_height, > > > > - libinput->user_data); > > > > - > > > > switch (e->code) { > > > > case ABS_MT_SLOT: > > > > evdev_flush_pending_event(device, time); > > > > @@ -267,16 +275,12 @@ evdev_process_touch(struct evdev_device *device, > > > > device->pending_event = EVDEV_ABSOLUTE_MT_UP; > > > > break; > > > > case ABS_MT_POSITION_X: > > > > - device->mt.slots[device->mt.slot].x = > > > > - (e->value - device->abs.min_x) * screen_width / > > > > - (device->abs.max_x - device->abs.min_x); > > > > + device->mt.slots[device->mt.slot].x = e->value; > > > > if (device->pending_event == EVDEV_NONE) > > > > device->pending_event = > > > > EVDEV_ABSOLUTE_MT_MOTION; > > > > break; > > > > case ABS_MT_POSITION_Y: > > > > - device->mt.slots[device->mt.slot].y = > > > > - (e->value - device->abs.min_y) * screen_height / > > > > - (device->abs.max_y - device->abs.min_y); > > > > + device->mt.slots[device->mt.slot].y = e->value; > > > > if (device->pending_event == EVDEV_NONE) > > > > device->pending_event = > > > > EVDEV_ABSOLUTE_MT_MOTION; > > > > break; > > > > @@ -287,28 +291,14 @@ static inline void > > > > evdev_process_absolute_motion(struct evdev_device *device, > > > > struct input_event *e) > > > > { > > > > - struct libinput *libinput = device->base.seat->libinput; > > > > - int screen_width; > > > > - int screen_height; > > > > - > > > > - libinput->interface->get_current_screen_dimensions( > > > > - &device->base, > > > > - &screen_width, > > > > - &screen_height, > > > > - libinput->user_data); > > > > - > > > > switch (e->code) { > > > > case ABS_X: > > > > - device->abs.x = > > > > - (e->value - device->abs.min_x) * screen_width / > > > > - (device->abs.max_x - device->abs.min_x); > > > > + device->abs.x = e->value; > > > > if (device->pending_event == EVDEV_NONE) > > > > device->pending_event = EVDEV_ABSOLUTE_MOTION; > > > > break; > > > > case ABS_Y: > > > > - device->abs.y = > > > > - (e->value - device->abs.min_y) * screen_height / > > > > - (device->abs.max_y - device->abs.min_y); > > > > + device->abs.y = e->value; > > > > if (device->pending_event == EVDEV_NONE) > > > > device->pending_event = EVDEV_ABSOLUTE_MOTION; > > > > break; > > > > diff --git a/src/evdev.h b/src/evdev.h > > > > index 58ae552..37c32e5 100644 > > > > --- a/src/evdev.h > > > > +++ b/src/evdev.h > > > > @@ -146,6 +146,16 @@ int > > > > evdev_device_has_capability(struct evdev_device *device, > > > > enum libinput_device_capability capability); > > > > > > > > +li_fixed_t > > > > +evdev_device_transform_x(struct evdev_device *device, > > > > + li_fixed_t x, > > > > + uint32_t width); > > > > + > > > > +li_fixed_t > > > > +evdev_device_transform_y(struct evdev_device *device, > > > > + li_fixed_t y, > > > > + uint32_t height); > > > > + > > > > void > > > > evdev_device_remove(struct evdev_device *device); > > > > > > > > diff --git a/src/libinput.c b/src/libinput.c > > > > index a77f165..cfce2c5 100644 > > > > --- a/src/libinput.c > > > > +++ b/src/libinput.c > > > > @@ -245,6 +245,28 @@ libinput_event_pointer_get_absolute_y( > > > > return event->y; > > > > } > > > > > > > > +LIBINPUT_EXPORT li_fixed_t > > > > +libinput_event_pointer_get_absolute_x_transformed( > > > > + struct libinput_event_pointer *event, > > > > + uint32_t width) > > > > +{ > > > > + struct evdev_device *device = > > > > + (struct evdev_device *) event->base.device; > > > > + > > > > + return evdev_device_transform_x(device, event->x, width); > > > > +} > > > > + > > > > +LIBINPUT_EXPORT li_fixed_t > > > > +libinput_event_pointer_get_absolute_y_transformed( > > > > + struct libinput_event_pointer *event, > > > > + uint32_t height) > > > > +{ > > > > + struct evdev_device *device = > > > > + (struct evdev_device *) event->base.device; > > > > + > > > > + return evdev_device_transform_y(device, event->y, height); > > > > +} > > > > + > > > > LIBINPUT_EXPORT uint32_t > > > > libinput_event_pointer_get_button( > > > > struct libinput_event_pointer *event) > > > > @@ -295,6 +317,28 @@ libinput_event_touch_get_x( > > > > } > > > > > > > > LIBINPUT_EXPORT li_fixed_t > > > > +libinput_event_touch_get_x_transformed( > > > > + struct libinput_event_touch *event, > > > > + uint32_t width) > > > > +{ > > > > + struct evdev_device *device = > > > > + (struct evdev_device *) event->base.device; > > > > + > > > > + return evdev_device_transform_x(device, event->x, width); > > > > +} > > > > + > > > > +LIBINPUT_EXPORT li_fixed_t > > > > +libinput_event_touch_get_y_transformed( > > > > + struct libinput_event_touch *event, > > > > + uint32_t height) > > > > +{ > > > > + struct evdev_device *device = > > > > + (struct evdev_device *) event->base.device; > > > > + > > > > + return evdev_device_transform_y(device, event->y, height); > > > > +} > > > > + > > > > +LIBINPUT_EXPORT li_fixed_t > > > > libinput_event_touch_get_y( > > > > struct libinput_event_touch *event) > > > > { > > > > diff --git a/src/libinput.h b/src/libinput.h > > > > index ddaeb73..8d347b9 100644 > > > > --- a/src/libinput.h > > > > +++ b/src/libinput.h > > > > @@ -395,16 +395,19 @@ libinput_event_pointer_get_dy( > > > > /** > > > > * @ingroup event_pointer > > > > * > > > > - * Return the absolute x coordinate of the device, scaled to screen > > > > - * coordinates. > > > > - * The axes' positive direction is device-specific. For pointer events > > > > that > > > > - * are not of type LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this > > > > function > > > > - * returns 0. > > > > + * Return the current absolute x coordinate of the pointer event. > > > > + * > > > > + * The coordinate is in a device specific coordinate space; to get the > > > > + * corresponding output screen coordinate, use > > > > + * libinput_event_pointer_get_x_transformed(). > > > > + * > > > > + * For pointer events that are not of type > > > > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this function returns 0. > > > > * > > > > * @note It is an application bug to call this function for events > > > > other than > > > > * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE. > > > > * > > > > - * @return the current absolute x coordinate scaled to screen > > > > coordinates. > > > > + * @return the current absolute x coordinate > > > > */ > > > > li_fixed_t > > > > libinput_event_pointer_get_absolute_x( > > > > @@ -413,15 +416,19 @@ libinput_event_pointer_get_absolute_x( > > > > /** > > > > * @ingroup event_pointer > > > > * > > > > - * Return the absolute y coordinate of the device, scaled to screen > > > > coordinates. > > > > - * The axes' positive direction is device-specific. For pointer events > > > > that > > > > - * are not of type LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this > > > > function > > > > - * returns 0. > > > > + * Return the current absolute y coordinate of the pointer event. > > > > + * > > > > + * The coordinate is in a device specific coordinate space; to get the > > > > + * corresponding output screen coordinate, use > > > > + * libinput_event_pointer_get_y_transformed(). > > > > + * > > > > + * For pointer events that are not of type > > > > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this function returns 0. > > > > * > > > > * @note It is an application bug to call this function for events > > > > other than > > > > * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE. > > > > * > > > > - * @return the current absolute y coordinate scaled to screen > > > > coordinates. > > > > + * @return the current absolute y coordinate > > > > */ > > > > li_fixed_t > > > > libinput_event_pointer_get_absolute_y( > > > > @@ -430,6 +437,50 @@ libinput_event_pointer_get_absolute_y( > > > > /** > > > > * @ingroup event_pointer > > > > * > > > > + * Return the current absolute x coordinate of the pointer event, > > > > transformed to > > > > + * screen coordinates. > > > > + * > > > > + * For pointer events that are not of type > > > > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, the return value of this > > > > function is > > > > + * undefined. > > > > + * > > > > + * @note It is an application bug to call this function for events > > > > other than > > > > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE. > > > > + * > > > > + * @param event The libinput pointer event > > > > + * @param width The current output screen width > > > > + * @return the current absolute x coordinate transformed to a screen > > > > coordinate > > > > + */ > > > > +li_fixed_t > > > > +libinput_event_pointer_get_absolute_x_transformed( > > > > + struct libinput_event_pointer *event, > > > > + uint32_t width); > > > > + > > > > +/** > > > > + * @ingroup event_pointer > > > > + * > > > > + * Return the current absolute y coordinate of the pointer event, > > > > transformed to > > > > + * screen coordinates. > > > > + * > > > > + * For pointer events that are not of type > > > > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, the return value of this > > > > function is > > > > + * undefined. > > > > + * > > > > + * @note It is an application bug to call this function for events > > > > other than > > > > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE. > > > > + * > > > > + * @param event The libinput pointer event > > > > + * @param height The current output screen height > > > > + * @return the current absolute y coordinate transformed to a screen > > > > coordinate > > > > + */ > > > > +li_fixed_t > > > > +libinput_event_pointer_get_absolute_y_transformed( > > > > + struct libinput_event_pointer *event, > > > > + uint32_t height); > > > > + > > > > +/** > > > > + * @ingroup event_pointer > > > > + * > > > > * Return the button that triggered this event. > > > > * For pointer events that are not of type > > > > LIBINPUT_EVENT_POINTER_BUTTON, > > > > * this function returns 0. > > > > @@ -531,9 +582,16 @@ libinput_event_touch_get_slot( > > > > /** > > > > * @ingroup event_touch > > > > * > > > > + * Return the current absolute x coordinate of the touch event. > > > > + * > > > > + * The coordinate is in a device specific coordinate space; to get the > > > > + * corresponding output screen coordinate, use > > > > + * libinput_event_touch_get_x_transformed(). > > > > + * > > > > * @note this function should not be called for > > > > LIBINPUT_EVENT_TOUCH_FRAME. > > > > * > > > > - * @return the absolute X coordinate on this touch device, scaled to > > > > screen coordinates. > > > > + * @param event The libinput touch event > > > > + * @return the current absolute x coordinate > > > > */ > > > > li_fixed_t > > > > libinput_event_touch_get_x( > > > > @@ -542,9 +600,16 @@ libinput_event_touch_get_x( > > > > /** > > > > * @ingroup event_touch > > > > * > > > > + * Return the current absolute y coordinate of the touch event. > > > > + * > > > > + * The coordinate is in a device specific coordinate space; to get the > > > > + * corresponding output screen coordinate, use > > > > + * libinput_event_touch_get_y_transformed(). > > > > + * > > > > * @note this function should not be called for > > > > LIBINPUT_EVENT_TOUCH_FRAME. > > > > * > > > > - * @return the absolute X coordinate on this touch device, scaled to > > > > screen coordinates. > > > > + * @param event The libinput touch event > > > > + * @return the current absolute y coordinate > > > > */ > > > > li_fixed_t > > > > libinput_event_touch_get_y( > > > > @@ -553,6 +618,38 @@ libinput_event_touch_get_y( > > > > /** > > > > * @ingroup event_touch > > > > * > > > > + * Return the current absolute x coordinate of the touch event, > > > > transformed to > > > > + * screen coordinates. > > > > + * > > > > + * @note this function should not be called for > > > > LIBINPUT_EVENT_TOUCH_FRAME. > > > > + * > > > > + * @param event The libinput touch event > > > > + * @param width The current output screen width > > > > + * @return the current absolute x coordinate transformed to a screen > > > > coordinate > > > > + */ > > > > +li_fixed_t > > > > +libinput_event_touch_get_x_transformed(struct libinput_event_touch > > > > *event, > > > > + uint32_t width); > > > > + > > > > +/** > > > > + * @ingroup event_touch > > > > + * > > > > + * Return the current absolute y coordinate of the touch event, > > > > transformed to > > > > + * screen coordinates. > > > > + * > > > > + * @note this function should not be called for > > > > LIBINPUT_EVENT_TOUCH_FRAME. > > > > + * > > > > + * @param event The libinput touch event > > > > + * @param height The current output screen height > > > > + * @return the current absolute y coordinate transformed to a screen > > > > coordinate > > > > + */ > > > > +li_fixed_t > > > > +libinput_event_touch_get_y_transformed(struct libinput_event_touch > > > > *event, > > > > + uint32_t height); > > > > + > > > > +/** > > > > + * @ingroup event_touch > > > > + * > > > > * @note this function should not be called for > > > > LIBINPUT_EVENT_TOUCH_FRAME. > > > > * > > > > * @return the type of touch that occured on the device > > > > @@ -586,11 +683,6 @@ struct libinput_interface { > > > > * libinput_create_from_udev() > > > > */ > > > > void (*close_restricted)(int fd, void *user_data); > > > > - > > > > - void (*get_current_screen_dimensions)(struct libinput_device > > > > *device, > > > > - int *width, > > > > - int *height, > > > > - void *user_data); > > > > }; > > > > > > > > /** > > > > diff --git a/test/litest.c b/test/litest.c > > > > index 5235e3c..216e1a0 100644 > > > > --- a/test/litest.c > > > > +++ b/test/litest.c > > > > @@ -318,20 +318,9 @@ close_restricted(int fd, void *userdata) > > > > close(fd); > > > > } > > > > > > > > -static void > > > > -get_current_screen_dimensions(struct libinput_device *device, > > > > - int *width, > > > > - int *height, > > > > - void *user_data) > > > > -{ > > > > - *width = 1024; > > > > - *height = 768; > > > > -} > > > > - > > > > const struct libinput_interface interface = { > > > > .open_restricted = open_restricted, > > > > .close_restricted = close_restricted, > > > > - .get_current_screen_dimensions = get_current_screen_dimensions, > > > > }; > > > > > > > > struct litest_device * > > > > -- > > > > 1.8.3.2 > > > > > > > > _______________________________________________ > > > > wayland-devel mailing list > > > > wayland-devel@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > > > > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel