After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 678586 - Modifiy ClutterGestureAction to support multi touch and multiple points
Modifiy ClutterGestureAction to support multi touch and multiple points
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: ClutterAction
git master
Other Linux
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks: 678587
 
 
Reported: 2012-06-22 01:42 UTC by Lionel Landwerlin
Modified: 2012-07-17 00:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch v1 (14.42 KB, patch)
2012-06-22 01:48 UTC, Lionel Landwerlin
reviewed Details | Review
patch v2 (17.45 KB, patch)
2012-06-26 21:34 UTC, Lionel Landwerlin
reviewed Details | Review
patch v3 (17.46 KB, patch)
2012-07-12 17:38 UTC, Lionel Landwerlin
accepted-commit_now Details | Review

Description Lionel Landwerlin 2012-06-22 01:42:59 UTC
Currently the ClutterGestureAction only supports 1 pointer input. The attached patch add support for multiple points as well as touch events.
Comment 1 Lionel Landwerlin 2012-06-22 01:48:36 UTC
Created attachment 216985 [details] [review]
Patch v1

This patch contains a couple of lines of code in comment. I'm not really sure whether this commented check is useful for something else... but it prevents stuff like DragAction/ZoomAction/RotateAction to be implemented on top of the GestureAction.
Comment 2 Emmanuele Bassi (:ebassi) 2012-06-22 09:23:18 UTC
Review of attachment 216985 [details] [review]:

generally, yes: this is what I envisioned for GestureAction.

::: clutter/clutter-gesture-action.c
@@ +63,3 @@
 #include "clutter-private.h"
 
+#define MAX_GESTURE_POINTS (10)

the size is small, but I don't entirely like to preallocate 320 bytes per gesture (on 32 bit; on 64 is going to be 400 bytes) on top of the contents of the GestureAction and its subclasses. we could use a GArray instead, and prealloc 2 or 3 slots, considering that the majority of the gestures are going to involve a two or three touch points.

@@ +239,1 @@
+      /* if (!clutter_actor_transform_stage_point (actor, */

are you using GestureAction with direct children of the Stage? what happens when you start packing actors with gestures inside containers?

also: are you using a touchscreen device? the coordinate space changes between direct touch devices and indirect touch devices; I think this will need some API inside ClutterInputDevice to be able to distinguish the cases.

the documentation says that the coordinates we pass to the signal handlers are always actor-relative, not stage-relative, so we need the transformation here.

@@ +614,3 @@
+ * @action: a #ClutterGestureAction
+ *
+ * Retrieves the number of requested points to trigger the gesture.

"the number of touch points required to begin the gesture."

@@ +633,3 @@
+ * @nb_points: a number of points
+ *
+ */

same as above.

::: clutter/clutter-gesture-action.h
@@ +103,3 @@
 ClutterAction * clutter_gesture_action_new                      (void);
 
+gint            clutter_gesture_action_get_n_touch_points       (ClutterGestureAction *action);

missing CLUTTER_AVAILABLE_IN_1_12 annotations
Comment 3 Lionel Landwerlin 2012-06-22 09:34:36 UTC
Review of attachment 216985 [details] [review]:

::: clutter/clutter-gesture-action.c
@@ +239,1 @@
+      /* if (!clutter_actor_transform_stage_point (actor, */

First most of the gestures do not really care about relative coordinates, they mostly operate on vectors.

Then the way the function was called shows that the coordinates where never transformed before.

And finally during my tests this function often prevented the emission of the GESTURE_UPDATE signal, I need to check why the transformation might fail.
Comment 4 Emmanuele Bassi (:ebassi) 2012-06-22 13:20:36 UTC
the only two reasons I know of that the transformation may fail are:

  - the actor has either size of 0;
  - the actor's has been transformed into a line through rotation or scaling.

and you're right, the transformation is a check on whether we can actually transform the coordinates in actor space: I was confused by the code in DragAction, which does transform the event coordinates into actor space because of the clutter_actor_move_by() call inside drag-motion, as well as passing the delta inside the signal handler.

I think the transformation requirement should be dropped; Tomeu wrote GestureAction, so he may have an idea of whether it's something that was meant to catch some issue or not.
Comment 5 Lionel Landwerlin 2012-06-26 21:34:12 UTC
Created attachment 217330 [details] [review]
patch v2
Comment 6 Emmanuele Bassi (:ebassi) 2012-06-28 17:11:12 UTC
Review of attachment 217330 [details] [review]:

looks good, apart from a couple of minor details.

::: clutter/clutter-gesture-action.c
@@ +330,3 @@
     return CLUTTER_EVENT_PROPAGATE;
 
+  point = gesture_register_point (action, event);

this should warn if point == NULL - though you don't use point later on anyway.

::: clutter/clutter-gesture-action.h
@@ +103,3 @@
 ClutterAction * clutter_gesture_action_new                      (void);
 
+gint            clutter_gesture_action_get_n_touch_points       (ClutterGestureAction *action);

missing CLUTTER_AVAILABLE_IN_1_12 annotation.

@@ +104,3 @@
 
+gint            clutter_gesture_action_get_n_touch_points       (ClutterGestureAction *action);
+void            clutter_gesture_action_set_n_touch_points       (ClutterGestureAction *action,

missing CLUTTER_AVAILABLE_IN_1_12 annotation.
Comment 7 Lionel Landwerlin 2012-07-12 17:38:30 UTC
Created attachment 218652 [details] [review]
patch v3

Fixed the points in the review
Comment 8 Emmanuele Bassi (:ebassi) 2012-07-16 15:36:06 UTC
Review of attachment 218652 [details] [review]:

looks good.
Comment 9 Lionel Landwerlin 2012-07-17 00:31:33 UTC
landed.