Fix/refactor/optim(StructureIconsLayer): restore structure icons after context loss, use WebGL/WebGPU/Canvas, and some improvements#3654
Conversation
… Most notably have it restore structure icons after webGL context loss.
WalkthroughThese changes optimize graphics rendering performance and enhance WebGL reliability in a PIXI-based rendering system. Key updates include canvas and texture caching in sprite creation, WebGL context loss detection and recovery, filter array reuse for visual effects, and improved resource cleanup with explicit child destruction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
239-246: Coalesce manual refresh with the restore rebuild path.
RefreshGraphicsEventcan fire while a context-restore rebuild is already queued. In that case this does the full destroy/recreate pass twice. Reuse the samerebuildPending+isLostguard here so recovery stays single-shot.♻️ Proposed change
this.eventBus.on(RefreshGraphicsEvent, () => { - // No redraw() here to be called from GameRenderer, because it triggers on - // on "contextrestored" event while we're a WebGL context. It also triggers - // on Alt-R, but we can listen to that event ourselves here - if (this.rendererInitialized) { - this.rebuildAllIcons(); - } + if ( + this.rendererInitialized && + !this.rebuildPending && + !this.renderer?.context?.isLost + ) { + this.rebuildPending = true; + requestAnimationFrame(() => { + this.rebuildPending = false; + if (!this.renderer?.context?.isLost) { + this.rebuildAllIcons(); + } + }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureIconsLayer.ts` around lines 239 - 246, The RefreshGraphicsEvent handler can trigger a duplicate destroy/recreate when a context-restore rebuild is already queued; update the eventBus.on(RefreshGraphicsEvent, ...) callback to guard against that by checking the same rebuildPending and isLost flags used in the restore path before calling rebuildAllIcons; i.e., only call this.rebuildAllIcons() when this.rendererInitialized is true AND rebuildPending is false AND isLost is false so the recovery path remains single-shot.
🤖 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/StructureIconsLayer.ts`:
- Around line 239-246: The RefreshGraphicsEvent handler can trigger a duplicate
destroy/recreate when a context-restore rebuild is already queued; update the
eventBus.on(RefreshGraphicsEvent, ...) callback to guard against that by
checking the same rebuildPending and isLost flags used in the restore path
before calling rebuildAllIcons; i.e., only call this.rebuildAllIcons() when
this.rendererInitialized is true AND rebuildPending is false AND isLost is false
so the recovery path remains single-shot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b9b76e4-cfe0-4260-a5f4-57c93eca19bc
📒 Files selected for processing (2)
src/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.ts
There was a problem hiding this comment.
Pull request overview
Updates the Pixi-based structure icon rendering path to better survive WebGL context loss and reduce per-frame allocations in the structure icon layer.
Changes:
- Hook
StructureIconsLayerinto Pixi’srenderer.runners.contextChangeto rebuild icons after context restoration and guard rendering/resizing when the context is lost. - Reduce allocations by reusing
OutlineFilterinstances/arrays and ensuring containers are destroyed with{ children: true }. - Improve
SpriteFactorytexture/canvas lifecycle: add cache clearing, destroy textures on invalidation, reuse a shared colorization canvas, and avoid destroyingPIXI.Texture.EMPTY.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/client/graphics/layers/StructureIconsLayer.ts | Adds context-change-driven rebuilds, lost-context guards, filter reuse, and more thorough container destruction. |
| src/client/graphics/layers/StructureDrawingUtils.ts | Adds explicit texture cache clearing/invalidation cleanup and reuses a shared offscreen canvas for colorization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…o canvas. Can be restricted later on. Also created restoration paths after context loss for each.
Description:
StructureIconsLayer and StructureDrawingUtils fixes and improvements. Most notably have it restore structure icons after webGL context loss.
Inspired by @Skigim's #3339, #3480. Fixes his #3207, contains only those fixes from the Issue that are actually valid and needed fixes, on top of his earlier merged PR.
CONTAINS (partly written by AI, excuse the exaggerated language)
1.
webglcontextrestored) but use PixiJS's internal hook (this.renderer.runners.contextChange). This prevents our cache-clearing commands from interrupting Pixi while it's still busy rebuilding its internal GL State Machine buffer. With links severed, we need to clear and rebuild all icons to restore them.this.renderer.context?.isLost): This prevents a crash in PixiJS. Fixes black map background and all graphics frozen, which has been reported a few times. Issue created in their repo: WebGL: error thrown on logPrettyShaderError when getShaderSource returns null pixijs/pixijs#12032.Container.destroy()does not recursively destroy its children. This PR explicitly adds.destroy({ children: true })inside icon deletion states. This stops thousands ofPIXI.SpriteandPIXI.BitmapTextchild nodes from leaking and choking Pixi when it attempts a WebGL restore.SpriteFactorynow correctly executes.destroy(true)onPIXI.Textureobjects. Previously, they were only deleted from the textureCache Map, retaining a phantom grip on GPU memory buffers.createTexture()inSpriteFactorystoresPIXI.Texture.EMPTY(a singleton) intextureCachewhen a structure type has no known shape. When not preventing removal of the EMPTY texture,clearCache()would calltexture.destroy(true)on PixiJS's shared global empty texture, breaking all sprites in the renderer that fall back to it.2. Small Memory/Perf Optimizations
<canvas>elements every time a structure color is loaded,SpriteFactorynow utilizes a cleanly sharedcolorCanvassingleton. To keep this safe from hardware acceleration crashes (where the 2D context dies alongside WebGL), it accurately nullifies itself inclearCache()and lazily instantiates on the next call (getImageColored()).skipCache: trueargument implicitly to dynamic UI elements viaPIXI.Texture.from(structureCanvas, true).renderGhost()previously spawned numerousnew OutlineFilter(...)WebGL shaders when hovering over invalid tiles, compounding to many leaked Shader Programs. We hoisted these filters to static class properties initialized once, and went a step further: hoisted the wrapping Array structures too (filterRedArray,filterGreenArray). This eliminates many pointless micro-allocations and GC sweeps entirely.BEFORE, for webGL:
https://youtu.be/durJxNFNePs
AFTER, for WebGL:
https://youtu.be/VnYEFMx4gfM
AFTER, for Canvas:
https://youtu.be/zT720oKxcaI
AFTER, for WebGPU:
https://youtu.be/J09Wee2qTs8
The performance optimizations weren't well measurable in my tests but there's no downgrade at least. WebGPU should bee better than WebGL when we would force it but again, currently PixiJS prefers WebGL hardcoded so only if we disallow WebGL will it use WebGPU if it is available, otherwise fallback gracefully to Canvas still.
Canvas skips parts gracefully, as long as the non-breaking issue exists in PixiJS (as explained above):

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