[PATCH libinput 2/2] evdev-mt-touchpad: Switch to smooth simple acceleration code from filter.c

[Peter Hutterer]

On Mon, Jun 30, 2014 at 02:27:19PM +0200, Hans de Goede wrote:
> The old touchpad accel code was clamping touchpad acceleration between
> 0.2 and 0.4, and on the test devices I have the constant_factor ended up
> such that in practice the accel was almost always 0.2, so rather then having

typo: than

> a velocity based acceleration curve, in essence it was just always using an
> acceleration of 0.2 .
> 
> This commit introduces actual velocity based acceleration based on the
> recently added smooth simple acceleration code from filter.c .
> 
> Before feeding motion events to filter.c, they first get adjusted for touchpad
> resolution and size, based on the diagonal-size in units see the large comment
> added to tp_init_accel() for details.
> 
> While at it rename's tp_init_accel's dispatch parameter from touchpad to tp
> to be consistent with all other functions.
> 
> Since the acceleration is also used for scrolling also adjust the scroll
> start threshold for these changes.
> 
> Note that switching to the smooth simple accel code, as an added bonus gives
> the tp code an accel profile with a threshold and a speed parameter, which
> is exactly what is needed for the upcoming configuration interface support.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/evdev-mt-touchpad.c | 75 ++++++++++++++++++++++++++-----------------------
>  src/evdev-mt-touchpad.h |  4 ---
>  2 files changed, 40 insertions(+), 39 deletions(-)
> 
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 9e858f1..0e43586 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -28,9 +28,7 @@
>  
>  #include "evdev-mt-touchpad.h"
>  
> -#define DEFAULT_CONSTANT_ACCEL_NUMERATOR 100
> -#define DEFAULT_MIN_ACCEL_FACTOR 0.20
> -#define DEFAULT_MAX_ACCEL_FACTOR 0.40
> +#define DEFAULT_SIZE_ACCEL_NUMERATOR 1200.0
>  #define DEFAULT_HYSTERESIS_MARGIN_DENOMINATOR 700.0
>  
>  static inline int
> @@ -46,27 +44,6 @@ tp_hysteresis(int in, int center, int margin)
>  		return center + diff + margin;
>  }
>  
> -static double
> -tp_accel_profile(struct motion_filter *filter,
> -		 void *data,
> -		 double velocity,
> -		 uint64_t time)
> -{
> -	struct tp_dispatch *tp =
> -		(struct tp_dispatch *) data;
> -
> -	double accel_factor;
> -
> -	accel_factor = velocity * tp->accel.constant_factor;
> -
> -	if (accel_factor > tp->accel.max_factor)
> -		accel_factor = tp->accel.max_factor;
> -	else if (accel_factor < tp->accel.min_factor)
> -		accel_factor = tp->accel.min_factor;
> -
> -	return accel_factor;
> -}
> -
>  static inline struct tp_motion *
>  tp_motion_history_offset(struct tp_touch *t, int offset)
>  {
> @@ -464,11 +441,11 @@ tp_post_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
>  
>  	tp_filter_motion(tp, &dx, &dy, time);
>  
> -	/* Require at least three px scrolling to start */
> -	if (dy <= -3.0 || dy >= 3.0)
> +	/* Require at least five px scrolling to start */
> +	if (dy <= -5.0 || dy >= 5.0)
>  		tp->scroll.direction |= (1 << LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL);
>  
> -	if (dx <= -3.0 || dx >= 3.0)
> +	if (dx <= -5.0 || dx >= 5.0)
>  		tp->scroll.direction |= (1 << LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL);
>  
>  	if (dy != 0.0 &&
> @@ -695,22 +672,50 @@ calculate_scale_coefficients(struct tp_dispatch *tp)
>  }
>  
>  static int
> -tp_init_accel(struct tp_dispatch *touchpad, double diagonal)
> +tp_init_accel(struct tp_dispatch *tp, double diagonal)
>  {
>  	struct motion_filter *accel;
>  
> -	calculate_scale_coefficients(touchpad);
> +	calculate_scale_coefficients(tp);
> +
> +	/*
> +	 * Not all touchpads report the same amount of units/mm (resolution).
> +	 * Besides resolution we also want to take the touchpad size into
> +	 * account, on a small touchpad we want to report a bigger pointer
> +	 * motion per mm moved.
> +	 *
> +	 * The kernel reports touchpad resolution only for a subset of all
> +	 * supported touchpads, and without resolution info it is impossible
> +	 * to calculate the physical size, so we cannot use either.
> +	 *
> +	 * Since physical-size == resolution * size-in-units, we end up with
> +	 * a scaling formula which has resolution as a factor in both
> +	 * the numerator and denominator, so we can simply leave resolution
> +	 * out of the formula and scale only by size-in-units.
> +	 *
> +	 * Example 1: A small touchpad with a high resultion needs to have
> +	 * events scaled up for its size, and scaled down for its resolution.
> +	 * This touchpad has small-physical-size * high-amount-units/mm =
> +	 * average-size-in-units.
> +	 *
> +	 * Example 2: A small touchpad with a low resolution needs to have
> +	 * events scaled up twice once for its small size, once for its low
> +	 * res. This touchpad has small-physical-size * low-amount-units/mm =
> +	 * quite-low-size-in-units.
> +	 *
> +	 * Scaling events by average-size-in-units / touchpad-size-in-units
> +	 * will result in the desired scaling in both cases.
> +	 */

The idea is interesting. Indeed on the T440 with a relatively large and
square-ish touchpad, this is much better now. On the x220 with a small
16:10 ratio touchpad it's better but not yet good. It feels like the pointer
acceleration kicks in too quickly and too strongly, so there is no middle
ground: movement is either slow and precise or it overshoots the target.

Both have the same scaling factor (0.24) but due to the resolution, the
values are oviously quite different. I played with adjusting the
accel/threshold parameters on both and both touchpads can be made to feel
good by adjusting the threshold a bit.

That doesn't bode well for the idea of "small touchpads should move the
pointer move", but I don't know how to measure this generically or even
objectively.

how did you come up with 1200 for DEFAULT_SIZE_ACCEL_NUMERATOR? I found a
value of 800 to be good enough by default on both, but that's
going to change with every touchpad. What touchpads did you test this on?

What about using the resolution to normalize movements based on physical
movement where possible and falling back to this method where the resolution
is set?

Talking about the patch:
What's confusing about the comment is that you talk about leaving resolution
out, right after we calculate the scaling coefficients based on the
resolution. Which of course only serves to correct uneven resolutions, but
still, it confused me. I'd like to see this reworded somehow.

Cheers,
   Peter



>  
> -	touchpad->accel.constant_factor =
> -		DEFAULT_CONSTANT_ACCEL_NUMERATOR / diagonal;
> -	touchpad->accel.min_factor = DEFAULT_MIN_ACCEL_FACTOR;
> -	touchpad->accel.max_factor = DEFAULT_MAX_ACCEL_FACTOR;
> +	tp->accel.x_scale_coeff *= DEFAULT_SIZE_ACCEL_NUMERATOR / diagonal;
> +	tp->accel.y_scale_coeff *= DEFAULT_SIZE_ACCEL_NUMERATOR / diagonal;
>  
> -	accel = create_pointer_accelator_filter(tp_accel_profile);
> +	accel = create_pointer_accelator_filter(
> +			pointer_accel_profile_smooth_simple);
>  	if (accel == NULL)
>  		return -1;
>  
> -	touchpad->filter = accel;
> +	tp->filter = accel;
>  
>  	return 0;
>  }
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 7afb3c4..051a7eb 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -152,10 +152,6 @@ struct tp_dispatch {
>  	struct motion_filter *filter;
>  
>  	struct {
> -		double constant_factor;
> -		double min_factor;
> -		double max_factor;
> -
>  		double x_scale_coeff;
>  		double y_scale_coeff;
>  	} accel;
> -- 
> 2.0.0
>