Cap StructureLayer Maximum Texture Size#3574
Conversation
WalkthroughChanged Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/StructureLayer.ts`:
- Around line 137-144: After resizing the canvas you must reapply the 2D context
smoothing settings and compute the transform from the actual integer canvas
dimensions (not the pre-truncated fractional bufferScale). Change the resize
logic around bufferScale, this.canvas.width/height and this.context.scale so
that you: compute bufferScale = Math.min(2, maxTextureSize / this.game.width(),
maxTextureSize / this.game.height()), set integerWidth =
Math.round(this.game.width() * bufferScale) and integerHeight =
Math.round(this.game.height() * bufferScale), assign those to
this.canvas.width/height, reapply this.context.imageSmoothingEnabled and
this.context.imageSmoothingQuality, then derive the final scale as actualScaleX
= integerWidth / this.game.width() and actualScaleY = integerHeight /
this.game.height() and call this.context.scale(actualScaleX, actualScaleY) (or
setTransform) instead of using the original fractional bufferScale so the
transform matches the real canvas size and smoothing is preserved.
🪄 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: 29ff6a5e-15aa-4064-a901-253f9839c420
📒 Files selected for processing (1)
src/client/graphics/layers/StructureLayer.ts
|
@EnderBoy9217 Hi, if you'd like to join the Dev server on Discord, there's an invite link in the readme here: EnderBoy9217 |
Thanks for letting me know! |
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureLayer.ts (1)
134-135: Remove redundant smoothing settings before resize.These lines set smoothing, but lines 142-149 resize the canvas, which resets all context state. Lines 150-151 already reapply smoothing after the resize. You can safely remove lines 134-135.
♻️ Proposed cleanup
this.context = context; - // Enable smooth scaling - this.context.imageSmoothingEnabled = true; - this.context.imageSmoothingQuality = "high"; - // Firefox's GPU limit is 8192, only known browser issue const maxTextureSize = 8192;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureLayer.ts` around lines 134 - 135, Remove the redundant pre-resize smoothing settings in StructureLayer: delete the calls that set this.context.imageSmoothingEnabled and this.context.imageSmoothingQuality before the canvas resize since the resize resets the canvas context state and the code already re-applies smoothing after the resize; keep only the post-resize settings (the later this.context.imageSmoothingEnabled / this.context.imageSmoothingQuality lines) to avoid duplicate/unused state changes.
🤖 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/StructureLayer.ts`:
- Around line 134-135: Remove the redundant pre-resize smoothing settings in
StructureLayer: delete the calls that set this.context.imageSmoothingEnabled and
this.context.imageSmoothingQuality before the canvas resize since the resize
resets the canvas context state and the code already re-applies smoothing after
the resize; keep only the post-resize settings (the later
this.context.imageSmoothingEnabled / this.context.imageSmoothingQuality lines)
to avoid duplicate/unused state changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 813b6c50-ede7-44cf-835a-762642a49b6a
📒 Files selected for processing (1)
src/client/graphics/layers/StructureLayer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureLayer.ts (1)
133-153: Implementation looks correct and addresses the Firefox texture limit issue.The math checks out:
- For large maps (e.g., 10000×10000),
targetScaleclamps to ~0.82, resulting in 8192×8192 canvas- The
context.scale()correctly compensates so drawing code using* 2coordinates still maps properly- For smaller maps, full 2× backing is preserved (
targetScale = 2,scale(1, 1))Past review concern is now fixed: smoothing settings are applied after the canvas resize (lines 148-149), so they won't be reset.
Minor clarity suggestion: The
/ (this.game.width() * 2)in the scale calculation might confuse future readers. A brief comment about why* 2appears everywhere (the drawing code's coordinate system) would help.♻️ Optional: Add explanatory comment
// Enable smooth scaling this.context.imageSmoothingEnabled = true; this.context.imageSmoothingQuality = "high"; + // Drawing code uses 2x game coordinates (e.g., cell.x * 2). + // Scale transform maps those coordinates to actual canvas pixels. this.context.scale( this.canvas.width / (this.game.width() * 2), this.canvas.height / (this.game.height() * 2), );,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureLayer.ts` around lines 133 - 153, Add a short explanatory comment in StructureLayer near the canvas scaling logic (around the targetScale calculation and this.context.scale call) that clarifies why a 2× multiplier appears in drawing coordinates and therefore why the scale uses "/ (this.game.width() * 2)" and "* 2" elsewhere; keep it concise and mention that the drawing code uses a 2× backing coordinate system so the scale compensates for that factor, helping future readers understand targetScale, this.canvas.width/height, and this.context.scale relationships.
🤖 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/StructureLayer.ts`:
- Around line 133-153: Add a short explanatory comment in StructureLayer near
the canvas scaling logic (around the targetScale calculation and
this.context.scale call) that clarifies why a 2× multiplier appears in drawing
coordinates and therefore why the scale uses "/ (this.game.width() * 2)" and "*
2" elsewhere; keep it concise and mention that the drawing code uses a 2×
backing coordinate system so the scale compensates for that factor, helping
future readers understand targetScale, this.canvas.width/height, and
this.context.scale relationships.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c3d6077-d873-4d21-8d1c-215637894c2c
📒 Files selected for processing (1)
src/client/graphics/layers/StructureLayer.ts
Resolves #3569
Description:
While browsers like Firefox will report their maximum texture size of 16384, going over 8192 causes extreme VRAM usage and massive FPS drops, even when no structures are actually being rendered (I experienced ~60ms rendering time on this layer with no structures present).
This PR sets the StructureLayer texture size to cap at 8192, while keeping near-exact scales. The result is increased performance, reduced VRAM Usage, (especially in larger maps), and the resolution of the unplayable performance issues when StructureLayer is present, with zero noticeable degradation.
VRAM Usage also no longer rises when zoomed in (Sitting at around a constant 40MB no matter zoom level, previously it would rise to over 160MB when StructureLayer was present).
All tested on Giant World, where the issues were first spotted, but applies to all maps.
Discord: @EnderBoy9217