feat: Multi controller support#1578
Open
phobos665 wants to merge 18 commits into
Open
Conversation
… than default to CurrentController.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extends the controller framework to support up to 4 concurrent gamepads instead of 1. Changes span from Java constants and ControllerManager device-to-slot mapping through WinHandler per-slot state management, input routing in XServerScreen, rumble lifecycle handling, shared-memory writing, and native-side player initialization and virtual joystick attachment/detachment, ensuring slot-aware behavior across the entire stack. Localization strings for a new controller debug menu are added across 15 languages. ChangesMulti-Controller Player Support (1–4 Players)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
phobos665
commented
Jun 12, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/cpp/evshim/evshim.c (1)
343-347:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp
EVSHIM_MAX_PLAYERSto a valid lower bound and parse it safely.At Line 345,
atoican turn malformed input into0, and the current logic never clampsplayersback to>= 1. That can skipsetup_shmentirely, causing downstreamnotifyStateChanged(slot)calls to fail and drop controller updates.Suggested fix
- int players = g_is_wine ? 1 : MAX_GAMEPADS; - const char *ep = getenv("EVSHIM_MAX_PLAYERS"); - if (ep) players = atoi(ep); - if (players > MAX_GAMEPADS) players = MAX_GAMEPADS; + int players = g_is_wine ? 1 : MAX_GAMEPADS; + const char *ep = getenv("EVSHIM_MAX_PLAYERS"); + if (ep && *ep) { + char *end = NULL; + long parsed = strtol(ep, &end, 10); + if (end != ep && *end == '\0') { + players = (int)parsed; + } + } + if (players < 1) players = 1; + if (players > MAX_GAMEPADS) players = MAX_GAMEPADS;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/cpp/evshim/evshim.c` around lines 343 - 347, The code reads EVSHIM_MAX_PLAYERS into players using atoi which can return 0 for malformed input and currently allows players to be <1; change the parsing to use a safe parser (e.g. strtol) and clamp the result to the valid range [1, MAX_GAMEPADS] so players is never less than 1 (respect g_is_wine which sets default to 1), then use that clamped players value for setup_shm and subsequent notifyStateChanged(slot) calls; update references in this block (getenv("EVSHIM_MAX_PLAYERS"), players, MAX_GAMEPADS, g_is_wine, setup_shm, notifyStateChanged) accordingly.
🧹 Nitpick comments (2)
app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java (1)
189-192: ⚡ Quick winUse
WinHandler.MAX_PLAYERSinstead of a local duplicate.This local constant duplicates
WinHandler.MAX_PLAYERS(also set to 4). If the constant inWinHandlerchanges, this file won't automatically stay in sync, risking a mismatch between the memory files created here and the buffers/slots managed byWinHandler.♻️ Suggested fix
Add the import:
+import com.winlator.winhandler.WinHandler;Then replace the local constant with the reference:
- final int MAX_PLAYERS = 4; + final int MAX_PLAYERS = WinHandler.MAX_PLAYERS;Or inline the reference at each usage site.
Also applies to: 234-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java` around lines 189 - 192, Replace the local MAX_PLAYERS constant in BionicProgramLauncherComponent with the canonical WinHandler.MAX_PLAYERS: remove the local declaration and use WinHandler.MAX_PLAYERS (or import WinHandler and reference MAX_PLAYERS directly) wherever MAX_PLAYERS is used (e.g., the for-loop that queries ControllerManager and the other usage around line ~234), ensuring you no longer have a duplicate hard-coded value.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
643-693: 💤 Low valueConsider making
runningvolatile for reliable cross-thread visibility.The rumble poller threads check
running(line 651) in a tight loop. Sincerunning(declared at line 72) is not volatile, the JIT may cache the value in a register and the threads might not see thefalsewritten bystop(). TherumbleTeardown()call mitigates this by unblockingwaitForRumble(), but addingvolatilewould guarantee correct visibility.Also, silently swallowing all exceptions (line 687) could mask unexpected errors during development. Consider logging at debug level.
♻️ Suggested improvements
At line 72, make the field volatile:
- private boolean running; + private volatile boolean running;At line 687, log the exception:
- } catch (Exception ignored) { + } catch (Exception e) { + Timber.d(e, "rumble-poller-%d exception", sl); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 643 - 693, Make the shared boolean "running" used by the rumble poller threads a volatile field (the declaration of running) so changes from stop() are reliably visible to threads running the loop in the rumble poller lambda (which calls waitForRumble(...) and is unblocked by rumbleTeardown()). Also replace the empty catch in the thread body (the exception swallowed in the try/catch inside the rumble-poller lambda) with a debug-level log of the exception so unexpected errors in startVibration/stopVibration/getGamepadBuffer/waitForRumble are surfaced during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1348-1360: The code is currently calling
winHandler.setCurrentController(it.event.device.id) before checking the slot
assignment, which can overwrite WinHandler.currentController for controllers
that already have a valid slot; only set the singleton currentController when no
slot is assigned (playerSlot < 0) and you are about to fall back to legacy
handling. Concretely: remove or move the
winHandler.setCurrentController(it.event.device.id) that runs unconditionally in
XServerScreen around the onKeyEvent flow, and instead call setCurrentController
just inside the "else" branch where playerSlot < 0 (i.e., before calling
winHandler.onKeyEvent when physicalControllerHandler and
PluviaApp.inputControlsView did not handle the event); apply the same change to
the onGenericMotionEvent handling (the same unconditional setCurrentController
there) so slot-aware paths (ControllerManager.getInstance().getSlotForDevice,
winHandler.onKeyEvent, winHandler.onGenericMotionEvent) run without mutating
currentController.
---
Outside diff comments:
In `@app/src/main/cpp/evshim/evshim.c`:
- Around line 343-347: The code reads EVSHIM_MAX_PLAYERS into players using atoi
which can return 0 for malformed input and currently allows players to be <1;
change the parsing to use a safe parser (e.g. strtol) and clamp the result to
the valid range [1, MAX_GAMEPADS] so players is never less than 1 (respect
g_is_wine which sets default to 1), then use that clamped players value for
setup_shm and subsequent notifyStateChanged(slot) calls; update references in
this block (getenv("EVSHIM_MAX_PLAYERS"), players, MAX_GAMEPADS, g_is_wine,
setup_shm, notifyStateChanged) accordingly.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 643-693: Make the shared boolean "running" used by the rumble
poller threads a volatile field (the declaration of running) so changes from
stop() are reliably visible to threads running the loop in the rumble poller
lambda (which calls waitForRumble(...) and is unblocked by rumbleTeardown()).
Also replace the empty catch in the thread body (the exception swallowed in the
try/catch inside the rumble-poller lambda) with a debug-level log of the
exception so unexpected errors in
startVibration/stopVibration/getGamepadBuffer/waitForRumble are surfaced during
development.
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 189-192: Replace the local MAX_PLAYERS constant in
BionicProgramLauncherComponent with the canonical WinHandler.MAX_PLAYERS: remove
the local declaration and use WinHandler.MAX_PLAYERS (or import WinHandler and
reference MAX_PLAYERS directly) wherever MAX_PLAYERS is used (e.g., the for-loop
that queries ControllerManager and the other usage around line ~234), ensuring
you no longer have a duplicate hard-coded value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9f22fbc-09b9-41a4-8d8f-30dd54d6107f
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (4)
app/src/main/cpp/evshim/evshim.capp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:391">
P2: Newly added extra gamepad RandomAccessFile mappings are not closed on stop, creating a multi-handle resource leak risk.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:867">
P2: `sendGamepadState()` is still player-1-only but is called from multi-controller event paths</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
hotplugging needs to be added, right now it's going to break some games just like the previous multi controller implementation because it has 4 ghost controllers all the time |
Contributor
Author
Yeah good call, I'll add that in today. Will need to pick your brains as the thread says that some people's arcade sticks aren't separating into different controllers correctly |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/inputcontrols/ControllerManager.java (1)
194-196: 💤 Low valueConsider returning an unmodifiable view.
Returning the internal
detectedDeviceslist directly allows callers to modify it, potentially corrupting internal state.♻️ Suggested fix
public List<InputDevice> getDetectedDevices() { - return detectedDevices; + return java.util.Collections.unmodifiableList(detectedDevices); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java` around lines 194 - 196, The getDetectedDevices() method returns the internal detectedDevices list directly, allowing callers to modify it and corrupt internal state. Wrap the return statement with Collections.unmodifiableList() to return an unmodifiable view of the list instead, preventing external callers from adding, removing, or modifying elements while still allowing read access.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 365-376: In the onDeviceDisconnected method, after removing the
slot assignment with slotAssignments.remove(slot), add a statement to set
enabledSlots[slot] = false to properly disable the slot and prevent it from
appearing as a ghost controller to games. This mirrors the corresponding logic
in onDeviceConnected which correctly sets enabledSlots[slot] = true when a
device connects.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/inputcontrols/ControllerManager.java`:
- Around line 194-196: The getDetectedDevices() method returns the internal
detectedDevices list directly, allowing callers to modify it and corrupt
internal state. Wrap the return statement with Collections.unmodifiableList() to
return an unmodifiable view of the list instead, preventing external callers
from adding, removing, or modifying elements while still allowing read access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b0c05ac-049b-467d-931f-1b76cdb983ba
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.javaapp/src/main/java/com/winlator/winhandler/WinHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/winhandler/WinHandler.java
Contributor
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:353">
P2: Hotplug auto-assignment unconditionally enables the chosen slot, overriding any previous user preference to disable it.</violation>
<violation number="2" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:370">
P1: Missing `enabledSlots[slot] = false` in `onDeviceDisconnected` — the slot assignment is removed but the enabled flag remains `true`. This causes `isSlotEnabled()` and `getEnabledPlayerCount()` to still report the slot as occupied, leaving a ghost controller visible to games. Compare with `onDeviceConnected` which correctly sets `enabledSlots[slot] = true`.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:863">
P1: Controller mapping refresh runs on every generic device scan, causing unnecessary native overhead and rumble interruptions</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
@phobos665 mhm, let me know on any questions |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/cpp/evshim/evshim.c (1)
282-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t mark the slot connected when attach fails.
attach_vjoy(idx)can return-1, butset_vjoy_connecteddrops that result andvjoy_updaterstill setslast_connected = connected. After one attach/open failure,connectedremains true,vjoy_handles[idx]stays NULL, and this slot will skip every future state update without retrying.Suggested retry-safe flow
-static void set_vjoy_connected(int idx, int connected) +static int set_vjoy_connected(int idx, int connected) { if (connected) { if (!vjoy_handles[idx]) { - attach_vjoy(idx); + return attach_vjoy(idx) == 0; } + return 1; } else { detach_vjoy(idx); + return vjoy_handles[idx] == NULL && vjoy_ids[idx] < 0; } } ... - set_vjoy_connected(idx, connected); - last_connected = connected; + if (set_vjoy_connected(idx, connected)) { + last_connected = connected; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/cpp/evshim/evshim.c` around lines 282 - 323, The set_vjoy_connected function does not check the return value of attach_vjoy, so when attach_vjoy fails (returns -1), vjoy_handles[idx] remains NULL but the connection state is still marked as successful. This causes vjoy_updater to set last_connected to the desired state even though attach failed, resulting in the slot being skipped in future iterations without retry. Modify set_vjoy_connected to return an int status (success/failure), have it return failure if attach_vjoy returns -1, and in vjoy_updater only update last_connected = connected after set_vjoy_connected successfully returns, so failed attach operations don't prevent future retry attempts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/cpp/evshim/evshim.c`:
- Around line 274-278: The vjoy_ids array stores volatile SDL device indexes
that shift when any joystick detaches, causing vjoy_ids[idx] to reference
incorrect devices after the first player detaches. Additionally, the return
value from p_SDL_JoystickDetachVirtual at line 275 is ignored and vjoy_ids[idx]
is unconditionally cleared at line 277 even if detach fails, potentially leaving
orphaned virtual joysticks. Fix this by querying the current device index for
the virtual joystick before calling p_SDL_JoystickDetachVirtual, checking the
return value to confirm successful detach, and only clearing vjoy_ids[idx] after
a successful detach operation. Alternatively, if the SDL library version
supports it, track joysticks by instance ID (SDL_JoystickID) instead of volatile
device indexes to avoid index shifting problems entirely.
---
Outside diff comments:
In `@app/src/main/cpp/evshim/evshim.c`:
- Around line 282-323: The set_vjoy_connected function does not check the return
value of attach_vjoy, so when attach_vjoy fails (returns -1), vjoy_handles[idx]
remains NULL but the connection state is still marked as successful. This causes
vjoy_updater to set last_connected to the desired state even though attach
failed, resulting in the slot being skipped in future iterations without retry.
Modify set_vjoy_connected to return an int status (success/failure), have it
return failure if attach_vjoy returns -1, and in vjoy_updater only update
last_connected = connected after set_vjoy_connected successfully returns, so
failed attach operations don't prevent future retry attempts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bf23ca9-1b4b-43d5-96d5-2f044c8d55ca
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (2)
app/src/main/cpp/evshim/evshim.capp/src/main/java/com/winlator/winhandler/WinHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/winhandler/WinHandler.java
Contributor
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:353">
P2: Hotplug auto-assignment unconditionally enables the chosen slot, overriding any previous user preference to disable it.</violation>
<violation number="2" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:370">
P1: Missing `enabledSlots[slot] = false` in `onDeviceDisconnected` — the slot assignment is removed but the enabled flag remains `true`. This causes `isSlotEnabled()` and `getEnabledPlayerCount()` to still report the slot as occupied, leaving a ghost controller visible to games. Compare with `onDeviceConnected` which correctly sets `enabledSlots[slot] = true`.</violation>
</file>
<file name="app/src/main/cpp/evshim/evshim.c">
<violation number="1" location="app/src/main/cpp/evshim/evshim.c:256">
P1: Using the stored device index (`vjoy_ids[idx]`) for `SDL_JoystickDetachVirtual` is unreliable because SDL2 device indexes are volatile—they shift down when any lower-indexed joystick is removed. If player 0 detaches first, `vjoy_ids[1]` no longer refers to the correct device. Additionally, the return value of `SDL_JoystickDetachVirtual` is ignored, meaning `vjoy_ids[idx]` is cleared unconditionally even if detach fails (leaving an orphaned virtual joystick).
Consider resolving the current device index from the joystick's instance ID before detaching, or tracking joysticks by instance ID (`SDL_JoystickInstanceID`) rather than volatile device indexes. Also check the return value before clearing local state.</violation>
<violation number="2" location="app/src/main/cpp/evshim/evshim.c:323">
P1: Attach failure can permanently strand a controller slot because `last_connected` is updated even when `attach_vjoy` fails, preventing retries.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:248">
P1: Startup/reconnect can retain stale gamepad state because neutralization is conditional on the persisted `OFF_CONNECTED` value. The mmap files are not explicitly zeroed on creation, so a prior run may leave `OFF_CONNECTED = 1` and stale button/axis bytes. The connect path only calls `writeNeutralGamepadState` when `connected && buffer.getInt(OFF_CONNECTED) == 0`, so if the persisted flag is already 1, stale input state persists until the next real event. Neutralize state unconditionally when a slot becomes connected (or track in-memory transitions) rather than relying on the persisted flag.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:391">
P2: Newly added extra gamepad RandomAccessFile mappings are not closed on stop, creating a multi-handle resource leak risk.</violation>
<violation number="3" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:867">
P2: `sendGamepadState()` is still player-1-only but is called from multi-controller event paths</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Any chance to push this feature forward please? It's really the only thing stopping me from moving from gamehub |
phobos665
changed the title
phobos665
commented
Jun 21, 2026
…ted. Added a debug menu for controllers.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
3176-3181: ⚡ Quick winRemove side effect from snapshot builder function.
autoAssignConnectedDevices()mutatesControllerManagerstate but is called insidebuildControllerStatusSnapshot(), which is invoked from arememberblock. This creates a code smell:
- Redundant – already invoked in
scanForExternalDevices()(line 891) and through hotplug callbacks- Violates
remembercontract – the lambda should be a pure computation; side effects during recomposition can cause subtle timing issuesSince auto-assignment is already triggered at the appropriate points in the hotplug lifecycle, this call can be safely removed.
Suggested fix
private fun buildControllerStatusSnapshot( controllerManager: ControllerManager, container: Container, areControlsVisible: Boolean, ): ControllerStatusSnapshot { - controllerManager.autoAssignConnectedDevices() val guestApi = describePreferredInputApi(container) val slots = (0 until WinHandler.MAX_PLAYERS).map { slot ->🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 3176 - 3181, Remove the call to controllerManager.autoAssignConnectedDevices() from inside the buildControllerStatusSnapshot() function, as this function is invoked from a remember block which should contain only pure computations without side effects. The auto-assignment functionality is already being triggered through scanForExternalDevices() and hotplug callbacks, making this call redundant and causing timing issues during recomposition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 3176-3181: Remove the call to
controllerManager.autoAssignConnectedDevices() from inside the
buildControllerStatusSnapshot() function, as this function is invoked from a
remember block which should contain only pure computations without side effects.
The auto-assignment functionality is already being triggered through
scanForExternalDevices() and hotplug callbacks, making this call redundant and
causing timing issues during recomposition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 587314b0-bcce-4c4a-b285-1912bd57f17c
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/inputcontrols/ControllerManager.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/inputcontrols/ControllerManager.java
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:353">
P2: Hotplug auto-assignment unconditionally enables the chosen slot, overriding any previous user preference to disable it.</violation>
<violation number="2" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:370">
P1: Missing `enabledSlots[slot] = false` in `onDeviceDisconnected` — the slot assignment is removed but the enabled flag remains `true`. This causes `isSlotEnabled()` and `getEnabledPlayerCount()` to still report the slot as occupied, leaving a ghost controller visible to games. Compare with `onDeviceConnected` which correctly sets `enabledSlots[slot] = true`.</violation>
</file>
<file name="app/src/main/cpp/evshim/evshim.c">
<violation number="1" location="app/src/main/cpp/evshim/evshim.c:256">
P1: Using the stored device index (`vjoy_ids[idx]`) for `SDL_JoystickDetachVirtual` is unreliable because SDL2 device indexes are volatile—they shift down when any lower-indexed joystick is removed. If player 0 detaches first, `vjoy_ids[1]` no longer refers to the correct device. Additionally, the return value of `SDL_JoystickDetachVirtual` is ignored, meaning `vjoy_ids[idx]` is cleared unconditionally even if detach fails (leaving an orphaned virtual joystick).
Consider resolving the current device index from the joystick's instance ID before detaching, or tracking joysticks by instance ID (`SDL_JoystickInstanceID`) rather than volatile device indexes. Also check the return value before clearing local state.</violation>
<violation number="2" location="app/src/main/cpp/evshim/evshim.c:323">
P1: Attach failure can permanently strand a controller slot because `last_connected` is updated even when `attach_vjoy` fails, preventing retries.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:248">
P1: Startup/reconnect can retain stale gamepad state because neutralization is conditional on the persisted `OFF_CONNECTED` value. The mmap files are not explicitly zeroed on creation, so a prior run may leave `OFF_CONNECTED = 1` and stale button/axis bytes. The connect path only calls `writeNeutralGamepadState` when `connected && buffer.getInt(OFF_CONNECTED) == 0`, so if the persisted flag is already 1, stale input state persists until the next real event. Neutralize state unconditionally when a slot becomes connected (or track in-memory transitions) rather than relying on the persisted flag.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:391">
P2: Newly added extra gamepad RandomAccessFile mappings are not closed on stop, creating a multi-handle resource leak risk.</violation>
<violation number="3" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:867">
P2: `sendGamepadState()` is still player-1-only but is called from multi-controller event paths</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
Author
It's under review with the other devs and should have something soonish. |
… into feat/multi-controller-support-without-ui
… into feat/multi-controller-support-without-ui
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:353">
P2: Hotplug auto-assignment unconditionally enables the chosen slot, overriding any previous user preference to disable it.</violation>
<violation number="2" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:370">
P1: Missing `enabledSlots[slot] = false` in `onDeviceDisconnected` — the slot assignment is removed but the enabled flag remains `true`. This causes `isSlotEnabled()` and `getEnabledPlayerCount()` to still report the slot as occupied, leaving a ghost controller visible to games. Compare with `onDeviceConnected` which correctly sets `enabledSlots[slot] = true`.</violation>
</file>
<file name="app/src/main/cpp/evshim/evshim.c">
<violation number="1" location="app/src/main/cpp/evshim/evshim.c:323">
P1: Attach failure can permanently strand a controller slot because `last_connected` is updated even when `attach_vjoy` fails, preventing retries.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:248">
P1: Startup/reconnect can retain stale gamepad state because neutralization is conditional on the persisted `OFF_CONNECTED` value. The mmap files are not explicitly zeroed on creation, so a prior run may leave `OFF_CONNECTED = 1` and stale button/axis bytes. The connect path only calls `writeNeutralGamepadState` when `connected && buffer.getInt(OFF_CONNECTED) == 0`, so if the persisted flag is already 1, stale input state persists until the next real event. Neutralize state unconditionally when a slot becomes connected (or track in-memory transitions) rather than relying on the persisted flag.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:391">
P2: Newly added extra gamepad RandomAccessFile mappings are not closed on stop, creating a multi-handle resource leak risk.</violation>
<violation number="3" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:867">
P2: `sendGamepadState()` is still player-1-only but is called from multi-controller event paths</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:2955">
P1: After hiding virtual on-screen controls, player 1 may remain unassigned because `hideInputControls()` refreshes controller mappings without restoring/reassigning a connected physical controller to slot 0.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… into feat/multi-controller-support-without-ui
…rect slots, as well as only activate slot when connected to avoid issues with single-controller games.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java">
<violation number="1" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:353">
P2: Hotplug auto-assignment unconditionally enables the chosen slot, overriding any previous user preference to disable it.</violation>
<violation number="2" location="app/src/main/java/com/winlator/inputcontrols/ControllerManager.java:370">
P1: Missing `enabledSlots[slot] = false` in `onDeviceDisconnected` — the slot assignment is removed but the enabled flag remains `true`. This causes `isSlotEnabled()` and `getEnabledPlayerCount()` to still report the slot as occupied, leaving a ghost controller visible to games. Compare with `onDeviceConnected` which correctly sets `enabledSlots[slot] = true`.</violation>
</file>
<file name="app/src/main/cpp/evshim/evshim.c">
<violation number="1" location="app/src/main/cpp/evshim/evshim.c:323">
P1: Attach failure can permanently strand a controller slot because `last_connected` is updated even when `attach_vjoy` fails, preventing retries.</violation>
<violation number="2" location="app/src/main/cpp/evshim/evshim.c:335">
P1: State updates are no longer gated by `connected`, so a retained virtual joystick handle can keep receiving stale input after the slot is logically disconnected. This can cause ghost inputs or stuck controller state when the Java side marks a slot disconnected without neutralizing the buffer.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:248">
P1: Startup/reconnect can retain stale gamepad state because neutralization is conditional on the persisted `OFF_CONNECTED` value. The mmap files are not explicitly zeroed on creation, so a prior run may leave `OFF_CONNECTED = 1` and stale button/axis bytes. The connect path only calls `writeNeutralGamepadState` when `connected && buffer.getInt(OFF_CONNECTED) == 0`, so if the persisted flag is already 1, stale input state persists until the next real event. Neutralize state unconditionally when a slot becomes connected (or track in-memory transitions) rather than relying on the persisted flag.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:391">
P2: Newly added extra gamepad RandomAccessFile mappings are not closed on stop, creating a multi-handle resource leak risk.</violation>
<violation number="3" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:867">
P2: `sendGamepadState()` is still player-1-only but is called from multi-controller event paths</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:2955">
P1: After hiding virtual on-screen controls, player 1 may remain unassigned because `hideInputControls()` refreshes controller mappings without restoring/reassigning a connected physical controller to slot 0.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:248">
P1: Startup/reconnect can retain stale gamepad state because neutralization is conditional on the persisted `OFF_CONNECTED` value. The mmap files are not explicitly zeroed on creation, so a prior run may leave `OFF_CONNECTED = 1` and stale button/axis bytes. The connect path only calls `writeNeutralGamepadState` when `connected && buffer.getInt(OFF_CONNECTED) == 0`, so if the persisted flag is already 1, stale input state persists until the next real event. Neutralize state unconditionally when a slot becomes connected (or track in-memory transitions) rather than relying on the persisted flag.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:2955">
P1: After hiding virtual on-screen controls, player 1 may remain unassigned because `hideInputControls()` refreshes controller mappings without restoring/reassigning a connected physical controller to slot 0.</violation>
</file>
<file name="app/src/main/cpp/evshim/evshim.c">
<violation number="1" location="app/src/main/cpp/evshim/evshim.c:341">
P1: Virtual joystick state updates can race with the new close/detach path in multi-controller mode. Each `vjoy_updater` thread reads `vjoy_handles[idx]` and calls `apply_vjoy_state` (which invokes `SDL_JoystickSetVirtualAxis/Button/Hat`) without holding `sdl_vjoy_lock`, while `detach_vjoy` now closes and detaches the joystick under that same lock. SDL documents that callers must use `SDL_LockJoysticks()` when accessing the joystick API from multiple threads, and the internal virtual-joystick set functions assert the joystick lock. This can lead to a use-after-close or crash when a controller disconnects while another slot is updating. Consider holding the same lock while reading the handle and applying state, or wrapping all SDL joystick calls with `SDL_LockJoysticks()`/`SDL_UnlockJoysticks()`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Comment on lines
+341
to
+344
| if (!connected || !js) { | ||
| continue; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
P1: Virtual joystick state updates can race with the new close/detach path in multi-controller mode. Each vjoy_updater thread reads vjoy_handles[idx] and calls apply_vjoy_state (which invokes SDL_JoystickSetVirtualAxis/Button/Hat) without holding sdl_vjoy_lock, while detach_vjoy now closes and detaches the joystick under that same lock. SDL documents that callers must use SDL_LockJoysticks() when accessing the joystick API from multiple threads, and the internal virtual-joystick set functions assert the joystick lock. This can lead to a use-after-close or crash when a controller disconnects while another slot is updating. Consider holding the same lock while reading the handle and applying state, or wrapping all SDL joystick calls with SDL_LockJoysticks()/SDL_UnlockJoysticks().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/cpp/evshim/evshim.c, line 341:
<comment>Virtual joystick state updates can race with the new close/detach path in multi-controller mode. Each `vjoy_updater` thread reads `vjoy_handles[idx]` and calls `apply_vjoy_state` (which invokes `SDL_JoystickSetVirtualAxis/Button/Hat`) without holding `sdl_vjoy_lock`, while `detach_vjoy` now closes and detaches the joystick under that same lock. SDL documents that callers must use `SDL_LockJoysticks()` when accessing the joystick API from multiple threads, and the internal virtual-joystick set functions assert the joystick lock. This can lead to a use-after-close or crash when a controller disconnects while another slot is updating. Consider holding the same lock while reading the handle and applying state, or wrapping all SDL joystick calls with `SDL_LockJoysticks()`/`SDL_UnlockJoysticks()`.</comment>
<file context>
@@ -332,7 +338,7 @@ static void *vjoy_updater(void *arg)
}
SDL_Joystick *js = vjoy_handles[idx];
- if (!js) {
+ if (!connected || !js) {
continue;
}
</file context>
AndreVto
reviewed
Jul 1, 2026
| p_SDL_JoystickSetVirtualAxis(js, 5, snap.state.rt); | ||
| for (int i = 0; i < 15; i++) { | ||
| p_SDL_JoystickSetVirtualButton(js, i, snap.state.btn[i]); | ||
| int connected = atomic_load_explicit(&s->connected, memory_order_acquire) != 0; |
Contributor
There was a problem hiding this comment.
we can read from the snapshot we just got right, that should be fine?
Contributor
Author
There was a problem hiding this comment.
Yeah should be. I'll pick this one up with you for more details on how to improve it since this bit was a struggle
Contributor
There was a problem hiding this comment.
oh, this one is simple but it didn't show as I expected
int connected can read from snap.connected directly since try_read_state was successful
Contributor
Author
There was a problem hiding this comment.
Yeah I felt this was just a bit easier to keep track this way
AndreVto
reviewed
Jul 1, 2026
|
|
||
| static void detach_vjoy(int idx) | ||
| { | ||
| pthread_mutex_lock(&sdl_vjoy_lock); |
Contributor
There was a problem hiding this comment.
are these mutexes actually needed?
thread for controller 1 will always read position for player 1 right? and the same for the others
Contributor
Author
There was a problem hiding this comment.
So I was having some issues regarding keeping slots, so popped this in to avoid this.
I can always remove and observe the behaviour
Contributor
There was a problem hiding this comment.
from what I understood there is 1 thread for each joy right? even when no controllers are actually attached? each thread keeps 1 controller id (0, 1, 2, 3) and when it accesses vjoy_handles it reads using the idx it receives when the thread was created
there is no collision of slots in evshim side, when java side notifies a state change it lets controller index x thread update, but there is only one for each controller. each thread updates 1 slot of memory of the array, there is no need for the mutex
Contributor
Author
There was a problem hiding this comment.
That's a fair point. I'll take a look through this over the next couple of days. I'll hit you up a bit more since I'm not too great with the evshim work
Contributor
Author
There was a problem hiding this comment.
Yeah after reading up on this, we don't need the mutex. We can just let the SDL logic handle this. I was being too pernickity.
Will remove and update.
Contributor
|
controllers have preferred slots? can you go over on the slot logic? |
Contributor
Author
So controllers should generally keep the slot ready to receive it in the case of a momentary disconnect and reconnect, unless another controller connects. Otherwise it should grab the lowest slot available. That might not really be desired behaviour though, but that's the intent. |
Contributor
is disconnect/reconnect state change also delayed for evshim? otherwise it'll be possibly out of sync between evshim/SDL and java code |
Owner
|
@phobos665 there's a question for you above |
Contributor
Author
|
TODO:
|
Contributor
Author
It's not no, we send down the connected state down to the evshim and essentially use that to control its state. That way it shouldn't get out of sync. (Might seem a bit blunt, but I feel like it's the easiest and most obvious way of controlling it). |
Contributor
Author
|
So after the discussion, only thing that we should do is optionally remove the mutex. Shouldn't cause issues either way. @utkarshdalal Happy to merge as-is and we can iterate/improve on this solution as we go if that's fine? Avalumi is happy with it, just had a couple of clarifying questions and quick sense-checks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add support for multi-controller support along with correct hot-swapping and correct detach logic.
Some behavioural notes:
Recording
Uploading multi-controller-small.mp4…
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Adds multi‑controller support for up to 4 players with slot‑aware input, per‑slot rumble, hot‑swap, and an optional in‑game controller debug overlay. SDL virtual pads now attach/detach per slot via a new SHM
connectedflag so only active slots appear to games.New Features
connectedat offset 40;evshimmanages up to 4 slots, opens/closes per‑slot SDL joystick handles, attaches/detaches onconnectedchanges, writes neutral state on connect, and applies state only when connected.WinHandlerroutes key/motion to assigned slots, maintains per‑slot buffers and rumble pollers, addssendVirtualGamepadState(state, slot), marks slots connected only when a controller or on‑screen pad is active, and clears disconnected slots.MainActivityandXServerScreenregisterInputManagerlisteners and refresh mappings on hot‑plug/show‑hide;ControllerManagerauto‑assigns devices, prefers last/recently freed slots, accepts built‑in controllers, caches identifiers, and notifies listeners.EVSHIM_MAX_PLAYERS=4.Bug Fixes
SDL_JoystickDetachVirtualto prevent ghost devices and leaks.Written for commit 9a93cf5. Summary will update on new commits.
Summary by CodeRabbit