subject:"\[PATCH libinput 1\/2\] Replace output screen size callback with transform helpers"

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

2014-01-31 Thread Jonas Ådahl

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 =

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

2014-01-30 Thread Peter Hutterer

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:

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

2014-01-29 Thread Jonas Ådahl

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);
+}
+
 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

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

2014-01-29 Thread Peter Hutterer

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.

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);

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:

   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 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 :)

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,
 -

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information