[PATCH libinput v5 5/5] libinput: add orientation and size of touch point and pressure to the API

[Peter Hutterer]

On Thu, Jul 09, 2015 at 08:06:28AM +0200, Andreas Pokorny wrote:
> Hi,
> 
> 
> On Wed, Jul 8, 2015 at 1:03 AM, Peter Hutterer <peter.hutterer at who-t.net>
> wrote:
> 
> > > +double
> > > +evdev_device_transform_touch_point_to_mm(struct evdev_device *device,
> > > +                                      int32_t axis_value,
> > > +                                      double axis_angle)
> >
> > oh dear. I just spent about 15 min trying to figure out the conversion and
> > wondering why you pass x/y in here etc. "point" refers to a point on the
> > screen. You're passing in the size of the ellipsis so please don't name
> > this
> > touch_point, it's very confusing. Name it ellipsis instead, that's how you
> > call it in the test suite too.
> >
> 
> Hm yes this the unified code of what was duplicated in the previous version.
> Agreed using the term point when the visual extent of something meant is
> confusing.
> 
> 
> > > +{
> > > +     double x_res = device->abs.absinfo_x->resolution;
> > > +     double y_res = device->abs.absinfo_y->resolution;
> > > +
> > > +     if (x_res == y_res)
> > > +             return axis_value / x_res;
> > > +
> > > +     if (device->abs.absinfo_orientation == NULL)
> > > +             return axis_value * 2.0 / (x_res + y_res);
> >
> > I admit, I'm a bit confused in how this should work. if we don't have
> > orientation, the angle should always be 0, right?
> > so the returned value is either /xres or /yres, depending on what you pass
> > in. can you explain this in more detail for me please?
> >
> 
> This is a rather broken case - the screen uses different vertical and
> horizontal
> resolution, but we have not orientation value. So the code estimates the
> resolution by taking the average. I do not have access to such a touch
> device
> but android considered this case.

right, add a comment then that this is what this code is supposed to do,
that would help making sense of this.

> 
> > > +
> > > +     return axis_value / (y_res*fabs(cos(deg2rad(axis_angle))) +
> > > +                          x_res*fabs(sin(deg2rad(axis_angle))));
> >
> > can we please use a tmp scale variable for the fabs(cos(deg2rad()))
> > nesting?
> > (besides, please dont mix spaces with no spaces)
> >
> > also, this is doing my head in. we're using x for the touch major and y for
> > the touch minor, despite, in the default angle, the major being the y axis
> > and the minor being the x axis. Which leads to the odd case of
> > in the simplest case, we have an angle of 0 -> cos 1, sin 0. a touch major
> > is assigned to touch_point.x and the returned value is x/yres. that's
> > correct I think, but confusing as hell.
> >
> > I think we should assign major to y and minor to x, that would make things
> > a
> > lot less confusing.
> >
> 
> To be honest not placing the values inside a device_coords structure is
> probably a
> safer way to avoid further confusion. What about struct ellipse { int
> major, minor};?

sure, give that a try and see how it works. if we don't need to convert to
much back and forth then that's the most sensible plan. also, in that case
you can probably pack orientation into that struct as well so you have them
grouped together.
 
 
> > [...]
> > > + */
> > > +double
> > > +libinput_event_touch_get_minor_transformed(struct libinput_event_touch
> > *event,
> > > +                                        uint32_t width,
> > > +                                        uint32_t height);
> > > +
> > > +/**
> > > + * @ingroup event_touch
> > > + *
> > > + * Return the current pressure value touch point normalized to the range
> > > + * [0,1]. If this value is not provided by the device, it is always 0.0.
> >
> > thinking about this - we will at some point have to add hovering to
> > libinput and that's where a pressure default of 0 is going to bite us.
> > I'd go with the opposite and say that if pressure doesn't exist, it's
> > always
> > 1 on touch.
> >
> 
> I had 1.0 originally so that users do not have to care whether the value is
> availble or not.
> You convinced me that 0.0 would be better since it allows users to deal
> with both cases.

right, for major/minor a value of 0 is appropriate because it cannot be
physically achieved by a touchpad. and since they're not normalized we don't
have any other magic values we could return anyway.

for normalized values though the rules are a bit different. for orientation
0 is a good default because it's quite close to what the finger is likely to
be at. for pressure - it is possible to have a pressure of 0 or almost zero
(we're working with a normalized range, so depending on what operations you
apply 0.01 may look the same as 0).
and since we say that we normalize to [0, 1], the _correct_ value is 1 when
we have a touch. it makes client code more straightforward because you can
rely on the value being useful.

if clients end up needing to know whether there is pressure, a
event_touch_has_pressure() is probably the best option.

CC-ing carlos, since he's one of the prime users his opinion matters
a lot to me.

> Now for hover (as in no physical contact i guess?) - why not resolve it by
> having another
> touch event type like .. LIBINPUT_EVENT_TOUCH_HOVER to differ between these
> cases? Pressure would not be defined for that event type, even when the
> device provides
> pressure on physical contacts. So 0.0 to indicate a lacking capability on
> TOUCH_MOTION
> still seems ok.

the touch_hover event is a good idea and it ends up being backwards
compatible - clients that don't handle hover now won't break if hover is
added to touch sequences.

Cheers,
   Peter