Fix missing boat sprite icon in attacks panel#4141
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughGameRenderer.ts adds controller imports to wire additional HUD layers and imports ChangesSprite Preloading in Renderer Setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| init() { | ||
| loadAllSprites().catch((err) => | ||
| console.error("Failed to preload attack display sprites:", err), | ||
| ); | ||
| } |
There was a problem hiding this comment.
maybe instead we should move it to GameRenderer.initialize(), since multiple layers use sprites.
There was a problem hiding this comment.
good call - done :)
Call loadAllSprites() from AttacksDisplay.init() so the sprite map is populated at startup. It was previously loaded by a deleted canvas layer, leaving the map empty and causing boat row icons to render blank. Fixes openfrontio#4100
84d7787 to
e43cccb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/hud/GameRenderer.ts (1)
352-354: 💤 Low valueConsider awaiting sprite preload for deterministic startup order.
The current fire-and-forget approach degrades gracefully (render paths retry and cache on success), but you could make the startup sequence more predictable:
await loadAllSprites().catch((err) => console.error("Failed to preload sprites:", err), );This would guarantee sprites are loaded before
layer.init()runs. The tradeoff is that initialization blocks until all sprites load, which could delay the first frame. Since your tests confirm the current approach works, this is optional—but it removes the timing window where icons might briefly appear blank.🤖 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/hud/GameRenderer.ts` around lines 352 - 354, Change the fire-and-forget sprite preload to an awaited preload before initializing layers: call and await loadAllSprites() (retaining the existing .catch logging) and only after it resolves call layer.init() so that sprite assets are guaranteed loaded before layer.init() runs; update the startup sequence where loadAllSprites() is invoked (and where layer.init() is called) to reflect this await and ensure errors from loadAllSprites() are still logged.
🤖 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.
Nitpick comments:
In `@src/client/hud/GameRenderer.ts`:
- Around line 352-354: Change the fire-and-forget sprite preload to an awaited
preload before initializing layers: call and await loadAllSprites() (retaining
the existing .catch logging) and only after it resolves call layer.init() so
that sprite assets are guaranteed loaded before layer.init() runs; update the
startup sequence where loadAllSprites() is invoked (and where layer.init() is
called) to reflect this await and ensure errors from loadAllSprites() are still
logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc107810-af75-478a-85bc-61e24b5db33c
📒 Files selected for processing (1)
src/client/hud/GameRenderer.ts
Resolves #4100
Description:
The boat row in the attacks panel (bottom-right UI) rendered an empty slot where the tinted boat sprite icon should appear, for both incoming and outgoing transport boats.
Root cause:
loadAllSprites()inSpriteLoader.tswas never called. It was previously invoked by a canvas layer that has since been deleted, so the sprite map stayed empty. As a resultgetColoredSprite()threw,AttacksDisplay.getBoatSpriteDataURL()caught the error and returned"", and the icon rendered blank.This fix calls
loadAllSprites()fromAttacksDisplay.init()(currently the only consumer of the sprite loader), so the sprite map is populated at startup.Demo after fix:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
cool_clarky