New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for interfacing with joysticks via DirectInput when it is available. #1326
Conversation
Would it be possible to split the implementations (JoystickImplDInput / JoystickImplWinMM -- or at least 2 distinct sets of functions within JoystickImpl) for better readability and maintainability? What are those macros in _mingw_dxhelper.h used for? |
Oh, didn't notice those new header files from MinGW before. IMO neither of them should be included since they're part of the compiler toolset/platform SDK. |
@LaurentGomila @MarioLiebisch Those headers are meant for compatibility with some (older?) versions of MinGW that for some reason don't come with dinput.h out of the box. See https://ci.sfml-dev.org/builders/windows-gcc-492-tdm-32/builds/404 If you check, GLFW does the same for probably similar reasons. |
Really confusing. According to this bug report it seems like the headers should be there. |
They are there in the 64-bit distribution: https://ci.sfml-dev.org/builders/windows-gcc-492-tdm-64/builds/401 However, they are missing in the 32-bit distribution. I just double checked on the CI server. |
9388213
to
c8138cb
Compare
I split the DirectInput implementation into its own functions further down in JoystickImpl.cpp. |
|
||
IDirectInput8W* directInput = NULL; | ||
HMODULE dinput8dll = NULL; | ||
HRESULT(WINAPI *DirectInput8CreateFunc)(HINSTANCE, DWORD, REFIID, LPVOID*, LPUNKNOWN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DirectInput8CreateFunc
doesn't follow our naming conventions (starts with capital letter). There's no need for it to be a global, it is only used in initializeDInput
.
The DirectInput-related variables could be stuffed into a structure (like DirectInputContext
), to limit the number of globals and make things clearer.
objectDataFormat
is also used locally in a single place, but since its address is taken, it may have to live as long as DInput is used; do you know more about this stuff?
c8138cb
to
a1e2dc4
Compare
Fixed. |
That sounds like a bug or issue. Why would that be the case? |
You can have a look yourself. There was an issue: https://sourceforge.net/p/mingw-w64/discussion/723797/thread/0176cb85/ Don't ask me why the distributions took so long to react. Fact is that this is only an issue for 4.9.2, meaning it will most probably never be fixed for that version since newer versions don't have this problem any more. Either we decide to drop support for 4.9.2 32-bit just because of the missing header or we bundle it ourselves. |
Is there any guide for getting a mxe setup with a different branch like yours? |
Tested this. No crashing when unplugging. Controller works as expected, didn't notice any issues. Ran a performance profile for 60 secs, nothing out of the ordinary. |
Just wanted to try this, still having MinGW-W64 (I think?) GCC 5.3.0 installed. While it got the Direct3D headers and others, it's missing the Direct Input one for whatever reason. Updated to 6.3.0 and the header still isn't there. I'd just suggest dropping those headers, letting CMake look for them, and then print a notice about them missing. If they're missing, compile the legacy API version only? What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, yet another attempt with GCC 7.2.0 x64 finally worked – as does the code.
But as mentioned, I'm still not 100% sure about handling legacy compilers.
I'd suggest letting CMake look for dinput.h
and other necessary files. If they're found, use Direct Input exclusively (move the fallback to a separate file); if they aren't found, spit out a warning and use the legacy/Win16 API only. If Direct Input is broken, the old API shouldn't work either (on any modern Win32 platform).
What do you think?
* Copyright (C) the Wine project | ||
* | ||
* This library is free software; you can redistribute it and/or | ||
* modify it under the terms of the GNU Lesser General Public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume LGPL headers are fine to be shipped, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only working around a toolchain "bug". Either you use the one you already have or you use the one we provide. In both cases you are #include
ing an LGPL header.
initializeDInput(); | ||
|
||
if (!directInput) | ||
err() << "DirectInput not available, falling back to Windows joystick API" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone doesn't want to use a joystick and doesn't have direct input available will this still output the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People have already "complained" a few times about not being able to disable joystick polling, so yes... this error will still be output as soon as window events are pumped. Realistically, these can only be Windows 98 or older systems, so if they get the rest of SFML running on those kinds of systems they deserve a medal.
I think, shipping our own header is fine. Will make our life easier as we won't have to deal with broke compilers. |
The workaround is trivial anyway, so I don't see any problems with it. AFAIK GLFW decided to go down the same route. |
a1e2dc4
to
22f1b85
Compare
What are the chances of getting this back ported to 2.4.2? |
Zero, as SFML 2.5 is around the corner. |
Pretty much zero. edit: aaah... eXpl0it3r was faster again 😄 |
Because I have no idea how to get the 2.5 sfml into a mxe environment so I can compile attract mode. see here under cross compile. |
I've never used mxe before (nor have I heard of it), but since it's obviously to cross-compile Win32 binaries, you'll just have to compile those yourself and replace those mxe uses. You'll probably have more luck in their community/communities though. |
The same way you'd get a new "backported" build in there. If you can do that, you can also do it with a new SFML version. Besides, backporting for minor versions is not a thing we do, especially not for features. |
Fixes #1251.
Test with this (might be worth making into an official example):