-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Bump up the target rate to 120fps. #6868
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
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
API Changes Check Passed Great! The PR description now includes the required "### API changes" section. This helps reviewers and SDK users understand the impact of your changes. |
🤖 Claude Code Review SummaryThis PR increases the target FPS from 60 to 120 and adds dynamic FPS throttling based on presence mode ('solo' vs collaborative). The browser will naturally throttle via requestAnimationFrame based on display capabilities. Code Quality ✅Strengths:
Areas for Improvement: 1. Logic Issue: Duplicate Timing Checks
|
Browser will throttle us on screens that can suport it (raf won't run more often than the device supports).
ebae44a
to
5eae162
Compare
We double checked whether they should be called.
🤖 Claude Code Review SummaryThis PR increases the target frame rate from 60fps to 120fps and adds custom FPS throttling capabilities to fpsThrottle(). The changes allow high-refresh-rate devices to benefit from smoother updates while maintaining backwards compatibility. Code Quality and Best PracticesPositive Aspects
Issues and Concerns1. Logic Bug: Premature timing update The custom FPS timing is updated BEFORE the function is queued, not when it actually runs. If the function gets queued but doesn't execute immediately, the timing will be inaccurate. The next call could be rejected even though the function hasn't actually run yet. Recommendation: Update the timestamp AFTER the function executes, within the flush() function. 2. Mixed throttling strategies
Functions with custom FPS are checked twice. Is this intentional? A function with getTargetFps() = 1 could still run up to 120 times per second if called frequently enough. 3. Performance consideration Test CoverageNo tests found for the fpsThrottle function or these changes. Recommendations:
ConclusionThe change addresses a real UX improvement for high-refresh-rate displays. However, there is a timing logic bug that should be fixed before merging. The implementation also introduces complexity with dual throttling that deserves clearer documentation. Recommendation: Request changes to fix timing update logic and add test coverage. |
🤖 Claude Code Review SummaryThis PR increases the target FPS from 60 to 120 and adds custom per-function FPS throttling capabilities. The changes are well-structured and the implementation appears sound for supporting high refresh rate displays. Code Quality ✅Strengths:
Minor concerns:
Performance Considerations
|
53216b8
to
ac6ba44
Compare
🤖 Claude Code Review SummaryThis PR bumps the target frame rate from 60fps to 120fps and adds adaptive throttling for network sync operations. The changes are well-structured, but there are a few concerns to address. Code Quality & Architecture✅ Strengths
|
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.
nice! i'm down with this change.
- nit: can we also pull the change from #6470 that updates
DefaultDebugPanel.tsx
? - does this work as expected on iOS?
|
||
/** Get the target FPS for network operations based on presence mode */ | ||
private getSyncFps(): number { | ||
return this.presenceMode?.get() === 'solo' ? SOLO_MODE_FPS : COLLABORATIVE_MODE_FPS |
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.
nice, much clearer than my version!
packages/utils/src/lib/throttle.ts
Outdated
const targetFps = 60 | ||
const targetTimePerFrame = Math.floor(1000 / targetFps) * 0.9 // ~15ms - we allow for some variance as browsers aren't that precise. | ||
const targetFps = 120 | ||
const targetTimePerFrame = Math.floor(1000 / targetFps) * 0.9 // ~7ms - we allow for some variance as browsers aren't that precise. |
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.
actually, is the 0.9 still necessary? i'm forgetting a bit on why this was needed... let's expand the comment if we still need this, we're gonna forget again in the future why we did this 🙃
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.
Added a comment here.
|
awesome, thanks for the info. actually to be crisper with my question: does this cause any issues on iOS with the browser trying to go too fast than it can go? (less of a question of whether it can go to 120hz but is it "overclocking" and causing it to freak out? :P) |
I didn't see any issues while testing 🤷♂️ I guess it would work pretty much the same as it did till now: we were setting the target to 60 but some actions were too slow so the fps dropped. Now we set a higher default so more actions will fall into that category. But from my tests the rendering stabilizes at the same fps as before. With the default being 60fps in Safari I'd expect many people on iOS won't even know the difference. Would be great if we could play with some older devices, though. Maybe some old androids with high refresh screens. |
Browser will throttle us on screens that can't support 120hz (raf won't run more often than the device supports).
Change type
improvement
Release notes
API changes
fpsThrottle
no accepts an optional callback which returns the target fps for the throttled function to run at. If no function is provided we will use the default fps of 120.