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
Removed time-based poll for joysticks and replaced with manual update #1195
Conversation
As @MarioLiebisch pointed out on IRC, this would stop joysticks working if there is no window opened, so I've implemented his suggested changes. The downside is that if you're using joysticks without a window then this won't change anything, but alternatives would probably change the API |
ConnectionCache connectionCache[sf::Joystick::Count]; | ||
|
||
//if true, will only update when WM_DEVICECHANGE message is received |
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.
Nitpicking: Should be // If true, ...
@@ -936,6 +950,13 @@ void WindowImplWin32::processEvent(UINT message, WPARAM wParam, LPARAM lParam) | |||
pushEvent(event); | |||
break; | |||
} | |||
case WM_DEVICECHANGE: | |||
{ | |||
//some sort of device change has happened, update joystick connections |
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.
Same nitpicking here: // Some ...
|
||
namespace | ||
{ | ||
unsigned int windowCount = 0; | ||
unsigned int handleCount = 0; |
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.
What's the difference between windowCount
and handleCount
?
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.
windowCount only counts windows owned by SFML
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 recommend adding a comment to avoid any future ambiguity.
@@ -141,6 +146,9 @@ m_cursorGrabbed (false) | |||
|
|||
if (m_handle) | |||
{ | |||
if (handleCount++ == 0) |
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 personally am not a big fan of this notation, but maybe others don't have an issue with it.
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 strongly think branching conditions should be side effect free; so yeah, I've an issue with this. 😛
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.
Me too. One extra line of code vs more headache when reading the code, my choice is made.
I've implemented the changes and squashed |
Only alternative for all cases would be creating an invisible window just for joystick polling (which would also remove the need for public static members in the implementation). But then again that would create a window after all, which might not be wanted in some odd cases. |
Merged in f053871 on master. |
Added updateConnections() to JoystickImpl which is called when WM_DEVICECHANGE message is received, this replaces the old functionality of just polling every controller every 0.5s, in the hope of addressing #1179