Re: [PATCH weston 16/25] libweston: introduce notify_touch_cal() and doc

[Pekka Paalanen]

On Tue, 10 Apr 2018 12:37:15 +0300
Pekka Paalanen  wrote:

> On Mon, 9 Apr 2018 12:12:49 +1000
> Peter Hutterer  wrote:
> 
> > On Fri, Mar 23, 2018 at 02:00:56PM +0200, Pekka Paalanen wrote:  
> > > From: Pekka Paalanen 
> > > 
> > > notify_touch_cal() is an extended form of notify_touch(), adding
> > > normalized touch coordinates which are necessary for calibrating a
> > > touchscreen.
> > > 
> > > It would be possible to invert the transformation and convert from
> > > global coordinates to normalized device coordinates in input.c without
> > > adding this API, but this way it is more robust against code changes.
> > > 
> > > Recovering normalized device coordinates is necessary because libinput
> > > calibration matrix must be given in normalized units, and it would be
> > > difficult to compute otherwise. Libinput API does not offer normalized
> > > coordinates directly either, but those can be fetched by pretending the
> > > output resolution is 1x1.
> > > 
> > > Anticipating touch calibration mode, the old notify_touch() is renamed
> > > into a private process_touch_normal(), and the new notify_touch_cal()
> > > delegates to it.
> > > 
> > > Co-developed by Louis-Francis and Pekka.
> > > 
> > > Cc: Louis-Francis Ratté-Boulianne 
> > > Signed-off-by: Pekka Paalanen 
> > > ---
> > >  libweston/compositor.h  | 21 +++-
> > >  libweston/input.c   | 60 
> > > -
> > >  libweston/libinput-device.c | 11 -
> > >  3 files changed, 79 insertions(+), 13 deletions(-)

> > Also, at this point I can only say creating structs for each coordinate type
> > in libinput has helped greatly in understanding what exactly you're dealing
> > with at any point in time. see libinput-private.h:
> > 
> > /* A coordinate pair in device coordinates */
> > struct device_coords {
> > int x, y;
> > };
> > 
> > /* A dpi-normalized coordinate pair */
> > struct normalized_coords {
> > double x, y;
> > };
> > 
> > /* A pair of coordinates normalized to a [0,1] or [-1, 1] range */
> > struct normalized_range_coords {
> > double x, y;
> > };
> > 
> > 
> > etc. These are passed through the various functions, so the compiler will
> > tell you when you're passing a device coordinate into something that should
> > take a [0, 1] normalized range. I cannot recommend this enough when you're
> > dealing with more than one coordinate system.  
> 
> That's a good idea indeed. I think I'll re-spin with that.

On second thought, even adding just

struct weston_point_global_double {
double x;
double y;
};

would be touching quite many places, and it would prompt me to add

struct weston_point_global_fixed {
wl_fixed_t x;
wl_fixed_t y;
};

which would touch even more that I'd probably be touching almost all
APIs there are. Those would need to be complemented with struct
weston_point_surface_{double,fixed} as well.

So if you don't mind, I would leave that yak for another time.

But, maybe I can start with struct weston_2d_normalized_range_coords
since that's not used elsewhere at all. If I can figure out a bit
shorter name... weston_point2d_output_normalized...


Thanks,
pq


pgpcCxHKN2sWP.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 16/25] libweston: introduce notify_touch_cal() and doc

[Pekka Paalanen]

On Mon, 9 Apr 2018 12:12:49 +1000
Peter Hutterer  wrote:

> On Fri, Mar 23, 2018 at 02:00:56PM +0200, Pekka Paalanen wrote:
> > From: Pekka Paalanen 
> > 
> > notify_touch_cal() is an extended form of notify_touch(), adding
> > normalized touch coordinates which are necessary for calibrating a
> > touchscreen.
> > 
> > It would be possible to invert the transformation and convert from
> > global coordinates to normalized device coordinates in input.c without
> > adding this API, but this way it is more robust against code changes.
> > 
> > Recovering normalized device coordinates is necessary because libinput
> > calibration matrix must be given in normalized units, and it would be
> > difficult to compute otherwise. Libinput API does not offer normalized
> > coordinates directly either, but those can be fetched by pretending the
> > output resolution is 1x1.
> > 
> > Anticipating touch calibration mode, the old notify_touch() is renamed
> > into a private process_touch_normal(), and the new notify_touch_cal()
> > delegates to it.
> > 
> > Co-developed by Louis-Francis and Pekka.
> > 
> > Cc: Louis-Francis Ratté-Boulianne 
> > Signed-off-by: Pekka Paalanen 
> > ---
> >  libweston/compositor.h  | 21 +++-
> >  libweston/input.c   | 60 
> > -
> >  libweston/libinput-device.c | 11 -
> >  3 files changed, 79 insertions(+), 13 deletions(-)

> > --- a/libweston/input.c
> > +++ b/libweston/input.c
> > @@ -2336,17 +2336,10 @@ weston_touch_set_focus(struct weston_touch *touch, 
> > struct weston_view *view)
> > touch->focus = view;
> >  }
> >  
> > -/**
> > - * notify_touch - emulates button touches and notifies surfaces 
> > accordingly.
> > - *
> > - * It assumes always the correct cycle sequence until it gets here: 
> > touch_down
> > - * → touch_update → ... → touch_update → touch_end. The driver is 
> > responsible
> > - * for sending along such order.
> > - *
> > - */
> > -WL_EXPORT void
> > -notify_touch(struct weston_touch_device *device, const struct timespec 
> > *time,
> > -int touch_id, double double_x, double double_y, int touch_type)
> > +static void
> > +process_touch_normal(struct weston_touch_device *device,
> > +const struct timespec *time, int touch_id,
> > +double double_x, double double_y, int touch_type)  
> 
> IMO just process_touch() is better, otherwise it's more confusing ("wait?
> why is this normal? and what's not normal? or is this an abbreviation of
> normalized?")

Actually, this is really the normal path. The not normal path is the
one for calibrator. You see the non-normal path added in "input:
introduce touch event mode for calibrator".

I didn't want to call it process_touch() originally, because it doesn't
process all touches, just the usual case. I even call it
WESTON_TOUCH_MODE_NORMAL.

Would you have other suggestions?


Thanks,
pq


pgpEUedgt3Y7u.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 16/25] libweston: introduce notify_touch_cal() and doc

[Pekka Paalanen]

On Mon, 9 Apr 2018 12:12:49 +1000
Peter Hutterer  wrote:

> On Fri, Mar 23, 2018 at 02:00:56PM +0200, Pekka Paalanen wrote:
> > From: Pekka Paalanen 
> > 
> > notify_touch_cal() is an extended form of notify_touch(), adding
> > normalized touch coordinates which are necessary for calibrating a
> > touchscreen.
> > 
> > It would be possible to invert the transformation and convert from
> > global coordinates to normalized device coordinates in input.c without
> > adding this API, but this way it is more robust against code changes.
> > 
> > Recovering normalized device coordinates is necessary because libinput
> > calibration matrix must be given in normalized units, and it would be
> > difficult to compute otherwise. Libinput API does not offer normalized
> > coordinates directly either, but those can be fetched by pretending the
> > output resolution is 1x1.
> > 
> > Anticipating touch calibration mode, the old notify_touch() is renamed
> > into a private process_touch_normal(), and the new notify_touch_cal()
> > delegates to it.
> > 
> > Co-developed by Louis-Francis and Pekka.
> > 
> > Cc: Louis-Francis Ratté-Boulianne 
> > Signed-off-by: Pekka Paalanen 
> > ---
> >  libweston/compositor.h  | 21 +++-
> >  libweston/input.c   | 60 
> > -
> >  libweston/libinput-device.c | 11 -
> >  3 files changed, 79 insertions(+), 13 deletions(-)
> > 

> > -/**
> > - * notify_touch - emulates button touches and notifies surfaces 
> > accordingly.
> > - *
> > - * It assumes always the correct cycle sequence until it gets here: 
> > touch_down
> > - * → touch_update → ... → touch_update → touch_end. The driver is 
> > responsible
> > - * for sending along such order.
> > - *
> > - */
> > -WL_EXPORT void
> > -notify_touch(struct weston_touch_device *device, const struct timespec 
> > *time,
> > -int touch_id, double double_x, double double_y, int touch_type)
> > +static void
> > +process_touch_normal(struct weston_touch_device *device,
> > +const struct timespec *time, int touch_id,
> > +double double_x, double double_y, int touch_type)  
> 
> IMO just process_touch() is better, otherwise it's more confusing ("wait?
> why is this normal? and what's not normal? or is this an abbreviation of
> normalized?")

Ok.

> >  {
> > struct weston_touch *touch = device->aggregate;
> > struct weston_touch_grab *grab = device->aggregate->grab;
> > @@ -2423,6 +2416,51 @@ notify_touch(struct weston_touch_device *device, 
> > const struct timespec *time,
> > }
> >  }
> >  
> > +/** Feed in touch down, motion, and up events, calibratable device.
> > + *
> > + * It assumes always the correct cycle sequence until it gets here: 
> > touch_down
> > + * → touch_update → ... → touch_update → touch_end. The driver is 
> > responsible
> > + * for sending along such order.
> > + *
> > + * \param device The physical device that generated the event.
> > + * \param time The event timestamp.
> > + * \param touch_id ID for the touch point of this event (multi-touch).
> > + * \param double_x X coordinate in compositor global space.
> > + * \param double_y Y coordinate in compositor global space.
> > + * \param norm_x Normalized device X coordinate in calibration space.
> > + * \param norm_y Normalized device Y coordinate in calibration space.
> > + * \param touch_type Either WL_TOUCH_DOWN, WL_TOUCH_UP, or WL_TOUCH_MOTION.
> > + *
> > + * Coordinates double_x and double_y are used for normal operation.
> > + *
> > + * Coordinates norm_x and norm_y are only used for touch device 
> > calibration.
> > + * If and only if the weston_touch_device does not support calibrating,
> > + * norm_x and norm_y must be WESTON_INVALID_TOUCH_COORDINATE.
> > + *
> > + * The calibration space is the normalized coordinate space
> > + * [0.0, 1.0]×[0.0, 1.0] of the weston_touch_device. This is assumed to
> > + * map to the similar normalized coordinate space of the associated
> > + * weston_output.
> > + */
> > +WL_EXPORT void
> > +notify_touch_cal(struct weston_touch_device *device,  
> 
> this should be notify_touch_normalized() because that's what the extra
> values are. That calibration affects those is just an implementation detail.
> 
> Also, at this point I can only say creating structs for each coordinate type
> in libinput has helped greatly in understanding what exactly you're dealing
> with at any point in time. see libinput-private.h:
> 
> /* A coordinate pair in device coordinates */
> struct device_coords {
> int x, y;
> };
> 
> /* A dpi-normalized coordinate pair */
> struct normalized_coords {
> double x, y;
> };
> 
> /* A pair of coordinates normalized to a [0,1] or [-1, 1] range */
> struct normalized_range_coords {
> double x, y;
> };
> 
> 
> etc. These are passed through the various functions, so 

Re: [PATCH weston 16/25] libweston: introduce notify_touch_cal() and doc

[Peter Hutterer]

On Fri, Mar 23, 2018 at 02:00:56PM +0200, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> notify_touch_cal() is an extended form of notify_touch(), adding
> normalized touch coordinates which are necessary for calibrating a
> touchscreen.
> 
> It would be possible to invert the transformation and convert from
> global coordinates to normalized device coordinates in input.c without
> adding this API, but this way it is more robust against code changes.
> 
> Recovering normalized device coordinates is necessary because libinput
> calibration matrix must be given in normalized units, and it would be
> difficult to compute otherwise. Libinput API does not offer normalized
> coordinates directly either, but those can be fetched by pretending the
> output resolution is 1x1.
> 
> Anticipating touch calibration mode, the old notify_touch() is renamed
> into a private process_touch_normal(), and the new notify_touch_cal()
> delegates to it.
> 
> Co-developed by Louis-Francis and Pekka.
> 
> Cc: Louis-Francis Ratté-Boulianne 
> Signed-off-by: Pekka Paalanen 
> ---
>  libweston/compositor.h  | 21 +++-
>  libweston/input.c   | 60 
> -
>  libweston/libinput-device.c | 11 -
>  3 files changed, 79 insertions(+), 13 deletions(-)
> 
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index b992b7eb..a1264e5a 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -1532,9 +1532,28 @@ notify_keyboard_focus_in(struct weston_seat *seat, 
> struct wl_array *keys,
>  void
>  notify_keyboard_focus_out(struct weston_seat *seat);
>  
> +/* an arbitrary unlikely value */
> +#define WESTON_INVALID_TOUCH_COORDINATE ((double)777e+7)
> +
>  void
> +notify_touch_cal(struct weston_touch_device *device,
> +  const struct timespec *time, int touch_id,
> +  double x, double y,
> +  double norm_x, double norm_y, int touch_type);
> +
> +/** Feed in touch down, motion, and up events, non-calibratable device.
> + *
> + * @sa notify_touch_cal
> + */
> +static inline void
>  notify_touch(struct weston_touch_device *device, const struct timespec *time,
> -  int touch_id, double x, double y, int touch_type);
> +  int touch_id, double x, double y, int touch_type)
> +{
> + notify_touch_cal(device, time, touch_id, x, y,
> +  WESTON_INVALID_TOUCH_COORDINATE,
> +  WESTON_INVALID_TOUCH_COORDINATE, touch_type);
> +}
> +
>  void
>  notify_touch_frame(struct weston_touch_device *device);
>  
> diff --git a/libweston/input.c b/libweston/input.c
> index bd7a9167..1658422c 100644
> --- a/libweston/input.c
> +++ b/libweston/input.c
> @@ -2336,17 +2336,10 @@ weston_touch_set_focus(struct weston_touch *touch, 
> struct weston_view *view)
>   touch->focus = view;
>  }
>  
> -/**
> - * notify_touch - emulates button touches and notifies surfaces accordingly.
> - *
> - * It assumes always the correct cycle sequence until it gets here: 
> touch_down
> - * → touch_update → ... → touch_update → touch_end. The driver is responsible
> - * for sending along such order.
> - *
> - */
> -WL_EXPORT void
> -notify_touch(struct weston_touch_device *device, const struct timespec *time,
> -  int touch_id, double double_x, double double_y, int touch_type)
> +static void
> +process_touch_normal(struct weston_touch_device *device,
> +  const struct timespec *time, int touch_id,
> +  double double_x, double double_y, int touch_type)

IMO just process_touch() is better, otherwise it's more confusing ("wait?
why is this normal? and what's not normal? or is this an abbreviation of
normalized?")

>  {
>   struct weston_touch *touch = device->aggregate;
>   struct weston_touch_grab *grab = device->aggregate->grab;
> @@ -2423,6 +2416,51 @@ notify_touch(struct weston_touch_device *device, const 
> struct timespec *time,
>   }
>  }
>  
> +/** Feed in touch down, motion, and up events, calibratable device.
> + *
> + * It assumes always the correct cycle sequence until it gets here: 
> touch_down
> + * → touch_update → ... → touch_update → touch_end. The driver is responsible
> + * for sending along such order.
> + *
> + * \param device The physical device that generated the event.
> + * \param time The event timestamp.
> + * \param touch_id ID for the touch point of this event (multi-touch).
> + * \param double_x X coordinate in compositor global space.
> + * \param double_y Y coordinate in compositor global space.
> + * \param norm_x Normalized device X coordinate in calibration space.
> + * \param norm_y Normalized device Y coordinate in calibration space.
> + * \param touch_type Either WL_TOUCH_DOWN, WL_TOUCH_UP, or WL_TOUCH_MOTION.
> + *
> + * Coordinates double_x and double_y are used for normal operation.
> + *
> + * Coordinates norm_x and norm_y 

[PATCH weston 16/25] libweston: introduce notify_touch_cal() and doc

[Pekka Paalanen]

From: Pekka Paalanen 

notify_touch_cal() is an extended form of notify_touch(), adding
normalized touch coordinates which are necessary for calibrating a
touchscreen.

It would be possible to invert the transformation and convert from
global coordinates to normalized device coordinates in input.c without
adding this API, but this way it is more robust against code changes.

Recovering normalized device coordinates is necessary because libinput
calibration matrix must be given in normalized units, and it would be
difficult to compute otherwise. Libinput API does not offer normalized
coordinates directly either, but those can be fetched by pretending the
output resolution is 1x1.

Anticipating touch calibration mode, the old notify_touch() is renamed
into a private process_touch_normal(), and the new notify_touch_cal()
delegates to it.

Co-developed by Louis-Francis and Pekka.

Cc: Louis-Francis Ratté-Boulianne 
Signed-off-by: Pekka Paalanen 
---
 libweston/compositor.h  | 21 +++-
 libweston/input.c   | 60 -
 libweston/libinput-device.c | 11 -
 3 files changed, 79 insertions(+), 13 deletions(-)

diff --git a/libweston/compositor.h b/libweston/compositor.h
index b992b7eb..a1264e5a 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -1532,9 +1532,28 @@ notify_keyboard_focus_in(struct weston_seat *seat, 
struct wl_array *keys,
 void
 notify_keyboard_focus_out(struct weston_seat *seat);
 
+/* an arbitrary unlikely value */
+#define WESTON_INVALID_TOUCH_COORDINATE ((double)777e+7)
+
 void
+notify_touch_cal(struct weston_touch_device *device,
+const struct timespec *time, int touch_id,
+double x, double y,
+double norm_x, double norm_y, int touch_type);
+
+/** Feed in touch down, motion, and up events, non-calibratable device.
+ *
+ * @sa notify_touch_cal
+ */
+static inline void
 notify_touch(struct weston_touch_device *device, const struct timespec *time,
-int touch_id, double x, double y, int touch_type);
+int touch_id, double x, double y, int touch_type)
+{
+   notify_touch_cal(device, time, touch_id, x, y,
+WESTON_INVALID_TOUCH_COORDINATE,
+WESTON_INVALID_TOUCH_COORDINATE, touch_type);
+}
+
 void
 notify_touch_frame(struct weston_touch_device *device);
 
diff --git a/libweston/input.c b/libweston/input.c
index bd7a9167..1658422c 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -2336,17 +2336,10 @@ weston_touch_set_focus(struct weston_touch *touch, 
struct weston_view *view)
touch->focus = view;
 }
 
-/**
- * notify_touch - emulates button touches and notifies surfaces accordingly.
- *
- * It assumes always the correct cycle sequence until it gets here: touch_down
- * → touch_update → ... → touch_update → touch_end. The driver is responsible
- * for sending along such order.
- *
- */
-WL_EXPORT void
-notify_touch(struct weston_touch_device *device, const struct timespec *time,
-int touch_id, double double_x, double double_y, int touch_type)
+static void
+process_touch_normal(struct weston_touch_device *device,
+const struct timespec *time, int touch_id,
+double double_x, double double_y, int touch_type)
 {
struct weston_touch *touch = device->aggregate;
struct weston_touch_grab *grab = device->aggregate->grab;
@@ -2423,6 +2416,51 @@ notify_touch(struct weston_touch_device *device, const 
struct timespec *time,
}
 }
 
+/** Feed in touch down, motion, and up events, calibratable device.
+ *
+ * It assumes always the correct cycle sequence until it gets here: touch_down
+ * → touch_update → ... → touch_update → touch_end. The driver is responsible
+ * for sending along such order.
+ *
+ * \param device The physical device that generated the event.
+ * \param time The event timestamp.
+ * \param touch_id ID for the touch point of this event (multi-touch).
+ * \param double_x X coordinate in compositor global space.
+ * \param double_y Y coordinate in compositor global space.
+ * \param norm_x Normalized device X coordinate in calibration space.
+ * \param norm_y Normalized device Y coordinate in calibration space.
+ * \param touch_type Either WL_TOUCH_DOWN, WL_TOUCH_UP, or WL_TOUCH_MOTION.
+ *
+ * Coordinates double_x and double_y are used for normal operation.
+ *
+ * Coordinates norm_x and norm_y are only used for touch device calibration.
+ * If and only if the weston_touch_device does not support calibrating,
+ * norm_x and norm_y must be WESTON_INVALID_TOUCH_COORDINATE.
+ *
+ * The calibration space is the normalized coordinate space
+ * [0.0, 1.0]×[0.0, 1.0] of the weston_touch_device. This is assumed to
+ * map to the similar normalized coordinate space of the associated
+ * weston_output.
+ */
+WL_EXPORT void