[PATCH libinput 1/2] Replace output screen size callback with transform helpers
Jonas Ådahl
jadahl at gmail.com
Fri Jan 31 00:46:00 PST 2014
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.
>
> > > 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 at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > > >
More information about the wayland-devel
mailing list