Skip to content

Chess corner behind the lounge couch: sit down and play an idle agent - #300

Open
iptoux wants to merge 2 commits into
AgentSystemLabs:mainfrom
iptoux:office/nibble-a12a
Open

iptoux wants to merge 2 commits into
AgentSystemLabs:mainfrom
iptoux:office/nibble-a12a

Conversation

@iptoux

@iptoux iptoux commented Oct 5, 2026

Copy link
Copy Markdown

A chess corner in the office lounge: a modern table with two chairs behind the couch (room side, x 7-9, z 0), a teal-and-foam 3D board on it, E on a chair sits down, E again opens the chess window.

What was built

  • Furniture (src/client/world/office/props.ts, lounge fixture in room.ts): code-built table + two chairs (toon/roundedBox, orange/teal) with colliders + seatables; decorative mid-game board prop.
  • Sitting: chess-chair-1/2 in SEATING (face each other, stool-like hips/depth/out, new SeatDef.chess), nav obstacles in shared/nav.ts (walkways verified walkable, routes from elevator/spawn reach both chairs).
  • Opening the game extends the seating-deps precedent (like arcade/bar): SeatingDeps.chess, wired in main.ts/parts.ts; new src/client/features/chess/ with installChess.
  • Engine (engine.ts, pure TS): legal moves incl. castling/en-passant/promotion, check/checkmate/stalemate, fifty-move/material/threefold draws, SAN move list.
  • Opponent: idle (status === 'idle') agent workers offered by name; greedy 1-ply AI replies (never misses mate in 1). Solo both-sides fallback when nobody is idle.
  • Window: board (own side at bottom), turn/check status, captured pieces, move list, promotion picker, New game/Resign; plain openModal so X/Esc return straight to mouse-look. Sound recipe + OfficeSound.chess, HELP row, docs (features/controls/README).

How it was checked

  • npm run typecheck, npm test (620/620 incl. 14 new chess tests), npm run build all pass.
  • Headless Chromium against a live office (scripted real key/aim path): teleport behind couch, hint offers chess chair, E sits (chess-chair-1:0), E opens window, played 1. e4 e5, Esc closes; injected an idle agent, picker lists it, AI replied 1...Nc6/Nf6, resign/new-game/X all verified, zero console errors.
  • Screenshots: chess corner in the lounge, open chess window, game vs idle agent, furniture close-up (props lab).
  • Left out (nice-to-have): seating the opponent agent's 3D character on the free chair — workers sit at desks via server state, so that would desync; the window names them instead.

A little table with two modern chairs behind the couch (room side),
a teal-and-foam board on it, and E on a chair to sit, E again to play.
Full rules (castling, en-passant, promotion, check/mate/stalemate,
draws, SAN list), a greedy 1-ply for the idle-agent opponents, and a
window with turn, captured pieces, new-game/resign.

@iptoux iptoux left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #300 — Chess corner behind the lounge couch

Summary and verdict

Solid, well-tested PR (engine + greedy AI + window + furniture/seating/nav wiring). Security found nothing. No blockers, but please fix the resign-during-AI-think race before merging — the board mutates underneath a final result. The engine hot-path items (3–4× move generation per move, fanned out ~30× per AI reply on the main thread) are worth addressing while the code is fresh; everything else is nits. Verdict: fix the race, then good to merge; rest can follow.

Findings (most serious first)

  • [Correctness] src/client/features/chess/ui.ts:280 (resign handler): resigning while the AI is thinking does not clear the pending AI timer (start() does, this doesn't), and the timer callback never checks over — so ~0.5–1s later the AI's move is played onto the board underneath the final "X resigns — Y wins!" banner (board, move list and captured rows mutate after the result). Clear the timer and set thinking = false in the resign handler (mirror start()), and/or guard the maybeAi timeout callback with if (!this.modal || over).
  • [Performance] src/client/features/chess/engine.ts:428 (play → allMoves + sanFor → allMoves + status → allMoves): one played move runs full legal-move generation 3–4×, and each legalFrom clones the 64-board per pseudo-move. chooseMove (ai.ts:149) multiplies this (~30 clones × plays per AI reply, all on the main thread). Fix: generate the legal list once per position and thread it through (play(found), sanFor(list), status(list)), or at least cache it per turn.
  • [Performance] src/client/world/office/props.ts:1327 (chessBoard): 64 individually-allocated BoxGeometry + ~75 meshes (frame, squares, pieces) = ~75 draw calls for one decorative prop, rendered on every floor (plus lab preview). Legs/table/chairs also allocate fresh geometries per instance. Fix: share two square geometries/materials (or mergeByMaterial/mergeByColor like the rest of the room) and share leg/pawn geometries.
  • [Performance] src/client/features/chess/ui.ts:909 (refresh): rebuilds all 64 squares' DOM (textContent='' + new span), the whole moves <ol>, and re-sorts captured pieces on every click and on the thinking toggle; targets.some(...) per cell makes it O(64×targets). move (ui.ts:977) also calls game.status() again after play()/resultText() already computed it. Fix: cache one status() per refresh, use a Set for target squares, update only changed cells.
  • [Correctness] src/client/features/chess/engine.ts:361 (key()): the repetition key includes the raw en-passant square even when no legal en-passant capture exists, so positions that are identical under the rules hash differently and a real threefold can be missed. Only include ep when an enemy pawn could actually capture en passant.
  • [Performance] src/client/features/chess/engine.ts:299 (clone): new ChessGame() runs reset() (builds the whole starting position, key() string, counts map) only to be overwritten line-by-line right after; then copies history array + counts Map per AI trial. Fix: a private constructor / Object.create path that fills fields directly without reset().
  • [Performance] src/client/features/chess/ui.ts:885 (refreshOpp): calls idleOpponents() twice per render (spread + filter + localeCompare sort of all workers each time) and rebuilds the whole <select> on every store.on('workers') event, which can steal focus mid-interaction. Fix: call once, keep the result, and skip rebuild when the opponent list is unchanged / the select has focus.
  • [Correctness] src/client/features/chess/ui.ts:107 (refreshOpp): the opponent <select> uses the agent's display name as the option value and tracks aiName only, so two idle agents with the same name are indistinguishable and the shown selection can diverge from the tracked one if the roster changes. Use the worker id as the option value and track the id (keep the name for display/nameOf).
  • [Correctness] src/client/features/chess/engine.ts:242 (castling in pseudo()): kingside/queenside are offered on rights flags plus empty squares alone, without checking a rook is actually on its corner; today the rights bookkeeping makes this consistent, but a single desync would let the king castle out of thin air (applyOn would then move null). Assert the rook is present on at(7/0, home) before offering the castle.
  • [Simplicity] src/client/features/chess/engine.ts:463 (rightsAfter): allocates the 4-entry home object on every move; hoist to module const. Same shape: slide([...DIAGONALS, ...STRAIGHTS]) (engine.ts:400) allocates per queen pseudo-move; hoist a QUEEN_DIRS const.
  • [Simplicity] src/client/features/chess/ai.ts:105 (score): status.winner === undefined && ... is always true when !status.over (winner is only set on checkmate), so the first conjunct is dead weight; drop it. center(t: string, ...) (ai.ts:125) should take PieceType, and score walks the board while material() (ai.ts:131) walks it again in the draw branch — compute material once.
  • [Simplicity] src/client/features/chess/engine.ts:312/344 (kingSquare + kingOf): kingOf is a one-line wrapper around kingSquare used in exactly one place — inline it.
  • [Simplicity] src/client/features/chess/ui.ts:863 (order): rebuilds the rank/file arrays and square list from scratch; flip never changes mid-game — compute once per play(), not per call.

…d-tracked opponents

- ui: resign clears the AI timer and thinking flag; the reply callback
  bails on over/closed so no move lands under the result banner
- engine: play threads one pre-move list (find+san) and one post-move
  list (status); sanFor/status take an optional list; clone fills
  fields directly instead of reset-then-overwrite; ep in the repetition
  key only when a pawn of the side to move could take it; castling
  needs a rook on the corner; RIGHTS_HOME and QUEEN_DIRS hoisted;
  kingOf inlined
- ai: score counts material once, drops the dead winner conjunct,
  center takes PieceType
- ui refresh: one status per paint, target Set, cells/moves/captured
  updated only when changed; opponent picker built from one list read,
  skipped when unchanged or focused; opponents tracked by worker id
  (same-name agents stay distinct); display order hoisted
- props: chess table/chairs/board share geometries and merge static
  parts by material (board ~75 meshes -> ~5)
- tests: ep-key inclusion, rook-less castle, same-name opponents
@iptoux

iptoux commented Oct 5, 2026

Copy link
Copy Markdown
Author

Addressed all 13 review findings in 2976a6f (verified in a clean worktree: npm run typecheck passes, npm test 623/623 pass incl. 3 new chess tests, npm run build passes, all touched files under the 600-line budget):

  • Resign race (blocker): resign now clears the AI timer and thinking, and the reply callback bails on over/closed — no move lands under the result banner.
  • Move-gen hot path: play threads one pre-move list (find + SAN) and one post-move list (status); sanFor/status take an optional list; clone fills fields directly instead of reset-then-overwrite.
  • Prop draw calls: chess table/chairs/board share geometries and merge static parts by material (board ~75 meshes → ~5); toon() materials were already cached.
  • Refresh churn: one status() per paint, target Set, cells/moves/captured updated only when changed; move reuses one status for result text + check sound.
  • Repetition key: ep included only when a pawn of the side to move could actually take it (plus a test).
  • Picker: single idleOpponents() read per render, rebuild skipped when the roster is unchanged or the select has focus.
  • Opponent identity: tracked by worker id, name display-only with rename-following and ghost fallback (plus a same-name test).
  • Rook check: castling needs a rook on the corner (plus a test); RIGHTS_HOME/QUEEN_DIRS hoisted; kingOf inlined; score counts material once, dead conjunct dropped, center takes PieceType; display order hoisted.

The resign race has no DOM-level test (UI timers aren't covered by the node suite) — confirmed by code path: resign clears the timer synchronously and the callback re-checks over.

@iptoux iptoux left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of PR #300 — Chess corner behind the lounge couch

Verdict: approve. No correctness or security issues found. The findings below are non-blocking performance/simplicity notes for the chess engine, AI search, and window refresh path.

Findings

  • [Performance & simplicity] src/client/features/chess/ai.ts:69-78 + src/client/features/chess/engine.ts:268,285 + src/client/features/chess/ai.ts:22: chooseMove runs ~3 full legal-move generations per candidate — play() calls allMoves() twice (line 268 for legality, line 285 for status) and score() calls status() (line 22) which generates allMoves() a third time; each generation copies a 64-array per pseudo-move in legalFrom. ~30 candidates x 3 generations of board copies on the main thread per AI reply. Reuse one legal list (pass it into status/play) and/or evaluate on a light board-only copy (applyOn on pieces, no history/counts clone, no SAN).
  • [Performance & simplicity] src/client/features/chess/engine.ts:144-145: clone() copies the full history array and counts map per AI candidate, both thrown away by the 1-ply search and growing with game length (O(game length) per candidate). Add a lightweight search clone (pieces/turn/castle/ep/half only) or apply/undo on a single board.
  • [Performance & simplicity] src/client/features/chess/engine.ts:366,289-290,347-348: key() rebuilds the 64-square string (+ rights + epKey) up to 4x per move (counts.get+set, plus status repetition check), on real and on every simulated move; bareKings filter/flatMap allocates per status() call. Compute the key once per position and thread it through, or gate repetition/material checks behind cheap preconditions.
  • [Performance & simplicity] src/client/features/chess/ui.ts:137-139: refresh() calls game.status() (a full allMoves() generation) on every refresh, including the thinking-toggle refresh where the board hasn't changed. Split the status-text update from the board update, or cache the last status per position.
  • [Performance & simplicity] src/client/features/chess/ui.ts:154-169: takenKey scans the full history via capturedBy() twice per refresh (plus a third scan + sort when it changes) even when nothing was captured, and the moves list is rebuilt from all LIs on every new move (O(n^2) DOM ops over a game). Update captured rows incrementally in move() and append one <li> per move.
  • [Performance & simplicity] src/client/features/chess/ai.ts:7 vs src/client/features/chess/ui.ts:14, and ui.ts:15 vs engine PROMOS: two piece-value tables (VALUE/VALUES) and two promotion lists (PROMO_CHOICE/PROMOS) to keep in sync; ui.ts:18-19 keeps two precomputed 64-entry board orders (ORDER_W/ORDER_B) plus a flip branch. Share one table/list (derive display values from one source, import PROMOS from the engine) and use a single order with a conditional reverse.

@iptoux

iptoux commented Oct 5, 2026

Copy link
Copy Markdown
Author

Verification pass before merge (checked the code at 2976a6f, not just the claims):

  • All 13 round-1 findings confirmed fixed in the tree: resign clears the AI timer + thinking and the reply callback bails on over/closed (ui.ts:274-320); play threads one pre-move list into sanFor and status takes an optional list; clone fills fields directly via Object.create; ep is conditional in the repetition key; rook presence is required for castling; RIGHTS_HOME/QUEEN_DIRS hoisted; score counts material once; refresh uses one status per paint with changed-cells-only updates; picker reads opponents once and skips rebuild when unchanged/focused; opponents tracked by worker id; prop geometries shared and merged.
  • Fresh verification in this checkout: npm run typecheck passes, npm test 623/623 (incl. 17/17 chess), npm run build passes. Branch is 2 commits ahead of main with no conflicts (merge-base is main tip).
  • On the remaining performance notes (merged review, approve verdict): leaving them as non-blocking. The residual costs — one extra move generation per play/score call, key() string rebuilds, one status() per refresh, full move-list rebuild — run once per human click or once per AI reply under a 450-950ms timer on 64-square boards, i.e. milliseconds. Further churn here adds risk without user-visible gain; happy to take them as follow-ups if anyone wants them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant