|
Description
Simon Thum
2006-10-10 04:08:13 UTC
Created attachment 7319 [details]
Documentation for the patch
Created attachment 7322 [details] [review] Patch for mouse speed/acceleration Created attachment 7492 [details]
updated documentation
Created attachment 7493 [details] [review] patch for enhanced mouse acceleration Some code cleanup, removed c++-comments, some optimization hi, the algorithms and implementation look pretty good to me at a first glance. could you please redo this against master, though? dix/getevents.c is what you want to be looking at: GetPointerEvents will call acceleratePointer if POINTER_ACCELERATE is set in flags. so what you want to be doing is storing all your information in the ValuatorClassRec (include/inputstr.h), and then changing which acceleration function you call. also, please wrap your lines at 80 characters. thanks! Raising the code to dix is entirely possible but i order to get configuration done I will have to extend the dix interface (export functions and a struct). If this is not a good idea please write soon. that's fine. i suggest you just add an acceleration type integer to ValuatorClassRec, and then a void *accelData or so, that the acceleration method of your choice can use. that way, we only ever need to break abi once. thanks! Created attachment 7772 [details] [review] Patch for enhanced acceleration The code is now in dix, but needs to be enabled for every server, which is exemplary in hw/xfree86. However, I have not yet managed to build Xorg, so the whole thing is untested and might not even compile. Could someone (Daniel?) please apply it and give me some feedback? Created attachment 7773 [details]
Documentation for the patch
Documentation for the patch in general and configuration options
Just noted there is a small error: This line: https://bugs.freedesktop.org/attachment.cgi?id=7772&action=diff#a/dix/getevents.c_sec4 is superfluous and should not compile. Going to respost later. Created attachment 7858 [details] [review] patch against master I finally managed to get the code to dix so one can see the effects. Sadly, there is a bug somewhere in the server that prevents (adaptive) deceleration from working, which IMHO ist the most appealing feature. Also, the integration is sort of a hack, but a more involved dev should easily be able to improve that. @Daniel: It seems that there is a Bug which shows up when you do e.g. xset m 2 0 then slow movements are screwed. It seems that if the acceleration does not accelerate but rather slow down strange things happen. You should not need to apply my patch to reproduce. Created attachment 7859 [details]
slightly revised doc.
I've been able to check my clain there is a bug in current git and it is (bug#9156). simply do >xset m 2 0 and see youself. Since by default the proposed algo does not decelerate, it hides the problem, as well as not using polynomial acceleration does. I tried to fix but it seemingly depends on other factors. IMHO, the acc function should be able to decelerate, but currently it's a mess. Created attachment 7969 [details] [review] patch against master This should now work and also fix the classic implementation. Integration is still suboptimal, but it works fine. Created attachment 7970 [details]
Documentation for the patch
Hi, I hope this is a good place to leave this comment. I'm not a dev, but I'm very interested in this patch. I just want to say thank you very much to Simon for his work in implementing it. I would encourage the developers to think seriously about including this in the mainstream code as soon as possible; I've seen a fair bit of wishing on the web (often among laptop users, because of their more restricted mouse requirements) that X had a smoother mouse acceleration function. Created attachment 8201 [details] [review] revised patch This patch should again apply in current master branch. I am still seeking feedback on this. Especially, I need hints on improving integration in xfree86 backend: 1) how to suppress those carryover_ fields (see ApplyExtendedValuatorSettings) 2) should the dix api be changed to support the new valuator fields 3) where is a good place to apply the extended settings (or to call ApplyExtendedValuatorSettings)? Must be after DEVICE_INIT (for valuators to exist), and I'd like to stay device driver independent. Also, hotplug is an issue. Thanks! some cosmetics: Use #defines for scheme names. Scheme 0 or 1 is not very explanatory. Can you write one acceleratePointer and then split from there into accelPointerDumb, accelPointerSmart or whatever you want to name them? GPE shouldn't decide on the accel scheme and it'll make it more readable. Don't call it carryover_accelScheme, don't be too apologetic. You might want to put scheme and data into a struct, it'll make it easier to add additional stuff at a later point in time. please don't mix tabs and spaces. http://wiki.x.org/wiki/CodingStyle xf86ProcessCommonOptions: bit hacky. don't strcmp, alloc memory inside PVC and call it dependent on option AccelerationScheme. PVC can then decide what to do. Thanks for the comments! Those carryover_ fields are in effect a hack I got rid of already. When it works (current Git won't on my machine even vanilla) youl'll see a post :) I'll see what I can do about the rest. *** Bug 10295 has been marked as a duplicate of this bug. *** Created attachment 9248 [details] [review] Patch agains 1.2.0 This patch is against a 7.2 release / 1.2.0. It contains some improvements in config handling and velocity approximation. The patch against git is not currently stable. Created attachment 9389 [details] [review] patch against current git This patch contains some of peters suggestions. I haven't introduced #defines but made up a function pointer to exchange schemes, so it can be implemented compile-time checked. Init/shutdown still needs improvement. Created attachment 9390 [details]
documentation
Revised documentation
There's a load of needless whitespace diffs in that patch, seems your whitespacing differs to what the file already has. Makes it very hard to read. Apart from that it looks good from the little I know. (In reply to comment #25) > There's a load of needless whitespace diffs in that patch, seems your > whitespacing differs to what the file already has. Makes it very hard to read. > Apart from that it looks good from the little I know. Got that one. In your previous post you said I shouldn't strcmp, but what am I supposed to do to match pointing devices? (In reply to comment #26) > (In reply to comment #25) > > There's a load of needless whitespace diffs in that patch, seems your > > whitespacing differs to what the file already has. Makes it very hard to read. > > Apart from that it looks good from the little I know. > Got that one. In your previous post you said I shouldn't strcmp, but what am I > supposed to do to match pointing devices? if dev->valuator exists, then that's enough to set up acceleration. the meat of the patch seems okay, but i have a few style issues with it (mainly adding two new headers and source files just for acceleration), so i'll clean it up and push it to an api-break branch this week. Created attachment 9565 [details] [review] patch against master this patch contains some minor improvements and works against current git. The 2 headers are overkill, mainly a failed attempt to integrate the 1.2.0 and current git patch versions. IMHO putting ptraccel.h into input[str].h should be best, since that would preserve the fact you only need ptrveloc.h if you actually fiddle with it. I have some improvements in queue[0], but apart from the small mem hole in CloseDevice() I don't know why it shouldn't be applied now. Created attachment 10106 [details] [review] revised patch against master This version of the patch has some improvements: - it now has only one additional header - slow diagonal moves are measured more precise - no known memory leaks - improved structure so adding arbitrary acceleration functions should be easier to do in an ABI-compatible manner Created attachment 10107 [details]
documentation
Created attachment 10162 [details] [review] patch against xorg-server 1.2.0 This patch contains an improvement backported from the git version so slow diagonal movements are handled better. It is also tested with multiple devices (mouse and synaptics). Created attachment 11617 [details] [review] updated patch against current master I've updated the patch to include profile selection, added a few acceleration profiles, and made the header structure more concise. Created attachment 11618 [details]
updated documentation
updated documentation.
Can velocity data be freed through a callback function? Better than checking for the AccelSheme in CloseDevice. I did not understand why xf86InitValuatorAxisStruct only applies the new settings if there are zero axis? typo? It is better if you move this into InitValuatorAxisStruct, I guess xf86IVAS will die out over time. aside from that, I like it. I haven't checked the actual algorithms for the acceleration though. One thing: hotplugging should be covered with your code once you move to IVAS instead of xf86IVAS. I recommend trying this scheme with both the mouse and the evdev driver. These are the most common ones in use. Cosmetics: #include "ptrveloc.h" instead of #include <ptrveloc.h> Indendation seems screwed in ApplySofteningAndConstantDeceleration, ClassicProfile Please avoid // comments For javadoc-like comments preceding each line with an asterisk makes the code easier to read (admittedly subjectively) /** * foobar */ ptrveloc.h: can you move the comments to be next to the declarations instead of above? It's horrendous to read right now, especially w/o syntax highlighting. (In reply to comment #34) > Can velocity data be freed through a callback function? Better than checking > for the AccelSheme in CloseDevice. > > I did not understand why xf86InitValuatorAxisStruct only applies the new > settings if there are zero axis? typo? > It is better if you move this into InitValuatorAxisStruct, I guess xf86IVAS > will die out over time. You've exactly hit the spots I consider suboptimal too :) It's not a typo, but makes sure AEVS is called only once, as xf86IVAS is called once for each axis. The reason I rely on all this is simply I need to evalute the config file, which makes that part viable only in xfree86 (AFAIK). I'm just not sure how to treat the issue such that I don't introduce more problems. E.g. moving it in IVAS would make that fn take more parameters, and suddenly all its callers get involved. Any thoughts? > aside from that, I like it. I haven't checked the actual algorithms for the > acceleration though. I don't feel like a math whiz, but I'm pretty sure they're fine :) Basically I think there should be some research but I don't know who would pay _that_. > One thing: hotplugging should be covered with your code once you move to IVAS > instead of xf86IVAS. I recommend trying this scheme with both the mouse and the > evdev driver. These are the most common ones in use. Regarding hotplug, I have not idea how to deal with config here. A XI DeviceControl API is some work, as I've seen. > Cosmetics: When it compiles again :) (In reply to comment #35) > It's not a typo, but makes sure AEVS is called only once, as xf86IVAS is called > once for each axis. > The reason I rely on all this is simply I need to evalute the config file, > which makes that part viable only in xfree86 (AFAIK). I'm just not sure how to > treat the issue such that I don't introduce more problems. > E.g. moving it in IVAS would make that fn take more parameters, and suddenly > all its callers get involved. Any thoughts? two options: - you modify all the callers. depends on when you want it to be in the server. there's more input ABI breakage down the road anyway. - you create a second interface InitValuatorAxisStructWithAccel() and let IVAS call IVASWA with sane defaults. this is probably the better idea for now. > Regarding hotplug, I have not idea how to deal with config here. A XI > DeviceControl API is some work, as I've seen. not as much as you'd think. we (well, daniels preferrably :) can probably knock something up in quite short time. Created attachment 11756 [details] [review] preliminary patch I chose option three: I applied your suggestion on cleanup in CloseDevice to init, and from the dix/ddx decoupling perspective I am quite satisfied now. The patch *DOES NOT WORK* but it shows how I intend to do it. However I need to modify ActivateDecice() or something like that. Are there any objections? I've seen there was some cleanup in the DeviceControl handling, maybe I'll take a a second chance an that. If we head for 1.5, there should be some time left, right? (In reply to comment #37) > Created an attachment (id=11756) [details] > preliminary patch > > I chose option three: I applied your suggestion on cleanup in CloseDevice to > init, and from the dix/ddx decoupling perspective I am quite satisfied now. The > patch *DOES NOT WORK* but it shows how I intend to do it. However I need to > modify ActivateDecice() or something like that. Are there any objections? > had a look at it on the last plane, looks good. One thing is left, which is more style than anything again: can you put accel stuff into a struct? so that in the end we get something like dev->valuator->accell.clean() dev->valuator->accell.init() instead of dev->valuator->AccelCleanupProc dev->valuator->AccelInitProc I think it makes the code more readable. aside from that, I like it. Created attachment 13692 [details] [review] patch agaist X server Created attachment 13693 [details] [review] Xinput proto - needed for Xserver Created attachment 13694 [details] [review] lib/libXi: proof-of concept runtime parameter adjustment Created attachment 13695 [details] [review] app/xinput: proof-of concept runtime parameter adjustment The structure is now OK, but changing parameters at runtime is not quite ready. It is however enough to try out and adjust the most important settings via the xinput tool. Comments encouraged! Created attachment 13697 [details]
User documentation
updated documentation
(In reply to comment #39) > Created an attachment (id=13692) [details] > patch agaist X server > This patch is a bit messy. CopySwapDeviceAccel() shouldn't even compile due to a stray "e->operation" SendErrorToClient is not used in current master anymore. just BadMatch directly. InitDefaultPointerAccelerationScheme(): you could just move the three lines to InitValuatorClassDeviceStruct and just have InitPointerAccelerationScheme. Or move them to the latter and have it as the default action if NULLs are provided. what also makes me wonder is the high amount of commented lines. if they're not needed, just remove them. can you fix these things please, then I can have another look at it. (In reply to comment #40) > Created an attachment (id=13693) [details] > Xinput proto - needed for Xserver > I'd be better if DEVICE_PTRACCEL only takes up one spot and then you have a sub-variable for the scheme in the struct. The range from DEVICE_PTRACCEL to DEVICE_PTRACCEL_MAX is a bit messy. Created attachment 13771 [details] [review] patch for Xserver I'm sorry! I accidentally uploaded an old development snapshot of the Xserver patch. I meant this one :) Sorry for wasting your time with this! > I'd be better if DEVICE_PTRACCEL only takes up one spot and then you have a sub-variable for the scheme in the struct. > The range from DEVICE_PTRACCEL to DEVICE_PTRACCEL_MAX is a bit messy. Agreed. It was like this in the first place. I changed the design in order to support getting parameters (GetDeviceControl doesn't upload a struct to the server) plus keeping the possibility to get/set (rather) arbitrary parameters. I thought it to be better than a loaded struct from which something will always be missing, especially if someone draws up additional schemes or the algorithm gets new tweaks. Maybe there is another way to do it? (In reply to comment #47) > > I'd be better if DEVICE_PTRACCEL only takes up one spot and then you have a > sub-variable for the scheme in the struct. > > > The range from DEVICE_PTRACCEL to DEVICE_PTRACCEL_MAX is a bit messy. > > Agreed. It was like this in the first place. I changed the design in order to > support getting parameters (GetDeviceControl doesn't upload a struct to the > server) plus keeping the possibility to get/set (rather) arbitrary parameters. > > I thought it to be better than a loaded struct from which something will always > be missing, especially if someone draws up additional schemes or the algorithm > gets new tweaks. Maybe there is another way to do it? there is a simple reason why the current approach is unsuitable. you're essentially making anybody else who may need to add a device ctrl dependent on your DEVICE_PTRACCEL_MAX. Now in the worst case s.b. adds a DevCtl as MAX + 1. Clients depend on it and then when you add an accel scheme suddenly the absolute value of MAX + 1 has changed. This is bad! Remember, #defines are compiled in so once they are in binary you can't change them anymore w/o recompilation. I'm also not sure why the current approach gets around the problem of an "incomplete" struct. Maybe I'm misunderstanding you here? What is wrong with typedef struct { XID control; /* DEVIC_PTRACCEL */ int length; int type; } XDeviceAccelerationControl, XDeviceAccelerationState; for the default, and then depending on the type you have a struct for each accel scheme that needs it. type determines which struct, and since length is in bytes anyway, nothing breaks by adding new schemes. I'll have a look at the server patch but the inputproto stuff in its current state is a dealbreaker. (In reply to comment #48) > typedef struct { > XID control; /* DEVIC_PTRACCEL */ > int length; > int type; > } XDeviceAccelerationControl, XDeviceAccelerationState; > > for the default, and then depending on the type you have a struct for each > accel scheme that needs it. type determines which struct, and since length is > in bytes anyway, nothing breaks by adding new schemes. Looks good, absolutely on option. Let's call it option (1). What it doesn't do is provide a simple way to add a parameter. You'll have to modify libXi, inputproto and modify marshalling (client and server). The current approach provides simple end-to-end transport - and by reserving a range incl. surplus we could be ABI safe too. (2) OTOH, I'm not sure if it is desirable to have this ability at all. > I'll have a look at the server patch but the inputproto stuff in its current > state is a dealbreaker. Would that still hold if the set of parameters was considerably smaller and with fixed codes assigned (and of course not resorting to range checks in switch() default: paths) ? This would however mean not every knob is tweakable, a thing I'm fine with. (3) Since libXi is yours, I'll leave the choice to you. My vote is for (2). (In reply to comment #49) > (In reply to comment #48) > > typedef struct { > > XID control; /* DEVIC_PTRACCEL */ > > int length; > > int type; > > } XDeviceAccelerationControl, XDeviceAccelerationState; > > > > for the default, and then depending on the type you have a struct for each > > accel scheme that needs it. type determines which struct, and since length is > > in bytes anyway, nothing breaks by adding new schemes. > Looks good, absolutely on option. Let's call it option (1). > What it doesn't do is provide a simple way to add a parameter. You'll have to > modify libXi, inputproto and modify marshalling (client and server). The > current approach provides simple end-to-end transport - and by reserving a > range incl. surplus we could be ABI safe too. (2) Ah. now I finally get it. I interpreted the parameters as different accel schemes rather than parameters for the same scheme. Nevertheless, my proposal still holds. The length field is 16 bit, which should be enough to stack device controls. Something along the line of typedef struct { XID control; /* DEVIC_PTRACCEL */ int length; int type; /* which accel scheme */ int parameters; /* number of parameters following */ } XDeviceAccelerationControl, XDeviceAccelerationState; parameters simply states how many of the following. length encloses the whole control (inc. all parameters) to be compatible with older versions. typedef struct { int param_type; int length; } XAccelParameterControl; and then you have multiple versions of typedef struct { int param_type; /* DevicePtrAccel_Coeff /* int length; int integer; int rational_num; int rational_den; } XDevicePtrAccelCoeff; this is a similar approach to what the ListInputDevices reply does with the classes. advantages: we don't mess up the CONTROL range, we have _a huge_ range of parameters. the type field allows for multiple different accel types parameters are re-usable. and 16 bit for the length should be enough for even multiple parameters. does that make sense? any comments? Created attachment 14254 [details] [review] patch: X server Created attachment 14255 [details] [review] patch: libXi Created attachment 14256 [details] [review] patch: xinput tool Created attachment 14257 [details] [review] patch: Xinput proto Created attachment 14258 [details]
Documentation
The XDeviceConrol interface has been reworked and should conform better now. The server-side implementation is considered complete. xinput has some rough edges but is useable.
FWIW, my final intention for this is 1.6, once I've finished the property code, since I'm loathe to add an API and then swiftly remove it. Currently working on XKB stuff, but should be back on to Xi by the end of the month. I don't have any real concerns with the patches, so any distros that want to merge this are welcome to. Created attachment 17544 [details] [review] patch against X server This patch should apply against current git. Small changelog: -A minimal interface for device drivers has been added (fully optional) -The velocity guessing is cleaner and potentially even more precise -the runtime parameter changing stuff has been removed -'classic' compatibility functions are now selectable as own profiles Created attachment 17545 [details]
documentation
the user-unfriendly documentation ;)
might miss a few bits, but correct in general
Created attachment 17626 [details] [review] revised patch didn't include important parts, this patch should be complete now. Pushed as c9eb0e870c87d291311491452adf7f91a911e24b Created attachment 17772 [details]
updated developer documentation
The attached doc should match with current git servers.
Contrary to what some people linking here stated, this effort was not commited until recently. This makes it a likely item in X11R7.5.
Until then, you have to use the patches here if you want it.
Created attachment 17773 [details] [review] patch for Xorg server 1.4.x This patch should apply against 1.4 servers. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.