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.
Chip
Hi Chip,
Yes, I extensively discussed this very issue with Alexandre and this was his recommendation on what he was most likely to accept.
Maybe we can work on a plan with him where the code can live in the relative graphic drivers but there is another unique registry key for the joystick driver? Though when I did that for the IME work it was quickly removed and Alexandre did not show much warmth to it when I proposed it.
I believe his thinking is that if a Mac user needs to use X11 for a game then that is the bug that needs to be fixed.
-aric
On 4/10/15 1:08 PM, Charles Davis 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.
Chip
On 4/10/15 1:28 PM, Aric Stewart wrote:
I believe his thinking is that if a Mac user needs to use X11 for a game then that is the bug that needs to be fixed.
I don't know the overall philosophy on the X11 driver outside of Linux, but it seems useful to me to keep X11 working on all platforms where it exists. It seems like X11 has useful features that wouldn't likely end up in wine's other platform-specific user-drivers (It's useful for remote display, and I thought that, e.g., x11vnc could do remote display of even opengl windows.)
On 4/10/15 1:44 PM, Josh DuBois wrote:
On 4/10/15 1:28 PM, Aric Stewart wrote:
I believe his thinking is that if a Mac user needs to use X11 for a game then that is the bug that needs to be fixed.
I don't know the overall philosophy on the X11 driver outside of Linux, but it seems useful to me to keep X11 working on all platforms where it exists. It seems like X11 has useful features that wouldn't likely end up in wine's other platform-specific user-drivers (It's useful for remote display, and I thought that, e.g., x11vnc could do remote display of even opengl windows.)
This does not prevent X11 from working, just OS X gamepad support when running with X11. Which would likely be completely irrelevant for remote displays and the like anyway.
-aric
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2015-04-10 um 21:04 schrieb Aric Stewart:
This does not prevent X11 from working, just OS X gamepad support when running with X11. Which would likely be completely irrelevant for remote displays and the like anyway.
Virtual Desktops don't work with the Mac driver, and this is a feature that is quite useful for some games. Users may want to run the game in a window even if fullscreen support worked perfectly.
Do we still need winejoystick.drv after these patches?
On 4/10/15 4:14 PM, Stefan Dösinger wrote:
Am 2015-04-10 um 21:04 schrieb Aric Stewart:
This does not prevent X11 from working, just OS X gamepad support when running with X11. Which would likely be completely irrelevant for remote displays and the like anyway.
Virtual Desktops don't work with the Mac driver, and this is a feature that is quite useful for some games. Users may want to run the game in a window even if fullscreen support worked perfectly.
True. The question is if it ends up being worth the tradeoff. Could Virtual Desktops get implemented in the Mac driver?
Do we still need winejoystick.drv after these patches?
Potentially, if these patches get agreed upon and in and the linux side is all transitioned also, winejoystick.drv will be able to go away and be rolled back into winmm.
-aric
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
Hi Ken,
Thanks for the comments.
On 4/12/15 9:27 PM, Ken Thomases wrote:
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.
I feel like the joystick code should be collected somewhere. Having platform specific code scattered about the code base is a pain in the butt. So maybe I push back on Alexandre and see if we should define this as a joystick driver. The Mac driver can happen to be the default joystick driver for the mac?
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.
It is a lot of code that I have worked through, I tried to debug and test extensively but I expected there would be some flaws. Thank you for finding these.
I'm not sure why the force feedback interfaces were added in a separate patch.
This was because of the size that the first patch was growing to. It seemed like a logical place to divide the work to not make it one monolithic 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 di
nput.
I designed this driver interface to match the current code. Which was all poll based as you pointed out. All your flaws with the poll base code exist currently as well. If we want to move towards a push base possibility. I do not think that is hard. It is just a matter of adding some interfaces into the joystick driver that would support a push endpoint and then anything that wanted push data would have to implement defined callbacks that receive the data from the driver when it is available.
I do not see that as an impossible or even difficult extension to the current work I did for the poll based reporting. I would not see the current approach as enforcing this poll based reporting as the only possible way for the future.
-aric
-
Aric Stewart -
Charles Davis -
Josh DuBois -
Ken Thomases -
Stefan Dösinger