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

[Andreas Pokorny]

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.


> > +
> > +     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};?


> [...]
> > + */
> > +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.
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.


regards
Andreas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150709/e25935c2/attachment-0001.html>