On Apr 10, 2015, at 1:08 PM, Charles Davis cdavis5x@gmail.com wrote:
I do not agree with this particular direction. You do realize that some people on Mac OS still use the X11 driver (e.g. to get virtual desktop support), right? With these patches, these people suddenly won’t have any joystick support at all.
Don’t get me wrong, I still think putting all the joystick code in one place is the right thing to do. I just don’t think that one place should be the graphics driver.
I agree with Chip on this.
Let's look at the other side, the non-Mac side. Besides Mac joystick/gamepad support, there's also support for Linux. Is that to be moved into the X11 driver? As it currently stands, it doesn't use X11. Furthermore, the X11 driver is not the Linux driver in the same way that the Mac driver is, well, the _Mac_ driver. That is, the X11 driver is not for a specific platform, so it doesn't make much sense to move the Linux joystick support to it.
Besides that general concern, there are problems with the patches. For example, in 0013-dinput-Add-Graphic-driver-connection-for-dinput.txt, the values array is allocated with one entry per axis, POV, and button. But POVs store 2 values (x and y). So, if there are any POVs, you'll overrun the array.
In 0016-winemac.drv-Add-Joystick-driver-to-the-mac-driver.txt, pov_idx is not really used. It's initialized and incremented for each POV enumerated, but nothing ever makes use of the value.
Those happened to jumped out at me. I haven't done a thorough review.
I'm not sure why the force feedback interfaces were added in a separate patch.
I worry about the design of the driver interface for this joystick/gamepad support. In particular, it sets in stone the polling nature of the current implementation. DInput supports buffered data and "spontaneous" notification via event rather than polling. (Wine's dinput does event notification, but the app has to poll to get the events.) The HID API of OS X supports similar features. The current implementation doesn't take advantage of this similarity, but it arguably should. For example, currently, button press-and-release sequences that occur between polls are lost. If the dinput OS X joystick support were changed to support these capabilities, it has all the flexibility it needs because it controls the whole device vtable. The interface you've designed for the graphics driver joystick support enforces just one approach. It's not clear how the graphics driver would support spontaneous event notification, for example, because that would probably require a call back to dinput.
-Ken