webgl perf#4138
Draft
evanpelle wants to merge 4 commits into
Draft
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
WalkthroughAdds getEffectiveDpr() (clamped DPR) and replaces direct devicePixelRatio uses across camera, renderer, radial menu, and client runner. Instruments CPU frame timing in the RAF loop and adds deferred GPU timing via EXT_disjoint_timer_query_webgl2 with a fixed-depth query ring. ChangesPerformance timing & DPR changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/render/gl/Renderer.ts`:
- Around line 161-169: dispose() currently doesn't free the WebGLQuery objects
stored for GPU timing (gpuQueries / gpuQueryIssued / gpuQueryIdx / gpuTimerExt),
causing a resource leak; update the Renderer.dispose() method to loop over
this.gpuQueries (up to Renderer.GPU_QUERY_DEPTH), call
this.gl.deleteQuery(query) for each non-null entry, clear/reset the arrays
(gpuQueries = [], gpuQueryIssued = [], gpuQueryIdx = 0) and set gpuTimerExt =
null so all GPU timer resources are released.
🪄 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: eef21adc-8b38-4365-91fd-95be92332c86
📒 Files selected for processing (2)
src/client/ClientGameRunner.tssrc/client/render/gl/Renderer.ts
Comment on lines
+161
to
+169
| // Deferred GPU timer | ||
| private gpuTimerExt: { | ||
| TIME_ELAPSED_EXT: number; | ||
| GPU_DISJOINT_EXT: number; | ||
| } | null = null; | ||
| private gpuQueries: (WebGLQuery | null)[] = []; | ||
| private gpuQueryIssued: boolean[] = []; | ||
| private gpuQueryIdx = 0; | ||
| private static readonly GPU_QUERY_DEPTH = 3; |
Contributor
There was a problem hiding this comment.
GPU query resources need cleanup in dispose().
The query objects allocated at line 226 should be deleted when the renderer is disposed. Currently, dispose() (line 1370) doesn't call gl.deleteQuery() for the timer queries, creating a small resource leak.
♻️ Add cleanup for GPU timer queries
In the dispose() method around line 1410, add:
this.gl.deleteFramebuffer(this.sceneTarget.fbo);
this.gl.deleteTexture(this.sceneTarget.tex);
+ for (const q of this.gpuQueries) {
+ if (q) this.gl.deleteQuery(q);
+ }
this.lastUnits = new Map();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/render/gl/Renderer.ts` around lines 161 - 169, dispose() currently
doesn't free the WebGLQuery objects stored for GPU timing (gpuQueries /
gpuQueryIssued / gpuQueryIdx / gpuTimerExt), causing a resource leak; update the
Renderer.dispose() method to loop over this.gpuQueries (up to
Renderer.GPU_QUERY_DEPTH), call this.gl.deleteQuery(query) for each non-null
entry, clear/reset the arrays (gpuQueries = [], gpuQueryIssued = [], gpuQueryIdx
= 0) and set gpuTimerExt = null so all GPU timer resources are released.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/render/gl/passes/RadialMenuPass.ts (1)
441-445:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive radial-menu scale from the actual drawing buffer.
This pass mixes
gl.drawingBufferWidth/Heightwith a freshgetEffectiveDpr()forax/ay,outerR, andiconHalf. Because the canvas size is rounded, those numbers can disagree even when DPR is stable, so the wheel can render slightly offset from its hit-test anchor. Compute the scale from the real buffer-to-CSS ratio instead of rereading DPR here.♻️ Keep the menu aligned with the real buffer size
draw(): void { @@ const gl = this.gl; - const dpr = getEffectiveDpr(); const vw = gl.drawingBufferWidth; const vh = gl.drawingBufferHeight; - const ax = this.anchorX * dpr; - const ay = this.anchorY * dpr; + const canvas = gl.canvas as HTMLCanvasElement; + const scaleX = canvas.clientWidth > 0 ? vw / canvas.clientWidth : 1; + const scaleY = canvas.clientHeight > 0 ? vh / canvas.clientHeight : 1; + const scale = Math.min(scaleX, scaleY); + const ax = this.anchorX * scaleX; + const ay = this.anchorY * scaleY; @@ this.drawRing( ax, ay, vw, vh, + scale, p, this.savedItems, -1, @@ this.drawRing( ax, ay, vw, vh, + scale, active, this.items, this.hoveredIndex >= 0 ? this.hoveredIndex : -1, @@ private drawRing( ax: number, ay: number, vw: number, vh: number, + scale: number, cfg: RingConfig, @@ - const dpr = getEffectiveDpr(); const n = items.length; @@ - const outerR = cfg.outerR * dpr; + const outerR = cfg.outerR * scale; @@ - const iconHalf = ih * dpr; + const iconHalf = ih * scale;Also applies to: 495-503
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/render/gl/passes/RadialMenuPass.ts` around lines 441 - 445, The pass mixes getEffectiveDpr() with gl.drawingBufferWidth/height causing mismatched scale; instead compute a single buffer-to-CSS scale from the real drawing buffer and use it for all geometry and hit-test math. Replace uses of getEffectiveDpr() for ax/ay, outerR, iconHalf (and the same block later at the other occurrence) with a computed scale = gl.drawingBufferWidth / (gl.canvas?.clientWidth || expectedCssWidth) (or use height analog if vertical), then compute ax = anchorX * scale, ay = anchorY * scale and multiply outerR and iconHalf by that same scale so the rendered wheel and hit tests align with the actual buffer. Ensure the same scale is reused in both the initial block (where ax/ay are set) and the subsequent block that computes outerR/iconHalf.
♻️ Duplicate comments (1)
src/client/render/gl/Renderer.ts (1)
1371-1411:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDelete the timer queries in
dispose().The constructor allocates
GPU_QUERY_DEPTHWebGLQueryobjects, butdispose()never frees them. RecreatingGPURendererwill leak GL objects across sessions.♻️ Minimal fix
dispose(): void { this.stopLoop(); @@ this.skinAtlas.dispose(); this.gl.deleteFramebuffer(this.sceneTarget.fbo); this.gl.deleteTexture(this.sceneTarget.tex); + for (const query of this.gpuQueries) { + if (query) this.gl.deleteQuery(query); + } + this.gpuQueries = []; + this.gpuQueryIssued = []; + this.gpuQueryIdx = 0; + this.gpuTimerExt = null; this.lastUnits = new Map(); this.lastStructures = new Map(); }#!/bin/bash set -euo pipefail printf '%s\n' '--- query allocation ---' sed -n '222,231p' src/client/render/gl/Renderer.ts printf '\n%s\n' '--- dispose() ---' sed -n '1371,1414p' src/client/render/gl/Renderer.ts printf '\n%s\n' '--- query lifecycle call sites ---' rg -n 'createQuery|deleteQuery' src/client/render/gl/Renderer.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/render/gl/Renderer.ts` around lines 1371 - 1411, The dispose() method currently omits deleting the WebGLQuery objects allocated with GPU_QUERY_DEPTH in the constructor; update dispose() to iterate over the queries allocated at construction and call this.gl.deleteQuery(...) for each (and then clear/null the array) so the GPU_QUERY_DEPTH WebGLQuery objects are freed and not leaked when GPURenderer is recreated. Ensure you reference the same query storage used in the constructor (the array/object holding the allocated queries) and remove/clear those references after calling deleteQuery to avoid dangling pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/render/gl/Camera.ts`:
- Around line 48-50: The camera currently rounds canvasW/canvasH in resize() but
recomputes DPR in screenToWorld() and worldToScreen(), causing half-pixel drift;
modify Camera to store the actual backing-store scale computed in resize()
(e.g., a new property like backingScale or devicePixelRatioUsed) when you set
canvasW/canvasH, and have screenToWorld() and worldToScreen() read and use that
cached scale instead of calling getEffectiveDpr() again (also update the same
logic in the conversion code around the other conversion block at lines ~167-184
so all world↔screen math uses the identical cached scale).
---
Outside diff comments:
In `@src/client/render/gl/passes/RadialMenuPass.ts`:
- Around line 441-445: The pass mixes getEffectiveDpr() with
gl.drawingBufferWidth/height causing mismatched scale; instead compute a single
buffer-to-CSS scale from the real drawing buffer and use it for all geometry and
hit-test math. Replace uses of getEffectiveDpr() for ax/ay, outerR, iconHalf
(and the same block later at the other occurrence) with a computed scale =
gl.drawingBufferWidth / (gl.canvas?.clientWidth || expectedCssWidth) (or use
height analog if vertical), then compute ax = anchorX * scale, ay = anchorY *
scale and multiply outerR and iconHalf by that same scale so the rendered wheel
and hit tests align with the actual buffer. Ensure the same scale is reused in
both the initial block (where ax/ay are set) and the subsequent block that
computes outerR/iconHalf.
---
Duplicate comments:
In `@src/client/render/gl/Renderer.ts`:
- Around line 1371-1411: The dispose() method currently omits deleting the
WebGLQuery objects allocated with GPU_QUERY_DEPTH in the constructor; update
dispose() to iterate over the queries allocated at construction and call
this.gl.deleteQuery(...) for each (and then clear/null the array) so the
GPU_QUERY_DEPTH WebGLQuery objects are freed and not leaked when GPURenderer is
recreated. Ensure you reference the same query storage used in the constructor
(the array/object holding the allocated queries) and remove/clear those
references after calling deleteQuery to avoid dangling pointers.
🪄 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: edb239fe-b3ea-411b-b622-225e4a09227e
📒 Files selected for processing (5)
src/client/ClientGameRunner.tssrc/client/render/gl/Camera.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/passes/RadialMenuPass.tssrc/client/utilities/Dpr.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/ClientGameRunner.ts
Comment on lines
48
to
50
| resize(cssWidth: number, cssHeight: number): void { | ||
| const dpr = window.devicePixelRatio || 1; | ||
| const dpr = getEffectiveDpr(); | ||
| this.canvasW = Math.round(cssWidth * dpr); |
Contributor
There was a problem hiding this comment.
Use the rounded backing-store scale for camera math.
resize() rounds canvasW/canvasH, but the conversion methods recompute a fresh DPR. When cssSize * dpr lands on a half-pixel, those values no longer match, so screenToWorld() and worldToScreen() drift from each other and clicks stop lining up exactly with what was rendered. Cache the actual scale from resize() and reuse it here. As per coding guidelines, Camera.ts should handle world↔screen math.
♻️ One simple way to keep the math exact
export class Camera {
@@
private canvasW = 1;
private canvasH = 1;
+ private scaleX = 1;
+ private scaleY = 1;
@@
resize(cssWidth: number, cssHeight: number): void {
const dpr = getEffectiveDpr();
this.canvasW = Math.round(cssWidth * dpr);
this.canvasH = Math.round(cssHeight * dpr);
+ this.scaleX = cssWidth > 0 ? this.canvasW / cssWidth : 1;
+ this.scaleY = cssHeight > 0 ? this.canvasH / cssHeight : 1;
if (this.needsInitialFit) {
this.fitMap();
}
@@
screenToWorld(screenX: number, screenY: number): { x: number; y: number } {
- const dpr = getEffectiveDpr();
- const ndcX = ((screenX * dpr) / this.canvasW) * 2 - 1;
- const ndcY = -(((screenY * dpr) / this.canvasH) * 2 - 1);
+ const ndcX = ((screenX * this.scaleX) / this.canvasW) * 2 - 1;
+ const ndcY = -(((screenY * this.scaleY) / this.canvasH) * 2 - 1);
@@
worldToScreen(worldX: number, worldY: number): { x: number; y: number } {
- const dpr = getEffectiveDpr();
return {
- x: (this.zoom * (worldX - this.offsetX)) / dpr + this.canvasW / (2 * dpr),
- y: (this.zoom * (worldY - this.offsetY)) / dpr + this.canvasH / (2 * dpr),
+ x:
+ (this.zoom * (worldX - this.offsetX)) / this.scaleX +
+ this.canvasW / (2 * this.scaleX),
+ y:
+ (this.zoom * (worldY - this.offsetY)) / this.scaleY +
+ this.canvasH / (2 * this.scaleY),
};
}Also applies to: 167-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/render/gl/Camera.ts` around lines 48 - 50, The camera currently
rounds canvasW/canvasH in resize() but recomputes DPR in screenToWorld() and
worldToScreen(), causing half-pixel drift; modify Camera to store the actual
backing-store scale computed in resize() (e.g., a new property like backingScale
or devicePixelRatioUsed) when you set canvasW/canvasH, and have screenToWorld()
and worldToScreen() read and use that cached scale instead of calling
getEffectiveDpr() again (also update the same logic in the conversion code
around the other conversion block at lines ~167-184 so all world↔screen math
uses the identical cached scale).
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
1 participant
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.
Add approved & assigned issue number here:
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME