-
Notifications
You must be signed in to change notification settings - Fork 562
perf(InputHandler): optimize mousemove handling to reduce CPU usage #2055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
WalkthroughMouseMoveEvent emissions are throttled to once per animation frame. The mousemove handler now stores the latest event and schedules a requestAnimationFrame if needed. The RAF callback emits a single MouseMoveEvent per frame when movement is detected. No public APIs were changed. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Window
participant InputHandler
participant EventBus
User->>Window: mousemove (many)
Window->>InputHandler: onMouseMove(evt)
Note over InputHandler: Save latest evt as pendingMouseMove<br/>If no frame scheduled -> requestAnimationFrame
rect rgba(200,240,255,0.25)
Window-->>InputHandler: RAF tick
InputHandler->>InputHandler: Take and clear pendingMouseMove
alt movementX or movementY present
InputHandler->>EventBus: emit MouseMoveEvent(clientX, clientY)
else
Note over InputHandler: Skip emit (no movement)
end
end
Note over EventBus,InputHandler: At most one MouseMoveEvent per frame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/InputHandler.ts (2)
196-199
: movementX/movementY can be 0 or undefined on some Safari/trackpad paths — add coord fallbackRelying only on movementX/Y may drop real moves on Safari/iPadOS or very slow motion. Guard with clientX/Y deltas too.
Apply:
@@ - if (evt && (evt.movementX || evt.movementY)) { - this.eventBus.emit(new MouseMoveEvent(evt.clientX, evt.clientY)); - } + if ( + evt && + ( + evt.movementX || evt.movementY || + evt.clientX !== lastMouseX || evt.clientY !== lastMouseY + ) + ) { + lastMouseX = evt.clientX; + lastMouseY = evt.clientY; + this.eventBus.emit(new MouseMoveEvent(evt.clientX, evt.clientY)); + }Plus, define these near the other locals:
@@ - let pendingMouseMove: MouseEvent | null = null; - let mouseMoveFrame = false; + let pendingMouseMove: MouseEvent | null = null; + let mouseMoveFrame = false; + let lastMouseX = 0; + let lastMouseY = 0;
186-187
: Make the handler and rAF cancelable; avoid leaks on re‑init/hot‑reloadThe inline listener cannot be removed in destroy(); the scheduled rAF cannot be canceled either. Store both on the instance and clean up in destroy().
Example (outline):
// fields private mouseMoveRAF: number | null = null; private mouseMoveHandler?: (e: MouseEvent) => void; // in initialize() this.mouseMoveHandler = (e) => { /* same logic, but use this.mouseMoveRAF instead of a boolean */ }; window.addEventListener("mousemove", this.mouseMoveHandler); // in destroy() if (this.mouseMoveHandler) window.removeEventListener("mousemove", this.mouseMoveHandler); if (this.mouseMoveRAF != null) cancelAnimationFrame(this.mouseMoveRAF);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/InputHandler.ts
(1 hunks)
🔇 Additional comments (1)
src/client/InputHandler.ts (1)
189-195
: rAF throttle looks goodBatches to one emit per frame; this should cut CPU spikes from hot mousemove paths.
Please sanity‑check:
- 30 fps caps (VSync off / battery saver) to catch perceived lag.
- Pointer lock mode (if used anywhere).
Do you have a perf profile to show how much cpu is saved? |
Description:
This PR optimizes the mousemove event handler in
InputHandler.ts
by throttling it with
requestAnimationFrame
to reduce CPU usage.No UI or gameplay logic is changed.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
GOC