Improve gamepads rumble and add device rumble#2790
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a unified haptic feedback API for Android and desktop platforms, introducing the HapticDevice interface and extending Joystick and JoyInput to support high/low frequency rumble and durations. It includes a new OnDeviceJoystickRumble setting and utilizes Android's VibrationEffect for improved control. Review feedback suggests ensuring proper lifecycle management in AndroidJoyInput14 to stop rumbles on pause or destruction, caching the Vibrator instance in JmeAndroidSystem for efficiency, and using the new stopRumble() method for clarity. Minor formatting adjustments in SdlJoystickInput were also recommended.
I am having trouble creating individual review comments. Click here to see my feedback.
jme3-android/src/main/java/com/jme3/input/android/AndroidJoyInput14.java (126)
The pauseJoysticks and destroy methods should be overridden to delegate lifecycle calls to joystickJoyInput. Without this, gamepad rumble might continue even when the application is paused or destroyed, as AndroidJoystickJoyInput14 implements its own rumble management that needs to be explicitly stopped.
}
@Override
public void pauseJoysticks() {
super.pauseJoysticks();
joystickJoyInput.pauseJoysticks();
}
@Override
public void destroy() {
super.destroy();
joystickJoyInput.destroy();
}jme3-android/src/main/java/com/jme3/input/android/AndroidJoyInput.java (128)
For better clarity and to leverage the newly added API, consider using JmeSystem.stopRumble() instead of passing a magic number 0f to the rumble method.
JmeSystem.stopRumble();
jme3-android/src/main/java/com/jme3/system/android/JmeAndroidSystem.java (232-238)
Repeatedly calling getSystemService and getContext every time a rumble is requested is less efficient than caching the Vibrator instance. Consider caching it in a static field after the first successful retrieval, ensuring it is cleared if the view is updated.
jme3-lwjgl3/src/main/java/com/jme3/input/lwjgl/SdlJoystickInput.java (401-403)
Adhere to standard Java coding conventions by adding spaces around else and if keywords for better readability.
} else if (duration <= 0f) {
durationMs = 0;
} else {
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified haptic feedback API, adding the HapticDevice interface and updating Joystick and JoyInput to support high/low frequency rumble with duration. The implementation covers Android, LWJGL, and SDL backends, and includes a new setting to redirect joystick rumble to the device vibrator on Android. Review feedback suggests ensuring consistent input clamping in the LWJGL backend, fixing minor formatting and indentation issues, and addressing a potential regression in rumble intensity simulation for older Android devices.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…t.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…icFeedback.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified haptic feedback API across the engine, enabling support for high/low frequency rumble and duration-based effects. Key changes include the introduction of the HapticDevice interface, updates to JoyInput and Joystick to support timed rumbles, and platform-specific implementations for Android (via a new AndroidHapticFeedback utility), LWJGL, and LWJGL3. The Android implementation now allows redirecting joystick rumble to the device vibrator through AppSettings. Review feedback suggests preventing a potential NegativeArraySizeException in the Android haptic pattern generation, optimizing the JInputJoyInput update loop by adding an early return to avoid unnecessary allocations, and using the idiomatic -1 value for infinite SDL rumble duration instead of a magic number.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified haptic feedback API across the engine, adding a HapticDevice interface and enhancing Joystick and JoyInput with support for high/low frequency rumble and specific durations. The Android implementation has been refactored to use a new AndroidHapticFeedback utility that handles varying API levels and amplitude control, while a new OnDeviceJoystickRumble setting allows for redirecting rumble to the device vibrator. Feedback was provided to simplify a redundant conditional block in the Android haptic logic to improve code conciseness.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive haptic feedback system, adding support for both device-level and joystick-level rumble. It includes a new HapticDevice interface, updates to AppSettings and JmeSystem to manage rumble settings, and implements specific rumble logic for Android and LWJGL/SDL backends. The review comments identified potential thread-safety issues with the rumbleStopTimes map in JInputJoyInput and suggested a more precise calculation for duration in nanoseconds, both of which are valid improvements.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Improves gamepads rumble support by making it consistent across backends, and implement device rumble on supported devices (ie. android).
Rumble methods are now abstracted with the
HapticDeviceinterface that is implemented by JmeSystemDelegate and Joystick.