Skip to content

Map Layers 🗺️ - #4703

Open
FloPinguin wants to merge 7 commits into
mainfrom
map-layers
Open

Map Layers 🗺️#4703
FloPinguin wants to merge 7 commits into
mainfrom
map-layers

Conversation

@FloPinguin

@FloPinguin FloPinguin commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description:

Adds a map layer system that lets mappers overlay PNG images (forests, little decorations, labels, deserts, huts, springs, waterfalls, ice fields, etc.) between the terrain and territory rendering passes. Each layer is a full-size PNG with transparency, placed on either land or water tiles. Layers can optionally be marked as nukeable, meaning they are permanently erased in the blast radius of any nuke. Also works with water nukes and compact maps!

How it works

A map defines layers in its info.json:

    {
      "name": "World",
      "categories": ["world"],
      "nations": [...],
      "layers": [
        { "id": "forests", "placement": "land", "nukeable": true },
        { "id": "ice", "placement": "water" }
      ]
    }

Each layer needs a matching PNG file (e.g. forests.png) in the map-generator assets folder. The PNG must be the same dimensions as the map's image.png. The placement field controls whether the layer appears on land tiles or water tiles. When nukeable is true, the layer is destroyed whenever a nuke detonates within its radius.

Players can toggle individual layers on/off in the Graphics Settings modal under a new "Map Layers" section. Visibility preferences are saved in graphics overrides and persist across sessions.

The map generator validates layer definitions at build time (id format, placement values, duplicate ids, PNG dimensions) and copies layer PNGs to the output alongside image.png. The gen-maps script also populates the map_layers section in en.json so layer names can be translated via Crowdin.

Comprehensive tests cover layer validation rules in info.json, PNG existence in both map-generator and resources directories, manifest/info.json consistency, and en.json coverage.

Media

Example: Water nukes cause tiles to use different layers after impact

2026-07-24.14-45-29.mp4

Example: Nukeable water layer

2026-07-24.14-46-35.mp4

Graphic settings: Toggle layers

2026-07-24.15-00-06.mp4

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory

Please put your Discord username so you can be contacted if a bug or regression is found:

FloPinguin

@FloPinguin FloPinguin added this to the v33 milestone Jul 24, 2026
@FloPinguin
FloPinguin requested a review from a team as a code owner July 24, 2026 13:35
@FloPinguin FloPinguin added Maps A new map, or adjustments to an existing map itself, its json, etc, Feature labels Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR adds validated map-layer metadata and PNG assets, loads and renders layers between terrain and territory, persists layer visibility settings, and propagates nuke blast-radius tiles so nukeable layers show destroyed tiles.

Changes

Map layers

Layer / File(s) Summary
Asset validation and generation
map-generator/..., resources/lang/en.json, src/core/game/Maps.gen.ts, tests/MapConsistency.test.ts, tests/MapLayers.test.ts, tests/util/layerValidation.ts
Layer metadata, identifiers, placements, PNG assets, generated map data, locale entries, and consistency checks are added.
Map layer loading
src/core/game/..., tests/perf/fullgame/NodeGameMapLoader.ts
Map loaders expose layerPng, validate manifests, preload images, resize Compact-map layers, and return layer data.
WebGL map-layer rendering
src/client/render/gl/...
Layer passes render images with terrain data, visibility, destroyed masks, context restoration, and cleanup.
Visibility controls
src/client/hud/..., src/client/ClientGameRunner.ts
Graphics settings persist layer visibility and connect toggles to the renderer.
Nuke-impact propagation
src/core/..., src/client/view/GameView.ts, src/client/WebGLFrameBuilder.ts, tests/core/executions/NukeExecution.test.ts
Nuke impacts are queued, packed into updates, transferred to the client, and applied to nukeable layers.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NukeExecution
  participant GameImpl
  participant GameRunner
  participant Worker
  participant GameView
  participant WebGLFrameBuilder
  participant MapRenderer
  NukeExecution->>GameImpl: queueNukeImpact(tile)
  GameRunner->>GameImpl: drainNukeImpacts()
  GameRunner->>Worker: send packedNukeImpacts
  Worker->>GameView: deliver update batch
  GameView->>WebGLFrameBuilder: expose recentlyNukedTiles()
  WebGLFrameBuilder->>MapRenderer: mark nukeable layer tiles destroyed
Loading

Possibly related PRs

Suggested reviewers: evanpelle

Poem

Layers rise where terrain lies,
Land and water, bright disguise.
Nukes leave marks in pixels small,
Toggles hide or show them all.
PNGs pass each map gate.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is related, but it is too generic to describe the main change clearly. Rename it to mention the map layer feature, such as adding map layers to rendering and map generation.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description matches the changeset and covers the new map layer system, validation, rendering, persistence, and tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (3)
src/client/WebGLFrameBuilder.ts (1)

294-310: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Double loop drives many individual per-tile GPU uploads for large nuke blasts.

syncNukeImpacts calls markLayerTileDestroyed once per (nukeable layer, nuked tile) pair. Each call rebinds the destroyed-mask texture and re-uploads a single texel (see MapLayerPass.markTileDestroyed), so a big blast radius across several nukeable layers turns into a large burst of individual WebGL calls on one frame.

🤖 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/WebGLFrameBuilder.ts` around lines 294 - 310, Update
syncNukeImpacts and the underlying MapLayerPass.markTileDestroyed path to batch
destroyed-mask updates per nukeable layer, avoiding one texture rebind and
single-texel upload for every nuked tile. Preserve the existing
full-blast-radius behavior while issuing consolidated GPU uploads for each
layer.
src/client/render/gl/Renderer.ts (1)

1406-1411: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

findIndex scan in the nuke-destruction hot path.

markLayerTileDestroyed re-scans storedLayers with findIndex on every call; WebGLFrameBuilder.syncNukeImpacts calls this once per (layer, tile) pair for the whole blast radius. Shares one root cause with MapLayerPass.markTileDestroyed's per-call texture rebind — see consolidated comment.

🤖 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 1406 - 1411, Optimize
markLayerTileDestroyed by avoiding the storedLayers.findIndex scan on every
destruction call. Reuse an existing layer-to-pass index mapping or maintain one
for direct lookup from layerId, while preserving the current bounds check and
markTileDestroyed(tileIndex) behavior.
src/client/render/gl/passes/MapLayerPass.ts (1)

120-139: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Per-tile bind + pixelStorei on every nuke-destruction call.

markTileDestroyed rebinds destroyedTex and re-sets pixelStorei on every invocation, with no batch entry point for multiple tiles. This is the root cause of a shared perf finding — see consolidated comment covering WebGLFrameBuilder.ts and 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/passes/MapLayerPass.ts` around lines 120 - 139, The
per-tile update path in markTileDestroyed should avoid rebinding destroyedTex
and resetting UNPACK_ALIGNMENT for every destroyed tile. Add or reuse a batch
update flow that binds/configures the texture once, uploads all pending tile
bytes, and expose the necessary coordination through the relevant rendering path
while preserving the existing bounds and duplicate-destruction checks.
🤖 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/hud/layers/GraphicsSettingsModal.ts`:
- Around line 356-365: Update onToggleLayer and every graphics-override write
path, including reset, preset import, and preset application, to use one shared
helper that synchronizes mapLayerVisibility with the WebGL layers. Keep
visibility-only updates out of the generic graphics-change refresh so they do
not rebuild terrain or refresh player colors, while preserving the existing
direct-toggle callback and UI update behavior.

In `@src/client/render/gl/passes/MapLayerPass.ts`:
- Line 1: Batch nuke-destruction updates per layer rather than issuing one GPU
update per tile. Add a batch method beside MapLayerPass.markTileDestroyed that
binds destroyedTex and configures pixelStorei once, then uploads every provided
index; update Renderer.markLayerTileDestroyed to resolve the layer pass once and
forward the full index list; and change WebGLFrameBuilder.syncNukeImpacts to
collect matching tile indices per nukeable layer before making one batch call.
- Around line 174-201: Cache the uniform locations for uLayerTex, uTerrainBytes,
and uDestroyedMask in the MapLayerPass constructor alongside uCamera,
uPlacement, uNukeable, and uVisible, storing them on the pass instance. Update
draw() to reuse those cached locations and remove the per-frame
gl.getUniformLocation calls.

In `@src/client/render/gl/Renderer.ts`:
- Around line 1364-1422: Replace the parallel mapLayerPasses array with a Map
keyed by layer id, updating setMapLayers, setLayerVisible,
markLayerTileDestroyed, and setLayerDestroyedMask to retrieve passes directly by
id and safely no-op when absent. Update draw and dispose to iterate
mapLayerPasses.values(), and clear the map after disposal so missing layer
images cannot shift pass associations.

In `@src/core/execution/NukeExecution.ts`:
- Around line 372-374: Add tests covering nuke-impact propagation for both land
and water nukes, using the repository’s setup() helper from tests/util/Setup.ts.
Verify drainNukeImpacts() returns all queued tiles after each impact and returns
an empty result on the subsequent drain, exercising the queueNukeImpact call in
NukeExecution.

In `@src/core/game/TerrainMapLoader.ts`:
- Around line 134-150: Update the layer-loading flow in TerrainMapLoader to
downsample each layer PNG to manifest.map4x dimensions when mapSize is
GameMapSize.Compact before uploading it. Pass the resulting image dimensions
into MapLayerPass and its shader so texture sampling uses the scaled texel space
while preserving full-resolution behavior for other map sizes.

In `@tests/MapLayers.test.ts`:
- Around line 61-95: Replace the locally defined VALID_ID_RE, reservedIds, and
validPlacements assertions in “Layer validation rules” with tests that invoke
the production layer validator or map generator. Assert the actual validation
results or errors for valid IDs, invalid characters, empty IDs, reserved
“image”, and invalid placement “air”, rather than testing duplicated local
rules.

---

Nitpick comments:
In `@src/client/render/gl/passes/MapLayerPass.ts`:
- Around line 120-139: The per-tile update path in markTileDestroyed should
avoid rebinding destroyedTex and resetting UNPACK_ALIGNMENT for every destroyed
tile. Add or reuse a batch update flow that binds/configures the texture once,
uploads all pending tile bytes, and expose the necessary coordination through
the relevant rendering path while preserving the existing bounds and
duplicate-destruction checks.

In `@src/client/render/gl/Renderer.ts`:
- Around line 1406-1411: Optimize markLayerTileDestroyed by avoiding the
storedLayers.findIndex scan on every destruction call. Reuse an existing
layer-to-pass index mapping or maintain one for direct lookup from layerId,
while preserving the current bounds check and markTileDestroyed(tileIndex)
behavior.

In `@src/client/WebGLFrameBuilder.ts`:
- Around line 294-310: Update syncNukeImpacts and the underlying
MapLayerPass.markTileDestroyed path to batch destroyed-mask updates per nukeable
layer, avoiding one texture rebind and single-texel upload for every nuked tile.
Preserve the existing full-blast-radius behavior while issuing consolidated GPU
uploads for each layer.
🪄 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 Plus

Run ID: dde52b81-a615-4305-8c3b-10cb4af7010f

📥 Commits

Reviewing files that changed from the base of the PR and between 462420b and 4d70868.

⛔ Files ignored due to path filters (2)
  • src/client/render/gl/shaders/map-layer/layer.frag.glsl is excluded by !**/*.glsl
  • src/client/render/gl/shaders/map-layer/layer.vert.glsl is excluded by !**/*.glsl
📒 Files selected for processing (26)
  • map-generator/codegen.go
  • map-generator/main.go
  • resources/lang/en.json
  • src/client/ClientGameRunner.ts
  • src/client/WebGLFrameBuilder.ts
  • src/client/hud/GameRenderer.ts
  • src/client/hud/layers/GraphicsSettingsModal.ts
  • src/client/render/gl/GraphicsOverrides.ts
  • src/client/render/gl/MapRenderer.ts
  • src/client/render/gl/Renderer.ts
  • src/client/render/gl/passes/MapLayerPass.ts
  • src/client/view/GameView.ts
  • src/core/GameRunner.ts
  • src/core/execution/NukeExecution.ts
  • src/core/game/BinaryLoaderGameMapLoader.ts
  • src/core/game/FetchGameMapLoader.ts
  • src/core/game/Game.ts
  • src/core/game/GameImpl.ts
  • src/core/game/GameMapLoader.ts
  • src/core/game/GameUpdates.ts
  • src/core/game/Maps.gen.ts
  • src/core/game/TerrainMapLoader.ts
  • src/core/worker/Worker.worker.ts
  • tests/MapConsistency.test.ts
  • tests/MapLayers.test.ts
  • tests/perf/fullgame/NodeGameMapLoader.ts

Comment thread src/client/hud/layers/GraphicsSettingsModal.ts
Comment thread src/client/render/gl/passes/MapLayerPass.ts
Comment thread src/client/render/gl/passes/MapLayerPass.ts
Comment thread src/client/render/gl/Renderer.ts
Comment thread src/core/execution/NukeExecution.ts
Comment thread src/core/game/TerrainMapLoader.ts Outdated
Comment thread tests/MapLayers.test.ts
@github-project-automation github-project-automation Bot moved this from Triage to Development in OpenFront Release Management Jul 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/MapConsistency.test.ts (1)

595-613: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fail when the map_layers section is absent.

At Line 600, the early return lets defined layers pass without any translation entries. Use an empty fallback object so Lines 612-618 report every missing layer name.

Proposed fix
-    if (!mapLayersSection) {
-      // No layers defined anywhere — that's fine.
-      return;
-    }
+    const mapLayers = mapLayersSection ?? {};
...
-        if (mapLayersSection[layer.id] === undefined) {
+        if (mapLayers[layer.id] === undefined) {
🤖 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 `@tests/MapConsistency.test.ts` around lines 595 - 613, Update the
mapLayersSection initialization in the “Layer names exist in en.json map_layers
section” test to use an empty object when enContent.map_layers is absent, and
remove the early return. Preserve the existing layer iteration and missing-entry
error reporting so defined layers are still validated.
🤖 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 `@tests/core/executions/NukeExecution.test.ts`:
- Around line 265-272: Strengthen the assertions around game.drainNukeImpacts()
to verify the complete expected blast set or count, rather than only checking
impacts.length is positive. Include the target tile and all queued impact tiles
in the expected result while preserving the existing validity checks for each
returned reference.

In `@tests/util/layerValidation.ts`:
- Around line 28-42: Update the layer validation logic in the layer-validation
routine to validate raw field types before applying ID checks: require layer.id
to be a string before testing emptiness, the reserved value, or VALID_ID_RE, and
reject non-string IDs without allowing regex coercion. When nukeable is present,
require it to be boolean and report invalid values consistently with the
existing errors collection.

---

Outside diff comments:
In `@tests/MapConsistency.test.ts`:
- Around line 595-613: Update the mapLayersSection initialization in the “Layer
names exist in en.json map_layers section” test to use an empty object when
enContent.map_layers is absent, and remove the early return. Preserve the
existing layer iteration and missing-entry error reporting so defined layers are
still validated.
🪄 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 Plus

Run ID: 968c50b1-9d7e-43d3-8742-ecb757cec9fd

📥 Commits

Reviewing files that changed from the base of the PR and between 4d70868 and a70c6c5.

📒 Files selected for processing (10)
  • src/client/WebGLFrameBuilder.ts
  • src/client/hud/layers/GraphicsSettingsModal.ts
  • src/client/render/gl/MapRenderer.ts
  • src/client/render/gl/Renderer.ts
  • src/client/render/gl/passes/MapLayerPass.ts
  • src/core/game/TerrainMapLoader.ts
  • tests/MapConsistency.test.ts
  • tests/MapLayers.test.ts
  • tests/core/executions/NukeExecution.test.ts
  • tests/util/layerValidation.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/MapLayers.test.ts
  • src/client/WebGLFrameBuilder.ts
  • src/client/render/gl/MapRenderer.ts
  • src/client/render/gl/passes/MapLayerPass.ts
  • src/client/hud/layers/GraphicsSettingsModal.ts
  • src/core/game/TerrainMapLoader.ts
  • src/client/render/gl/Renderer.ts

Comment thread tests/core/executions/NukeExecution.test.ts
Comment thread tests/util/layerValidation.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/core/executions/NukeExecution.test.ts`:
- Around line 346-355: Strengthen the assertions in the water-nuke test around
drainNukeImpacts by verifying the deterministic full impact set or count, rather
than only checking that impacts are non-empty and contain the land target. Add
an assertion for a known water tile within the blast radius, while preserving
the existing ref validity checks and empty follow-up drain assertion.
🪄 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 Plus

Run ID: ecc260cb-7a15-48e2-900d-dbd41cb50435

📥 Commits

Reviewing files that changed from the base of the PR and between 437dfa7 and 4d6f5be.

📒 Files selected for processing (1)
  • tests/core/executions/NukeExecution.test.ts

Comment thread tests/core/executions/NukeExecution.test.ts
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 24, 2026

@evanpelle evanpelle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Claude:

Correctness & Risk

  1. Full-map terrain re-upload becomes ~3.4M GL calls (high)

Renderer.applyTerrainDelta now does a 1×1 texSubImage2D plus a fresh new Uint8Array([...]) allocation per ref (Renderer.ts:955-980). That's fine for a nuke burst, but ClientGameRunner.ts:473 calls it on context restore with every tile on the map:

const allRefs = new Array(mapSize); // 2000×1700 = 3.4M for a typical map
view.applyTerrainDelta(allRefs, allTerrain);

TerrainPass has the same per-ref loop (pre-existing), but at least reuses this.pixelScratch. This PR adds a second full pass over it plus 3.4M short-lived allocations — and it runs on every map, including the ones with no layers, since terrainBytesTex is always created.

Suggested fix:
if (this.mapLayerPasses.size === 0) return; // nothing consumes it
if (refs.length === this.mapW * this.mapH) { // full re-upload fast path
// single texSubImage2D of terrainBytes
} else { /* per-ref, reusing a scratch Uint8Array(1) */ }

  1. Layer PNGs are also loaded in the Web Worker (high)

loadTerrainMap is called from both ClientGameRunner (main thread, needs the bitmaps) and GameRunner.createGameRunner (GameRunner.ts:42, runs in Worker.worker.ts, never uses them). The worker will therefore fetch, decode and permanently retain an ImageBitmap per layer — ~13.6 MB for a 2000×1700 layer, held forever in the module-level loadedMaps cache — and block worker startup on it.

Gate image loading behind a parameter (loadLayerImages = false by default) or split it into a separate loadMapLayerImages() that only the client calls.

  1. Layer loading blocks game start (medium)

The await Promise.all(layers.map(...)) sits inside loadTerrainMap before the map data is returned, so multi-MB full-size transparent PNGs are on the critical path for every player on a layered map. Since the renderer already tolerates a missing layer (console.warn + skip), consider loading them off the critical path and calling setMapLayers when they arrive.

  1. Context restore silently drops layer state (medium)

MapRenderer.handleContextRestored re-creates passes via setMapLayers(...), which builds fresh MapLayerPass objects with a zeroed destroyedData and _visible = true. After a context loss:

  • every nuke-destroyed layer tile reappears, and
  • layers the player hid become visible again.

MapLayerPass.restoreFrom() and Renderer/MapRenderer.setLayerDestroyedMask() were clearly written for this but are never called (confirmed by grep across src + tests). Either wire them up (keep the masks + visibility map on MapRenderer, re-apply after restore) or delete them.

…loading, context-restore state, nuke batch upload

- Renderer/RailroadPass/TerrainPass: full-map fast path (single texSubImage2D) + scratch buffer reuse, eliminating ~3.4M per-tile GL calls on context restore
- TerrainMapLoader: add loadImages param (default true); worker passes false to skip layer ImageBitmap loading
- ClientGameRunner: load layer images off the critical path via loadLayerImages(); setMapLayers when ready
- MapRenderer: preserve layer visibility and destroyed masks across context loss; re-apply on restore
- MapLayerPass.markTilesDestroyed: bounding-box batch sub-region upload instead of per-tile 1x1 calls
- WebGLFrameBuilder.syncNukeImpacts: filter nuked tiles by layer placement (water layer skips land tiles)
@FloPinguin

Copy link
Copy Markdown
Contributor Author

I have added a big performance boost to nukeable layers, and addressed the claude review:

Issue 1: Full-map terrain re-upload — ~3.4M GL calls ✅ Fixed

Root cause: Renderer.applyTerrainDelta, RailroadPass.applyTerrainDelta, and TerrainPass.applyTerrainDelta all do per-tile 1×1 texSubImage2D calls. Context restore triggers these with every tile (3.4M for a 2000×1700 map). Renderer also allocated new Uint8Array([...]) per ref; RailroadPass allocated new Uint8Array(1) per iteration.

Changes:

  • Renderer.ts: Added terrainDeltaScratch field. Fast path: single texSubImage2D of the full R8UI buffer when all tiles are being uploaded. Per-tile path reuses scratch.
  • RailroadPass.ts: Added terrainDeltaScratch field. Same full-map fast path + scratch reuse.
  • TerrainPass.ts: Fast path delegates to setTerrainColors() which does a single RGBA texture rebuild + upload instead of 3.4M individual calls.

Issue 2: Layer PNGs loaded in Web Worker ✅ Fixed

Root cause: loadTerrainMap unconditionally loads layer PNG ImageBitmaps. Called from GameRunner.createGameRunner which runs in Worker.worker.ts. The worker never renders layers, so these multi-MB bitmaps are wasted memory held forever in the loadedMaps cache.

Changes:

  • TerrainMapLoader.ts: Added loadImages parameter (default true). When false, skips await Promise.all(layers.map(...)).
  • GameRunner.ts: Passes false — "Worker never renders layers — skip image loading to save memory."

Issue 3: Layer loading blocks game start ✅ Fixed

Root cause: await Promise.all(layers.map(...)) inside loadTerrainMap is on the critical path. Multi-MB PNGs block the game from starting.

Changes:

  • TerrainMapLoader.ts: Exported new loadLayerImages() function for standalone async image loading.
  • ClientGameRunner.ts: Both loadTerrainMap calls now pass loadImages: false. After game setup, if layers exist but images aren't cached, loadLayerImages() is kicked off asynchronously. setMapLayers is called when images arrive. Visibility overrides are applied at that point. Uses graphicsListenerAbort.signal.aborted guard to avoid applying to a stopped game.

Issue 4: Context restore silently drops layer state ✅ Fixed

Root cause: MapRenderer.handleContextRestored calls setMapLayers() which creates fresh MapLayerPass objects with zeroed destroyedData and _visible = true. Nuke-destroyed layer tiles reappear; hidden layers become visible again. MapLayerPass.restoreFrom() and setLayerDestroyedMask() existed but were never called.

@FloPinguin
FloPinguin requested a review from evanpelle July 28, 2026 20:44

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/client/render/gl/passes/RailroadPass.ts (1)

251-280: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Require canonical refs before bulk-uploading bytes.

bytes is documented as parallel to refs, but this path ignores refs and treats bytes[0] as tile (0,0). refs.length === mapW * mapH does not prove refs are unique and ordered, so an unordered full-sized delta corrupts bridge water detection. Only use this path when every refs[i] === i; otherwise retain per-tile uploads.

Proposed fix
-    if (refs.length === this.mapW * this.mapH) {
+    if (
+      refs.length === this.mapW * this.mapH &&
+      refs.every((ref, index) => ref === index)
+    ) {
🤖 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/RailroadPass.ts` around lines 251 - 280, Update
the full-map fast path in the terrain upload logic to require canonical
ordering, not just refs.length === this.mapW * this.mapH: verify every refs[i]
equals i before bulk-uploading bytes. Fall back to the existing per-tile
texSubImage2D loop for unordered or duplicate refs, preserving the current fast
path only for canonical refs.
src/core/game/TerrainMapLoader.ts (2)

71-73: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not cache incompatible loadImages results.

The cache key ignores loadImages. A first loadTerrainMap(..., false) caches layerImages: undefined, so a later default call returns without loading images; the reverse also defeats the caller’s request not to retain images. Cache the image-free terrain base separately and construct image-bearing results per mode.

🤖 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/core/game/TerrainMapLoader.ts` around lines 71 - 73, Update the cache
flow around loadedMaps and loadTerrainMap so cache entries never mix loadImages
modes: store and reuse only the image-free terrain base, then construct
image-bearing results when requested and image-free results when not requested.
Ensure a prior false-mode load does not bypass image loading for a later
true/default call, and a true-mode request does not force images into a
false-mode result.

14-14: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Move ImageBitmap and PNG loading out of TerrainMapLoader.

loadTerrainMap()/loadLayerImages() and ImageBitmap now live in src/core/game; this binds core map loading to the browser image APIs. Keep map metadata/simulation in core, and pass layer image data through an owned typed loader in the client instead of ImageBitmap.

🤖 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/core/game/TerrainMapLoader.ts` at line 14, Remove ImageBitmap and PNG
loading responsibilities from TerrainMapLoader and its
loadTerrainMap()/loadLayerImages() flow. Keep TerrainMapLoader focused on core
terrain metadata and simulation, and introduce or reuse a client-owned typed
loader to provide layer image data without exposing browser ImageBitmap types in
core.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@src/client/render/gl/passes/RailroadPass.ts`:
- Around line 251-280: Update the full-map fast path in the terrain upload logic
to require canonical ordering, not just refs.length === this.mapW * this.mapH:
verify every refs[i] equals i before bulk-uploading bytes. Fall back to the
existing per-tile texSubImage2D loop for unordered or duplicate refs, preserving
the current fast path only for canonical refs.

In `@src/core/game/TerrainMapLoader.ts`:
- Around line 71-73: Update the cache flow around loadedMaps and loadTerrainMap
so cache entries never mix loadImages modes: store and reuse only the image-free
terrain base, then construct image-bearing results when requested and image-free
results when not requested. Ensure a prior false-mode load does not bypass image
loading for a later true/default call, and a true-mode request does not force
images into a false-mode result.
- Line 14: Remove ImageBitmap and PNG loading responsibilities from
TerrainMapLoader and its loadTerrainMap()/loadLayerImages() flow. Keep
TerrainMapLoader focused on core terrain metadata and simulation, and introduce
or reuse a client-owned typed loader to provide layer image data without
exposing browser ImageBitmap types in core.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 334c705a-31f3-492d-9619-e3d0ff115d1b

📥 Commits

Reviewing files that changed from the base of the PR and between df212de and 3669e12.

📒 Files selected for processing (9)
  • src/client/ClientGameRunner.ts
  • src/client/WebGLFrameBuilder.ts
  • src/client/render/gl/MapRenderer.ts
  • src/client/render/gl/Renderer.ts
  • src/client/render/gl/passes/MapLayerPass.ts
  • src/client/render/gl/passes/RailroadPass.ts
  • src/client/render/gl/passes/TerrainPass.ts
  • src/core/GameRunner.ts
  • src/core/game/TerrainMapLoader.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/core/GameRunner.ts
  • src/client/WebGLFrameBuilder.ts
  • src/client/ClientGameRunner.ts
  • src/client/render/gl/MapRenderer.ts
  • src/client/render/gl/Renderer.ts
  • src/client/render/gl/passes/MapLayerPass.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Maps A new map, or adjustments to an existing map itself, its json, etc,

Projects

Status: Development

Development

Successfully merging this pull request may close these issues.

2 participants