Add Cosmetic Pack System (Initial Implementation with Test Pack) #2124
Conversation
WalkthroughAdds a player cosmetics “pack” across client, server, schemas, and graphics. Extends lobby/join payloads and user settings with many new structure/sprite fields. Introduces URL resolution via fetchUrl. Graphics layers and sprite loader now resolve assets from the player pack and lazy-load per player during the first tick. New cosmetic manifest schema added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant US as UserSettings
participant C as Client (Main/SinglePlayerModal)
participant T as Transport
participant S as Server Worker
participant G as Game (Client)
participant UL as UnitLayer
participant SL as StructureLayer
participant SLdr as SpriteLoader
U->>US: Read selected pack ids
C->>T: joinLobby({ cosmetics: { flag, pattern, pack refs } })
T->>S: Send ClientJoinMessage
S->>S: checkCosmetics(pack refs)
S->>S: Resolve URLs via fetchUrl(...)
S-->>T: Ack with cosmetics { flag, pattern, pack }
T-->>G: Start game with player cosmetics
Note over UL,SL: First tick with myPlayer available
G->>UL: tick()
UL->>UL: if !spritesLoaded
UL->>SLdr: loadAllSprites(pack)
SLdr-->>UL: Sprites loaded (pack URLs with fallback)
G->>SL: tick()
SL->>SL: if !structureLoaded
SL->>SL: loadIconData(pack) async
SL-->>G: Structures use pack icons
sequenceDiagram
participant SLdr as SpriteLoader
participant P as PlayerPack
participant Cfg as SPRITE_CONFIG
SLdr->>Cfg: Iterate entries {key,url}
SLdr->>P: Resolve finalUrl = P[key] ?? url
alt finalUrl defined
SLdr->>SLdr: loadImage(finalUrl)
else
SLdr->>SLdr: warn and skip
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/CosmeticSchemas.ts (1)
2-2: Use consistent Zod importThe rest of the codebase imports from "zod". Importing "zod/v4" risks duplicate bundles or missing module.
-import { z } from "zod/v4"; +import { z } from "zod";src/core/Schemas.ts (1)
389-395: Constrain pack id with a reusable schema
Extract aPackIdSchemainsrc/core/Schemas.ts:const PackIdSchema = z.string().regex(/^[a-z0-9_-]{1,64}$/i, "Invalid pack id");Then replace both occurrences of
pack: z.string().optional(),in
PlayerCosmeticRefsSchemaandPlayerCosmeticsSchemawithpack: PackIdSchema.optional(),src/server/Worker.ts (1)
488-495: Sanitize pack ID before returning to clients
packis direct user input and isn’t validated incheckCosmeticsor in thePlayerCosmeticsSchema(src/core/Schemas.ts). This can produce invalid URLs and log spam.– In Worker.ts’s
checkCosmetics, replace the raw passthrough with:cosmetics: { flag: cosmetics.flag, pattern, - pack: cosmetics.pack, + pack: /^[A-Za-z0-9_-]{1,64}$/.test(cosmetics.pack ?? '') + ? cosmetics.pack + : undefined, },– In src/core/Schemas.ts, tighten the schema:
pack: z.string() .regex(/^[A-Za-z0-9_-]{1,64}$/, 'Invalid pack ID') .optional(),These changes ensure only short, URL-safe IDs are accepted.
🧹 Nitpick comments (5)
src/client/graphics/layers/UnitLayer.ts (1)
39-41: Load sprites once per pack id; avoid double loadsCurrently loads in init (default) and again in tick (pack). Deduplicate and reload only when pack id changes.
Apply:
export class UnitLayer implements Layer { @@ - private packId: string | undefined = undefined; - private spritesLoaded = false; + private packId: string | undefined = undefined; + private lastLoadedPackId: string | null = null; @@ - if (!this.spritesLoaded) { - const myPlayer = this.game.myPlayer(); - if (myPlayer) { - this.packId = myPlayer.cosmetics.pack; - loadAllSprites(this.packId); - this.spritesLoaded = true; - } - } + const currentPackId = this.game.myPlayer()?.cosmetics.pack; + if ((currentPackId ?? null) !== this.lastLoadedPackId) { + this.packId = currentPackId; + void loadAllSprites(this.packId); // fire-and-forget; loader should cache + this.lastLoadedPackId = currentPackId ?? null; + } @@ - loadAllSprites(this.packId); + // Sprite loading is handled in tick() when packId is known.Optional: if
loadAllSpritesis not idempotent, add memoization there keyed bypackId.Also applies to: 74-81, 89-90
src/client/graphics/SpriteLoader.ts (1)
63-97: Optional: simplify image loading with decode() and reduce logsUse img.decode() instead of onload Promise boilerplate; avoid noisy “All sprites loaded” counter.
- await new Promise<void>((resolve, reject) => { - img.onload = () => resolve(); - img.onerror = (err) => reject(err); - }); + await img.decode(); @@ - loadedCount++; - - if (loadedCount === totalSprites) { - console.log("All sprites loaded."); - } + // optionally track progress via a debug metric instead of console spamsrc/client/CosmeticPackLoader.ts (1)
49-53: Harden traversal loop for non-object leavesBreak if current is null or not an object; avoids runtime errors on malformed nodes.
- for (const part of parts) { - if (current === null) break; - current = current[part]; - } + for (const part of parts) { + if (current == null || typeof current !== "object") { + current = undefined; + break; + } + current = (current as Record<string, unknown>)[part]; + }src/client/graphics/layers/StructureLayer.ts (2)
86-87: Avoid double icon loads; gate by packIdConstructor calls loadIconData once (base), then tick calls it again after packId arrives. This duplicates work and downloads. Cache the last applied pack and no-op if unchanged.
Example change:
- private structureLoaded = false; + private structureLoaded = false; + private iconsAppliedForPack?: string; // "base" when undefined @@ - this.loadIconData(); + // Defer until pack is known via tick to avoid duplicate loads. @@ private async loadIconData() { - await this.applyCosmeticIcons(); + const current = this.packId ?? "base"; + if (this.iconsAppliedForPack === current) return; + await this.applyCosmeticIcons(); + this.iconsAppliedForPack = current; // Optionally clear to free memory, or keep to avoid flicker: // this.unitIcons.clear(); for (const [unitTypeStr, config] of Object.entries(this.unitConfigs)) { if (!config) continue; this.loadIcon(Number(unitTypeStr) as UnitType, config); } } @@ - this.loadIconData(); + void this.loadIconData();Also applies to: 122-129, 103-108
136-185: Reduce repetition in applyCosmeticIcons with a simple data tableThe six almost-identical blocks are error-prone. Drive them from a small array of tuples.
Sketch:
const ICON_SPECS: Array<[UnitType, string, string]> = [ [UnitType.Port, "structure/img/port", anchorIcon], [UnitType.City, "structure/img/city", cityIcon], [UnitType.Factory, "structure/img/factory", factoryIcon], [UnitType.MissileSilo, "structure/img/missilesilo", missileSiloIcon], [UnitType.DefensePost, "structure/img/defensepost", shieldIcon], [UnitType.SAMLauncher, "structure/img/samlauncher", SAMMissileIcon], ]; private async applyCosmeticIcons() { await Promise.all( ICON_SPECS.map(async ([t, key, fallback]) => { this.unitConfigs[t] = { ...this.unitConfigs[t]!, icon: await resolveCosmeticUrl(this.packId, key, fallback), }; }), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
resources/cosmetics/cosmetic_pack/test/sprites/test.pngis excluded by!**/*.pngresources/cosmetics/cosmetic_pack/test/structure/test.pngis excluded by!**/*.png
📒 Files selected for processing (13)
resources/cosmetics/cosmetic_pack/test/manifest.json(1 hunks)src/client/ClientGameRunner.ts(1 hunks)src/client/CosmeticPackLoader.ts(1 hunks)src/client/Main.ts(1 hunks)src/client/SinglePlayerModal.ts(2 hunks)src/client/Transport.ts(1 hunks)src/client/graphics/SpriteLoader.ts(2 hunks)src/client/graphics/layers/StructureLayer.ts(4 hunks)src/client/graphics/layers/UnitLayer.ts(2 hunks)src/core/CosmeticSchemas.ts(1 hunks)src/core/Schemas.ts(2 hunks)src/core/game/UserSettings.ts(1 hunks)src/server/Worker.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/client/CosmeticPackLoader.ts (1)
src/core/CosmeticSchemas.ts (2)
CosmeticManifest(125-125)CosmeticManifestSchema(112-124)
src/client/graphics/layers/StructureLayer.ts (2)
src/core/game/GameView.ts (1)
myPlayer(568-570)src/client/CosmeticPackLoader.ts (1)
resolveCosmeticUrl(25-62)
src/client/graphics/layers/UnitLayer.ts (2)
src/core/game/GameView.ts (1)
myPlayer(568-570)src/client/graphics/SpriteLoader.ts (1)
loadAllSprites(56-98)
src/client/graphics/SpriteLoader.ts (1)
src/client/CosmeticPackLoader.ts (1)
resolveCosmeticUrl(25-62)
🔇 Additional comments (8)
src/client/Main.ts (1)
519-519: LGTM! Pack field integration follows existing pattern.The pack field is correctly threaded through the lobby join flow, consistent with how flag and pattern are handled.
src/client/Transport.ts (1)
384-384: LGTM! Pack correctly propagated to join message.The pack field is appropriately included in the cosmetics payload sent to the server.
src/client/ClientGameRunner.ts (1)
55-55: LGTM! Clean type definition for pack field.The optional pack field is correctly typed and fits naturally into the LobbyConfig interface.
src/core/game/UserSettings.ts (1)
158-160: LGTM! Simple and consistent localStorage accessor.The method follows the existing pattern in UserSettings and correctly returns the pack ID or undefined.
src/client/SinglePlayerModal.ts (2)
451-451: LGTM! Pack retrieval follows established pattern.The pack ID is retrieved from user settings consistently with how the pattern is handled.
470-470: LGTM! Pack correctly included in single-player join payload.The pack field is appropriately added to the cosmetics object with proper undefined fallback.
resources/cosmetics/cosmetic_pack/test/manifest.json (1)
1-29: Assets keys and files verifiedAll sprite and structure keys match the loader’s expectations, and both
sprites/test.pngandstructure/test.pngexist in the pack. No further changes needed.src/client/graphics/SpriteLoader.ts (1)
56-58: No action needed: call sites already pass thepackIdparameter.Likely an incorrect or invalid review comment.
Aotumuri
dismissed stale reviews from coderabbitai[bot] and Duwibi
via
114ae7f
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/Schemas.ts (1)
389-411: Eliminate duplication: extract shared cosmetic fields into a base schema.
PlayerPackSchema(lines 419-437) duplicates the same 17 structure and sprite fields already added toPlayerCosmeticRefsSchema(lines 394-410). This violates DRY and means future cosmetic fields must be added in two places.Extract the common fields:
+const CosmeticAssetFieldsSchema = z.object({ + structurePort: z.string().optional(), + structureCity: z.string().optional(), + structureFactory: z.string().optional(), + structureMissilesilo: z.string().optional(), + structureDefensepost: z.string().optional(), + structureSamlauncher: z.string().optional(), + spriteTransportship: z.string().optional(), + spriteWarship: z.string().optional(), + spriteSammissile: z.string().optional(), + spriteAtombomb: z.string().optional(), + spriteHydrogenbomb: z.string().optional(), + spriteTradeship: z.string().optional(), + spriteMirv: z.string().optional(), + spriteEngine: z.string().optional(), + spriteCarriage: z.string().optional(), + spriteLoadedcarriage: z.string().optional(), +}); + export const PlayerCosmeticRefsSchema = z.object({ flag: FlagSchema.optional(), patternName: PatternNameSchema.optional(), patternColorPaletteName: z.string().optional(), - - structurePort: z.string().optional(), - // ... rest of duplicate fields -}); +}).merge(CosmeticAssetFieldsSchema); -export const PlayerPackSchema = z.object({ - structurePort: z.string().optional(), - // ... rest of duplicate fields -}); +export const PlayerPackSchema = CosmeticAssetFieldsSchema; export type PlayerPack = z.infer<typeof PlayerPackSchema>;Also applies to: 419-437
♻️ Duplicate comments (2)
src/client/graphics/layers/StructureLayer.ts (1)
36-116: Fix icon map to useUnitTypekeysRight now we store icons in
unitIconswith string keys (fromObject.entries) but later we read them withUnitTypeenum values (numbers). The lookups always miss, so every structure falls back to a blank icon both for base art and packs. Please key the map byUnitTypeand convert the entry key before storing.Apply this diff:
- private unitIcons: Map<string, HTMLImageElement> = new Map(); + private unitIcons: Map<UnitType, HTMLImageElement> = new Map(); @@ - private loadIcon(unitType: string, config: UnitRenderConfig) { + private loadIcon(unitType: UnitType, config: UnitRenderConfig) { @@ - Object.entries(this.unitConfigs).forEach(([unitType, config]) => { - config.icon = this.pack?.[config.key] ?? config.icon; - this.loadIcon(unitType, config); - }); + for (const [unitTypeKey, config] of Object.entries(this.unitConfigs)) { + if (!config) continue; + const unitType = Number(unitTypeKey) as UnitType; + config.icon = this.pack?.[config.key] ?? config.icon; + this.loadIcon(unitType, config); + }src/client/graphics/SpriteLoader.ts (1)
26-51: Past critical issue remains unresolved: enum key type mismatch will break sprite lookups.The previous review identified that
Object.entriesconverts numericUnitTypeenum keys to strings at runtime, butspriteMaplookups use numeric enum values directly. The type cast at line 63 doesn't fix the runtime mismatch. Non-train sprites won't resolve.Apply the diff from the previous review to fix this issue by consistently keying with strings.
Also applies to: 53-58, 64-76, 111-127
🧹 Nitpick comments (3)
src/client/graphics/SpriteLoader.ts (1)
26-51: ConstrainkeytoPlayerPackkeys for compile-time safety.The
keyfield is currently untypedstring. If you typo a key name, the error only appears at runtime when the pack lookup silently fails. Constrain it to validPlayerPackkeys:const SPRITE_CONFIG: Partial< - Record<UnitType | TrainTypeSprite, { key: string; url: string }> + Record<UnitType | TrainTypeSprite, { key: keyof PlayerPack; url: string }> > = {This ensures every key matches a field in
PlayerPackSchema.src/core/Schemas.ts (1)
394-410: Add validation for cosmetic asset URL fields.All structure and sprite fields accept any string with no length limit or format check. Since these become URLs loaded by the browser (per
SpriteLoader.ts), invalid values cause runtime errors.Add URL validation and reasonable length limits:
+const AssetUrlSchema = z.string().url().max(2048).optional(); + const CosmeticAssetFieldsSchema = z.object({ - structurePort: z.string().optional(), - structureCity: z.string().optional(), + structurePort: AssetUrlSchema, + structureCity: AssetUrlSchema, // ... apply to all asset fields });The 2048 limit accommodates data URIs while preventing abuse.
Also applies to: 427-436
src/core/game/UserSettings.ts (1)
172-234: Simplify return type:string | nullmatcheslocalStoragecontract.All getters use
?? undefinedto convertlocalStorage.getItem'snulltoundefined, but the return type could bestring | nullinstead. Both are falsy; the distinction rarely matters to callers.- getSelectedStructurePort(): string | undefined { - return localStorage.getItem("structurePort") ?? undefined; + getSelectedStructurePort(): string | null { + return localStorage.getItem("structurePort"); }This removes 16 unnecessary coalescing operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/test/test.pngis excluded by!**/*.png
📒 Files selected for processing (11)
src/client/ClientGameRunner.ts(1 hunks)src/client/CosmeticPackLoader.ts(1 hunks)src/client/Main.ts(1 hunks)src/client/SinglePlayerModal.ts(2 hunks)src/client/Transport.ts(1 hunks)src/client/graphics/SpriteLoader.ts(2 hunks)src/client/graphics/layers/StructureLayer.ts(6 hunks)src/client/graphics/layers/UnitLayer.ts(3 hunks)src/core/Schemas.ts(1 hunks)src/core/game/UserSettings.ts(1 hunks)src/server/Worker.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/Main.ts
- src/server/Worker.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/client/graphics/layers/UnitLayer.ts (3)
src/core/Schemas.ts (1)
PlayerPack(438-438)src/core/game/GameView.ts (1)
myPlayer(568-570)src/client/graphics/SpriteLoader.ts (1)
loadAllSprites(56-97)
src/client/SinglePlayerModal.ts (1)
src/client/CosmeticPackLoader.ts (1)
fetchUrl(1-22)
src/client/graphics/SpriteLoader.ts (1)
src/core/Schemas.ts (1)
PlayerPack(438-438)
src/client/graphics/layers/StructureLayer.ts (2)
src/core/Schemas.ts (1)
PlayerPack(438-438)src/core/game/GameView.ts (1)
myPlayer(568-570)
src/core/Schemas.ts (1)
src/core/CosmeticSchemas.ts (3)
PatternNameSchema(19-22)PatternDataSchema(24-45)ColorPaletteSchema(47-51)
Description:
This PR adds the base system for Cosmetic Packs, allowing assets like structures and unit sprites to be overridden via JSON manifests.
No UI changes in this PR — packs are only loaded if cosmeticPackId is already set in user settings.
Audio/FX support will be added in a future PR.
Permission checks and forced application of event packs are not implemented yet.
Key Points
The pack only applies to the person who owns it, and it affects the entire way things are displayed for that user.
Example:
From the perspective of someone using the pack
From the perspective of someone in the same game but not using the pack
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri