Render name layer with Pixi#3888
Draft
scamiv wants to merge 9 commits intoopenfrontio:mainfrom
Draft
Conversation
Contributor
WalkthroughConverts NameLayer from DOM/CSS to PIXI rendering, adds an asset builder that produces bitmap font and sprite atlases (with deterministic fallbacks), introduces layout and asset-loading helpers, updates renderer/device-pixel handling, and expands tests. ChangesNameLayer PIXI Rendering Refactor
Sequence Diagram(s)sequenceDiagram
participant Init
participant AssetLoader as NameLayerAssets
participant PIXIStage as PIXI Stage
participant GameLoop
participant NameLayer
Init->>AssetLoader: preload()
AssetLoader->>AssetLoader: load bitmap font + atlases
Init->>NameLayer: init()
NameLayer->>PIXIStage: setupRenderer
NameLayer->>NameLayer: subscribe to events
GameLoop->>NameLayer: tick()
NameLayer->>NameLayer: updateTransformsAndVisibility()
NameLayer->>NameLayer: throttled renderPlayerInfo()
NameLayer->>PIXIStage: update containers/sprites/text
GameLoop->>NameLayer: renderLayer(mainContext)
PIXIStage->>NameLayer: render to canvas
NameLayer->>GameLoop: composite to main context
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
src/client/graphics/layers/NameLayer.ts (8)
528-557: 💤 Low value
updateAllianceIcondoes not refresh the colored sprite source whenicon.srcchanges.The shape detection on line 535 only checks
iconRender.allianceexists. If the player's alliance partner changes andicon.srcdiffers (different ally avatar), the cachediconRender.srcstays at the original value andcolored.textureis reassigned at line 570 from the newcoloredTexturelookup — which uses the newicon.src. So functionally it's correct because we re-resolve the texture every refresh.However, the
iconRender.srcfield is never updated to match the newicon.src, so any future code that checks "did the source change" on alliance icons will be reading stale data. Consider mirroring theupdateImageIconpattern and updatingiconRender.src = icon.srcwhenevericon.srcchanges.♻️ Keep the cached src in sync
const refs = iconRender.alliance!; + iconRender.src = icon.src; refs.base.texture = baseTexture;🤖 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/graphics/layers/NameLayer.ts` around lines 528 - 557, updateAllianceIcon currently creates or reuses iconRender but never updates iconRender.src when icon.src changes; mirror the updateImageIcon pattern: after resolving/assigning the new colored texture (and any icon sprites), set iconRender.src = icon.src so the cached src stays in sync; locate the updateAllianceIcon function and update the branch where iconRender exists (and after you compute/assign colored.texture) to assign the new src to the existing iconRender.src.
91-92: 💤 Low value
pixicanvasfield name and visibility.Tiny readability nit:
pixicanvasreads oddly (lowercase compound).pixiCanvaswould match the rest of the codebase's camelCase. Alsoprivateis fine, but please mark itreadonlyonce initialized to communicate that no other code path reassigns it.♻️ Suggested rename
- private pixicanvas: HTMLCanvasElement; + private pixiCanvas!: HTMLCanvasElement;(plus call-site updates in
setupRendererandresizeCanvas)🤖 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/graphics/layers/NameLayer.ts` around lines 91 - 92, Rename the field pixicanvas to pixiCanvas on the NameLayer class and mark it readonly to reflect it isn't reassigned after initialization; update all references/call-sites in this class (notably within setupRenderer and resizeCanvas methods) to use pixiCanvas, preserve its type HTMLCanvasElement, and ensure any assignments occur only at construction/initialization so the readonly constraint is satisfied.
208-213: 💤 Low valueWebGPU
device.lostresolves once — handler will not run a second time.
device.lostis a Promise that resolves a single time per device. Afterredraw()finishes andsetupRenderer()swaps in a new renderer, the new device'slostpromise needs to be re-attached, which happens here becausesetupRenderer()runs on each rebuild — so this is currently fine. Just please add a comment noting that the re-attach is intentional, since dropping it later would be a silent regression. Also, prefervoid this.redraw()to make the floating-promise lint-clean.♻️ Tiny cleanup
if (this.renderer.name === "webgpu") { const gpuRenderer = this.renderer as PIXI.WebGPURenderer; - gpuRenderer.gpu.device.lost.then(() => { - this.redraw(); - }); + // device.lost resolves once per device; redraw() will re-attach on the new device. + void gpuRenderer.gpu.device.lost.then(() => { + void this.redraw(); + }); }🤖 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/graphics/layers/NameLayer.ts` around lines 208 - 213, In NameLayer where you attach the WebGPU device.lost handler (check this.renderer.name === "webgpu" and the block using gpu.device.lost), change the call to use void this.redraw() to satisfy floating-promise linting and add an inline comment explaining that device.lost is a one-time Promise and that re-attaching the handler happens intentionally via setupRenderer() on each rebuild so dropping this re-attach later would be a silent regression; keep the handler logic otherwise unchanged.
583-591: 💤 Low valueAlliance progress mask can produce a zero-height rect.
When
topCut >= size(full cut at fraction 0 —computeAllianceTopCutPercent(0) = 82.4%, so it stays undersize, butcomputeAllianceTopCutPercentis only clamped via1 - fractioninPlayerIcons.ts, not here), theMath.max(0, size - topCut)guard already protects against negative height. PIXI v8 handles zero-area rects, so this is safe today. Worth a short comment noting that theMath.max(0, ...)is intentional load-bearing, since removing it later would silently break the mask at the boundary.🤖 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/graphics/layers/NameLayer.ts` around lines 583 - 591, The mask calculation can produce a zero-height rectangle when topCut >= size; keep the Math.max(0, size - topCut) guard and add a concise inline comment above the refs.mask.rect call in NameLayer (near the fraction/topCut calculation) explaining that zero-height rects are intentional and relied upon (PIXI v8 tolerates zero-area rects) so the Math.max must not be removed; reference computeAllianceTopCutPercent, remaining, this.allianceDuration and refs.mask.rect in the comment so future editors understand why the guard exists.
123-142: 💤 Low value
redraw()swallows errors andsetupRendererfailure can leave the layer wedged.If
setupRenderer()throws (e.g., GPU init fails after device loss), thetry/finallyresetsrebuildPending = falsebutthis.rendererstays whatever it was (possibly destroyed insidesetupRenderer'sif (this.renderer)block) andrendererInitializedisfalse. From then on everyrenderLayer()call early-returns silently, with no retry. Consider logging the failure and scheduling a retry on the next animation frame, or at least surfacing it.♻️ Defensive logging
async redraw() { if (this.rebuildPending || this.rendererOrGLContextLost()) { return; } this.rebuildPending = true; try { if (this.renderer?.name === "webgpu") { this.rendererInitialized = false; - await this.setupRenderer(); + try { + await this.setupRenderer(); + } catch (error) { + console.error("NameLayer renderer rebuild failed", error); + return; + } }🤖 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/graphics/layers/NameLayer.ts` around lines 123 - 142, redraw() currently swallows exceptions from setupRenderer() and can leave the layer wedged; wrap the await this.setupRenderer() call in a try/catch inside redraw(), and in the catch log the error (using the layer's logger or console.error), normalize state (ensure this.renderer is null if it was destroyed and keep rendererInitialized = false) and schedule a retry via requestAnimationFrame(() => this.redraw()), so future renderLayer() calls can attempt initialization again; keep the existing finally that clears rebuildPending.
376-389: 💤 Low valueStale
fontColorcheck leaves troops text un-restyled when only the color changes.In the troops-update block, the conditions check
render.fontColor !== fontColor, butrender.fontColoris also the field used by the name block immediately above and is reassigned at the very end of the method (line 391). The first block already updateslastDisplayNamewhenfontColorchanges; this second block's condition still evaluates against the same not-yet-updatedrender.fontColor, so it works today — but the two blocks reading and the method tail writing the same field is fragile. Cache the previous color in a local before the two blocks to keep the intent readable.♻️ Tiny refactor
+ const previousColor = render.fontColor; @@ - if ( - render.lastDisplayName !== displayName || - render.fontColor !== fontColor || + if ( + render.lastDisplayName !== displayName || + previousColor !== fontColor || render.nameText.style.fontSize !== render.fontSize || render.nameText.style.fontFamily !== this.assets.fontFamily ) { @@ - if ( - render.lastTroopsText !== troopsText || - render.fontColor !== fontColor || + if ( + render.lastTroopsText !== troopsText || + previousColor !== fontColor || render.troopsText.style.fontSize !== render.fontSize || render.troopsText.style.fontFamily !== this.assets.fontFamily ) {🤖 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/graphics/layers/NameLayer.ts` around lines 376 - 389, The troops text update condition compares render.fontColor against the new fontColor while render.fontColor is also read/updated by the name block and overwritten at the end of the method, making the color-only change fragile; fix this by capturing the current color into a local (e.g., const prevFontColor = render.fontColor) before the name and troops blocks and then use prevFontColor in the troops block condition (compare prevFontColor !== fontColor) so render.troopsText, render.lastTroopsText, and its style (fontFamily from this.assets.fontFamily, fontSize from render.fontSize, fill) are updated correctly when only color changes, leaving the final assignment to render.fontColor at the method tail unchanged.
159-166: 💤 Low value
updateTransformsAndVisibilityruns every frame;renderPlayerInfois throttled — drift possible.
updateTransformsAndVisibility()iterates every render every frame, which is correct for smooth pan/zoom. Butrender.fontSizeis only refreshed insiderenderPlayerInfo()(gated byrenderRefreshRate = 500ms). Between refreshes,computeNameLayerScale(baseSize)uses up-to-datebaseSizewhile the actualnameText.style.fontSizestill reflects the value from up to 500ms ago, so on a fast zoom you may see briefly mis-sized text. Acceptable given the "practical parity" target — flagging as a perf/UX trade-off, not a defect.🤖 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/graphics/layers/NameLayer.ts` around lines 159 - 166, The code currently throttles renderPlayerInfo() (which refreshes nameText.style.fontSize) but calls updateTransformsAndVisibility() every frame, so computeNameLayerScale(baseSize) can use an up-to-date baseSize while the DOM fontSize is stale leading to visual drift; fix by moving the font-size refresh out of the throttled path — either update the DOM font size each frame inside updateTransformsAndVisibility() (or call a new updateFontSize() from it) or adjust computeNameLayerScale() to read the current nameText.style.fontSize from the DOM instead of relying on the cached value from renderPlayerInfo(); update references: updateTransformsAndVisibility(), renderPlayerInfo(), computeNameLayerScale(), and nameText.style.fontSize accordingly.
394-428: 💤 Low value
updateFlagresets state in some paths but not others;flagSrccan desync.When
textureisnull(line 411) the function returns withflagSrcstill set to the newsrcand a hidden sprite. If the player's flag cosmetic changes again before the texture ever loads, the next call will execute thesrc !== render.flagSrcbranch and destroy/recreate the sprite — but theassets.getTexture(src)for the previous failed src may still resolve later and callthis.textures.set(src, texture), which is harmless. So far so good.The actual concern: if
flagis cleared (line 397),flagSrcis reset to""but the icon-row layout inlayoutRenderchecksrender.flagSprite?.visible, which we just set to nothing because we destroyed the sprite. That path is correct. The earlier path (line 411) leavesrender.flagSpritewithvisible = falsewhileflagSrcis still the new src, so subsequent layout treatshasFlag = false. Functionally OK, but the asymmetry between "no src" vs "src present, texture missing" makes it easy to break later. A short comment or a small helper to centralize "hide flag" would help.🤖 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/graphics/layers/NameLayer.ts` around lines 394 - 428, updateFlag currently sets render.flagSrc before confirming a texture exists, and hiding logic is duplicated; extract a small helper (e.g., hideFlag(render)) that destroys/nulls render.flagSprite, sets visible false and clears flagSrc as appropriate, and call it in the "no flag" branch and the "texture missing" branch; also move the assignment render.flagSrc = src (the src update) to after you confirm texture exists (or set it only when a sprite is created) so render.flagSrc stays in sync with render.flagSprite/visible; reference updateFlag, render.flagSprite, render.flagSrc, assets.getTexture, and layoutRender when making these changes.src/client/graphics/layers/NameLayerLayout.ts (2)
119-125: ⚡ Quick winCentered icons all share
(0, nameY)and will overlap.
centeredIconPositionsproduces N copies of the same point, so any caller that places multiple centered icons will stack them on top of each other. If only ever one centered icon is expected, please make that contract explicit (assert or accept a single boolean instead of a count); otherwise spread them like the regular icons.♻️ Possible spread, mirroring `iconPositions`
- const centeredIconPositions = Array.from( - { length: centeredIconCount }, - () => ({ - x: 0, - y: nameY, - }), - ); + const centeredRowWidth = + centeredIconCount > 0 + ? centeredIconCount * iconSize + + (centeredIconCount - 1) * NAME_LAYER_ICON_GAP + : 0; + const centeredStartX = -centeredRowWidth / 2 + iconSize / 2; + const centeredIconPositions = Array.from( + { length: centeredIconCount }, + (_, i) => ({ + x: centeredStartX + i * (iconSize + NAME_LAYER_ICON_GAP), + y: nameY, + }), + );🤖 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/graphics/layers/NameLayerLayout.ts` around lines 119 - 125, centeredIconPositions currently creates centeredIconCount identical points (x:0,y:nameY) causing stacked icons; update the logic in NameLayerLayout to either assert/accept that centeredIconCount must be <=1 (throw or convert the count into a boolean) or compute distinct x positions by spreading centered icons similarly to the existing iconPositions calculation (use centeredIconCount and nameY to generate evenly spaced x coordinates instead of repeating {x:0,y:nameY}); update references to centeredIconPositions and centeredIconCount accordingly so callers get non-overlapping positions.
169-191: 💤 Low valueGlyph check is single–code-point and will mangle emoji ZWJ clusters.
for (const char of value)iterates code points, so a grapheme cluster like 👨👩👧 becomes five separate code points and turns into?????. That is fine for bitmap-font names where these would not render anyway, but worth noting becausereplaceUnsupportedNameGlyphsis also used forrenderTroops()output (just numbers + suffix, so harmless today). If display names are ever expected to contain ZWJ emojis, consider collapsing runs of?or detecting clusters.🤖 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/graphics/layers/NameLayerLayout.ts` around lines 169 - 191, replaceUnsupportedNameGlyphs currently iterates codepoints (for...of) and will split ZWJ emoji/grapheme clusters into multiple "?" characters; change the implementation to operate on grapheme clusters instead: use Intl.Segmenter (or a small grapheme-splitter lib) to iterate segments and test each segment against SUPPORTED_TEXT_CHARS (or treat any segment longer than one codepoint as unsupported), add the replacement "?" once per unsupported segment, and record the original segment in warnedUnsupportedGlyphs (update that set to store the full segment string) so warnings are not repeated; alternatively, a minimal fix is to post-process the result to collapse consecutive "?" runs into a single "?" before returning.tests/NameLayer.test.ts (1)
99-105: 💤 Low valueAdd a phase-shift assertion to lock in the alpha waveform.
The current test only hits two phases (
0and500msatduration ≈ 1s). A small extra assertion atnowMs = 250(mid-rising edge) protects against accidental refactors of the cosine ease without exploding test count. Optional.♻️ Suggested addition
expect(computeTraitorFlashAlpha(150, 0)).toBeCloseTo(1); + expect(computeTraitorFlashAlpha(150, 250)).toBeCloseTo(0.65); expect(computeTraitorFlashAlpha(150, 500)).toBeCloseTo(0.3);🤖 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/NameLayer.test.ts` around lines 99 - 105, Test currently checks alpha at phase 0 and 500ms only, leaving the waveform phase-shift untested; add an assertion for computeTraitorFlashAlpha(durationMs ≈ 150, nowMs = 250) to lock the mid-phase (rising) value so the cosine easing can't be accidentally changed — locate the existing test case using computeTraitorFlashDurationSeconds and computeTraitorFlashAlpha and add one expect(...) for nowMs = 250 with a toBeCloseTo value matching the current implementation's midpoint alpha.src/client/graphics/layers/NameLayerAssets.ts (2)
69-87: 💤 Low valueFallback font path leaves
fontFamilypointing at the primary on total failure.If both
nameLayerFontandfallbackFontfail to load,this.fontFamilyis stillNAME_LAYER_FONT_FAMILYfrom the field initializer. DownstreamPIXI.BitmapTextwith a missing family will throw or render empty. A small guard makes the failure mode obvious.♻️ Suggested tweak
try { await PIXI.Assets.load(fallbackFont); this.fontFamily = NAME_LAYER_FALLBACK_FONT_FAMILY; } catch (error) { console.error("NameLayer failed to load bitmap font", error); + // Leave fontFamily as-is; callers should still degrade gracefully. }Optional: also expose a boolean like
fontReadysoNameLayercan skip BitmapText creation until a font is available, instead of depending on PIXI's internal error path.🤖 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/graphics/layers/NameLayerAssets.ts` around lines 69 - 87, loadFont currently leaves this.fontFamily set to NAME_LAYER_FONT_FAMILY from the initializer even when both primary and fallback loads fail; change loadFont so it only assigns this.fontFamily after a successful PIXI.Assets.load call (assign NAME_LAYER_FONT_FAMILY on primary success, NAME_LAYER_FALLBACK_FONT_FAMILY on fallback success) and if both fail explicitly clear/unset this.fontFamily (e.g. set to null or undefined) and set a new boolean this.fontReady = false (set true on any successful load) so callers can avoid creating PIXI.BitmapText when no font is available; update any code that creates BitmapText to check this.fontReady before instantiation.
25-49: 💤 Low valueLazy load works, but "null forever" makes transient failures permanent.
When
PIXI.Assets.load(src)rejects (e.g., transient network blip or CORS hiccup), the entry is set tonullandgetTexture(src)will returnnullforever after. Given the PR notes call out CORS-blocked external flags, this is likely intentional and aligns with the once-only warning policy. Worth confirming you do not want a periodic retry path for player-attached textures (flags) that the user can later un-block.🤖 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/graphics/layers/NameLayerAssets.ts` around lines 25 - 49, The current getTexture marks failures as permanent by setting this.textures.set(src, null) in the PIXI.Assets.load catch, so transient errors never re-attempt; change the failure handling in getTexture: do not set a permanent null for src in this.textures, instead either remove the texture entry (this.textures.delete(src)) or store a soft-failure marker and schedule a retry/backoff (e.g., setTimeout to clear the marker after a delay) so subsequent getTexture(src) calls will re-trigger PIXI.Assets.load; update the catch branch that calls warnTextureFailure to delete or soft-mark the entry and ensure pendingTextures.delete(src) still runs in finally; keep references to getTexture, this.textures, this.pendingTextures, and warnTextureFailure when making the change.
🤖 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 `@scripts/build-namelayer-assets.mjs`:
- Around line 308-313: The current parsing of emojiTable silently returns []
when the regex fails (tableSource is falsy), causing builds to produce empty
emoji atlases; update the code around utilSource and tableSource so it fails
fast: if utilSource.match(/export const emojiTable = \[([\s\S]*?)\] as const;/)
yields no result (tableSource is undefined), throw a descriptive Error (or call
process.exit(1) after logging) that includes context like "emojiTable not found
in utilSource" and a short snippet of utilSource or file identifier; keep the
existing extraction into Array.from(...) when tableSource exists and only
proceed with writing assets after the check succeeds.
In `@src/client/graphics/layers/NameLayer.ts`:
- Line 117: The resize listener added in NameLayer.init() uses an anonymous
arrow function so it cannot be removed, causing leaks on re-instantiation; store
a bound handler (e.g. this.onWindowResize = () => this.resizeCanvas()) and use
that in window.addEventListener and window.removeEventListener during cleanup;
add a destroy() method on NameLayer (or implement Layer.destroy) that calls
window.removeEventListener("resize", this.onWindowResize) and also calls
this.renderer?.destroy(true) to release PIXI resources and allow the instance to
be garbage collected.
- Around line 159-180: Replace the 2-arg drawImage call in NameLayer.renderLayer
with the full source/destination rectangle overload so the Pixi canvas is
explicitly stretched to the main canvas (e.g., copy from renderer.canvas 0,0,
renderer.canvas.width, renderer.canvas.height to mainContext.canvas 0,0,
mainContext.canvas.width, mainContext.canvas.height); apply the same change in
StructureIconsLayer.renderLayer (around the drawImage at line ~354). Also update
the Pixi resizeCanvas implementation to size the renderer.canvas using
window.innerWidth/innerHeight multiplied by devicePixelRatio (and adjust
CSS/style to the CSS pixel size) so the canvas backing store matches
devicePixelRatio and avoids blurriness on HiDPI screens.
---
Nitpick comments:
In `@src/client/graphics/layers/NameLayer.ts`:
- Around line 528-557: updateAllianceIcon currently creates or reuses iconRender
but never updates iconRender.src when icon.src changes; mirror the
updateImageIcon pattern: after resolving/assigning the new colored texture (and
any icon sprites), set iconRender.src = icon.src so the cached src stays in
sync; locate the updateAllianceIcon function and update the branch where
iconRender exists (and after you compute/assign colored.texture) to assign the
new src to the existing iconRender.src.
- Around line 91-92: Rename the field pixicanvas to pixiCanvas on the NameLayer
class and mark it readonly to reflect it isn't reassigned after initialization;
update all references/call-sites in this class (notably within setupRenderer and
resizeCanvas methods) to use pixiCanvas, preserve its type HTMLCanvasElement,
and ensure any assignments occur only at construction/initialization so the
readonly constraint is satisfied.
- Around line 208-213: In NameLayer where you attach the WebGPU device.lost
handler (check this.renderer.name === "webgpu" and the block using
gpu.device.lost), change the call to use void this.redraw() to satisfy
floating-promise linting and add an inline comment explaining that device.lost
is a one-time Promise and that re-attaching the handler happens intentionally
via setupRenderer() on each rebuild so dropping this re-attach later would be a
silent regression; keep the handler logic otherwise unchanged.
- Around line 583-591: The mask calculation can produce a zero-height rectangle
when topCut >= size; keep the Math.max(0, size - topCut) guard and add a concise
inline comment above the refs.mask.rect call in NameLayer (near the
fraction/topCut calculation) explaining that zero-height rects are intentional
and relied upon (PIXI v8 tolerates zero-area rects) so the Math.max must not be
removed; reference computeAllianceTopCutPercent, remaining,
this.allianceDuration and refs.mask.rect in the comment so future editors
understand why the guard exists.
- Around line 123-142: redraw() currently swallows exceptions from
setupRenderer() and can leave the layer wedged; wrap the await
this.setupRenderer() call in a try/catch inside redraw(), and in the catch log
the error (using the layer's logger or console.error), normalize state (ensure
this.renderer is null if it was destroyed and keep rendererInitialized = false)
and schedule a retry via requestAnimationFrame(() => this.redraw()), so future
renderLayer() calls can attempt initialization again; keep the existing finally
that clears rebuildPending.
- Around line 376-389: The troops text update condition compares
render.fontColor against the new fontColor while render.fontColor is also
read/updated by the name block and overwritten at the end of the method, making
the color-only change fragile; fix this by capturing the current color into a
local (e.g., const prevFontColor = render.fontColor) before the name and troops
blocks and then use prevFontColor in the troops block condition (compare
prevFontColor !== fontColor) so render.troopsText, render.lastTroopsText, and
its style (fontFamily from this.assets.fontFamily, fontSize from
render.fontSize, fill) are updated correctly when only color changes, leaving
the final assignment to render.fontColor at the method tail unchanged.
- Around line 159-166: The code currently throttles renderPlayerInfo() (which
refreshes nameText.style.fontSize) but calls updateTransformsAndVisibility()
every frame, so computeNameLayerScale(baseSize) can use an up-to-date baseSize
while the DOM fontSize is stale leading to visual drift; fix by moving the
font-size refresh out of the throttled path — either update the DOM font size
each frame inside updateTransformsAndVisibility() (or call a new
updateFontSize() from it) or adjust computeNameLayerScale() to read the current
nameText.style.fontSize from the DOM instead of relying on the cached value from
renderPlayerInfo(); update references: updateTransformsAndVisibility(),
renderPlayerInfo(), computeNameLayerScale(), and nameText.style.fontSize
accordingly.
- Around line 394-428: updateFlag currently sets render.flagSrc before
confirming a texture exists, and hiding logic is duplicated; extract a small
helper (e.g., hideFlag(render)) that destroys/nulls render.flagSprite, sets
visible false and clears flagSrc as appropriate, and call it in the "no flag"
branch and the "texture missing" branch; also move the assignment render.flagSrc
= src (the src update) to after you confirm texture exists (or set it only when
a sprite is created) so render.flagSrc stays in sync with
render.flagSprite/visible; reference updateFlag, render.flagSprite,
render.flagSrc, assets.getTexture, and layoutRender when making these changes.
In `@src/client/graphics/layers/NameLayerAssets.ts`:
- Around line 69-87: loadFont currently leaves this.fontFamily set to
NAME_LAYER_FONT_FAMILY from the initializer even when both primary and fallback
loads fail; change loadFont so it only assigns this.fontFamily after a
successful PIXI.Assets.load call (assign NAME_LAYER_FONT_FAMILY on primary
success, NAME_LAYER_FALLBACK_FONT_FAMILY on fallback success) and if both fail
explicitly clear/unset this.fontFamily (e.g. set to null or undefined) and set a
new boolean this.fontReady = false (set true on any successful load) so callers
can avoid creating PIXI.BitmapText when no font is available; update any code
that creates BitmapText to check this.fontReady before instantiation.
- Around line 25-49: The current getTexture marks failures as permanent by
setting this.textures.set(src, null) in the PIXI.Assets.load catch, so transient
errors never re-attempt; change the failure handling in getTexture: do not set a
permanent null for src in this.textures, instead either remove the texture entry
(this.textures.delete(src)) or store a soft-failure marker and schedule a
retry/backoff (e.g., setTimeout to clear the marker after a delay) so subsequent
getTexture(src) calls will re-trigger PIXI.Assets.load; update the catch branch
that calls warnTextureFailure to delete or soft-mark the entry and ensure
pendingTextures.delete(src) still runs in finally; keep references to
getTexture, this.textures, this.pendingTextures, and warnTextureFailure when
making the change.
In `@src/client/graphics/layers/NameLayerLayout.ts`:
- Around line 119-125: centeredIconPositions currently creates centeredIconCount
identical points (x:0,y:nameY) causing stacked icons; update the logic in
NameLayerLayout to either assert/accept that centeredIconCount must be <=1
(throw or convert the count into a boolean) or compute distinct x positions by
spreading centered icons similarly to the existing iconPositions calculation
(use centeredIconCount and nameY to generate evenly spaced x coordinates instead
of repeating {x:0,y:nameY}); update references to centeredIconPositions and
centeredIconCount accordingly so callers get non-overlapping positions.
- Around line 169-191: replaceUnsupportedNameGlyphs currently iterates
codepoints (for...of) and will split ZWJ emoji/grapheme clusters into multiple
"?" characters; change the implementation to operate on grapheme clusters
instead: use Intl.Segmenter (or a small grapheme-splitter lib) to iterate
segments and test each segment against SUPPORTED_TEXT_CHARS (or treat any
segment longer than one codepoint as unsupported), add the replacement "?" once
per unsupported segment, and record the original segment in
warnedUnsupportedGlyphs (update that set to store the full segment string) so
warnings are not repeated; alternatively, a minimal fix is to post-process the
result to collapse consecutive "?" runs into a single "?" before returning.
In `@tests/NameLayer.test.ts`:
- Around line 99-105: Test currently checks alpha at phase 0 and 500ms only,
leaving the waveform phase-shift untested; add an assertion for
computeTraitorFlashAlpha(durationMs ≈ 150, nowMs = 250) to lock the mid-phase
(rising) value so the cosine easing can't be accidentally changed — locate the
existing test case using computeTraitorFlashDurationSeconds and
computeTraitorFlashAlpha and add one expect(...) for nowMs = 250 with a
toBeCloseTo value matching the current implementation's midpoint alpha.
🪄 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: 0404470f-32bc-4a32-b21e-27788b4287ac
⛔ Files ignored due to path filters (4)
resources/fonts/namelayer_overpass.pngis excluded by!**/*.pngresources/fonts/overpass-regular.otfis excluded by!**/*.otfresources/images/namelayer-emojis.pngis excluded by!**/*.pngresources/images/namelayer-icons.pngis excluded by!**/*.png
📒 Files selected for processing (12)
eslint.config.jspackage.jsonresources/fonts/namelayer_overpass.xmlresources/fonts/overpass-OFL.txtresources/images/namelayer-emojis.jsonresources/images/namelayer-icons.jsonscripts/build-namelayer-assets.mjssrc/client/graphics/PlayerIcons.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/NameLayerAssets.tssrc/client/graphics/layers/NameLayerLayout.tstests/NameLayer.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/NameLayerAssets.ts (1)
58-60: ⚡ Quick win
resetWarningsForTests()leavespreloadPromise,fontReady, andfontFamilystale between test runs.The method only clears
warnedTextureFailures. If a test callspreload()and the font load fails (no network in test environment),preloadPromiseis set to a resolved-but-failed promise andfontReadystaysfalse. A subsequent test that reuses the same instance cannot re-triggerloadBaseAssets()becausepreloadPromise ??= …returns the cached failure. A complete reset is needed for real test isolation.♻️ Proposed fix
resetWarningsForTests(): void { this.warnedTextureFailures.clear(); + this.failedTextures.clear(); // if you adopt the failedTextures fix above + this.preloadPromise = null; + this.fontFamily = null; + this.fontReady = false; + this.textures.clear(); + this.pendingTextures.clear(); }🤖 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/graphics/layers/NameLayerAssets.ts` around lines 58 - 60, resetWarningsForTests currently only clears warnedTextureFailures but leaves preloadPromise, fontReady, and fontFamily set which causes cached failed font loads to persist across tests; update resetWarningsForTests to also reset this.preloadPromise to undefined/null, this.fontReady to false, and this.fontFamily to undefined (or the initial empty value) so subsequent calls to preload() can re-run loadBaseAssets() and attempt a fresh font load; reference resetWarningsForTests, preloadPromise, fontReady, fontFamily, preload(), and loadBaseAssets() when making the changes.
🤖 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/graphics/layers/NameLayerAssets.ts`:
- Around line 32-49: getTexture currently retries failed loads on every call
because rejected PIXI.Assets.load entries are removed from pendingTextures and
never marked as permanently failed; add a failedTextures Set and check it at the
top of getTexture to immediately return null if src is in failedTextures, set
failedTextures.add(src) in the .catch handler (before/inside finally) so
subsequent calls won’t re-trigger network loads, and update
resetWarningsForTests to clear failedTextures alongside warnedTextureFailures so
tests can reset state; reference functions/fields: getTexture, pendingTextures,
textures, warnTextureFailure, warnedTextureFailures, resetWarningsForTests, and
add the new failedTextures Set to the class.
---
Nitpick comments:
In `@src/client/graphics/layers/NameLayerAssets.ts`:
- Around line 58-60: resetWarningsForTests currently only clears
warnedTextureFailures but leaves preloadPromise, fontReady, and fontFamily set
which causes cached failed font loads to persist across tests; update
resetWarningsForTests to also reset this.preloadPromise to undefined/null,
this.fontReady to false, and this.fontFamily to undefined (or the initial empty
value) so subsequent calls to preload() can re-run loadBaseAssets() and attempt
a fresh font load; reference resetWarningsForTests, preloadPromise, fontReady,
fontFamily, preload(), and loadBaseAssets() when making the changes.
🪄 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: 053b11ec-8c71-4eb4-9536-008ca7c74e1f
📒 Files selected for processing (7)
scripts/build-namelayer-assets.mjssrc/client/graphics/layers/Layer.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/NameLayerAssets.tssrc/client/graphics/layers/NameLayerLayout.tssrc/client/graphics/layers/StructureIconsLayer.tstests/NameLayer.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/graphics/layers/NameLayerLayout.ts
- scripts/build-namelayer-assets.mjs
- tests/NameLayer.test.ts
- src/client/graphics/layers/NameLayer.ts
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…oad in tests Track permanently failed texture URLs so getTexture does not call PIXI.Assets.load again after a rejection. Extend resetWarningsForTests to clear failedTextures and to reset preloadPromise, fontReady, and fontFamily so tests can run preload() and loadBaseAssets() from a clean state.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/client/graphics/layers/NameLayerAssets.ts (1)
16-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop retrying permanently failed textures.
After the first
PIXI.Assets.load(src)rejection,srcis removed frompendingTexturesbut never marked as failed. Every latergetTexture()call starts a fresh request again, so a CORS-blocked flag can hammer the network every frame.♻️ Minimal fix
private readonly atlasTextures = new Map<string, PIXI.Texture>(); private readonly emojiTextures = new Map<string, PIXI.Texture>(); private readonly pendingTextures = new Map<string, Promise<void>>(); + private readonly failedTextures = new Set<string>(); private readonly warnedTextureFailures = new Set<string>(); private readonly warnedMissingEmojis = new Set<string>(); @@ const cached = this.textures.get(src); if (cached) { return cached; } + if (this.failedTextures.has(src)) { + return null; + } if (!this.pendingTextures.has(src)) { this.pendingTextures.set( src, PIXI.Assets.load(src) .then((texture: PIXI.Texture) => { this.textures.set(src, texture); }) .catch((error) => { - this.textures.delete(src); + this.failedTextures.add(src); this.warnTextureFailure(src, error); }) .finally(() => { this.pendingTextures.delete(src); }), @@ resetWarningsForTests(): void { + this.failedTextures.clear(); this.warnedTextureFailures.clear(); this.warnedMissingEmojis.clear(); }Also applies to: 40-53, 78-81
🤖 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/graphics/layers/NameLayerAssets.ts` around lines 16 - 21, The code currently removes a failed load from pendingTextures but never records it as permanently failed, so subsequent getTexture calls retry endlessly; update the texture-loading path (the function that calls PIXI.Assets.load(src) and the consumer getTexture/getOrLoad methods) to add the src to the warnedTextureFailures (or a new failedTextures set) on rejection and short-circuit future loads by checking that set before starting a new PIXI.Assets.load; also ensure pendingTextures is cleared on reject and that any callers use the failed set to return the fallback texture or undefined instead of re-issuing network requests.
🤖 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/graphics/layers/NameLayer.ts`:
- Around line 117-118: The AlternateViewEvent handler is currently registered
inline and never unsubscribed; create a class property (e.g., private readonly
onAlternateViewHandler = (e: AlternateViewEvent) =>
this.onAlternateViewChange(e)) and use that when calling
this.eventBus.on(AlternateViewEvent, this.onAlternateViewHandler); then in
destroy() call this.eventBus.off(AlternateViewEvent,
this.onAlternateViewHandler) alongside the existing
window.removeEventListener("resize", this.onWindowResize) to properly
unsubscribe and prevent the old instance from mutating state after teardown.
- Around line 107-122: The async NameLayer.init() is not being awaited in
GameRenderer.initialize(), allowing tick() to run before
assets.preload()/setupRenderer complete and causing createPlayerRender() to
return null (assets.fontReady false) and players to remain only in seenPlayers
without being added to renders; fix by making GameRenderer.initialize() async
and awaiting every layer init (including NameLayer.init()), or alternatively
refactor init flow so asynchronous work (assets.preload(), setupRenderer()) is
moved out of init into an explicit async prepare() that
GameRenderer.initialize() awaits; ensure tick() only starts after awaiting these
(so createPlayerRender(), assets.fontReady, seenPlayers→renders population and
redraw() happen reliably).
---
Duplicate comments:
In `@src/client/graphics/layers/NameLayerAssets.ts`:
- Around line 16-21: The code currently removes a failed load from
pendingTextures but never records it as permanently failed, so subsequent
getTexture calls retry endlessly; update the texture-loading path (the function
that calls PIXI.Assets.load(src) and the consumer getTexture/getOrLoad methods)
to add the src to the warnedTextureFailures (or a new failedTextures set) on
rejection and short-circuit future loads by checking that set before starting a
new PIXI.Assets.load; also ensure pendingTextures is cleared on reject and that
any callers use the failed set to return the fallback texture or undefined
instead of re-issuing network requests.
🪄 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: b3f5d20d-f2cc-487a-a8a3-5706fcab8b35
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonresources/fonts/namelayer_overpass.pngis excluded by!**/*.pngresources/images/namelayer-emojis.pngis excluded by!**/*.pngresources/images/namelayer-icons.pngis excluded by!**/*.png
📒 Files selected for processing (9)
package.jsonresources/fonts/namelayer_overpass.xmlresources/images/namelayer-emojis.jsonresources/images/namelayer-icons.jsonscripts/build-namelayer-assets.mjssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/NameLayerAssets.tssrc/client/graphics/layers/NameLayerLayout.tstests/NameLayer.test.ts
✅ Files skipped from review due to trivial changes (3)
- resources/images/namelayer-emojis.json
- resources/images/namelayer-icons.json
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/NameLayer.test.ts
- src/client/graphics/layers/NameLayerLayout.ts
Replace node-canvas in the namelayer asset builder with skia-canvas so icon and emoji atlases are rasterized through the same cross-platform rendering backend. This removes the previous dependency on host Cairo/Pango emoji rendering, which could produce monochrome emoji sprites depending on the local font stack. Add twemoji-colr-font as a pinned COLR/CPAL emoji font input for the build. The emoji atlas now renders from that explicit font instead of whichever OS emoji font happens to be installed, making the generated sprite sheet stable across developer machines and CI. Generalize atlas frame packing for both SVG icons and emoji glyphs: render into a scratch canvas, trim to alpha bounds, center, and scale into the cell with consistent padding. This prevents icon frames from touching cell edges while preserving the existing Pixi atlas JSON contract. Regenerate namelayer emoji and icon atlases. The builder now validates generated atlas frames and fails if any frame is empty or if the emoji atlas contains no color pixels, catching the monochrome-regeneration failure at build time.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/build-namelayer-assets.mjs (2)
360-360: 💤 Low valueConsider naming the color detection threshold.
The magic number
12on line 360 is used to detect whether pixels are grayscale or colored (via RGB range). A named constant would make this threshold more explicit and maintainable.♻️ Named constant for threshold
Near the top of the file, add:
+const colorDetectionThreshold = 12; // RGB range threshold to distinguish color from grayscaleThen use it in the validation:
- if (Math.max(r, g, b) - Math.min(r, g, b) > 12) { + if (Math.max(r, g, b) - Math.min(r, g, b) > colorDetectionThreshold) {🤖 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 `@scripts/build-namelayer-assets.mjs` at line 360, Replace the magic number 12 used to detect colored vs grayscale pixels with a named constant to improve readability and maintainability: declare a top-of-file constant like COLOR_RANGE_THRESHOLD (or COLOR_DETECTION_THRESHOLD) set to 12, then update the condition in the color-detection check (the if using Math.max(r, g, b) - Math.min(r, g, b)) to compare against that constant instead of the literal 12.
189-199: ⚡ Quick winSimplify viewBox parsing for clarity.
The current destructuring pattern with
?? []fallback works, but the flow from regex match to fallback values is indirect. Consider extracting the viewBox match separately to make the logic more explicit.♻️ Clearer viewBox parsing
let svg = fs.readFileSync(sourcePath, "utf8"); if (!/<svg[^>]*\swidth=/i.test(svg) || !/<svg[^>]*\sheight=/i.test(svg)) { - const [, , , width, height] = - svg.match( - /viewBox=["']\s*([-\d.]+)\s+([-\d.]+)\s+([-\d.]+)\s+([-\d.]+)\s*["']/i, - ) ?? []; + const viewBoxMatch = svg.match( + /viewBox=["']\s*([-\d.]+)\s+([-\d.]+)\s+([-\d.]+)\s+([-\d.]+)\s*["']/i, + ); + const width = viewBoxMatch?.[3]; + const height = viewBoxMatch?.[4]; svg = svg.replace( /<svg\b/i, `<svg width="${width ?? 64}" height="${height ?? 64}"`, ); }🤖 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 `@scripts/build-namelayer-assets.mjs` around lines 189 - 199, The viewBox extraction is indirect and hard to read; assign the regex result to a variable (e.g., const viewBoxMatch = svg.match(/viewBox=.../i)), check if viewBoxMatch exists, then pull width/height from the explicit capture indices (e.g., viewBoxMatch[3] and viewBoxMatch[4]) with fallback defaults before calling svg.replace; update the logic around the existing svg, sourcePath variables and the viewBox regex so width/height are clearly derived and then injected into the <svg ...> tag.
🤖 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 `@scripts/build-namelayer-assets.mjs`:
- Line 54: The call to FontLibrary.use is passing emojiFontPath as a string but
the API expects an array of paths; update the invocation that references
FontLibrary.use(emojiFontFamily, emojiFontPath) to pass an array containing
emojiFontPath (i.e., use emojiFontPath wrapped in an array) so the second
argument conforms to the FontLibrary.use(familyName, arrayOfPaths) signature.
---
Nitpick comments:
In `@scripts/build-namelayer-assets.mjs`:
- Line 360: Replace the magic number 12 used to detect colored vs grayscale
pixels with a named constant to improve readability and maintainability: declare
a top-of-file constant like COLOR_RANGE_THRESHOLD (or COLOR_DETECTION_THRESHOLD)
set to 12, then update the condition in the color-detection check (the if using
Math.max(r, g, b) - Math.min(r, g, b)) to compare against that constant instead
of the literal 12.
- Around line 189-199: The viewBox extraction is indirect and hard to read;
assign the regex result to a variable (e.g., const viewBoxMatch =
svg.match(/viewBox=.../i)), check if viewBoxMatch exists, then pull width/height
from the explicit capture indices (e.g., viewBoxMatch[3] and viewBoxMatch[4])
with fallback defaults before calling svg.replace; update the logic around the
existing svg, sourcePath variables and the viewBox regex so width/height are
clearly derived and then injected into the <svg ...> tag.
🪄 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: c0aa621b-d7d3-4081-b9f3-bc7c63aa05c7
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonresources/images/namelayer-emojis.pngis excluded by!**/*.pngresources/images/namelayer-icons.pngis excluded by!**/*.png
📒 Files selected for processing (2)
package.jsonscripts/build-namelayer-assets.mjs
✅ Files skipped from review due to trivial changes (1)
- package.json
Make layer initialization awaitable and have GameRenderer wait for each layer init before input and worker ticks start. This prevents the Pixi NameLayer from seeing players before its async font, atlas, and renderer setup has completed. Also store the AlternateViewEvent handler on NameLayer so destroy() can unsubscribe it, matching the existing resize listener cleanup and preventing torn-down instances from mutating state.
Pass the pinned Twemoji font path to FontLibrary.use as an explicit path array, name the color-detection threshold used by atlas validation, and make SVG viewBox width/height extraction easier to read. Validated with the namelayer asset build and eslint for scripts/build-namelayer-assets.mjs.
Contributor
There was a problem hiding this comment.
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 `@src/client/ClientGameRunner.ts`:
- Around line 413-415: After awaiting this.renderer.initialize() in the start
flow, add a guard that checks the instance's isActive flag (or equivalent) and
bail out early if false so subsequent calls to this.input.initialize() and
this.worker.start(...) (and any transport rejoin logic inside start) are not
executed; specifically, after await this.renderer.initialize() check
this.isActive (or this.active) and return immediately when it's false to prevent
rejoining/stopping races with stop().
🪄 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: 90d6961c-9a37-44ad-8b6a-d67a3c3167d9
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonresources/images/namelayer-emojis.pngis excluded by!**/*.pngresources/images/namelayer-icons.pngis excluded by!**/*.png
📒 Files selected for processing (6)
package.jsonscripts/build-namelayer-assets.mjssrc/client/ClientGameRunner.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/Layer.tssrc/client/graphics/layers/NameLayer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/graphics/layers/Layer.ts
- package.json
- scripts/build-namelayer-assets.mjs
- src/client/graphics/layers/NameLayer.ts
Comment on lines
+413
to
415
| await this.renderer.initialize(); | ||
| this.input.initialize(); | ||
| this.worker.start((gu: GameUpdateViewData | ErrorUpdate) => { |
Contributor
There was a problem hiding this comment.
Add a stop/cancel guard after renderer init await.
At Line 413, start() can pause long enough for stop() to run. When it resumes, it still initializes input, starts worker, and rejoins transport even if isActive is already false.
💡 Suggested minimal fix
await this.renderer.initialize();
+ if (!this.isActive) {
+ return;
+ }
this.input.initialize();
this.worker.start((gu: GameUpdateViewData | ErrorUpdate) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await this.renderer.initialize(); | |
| this.input.initialize(); | |
| this.worker.start((gu: GameUpdateViewData | ErrorUpdate) => { | |
| await this.renderer.initialize(); | |
| if (!this.isActive) { | |
| return; | |
| } | |
| this.input.initialize(); | |
| this.worker.start((gu: GameUpdateViewData | ErrorUpdate) => { |
🤖 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/ClientGameRunner.ts` around lines 413 - 415, After awaiting
this.renderer.initialize() in the start flow, add a guard that checks the
instance's isActive flag (or equivalent) and bail out early if false so
subsequent calls to this.input.initialize() and this.worker.start(...) (and any
transport rejoin logic inside start) are not executed; specifically, after await
this.renderer.initialize() check this.isActive (or this.active) and return
immediately when it's false to prevent rejoining/stopping races with stop().
Keep the NameLayer emoji atlas generation deterministic by loading Twemoji COLR from resources/fonts instead of resolving it through the twemoji-colr-font npm package. The font file and its OFL license are now checked in next to the other namelayer font assets. This removes the deprecated package dependency while preserving the current Twemoji output until the project chooses a final emoji font/set.
Contributor
There was a problem hiding this comment.
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 `@resources/fonts/twemoji-colr-OFL.txt`:
- Around line 1-3: The file twemoji-colr-OFL.txt currently only contains the SIL
Open Font License heading; add the verbatim upstream copyright attribution lines
and any reserved font name notices from the original upstream package into this
file (i.e., copy the exact copyright holder names, years, and reserved name text
from the source repo/package) so redistribution is compliant; update both the
top block (where the SIL OFL is present) and the other referenced region around
lines 91-93 to include those verbatim attribution notices.
🪄 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: b0f1532b-4421-44f8-ae6c-dc8ef9ea133a
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonresources/fonts/twemoji-colr.woff2is excluded by!**/*.woff2
📒 Files selected for processing (3)
package.jsonresources/fonts/twemoji-colr-OFL.txtscripts/build-namelayer-assets.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- scripts/build-namelayer-assets.mjs
Comment on lines
+1
to
+3
| # License for the font file | ||
|
|
||
| SIL OPEN FONT LICENSE |
Contributor
There was a problem hiding this comment.
Add explicit upstream copyright attribution text (not only license bodies/links).
This file includes license text and links, but it does not clearly include the actual copyright-owner attribution lines for the vendored font/artwork. That can create a redistribution compliance gap. Please copy the exact upstream attribution notices (copyright holder names/years and any reserved font name notice) into this file verbatim from the source package/repo.
Also applies to: 91-93
🤖 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 `@resources/fonts/twemoji-colr-OFL.txt` around lines 1 - 3, The file
twemoji-colr-OFL.txt currently only contains the SIL Open Font License heading;
add the verbatim upstream copyright attribution lines and any reserved font name
notices from the original upstream package into this file (i.e., copy the exact
copyright holder names, years, and reserved name text from the source
repo/package) so redistribution is compliant; update both the top block (where
the SIL OFL is present) and the other referenced region around lines 91-93 to
include those verbatim attribution notices.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Draft PR for replacing the DOM-based
NameLayerwith a Pixi-native implementation.This renders player names, troop counts, flags, alliance/status icons, and emoji through an offscreen Pixi renderer and composites the result into the main canvas, matching the existing
StructureIconsLayerpattern.The branch also adds deterministic generated assets for the Pixi name layer: an MSDF Overpass bitmap font, icon atlases, and an emoji atlas.
What Changed
NameLayerrendering with Pixi containers and offscreen renderer compositing.GameRendererlayer order.getPlayerIcons()as the single source for player icon eligibility.npm run build:namelayer-assets.resources.node-canvasin the asset generation pipeline withskia-canvas.twemoji-colr-fontas a pinned COLR/CPAL emoji font input for emoji atlas generation.Font / Emoji Asset Notes
The asset build now uses:
skia-canvasas the raster backend for generated icon and emoji atlases.twemoji-colr-fontas the current pinned color emoji font source.The reason for introducing
skia-canvasand a pinned emoji font is to avoid platform-dependent emoji rendering.The exact emoji font and emoji set choice should still be reviewed before merging.
twemoji-colr-fontis used here because it gives deterministic color emoji rendering from a font in the build pipeline, but the project may still want to choose a different source, vendor the font directly underresources/fonts, or align the emoji style with another product/design decision.Known Limitations / Draft Notes
?and warn once.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME