fix(compositor): clamp the click-bounce cursor size at zero - #680
Conversation
plan_cursor scales the bounce deviation by the raw clickBounce setting, whose slider goes up to 5. The press trough of CursorTrack::bounce is 0.76, so above 1/0.24 (about 4.17) the factor went negative and a negative size_px reached every backend's sprite draw, drawing the cursor rotated by 180 degrees for a few frames after each click. Floor the factor at 0 in plan_cursor, the one site all three backends share, and return None when the size is not positive. Skipping is safer than drawing a zero-size sprite: on a tilted plane its four corners coincide and the mode 13 inverse warp solves 0/0, whose rejection only holds if NaN comparisons behave, which Metal's fast-math does not promise. The motion-blur trail reuses the same size_px, so it is covered too. The new test sweeps a click at clickBounce 5 and fails without the clamp (size -0.0138 px at t = 0.531 s).
CursorTrack::bounce had no test. The new one pins exactly 1.0 outside the 260 ms window, continuity at elapsed fractions 0, 0.38 and 1, the 0.76 trough at +49.4 ms, the 1.16 peak at +179.4 ms, and that a later click restarts the curve. Stale references cleaned up on the way: - the bounce doc comment cited getNativeCursorClickBounceScale in nativeCursor.ts, deleted with pixi.js; Rust is now the only authority and the comment says so; - scripts/inspect-native-cursor-click-bounce.mjs divided clickBounce by 5, so its report was off by up to 5x from what the compositor renders. It now scales the deviation by the raw value and floors at 0 like plan_cursor, and defaults to the app default (2.5); - the types comment claimed ~120 Hz cursor sampling; every recorder samples every 33 ms (CURSOR_SAMPLE_INTERVAL_MS) and no faster path exists.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe compositor now handles extreme cursor bounce without negative sprite sizes. Tests cover the bounce envelope and cursor planning. The inspection script matches compositor defaults and scaling. Cursor asset documentation reflects the removed TypeScript source. ChangesCursor bounce behavior
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The extreme click-bounce case now skips invalid cursor geometry instead of rendering an inverted sprite. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 78.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use 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 |
1 participant
Summary
Negative cursor size.
plan_cursorcomputed1 + (bounce(t) - 1) * clickBouncewith no bound. The press trough ofbounce()is 0.76 and the slider goes up to 5, so above 4.17 the factor went negative, and so did the size every backend drew. With both extents negative the sprite was drawn flipped 180°. The factor is now floored at 0 in the one shared place, and a non-positive size draws nothing. The tilted path would otherwise solve a 0/0 warp. At clickBounce 5 the cursor now disappears for about 37 ms at the bottom of the press.The click envelope had no test.
cursor::tests::click_bounce_envelopenow pins it:A second test sweeps
plan_cursorat clickBounce 5.Stale references:
bounce()citednativeCursor.ts, deleted with pixi.js; Rust is now the only reference for the curve;scripts/inspect-native-cursor-click-bounce.mjsdivided the intensity by 5, so its report was off by up to 5×; it now copies the Rust formula and defaults to 2.5 like the app;cursor.rssaid positions are sampled at ~120 Hz; the recorders sample every 33 ms;cursor.mdandgenerate-default-cursor-sprites.mjspointed at the deleted file.Related issue
None filed.
Type of change
Release impact
Desktop impact
Testing
cargo test -p openscreen-compositor --libgives 206 passed. Removing the clamp makes the newframe_geometrytest fail with a size of -0.0138 (mutation run).nix develop .#compositor-view, pinned ffmpeg SDK, nix lavapipe),cargo test -p openscreen-compositor --lib --testsgives 224 passed.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation