[PATCH libinput 05/10] filter: adjust acceleration curve depending on speed

[Hans de Goede]

Hi,

On 09/19/2014 07:44 AM, Peter Hutterer wrote:
> The acceleration curve consists of four parts, in ascii-art like this:
>         _____________
>        /
>   ____/
>  /
> /
> 
> where the x axis is the speed, y is the acceleration factor.
> The first plateau is at the acceleration factor 1 (i.e. unaccelerated
> movement), the second plateau is at the max acceleration factor. The threshold
> in the code defines where and how long the plateau is.
> 
> This patch adjusts the curve based on a [-1, 1] range. For anything below 0,
> the plateau is longer (i.e. accel kicks in at a higher speed), the second
> incline is flatter (i.e. accel kicks in slower) and the max accel factor is
> lower (i.e. maximum speed is slower). For anything above 0, the inverse is
> true, acceleration kicks in earlier, harder and is faster in general. So the
> default/min/max curves overlaid look something like this:
>        ________ max
>       | _______ default
>      | /  _____ min
>   ___|/_/
>  /
> /
> 
> Note that there's a limit to what ascii art can do...
> 
> Note that there are additional tweaks we can introduce later, such as
> decreaseing the unaccelerated speed of the device (i.e. lowering the first
> plateau).
> 
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

Patches 1-4 looks good, and are:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Comments on this one inline.


> ---
>  src/filter.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/filter.c b/src/filter.c
> index 327dbce..da5c232 100644
> --- a/src/filter.c
> +++ b/src/filter.c
> @@ -69,6 +69,7 @@ filter_get_speed(struct motion_filter *filter)
>  
>  #define DEFAULT_THRESHOLD 0.4			/* in units/ms */
>  #define DEFAULT_ACCELERATION 2.0		/* unitless factor */
> +#define DEFAULT_INCLINE 1.1			/* unitless factor */
>  
>  /*
>   * Pointer acceleration filter constants
> @@ -101,6 +102,7 @@ struct pointer_accelerator {
>  
>  	double threshold;	/* units/ms */
>  	double accel;		/* unitless factor */
> +	double incline;		/* incline of the function */
>  };
>  
>  static void
> @@ -255,10 +257,21 @@ static bool
>  accelerator_set_speed(struct motion_filter *filter,
>  		      double speed)
>  {
> +	struct pointer_accelerator *accel_filter =
> +		(struct pointer_accelerator *)filter;
> +
>  	assert(speed >= -1.0 && speed <= 1.0);
>  
> +	/* delay when accel kicks in */
> +	accel_filter->threshold = DEFAULT_THRESHOLD - (speed - 1)/6.0;
> +

Hmm, AFAIK a set_speed with 0 should lead to the defaults being set,
and that is not the case for the threshold here. I think you want
to use just speed rather then (speed - 1).

Also in the future in cases where you do use 1, and it is a double can
you please write 1.0 ?

> +	/* adjust max accel factor */
> +	accel_filter->accel = DEFAULT_ACCELERATION + speed;
> +
> +	/* higher speed -> faster to reach max */
> +	accel_filter->incline = DEFAULT_INCLINE + (speed + 1)/2.0;

Same comment as for the threshold, I think you want to drop the
+ 1.

> +
>  	filter->speed = speed;
> -
>  	return true;
>  }
>  
> @@ -290,6 +303,7 @@ create_pointer_accelator_filter(accel_profile_func_t profile)
>  
>  	filter->threshold = DEFAULT_THRESHOLD;
>  	filter->accel = DEFAULT_ACCELERATION;
> +	filter->incline = DEFAULT_INCLINE;
>  
>  	return &filter->base;
>  }
> @@ -314,9 +328,10 @@ pointer_accel_profile_linear(struct motion_filter *filter,
>  	double s1, s2;
>  	const double max_accel = accel_filter->accel; /* unitless factor */
>  	const double threshold = accel_filter->threshold; /* units/ms */
> +	const double incline = accel_filter->incline;
>  
>  	s1 = min(1, speed_in * 5);
> -	s2 = 1 + (speed_in - threshold) * 1.1;
> +	s2 = 1 + (speed_in - threshold) * incline;
>  
>  	return min(max_accel, s2 > 1 ? s2 : s1);
>  }
> 

Otherwise looks good.

Regards,

Hans