Android: Samsung S-Pen stylus support - #19076
Conversation
|
If you can commit this to your PR on top, this addresses some of the issues I still have with it |
Hardware-validation finding: the soft-hide change breaks hover-driven cursor for stylus +
|
|
Pushed the revert as commit If you'd rather amend |
| return android->pointer[idx].full_y; | ||
| return android->pointer[idx].confined_y; | ||
| case RETRO_DEVICE_ID_POINTER_PRESSED: | ||
| #ifdef DEBUG_ANDROID_INPUT |
There was a problem hiding this comment.
This macro isn't used anywhere else. Should it just be the default debug macro instead? Or just remove it entirely. Seems like your trouble shooting.
| action == AMOTION_EVENT_ACTION_UP | ||
| || action == AMOTION_EVENT_ACTION_CANCEL | ||
| || action == AMOTION_EVENT_ACTION_POINTER_UP); | ||
| /* C89: All variable declarations at function start */ |
There was a problem hiding this comment.
We don't need a note about C89 declarations. This is already a norm across the code-base.
|
|
||
| --- | ||
|
|
||
| *This documentation should be updated when modifying the S Pen implementation to ensure future maintainers understand the complete protection strategy.* |
There was a problem hiding this comment.
This doc is crazy large, and extremely verbose. Doubt all of it is needed.
| /* Calculate pressure threshold from user setting (1-100 -> 0.0248-0.0000) */ | ||
| /* Higher sensitivity value = lower threshold = more sensitive */ | ||
| /* At max sensitivity (100), threshold is 0 = instant click on contact */ | ||
| pressure_threshold = 0.0f + ((100 - settings->uints.input_stylus_pressure_sensitivity) * 0.00025f); |
There was a problem hiding this comment.
= 0.0f + ? Likely better to cast instead.
| /* Defined in input/drivers/android_input.c. True for ~2s after any S-Pen event; | ||
| * used below to auto-hide the on-screen touch overlay while the stylus is in use. */ | ||
| extern bool android_input_stylus_recently_active(void); |
There was a problem hiding this comment.
Having this in run_loop.c seems silly. Maybe introduce a android_input.h or something?
| input_overlay_unload(); | ||
| else | ||
| input_overlay_init(); | ||
| last_stylus_hidden = stylus_hidden; |
There was a problem hiding this comment.
This is going to be run whether or not the Stylus device is enabled. Intentional?
| return snprintf(s, len, "%d", *setting->value.target.unsigned_integer); | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is essentially a 1-line function. Why not just call snprintf() directly instead?
|
I ran out of patience on reviewing the vibes here. |
then task an agent ;) my implementation works, its been hardware tested. I am happy to keep working on this and refining it but lets keep our comments constructive. your emotions, and patience dont really have a place in the conversation, respectfully. |
…y gate) Five small fixes from RobLoach's review of libretro#19076 plus a verified-not-a-bug correction. Behavior change is confined to the stylus-enable gate; the rest is cosmetic. - Drop self-add from copyright header (RobLoach style: contributors don't amend the header for feature additions). - Delete the "C89: variables at function start" reminder comment; CODING-GUIDELINES already documents this convention and no other source file in the tree explains it inline. - Drop the dead `0.0f +` prefix from pressure_threshold; the `* 0.00025f` already forces float promotion. Explicit `(int)` cast on the unsigned operand avoids a signed/unsigned mix. - Reorder the stylus-enable check: settings lookup -> enable gate -> timestamp update, so a user who has disabled S-Pen no longer has their on-screen overlay auto-hidden by a passing pen. - Replace the tautological `else if (is_finger || !is_stylus)` with plain `else`. The condition was always true inside the `else` branch of `if (is_stylus)`. Hardware-validated on Z Fold 5 + snes9x stylus core (Mario Paint EUR): hover cursor tracking unchanged, tap-to-click unchanged, stylus-disabled correctly leaves the overlay visible during pen activity.
Batch 2/3 review nits + a comment cleanup pass in response to RobLoach's
inline comments and cscd98's follow-up ("remove docs/, remove excessive
AI generated comments").
Review nits:
- runloop.c: replace the inline extern declaration with a new
input/drivers/android_input.h header.
- runloop.c: gate the S-Pen overlay auto-hide on input_stylus_enable
so it does not run when the user has disabled the feature.
- menu/menu_setting.c: drop the one-line pressure_sensitivity
representation helper; reuse setting_get_string_representation_max_users
which does the same %d formatting.
- android_input.c: bounds-check input_stylus_pressure_sensitivity
to 0..100 before computing the pressure threshold.
- Remove the verbose per-event pressure log (troubleshooting
leftover; DEBUG_ANDROID_INPUT block still covers real debug needs).
Comment / doc cleanup:
- Delete docs/S-Pen-Implementation.md; the PR body already describes
the feature at the right level of detail.
- Trim excessive commentary in android_input.c: drop section-banner
comments, WHAT-restatement comments, and architecture essays.
Retained comments now document only hidden constraints (Samsung
phantom-touch quirk, pressure-threshold mapping, semantic pointer
indices, overlay auto-hide invariant).
- Trim android_input.h and runloop.c comment blocks to one useful
sentence each.
Constants:
- Introduce DEFAULT_INPUT_STYLUS_PRESSURE_SENSITIVITY and use it in
configuration.c and menu/menu_setting.c so the literal 70 is not
duplicated across three sites.
Verified: aarch64 debug APK builds clean.
9dab124 to
e058abf
Compare
Comprehensive stylus support for the Android input driver: - Phantom-tap suppression for Samsung pens (hover guard, proximity tracking, quick-tap gating, menu gesture isolation). - toolType-based stylus/finger classification with source fallback. - Distance-based contact detection (instant cursor) separated from pressure-based click detection (configurable sensitivity). - Pointer index contract for cores: 0=cursor, 1=tip contact, 2=side button. - PRIMARY + SECONDARY side-button support. - Settings (configuration + menu + i18n wiring): input_stylus_enable, input_stylus_pressure_sensitivity, input_stylus_require_contact_for_click, input_stylus_hover_moves_pointer. - Auto-hide the on-screen overlay while the stylus is active, restored ~2s after the last stylus event (runloop hook next to the existing controller-connected hide block). Squashed from the prior S-Pen commit series; stray version.all / AndroidManifest version downgrades dropped.
Architecture and maintainer reference for the Android S-Pen input driver, settings, contact/click separation, and the pointer contract for cores.
Follow-up fixes on top of the S-Pen stylus PR: - C89: hoist side_pressed/tip_touching/pressure_threshold to the android_input_poll_event_type_motion declaration block; convert the inline definitions to assignments. Wrap the Android physical-keyboard selector in menu_setting.c in its own block so st/current_input are declared at block start. RetroArch .c files must build clean under gcc -std=c89 -pedantic. - Add AMOTION_EVENT_AXIS_DISTANCE (24) to the __ANDROID_API__ < 14 fallback enum. It was used by the contact-distance path but only present in current NDK headers, breaking the fallback contract that the rest of the new constants already honor. - runloop.c: when input_overlay_pointer_enable is set, soft-hide the overlay via INPUT_OVERLAY_GAMEPAD_HIDDEN instead of unloading it, mirroring the gamepad-connected block. Unloading tore down overlay_ptr, which is the pointer surface the stylus drives.
Hardware testing on Samsung S-Pen + snes9x stylus/spen-support core (Mario Paint Europe, CRC 266B220E) confirmed the soft-hide path added in 887f038 breaks hover-driven cursor movement. With pointer enable true, a loaded overlay intercepts RETRO_DEVICE_POINTER queries and answers from its own touch tracker (count=1 PRESSED=true) instead of the native android_input pointer state (count=0 PRESSED=false for hover), destroying the count-independent coord-change hover contract that S-Pen-aware cores rely on. Symptom: hover stops moving the in-game cursor; only tip+drag moves it. Toggling Display On-Screen Overlay OFF restores hover immediately. The reasoning behind the soft-hide ("Unloading tore down overlay_ptr, which is the pointer surface the stylus drives") does not apply to native S-Pen events: those write directly to android->pointer[0..2].x/y in android_input.c and bypass overlay_ptr entirely. overlay_ptr IS the pointer surface for finger-driven overlay-as-mouse/lightgun, but that use case does not apply during stylus activity (auto-hide is exactly the window where no finger is on the overlay), so preserving it has no benefit and the cost is breaking the feature this PR is built around. Reverts the runloop.c change from 887f038 to the original 47fea8b24 behavior (always input_overlay_unload() on edge). Keeps the C89 and AMOTION_EVENT_AXIS_DISTANCE fixes from 887f038 intact. Adds an extended comment block documenting the invariant so a future review does not replay the same thread.
Make the S-Pen act as a continuous pointer (mouse-like), not a touchscreen-like tip-down-only input. Hover already updates pointer[0] X/Y without asserting PRESSED, so the tap=left/barrel=right click contract is preserved; this flip just lets the cursor follow the pen without requiring tip contact.
…y gate) Five small fixes from RobLoach's review of libretro#19076 plus a verified-not-a-bug correction. Behavior change is confined to the stylus-enable gate; the rest is cosmetic. - Drop self-add from copyright header (RobLoach style: contributors don't amend the header for feature additions). - Delete the "C89: variables at function start" reminder comment; CODING-GUIDELINES already documents this convention and no other source file in the tree explains it inline. - Drop the dead `0.0f +` prefix from pressure_threshold; the `* 0.00025f` already forces float promotion. Explicit `(int)` cast on the unsigned operand avoids a signed/unsigned mix. - Reorder the stylus-enable check: settings lookup -> enable gate -> timestamp update, so a user who has disabled S-Pen no longer has their on-screen overlay auto-hidden by a passing pen. - Replace the tautological `else if (is_finger || !is_stylus)` with plain `else`. The condition was always true inside the `else` branch of `if (is_stylus)`. Hardware-validated on Z Fold 5 + snes9x stylus core (Mario Paint EUR): hover cursor tracking unchanged, tap-to-click unchanged, stylus-disabled correctly leaves the overlay visible during pen activity.
|
@f4mrfaux Sidebar, I've been working on some SDL3 updates within RetroArch. There is a Pen API that may work with it? https://wiki.libsdl.org/SDL3/CategoryPen I don't have a Samsung S-Pen to test, but would be curious if it could cover some of what this does. Hope to build out the PR for it within the week, after some of the initial fixes are in. |
Batch 2/3 review nits + a comment cleanup pass in response to RobLoach's
inline comments and cscd98's follow-up ("remove docs/, remove excessive
AI generated comments").
Review nits:
- runloop.c: replace the inline extern declaration with a new
input/drivers/android_input.h header.
- runloop.c: gate the S-Pen overlay auto-hide on input_stylus_enable
so it does not run when the user has disabled the feature.
- menu/menu_setting.c: drop the one-line pressure_sensitivity
representation helper; reuse setting_get_string_representation_max_users
which does the same %d formatting.
- android_input.c: bounds-check input_stylus_pressure_sensitivity
to 0..100 before computing the pressure threshold.
- Remove the verbose per-event pressure log (troubleshooting
leftover; DEBUG_ANDROID_INPUT block still covers real debug needs).
Comment / doc cleanup:
- Delete docs/S-Pen-Implementation.md; the PR body already describes
the feature at the right level of detail.
- Trim excessive commentary in android_input.c: drop section-banner
comments, WHAT-restatement comments, and architecture essays.
Retained comments now document only hidden constraints (Samsung
phantom-touch quirk, pressure-threshold mapping, semantic pointer
indices, overlay auto-hide invariant).
- Trim android_input.h and runloop.c comment blocks to one useful
sentence each.
Constants:
- Introduce DEFAULT_INPUT_STYLUS_PRESSURE_SENSITIVITY and use it in
configuration.c and menu/menu_setting.c so the literal 70 is not
duplicated across three sites.
Verified: aarch64 debug APK builds clean.
e058abf to
15ee728
Compare
4 participants
Description
Focused, S-Pen-only re-submission of Android Samsung S-Pen stylus support, rebased onto current
master.This addresses the review that closed the earlier version of this PR. The branch has been split down to the S-Pen feature alone — the unrelated files (
cheat_managerand other "omnibus" changes, the Pokémon-branded files,config.def.h) and the non-portablestrcasestrusage are gone. It builds, and the history is two clean commits on top of currentmaster.What it does
RETRO_DEVICE_POINTERpath — no ad-hoc mouse/touch emulation.toolType-based stylus/finger classification with input-source fallback.AMOTION_EVENT_BUTTON_STYLUS_PRIMARYand…_SECONDARYbarrel buttons (devices vary).Settings (Android only; menu + i18n wired)
input_stylus_enable— master switch (default ON)input_stylus_require_contact_for_click— tip contact required for a click (default ON)input_stylus_hover_moves_pointer— hover moves pointer without asserting PRESSED (default OFF)input_stylus_pressure_sensitivity— click pressure threshold, 1–100 (default 70)How prior review was addressed
cheat_manager/unrelated files.strcasestr(or other non-portable libc calls) introduced.aarch64debug build. Also fixed a config-persistence bug whereinput_stylus_pressure_sensitivity(a uint) was mistakenly registered among the bool settings, so it now saves/loads correctly.Files changed
input/drivers/android_input.c,configuration.c,configuration.h,menu/menu_setting.c,menu/menu_displaylist.c,menu/cbs/menu_cbs_sublabel.c,menu/menu_driver.c,msg_hash.h,intl/msg_hash_lbl.h,intl/msg_hash_us.h,runloop.c,docs/S-Pen-Implementation.md.Related issues
Credits