Merge lp:~kevin-wells/libgrip/touch-points into lp:libgrip

[Kevin Wells]

For demonstration purposes I needed to display the actual touch points used in the gestures exposed by libgrip, but that information was not in the event structures (although those points are contained in the geis attributes).

Also, the information passed in the libgrip event structures is not enough to recreate how the object being transformed should react to the gesture. As an example, I am using the pinch, drag, and rotate gestures to apply transformations to a map with the goal that the points on the map that are pressed stay under the touch points as the touch points move around the screen. I realize this could only work with 2 touch points, but my netbook only supports 2 touch points. The problem is that the only notion of position of the event is focus_x and focus_y. In the case of a pinch event, only knowing the focus_x and focus_y and radius_delta will only allow the map to be tracked accurately if both points have moved towards or away from the focus an equal amount (radius_delta / 2). If one point is kept stationary and the second is moved toward or away from the focus, tracking on the map becomes inaccurate.

While this is ultimately an issue with the gesture recognition library, geis provides finger positions, and with that additional information, this issue can be avoided. This branch adds finger_x and finger_y arrays of size fingers to the pinch, drag, and rotate event structures and populates them with point information from geis.

If this is something that is deemed useful, I can update the rectangle-mover example to show touch points and demonstrate the issue I mentioned.

[Stephen M. Webb (bregma)]

I don't have a problem with propagating the touch points in general, but be aware that the significance of the touch points depends on the type of device. For example, a number of touchpads report only the bounding box, which will give focus and radius but not information about which individual points are in motion. The actual coordinates of a touchpad (an indirect device) may also not correspond to points on the screen in the same way as touchscreen touches do.

In short, I approve this patch as it is as a way to propagate the touch information through the libgrip structures, but also note it is insufficient for the purpose stated since that also requires the device information to be propagated. That information can come in a separate patch.

I also note the indentation appears to be a little messed up in a couple of places, particularly the parameters lists.

I'll wait and see what others have to say.

review: Approve

[lp:~kevin-wells/libgrip/touch-points]

40. By Kevin Wells on 2011-03-22

Fix indentation

[Henrik Rydberg (rydberg)]

You may be interested in this branch: lp:~utouch-team/utouch-grail/grail2, which contains an example code, grail-transform, that does what you describe.

[Chase Douglas (chasedouglas)]

Hi Kevin,

I'm not opposed to adding the touches, but I don't think they are actually necessary for what you want to do.

First, I'd like to define the meaning of the focus coordinates. These are the central coordinates of the touches when the gesture begins. They are used for event propagation to the correct X window and then to the correct toolkit widget. The values of the focus coordinates do not change during the gesture lifetime.

Also, all values are in screen coordinates. One unit of a value from the uTouch stack is equal to one pixel on screen.

When you receive gesture events, you can think of them as transformations upon a point on screen. Thus, a drag event moves this point by the delta_{x,y} amount. A pinch event does not move the point, but the object being interacted with may be shrunk or expanded.

The key here is that the gesture event transformations are not about the focus coordinates. They are about the current transformation position. Unfortunately, this is where we begin to see a problem with libgrip as it currently exists. The transformation position isn't provided in the libgrip gesture events. If you run geistest (from the utouch-geis-tools package), you'll see "position x" and "position y" attributes. These are the current transformation coordinates about which pinch to zoom or rotations should be applied.

My thoughts are that we should do the following:

1. Immediately add the transformation position coordinates to libgrip and push to natty.
2. Hold off on merging this change until device types are added to libgrip (as Stephen wrote, the device type is necessary for correct interpretation of touch coordinates)

[Henrik Rydberg (rydberg)]

I think the problem is clear to everyone, these things were originally scheduled to be fixed in natty, but we had to push some things to oneiric, hence there is still a bit of confusion.

First thing - yes, using coordinates makes perfect sense for some applications, I think the question is how these coordinates should be presented. In grail2 (see link above), we have started exposing the touch frames through the grail2 interface, which gives you access to all touch information, not only coordinates. The touch frames are supposed to also be visible through geis, as abstracted touch frames. The implementation further exposing these in libgrip should ideally be using those, although this can of course change later, as they become available.

Second thing - the information needed for accurate transforms is only partly present, and it is not only a question of clear definitions, but also about backwards compatibility. The transformation needs the pivot (position of scale and rotate), the scale, the angle, and the drag. These values can be used to construct the 3x3 transformation matrix, as seen in the grail_element. Note that the drag is different from the center movement, by a factor (called moveness in grail2) which depends on the difference between the center point and the pivot point. Since the center movement is also important, in particular for applications not using rotation, both the center movement and the drag (or at least the moveness factor) needs to be present in the events. Hence, the current geis events will not be quite enough either.

Since the event properties in the current geis/grail implementation carries certain semantics, perfectly reasonable for the simpler rigid case implemented so far, we might even have to consider adding new properties for all parts of the new transformations.

To conclude, I think your patches make perfect sense, and they point at the current deficiencies very well. However, I think we may need to take this opportunity to add all of the missing pieces in libgrip, based on the considerations so outlined.

[Stephen M. Webb (bregma)]

I think this discussion has moved beyond the scope if this particular merge request and should be continued in another forum.

This particular merge request was simply to propagate some additional information the original proposer found useful. I agree it is missing data and it should not hurt to add it. There may well be additional data that should also be added: let them be proposed in additional merge requests.

New functionality not yet available in GEIS or qhich require a rewrite of libgrip to use GEIS v2 are also not in scope.

Please give an approve/disapprove for the proposed merge request.

[Henrik Rydberg (rydberg)]

I would like to commend the idea and the initiative, but await the notion of touch frames through libgrip.

review: Disapprove

[Kevin Wells (kevin-wells)]

> Hi Kevin,
>
> I'm not opposed to adding the touches, but I don't think they are actually
> necessary for what you want to do.
>
> First, I'd like to define the meaning of the focus coordinates. These are the
> central coordinates of the touches when the gesture begins. They are used for
> event propagation to the correct X window and then to the correct toolkit
> widget. The values of the focus coordinates do not change during the gesture
> lifetime.
>
> Also, all values are in screen coordinates. One unit of a value from the
> uTouch stack is equal to one pixel on screen.
>
> When you receive gesture events, you can think of them as transformations upon
> a point on screen. Thus, a drag event moves this point by the delta_{x,y}
> amount. A pinch event does not move the point, but the object being interacted
> with may be shrunk or expanded.
>
> The key here is that the gesture event transformations are not about the focus
> coordinates. They are about the current transformation position.
> Unfortunately, this is where we begin to see a problem with libgrip as it
> currently exists. The transformation position isn't provided in the libgrip
> gesture events. If you run geistest (from the utouch-geis-tools package),
> you'll see "position x" and "position y" attributes. These are the current
> transformation coordinates about which pinch to zoom or rotations should be
> applied.
>
> My thoughts are that we should do the following:
>
> 1. Immediately add the transformation position coordinates to libgrip and push
> to natty.
> 2. Hold off on merging this change until device types are added to libgrip (as
> Stephen wrote, the device type is necessary for correct interpretation of
> touch coordinates)

I have been using focus_{x,y} as the transformation position, but did discover that it was in screen coordinates and needed to be converted. When I run geistest, it seems that focus_{x,y} is equal to position_{x,y} for drag events (position is not present in pinch or rotate), but this also means that focus_{x,y} is changing throughout the lifetime of the gesture. I am running Maverick, so I am not sure if this is something that has changed recently.

I didn't know that different devices report point touch coordinates differently (I only have one device multitouch capable), and I agree that device information would be useful when considering using touch points.

[Stephen M. Webb (bregma)]

Given the disapprovals and the bitrot, I'm rejecting this proposal.

Unmerged revisions

40. By Kevin Wells on 2011-03-22

Fix indentation

39. By Kevin Wells on 2011-03-22

Use slice allocators instead of g_new for finger points

38. By Kevin Wells on 2011-03-22

Add finger_x and finger_y to drag and rotate events.

37. By Kevin Wells on 2011-03-10

Add finger locations to Pinch gestures

=== modified file 'src/gripgesturemanager.c'
--- src/gripgesturemanager.c	2011-01-24 20:04:47 +0000
+++ src/gripgesturemanager.c	2011-03-22 23:57:27 +0000
@@ -221,6 +221,72 @@
   gobject_class->finalize    = grip_gesture_manager_finalize;
 }
 
+static void
+gesture_handle_finger_properties (gint                fingers,
+                                  gdouble           **finger_x,
+                                  gdouble           **finger_y,
+                                  GeisSize            attr_count,
+                                  GeisGestureAttr    *attrs)
+{
+  *finger_x = g_slice_alloc0 (sizeof (gdouble) * fingers);
+  *finger_y = g_slice_alloc0 (sizeof (gdouble) * fingers);
+
+  int i;
+  for (i = 0; i < attr_count; ++i)
+    {
+      if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_0_X) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_x)[0] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_0_Y) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_y)[0] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_1_X) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_x)[1] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_1_Y) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_y)[1] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_2_X) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_x)[2] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_2_Y) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_y)[2] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_3_X) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_x)[3] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_3_Y) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_y)[3] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_4_X) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_x)[4] = attrs[i].float_val;
+        }
+      else if (g_strcmp0 (attrs[i].name, GEIS_GESTURE_ATTRIBUTE_TOUCH_4_Y) == 0 &&
+               attrs[i].type == GEIS_ATTR_TYPE_FLOAT)
+        {
+          (*finger_y)[4] = attrs[i].float_val;
+        }
+    }
+}
+
 static gint
 pinch_gesture_handle_properties (GripEventGesturePinch *event,
                                  GeisSize               attr_count,
@@ -268,6 +334,12 @@
         }
     }
 
+  gesture_handle_finger_properties (touches, 
+                                    &event->finger_x, 
+                                    &event->finger_y, 
+                                    attr_count, 
+                                    attrs);
+
   return touches;
 }
 
@@ -333,6 +405,12 @@
         }
     }
 
+  gesture_handle_finger_properties (touches, 
+                                    &event->finger_x, 
+                                    &event->finger_y, 
+                                    attr_count, 
+                                    attrs);
+
   return touches;
 }
 
@@ -382,6 +460,12 @@
           event->angle = attrs[i].float_val;
         }
     }
+  
+  gesture_handle_finger_properties (touches, 
+                                    &event->finger_x, 
+                                    &event->finger_y, 
+                                    attr_count, 
+                                    attrs);
 
   return touches;
 }
@@ -1260,6 +1344,28 @@
   new_event = grip_gesture_event_new (event->type);
   *new_event = *event;
 
+  if (event->type == GRIP_GESTURE_DRAG)
+    {
+      new_event->drag.finger_x = g_slice_copy (sizeof (gdouble) * event->drag.fingers, 
+                                               event->drag.finger_x);
+      new_event->drag.finger_y = g_slice_copy (sizeof (gdouble) * event->drag.fingers,
+                                               event->drag.finger_y);
+    }
+  else if (event->type == GRIP_GESTURE_PINCH)
+    {
+      new_event->pinch.finger_x = g_slice_copy (sizeof (gdouble) * event->pinch.fingers,
+                                                event->pinch.finger_x);
+      new_event->pinch.finger_y = g_slice_copy (sizeof (gdouble) * event->pinch.fingers,
+                                                event->pinch.finger_y);
+    }
+  else if (event->type == GRIP_GESTURE_ROTATE)
+    {
+      new_event->rotate.finger_x = g_slice_copy (sizeof (gdouble) * event->rotate.fingers,
+                                                 event->rotate.finger_x);
+      new_event->rotate.finger_y = g_slice_copy (sizeof (gdouble) * event->rotate.fingers,
+                                                 event->rotate.finger_y);
+    }
+
   return new_event;
 }
 
@@ -1268,5 +1374,21 @@
 {
   g_return_if_fail (event != NULL);
 
+  if (event->type == GRIP_GESTURE_DRAG)
+    {
+      g_slice_free1 (sizeof (gdouble) * event->drag.fingers, event->drag.finger_x);
+      g_slice_free1 (sizeof (gdouble) * event->drag.fingers, event->drag.finger_y);
+    }
+  else if (event->type == GRIP_GESTURE_PINCH)
+    {
+      g_slice_free1 (sizeof (gdouble) * event->pinch.fingers, event->pinch.finger_x);
+      g_slice_free1 (sizeof (gdouble) * event->pinch.fingers, event->pinch.finger_y);
+    }
+  else if (event->type == GRIP_GESTURE_ROTATE)
+    {
+      g_slice_free1 (sizeof (gdouble) * event->rotate.fingers, event->rotate.finger_x);
+      g_slice_free1 (sizeof (gdouble) * event->rotate.fingers, event->rotate.finger_y);
+    }
+
   g_slice_free (GripGestureEvent, event);
 }

=== modified file 'src/gripgesturemanager.h'
--- src/gripgesturemanager.h	2011-01-21 17:19:26 +0000
+++ src/gripgesturemanager.h	2011-03-22 23:57:27 +0000
@@ -78,6 +78,8 @@
   gint              fingers;
   gdouble           focus_x;
   gdouble           focus_y;
+  gdouble          *finger_x;
+  gdouble          *finger_y;
   gint              delta_x;
   gint              delta_y;
   gdouble           velocity_x;
@@ -97,6 +99,8 @@
   guint             fingers;
   gdouble           focus_x;
   gdouble           focus_y;
+  gdouble          *finger_x;
+  gdouble          *finger_y;
   gdouble           radius_delta;
   gdouble           radial_velocity;
   gdouble           radius;
@@ -113,6 +117,8 @@
   guint             fingers;
   gdouble           focus_x;
   gdouble           focus_y;
+  gdouble          *finger_x;
+  gdouble          *finger_y;
   gdouble           angle_delta;
   gdouble           angular_velocity;
   gdouble           angle;