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

Read and store joypad events in a separate thread on x11 platform #56125

Merged
merged 1 commit into from
May 5, 2022

Conversation

madmiraal
Copy link
Contributor

Currently, joypad events are only read once every iteration. As demonstrated in #55850, this can result in joypad events being missed. The OS will drop old events in its queue if they are not read and its queue gets full.

This PR creates a separate thread to read the OS' joypad events and store them in a Godot queue until they are processed. This PR only fixes the Linux platform. The other platforms probably need to be updated in the same way.

Note: A lot of the changes are around updating the mutexes; as there are now three threads trying to access the joypads array:

  1. Instead of putting a mutex on methods that may access this array, the mutex is now used to protect access to the Joypad elements at the time they're accessed.
  2. To minimise the time a mutex is locked (when looping through the array) which may delay other threads, each Joypad element in the array now has its own mutex.
  3. To minimise the use of mutexes and the risk of deadlocks, access to the arrays is now only done when opening or closing a connection to a joypad or reading and writing to the Joypad event queues.
  4. Instead of being allowed to access the arrays themselves, other methods are passed a reference to the mutex protected Joypad element.

Fixes #55850.

}
if (no_events) {
usleep(10000); // 10ms
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: It would probably be better to use a poll() to detect changes. However:

  1. This is how the other thread is being managed; so I used this approach for consistency.
  2. poll() should be used on both threads, but this would the require refactoring the Joypad class (separating the OS file descriptors from the Godot data to avoid mutex sharing) and is probably best done in a separate PR as an enhancement instead of a bug fix.
  3. The other thread (as well as the XEvent polling) uses a select() call (which is currently not used to detect changes, but only to check whether a change has happened to prevent the next call from blocking). These select() calls should be replaced with poll() calls too. Again, this is best done in a separate PR as an enhancement.

Copy link
Member

@Calinou Calinou Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The USB polling rate is 125 Hz by default (I believe most gamepads stick to this value). Shouldn't we use 8000 as a sleep duration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense if there was a single joypad and the purpose of the delay was to wait until there was new data. In reality the delay would be less than 8000 μs, because there is the time needed to read the data. The required delay would also be indeterminate if there were multiple joypads, because they wouldn't be synchronised.

Note: The purpose of this delay is to not be busy waiting; especially when there are no joypads attached.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread/Mutex changes seems fine to me.
Not sure about usleep(10000); // 10ms, but probably it's also OK.

@akien-mga akien-mga self-requested a review January 18, 2022 15:58
@akien-mga
Copy link
Member

Sorry for the delay reviewing. Should be fine to merge after a rebase.

@akien-mga akien-mga merged commit 11b9435 into godotengine:master May 5, 2022
@akien-mga
Copy link
Member

Thanks!

@madmiraal madmiraal deleted the fix-55850 branch May 5, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input released event missed entirely if it happens while the main thread is very busy (DS4 only?)
4 participants