[PATCH libinput v2 4/4] touchpad: Implement pinch gesture support
Hans de Goede
hdegoede at redhat.com
Thu Apr 16 06:52:37 PDT 2015
Hi,
On 09-04-15 07:25, Peter Hutterer wrote:
> On Thu, Mar 26, 2015 at 10:04:40AM +0100, Hans de Goede wrote:
>> Implement touchpad pinch (and rotate) gesture support.
>>
>> Note that two two-finger scrolling tests are slightly tweaked to assure that
>> there is enough touch movement to allow the scroll-or-pinch detect code to do
>> its works.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> src/evdev-mt-touchpad-gestures.c | 268 ++++++++++++++++++++++++++++++++++++++-
>> src/evdev-mt-touchpad.h | 17 +++
>> src/libinput-private.h | 33 +++++
>> test/touchpad.c | 4 +-
>> 4 files changed, 315 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c
>> index 017354c..3d3dcaf 100644
>> --- a/src/evdev-mt-touchpad-gestures.c
>> +++ b/src/evdev-mt-touchpad-gestures.c
>> @@ -22,7 +22,6 @@
>>
>> #include "config.h"
>>
>> -#include <assert.h>
>> #include <math.h>
>> #include <stdbool.h>
>> #include <limits.h>
>> @@ -30,6 +29,7 @@
>> #include "evdev-mt-touchpad.h"
>>
>> #define DEFAULT_GESTURE_SWITCH_TIMEOUT 100 /* ms */
>> +#define DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT 1000 /* ms */
>>
>> static struct normalized_coords
>> tp_get_touches_delta(struct tp_dispatch *tp, bool average)
>> @@ -75,6 +75,7 @@ tp_get_average_touches_delta(struct tp_dispatch *tp)
>> static void
>> tp_gesture_start(struct tp_dispatch *tp, uint64_t time)
>> {
>> + struct libinput *libinput = tp->device->base.seat->libinput;
>> const struct normalized_coords zero = { 0.0, 0.0 };
>>
>> if (tp->gesture.started)
>> @@ -82,7 +83,21 @@ tp_gesture_start(struct tp_dispatch *tp, uint64_t time)
>>
>> switch (tp->gesture.finger_count) {
>> case 2:
>> - /* NOP */
>> + switch (tp->gesture.twofinger_state) {
>> + case GESTURE_2FG_STATE_NONE:
>> + case GESTURE_2FG_STATE_UNKNOWN:
>> + log_bug_libinput(libinput,
>> + "%s in unknown gesture mode\n", __func__);
>
> nitpick: for multi-line calls please line up the arguments with the opening
> parenthesis where possible and use one arg per line. So this would become:
> log_bug_libinput(libinput,
> "%s in unknown gesture mode\n",
> __func__);
Thanks for the review.
Fixed in the gestures branch of my personal repo.
>
>
>> + break;
>> + case GESTURE_2FG_STATE_SCROLL:
>> + /* NOP */
>> + break;
>> + case GESTURE_2FG_STATE_PINCH:
>> + gesture_notify_pinch(&tp->device->base, time,
>> + LIBINPUT_EVENT_GESTURE_PINCH_START,
>> + &zero, &zero, 0.0, 0.0);
>> + break;
>> + }
>> break;
>> case 3:
>> case 4:
>> @@ -114,8 +129,172 @@ tp_gesture_post_pointer_motion(struct tp_dispatch *tp, uint64_t time)
>> }
>> }
>>
>> +static unsigned int
>> +tp_gesture_get_active_touches(struct tp_dispatch *tp,
>> + struct tp_touch **touches,
>> + unsigned int count)
>> +{
>> + unsigned int i, n = 0;
>> + struct tp_touch *t;
>> +
>> + memset(touches, 0, count * sizeof(struct tp_touch *));
>> +
>> + for (i = 0; i < tp->real_touches; i++) {
>> + t = &tp->touches[i];
>> + if (tp_touch_active(tp, t)) {
>> + touches[n++] = t;
>> + if (n == count)
>> + return count;
>> + }
>> + }
>> +
>> + /*
>> + * This can happen when the user does .e.g:
>> + * 1) Put down 1st finger in center (so active)
>> + * 2) Put down 2nd finger in a button area (so inactive)
>> + * 3) Put down 3th finger somewhere, gets reported as a fake finger,
>> + * so gets same coordinates as 1st -> active
>> + *
>> + * We could avoid this by looking at all touches, be we really only
>> + * want to look at real touches.
>> + */
>> + return n;
>> +}
>> +
>> +static int
>> +tp_gesture_get_direction(struct tp_dispatch *tp, int touch)
>> +{
>> + struct normalized_coords normalized;
>> + double move_threshold;
>> +
>> + /*
>> + * Semi-mt touchpads have somewhat inaccurate coordinates when
>> + * 2 fingers are down, so use a slightly larger threshold.
>> + */
>> + if (tp->semi_mt)
>> + move_threshold = TP_MM_TO_DPI_NORMALIZED(4);
>> + else
>> + move_threshold = TP_MM_TO_DPI_NORMALIZED(3);
>> +
>> + normalized = tp_normalize_delta(tp,
>> + device_delta(tp->gesture.touches[touch]->point,
>> + tp->gesture.touches[touch]->gesture.initial));
>
> temp. variable please, with the indentation it's not immediately obvious
> what calls what, so this would be better as:
>
> struct tp_touch *t = &tp->gesture.touches[touch];
> struct device_float_coords delta;
>
> delta = device_delta(t->point, t->gesture.initial);
> normalized = tp_normalize_delta(tp, &delta);
>
> also - any reason you're passing in the touch index rather than a
> struct tp_touch directly?
No reason, both fixed in the gestures branch of my personal repo.
>
>
>> + if (normalized_length(normalized) < move_threshold)
>> + return UNDEFINED_DIRECTION;
>> +
>> + return normalized_get_direction(normalized);
>> +}
>> +
>> +static void
>> +tp_gesture_get_pinch_info(struct tp_dispatch *tp,
>> + double *distance,
>> + double *angle,
>> + struct device_float_coords *center)
>> +{
>> + struct normalized_coords normalized;
>> +
>> + normalized = tp_normalize_delta(tp,
>> + device_delta(tp->gesture.touches[0]->point,
>> + tp->gesture.touches[1]->point));
>
> same as above, except for the function argument.
> struct tp_touch *first = &tp->gestures.touches[0],
> *second = &tp->gestures.touches[1];
>
> delta = device_delta(first->point, second->point);
> normalized = ...
>
Fixed everywhere.
>> + *distance = normalized_length(normalized);
>> +
>> + if (!tp->semi_mt)
>> + *angle = atan2(normalized.y, normalized.x) * 180.0 / M_PI;
>> + else
>> + *angle = 0.0;
>> +
>> + *center = device_average(tp->gesture.touches[0]->point,
>> + tp->gesture.touches[1]->point);
>
> and re-use first/second here.
>
>> +}
>> +
>> +static void
>> +tp_gesture_set_scroll_buildup(struct tp_dispatch *tp)
>> +{
>> + struct device_float_coords d0, d1;
>> +
>> + d0 = device_delta(tp->gesture.touches[0]->point,
>> + tp->gesture.touches[0]->gesture.initial);
>> + d1 = device_delta(tp->gesture.touches[1]->point,
>> + tp->gesture.touches[1]->gesture.initial);
>
> here too.
>
>> +
>> + tp->device->scroll.buildup =
>> + tp_normalize_delta(tp, device_float_average(d0, d1));
>> +}
>> +
>> +static void
>> +tp_gesture_twofinger_state_none(struct tp_dispatch *tp, uint64_t time)
>> +{
>> + if (tp_gesture_get_active_touches(tp, tp->gesture.touches, 2) != 2)
>> + return;
>> +
>> + tp->gesture.initial_time = time;
>> + tp->gesture.touches[0]->gesture.initial =
>> + tp->gesture.touches[0]->point;
>> + tp->gesture.touches[1]->gesture.initial =
>> + tp->gesture.touches[1]->point;
>
> here too.
>> +
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_UNKNOWN;
>> +}
>> +
>> +static void
>> +tp_gesture_twofinger_state_unknown(struct tp_dispatch *tp, uint64_t time)
>> +{
>> + struct normalized_coords normalized;
>> + int dir0, dir1;
>> +
>> + normalized = tp_normalize_delta(tp,
>> + device_delta(tp->gesture.touches[0]->point,
>> + tp->gesture.touches[1]->point));
>
> and here, which then also measns you can re-use first/second in the
> tp_gesture_get_direction() call, which goes nicely with one of my comments
> above.
>
>> +
>> + /* If fingers are further than 3 cm apart assume pinch */
>> + if (normalized_length(normalized) > TP_MM_TO_DPI_NORMALIZED(30)) {
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_PINCH;
>> + tp_gesture_get_pinch_info(tp,
>> + &tp->gesture.distance,
>> + &tp->gesture.angle,
>> + &tp->gesture.center);
>> + return;
>> + }
>> +
>> + /* Elif fingers have been close together for a while, scroll */
>> + if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) {
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_SCROLL;
>> + tp_gesture_set_scroll_buildup(tp);
>> + return;
>> + }
>> +
>> + /* Else wait for both fingers to have moved */
>> + dir0 = tp_gesture_get_direction(tp, 0);
>> + dir1 = tp_gesture_get_direction(tp, 1);
>> + if (dir0 == UNDEFINED_DIRECTION || dir1 == UNDEFINED_DIRECTION)
>> + return;
>> +
>> + /*
>> + * If both touches are moving in the same direction assume scroll.
>> + *
>> + * In some cases (semi-mt touchpads) We may seen one finger move
>> + * e.g. N/NE and the other W/NW so we not only check for overlapping
>> + * directions, but also for neighboring bits being set.
>> + * The ((dira & 0x80) && (dirb & 0x01)) checks are to check for bit 0
>> + * and 7 being set as they also represent neighboring directions.
>> + */
>> + if (((dir0 | (dir0 >> 1)) & dir1) ||
>> + ((dir1 | (dir1 >> 1)) & dir0) ||
>> + ((dir0 & 0x80) && (dir1 & 0x01)) ||
>> + ((dir1 & 0x80) && (dir0 & 0x01))) {
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_SCROLL;
>> + tp_gesture_set_scroll_buildup(tp);
>> + } else {
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_PINCH;
>> + tp_gesture_get_pinch_info(tp,
>> + &tp->gesture.distance,
>> + &tp->gesture.angle,
>> + &tp->gesture.center);
>> + }
>> +}
>> +
>> static void
>> -tp_gesture_post_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
>> +tp_gesture_twofinger_state_scroll(struct tp_dispatch *tp, uint64_t time)
>> {
>> struct normalized_coords delta;
>>
>> @@ -133,6 +312,65 @@ tp_gesture_post_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
>> }
>>
>> static void
>> +tp_gesture_twofinger_state_pinch(struct tp_dispatch *tp, uint64_t time)
>> +{
>> + double angle, d, distance;
>> + struct device_float_coords center;
>> + struct normalized_coords delta, unaccel;
>> +
>> + tp_gesture_get_pinch_info(tp, &distance, &angle, ¢er);
>> +
>> + d = distance - tp->gesture.distance;
>> + tp->gesture.distance = distance;
>> + distance = d;
>> +
>> + d = angle - tp->gesture.angle;
>> + if (d > 180.0)
>> + d -= 360.0;
>> + else if (d < -180.0)
>> + d += 360.0;
>> + tp->gesture.angle = angle;
>> + angle = d;
>> +
>> + unaccel = tp_normalize_delta(tp,
>> + device_float_delta(center, tp->gesture.center));
>
> same as above
>
>> + tp->gesture.center = center;
>> + delta = tp_filter_motion(tp, &unaccel, time);
>> +
>> + if (normalized_is_zero(delta) && normalized_is_zero(unaccel) &&
>> + distance == 0.0 && angle == 0.0)
>> + return;
>> +
>> + tp_gesture_start(tp, time);
>> + gesture_notify_pinch(&tp->device->base, time,
>> + LIBINPUT_EVENT_GESTURE_PINCH_UPDATE,
>> + &delta, &unaccel, distance, angle);
>> +}
>> +
>> +static void
>> +tp_gesture_post_twofinger(struct tp_dispatch *tp, uint64_t time)
>> +{
>> + switch (tp->gesture.twofinger_state) {
>> + case GESTURE_2FG_STATE_NONE:
>> + tp_gesture_twofinger_state_none(tp, time);
>> + if (tp->gesture.twofinger_state != GESTURE_2FG_STATE_UNKNOWN)
>> + break;
>> + /* fall through */
>> + case GESTURE_2FG_STATE_UNKNOWN:
>> + tp_gesture_twofinger_state_unknown(tp, time);
>> + if (tp->gesture.twofinger_state != GESTURE_2FG_STATE_SCROLL)
>> + break;
>> + /* fall through */
>> + case GESTURE_2FG_STATE_SCROLL:
>> + tp_gesture_twofinger_state_scroll(tp, time);
>> + break;
>> + case GESTURE_2FG_STATE_PINCH:
>> + tp_gesture_twofinger_state_pinch(tp, time);
>> + break;
>> + }
>
> I don't think a switch statement is good for this type of logic. the
> fallthroughs and behind-our-back state changes are not really obvious.
> Better to use a if chain instead:
>
> if (state == NONE)
> handle_state_none();
>
> if (state == unknown)
> handle state_unknown();
>
> etc. ideally you'd return the new state to make it even more obvious rather
> than setting the state in the struct and re-checking that.
Good idea, fixed.
> also, right now if you switch from none to scroll you post an event
> immediately, but for pinch you don't until the next run. That seems
> unbalanced and a bit odd, is there a reason for this?
Yes, we post the scroll immediately to reduce latency, when can do this
as we've the per touch motion history which will be used to generate
an event (if enough events have buildup there).
For pinch this not possible pinch sends delta events of the finger distance
so once we lock into pinch mode we get the initial distance, and then
on the next event we can start sending events. We could save a set
of initial pinch info, but I do not think this is a good idea for
2 reasons:
1) When starting with the fingers close together there may be a significant
amount of finger movement before we decide this is a pinch, if we then send
a single delta for all that movement the result will be a large delta at
once, where as with scrolling if the user wants to scroll slow he will
be moving his fingers slowly and the initial event will match this.
2) This requires extra code (with scrolling we get this for free)
>
>> +}
>> +
>> +static void
>> tp_gesture_post_swipe(struct tp_dispatch *tp, uint64_t time)
>> {
>> struct normalized_coords delta, unaccel;
>> @@ -171,7 +409,7 @@ tp_gesture_post_events(struct tp_dispatch *tp, uint64_t time)
>> tp_gesture_post_pointer_motion(tp, time);
>> break;
>> case 2:
>> - tp_gesture_post_twofinger_scroll(tp, time);
>> + tp_gesture_post_twofinger(tp, time);
>> break;
>> case 3:
>> case 4:
>> @@ -191,14 +429,32 @@ tp_gesture_stop_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
>> void
>> tp_gesture_stop(struct tp_dispatch *tp, uint64_t time)
>> {
>> + struct libinput *libinput = tp->device->base.seat->libinput;
>> + enum tp_gesture_2fg_state twofinger_state = tp->gesture.twofinger_state;
>> const struct normalized_coords zero = { 0.0, 0.0 };
>>
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_NONE;
>> +
>> if (!tp->gesture.started)
>> return;
>>
>> switch (tp->gesture.finger_count) {
>> case 2:
>> - tp_gesture_stop_twofinger_scroll(tp, time);
>> + switch (twofinger_state) {
>> + case GESTURE_2FG_STATE_NONE:
>> + case GESTURE_2FG_STATE_UNKNOWN:
>> + log_bug_libinput(libinput,
>> + "%s in unknown gesture mode\n", __func__);
>> + break;
>> + case GESTURE_2FG_STATE_SCROLL:
>> + tp_gesture_stop_twofinger_scroll(tp, time);
>> + break;
>> + case GESTURE_2FG_STATE_PINCH:
>> + gesture_notify_pinch(&tp->device->base, time,
>> + LIBINPUT_EVENT_GESTURE_PINCH_END,
>> + &zero, &zero, 0.0, 0.0);
>> + break;
>> + }
>> break;
>> case 3:
>> case 4:
>> @@ -258,6 +514,8 @@ tp_gesture_handle_state(struct tp_dispatch *tp, uint64_t time)
>> int
>> tp_init_gesture(struct tp_dispatch *tp)
>> {
>> + tp->gesture.twofinger_state = GESTURE_2FG_STATE_NONE;
>> +
>> libinput_timer_init(&tp->gesture.finger_count_switch_timer,
>> tp->device->base.seat->libinput,
>> tp_gesture_finger_count_switch_timeout, tp);
>> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
>> index 19a262e..83f0492 100644
>> --- a/src/evdev-mt-touchpad.h
>> +++ b/src/evdev-mt-touchpad.h
>> @@ -120,6 +120,13 @@ enum tp_edge_scroll_touch_state {
>> EDGE_SCROLL_TOUCH_STATE_AREA,
>> };
>>
>> +enum tp_gesture_2fg_state {
>> + GESTURE_2FG_STATE_NONE,
>> + GESTURE_2FG_STATE_UNKNOWN,
>> + GESTURE_2FG_STATE_SCROLL,
>> + GESTURE_2FG_STATE_PINCH,
>> +};
>> +
>> struct tp_touch {
>> struct tp_dispatch *tp;
>> enum touch_state state;
>> @@ -171,6 +178,10 @@ struct tp_touch {
>> struct device_coords first; /* first coordinates if is_palm == true */
>> uint32_t time; /* first timestamp if is_palm == true */
>> } palm;
>> +
>> + struct {
>> + struct device_coords initial;
>> + } gesture;
>> };
>>
>> struct tp_dispatch {
>> @@ -205,6 +216,12 @@ struct tp_dispatch {
>> unsigned int finger_count;
>> unsigned int finger_count_pending;
>> struct libinput_timer finger_count_switch_timer;
>> + enum tp_gesture_2fg_state twofinger_state;
>> + struct tp_touch *touches[2];
>> + uint64_t initial_time;
>> + double distance;
>> + double angle;
>> + struct device_float_coords center;
>> } gesture;
>>
>> struct {
>> diff --git a/src/libinput-private.h b/src/libinput-private.h
>> index 2c9f42b..b433126 100644
>> --- a/src/libinput-private.h
>> +++ b/src/libinput-private.h
>> @@ -391,6 +391,39 @@ device_delta(struct device_coords a, struct device_coords b)
>> return delta;
>> }
>>
>> +static inline struct device_float_coords
>> +device_average(struct device_coords a, struct device_coords b)
>> +{
>> + struct device_float_coords average;
>> +
>> + average.x = (a.x + b.x) / 2.0;
>> + average.y = (a.y - b.y) / 2.0;
>
> this doesn't look right, is this intended?
Nope copy paste error (I started with copying the delta func), fixed.
Regards,
Hans
More information about the wayland-devel
mailing list