Conversation
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
left a comment
There was a problem hiding this comment.
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 checksover— 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 setthinking = falsein the resign handler (mirrorstart()), and/or guard themaybeAitimeout callback withif (!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 eachlegalFromclones 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-allocatedBoxGeometry+ ~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 (ormergeByMaterial/mergeByColorlike 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 callsgame.status()again afterplay()/resultText()already computed it. Fix: cache onestatus()per refresh, use aSetfor 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 includeepwhen an enemy pawn could actually capture en passant. - [Performance]
src/client/features/chess/engine.ts:299(clone):new ChessGame()runsreset()(builds the whole starting position,key()string, counts map) only to be overwritten line-by-line right after; then copieshistoryarray +countsMap per AI trial. Fix: a private constructor /Object.createpath that fills fields directly withoutreset(). - [Performance]
src/client/features/chess/ui.ts:885(refreshOpp): callsidleOpponents()twice per render (spread + filter +localeComparesort of all workers each time) and rebuilds the whole<select>on everystore.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 tracksaiNameonly, 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 inpseudo()): 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 (applyOnwould then movenull). Assert the rook is present onat(7/0, home)before offering the castle. - [Simplicity]
src/client/features/chess/engine.ts:463(rightsAfter): allocates the 4-entryhomeobject on every move; hoist to module const. Same shape:slide([...DIAGONALS, ...STRAIGHTS])(engine.ts:400) allocates per queen pseudo-move; hoist aQUEEN_DIRSconst. - [Simplicity]
src/client/features/chess/ai.ts:105(score):status.winner === undefined && ...is always true when!status.over(winneris only set on checkmate), so the first conjunct is dead weight; drop it.center(t: string, ...)(ai.ts:125) should takePieceType, andscorewalks the board whilematerial()(ai.ts:131) walks it again in the draw branch — compute material once. - [Simplicity]
src/client/features/chess/engine.ts:312/344(kingSquare+kingOf):kingOfis a one-line wrapper aroundkingSquareused 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;flipnever changes mid-game — compute once perplay(), 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
|
Addressed all 13 review findings in
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 |
iptoux
left a comment
There was a problem hiding this comment.
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:chooseMoveruns ~3 full legal-move generations per candidate —play()callsallMoves()twice (line 268 for legality, line 285 for status) andscore()callsstatus()(line 22) which generatesallMoves()a third time; each generation copies a 64-array per pseudo-move inlegalFrom. ~30 candidates x 3 generations of board copies on the main thread per AI reply. Reuse one legal list (pass it intostatus/play) and/or evaluate on a light board-only copy (applyOnon pieces, no history/counts clone, no SAN). - [Performance & simplicity]
src/client/features/chess/engine.ts:144-145:clone()copies the fullhistoryarray andcountsmap 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, plusstatusrepetition check), on real and on every simulated move;bareKingsfilter/flatMap allocates perstatus()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()callsgame.status()(a fullallMoves()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:takenKeyscans the full history viacapturedBy()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 inmove()and append one<li>per move. - [Performance & simplicity]
src/client/features/chess/ai.ts:7vssrc/client/features/chess/ui.ts:14, andui.ts:15vs enginePROMOS: two piece-value tables (VALUE/VALUES) and two promotion lists (PROMO_CHOICE/PROMOS) to keep in sync;ui.ts:18-19keeps two precomputed 64-entry board orders (ORDER_W/ORDER_B) plus aflipbranch. Share one table/list (derive display values from one source, importPROMOSfrom the engine) and use a single order with a conditional reverse.
|
Verification pass before merge (checked the code at 2976a6f, not just the claims):
|
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
src/client/world/office/props.ts, lounge fixture inroom.ts): code-built table + two chairs (toon/roundedBox, orange/teal) with colliders + seatables; decorative mid-game board prop.chess-chair-1/2in SEATING (face each other, stool-like hips/depth/out, newSeatDef.chess), nav obstacles inshared/nav.ts(walkways verified walkable, routes from elevator/spawn reach both chairs).SeatingDeps.chess, wired inmain.ts/parts.ts; newsrc/client/features/chess/withinstallChess.engine.ts, pure TS): legal moves incl. castling/en-passant/promotion, check/checkmate/stalemate, fifty-move/material/threefold draws, SAN move list.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.openModalso 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 buildall pass.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.