## Fix: Add FPS limit to prevent GPU overload and watchdog crashes on large maps#3462
Open
samisbakedham wants to merge 4 commits intoopenfrontio:mainfrom
Open
## Fix: Add FPS limit to prevent GPU overload and watchdog crashes on large maps#3462samisbakedham wants to merge 4 commits intoopenfrontio:mainfrom
samisbakedham wants to merge 4 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
WalkthroughTerritoryLayer.renderLayer now draws only the viewport-visible subregion of the map and highlight canvases. It computes the screen bounding rect, clamps it to map bounds, derives source width/height, and skips drawing when the clipped region has zero area. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TerritoryLayer
participant TransformHandler
participant MapCanvas
participant HighlightCanvas
Client->>TerritoryLayer: request renderLayer()
TerritoryLayer->>TransformHandler: screenBoundingRect()
TransformHandler-->>TerritoryLayer: visibleRect (screen coords)
TerritoryLayer->>MapCanvas: compute/clamp source rect (sx, sy, cw, ch)
TerritoryLayer->>HighlightCanvas: compute/clamp highlight source rect
alt cw > 0 and ch > 0
TerritoryLayer->>MapCanvas: drawImage(source sx,sy,cw,ch -> dest)
TerritoryLayer->>HighlightCanvas: drawImage(highlight source -> dest)
else
TerritoryLayer-->>Client: skip drawing (no visible area)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (5 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
Collaborator
|
@scamiv mind reviewing this? |
scamiv
reviewed
Mar 20, 2026
Contributor
There was a problem hiding this comment.
The blit is almost free on Chrome based browsers, could be a improvement on Firefox.
may also lead to more re-computation or artifacting when zooming fast (since the browser actually has to wait for the next raf).
Would need to do proper benchmark/testing to vouch for this.
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
VariableVince
added
will not stale
Contributor
|
This needs testing and benchmarking like @scamiv said so may take a while still. Put label 'will not stale' to give it more time; any fixes to performance and crashes are very welcome of course (personally i get the focus on monetization but.. having more players actually be able to play the game without extensive troubleshooting, and without the player just tossing the game aside, is also important to keep the game afloat. So bugfixes yay, features can wait i'd say but who am i) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/TerritoryLayer.ts (1)
506-517: Consider one helper for the clipped blit.The territory and highlight paths now share the same source/destination math. A tiny helper would keep future fixes to the clip math in one place.
Refactor sketch
+ private drawVisibleCanvas( + context: CanvasRenderingContext2D, + canvas: HTMLCanvasElement, + x: number, + y: number, + width: number, + height: number, + ) { + if (width <= 0 || height <= 0) return; + context.drawImage( + canvas, + x, + y, + width, + height, + x - this.game.width() / 2, + y - this.game.height() / 2, + width, + height, + ); + } + - if (cw > 0 && ch > 0) { - context.drawImage( - this.canvas, - cx0, - cy0, - cw, - ch, - cx0 - this.game.width() / 2, - cy0 - this.game.height() / 2, - cw, - ch, - ); - } + this.drawVisibleCanvas(context, this.canvas, cx0, cy0, cw, ch); ... - if (cw > 0 && ch > 0) { - context.drawImage( - this.highlightCanvas, - cx0, - cy0, - cw, - ch, - cx0 - this.game.width() / 2, - cy0 - this.game.height() / 2, - cw, - ch, - ); - } + this.drawVisibleCanvas(context, this.highlightCanvas, cx0, cy0, cw, ch);Also applies to: 522-533
🤖 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 506 - 517, The clipped drawImage math is duplicated for territory and highlight rendering; extract a small helper (e.g., blitClipped or drawClippedFromCanvas) inside TerritoryLayer that takes the rendering context, source canvas (this.canvas), source rect (cx0, cy0, cw, ch) and destination offsets (use this.game.width()/2 and this.game.height()/2 to compute destX/destY) and call context.drawImage once; replace both occurrences (the block using this.canvas and the later similar block) to call this helper so all clip/source->dest math is centralized.
🤖 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/layers/TerritoryLayer.ts`:
- Around line 499-505: The clip rect is being treated as inclusive even though
this.transformHandler.screenBoundingRect() returns an exclusive bottom-right;
update the clamping and size math to keep it exclusive by removing the -1
offsets and the +1 in width/height calculation: clamp cx1/cy1 with
Math.min(this.game.width(), bottomRight.x) and Math.min(this.game.height(),
bottomRight.y) (or equivalent), then compute cw = cx1 - cx0 and ch = cy1 - cy0;
change references in TerritoryLayer.ts around screenBoundingRect(), cx0/cy0,
cx1/cy1, and cw/ch accordingly.
---
Nitpick comments:
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 506-517: The clipped drawImage math is duplicated for territory
and highlight rendering; extract a small helper (e.g., blitClipped or
drawClippedFromCanvas) inside TerritoryLayer that takes the rendering context,
source canvas (this.canvas), source rect (cx0, cy0, cw, ch) and destination
offsets (use this.game.width()/2 and this.game.height()/2 to compute
destX/destY) and call context.drawImage once; replace both occurrences (the
block using this.canvas and the later similar block) to call this helper so all
clip/source->dest math is centralized.
🪄 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: b41b8211-852b-4779-8d32-a052050d663a
📒 Files selected for processing (1)
src/client/graphics/layers/TerritoryLayer.ts
Comment on lines
+499
to
+505
| const [topLeft, bottomRight] = this.transformHandler.screenBoundingRect(); | ||
| const cx0 = Math.max(0, topLeft.x); | ||
| const cy0 = Math.max(0, topLeft.y); | ||
| const cx1 = Math.min(this.game.width() - 1, bottomRight.x); | ||
| const cy1 = Math.min(this.game.height() - 1, bottomRight.y); | ||
| const cw = cx1 - cx0 + 1; | ||
| const ch = cy1 - cy0 + 1; |
Contributor
There was a problem hiding this comment.
Make the clip rect exclusive.
screenBoundingRect() already returns an exclusive bottom-right, so the - 1 clamp plus + 1 width/height keeps one extra tile strip in the hot path every frame. That weakens the win on large maps.
Suggested fix
- const cx1 = Math.min(this.game.width() - 1, bottomRight.x);
- const cy1 = Math.min(this.game.height() - 1, bottomRight.y);
- const cw = cx1 - cx0 + 1;
- const ch = cy1 - cy0 + 1;
+ const cx1 = Math.min(this.game.width(), bottomRight.x);
+ const cy1 = Math.min(this.game.height(), bottomRight.y);
+ const cw = Math.max(0, cx1 - cx0);
+ const ch = Math.max(0, cy1 - cy0);🤖 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 499 - 505, The
clip rect is being treated as inclusive even though
this.transformHandler.screenBoundingRect() returns an exclusive bottom-right;
update the clamping and size math to keep it exclusive by removing the -1
offsets and the +1 in width/height calculation: clamp cx1/cy1 with
Math.min(this.game.width(), bottomRight.x) and Math.min(this.game.height(),
bottomRight.y) (or equivalent), then compute cw = cx1 - cx0 and ch = cy1 - cy0;
change references in TerritoryLayer.ts around screenBoundingRect(), cx0/cy0,
cx1/cy1, and cw/ch accordingly.
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
6 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