feat(mobile): map touches to Super Scope and Mouse coordinates - #292
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe mobile crate now exposes Rust and UniFFI touch mapping. It converts viewport coordinates to framebuffer aim points, tracks fractional touch-mouse movement, and documents tested mapping alongside outstanding on-screen controls. ChangesMobile touch input
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TouchInput
participant TouchMouse
participant MouseAccumulator
TouchInput->>TouchMouse: begin(tx, ty)
TouchMouse->>MouseAccumulator: initialize drag origin
TouchInput->>TouchMouse: drag(tx, ty, sensitivity)
TouchMouse->>MouseAccumulator: calculate whole delta and residual
MouseAccumulator-->>TouchMouse: MouseDelta
TouchMouse-->>TouchInput: MouseDelta
TouchInput->>TouchMouse: end()
TouchMouse->>MouseAccumulator: reset drag state
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/rustysnes-mobile/src/touch.rs`:
- Around line 176-214: Extract a private helper on TouchMouse that locks self.0
and applies the existing PoisonError::into_inner recovery, returning the mutex
guard. Replace the repeated lock expressions in begin, drag, and end with calls
to this helper, preserving each method’s current behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e52cfe95-ac24-443b-a600-2ba5054ac568
📒 Files selected for processing (4)
CHANGELOG.mdcrates/rustysnes-mobile/src/lib.rscrates/rustysnes-mobile/src/touch.rsdocs/mobile-readiness.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (12)
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/mobile-readiness.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/mobile-readiness.mdcrates/rustysnes-mobile/src/lib.rsCHANGELOG.mdcrates/rustysnes-mobile/src/touch.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/mobile-readiness.mdcrates/rustysnes-mobile/src/lib.rsCHANGELOG.mdcrates/rustysnes-mobile/src/touch.rs
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/mobile-readiness.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/mobile-readiness.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/mobile-readiness.mdCHANGELOG.md
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-mobile/src/lib.rscrates/rustysnes-mobile/src/touch.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
crates/rustysnes-mobile/src/lib.rscrates/rustysnes-mobile/src/touch.rs
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-mobile/src/lib.rscrates/rustysnes-mobile/src/touch.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
crates/rustysnes-mobile/src/lib.rscrates/rustysnes-mobile/src/touch.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-mobile/src/lib.rscrates/rustysnes-mobile/src/touch.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
🔇 Additional comments (7)
crates/rustysnes-mobile/src/touch.rs (3)
78-105: LGTM!
112-168: LGTM!
216-364: LGTM!crates/rustysnes-mobile/src/lib.rs (1)
36-37: LGTM!CHANGELOG.md (1)
14-29: LGTM!docs/mobile-readiness.md (2)
217-225: LGTM!
264-265: LGTM!
049fb4c to
cf3a0ee
Compare
The `.lock().unwrap_or_else(PoisonError::into_inner)` chain was written out in begin, drag and end, so the poisoning policy was stated three times and could drift. CodeRabbit's finding on #292. The helper lives in a separate plain impl block: `#[uniffi::export]` exports every method it contains, and a MutexGuard has no FFI representation. Regenerated the Kotlin bindings to confirm the exported interface is unchanged -- begin/drag/end only, no leaked `get`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The FFI exposed set_superscope and set_mouse, but both take units a touchscreen does not have: the Scope wants SNES screen space, the Mouse wants a relative delta in counts. rustysnes-mobile::touch closes that gap. map_touch_to_screen maps through the letterboxed viewport and takes the active framebuffer size as a parameter, so aim stays correct when a game switches to a hi-res mode mid-scene; a touch in the letterbox bars is reported off-screen rather than snapped to an edge, because several games read the Scope's off-screen state as "reload". TouchMouse carries the sub-count residual across FFI calls. Without it a slow drag truncates to zero every frame and the pointer never moves at all -- the load-bearing behaviour here, and the one the injection test confirms. This lives in Rust rather than in the shells because otherwise it is written twice, in Kotlin and in Swift, and the two drift; and because this crate is the only place it can be tested at all, neither shell having a test harness and this environment having no macOS toolchain. Kotlin bindings verified to generate. The on-screen controls that would drive it are still outstanding; docs/mobile-readiness.md now records which half is done. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every existing case in touch.rs used `view.y == 0`, which made the `- view.y` term in the vertical mapping untested: injecting `rel_y = ty / view.height` -- dropping the origin entirely -- passed all 18 tests. A portrait phone holding a 4:3 picture has bars on the top and bottom, not the sides, so this is the orientation most users would actually hit; aim would have been off by the bar height for all of them. The new case fails under that injection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `.lock().unwrap_or_else(PoisonError::into_inner)` chain was written out in begin, drag and end, so the poisoning policy was stated three times and could drift. CodeRabbit's finding on #292. The helper lives in a separate plain impl block: `#[uniffi::export]` exports every method it contains, and a MutexGuard has no FFI representation. Regenerated the Kotlin bindings to confirm the exported interface is unchanged -- begin/drag/end only, no leaked `get`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4972f17 to
03d5de4
Compare
Antigravity review (Gemini via Ultra)Adds Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
🟡 Not ready to approve
There is a confirmed accumulator correctness issue when mouse deltas exceed i32 range due to residual subtraction using the pre-saturated float value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a Rust-side, UniFFI-exported touch-mapping layer for mobile so touch events can be converted into Super Scope screen-space aim points and SNES Mouse relative deltas (with residual accumulation), and updates mobile-readiness documentation and the changelog to reflect what’s now covered vs. still missing UI.
Changes:
- Introduce
rustysnes-mobile::touchwithmap_touch_to_screen/map_aimand a statefulTouchMouse(mutex-backed) that preserves sub-count mouse movement across FFI calls. - Add unit tests covering viewport letterboxing, degenerate viewports, hi-res framebuffer sizing, and slow-drag residual behavior.
- Update
docs/mobile-readiness.mdandCHANGELOG.mdto clarify that arithmetic/mapping is implemented and tested, while on-screen controls and Multitap UX remain outstanding.
File summaries
| File | Description |
|---|---|
| docs/mobile-readiness.md | Updates the mobile-readiness gap statement to distinguish tested arithmetic from still-missing on-screen UX and Multitap UI. |
| crates/rustysnes-mobile/src/touch.rs | Implements touch→screen mapping and drag→mouse-count accumulation, exported via UniFFI with unit tests. |
| crates/rustysnes-mobile/src/lib.rs | Exposes the new touch module from the mobile UniFFI bridge crate. |
| CHANGELOG.md | Adds release notes describing the new mobile touch-to-peripheral mapping and what remains out of scope. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| let dx = self.residual_x.trunc(); | ||
| let dy = self.residual_y.trunc(); | ||
| self.residual_x -= dx; | ||
| self.residual_y -= dy; | ||
|
|
||
| #[allow(clippy::cast_possible_truncation)] | ||
| (dx as i32, dy as i32) |
Part of
v1.30.0(mobile store-readiness engineering).docs/mobile-readiness.mdhas listed "no Mouse/Super Scope/Multitap touch UX" as an open gap sincev1.16.0. This closes the half of it that can be verified, and says plainly which half is left.The gap
set_superscopeandset_mousewere already on the FFI, but neither takes anything a touchscreenproduces:
frame.
What landed
crates/rustysnes-mobile/src/touch.rs:map_touch_to_screen— maps a touch through the viewport. It takes the active framebuffersize as a parameter rather than assuming 256x224, so aim stays correct when a game switches to a
hi-res mode mid-scene, and it reports a touch in the letterbox bars as
on_screen: falseratherthan snapping it to an edge — several games read the Scope's off-screen state as "reload", so the
caller has to be able to tell. A degenerate 0x0 viewport is handled rather than assumed away;
Android reports one between rotation and first layout.
TouchMouse— carries the sub-count residual across FFI calls, which is the load-bearingpart: without it a naive
delta as i32truncates every slow-drag frame to zero and the pointernever moves at all.
Why in Rust and not in the shells
Otherwise it is written twice, in Kotlin and in Swift, and the two drift. It is also the only place
it can be tested — neither shell has a test harness and this environment has no macOS toolchain at
all. Keeping the platform layer to "forward the touch, forward the result" makes a mapping bug a
cargo testfailure rather than a user aiming half a screen off.Verification
18 tests. Kotlin bindings generated and inspected —
mapTouchToScreen,TouchMouseInterface(
begin/drag/end), and theAimPoint/MouseDelta/Viewportrecords are all present.Two mechanisms were injection-tested rather than assumed:
mul_addresidual accumulation with a plain multiply failsa_slow_drag_still_moves_the_mouse, as intended;beginsets it,endclears it), so injecting ano-op into either one alone leaves the other covering and the test still passes. Removing both
fails it. The test comment now records that it pins the pair rather than claiming it pins a single
guard — the redundancy is deliberate, since a shell that drops a
begin(Android deliversACTION_MOVEwithout a precedingACTION_DOWNafter a gesture-recognizer steal) is still safe.Still outstanding
The on-screen affordances themselves — a Scope reticle, Mouse button targets, a peripheral picker —
and Multitap, whose port assignment is UI rather than arithmetic.
docs/mobile-readiness.mdisupdated in both places it made the old claim.
🤖 Generated with Claude Code
Summary
This change claims that mobile touch input maps correctly to Super Scope and Mouse input in Rust.
The claim is false if letterbox bars are treated as on-screen, framebuffer dimensions are ignored, zero-sized viewports panic or produce invalid coordinates, or slow drags lose fractional movement across calls.
map_touch_to_screenmaps touches using the active framebuffer size and marks letterbox-bar touches as off-screen.TouchMousepreserves movement residuals across FFI calls. Tests cover mapping, viewport edge cases, lifecycle behavior, and residual bounds. Kotlin bindings were verified.The change does not add on-screen controls, peripheral selection, or Multitap support. These remain outstanding.