Make structure icon recovery state-sourced after runtime renderer disruption#3480
Make structure icon recovery state-sourced after runtime renderer disruption#3480Skigim wants to merge 7 commits intoopenfrontio:mainfrom
Conversation
Co-authored-by: Skigim <217411484+Skigim@users.noreply.github.com>
Co-authored-by: Skigim <217411484+Skigim@users.noreply.github.com>
Co-authored-by: Skigim <217411484+Skigim@users.noreply.github.com>
|
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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReworked rendering lifecycle: added dispose hooks and stable event handlers; StructureIconsLayer now handles WebGL context lost/restored, resizes stages, recomputes screen positions, and rebuilds structure renders from game state. Minor fixes in UnitLayer and ClientGameRunner; tests updated to cover new layer behavior. Changes
Sequence DiagramsequenceDiagram
participant Browser
participant WebGLContext as WebGL Context
participant PixiCanvas
participant StructureLayer
participant GameState
Browser->>WebGLContext: underlying context lost
WebGLContext->>PixiCanvas: webglcontextlost event
PixiCanvas->>StructureLayer: onContextLost handler
StructureLayer->>StructureLayer: preventDefault(), cancel frame, stop draws
Browser->>WebGLContext: context restored
WebGLContext->>PixiCanvas: webglcontextrestored event
PixiCanvas->>StructureLayer: onContextRestored handler
StructureLayer->>StructureLayer: resizeCanvas() / resizeStages()
StructureLayer->>StructureLayer: clearAllStructureRenders()
StructureLayer->>GameState: iterate game.units()
GameState-->>StructureLayer: active structure list
StructureLayer->>StructureLayer: rebuildAllStructuresFromState()
StructureLayer->>PixiCanvas: redraw structures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Pull request overview
This PR aims to make structure icon rendering more resilient to renderer resets by rebuilding structure icon state from the authoritative game.units() state (instead of relying primarily on incremental unit updates), and by doing additional recovery work after WebGL context restoration and resize/reset.
Changes:
- Rebuild structure icons from current game state during
StructureIconsLayer.redraw()and after WebGL context restore. - Recompute tracked structure screen locations after canvas resize/reset and deep-clear Pixi stages before full rebuilds.
- Add targeted unit tests covering redraw + full rebuild behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/client/graphics/layers/StructureIconsLayer.test.ts | Adds tests asserting redraw triggers a state rebuild and rebuild removes stale/inactive renders. |
| src/client/graphics/layers/UnitLayer.ts | Makes updateUnitsSprites robust to nullish mapping results and avoids running update passes on empty arrays. |
| src/client/graphics/layers/StructureIconsLayer.ts | Adds state-sourced rebuild logic, WebGL context restore handling, stage resizing, and stage deep-clear helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nd StructureIconsLayer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
202-212:⚠️ Potential issue | 🟡 MinorReposition the active ghost on resize/reset too.
resizeCanvas()now fixes stored structure renders, but the build ghost only moves inmoveGhost(). If the user is placing a structure when the window is resized or the renderer is reset, the ghost preview and range can stay at the old local coordinates until the next mouse move.💡 Suggested change
this.renderer.resize(innerWidth, innerHeight, 1); this.resizeStages(); + if (this.ghostUnit) { + const rect = this.transformHandler.boundingRect(); + if (rect) { + const localX = this.mousePos.x - rect.left; + const localY = this.mousePos.y - rect.top; + this.ghostUnit.container.position.set(localX, localY); + this.ghostUnit.range?.position.set(localX, localY); + } + } // Canvas size changes affect screen-space culling and icon placement, so // recompute every tracked structure location after a resize/reset. for (const render of this.rendersByUnitId.values()) { this.computeNewLocation(render); }🤖 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 202 - 212, resizeCanvas currently recomputes stored renders but doesn't update the live placement preview, so when the window/resizer resets the build preview (active ghost) stays at stale local coords; after you finish the loop over rendersByUnitId in resizeCanvas, if this.activeGhost is present call the same update path used for mouse moves (e.g. invoke moveGhost with the active ghost's current target or recompute its world/local coords via computeNewLocation and then refresh its range/placement visuals) so the ghost and its range are repositioned immediately on resize/reset.
🧹 Nitpick comments (2)
tests/client/graphics/layers/StructureIconsLayer.test.ts (1)
162-170: Exercise the stale-stage-child cleanup in this test.All three
removeChildren()mocks return[], so this case never runs the newclearStageChildren()destroy path. If orphan Pixi children stop being cleaned up, the suite still passes. Returning at least one mock child with adestroyspy from each stage would cover the branch this PR added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/graphics/layers/StructureIconsLayer.test.ts` around lines 162 - 170, The test currently stubs layerInternals.iconsStage, levelsStage, and dotsStage removeChildren() to always return [] so the new clearStageChildren() destroy branch is never exercised; update each stage mock (layerInternals.iconsStage, layerInternals.levelsStage, layerInternals.dotsStage) so their removeChildren() returns an array containing at least one mock child object with a destroy spy (e.g., { destroy: vi.fn() }) to ensure the destroy path is executed for each stage in StructureIconsLayer.test.ts.src/client/graphics/layers/StructureIconsLayer.ts (1)
174-176: Useredraw()as the single recovery path.This listener repeats the same two steps that
redraw()already owns. Callingthis.redraw()here keeps the restore logic in one place, which makes future recovery changes harder to miss.♻️ Suggested change
this.pixicanvas.addEventListener("webglcontextrestored", () => { - this.resizeCanvas(); - this.rebuildAllStructuresFromState(); + this.redraw(); });🤖 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 174 - 176, Replace the webglcontextrestored listener body to call the single recovery path method redraw() instead of repeating resizeCanvas() and rebuildAllStructuresFromState(); locate the event handler attached to this.pixicanvas for "webglcontextrestored" and invoke this.redraw() so future restore logic stays centralized in the redraw() method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 202-212: resizeCanvas currently recomputes stored renders but
doesn't update the live placement preview, so when the window/resizer resets the
build preview (active ghost) stays at stale local coords; after you finish the
loop over rendersByUnitId in resizeCanvas, if this.activeGhost is present call
the same update path used for mouse moves (e.g. invoke moveGhost with the active
ghost's current target or recompute its world/local coords via
computeNewLocation and then refresh its range/placement visuals) so the ghost
and its range are repositioned immediately on resize/reset.
---
Nitpick comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 174-176: Replace the webglcontextrestored listener body to call
the single recovery path method redraw() instead of repeating resizeCanvas() and
rebuildAllStructuresFromState(); locate the event handler attached to
this.pixicanvas for "webglcontextrestored" and invoke this.redraw() so future
restore logic stays centralized in the redraw() method.
In `@tests/client/graphics/layers/StructureIconsLayer.test.ts`:
- Around line 162-170: The test currently stubs layerInternals.iconsStage,
levelsStage, and dotsStage removeChildren() to always return [] so the new
clearStageChildren() destroy branch is never exercised; update each stage mock
(layerInternals.iconsStage, layerInternals.levelsStage,
layerInternals.dotsStage) so their removeChildren() returns an array containing
at least one mock child object with a destroy spy (e.g., { destroy: vi.fn() })
to ensure the destroy path is executed for each stage in
StructureIconsLayer.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 930a4e59-ecc0-4612-a049-94f76f3fe820
📒 Files selected for processing (3)
src/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/UnitLayer.tstests/client/graphics/layers/StructureIconsLayer.test.ts
…ot initialized; update ghost repositioning on canvas resize
|
mild performance tradeoff here - redraw is heavier but should be separate from hot path - tick is still incremental, full redraw should only kick in to recover from desync. |
|
@Skigim I don't know enough about this. But
"I was thinking for StructureIconsLayer we could listen to event "webglcontextlost" and then after X milliseconds check new PIXI.GlContextSystem(this.renderer).isLost() if it was auto restored by pixiJS itself or not. And have a seperate function restore the icons if needed. But i don't have actual experience with PixiJS nor WebGL. So don't know if this is a good way to go about it. Also don't know if there's a good way to test. GlContextSystem.forceContextLoss() works, but also auto-restores right away. While WEBGL_lose_context.loseContext works as a test to lose context, but it doesn't trigger pixiJS protected function handleContextLoss like it would in reality so we don't have a real-world test."
|
|
@VariableVince Even if Pixi is recovering, that evidently doesn’t guarantee the structure icon layer comes back in the right state. This change is basically saying “the visuals may be wrong after disruption” and rebuilding from game state, rather than only acting if Pixi itself failed to restore - since reproducing that isn't something I've managed to do yet. The bigger problem here is that I still can’t reliably reproduce the bug. There seem to be too many contributing factors, especially without knowing whether there are specific in-game triggers. along your point though, the follow-up should be to check whether Pixi is actually failing to restore, and if so whether that’s the direct cause of the original bug. I can track that in the original issue thread so it doesn’t get lost. Even aside from this specific bug, I think this gives the layer a more reliable recovery path. I also don’t think it obscures the issue so much as it narrows one part of the investigation. |
|
agree with @VariableVince here, good analysis. |
|
@scamiv @VariableVince understood. I'll get back to the drawing board, and see if I can reach out to clan members to run some customs and try to force the bug out - then I can at least test Pixi's native restore path. Any other ideas I can try to reproduce? |
| this.resizeStages(); | ||
| // Canvas size changes affect screen-space culling and icon placement, so | ||
| // recompute every tracked structure location after a resize/reset. | ||
| for (const render of this.rendersByUnitId.values()) { | ||
| this.computeNewLocation(render); | ||
| } | ||
| if (this.ghostUnit) { | ||
| this.moveGhost(new MouseMoveEvent(this.mousePos.x, this.mousePos.y)); | ||
| } |
There was a problem hiding this comment.
Is this related the the canvas redraw?
There was a problem hiding this comment.
This could fit well with my attempt at fixing the "icons displaced on zoom" in #3453. Which is currently waiting for testers.
If this code isn't accepted as part of this PR because it is outside its scope, it may well fit within the scope of my PR 3453 or as a seperate PR. Since resize isn't covered by TransformHandler, currently icon locations are indeed not recalculated in all cases it seems.
(also @Skigim )
If you don't know it yet like me until recently, you could try chrome://gpucrash on another tab while the game is running. That forces a deeper crash than just telling it to lose webGL context. Coming back to the game tab you can see what is restores by itself and how your PR affects it. A solution like in this PR may still be needed to catch crashes even if we fixed/optimized all that we can from the games' side, if anything. Because you're right about not all icons etc being restored, you'll see when you use this to force a gpu crash. |
|
|
||
| redraw() { | ||
| this.resizeCanvas(); | ||
| this.rebuildAllStructuresFromState(); |
There was a problem hiding this comment.
Must say i do like that, as a by-effect, Alt+R will work on the structure icons with this. Whereas currently only some of the graphics are rebuild if the user tries Alt+R.
|
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
|
Comment to prevent staleness. We should consider adding this, after all the map dissappearing issue was fixed in the same way. Question is if we can prevent webGL context loss at all times, probably not as it can have outside reasons. So while it is good to research root causes further, maybe it is more important to keep the game playable for more players in the end? Ie. by merging this PR |
github-actions
Bot
removed
the
Stale
|
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
github-project-automation
Bot
moved this from Triage
to Complete
in OpenFront Release Management
…r context loss, use WebGL/WebGPU/Canvas, and some improvements (#3654) ## 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.** * ** AutoDetectRenderer: ** now, if Hardware Acceleration is unavailable or disabled, Structure Icons will be displayed using Canvas renderer. Otherwise it will use either WebGL or WebGPU, depeding on which is available. PixiJS currently prefers WebGL but it will switch this to WebGPU at one point. We can also force it to WebGPU as explained in the comment. * ** Canvas: ** on Canvas, what doesn't work is gracefully skipped. The non-working parts will be fixed, see this issue in their repo, but until then it will work fine for us anyway: pixijs/pixijs#11981 * **WebGPU Context Loss:** PixiJS doesn't restore this context itself. Instead we do it by calling setupRenderer again on device loss. * **WebGL Context Loss:** To know when we need to restore the layer, don't use native event (`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. * **WebGL Context existance Check (`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: pixijs/pixijs#12032. * **Redraw:** for Canvas context restore or on Alt-R, a call from GameRenderer now actually restores icons. Also called for WebGPU device loss and after contextChange WebGL restoration. Checks for WebGL context.isLost so a calls from Alt-R etc won't meddle while GL context is lost. * **Orphaned Object Leaks:** In PixiJS v8, `Container.destroy()` does *not* recursively destroy its children. This PR explicitly adds `.destroy({ children: true })` inside icon deletion states. This stops thousands of `PIXI.Sprite` and `PIXI.BitmapText` child nodes from leaking and choking Pixi when it attempts a WebGL restore. * **Texture Lifecycle:** Invalidate caching logic in `SpriteFactory` now correctly executes `.destroy(true)` on `PIXI.Texture` objects. Previously, they were only deleted from the textureCache Map, retaining a phantom grip on GPU memory buffers. * **Don't remove PIXI.Texture.EMPTY from textureCache: `createTexture()` in `SpriteFactory` stores `PIXI.Texture.EMPTY` (a singleton) in `textureCache` when a structure type has no known shape. When not preventing removal of the EMPTY texture, `clearCache()` would call `texture.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** * **The Shared 2D Canvas Optimization:** To prevent allocating endless tiny `<canvas>` elements every time a structure color is loaded, `SpriteFactory` now utilizes a cleanly shared `colorCanvas` singleton. To keep this safe from hardware acceleration crashes (where the 2D context dies alongside WebGL), it accurately nullifies itself in `clearCache()` and lazily instantiates on the next call (`getImageColored()`). * **Bypassing Inefficient Textures Cache:** Now passing the `skipCache: true` argument implicitly to dynamic UI elements via `PIXI.Texture.from(structureCanvas, true)`. * **Zero-Allocation Filters (No more GC Stutters):** `renderGhost()` previously spawned numerous `new 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): <img width="952" height="705" alt="AFTER Canvas in Firefox skips non-supported gracefully" src="https://github.com/user-attachments/assets/17e8d8ab-05dc-47cb-ab11-f0f4d015a42a" /> ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description:
I don't have a clean repro for the mid-match structure icon issue yet, so this PR is meant to reduce the chance of structure icons getting lost after a renderer reset.
This also does not address the separate issue where structure icons sometimes fail to show from game start.
This changes structure icon rendering to rebuild from the current game state instead of mostly waiting for future unit updates.
Main changes:
StructureIconsLayer.redraw()rebuild structure icons fromgame.units()Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
skigim