Mario, After looking over the patch, I like the idea that you've got here. There are a couple of questions that I have for you though, and a few small requested changes. Please look them over.
Also, in the future, please post patches inline if possible. It makes replying with comments a lot easier :) > Dear developers, > > The following patch adds the possibility (with the default parameters it have > no effect) to remove distortion on the borders of the screen by defining a > border width, if the stylus is in the border (according to its width) the > position of the cursor is corrected by a polynomial of degree 3. The four > coefficients of the polynomial must also be given in parameter. > Therefore there is 4(topx topy bottomx bottomy) x (1(width)+4(polynomial > coefficients)) = 20 floating point values in the parameter. > > To find the appropriate parameters I am creating a calibration tool, it > calibrate the center of the screen then it calibrate the borders. > git link : https://github.com/antigol/wacom-distortion > The calibration tool is still in development but it seems to work. > > Yours faithfully, > Mario Geiger > > distortion.patch > > diff --git include/wacom-properties.h include/wacom-properties.h > index b845083..54e7793 100644 > --- include/wacom-properties.h > +++ include/wacom-properties.h > @@ -27,6 +27,9 @@ > /* 32 bit, 4 values, top x, top y, bottom x, bottom y */ > #define WACOM_PROP_TABLET_AREA "Wacom Tablet Area" > > +/* 32 bit, 4x5=20 values, 4x[border width, polynomial coefficient x^3, x^2, > x, 1] */ > +#define WACOM_PROP_TABLET_DISTORTION "Wacom Border Distortion" > + Have you considered allowing a variable length? Changing the functions to support polynomials of arbitrary degree would be trivial and could potentially allow for more accurate calibration (obviously at the cost of increased CPU use). > /* 8 bit, 1 value, [0 - 3] (NONE, CW, CCW, HALF) */ > #define WACOM_PROP_ROTATION "Wacom Rotation" > > diff --git src/wcmCommon.c src/wcmCommon.c > index 9408f42..f8be62e 100644 > --- src/wcmCommon.c > +++ src/wcmCommon.c > @@ -438,6 +438,21 @@ static void sendCommonEvents(InputInfoPtr pInfo, const > WacomDeviceState* ds, > sendWheelStripEvents(pInfo, ds, first_val, num_vals, valuators); > } > > +static double wcmBorderDistortionCorrection(double coord, float border, > float* polynomial) I'd love to see a comment documenting what this function does and its use :) > +{ > + if (coord < border) { > + double x = coord; > + coord = polynomial[0]; > + coord *= x; > + coord += polynomial[1]; // I wonder if double+float is slower > than double+double The speed difference is probably going to be minimal. I'd be tempted to use 'float' everywhere since it will have more than enough precision to represent the coordinates, and is what the coefficients are already stored in. > + coord *= x; > + coord += polynomial[2]; > + coord *= x; > + coord += polynomial[3]; > + } > + return coord; > +} > + > /* rotate x and y before post X inout events */ > void wcmRotateAndScaleCoordinates(InputInfoPtr pInfo, int* x, int* y) > { > @@ -446,19 +461,36 @@ void wcmRotateAndScaleCoordinates(InputInfoPtr pInfo, > int* x, int* y) > DeviceIntPtr dev = pInfo->dev; > AxisInfoPtr axis_x, axis_y; > int tmp_coord; > + double f; > > /* scale into on topX/topY area */ > axis_x = &dev->valuator->axes[0]; > axis_y = &dev->valuator->axes[1]; > > /* Don't try to scale relative axes */ > - if (axis_x->max_value > axis_x->min_value) > - *x = xf86ScaleAxis(*x, axis_x->max_value, axis_x->min_value, > - priv->bottomX, priv->topX); > - > - if (axis_y->max_value > axis_y->min_value) > - *y = xf86ScaleAxis(*y, axis_y->max_value, axis_y->min_value, > - priv->bottomY, priv->topY); > + if (axis_x->max_value > axis_x->min_value) { > + f = (*x - priv->topX) / (double)(priv->bottomX - priv->topX); > + f = wcmBorderDistortionCorrection(f, > priv->distortion_topX_border, priv->distortion_topX_poly); > + f = 1.0 - f; > + f = wcmBorderDistortionCorrection(f, > priv->distortion_bottomX_border, priv->distortion_bottomX_poly); > + f = 1.0 - f; > + Do you think it would be reasonable/better/worse to work with the actual coordinates instead of a fraction? I'm not sure which would be better, myself. Also, my gut says that the correction should be done in the tablet's native coordindate space (axis_x->min_value; axis_x->max_value) rather than the currently-set area (priv->topX, priv->topY). I'm not 100% sure though given that it might make it quite difficult for programs to calculate the polynomial coefficients. I'll have to think a little harder... Finally, it wasn't immediately obvious why you were doing all this subtraction. It took a moment for it to connect that the math that's being done in 'wcmBorderDistortionCorrection' has its origin at the edge being worked on rather than (0,0). You might consider reworking things a little for clarity. > + *x = round(f * (axis_x->max_value - axis_x->min_value) + > axis_x->min_value); > + if (*x < axis_x->min_value) *x = axis_x->min_value; > + if (*x > axis_x->max_value) *x = axis_x->max_value; > + } > + > + if (axis_y->max_value > axis_y->min_value) { > + f = (*y - priv->topY) / (double)(priv->bottomY - priv->topY); > + f = wcmBorderDistortionCorrection(f, > priv->distortion_topY_border, priv->distortion_topY_poly); > + f = 1.0 - f; > + f = wcmBorderDistortionCorrection(f, > priv->distortion_bottomY_border, priv->distortion_bottomY_poly); > + f = 1.0 - f; > + > + *y = round(f * (axis_y->max_value - axis_y->min_value) + > axis_y->min_value); > + if (*y < axis_y->min_value) *y = axis_y->min_value; > + if (*y > axis_y->max_value) *y = axis_y->max_value; > + } > > /* coordinates are now in the axis rage we advertise for the device */ > > diff --git src/wcmXCommand.c src/wcmXCommand.c > index 346ff61..ec0d81c 100644 > --- src/wcmXCommand.c > +++ src/wcmXCommand.c > @@ -33,6 +33,12 @@ > #define XI_PROP_PRODUCT_ID "Device Product ID" > #endif > > +#ifndef XATOM_FLOAT > +#define XATOM_FLOAT "FLOAT" > +#endif > + Not necessary -- see below. > +static Atom float_type; > + > static void wcmBindToSerial(InputInfoPtr pInfo, unsigned int serial); > > > /***************************************************************************** > @@ -82,6 +88,7 @@ int wcmDevSwitchMode(ClientPtr client, DeviceIntPtr dev, > int mode) > static Atom prop_devnode; > static Atom prop_rotation; > static Atom prop_tablet_area; > +static Atom prop_distortion; > static Atom prop_pressurecurve; > static Atom prop_serials; > static Atom prop_serial_binding; > @@ -204,11 +211,23 @@ static Atom InitWcmAtom(DeviceIntPtr dev, const char > *name, Atom type, int forma > return atom; > } > > +static Atom InitFloatAtom(DeviceIntPtr dev, const char *name, int nvalues, > float *values) > +{ > + Atom atom; > + > + atom = MakeAtom(name, strlen(name), TRUE); > + XIChangeDeviceProperty(dev, atom, float_type, 32, > + PropModeReplace, nvalues, > values, FALSE); Your tab size is off (4 instead of 8). This should have 3 tabs and 7 spaces. > + XISetDevicePropertyDeletable(dev, atom, FALSE); > + return atom; > +} > + > void InitWcmDeviceProperties(InputInfoPtr pInfo) > { > WacomDevicePtr priv = (WacomDevicePtr) pInfo->private; > WacomCommonPtr common = priv->common; > int values[WCM_MAX_BUTTONS]; > + float fvalues[20]; > int i; > > DBG(10, priv, "\n"); > @@ -227,6 +246,23 @@ void InitWcmDeviceProperties(InputInfoPtr pInfo) > prop_tablet_area = InitWcmAtom(pInfo->dev, > WACOM_PROP_TABLET_AREA, XA_INTEGER, 32, 4, values); > } > > + if (!IsPad(priv)) { > + float_type = XIGetKnownProperty(XATOM_FLOAT); > + if (!float_type) float_type = MakeAtom(XATOM_FLOAT, > strlen(XATOM_FLOAT), TRUE); > + The typical idiom is to use something like: float_atom = XInternAtom(dpy, "FLOAT", False); which will does the same thing as these two lines and just uses a hardcoded string. > + if (float_type) { > + // topX, topY, bottomX, bottomY > + for (i = 0; i < 4; ++i) { > + fvalues[i*5+0] = 0.0; // border > + fvalues[i*5+1] = 0.0; // x^3 > + fvalues[i*5+2] = 0.0; // x^2 > + fvalues[i*5+3] = 1.0; // x > + fvalues[i*5+4] = 0.0; // 1 > + } > + prop_distortion = InitFloatAtom(pInfo->dev, > WACOM_PROP_TABLET_DISTORTION, 20, fvalues); > + } > + } > + > values[0] = common->wcmRotate; > if (!IsPad(priv)) { > prop_rotation = InitWcmAtom(pInfo->dev, WACOM_PROP_ROTATION, > XA_INTEGER, 8, 1, values); > @@ -683,6 +719,15 @@ int wcmDeleteProperty(DeviceIntPtr dev, Atom property) > return (i >= 0) ? BadAccess : Success; > } > > +static void setDistortionProperty(float* values, float *border, float > *polynomial) Instead of using inline comments below, I would prefer if you added a comment block to the top of this function which describes what it does and its use. > +{ > + *border = values[0]; // border width relatively to the screen > dimension > + polynomial[0] = values[1]; // x^3 coefficient of the polynomial > + polynomial[1] = values[2]; // x^2 coefficient > + polynomial[2] = values[3]; // x coefficient > + polynomial[3] = values[4]; // constant coefficient > +} > + > int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop, > BOOL checkonly) > { > @@ -717,6 +762,20 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, > XIPropertyValuePtr prop, > priv->bottomX = values[2]; > priv->bottomY = values[3]; > } > + } else if (property == prop_distortion) > + { > + float *values = (float*)prop->data; > + > + if (prop->size != 20 || prop->format != 32 || prop->type != > float_type) > + return BadValue; > + > + if (!checkonly) > + { > + setDistortionProperty(values, > &priv->distortion_topX_border, priv->distortion_topX_poly); > + setDistortionProperty(values+5, > &priv->distortion_topY_border, priv->distortion_topY_poly); > + setDistortionProperty(values+10, > &priv->distortion_bottomX_border, priv->distortion_bottomX_poly); > + setDistortionProperty(values+15, > &priv->distortion_bottomY_border, priv->distortion_bottomY_poly); > + } > } else if (property == prop_pressurecurve) > { > INT32 *pcurve; > diff --git src/xf86WacomDefs.h src/xf86WacomDefs.h > index 1575960..2d76514 100644 > --- src/xf86WacomDefs.h > +++ src/xf86WacomDefs.h > @@ -262,6 +262,16 @@ struct _WacomDeviceRec > unsigned int cur_serial; /* current serial in prox */ > int cur_device_id; /* current device ID in prox */ > > + /* distortion */ > + float distortion_topX_border; > + float distortion_topY_border; > + float distortion_bottomX_border; > + float distortion_bottomY_border; > + float distortion_topX_poly[4]; > + float distortion_topY_poly[4]; > + float distortion_bottomX_poly[4]; > + float distortion_bottomY_poly[4]; > + > /* button mapping information > * > * 'button' variables are indexed by physical button number > (0..nbuttons) -- Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... ------------------------------------------------------------------------------ _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
