Skip to content
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

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Conversation

Yontipon
Copy link
Contributor

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.

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.

checkForNewControllers() has been replaced by reconcileControllers(). In addition to connecting new controllers, this method also removes disconnected controllers, and reassigns ControllerIndex objects between JamepadControllers to keep them paired with the same physical device.

Cuts out the check for new controllers in JamepadControllerMonitor by instead updating the controller list when Jamepad reports that its controller list was updated.

reconcileControllers() does not check if the controller list need to be updated like checkForNewControllers() did. Instead, we check the new return value of controllerManager.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's update() 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's remove() method, avoiding the need for a temporary array here. The same structure is used in reconcileControllers().

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 that controllerManager.initSDLGamepad() must be called before the constructor, but it already was.

Tuple's ControllerIndex is no longer final. This is necessary to update the controller pairings.

The IntMap of controllers now uses the Jamepad Configuration's maxNumControllers 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

Adds the ability for JamepadController to report its name when disconnected by storing the name on connect.

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.

Allows JampadController to treat a null ControllerIndex as a disconnected controller.

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 a ControllerUnpluggedException now also catches NullPointerException and treats them the same way.

Other notes:

setDisconnected() is now public so reconcileControllers() can proactively disconnect controllers. When used this way, its ControllerIndex 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

Fixes a bug in JamepadControllerManager that caused the controller update code to run twice as often as intended.

The call to Gdx.app.postRunnable(monitor) was removed. The call to monitor.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

Fixes a bug in ControllersTest that caused the first controller to be unresponsive when the program first starts.

refreshControllersList() was called fairly early in create(), before some listeners were set up. Moving it to the end of create() doesn't seem to cause problems, and gives a more complete state at startup.

Fixes a bug in ControllersTest that caused duplicate disconnect messages in some circumstances.

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 the newest snapshot of Jamepad.

Updates to Jamepad's API are used here.

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.
Copy link
Member

@MrStahlfelge MrStahlfelge left a 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.

@Yontipon
Copy link
Contributor Author

Alright, I can make those changes. While I'm at it, would you like me to update the workflows like I did for Jamepad?

@MrStahlfelge
Copy link
Member

Sure, please do

Updates Jamepad to stable version.

Improves code formatting.
@Yontipon
Copy link
Contributor Author

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 setup-android seem to require Java 11, which we can easily install instead.

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 setup-android action or not?

@MrStahlfelge
Copy link
Member

Okay, let's do this with a new PR then.
We are good to switch to Java 11 at this point of time. If it works without any extra setups, we can skip this, too. Less code less bugs.

@MrStahlfelge MrStahlfelge merged commit 6c892a3 into libgdx:master Apr 27, 2023
1 check passed
@Berstanio
Copy link

Berstanio commented Apr 27, 2023

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).

@MrStahlfelge
Copy link
Member

Can you check if this is caused by the latest jamepad snapshot by using it with 2.3.3?

@Berstanio
Copy link

Yes, I run the test suit with jampad 2.26.4.0 and without the merged commit and I get the same result.
So I guess it really is a jampad 2.26.4.0 issue

@MrStahlfelge
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with hotplugging on Desktop
3 participants