## Fix: Add FPS limit to prevent GPU overload and watchdog crashes on large maps#3462
Open
samisbakedham wants to merge 3 commits intoopenfrontio:mainfrom
Open
## Fix: Add FPS limit to prevent GPU overload and watchdog crashes on large maps#3462samisbakedham wants to merge 3 commits intoopenfrontio:mainfrom
samisbakedham wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
…e maps (openfrontio#3461) The render loop called requestAnimationFrame unconditionally with no rate limiting. On high-refresh-rate displays and powerful GPUs, this could push render throughput to hundreds or thousands of frames per second on large maps like Giant World Map, triggering a Windows TDR (Timeout Detection and Recovery) watchdog reset. - Add configurable FPS limit in UserSettings (default 60, 0 = uncapped) - Throttle renderGame() using timestamp delta check before rendering work - Add FPS Limit slider to Settings UI (30/60/120/144/240/Uncapped) - Add translation keys for fps_limit_label, fps_limit_desc, fps_limit_uncapped Does not affect game logic or tick rate — render loop only.
|
|
Contributor
WalkthroughThe TerritoryLayer rendering now clips output to the visible viewport instead of blitting the entire map to the canvas. The source rectangle is computed from screen bounds, clamped to map limits, and only that visible region is drawn. Guards prevent rendering when the clipped area has zero dimensions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/client/graphics/layers/SettingsModal.ts (1)
190-208: Use one shared FPS step list and clamp slider index on both ends.The FPS step list is duplicated, and index clamping currently has only an upper bound. A shared constant plus
Math.max/Math.minkeeps behavior safe and easier to maintain.♻️ Proposed refactor
+const FPS_LIMIT_STEPS = [30, 60, 120, 144, 240, 0] as const; + private onFpsLimitChange(event: Event) { - const raw = parseInt((event.target as HTMLInputElement).value, 10); - // Slider steps: 30, 60, 120, 144, 240, 0 (uncapped) - const steps = [30, 60, 120, 144, 240, 0]; - const value = steps[Math.min(raw, steps.length - 1)]; + const raw = Number((event.target as HTMLInputElement).value); + const safeIndex = Math.max( + 0, + Math.min( + Number.isFinite(raw) ? Math.trunc(raw) : 1, + FPS_LIMIT_STEPS.length - 1, + ), + ); + const value = FPS_LIMIT_STEPS[safeIndex]; this.userSettings.setFpsLimit(value); this.requestUpdate(); } private fpsLimitSliderIndex(): number { - const steps = [30, 60, 120, 144, 240, 0]; - const idx = steps.indexOf(this.userSettings.fpsLimit()); + const idx = FPS_LIMIT_STEPS.indexOf( + this.userSettings.fpsLimit() as (typeof FPS_LIMIT_STEPS)[number], + ); return idx === -1 ? 1 : idx; // default to 60 (index 1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/SettingsModal.ts` around lines 190 - 208, Extract the repeated FPS steps array into a single shared constant (e.g. FPS_STEPS) and replace the inline arrays in onFpsLimitChange and fpsLimitSliderIndex with that constant; in onFpsLimitChange compute the step index from the parsed input and clamp it to the valid range using Math.max(0, Math.min(rawIndex, FPS_STEPS.length - 1)) before mapping to the FPS value and calling this.userSettings.setFpsLimit(value); in fpsLimitSliderIndex use FPS_STEPS.indexOf(this.userSettings.fpsLimit()) and if index === -1 clamp to a safe default with Math.max(0, Math.min(index, FPS_STEPS.length - 1)) (or return a default index like 1) so both upper and lower bounds are enforced and the step list is maintained in one place.src/core/game/UserSettings.ts (1)
224-226: Normalize FPS values in the setter before storing.
fpsLimit()enforces the contract, butsetFpsLimit()currently persists raw input. Normalizing in the setter keeps stored values stable and avoids odd values leaking into UI state.♻️ Proposed setter normalization
setFpsLimit(value: number): void { - this.setFloat("settings.fpsLimit", value); + if (!Number.isFinite(value)) { + this.setFloat("settings.fpsLimit", 60); + return; + } + if (value === 0) { + this.setFloat("settings.fpsLimit", 0); + return; + } + const normalized = Math.max(10, Math.min(360, Math.round(value))); + this.setFloat("settings.fpsLimit", normalized); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 224 - 226, The setter setFpsLimit currently writes raw input via this.setFloat("settings.fpsLimit", value); change it to normalize the value using the same logic as the getter/validator (fpsLimit) before storing — e.g. compute the normalized/clamped/rounded FPS by calling the existing fpsLimit normalization logic (or reuse the fpsLimit function) and then call this.setFloat("settings.fpsLimit", normalizedValue) so only validated values are persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 405-418: The render loop in GameRenderer.renderGame is calling
this.userSettings.fpsLimit() every frame, causing synchronous localStorage
reads; fix it by introducing a cached property (e.g., this.cachedFpsLimit) used
in the renderGame FPS check instead of calling userSettings.fpsLimit() each
frame, initialize it once when GameRenderer is constructed, and update it via
the existing settings change hook or an observer (subscribe to userSettings
change event and set this.cachedFpsLimit = this.userSettings.fpsLimit()) so
changes still take effect without per-frame storage access.
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 521-550: Prettier formatting is failing CI for the SettingsModal
component; run the project Prettier formatter and save the fixed file (e.g., npx
prettier --write) to reformat src/client/graphics/layers/SettingsModal.ts, then
stage the updated file and commit; no code logic changes are needed—just
reformat the JSX/TSX block around the SettingsModal class and its elements
(fpsLimitSliderIndex, onFpsLimitChange, fpsLimitLabel) so the file passes
Prettier checks.
---
Nitpick comments:
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 190-208: Extract the repeated FPS steps array into a single shared
constant (e.g. FPS_STEPS) and replace the inline arrays in onFpsLimitChange and
fpsLimitSliderIndex with that constant; in onFpsLimitChange compute the step
index from the parsed input and clamp it to the valid range using Math.max(0,
Math.min(rawIndex, FPS_STEPS.length - 1)) before mapping to the FPS value and
calling this.userSettings.setFpsLimit(value); in fpsLimitSliderIndex use
FPS_STEPS.indexOf(this.userSettings.fpsLimit()) and if index === -1 clamp to a
safe default with Math.max(0, Math.min(index, FPS_STEPS.length - 1)) (or return
a default index like 1) so both upper and lower bounds are enforced and the step
list is maintained in one place.
In `@src/core/game/UserSettings.ts`:
- Around line 224-226: The setter setFpsLimit currently writes raw input via
this.setFloat("settings.fpsLimit", value); change it to normalize the value
using the same logic as the getter/validator (fpsLimit) before storing — e.g.
compute the normalized/clamped/rounded FPS by calling the existing fpsLimit
normalization logic (or reuse the fpsLimit function) and then call
this.setFloat("settings.fpsLimit", normalizedValue) so only validated values are
persisted.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38339edd-6953-4e3f-a2f1-1dd25a8c5f7c
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/SettingsModal.tssrc/core/game/UserSettings.ts
Comment on lines
+521
to
+550
| <div | ||
| class="flex gap-3 items-center w-full text-left p-3 hover:bg-slate-700 rounded-sm text-white transition-colors" | ||
| > | ||
| <img | ||
| src=${settingsIcon} | ||
| alt="fpsLimit" | ||
| width="20" | ||
| height="20" | ||
| /> | ||
| <div class="flex-1"> | ||
| <div class="font-medium"> | ||
| ${translateText("user_setting.fps_limit_label")} | ||
| </div> | ||
| <div class="text-sm text-slate-400"> | ||
| ${translateText("user_setting.fps_limit_desc")} | ||
| </div> | ||
| <input | ||
| type="range" | ||
| min="0" | ||
| max="5" | ||
| step="1" | ||
| .value=${this.fpsLimitSliderIndex()} | ||
| @input=${this.onFpsLimitChange} | ||
| class="w-full border border-slate-500 rounded-lg mt-1" | ||
| /> | ||
| </div> | ||
| <div class="text-sm text-slate-400"> | ||
| ${this.fpsLimitLabel()} | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Prettier check is currently blocking CI for this file.
Please run Prettier on src/client/graphics/layers/SettingsModal.ts before merge (npx prettier --write src/client/graphics/layers/SettingsModal.ts).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/SettingsModal.ts` around lines 521 - 550, Prettier
formatting is failing CI for the SettingsModal component; run the project
Prettier formatter and save the fixed file (e.g., npx prettier --write) to
reformat src/client/graphics/layers/SettingsModal.ts, then stage the updated
file and commit; no code logic changes are needed—just reformat the JSX/TSX
block around the SettingsModal class and its elements (fpsLimitSliderIndex,
onFpsLimitChange, fpsLimitLabel) so the file passes Prettier checks.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
|
https://developer.mozilla.org/en-US/docs/Web/API/Window/requestAnimationFrame limiting fps MAY make sense for tdp limited cases, this is the wrong way to do it |
… on large maps (openfrontio#3461)" This reverts commit aa4b37c.
…openfrontio#3461) Previously, renderLayer() called drawImage() on the full map canvas every frame regardless of how much of the map was visible. On large maps like Giant World Map, this meant uploading and drawing a massive texture to the GPU each frame even when only a small viewport region was visible. Clip both the territory and highlight canvas drawImage calls to the visible screen bounding rect (already computed for putImageData). This can reduce GPU work per frame by an order of magnitude when zoomed in, which addresses the sustained GPU load that triggers Windows TDR watchdog resets.
Member
Author
Good call, thanks for catching that. You're right, rAF is already vsync-locked so the timestamp throttle was solving the wrong problem and added nothing meaningful. The actual bottleneck is Should meaningfully reduce per-frame GPU work when zoomed in on large maps. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/TerritoryLayer.ts (1)
496-503: Consider extracting the viewport clip rectangle calculation into one helper.This same math is repeated near Line 468-475 and here. A shared helper would reduce drift risk (especially off-by-one behavior) and keep future tuning simpler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 496 - 503, The viewport clipping rectangle calculation is duplicated; extract it into a single helper (e.g., a private method on TerritoryLayer like computeViewportClip or a utility function) that calls this.transformHandler.screenBoundingRect(), clamps to 0..this.game.width()-1 and 0..this.game.height()-1, and returns {cx0,cy0,cx1,cy1,cw,ch}; replace the duplicated blocks (the one around line 468-475 and the block using topLeft/bottomRight) to call the helper so off-by-one logic is centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 496-503: The viewport clipping rectangle calculation is
duplicated; extract it into a single helper (e.g., a private method on
TerritoryLayer like computeViewportClip or a utility function) that calls
this.transformHandler.screenBoundingRect(), clamps to 0..this.game.width()-1 and
0..this.game.height()-1, and returns {cx0,cy0,cx1,cy1,cw,ch}; replace the
duplicated blocks (the one around line 468-475 and the block using
topLeft/bottomRight) to call the helper so off-by-one logic is centralized and
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: feec720a-2906-453d-a494-93a3d9c19b23
📒 Files selected for processing (1)
src/client/graphics/layers/TerritoryLayer.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: Add FPS limit to prevent GPU overload and watchdog crashes on large maps
Closes #3461
Problem
The render loop called
requestAnimationFrameunconditionally on every frame with no rate limiting. On high-refresh-rate displays and powerful GPUs (e.g. RTX 4060 with a 144Hz+ monitor), this could push render throughput to several hundred or even thousands of frames per second on large maps like Giant World Map.On Windows, sustained GPU overload at this level can trigger a TDR (Timeout Detection and Recovery) event — the driver stops responding, Windows watchdog detects the hang, and the system forces a restart. This is what was reported in #3461.
This is a known Windows behavior: the GPU driver has a default ~2 second timeout, and if the GPU doesn't respond within that window (because it's saturated), the OS kills the driver and reboots.
Fix
Added a configurable FPS cap using a timestamp delta check at the top of
renderGame(). If the time since the last rendered frame is less than the target frame interval, the frame is skipped andrequestAnimationFramereschedules for the next opportunity — keeping GPU load proportional to the cap rather than running unbounded.Default is 60 FPS. Users can adjust via Settings.
Changes
src/core/game/UserSettings.ts— AddedfpsLimit()(default: 60, 0 = uncapped) andsetFpsLimit()src/client/graphics/GameRenderer.ts— AddedlastFrameTimefield and FPS throttle logic at the top ofrenderGame(); wiredUserSettingsinto the renderersrc/client/graphics/layers/SettingsModal.ts— Added FPS Limit slider to Settings UI with steps: 30 / 60 / 120 / 144 / 240 / Uncappedresources/lang/en.json— Addedfps_limit_label,fps_limit_desc,fps_limit_uncappedtranslation keysNotes