Add PC-accurate controller vibration with per-container settings#1214
Open
TideGear wants to merge 1 commit intoutkarshdalal:masterfrom
Open
Add PC-accurate controller vibration with per-container settings#1214TideGear wants to merge 1 commit intoutkarshdalal:masterfrom
TideGear wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
Vibration works over Bluetooth with DualShock 4, DualSense, and XInput controllers (8BitDo Pro 2 tested). For XInput controllers, vibration does not currently work over USB.
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive vibration/rumble configuration support across the application stack. It adds a native Changes
Sequence DiagramsequenceDiagram
participant UI as UI (ControllerTab)
participant Config as Config<br/>(ContainerData)
participant WinHandler as WinHandler
participant PlayerDevice as InputDevice<br/>(Player)
participant Vibrator as Android Vibrator
participant EVShim as EVShim (native)
UI->>Config: User selects vibration mode & intensity
Config->>Config: Store mode/intensity settings
Config->>WinHandler: setVibrationMode(mode)<br/>setVibrationIntensity(intensity)
WinHandler->>WinHandler: Update vibrationMode & vibrationIntensity fields
Note over EVShim,Vibrator: Runtime Rumble Poll Loop
EVShim->>EVShim: Read per-player rumble frequencies from buffer
EVShim->>WinHandler: Poll rumble state for each player
alt Mode: "controller" or "both"
WinHandler->>PlayerDevice: resolveInputDeviceForPlayer(player)
WinHandler->>WinHandler: startVibrationForPlayer()<br/>with intensity scaling
WinHandler->>PlayerDevice: Vibrate controller
end
alt Mode: "device" or "both"
WinHandler->>Vibrator: VibratorManager.vibrate()<br/>with CombinedVibration (SDK≥31)
Vibrator->>Vibrator: Scale amplitude by intensity %
end
EVShim->>EVShim: Periodic keepalive:<br/>Re-send non-zero rumble<br/>every KEEPALIVE_DUR_MS
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 12 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/app/gamenative/PrefManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/PrefManager.kt:223">
P2: `vibrationMode` persists arbitrary strings despite a closed set of supported modes, creating a cross-file contract bug where invalid values silently disable vibration behavior.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt:75">
P2: New user-visible vibration mode labels are hardcoded in Compose instead of string resources, breaking localization/resource centralization.</violation>
</file>
<file name="app/src/main/cpp/extras/evshim.c">
<violation number="1" location="app/src/main/cpp/extras/evshim.c:163">
P1: Unsynchronized shared keepalive rumble state (`last_rumble_low/high`) is read/written across callback/updater contexts, creating a C data race (undefined behavior).</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:776">
P2: Rumble routing falls back to the first detected gamepad, which can vibrate the wrong controller when multiple devices are connected.</violation>
<violation number="2" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:876">
P1: Stopping rumble depends on current mode, so changing mode can skip canceling previously-started device vibration (up to 60s one-shot).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| * XInput "set and forget" semantics through the SDL translation. */ | ||
| if (p_SDL_JoystickRumble && ++keepalive_ctr >= RUMBLE_KEEPALIVE_TICKS) { | ||
| keepalive_ctr = 0; | ||
| uint16_t kl = last_rumble_low[idx]; |
Contributor
There was a problem hiding this comment.
P1: Unsynchronized shared keepalive rumble state (last_rumble_low/high) is read/written across callback/updater contexts, creating a C data race (undefined behavior).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/cpp/extras/evshim.c, line 163:
<comment>Unsynchronized shared keepalive rumble state (`last_rumble_low/high`) is read/written across callback/updater contexts, creating a C data race (undefined behavior).</comment>
<file context>
@@ -134,6 +155,19 @@ static void *vjoy_updater(void *arg)
+ * XInput "set and forget" semantics through the SDL translation. */
+ if (p_SDL_JoystickRumble && ++keepalive_ctr >= RUMBLE_KEEPALIVE_TICKS) {
+ keepalive_ctr = 0;
+ uint16_t kl = last_rumble_low[idx];
+ uint16_t kh = last_rumble_high[idx];
+ if (kl != 0 || kh != 0) {
</file context>
| if (phoneVibrator != null) { | ||
| phoneVibrator.cancel(); | ||
|
|
||
| if (!"controller".equals(vibrationMode)) { |
Contributor
There was a problem hiding this comment.
P1: Stopping rumble depends on current mode, so changing mode can skip canceling previously-started device vibration (up to 60s one-shot).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/winhandler/WinHandler.java, line 876:
<comment>Stopping rumble depends on current mode, so changing mode can skip canceling previously-started device vibration (up to 60s one-shot).</comment>
<file context>
@@ -607,65 +650,241 @@ private void startRumblePoller() {
- if (phoneVibrator != null) {
- phoneVibrator.cancel();
+
+ if (!"controller".equals(vibrationMode)) {
+ try {
+ Vibrator phoneVibrator = (Vibrator) activity.getSystemService(Context.VIBRATOR_SERVICE);
</file context>
| var vibrationMode: String | ||
| get() = getPref(VIBRATION_MODE, "controller") | ||
| set(value) { | ||
| setPref(VIBRATION_MODE, value) |
Contributor
There was a problem hiding this comment.
P2: vibrationMode persists arbitrary strings despite a closed set of supported modes, creating a cross-file contract bug where invalid values silently disable vibration behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/PrefManager.kt, line 223:
<comment>`vibrationMode` persists arbitrary strings despite a closed set of supported modes, creating a cross-file contract bug where invalid values silently disable vibration behavior.</comment>
<file context>
@@ -216,6 +216,20 @@ object PrefManager {
+ var vibrationMode: String
+ get() = getPref(VIBRATION_MODE, "controller")
+ set(value) {
+ setPref(VIBRATION_MODE, value)
+ }
+
</file context>
Suggested change
| setPref(VIBRATION_MODE, value) | |
| setPref(VIBRATION_MODE, when (value.lowercase()) { | |
| "off", "controller", "device", "both" -> value.lowercase() | |
| else -> "controller" | |
| }) |
| state.config.value = config.copy(dinputMapperType = if (index == 0) 1 else 2) | ||
| }, | ||
| ) | ||
| val vibrationModes = listOf("Off", "Controller", "Device", "Both") |
Contributor
There was a problem hiding this comment.
P2: New user-visible vibration mode labels are hardcoded in Compose instead of string resources, breaking localization/resource centralization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt, line 75:
<comment>New user-visible vibration mode labels are hardcoded in Compose instead of string resources, breaking localization/resource centralization.</comment>
<file context>
@@ -65,6 +72,36 @@ fun ControllerTabContent(state: ContainerConfigState, default: Boolean) {
state.config.value = config.copy(dinputMapperType = if (index == 0) 1 else 2)
},
)
+ val vibrationModes = listOf("Off", "Controller", "Device", "Both")
+ val vibrationModeValues = listOf("off", "controller", "device", "both")
+ val vibrationModeIndex = vibrationModeValues.indexOf(config.vibrationMode).coerceAtLeast(0)
</file context>
| if (player == 0) { | ||
| List<InputDevice> detected = controllerManager.getDetectedDevices(); | ||
| if (!detected.isEmpty()) { | ||
| return detected.get(0); |
Contributor
There was a problem hiding this comment.
P2: Rumble routing falls back to the first detected gamepad, which can vibrate the wrong controller when multiple devices are connected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/winhandler/WinHandler.java, line 776:
<comment>Rumble routing falls back to the first detected gamepad, which can vibrate the wrong controller when multiple devices are connected.</comment>
<file context>
@@ -607,65 +650,241 @@ private void startRumblePoller() {
+ if (player == 0) {
+ List<InputDevice> detected = controllerManager.getDetectedDevices();
+ if (!detected.isEmpty()) {
+ return detected.get(0);
+ }
+ }
</file context>
Suggested change
| return detected.get(0); | |
| return null; |
![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: 5
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/PrefManager.kt (1)
219-231: Normalize vibration preference values at the persistence boundary.
vibrationModecurrently persists arbitrary strings, andvibrationIntensityis only bounded on write. Normalizing on read+write avoids undefined behavior from stale/corrupt prefs.♻️ Suggested hardening patch
+ private val VALID_VIBRATION_MODES = setOf("off", "controller", "device", "both") + private val VIBRATION_MODE = stringPreferencesKey("vibration_mode") var vibrationMode: String - get() = getPref(VIBRATION_MODE, "controller") + get() = getPref(VIBRATION_MODE, "controller") + .takeIf { it in VALID_VIBRATION_MODES } ?: "controller" set(value) { - setPref(VIBRATION_MODE, value) + setPref(VIBRATION_MODE, value.takeIf { it in VALID_VIBRATION_MODES } ?: "controller") } private val VIBRATION_INTENSITY = intPreferencesKey("vibration_intensity") var vibrationIntensity: Int - get() = getPref(VIBRATION_INTENSITY, 100) + get() = getPref(VIBRATION_INTENSITY, 100).coerceIn(0, 100) set(value) { setPref(VIBRATION_INTENSITY, value.coerceIn(0, 100)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/PrefManager.kt` around lines 219 - 231, Persisted vibration preferences aren't normalized on read and only partially bounded on write, so stale or corrupt values can cause undefined behavior; update vibrationMode and vibrationIntensity accessors to validate and normalize both on get and set: for vibrationMode (symbol VIBRATION_MODE and property vibrationMode) accept only a defined set of allowed values (e.g., include "controller" as the default) and on get return the default if stored value is missing/unknown, and on set constrain input to the allowed set before calling setPref; for vibrationIntensity (symbol VIBRATION_INTENSITY and property vibrationIntensity) apply value.coerceIn(0,100) on both set and when reading from getPref (or fallback to the default 100 if stored value is out of range) so getPref/getters never return invalid values; use existing getPref and setPref helpers to centralize this normalization.app/src/main/java/com/winlator/winhandler/WinHandler.java (1)
104-107: ValidatevibrationModebefore storing it.This is persisted as a free-form string, and an unexpected value or casing currently falls through every branch in
startVibrationForPlayer()while still marking the slot as rumbling. Normalizing to the supported set here avoids silent no-op sessions from malformed container data.Suggested normalization
public void setVibrationMode(String mode) { - this.vibrationMode = mode != null ? mode : "controller"; + String normalized = mode == null ? "controller" : mode.toLowerCase(java.util.Locale.ROOT); + switch (normalized) { + case "off": + case "controller": + case "device": + case "both": + this.vibrationMode = normalized; + break; + default: + Log.w(TAG, "Unknown vibration mode '" + mode + "', defaulting to controller"); + this.vibrationMode = "controller"; + break; + } Log.i(TAG, "Vibration mode set to: " + this.vibrationMode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 104 - 107, Validate and normalize the incoming vibrationMode in setVibrationMode before storing: create a canonical set (e.g., SUPPORTED_VIBRATION_MODES) and normalize the input (trim + toLowerCase) and map known aliases to canonical values; if the normalized value is not in SUPPORTED_VIBRATION_MODES, fall back to "controller" and log the normalization. Update setVibrationMode to use this check/mapping so vibrationMode only ever contains a supported value and startVibrationForPlayer will not receive unexpected free-form strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/cpp/extras/evshim.c`:
- Around line 78-90: OnRumble() is writing last_rumble_low/last_rumble_high
concurrently with vjoy_updater() reading them, causing a data race; acquire
shm_mutex around updates and the pwrite snapshot so the pair of low/high values
are written atomically (i.e., lock shm_mutex, update last_rumble_low[idx] and
last_rumble_high[idx] and prepare vals[], call pwrite while still holding the
mutex, then unlock), but move any call to SDL_JoystickRumble() to after
unlocking; apply the same mutex-protected snapshot fix to the other occurrence
that updates last_rumble_* (the block referenced at lines 161-167) so both sites
use shm_mutex for consistent reads/writes.
In `@app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt`:
- Around line 77-104: The UI shows a mismatch because vibrationModeIndex is
derived from vibrationModeValues using config.vibrationMode but the slider
visibility checks the raw config.vibrationMode; normalize the vibration mode
once (e.g., map/normalize config.vibrationMode to a valid value from
vibrationModeValues) before using it in the UI and persist that normalized value
back into state.config (or use a local normalizedMode for both the dropdown and
the slider visibility check). Update the logic around vibrationModeIndex, the
remember block for intensitySlider, and the conditional "if
(config.vibrationMode != \"off\")" to use the normalized value (reference:
vibrationModeIndex, vibrationModeValues, config.vibrationMode,
state.config.value, intensitySlider).
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1546-1547: The code passes raw extras to
WinHandler.setVibrationMode via
handler.setVibrationMode(container.getExtra("vibrationMode", "controller")),
which can store variants like "Controller" and later break
startVibrationForPlayer that only recognizes "off/controller/device/both";
normalize the value before calling setVibrationMode (e.g. read
container.getExtra("vibrationMode", "controller"), trim and toLowerCase it, then
map common synonyms/capitalizations to the canonical tokens "off", "controller",
"device", or "both") so WinHandler.setVibrationMode always receives one of the
accepted lowercase values.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 146-147: The new container creation is bypassing the updated
defaults: update createNewContainer(...) so the ContainerData it constructs
includes vibrationMode and vibrationIntensity from PrefManager (or simply
initialize it from getDefaultContainerData()) instead of hardcoding
"controller"/100; modify the ContainerData construction inside
createNewContainer to set vibrationMode = PrefManager.vibrationMode and
vibrationIntensity = PrefManager.vibrationIntensity (or replace the manual
builder with a call to getDefaultContainerData()) to ensure fresh containers
respect saved defaults.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 616-645: The keepalive logic is re-triggering the 60s phone
one-shot because unchanged rumble calls startVibrationForPlayer(p, lowFreq,
highFreq) which routes "device"/"both" into vibrateDevice(); change the
keepalive path so device vibration is not restarted every keepalive tick—either
add a new method like refreshDeviceVibrationKeepalive(playerId, lowFreq,
highFreq) or add a flag/overload to startVibrationForPlayer to distinguish
initial/start vs keepalive; ensure the initial start still calls vibrateDevice()
with DEVICE_RUMBLE_MS but keepalive calls a separate device refresh with a much
longer cadence (or a no-op for device) and adjust rumbleKeepaliveCtr handling
accordingly so controller rumble continues to be refreshed while device
vibration is not restarted every ~240ms.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 219-231: Persisted vibration preferences aren't normalized on read
and only partially bounded on write, so stale or corrupt values can cause
undefined behavior; update vibrationMode and vibrationIntensity accessors to
validate and normalize both on get and set: for vibrationMode (symbol
VIBRATION_MODE and property vibrationMode) accept only a defined set of allowed
values (e.g., include "controller" as the default) and on get return the default
if stored value is missing/unknown, and on set constrain input to the allowed
set before calling setPref; for vibrationIntensity (symbol VIBRATION_INTENSITY
and property vibrationIntensity) apply value.coerceIn(0,100) on both set and
when reading from getPref (or fallback to the default 100 if stored value is out
of range) so getPref/getters never return invalid values; use existing getPref
and setPref helpers to centralize this normalization.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 104-107: Validate and normalize the incoming vibrationMode in
setVibrationMode before storing: create a canonical set (e.g.,
SUPPORTED_VIBRATION_MODES) and normalize the input (trim + toLowerCase) and map
known aliases to canonical values; if the normalized value is not in
SUPPORTED_VIBRATION_MODES, fall back to "controller" and log the normalization.
Update setVibrationMode to use this check/mapping so vibrationMode only ever
contains a supported value and startVibrationForPlayer will not receive
unexpected free-form strings.
🪄 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: 2ea17b0f-27f1-4d46-b5d7-819da35201fa
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (11)
app/src/main/cpp/extras/CMakeLists.txtapp/src/main/cpp/extras/evshim.capp/src/main/cpp/extras/sdl2_stub/SDL2/SDL.happ/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/res/values/strings.xml
Comment on lines
+78
to
+90
| if (low_frequency_rumble != 0 || high_frequency_rumble != 0) { | ||
| last_rumble_low [idx] = low_frequency_rumble; | ||
| last_rumble_high[idx] = high_frequency_rumble; | ||
| } else { | ||
| last_rumble_low [idx] = 0; | ||
| last_rumble_high[idx] = 0; | ||
| } | ||
|
|
||
| uint16_t vals[2] = { low_frequency_rumble, high_frequency_rumble }; | ||
|
|
||
| pthread_mutex_lock(&shm_mutex); /* NEW */ | ||
| pthread_mutex_lock(&shm_mutex); | ||
| ssize_t w = pwrite(rumble_fd[idx], vals, sizeof(vals), 32); | ||
| pthread_mutex_unlock(&shm_mutex); /* NEW */ | ||
| pthread_mutex_unlock(&shm_mutex); |
Contributor
There was a problem hiding this comment.
Protect last_rumble_* with the same synchronization as the keepalive reader.
OnRumble() now updates these arrays from one thread while vjoy_updater() reads them later for keepalive from another. In C that's a real data race, so the keepalive can replay stale or mixed low/high values. Take the snapshot under the mutex, then call SDL_JoystickRumble() after releasing it.
🔧 Suggested fix
- if (low_frequency_rumble != 0 || high_frequency_rumble != 0) {
- last_rumble_low [idx] = low_frequency_rumble;
- last_rumble_high[idx] = high_frequency_rumble;
- } else {
- last_rumble_low [idx] = 0;
- last_rumble_high[idx] = 0;
- }
-
uint16_t vals[2] = { low_frequency_rumble, high_frequency_rumble };
pthread_mutex_lock(&shm_mutex);
+ last_rumble_low[idx] = low_frequency_rumble;
+ last_rumble_high[idx] = high_frequency_rumble;
ssize_t w = pwrite(rumble_fd[idx], vals, sizeof(vals), 32);
pthread_mutex_unlock(&shm_mutex);
@@
- uint16_t kl = last_rumble_low[idx];
- uint16_t kh = last_rumble_high[idx];
+ uint16_t kl, kh;
+ pthread_mutex_lock(&shm_mutex);
+ kl = last_rumble_low[idx];
+ kh = last_rumble_high[idx];
+ pthread_mutex_unlock(&shm_mutex);
if (kl != 0 || kh != 0) {
p_SDL_JoystickRumble(js, kl, kh, RUMBLE_KEEPALIVE_DUR_MS);Also applies to: 161-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/cpp/extras/evshim.c` around lines 78 - 90, OnRumble() is writing
last_rumble_low/last_rumble_high concurrently with vjoy_updater() reading them,
causing a data race; acquire shm_mutex around updates and the pwrite snapshot so
the pair of low/high values are written atomically (i.e., lock shm_mutex, update
last_rumble_low[idx] and last_rumble_high[idx] and prepare vals[], call pwrite
while still holding the mutex, then unlock), but move any call to
SDL_JoystickRumble() to after unlocking; apply the same mutex-protected snapshot
fix to the other occurrence that updates last_rumble_* (the block referenced at
lines 161-167) so both sites use shm_mutex for consistent reads/writes.
Comment on lines
+77
to
+104
| val vibrationModeIndex = vibrationModeValues.indexOf(config.vibrationMode).coerceAtLeast(0) | ||
| SettingsListDropdown( | ||
| colors = settingsTileColors(), | ||
| title = { Text(text = stringResource(R.string.vibration_mode)) }, | ||
| value = vibrationModeIndex, | ||
| items = vibrationModes, | ||
| onItemSelected = { index -> | ||
| state.config.value = config.copy(vibrationMode = vibrationModeValues[index]) | ||
| }, | ||
| ) | ||
| if (config.vibrationMode != "off") { | ||
| var intensitySlider by remember(config.vibrationIntensity) { | ||
| mutableIntStateOf(config.vibrationIntensity.coerceIn(0, 100)) | ||
| } | ||
| Column(modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp)) { | ||
| Text(text = stringResource(R.string.vibration_intensity)) | ||
| Slider( | ||
| value = intensitySlider.toFloat(), | ||
| onValueChange = { newValue -> | ||
| val clamped = newValue.roundToInt().coerceIn(0, 100) | ||
| intensitySlider = clamped | ||
| state.config.value = config.copy(vibrationIntensity = clamped) | ||
| }, | ||
| valueRange = 0f..100f, | ||
| ) | ||
| Text(text = "$intensitySlider%") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize the stored vibration mode once before deriving UI state.
indexOf(...).coerceAtLeast(0) makes mixed-case or unknown values render as Off, but the slider branch still checks the raw config.vibrationMode. That yields contradictory UI for stale/imported configs: Off is selected while the intensity slider is still visible.
🔧 Suggested fix
- val vibrationModeIndex = vibrationModeValues.indexOf(config.vibrationMode).coerceAtLeast(0)
+ val normalizedVibrationMode = config.vibrationMode
+ .lowercase()
+ .takeIf { it in vibrationModeValues }
+ ?: "off"
+ val vibrationModeIndex = vibrationModeValues.indexOf(normalizedVibrationMode)
@@
- if (config.vibrationMode != "off") {
+ if (normalizedVibrationMode != "off") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val vibrationModeIndex = vibrationModeValues.indexOf(config.vibrationMode).coerceAtLeast(0) | |
| SettingsListDropdown( | |
| colors = settingsTileColors(), | |
| title = { Text(text = stringResource(R.string.vibration_mode)) }, | |
| value = vibrationModeIndex, | |
| items = vibrationModes, | |
| onItemSelected = { index -> | |
| state.config.value = config.copy(vibrationMode = vibrationModeValues[index]) | |
| }, | |
| ) | |
| if (config.vibrationMode != "off") { | |
| var intensitySlider by remember(config.vibrationIntensity) { | |
| mutableIntStateOf(config.vibrationIntensity.coerceIn(0, 100)) | |
| } | |
| Column(modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp)) { | |
| Text(text = stringResource(R.string.vibration_intensity)) | |
| Slider( | |
| value = intensitySlider.toFloat(), | |
| onValueChange = { newValue -> | |
| val clamped = newValue.roundToInt().coerceIn(0, 100) | |
| intensitySlider = clamped | |
| state.config.value = config.copy(vibrationIntensity = clamped) | |
| }, | |
| valueRange = 0f..100f, | |
| ) | |
| Text(text = "$intensitySlider%") | |
| } | |
| } | |
| val normalizedVibrationMode = config.vibrationMode | |
| .lowercase() | |
| .takeIf { it in vibrationModeValues } | |
| ?: "off" | |
| val vibrationModeIndex = vibrationModeValues.indexOf(normalizedVibrationMode) | |
| SettingsListDropdown( | |
| colors = settingsTileColors(), | |
| title = { Text(text = stringResource(R.string.vibration_mode)) }, | |
| value = vibrationModeIndex, | |
| items = vibrationModes, | |
| onItemSelected = { index -> | |
| state.config.value = config.copy(vibrationMode = vibrationModeValues[index]) | |
| }, | |
| ) | |
| if (normalizedVibrationMode != "off") { | |
| var intensitySlider by remember(config.vibrationIntensity) { | |
| mutableIntStateOf(config.vibrationIntensity.coerceIn(0, 100)) | |
| } | |
| Column(modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp)) { | |
| Text(text = stringResource(R.string.vibration_intensity)) | |
| Slider( | |
| value = intensitySlider.toFloat(), | |
| onValueChange = { newValue -> | |
| val clamped = newValue.roundToInt().coerceIn(0, 100) | |
| intensitySlider = clamped | |
| state.config.value = config.copy(vibrationIntensity = clamped) | |
| }, | |
| valueRange = 0f..100f, | |
| ) | |
| Text(text = "$intensitySlider%") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt` around
lines 77 - 104, The UI shows a mismatch because vibrationModeIndex is derived
from vibrationModeValues using config.vibrationMode but the slider visibility
checks the raw config.vibrationMode; normalize the vibration mode once (e.g.,
map/normalize config.vibrationMode to a valid value from vibrationModeValues)
before using it in the UI and persist that normalized value back into
state.config (or use a local normalizedMode for both the dropdown and the slider
visibility check). Update the logic around vibrationModeIndex, the remember
block for intensitySlider, and the conditional "if (config.vibrationMode !=
\"off\")" to use the normalized value (reference: vibrationModeIndex,
vibrationModeValues, config.vibrationMode, state.config.value, intensitySlider).
Comment on lines
+1546
to
+1547
| handler.setVibrationMode(container.getExtra("vibrationMode", "controller")) | ||
| handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) |
Contributor
There was a problem hiding this comment.
Normalize vibrationMode before handing it to WinHandler.
WinHandler.setVibrationMode() only falls back when the value is null, and startVibrationForPlayer() later recognizes only off/controller/device/both. A stale or imported extra like "Controller" will therefore be stored verbatim and silently disable rumble.
🔧 Suggested fix
- handler.setVibrationMode(container.getExtra("vibrationMode", "controller"))
+ val supportedVibrationModes = setOf("off", "controller", "device", "both")
+ val vibrationMode = container.getExtra("vibrationMode", "controller")
+ .lowercase(Locale.ROOT)
+ .takeIf { it in supportedVibrationModes }
+ ?: "controller"
+ handler.setVibrationMode(vibrationMode)
handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 1546 - 1547, The code passes raw extras to WinHandler.setVibrationMode via
handler.setVibrationMode(container.getExtra("vibrationMode", "controller")),
which can store variants like "Controller" and later break
startVibrationForPlayer that only recognizes "off/controller/device/both";
normalize the value before calling setVibrationMode (e.g. read
container.getExtra("vibrationMode", "controller"), trim and toLowerCase it, then
map common synonyms/capitalizations to the canonical tokens "off", "controller",
"device", or "both") so WinHandler.setVibrationMode always receives one of the
accepted lowercase values.
Comment on lines
+146
to
+147
| vibrationMode = PrefManager.vibrationMode, | ||
| vibrationIntensity = PrefManager.vibrationIntensity, |
Contributor
There was a problem hiding this comment.
New containers still ignore the saved vibration defaults.
This updates getDefaultContainerData(), but Line 811 still builds ContainerData manually without vibrationMode or vibrationIntensity. Fresh containers will therefore save "controller" / 100 instead of the current PrefManager values.
Suggested fix in createNewContainer(...)
portraitMode = PrefManager.portraitMode,
externalDisplayMode = PrefManager.externalDisplayInputMode,
externalDisplaySwap = PrefManager.externalDisplaySwap,
+ vibrationMode = PrefManager.vibrationMode,
+ vibrationIntensity = PrefManager.vibrationIntensity,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vibrationMode = PrefManager.vibrationMode, | |
| vibrationIntensity = PrefManager.vibrationIntensity, | |
| portraitMode = PrefManager.portraitMode, | |
| externalDisplayMode = PrefManager.externalDisplayInputMode, | |
| externalDisplaySwap = PrefManager.externalDisplaySwap, | |
| vibrationMode = PrefManager.vibrationMode, | |
| vibrationIntensity = PrefManager.vibrationIntensity, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt` around lines 146 -
147, The new container creation is bypassing the updated defaults: update
createNewContainer(...) so the ContainerData it constructs includes
vibrationMode and vibrationIntensity from PrefManager (or simply initialize it
from getDefaultContainerData()) instead of hardcoding "controller"/100; modify
the ContainerData construction inside createNewContainer to set vibrationMode =
PrefManager.vibrationMode and vibrationIntensity =
PrefManager.vibrationIntensity (or replace the manual builder with a call to
getDefaultContainerData()) to ensure fresh containers respect saved defaults.
Comment on lines
+616
to
645
| for (int p = 0; p < MAX_PLAYERS; p++) { | ||
| try { | ||
| MappedByteBuffer buf = getBufferForPlayer(p); | ||
| if (buf == null) continue; | ||
| short lowFreq = buf.getShort(32); | ||
| short highFreq = buf.getShort(34); | ||
| boolean changed = lowFreq != lastLowFreq[p] || highFreq != lastHighFreq[p]; | ||
| if (changed) { | ||
| lastLowFreq[p] = lowFreq; | ||
| lastHighFreq[p] = highFreq; | ||
| rumbleKeepaliveCtr[p] = 0; | ||
| if (lowFreq == 0 && highFreq == 0) { | ||
| stopVibration(); | ||
| stopVibrationForPlayer(p); | ||
| } else { | ||
| startVibration(lowFreq, highFreq); | ||
| startVibrationForPlayer(p, lowFreq, highFreq); | ||
| } | ||
| } else if (isRumbling[p]) { | ||
| rumbleKeepaliveCtr[p]++; | ||
| if (rumbleKeepaliveCtr[p] >= RUMBLE_KEEPALIVE_INTERVAL) { | ||
| rumbleKeepaliveCtr[p] = 0; | ||
| startVibrationForPlayer(p, lowFreq, highFreq); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| // Buffer may be unmapped; continue polling | ||
| } | ||
| } catch (Exception e) { | ||
| continue; | ||
| } | ||
| try { | ||
| Thread.sleep(20); // Poll for new commands 50 times per second | ||
| Thread.sleep(20); | ||
| } catch (InterruptedException e) { |
Contributor
There was a problem hiding this comment.
Keepalive is re-triggering 60s phone vibrations.
Lines 632-636 call startVibrationForPlayer() on unchanged rumble, and Lines 845-849 route "device" / "both" back into vibrateDevice(). Because vibrateDevice() issues a 60000 ms one-shot, sustained rumble in device modes gets restarted every ~240 ms instead of just letting the existing effect run. The controller keepalive needs a separate path from the device vibrator.
Suggested direction
} else if (isRumbling[p]) {
rumbleKeepaliveCtr[p]++;
if (rumbleKeepaliveCtr[p] >= RUMBLE_KEEPALIVE_INTERVAL) {
rumbleKeepaliveCtr[p] = 0;
- startVibrationForPlayer(p, lowFreq, highFreq);
+ if ("controller".equals(vibrationMode) || "both".equals(vibrationMode)) {
+ vibrateController(p, lowFreq, highFreq);
+ }
}
}Use a separate, much longer refresh cadence if you still need device vibration to outlive DEVICE_RUMBLE_MS.
Also applies to: 843-849
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 616 -
645, The keepalive logic is re-triggering the 60s phone one-shot because
unchanged rumble calls startVibrationForPlayer(p, lowFreq, highFreq) which
routes "device"/"both" into vibrateDevice(); change the keepalive path so device
vibration is not restarted every keepalive tick—either add a new method like
refreshDeviceVibrationKeepalive(playerId, lowFreq, highFreq) or add a
flag/overload to startVibrationForPlayer to distinguish initial/start vs
keepalive; ensure the initial start still calls vibrateDevice() with
DEVICE_RUMBLE_MS but keepalive calls a separate device refresh with a much
longer cadence (or a no-op for device) and adjust rumbleKeepaliveCtr handling
accordingly so controller rumble continues to be refreshed while device
vibration is not restarted every ~240ms.
Author
|
I'll address the conflicts and AI points tonight. |
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
1 participant
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.
...or as accurate as I'm smart enough to achieve.
Adds full controller vibration support that mirrors PC behavior — games trigger the physical controller's rumble motors instead of (or in addition to) the Android device vibrator.
What's included:
VibratorManager(API 31+) to drive low-frequency and high-frequency motors independently on DualShock 4, DualSense, and XInput controllers; falls back to blended single-vibrator on older APIsread()→pread(..., 0)invjoy_updaterso the polling loop always reads from offset 0 instead of advancing past the data after one iteration (this was silently breaking button/axis forwarding)BionicProgramLauncherComponentso evshim updates ship with the APK rather than requiring imagefs rebuildsKnown limitation: XInput controller vibration does not currently work over USB — this is an Android platform limitation that will be addressed in a follow-up.
Recording
https://photos.app.goo.gl/uRuX5h6xcWU9iZHf9
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 PC-accurate controller vibration with per-container settings. Supports DualShock 4, DualSense, and XInput over Bluetooth; XInput over USB is not yet supported.
New Features
ContainerData/PrefManagerand exposed in the Controller tab.VibratorManager(API 31+) with fallback to blended single motor on older APIs; per-player rumble with SDL keepalive to match PC/XInput “set and forget” behavior.libevshim.soin the APK and auto-deploy on launch.Bug Fixes
pread(..., 0)to avoid advancing past the buffer, fixing lost button/axis updates and rumble reads.Written for commit 0998922. Summary will update on new commits.
Summary by CodeRabbit