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? |
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