Add performance improvements documentation and game-logic enhancements

- Add IMPROVEMENTS.md with detailed analysis of performance issues and bugs - Update CLAUDE.md with negative Y overflow explanation - Update README.md with socket events documentation - Enhance game-logic.js with improved comments and validation - Improve server/index.js with better documentation and edge case handling
2026-03-24 20:46:34 +00:00
parent 1af068923b
commit aeacf9e68f
5 changed files with 335 additions and 5 deletions
@@ -0,0 +1,212 @@
+# Tetris Battle Royale - Performance Improvements and Bug Fixes
+
+This document outlines identified performance issues and bugs in the Tetris Battle Royale codebase, along with recommended fixes.
+
+---
+
+## Performance Issues
+
+### 1. **Excessive JSON Serialization** (Critical)
+**Location:** `server/index.js:577-592`, `public/js/game.js:354-370`, `renderer.js:83-149`
+
+**Problem:** The `getStates()` function uses `JSON.parse(JSON.stringify())` on every 50ms tick, serializing the entire game state for all players. With 8 players, this means serializing ~8 boards × 20 rows × 10 cols = 1600 cells per tick.
+
+**Impact:** High CPU usage, network bandwidth waste, potential lag with many players.
+
+**Fix:** Only send delta changes or use a more efficient serialization format. Consider:
+- Sending only changed fields per player
+- Batching multiple ticks into one message
+- Using a binary format like Protocol Buffers or MessagePack
+
+---
+
+### 2. **Client-Side Auto-Drop Conflicts** (Bug)
+**Location:** `public/js/game.js:348-351`
+
+**Problem:** The client has its own `update(deltaTime)` function that auto-drops pieces independently:
+
+```javascript
+if (!this.inputLocked && Date.now() - this.lastDrop >= this.dropInterval) {
+  this.drop();
+  this.lastDrop = Date.now();
+}
+```
+
+**Bug:** This conflicts with server authority. The server drops every 50ms, but the client also tries to auto-drop based on `dropInterval`. This causes visual desync.
+
+**Fix:** Remove client-side auto-drop. Client should only render what server sends.
+
+---
+
+### 3. **Ghost Piece Calculation Wasted** (Performance)
+**Location:** `server/index.js:582`
+
+**Problem:** Server sends `ghostY` to all clients every tick, but clients recalculate it in `game.js:297-305` anyway.
+
+**Fix:** Remove redundant client-side ghost Y calculation.
+
+---
+
+### 4. **Renderer Re-renders Every Frame** (Performance)
+**Location:** `renderer.js:83-149`
+
+**Problem:** `renderPlayer()` is called every `requestAnimationFrame` (~60fps) even when state hasn't changed. It clears and redraws the entire canvas each time.
+
+**Fix:** Implement dirty checking - only re-render when state actually changes. Cache previous state and compare.
+
+---
+
+### 5. **Duplicate Tetromino Definitions** (Maintenance Bug)
+**Location:** `server/game-logic.js:8-16`, `public/js/game.js:8-16`
+
+**Problem:** Tetrominoes are defined in both server and client code. They can drift out of sync.
+
+**Fix:** Share definitions via a common module or sync from server on connection.
+
+---
+
+## Bugs
+
+### 1. **Incorrect Elimination Logic on Client** (Critical Bug)
+**Location:** `public/js/game.js:228-246`
+
+**Problem:** Client eliminates player when piece is partially above board:
+
+```javascript
+let pieceAboveBoard = false;
+for (let row = 0; row < this.currentPiece.shape.length; row++) {
+  for (let col = 0; col < this.currentPiece.shape[row].length; col++) {
+    if (this.currentPiece.shape[row][col]) {
+      const boardY = this.currentPiece.y + row;
+      if (boardY < 0) {
+        pieceAboveBoard = true;
+        break;
+      }
+    }
+  }
+}
+if (pieceAboveBoard) {
+  this.gameOver = true;
+  this.eliminated = true;
+}
+```
+
+**Bug:** Server only eliminates when piece **LOCKS** with blocks in rows 0-1. Client eliminates as soon as any part goes negative, which is wrong.
+
+**Fix:** Remove client-side elimination logic. Trust server authority.
+
+---
+
+### 2. **Garbage Row Insertion Order Wrong** (Bug)
+**Location:** `public/js/game.js:311-320`
+
+**Problem:**
+```javascript
+this.board.pop();        // Remove bottom
+this.board.unshift(garbageRow);  // Add to top
+```
+
+**Bug:** This adds garbage to the TOP of the board instead of the bottom. Server correctly uses `addGarbageRow()` which shifts up and adds to bottom.
+
+**Fix:** Match server logic - shift up (pop top), add garbage to bottom.
+
+---
+
+### 3. **Typo in Color Constant** (Bug)
+**Location:** `public/js/game.js:19`
+
+```javascript
+const GARbage_COLOR = '#666666';  // Should be GARBAGE_COLOR
+```
+
+**Fix:** Rename to `GARBAGE_COLOR`.
+
+---
+
+### 4. **Hold Piece Sequence Index Desync** (Bug)
+**Location:** `server/index.js:287-299`
+
+**Problem:** When client holds, server advances sequence index but client doesn't. Client uses random piece generator instead of shared queue.
+
+**Fix:** Client should track sequence index from server state and not generate pieces locally.
+
+---
+
+### 5. **Input Lock Not Synced** (Bug)
+**Location:** `public/js/game.js:39-41`
+
+**Problem:** Client has `inputLocked` and `applyDelayHit()` but server never sends this state. Client auto-drop can interfere with server ticks.
+
+**Fix:** Remove client-side input lock or implement server-side delay hit mechanism.
+
+---
+
+### 6. **Spectator Cannot Rejoin as Player** (UX Bug)
+**Location:** `server/index.js:613-638`
+
+**Problem:** After game over, spectators are converted to players with reset state, but they don't get sent back to lobby screen to rejoin.
+
+**Fix:** After game over, emit event to switch all clients back to lobby screen.
+
+---
+
+## Implementation Priority
+
+### Priority 1 (Critical - Fix Immediately)
+1. **Fix garbage row insertion** in client (`game.js:311-320`)
+   - Change `unshift` to `push`, remove `pop`
+2. **Fix elimination logic** to match server authority
+   - Remove client-side `pieceAboveBoard` check in `lockPiece()`
+3. **Remove client-side auto-drop** to prevent desync
+   - Remove `update()` method's auto-drop logic in `game.js`
+
+### Priority 2 (Performance - High Impact)
+4. **Reduce JSON serialization frequency**
+   - Batch state updates or send only changed fields
+5. **Implement dirty checking in renderer**
+   - Only re-render when state actually changes
+6. **Share tetromino definitions**
+   - Move to common module or sync from server
+
+### Priority 3 (Bugs - Medium Impact)
+7. **Fix typo** `GARbage_COLOR` → `GARBAGE_COLOR`
+8. **Sync hold piece sequence index** between client/server
+9. **Fix spectator rejoin flow** after game over
+   - Emit lobby reset event to all clients
+
+---
+
+## Testing Strategy
+
+After making changes:
+
+1. **Unit tests:** Update existing tests to verify fixes
+2. **Integration tests:** Test multi-player scenarios
+3. **Manual testing:**
+   - Join with 2-8 browser tabs
+   - Test garbage reception and elimination
+   - Test hold mechanic
+   - Test spectator mode and rejoin
+
+---
+
+## Files to Modify
+
+| File | Changes Needed | Priority |
+|------|---------------|----------|
+| `public/js/game.js` | Fix garbage insertion, remove client elimination, remove auto-drop | P1 |
+| `public/js/game.js` | Fix typo `GARbage_COLOR` | P3 |
+| `public/js/renderer.js` | Add dirty checking | P2 |
+| `server/index.js` | Fix spectator rejoin flow | P3 |
+| `public/js/` | Create shared tetromino module | P2 |
+| `server/index.js` | Optimize state serialization | P2 |
+
+---
+
+## Notes
+
+- Server is authoritative - client should never make game logic decisions
+- All game state changes should originate from server
+- Client is purely for rendering and input collection
+- 50ms tick rate is good for real-time gameplay
+- Current test suite (105 tests) provides good coverage for regression testing