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).
> > 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?
> > 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
