-
Notifications
You must be signed in to change notification settings - Fork 759
feat: show next map and mode info in public lobby #2629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ import { | |
| Quads, | ||
| Trios, | ||
| } from "../core/game/Game"; | ||
| import { GameID, GameInfo } from "../core/Schemas"; | ||
| import { GameID, GameInfo, TeamCountConfig } from "../core/Schemas"; | ||
| import { generateID } from "../core/Util"; | ||
| import { JoinLobbyEvent } from "./Main"; | ||
| import { terrainMapFileLoader } from "./TerrainMapFileLoader"; | ||
|
|
@@ -124,37 +124,44 @@ export class PublicLobby extends LitElement { | |
| // Format time to show minutes and seconds | ||
| const timeDisplay = renderDuration(timeRemaining); | ||
|
|
||
| const teamCount = | ||
| lobby.gameConfig.gameMode === GameMode.Team | ||
| ? (lobby.gameConfig.playerTeams ?? 0) | ||
| : null; | ||
|
|
||
| const maxPlayers = lobby.gameConfig.maxPlayers ?? 0; | ||
| const teamSize = this.getTeamSize(teamCount, maxPlayers); | ||
| const teamTotal = this.getTeamTotal(teamCount, teamSize, maxPlayers); | ||
| const modeLabel = this.getModeLabel( | ||
| lobby.gameConfig.gameMode, | ||
| teamCount, | ||
| teamTotal, | ||
| ); | ||
| const teamDetailLabel = this.getTeamDetailLabel( | ||
| const { | ||
| modeLabel, | ||
| teamDetailLabel, | ||
| mapName: currentMapName, | ||
| } = this.getGameDisplayDetails( | ||
| lobby.gameConfig.gameMap, | ||
| lobby.gameConfig.gameMode, | ||
| teamCount, | ||
| teamTotal, | ||
| teamSize, | ||
| lobby.gameConfig.playerTeams, | ||
| lobby.gameConfig.maxPlayers, | ||
| ); | ||
|
|
||
|
|
||
|
|
||
| const fullModeLabel = teamDetailLabel | ||
| ? `${modeLabel} ${teamDetailLabel}` | ||
| : modeLabel; | ||
|
|
||
| const mapImageSrc = this.mapImages.get(lobby.gameID); | ||
|
|
||
| // Calculate details for Next Game | ||
| const { | ||
| modeLabel: nextModeLabel, | ||
| teamDetailLabel: nextTeamDetailLabel, | ||
| mapName: nextMapName, | ||
| } = this.getGameDisplayDetails( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be a bit cleaner to pass the entire game config into getGameDisplayDetails, instead of passing in 4 arguments. |
||
| lobby.nextGameConfig?.gameMap, | ||
| lobby.nextGameConfig?.gameMode, | ||
| lobby.nextGameConfig?.playerTeams, | ||
| lobby.nextGameConfig?.maxPlayers, | ||
| ); | ||
|
|
||
| const fullNextModeLabel = nextTeamDetailLabel | ||
| ? `${nextModeLabel} ${nextTeamDetailLabel}` | ||
| : nextModeLabel; | ||
| return html` | ||
| <button | ||
| @click=${() => this.lobbyClicked(lobby)} | ||
| ?disabled=${this.isButtonDebounced} | ||
| class="isolate grid h-40 grid-cols-[100%] grid-rows-[100%] place-content-stretch w-full overflow-hidden ${this | ||
| class="isolate grid h-60 grid-cols-[100%] grid-rows-[100%] place-content-stretch w-full overflow-hidden ${this | ||
| .isLobbyHighlighted | ||
| ? "bg-gradient-to-r from-emerald-600 to-emerald-500" | ||
| : "bg-gradient-to-r from-red-800 to-red-700"} text-white font-medium rounded-xl transition-opacity duration-200 hover:opacity-90 ${this | ||
|
|
@@ -183,11 +190,7 @@ export class PublicLobby extends LitElement { | |
| <span class="text-sm text-red-800 bg-white rounded-sm px-1 mr-1" | ||
| >${fullModeLabel}</span | ||
| > | ||
| <span | ||
| >${translateText( | ||
| `map.${lobby.gameConfig.gameMap.toLowerCase().replace(/[\s.]+/g, "")}`, | ||
| )}</span | ||
| > | ||
| <span>${currentMapName}</span> | ||
| </div> | ||
| </div> | ||
|
|
||
|
|
@@ -197,6 +200,25 @@ export class PublicLobby extends LitElement { | |
| </div> | ||
| <div class="text-md font-medium text-white-400">${timeDisplay}</div> | ||
| </div> | ||
| ${lobby.nextGameConfig | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this makes the PublicLobby component too large. would it be possible to keep it the same size, and have the "up next" be a small chip on the bottom left. |
||
| ? html` <div | ||
| class="col-span-full border-t border-white/20 pt-2 mt-2 flex flex-col items-end" | ||
| > | ||
| <div | ||
| class="text-xs font-semibold uppercase tracking-wide opacity-80 mb-1" | ||
| > | ||
| ${translateText("public_lobby.up_next")} | ||
| </div> | ||
| <div | ||
| class="text-sm font-medium text-white-300 flex items-center justify-end gap-1" | ||
| > | ||
| <span class="text-xs text-red-800 bg-white rounded-sm px-1" | ||
| >${fullNextModeLabel}</span | ||
| > | ||
| <span>${nextMapName}</span> | ||
| </div> | ||
| </div>` | ||
| : ""} | ||
| </div> | ||
| </button> | ||
| `; | ||
|
|
@@ -269,6 +291,37 @@ export class PublicLobby extends LitElement { | |
| return null; | ||
| } | ||
|
|
||
| private getGameDisplayDetails( | ||
| gameMap?: string, | ||
| gameMode?: GameMode, | ||
| playerTeams?: TeamCountConfig, | ||
| maxPlayers?: number, | ||
| ) { | ||
| const teamCount = gameMode === GameMode.Team ? (playerTeams ?? 0) : null; | ||
|
|
||
| const totalMaxPlayers = maxPlayers ?? 0; | ||
| const teamSize = this.getTeamSize(teamCount, totalMaxPlayers); | ||
| const teamTotal = this.getTeamTotal(teamCount, teamSize, totalMaxPlayers); | ||
|
|
||
| const modeLabel = gameMode | ||
| ? this.getModeLabel(gameMode, teamCount, teamTotal) | ||
| : ""; | ||
|
|
||
| const teamDetailLabel = gameMode | ||
| ? this.getTeamDetailLabel(gameMode, teamCount, teamTotal, teamSize) | ||
| : null; | ||
|
|
||
| const mapName = gameMap | ||
| ? translateText(`map.${gameMap.toLowerCase().replace(/[\s.]+/g, "")}`) | ||
| : ""; | ||
|
|
||
| return { | ||
| modeLabel, | ||
| teamDetailLabel, | ||
| mapName, | ||
| }; | ||
| } | ||
|
|
||
| private lobbyClicked(lobby: GameInfo) { | ||
| if (this.isButtonDebounced) { | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,17 @@ | ||
| import { z } from "zod"; | ||
| import { GameConfigSchema } from "./Schemas"; | ||
|
|
||
| export const CreateGameInputSchema = GameConfigSchema.or( | ||
| z | ||
| .object({}) | ||
| .strict() | ||
| .transform((val) => undefined), | ||
| ); | ||
| export const CreateGameInputSchema = z | ||
| .object({ | ||
| gameConfig: GameConfigSchema, | ||
| nextGameConfig: GameConfigSchema.optional(), | ||
| }) | ||
| .or(GameConfigSchema) | ||
| .or( | ||
| z | ||
| .object({}) | ||
| .strict() | ||
| .transform((val) => undefined), | ||
| ); | ||
|
Comment on lines
+9
to
+15
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be possible to remove these two .or, instead just have: .object({ |
||
|
|
||
| export const GameInputSchema = GameConfigSchema.partial(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to modify this file at all? why not just call "gameConfig()" twice?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From delowan 3 days ago
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -60,6 +60,7 @@ const frequency: Partial<Record<GameMapName, number>> = { | |||||||||||||||||||||||||||
| interface MapWithMode { | ||||||||||||||||||||||||||||
| map: GameMapType; | ||||||||||||||||||||||||||||
| mode: GameMode; | ||||||||||||||||||||||||||||
| playerTeams?: TeamCountConfig; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const TEAM_COUNTS = [ | ||||||||||||||||||||||||||||
|
|
@@ -79,13 +80,52 @@ export class MapPlaylist { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| constructor(private disableTeams: boolean = false) {} | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public gameConfig(): GameConfig { | ||||||||||||||||||||||||||||
| const { map, mode } = this.getNextMap(); | ||||||||||||||||||||||||||||
| public gameConfig(): { | ||||||||||||||||||||||||||||
| gameConfig: GameConfig; | ||||||||||||||||||||||||||||
| nextGameConfig?: GameConfig; | ||||||||||||||||||||||||||||
| } { | ||||||||||||||||||||||||||||
| const nextItem = this.getNextMap(); | ||||||||||||||||||||||||||||
| const { map, mode } = nextItem; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| this.ensurePlaylistPopulated(); | ||||||||||||||||||||||||||||
| const upNextItem = this.mapsPlaylist[0]; | ||||||||||||||||||||||||||||
| let nextPlayerTeams: TeamCountConfig | undefined; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (upNextItem) { | ||||||||||||||||||||||||||||
| if (upNextItem.mode === GameMode.Team && !upNextItem.playerTeams) { | ||||||||||||||||||||||||||||
| upNextItem.playerTeams = this.getTeamCount(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| nextPlayerTeams = upNextItem.playerTeams; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const playerTeams = | ||||||||||||||||||||||||||||
| mode === GameMode.Team ? this.getTeamCount() : undefined; | ||||||||||||||||||||||||||||
| mode === GameMode.Team | ||||||||||||||||||||||||||||
| ? (nextItem.playerTeams ?? this.getTeamCount()) | ||||||||||||||||||||||||||||
| : undefined; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const gameConfig = this.createConfig(map, mode, playerTeams); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| let nextGameConfig: GameConfig | undefined; | ||||||||||||||||||||||||||||
| if (upNextItem) { | ||||||||||||||||||||||||||||
| nextGameConfig = this.createConfig( | ||||||||||||||||||||||||||||
| upNextItem.map, | ||||||||||||||||||||||||||||
| upNextItem.mode, | ||||||||||||||||||||||||||||
| nextPlayerTeams, | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Create the default public game config (from your GameManager) | ||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||
| gameConfig, | ||||||||||||||||||||||||||||
| nextGameConfig, | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private createConfig( | ||||||||||||||||||||||||||||
| map: GameMapType, | ||||||||||||||||||||||||||||
| mode: GameMode, | ||||||||||||||||||||||||||||
| playerTeams?: TeamCountConfig, | ||||||||||||||||||||||||||||
| ): GameConfig { | ||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||
| donateGold: mode === GameMode.Team, | ||||||||||||||||||||||||||||
| donateTroops: mode === GameMode.Team, | ||||||||||||||||||||||||||||
|
|
@@ -104,24 +144,28 @@ export class MapPlaylist { | |||||||||||||||||||||||||||
| playerTeams, | ||||||||||||||||||||||||||||
| bots: 400, | ||||||||||||||||||||||||||||
| disabledUnits: [], | ||||||||||||||||||||||||||||
| } satisfies GameConfig; | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private getTeamCount(): TeamCountConfig { | ||||||||||||||||||||||||||||
| return TEAM_COUNTS[Math.floor(Math.random() * TEAM_COUNTS.length)]; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private getNextMap(): MapWithMode { | ||||||||||||||||||||||||||||
| private ensurePlaylistPopulated() { | ||||||||||||||||||||||||||||
| if (this.mapsPlaylist.length === 0) { | ||||||||||||||||||||||||||||
| const numAttempts = 10000; | ||||||||||||||||||||||||||||
| for (let i = 0; i < numAttempts; i++) { | ||||||||||||||||||||||||||||
| if (this.shuffleMapsPlaylist()) { | ||||||||||||||||||||||||||||
| log.info(`Generated map playlist in ${i} attempts`); | ||||||||||||||||||||||||||||
| return this.mapsPlaylist.shift()!; | ||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| log.error("Failed to generate a valid map playlist"); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private getNextMap(): MapWithMode { | ||||||||||||||||||||||||||||
| this.ensurePlaylistPopulated(); | ||||||||||||||||||||||||||||
| // Even if it failed, playlist will be partially populated. | ||||||||||||||||||||||||||||
| return this.mapsPlaylist.shift()!; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+167
to
171
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-null assertion may be unsafe. Line 170 uses the non-null assertion operator ( Consider either:
🔎 Safer alternative private getNextMap(): MapWithMode {
this.ensurePlaylistPopulated();
- // Even if it failed, playlist will be partially populated.
- return this.mapsPlaylist.shift()!;
+ const next = this.mapsPlaylist.shift();
+ if (!next) {
+ throw new Error("Map playlist is empty - failed to generate valid playlist");
+ }
+ return next;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better if it said something like "Next Game"
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was what I put initially (well it was "Up next", hence the branch name). Then I realized that the button to join the game was actually "Join next Game". This would have result into two usage of "next" and they were not referring to the same game, that might have been confusing, that's why I used another name.
I even thought of changing "Join next Game" by just "Join Game" to get rid of the "next", but I didn't had the courage to change all the translations !
Or maybe I am just overthinking this, I am happy to get it back to to "Up next" or "Next Game" if you find it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer "up next"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about "Starting Soon" and then "Up Next" because I do agree having two "next"s is odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to change "Join next game" to "Join this lobby" or simply "Join this game"?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to "Join Game" and "Up next": fix displayed text