-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improvements to hotplugging on Desktop. #31
Conversation
Changes JamepadControllerMonitor to track controllers by SDL instance ID instead of SDL index. This keeps the association of Controller java objects and physical devices constant. Cuts out the check for new controllers in JamepadControllerMonitor by instead updating the controller list when Jamepad reports that its controller list was updated. Adds the ability for JamepadController to report its name when disconnected by storing the name on connect. Allows JampadController to treat a null ControllerIndex as a disconnected controller. Fixes a bug in JamepadControllerManager that caused the controller update code to run twice as often as intended. Fixes a bug in ControllersTest that caused the first controller to be unresponsive when the program first starts. Fixes a bug in ControllersTest that caused duplicate disconnect messages in sone circumstances. Updates to the newest snapshot of Jamepad.
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 have tested on Linux with several controllers and it works as intended so we are good to go, but please see the comments.
...ers-desktop/src/main/java/com/badlogic/gdx/controllers/desktop/JamepadControllerManager.java
Show resolved
Hide resolved
...rs-desktop/src/main/java/com/badlogic/gdx/controllers/desktop/support/JamepadController.java
Show resolved
Hide resolved
...rs-desktop/src/main/java/com/badlogic/gdx/controllers/desktop/support/JamepadController.java
Outdated
Show resolved
Hide resolved
Alright, I can make those changes. While I'm at it, would you like me to update the workflows like I did for Jamepad? |
Sure, please do |
Updates Jamepad to stable version. Improves code formatting.
I added a commit to address your comments, but I didn't update the workflows because we have a small choice to make there. Currently in the workflows, we install Java 8, then android. The newer versions of However, it seems that Github's ubuntu image already has android installed, so we could simply skip the android install and it still seems to work. We may even be able to skip installing Java. Do you have any preference between continuing to use the |
Okay, let's do this with a new PR then. |
Has anyone else tested this on a mac? The latest snapshot seems to be pretty busted on my mac (key presses don't get recognized at all, as it seems). |
Can you check if this is caused by the latest jamepad snapshot by using it with 2.3.3? |
Yes, I run the test suit with jampad 2.26.4.0 and without the merged commit and I get the same result. |
Thanks for trying. Can you also try 2.24.0.0-SNAPSHOT and move the issue to Jamepad repo as it is caused by Jamepad, not this PR. Thanks! |
Fixes #29
I was only able to test these changes on Windows 7, so I recommend testing on other major operating systems. There is no change to non-Desktop platforms except a few changes to the test app.
Below I will annotate my commit message with information about each change. I'm open to suggestions if you disagree with any of them.
JamepadControllerMonitor
This is the most heavily changed file, with most of the important changes.
checkForNewControllers()
has been replaced byreconcileControllers()
. In addition to connecting new controllers, this method also removes disconnected controllers, and reassignsControllerIndex
objects betweenJamepadController
s to keep them paired with the same physical device.reconcileControllers()
does not check if the controller list need to be updated likecheckForNewControllers()
did. Instead, we check the new return value ofcontrollerManager.update()
, which tells us when Jamepad's list was updated. I believe this should always detect when changes are needed, but we find that it doesn't, we could write a new test.Other notes:
JamepadControllerMonitor
'supdate()
method still tries to disconnect controllers the old way, though in practice,reconcileControllers()
should handle it most of the time.update()
already implicitly used an iterator, so I've made that iterator explicit so we can use it'sremove()
method, avoiding the need for a temporary array here. The same structure is used inreconcileControllers()
.A new temporary array is used when connecting new controllers. Sending out a
connect()
event while the controller list is being reconstructed can cause issues, as it gives the user a chance to call methods on the other controllers, which may be in an incomplete state. Instead, we store the new controller and set up its listener at the end.reconcileControllers()
is now called in the constructor to set up the initial state. This introduces the restriction thatcontrollerManager.initSDLGamepad()
must be called before the constructor, but it already was.Tuple
'sControllerIndex
is no longer final. This is necessary to update the controller pairings.The
IntMap
of controllers now uses the JamepadConfiguration
'smaxNumControllers
as an initial capacity. This is a minor optimization. We iterate through the list much more often than we search it, so the smaller size will likely be better. It will now usually have a backing array size of 8 instead of 64.JamepadController
The committed implementation polls and stores the name in the constructor, continues to poll (but not store) the name while connected, then returns the stored name when disconnected. If we are confident that the name won't change, we could instead always report the stored name. If we are worried the name will change, we could poll and store the name each time while connected.
The
ControllerIndex
that the controller was using may be in use by another, still-connected controller, so being able to function without one is safest. For this reason, every method that caught aControllerUnpluggedException
now also catchesNullPointerException
and treats them the same way.Other notes:
setDisconnected()
is now public soreconcileControllers()
can proactively disconnect controllers. When used this way, itsControllerIndex
will be null, and it will skip logging that it failed to query it (since that didn't happen).The
controllerIndex
variable is no longer final, and there is a new method (setControllerIndex
) to assign it.JamepadControllerManager
The call to
Gdx.app.postRunnable(monitor)
was removed. The call tomonitor.run()
2 lines earlier ends by adding the monitor to the list of runnables. Adding it again caused it to be in the list twice, so it would run twice back-to-back each frame, which was unnecessary.ControllersTest
refreshControllersList()
was called fairly early increate()
, before some listeners were set up. Moving it to the end ofcreate()
doesn't seem to cause problems, and gives a more complete state at startup.The listener was added to each controller each time
refreshControllersList()
was called. This could result in the listener being added to the same controller many times, resulting in many disconnect events when disconnected. Attempting to remove the listener before adding it prevents this.build.gradle
Updates to Jamepad's API are used here.