From 36760a7f92fb993408949fffdaede971af30766c Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:57:33 +1100 Subject: [PATCH 01/19] chore: add AI memory files for code standards analysis session Co-Authored-By: Claude Opus 4.6 --- .../code-standards-guidelines-44f2a42/init.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 ai-memory/analysis/code-standards-guidelines-44f2a42/init.md diff --git a/ai-memory/analysis/code-standards-guidelines-44f2a42/init.md b/ai-memory/analysis/code-standards-guidelines-44f2a42/init.md new file mode 100644 index 00000000..d1c2685e --- /dev/null +++ b/ai-memory/analysis/code-standards-guidelines-44f2a42/init.md @@ -0,0 +1,17 @@ +# Code Standards Analysis Task + +## Request +Analyze the entire Hytopia codebase in extreme detail to: +1. Document coding style, design patterns, and architecture +2. Create comprehensive guidelines/principles for AI code contributions +3. Create linting rules enforceable by AI review +4. Identify areas where current code could be improved (so new contributions are held to a higher standard) + +## Approach +Using multiple agent teams to analyze different aspects in parallel: +- Team 1: Server architecture & patterns +- Team 2: Client architecture & patterns +- Team 3: Protocol layer analysis +- Team 4: Code smells & improvement opportunities +- Team 5: Style & conventions extraction +- Team 6: Enterprise patterns & clean code analysis From 9ff1175e0bca5d5b3297a941db0fab0b4e1e0157 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:57:40 +1100 Subject: [PATCH 02/19] docs: add server architecture analysis report Comprehensive analysis of server/src/ covering: - Boot sequence and entry points - 8 design patterns (Singleton, Manager, Registry, Observer, Controller/Strategy, Composition, Serializable, Fixed Timestep) - Event system with typed events and 3 emission scopes - Dual transport networking (WebSocket + WebTransport) - Entity/World system with physics integration - Player lifecycle and management - Module dependency graph - Error handling (3-tier severity) - Performance optimizations (IterationMap, packet caching, update thresholds) Co-Authored-By: Claude Opus 4.6 --- analysis-output/server-architecture.md | 546 +++++++++++++++++++++++++ 1 file changed, 546 insertions(+) create mode 100644 analysis-output/server-architecture.md diff --git a/analysis-output/server-architecture.md b/analysis-output/server-architecture.md new file mode 100644 index 00000000..21eba8d6 --- /dev/null +++ b/analysis-output/server-architecture.md @@ -0,0 +1,546 @@ +# Hytopia Server Architecture Analysis + +## 1. Entry Point & Boot Sequence + +### Entry: `server/src/index.ts` +The barrel export file re-exports every public class, enum, type, and constant from the server SDK. Uses path aliases (`@/...`) throughout. Every class uses `export default` with named re-exports in the barrel. + +### Boot: `server/src/GameServer.ts:66` +`startServer(init)` is the user-facing entry point. Boot sequence: +1. `RAPIER.init()` - Physics engine initialization (async) +2. `GameServer.instance.blockTextureRegistry.preloadAtlas()` - Texture preload +3. `GameServer.instance.modelRegistry.preloadModels()` - Model preload +4. User `init(world?)` callback - If callback declares a parameter (`init.length > 0`), a default world is lazily created via `WorldManager.getDefaultWorld()` +5. `GameServer.instance.start()` - Emits `GameServerEvent.START`, starts `WebServer`, enables crash protection + +**Pattern**: Promise chain (not async/await) with `.catch()` calling `ErrorHandler.fatalError()`. + +--- + +## 2. Design Patterns + +### 2.1 Singleton Pattern +The **dominant pattern** throughout the codebase. Two variants are used: + +**Variant A - Lazy Singleton (private constructor + getter)** +- `GameServer` (`GameServer.ts:104-146`): `private constructor()`, `static get instance()` with lazy init + +**Variant B - Eager Singleton (public static readonly)** +- `EventRouter.globalInstance` (`EventRouter.ts:26`) +- `PlatformGateway.instance` (`PlatformGateway.ts:98`) +- `PersistenceManager.instance` (`PersistenceManager.ts:28`) +- `PlayerManager.instance` (`PlayerManager.ts:75`) +- `WorldManager.instance` (`WorldManager.ts:63`) +- `Socket.instance` (`Socket.ts:48`) +- `WebServer.instance` (`WebServer.ts:118`) +- `BlockTextureRegistry.instance` (referenced in `GameServer.ts:109`) +- `ModelRegistry.instance` (referenced in `GameServer.ts:112`) +- `AssetsLibrary.instance` (`AssetsLibrary.ts:37`) + +**Observation**: Variant B (`public static readonly instance = new ClassName()`) is overwhelmingly preferred. The `GameServer` is the only lazy singleton, presumably because it wraps the other singletons and must control instantiation order. + +### 2.2 Manager Pattern +Heavy use of "Manager" classes that own collections of domain objects: + +| Manager | Manages | Location | +|---------|---------|----------| +| `PlayerManager` | All connected `Player` instances | `players/PlayerManager.ts` | +| `WorldManager` | All `World` instances | `worlds/WorldManager.ts` | +| `EntityManager` | All `Entity` instances in a world | `worlds/entities/EntityManager.ts` | +| `AudioManager` | Audio instances in a world | `worlds/audios/AudioManager.ts` | +| `ChatManager` | Chat messages/commands in a world | `worlds/chat/ChatManager.ts` | +| `ParticleEmitterManager` | Particle emitters in a world | `worlds/particles/ParticleEmitterManager.ts` | +| `SceneUIManager` | Scene UI elements in a world | `worlds/ui/SceneUIManager.ts` | + +**Pattern**: Managers are either global singletons (`PlayerManager`, `WorldManager`) or per-world instances created in `World`'s constructor. + +### 2.3 Registry Pattern +Used for type/definition registration: + +- `BlockTypeRegistry` (`worlds/blocks/BlockTypeRegistry.ts`) - Block type definitions +- `BlockTextureRegistry` (`textures/BlockTextureRegistry.ts`) - Texture atlas management +- `ModelRegistry` (`models/ModelRegistry.ts`) - Model preloading and data extraction + +### 2.4 Observer/Event Pattern +The codebase's most pervasive architectural pattern. `EventRouter` is the central event infrastructure class. + +**EventRouter** (`events/EventRouter.ts`): +- Wraps `eventemitter3` +- Provides typed events via `EventPayloads` interface +- Three emission scopes: `emit()` (local), `emitWithGlobal()` (local + global singleton), `emitWithWorld()` (local + world) +- Special `final()` listeners: one-per-event terminal listener invoked after all normal listeners +- Used by `NetworkSynchronizer` to install terminal listeners on the world's event router that queue network sync deltas + +**Event Type Definition Convention**: +Every event-emitting class follows this pattern: +```typescript +export enum SomeEvent { + EVENT_NAME = 'NAMESPACE.EVENT_NAME', +} + +export interface SomeEventPayloads { + [SomeEvent.EVENT_NAME]: { /* payload type */ } +} +``` + +**EventPayloads Union** (`events/Events.ts`): +All event payload interfaces are merged into a single `EventPayloads` interface via interface extension. This provides compile-time type safety for all event emissions. + +### 2.5 Controller Pattern (Strategy) +Entity behavior is controlled via the Strategy pattern through `BaseEntityController`: + +- `BaseEntityController` (`worlds/entities/controllers/BaseEntityController.ts:76`) - Abstract base +- `DefaultPlayerEntityController` - Default player movement/physics +- `SimpleEntityController` - Basic movement API +- `PathfindingEntityController` - A* pathfinding + +**Lifecycle**: `attach()` -> `spawn()` -> `tickWithPlayerInput()` -> `tick()` -> `detach()` -> `despawn()` + +Controllers are EventRouters themselves, allowing external code to hook into controller events (e.g., `TICK_WITH_PLAYER_INPUT`). + +### 2.6 Composition Over Inheritance +`World` is the prime example of composition (`World.ts:299-330`): +``` +World composes: + AudioManager + BlockTypeRegistry + ChatManager + ChunkLattice + EntityManager + WorldLoop + NetworkSynchronizer + ParticleEmitterManager + SceneUIManager + Simulation +``` + +Each sub-system is injected the `World` reference during construction, creating a bidirectional dependency. The `World` exposes them via read-only getters. + +### 2.7 Serializable Pattern +Domain objects implement `protocol.Serializable` interface: +- `Player.serialize()` -> delegates to `Serializer.serializePlayer(this)` +- `PlayerCamera.serialize()` -> delegates to `Serializer.serializePlayerCamera(this)` +- `World.serialize()` -> delegates to `Serializer.serializeWorld(this)` +- `Entity.serialize()` -> delegates to `Serializer.serializeEntity(this)` + +The `Serializer` class (`networking/Serializer.ts`) is a static-only utility class that transforms domain objects into protocol schema objects. This separates serialization concerns from domain logic. + +### 2.8 Fixed Timestep Game Loop +`Ticker` (`shared/classes/Ticker.ts`) implements a fixed-timestep loop: +- Accumulates elapsed time +- Processes up to `TICK_SLOW_UPDATE_CAP` (2) updates per frame when behind +- Caps accumulator at `MAX_ACCUMULATOR_TICK_MULTIPLE` (3x) the timestep to prevent spiral of death +- Uses `setTimeout` (not `setImmediate`) to allow GC breathing room + +`WorldLoop` (`worlds/WorldLoop.ts:149-195`) orchestrates per-tick: +1. Tick entities (via `EntityManager.tickEntities()`) +2. Step physics (via `Simulation.step()`) +3. Check and emit entity position/rotation updates (via `EntityManager.checkAndEmitUpdates()`) +4. Network synchronize at reduced rate (via `NetworkSynchronizer.synchronize()`) + +--- + +## 3. Event System Deep Dive + +### `EventRouter` (`events/EventRouter.ts`) + +**Architecture**: +- Wraps `eventemitter3` (not Node.js `EventEmitter`) +- Private `_emitter` and `_finalListeners` record +- Method overloading with typed + untyped signatures for all methods (`on`, `off`, `emit`, etc.) + +**Key methods**: +- `emit(eventType, payload)` - Local emission, catches errors +- `emitWithGlobal(eventType, payload)` - Emits locally + to `EventRouter.globalInstance` +- `emitWithWorld(world, eventType, payload)` - Emits locally + to the world's EventRouter +- `final(eventType, listener)` - Single terminal listener per event type (used by `NetworkSynchronizer`) +- `on()`, `once()`, `off()`, `offAll()` - Standard listener management + +**Event naming convention**: `NAMESPACE.ACTION` (e.g., `ENTITY.SPAWN`, `PLAYER.JOINED_WORLD`, `CONNECTION.OPENED`) + +**Typed events** (`events/Events.ts:30-53`): The `EventPayloads` interface extends all 22 subsystem-specific event payload interfaces, providing a unified type map. + +### Event Flow Example: Entity Spawn +1. `Entity.spawn()` calls `this.emitWithWorld(world, EntityEvent.SPAWN, { entity: this })` +2. This emits to the entity's own listeners AND the world's event router +3. `NetworkSynchronizer` has a `final` listener on the world for `EntityEvent.SPAWN` +4. The `final` listener queues the entity serialization for the next network sync +5. On next sync tick, the queued data is sent as a packet to all connected players + +--- + +## 4. Networking Architecture + +### Transport Layer +The server supports **dual transport**: WebSocket (ws) and WebTransport (HTTP/3). + +**Socket** (`networking/Socket.ts`): +- Manages connection lifecycle (auth, binding, reconnection) +- Tracks connections by `connectionId` (UUID) and `userId` +- Handles duplicate connection detection and killing +- Extends `EventRouter` + +**Connection** (`networking/Connection.ts:64`): +- Extends `EventRouter` (inherits event emission) +- Supports WebSocket (`_ws`) and WebTransport (`_wt`) simultaneously +- Reconnect window: 30 seconds (`RECONNECT_WINDOW_MS`) +- Packet caching: `_cachedPacketsSerializedBuffer` (static Map) caches serialized packets by array identity to avoid re-encoding for multiple players +- Compression: gzip for packets > 64KB (mainly chunks) +- WebTransport uses reliable (bidirectional streams) and unreliable (datagrams, < 1200 bytes) channels +- Packet framing for WebTransport reliable streams (WT doesn't frame like WS does) +- Uses msgpackr for serialization + +**WebServer** (`networking/WebServer.ts:112`): +- HTTP/2 secure server (TLS required) +- Serves static assets from `assets/` directory +- ETag caching for asset responses +- Health check endpoint at `/` +- Emits `UPGRADE` events for WebSocket handshakes +- Path traversal protection (`..` check + `startsWith` validation) +- CORS headers on all responses +- Global HTTP dispatcher configured via `undici` for connection pooling + +### Network Synchronization +**NetworkSynchronizer** (`networking/NetworkSynchronizer.ts`): +- Runs at 30Hz (half the default 60Hz tick rate) +- Maintains **sync queues** for each data type (entities, blocks, chunks, audio, etc.) +- Two queue types: + - `SyncQueue` - Keyed by ID, with broadcast and per-player maps + - `SingletonSyncQueue` - Single value (camera, UI, world state) +- Subscribes to world events via `final()` listeners +- On sync tick, collects all queued deltas into packets +- Entity position/rotation updates go over **unreliable** channel (UDP datagrams) +- All other updates go over **reliable** channel +- Clears all queues after sending (with guarded `.size > 0` checks to avoid unnecessary GC) + +**Serializer** (`networking/Serializer.ts`): +- Static-only class (no instances) +- Transforms domain objects -> protocol schema objects +- Short property names for wire efficiency (e.g., `i` for id, `p` for position, `r` for rotation) +- Vectors serialized as `[x, y, z]` tuples, quaternions as `[x, y, z, w]` + +### Protocol +Uses `@hytopia.com/server-protocol` package for: +- Packet definitions and validation (`protocol.isValidPacket()`) +- Schema types (`protocol.EntitySchema`, `protocol.WorldSchema`, etc.) +- Packet creation (`protocol.createPacket()`) +- Packet IDs (`protocol.PacketId.INPUT`, etc.) +- Buffer framing/unframing for WebTransport + +### Platform Integration +**PlatformGateway** (`networking/PlatformGateway.ts:97`): +- Singleton with private constructor +- Integrates with Hytopia platform services +- Session validation, cosmetics (via GraphQL), KV persistence, notifications +- Falls back to local filesystem persistence in development +- GraphQL client for player cosmetics (`graphql-ws`) +- REST API for notifications + +--- + +## 5. Entity/World System + +### World Lifecycle +1. `WorldManager.createWorld(options)` creates a `World` instance with auto-incrementing ID +2. `World` constructor initializes all sub-managers (composition) +3. `world.start()` starts the `WorldLoop` ticker +4. Each tick: entities tick -> physics step -> emit updates -> network sync +5. `world.stop()` stops the ticker + +### World (`worlds/World.ts:217`) +- Extends `EventRouter`, implements `protocol.Serializable` +- Contains environment properties (lighting, fog, skybox) +- `loadMap(map)` processes `WorldMap` data: registers block types, creates chunks, spawns environmental entities + +### Entity System +**Entity** (`worlds/entities/Entity.ts`): +- Union type options: `BlockEntityOptions | ModelEntityOptions` +- Has RigidBody (physics), Colliders, model animations, model node overrides +- `spawn(world, position)` / `despawn()` +- `isEnvironmental` flag: environmental entities skip tick/position updates +- Position/rotation change detection with thresholds (`ENTITY_POSITION_UPDATE_THRESHOLD_SQ`, `ENTITY_ROTATION_UPDATE_THRESHOLD`) + +**EntityManager** (`worlds/entities/EntityManager.ts`): +- Separate `_activeEntities` (Set) for non-environmental entities (performance optimization) +- Auto-incrementing entity IDs per world +- Querying: `getAllEntities()`, `getAllPlayerEntities()`, `getPlayerEntitiesByPlayer()`, `getEntitiesByTag()`, `getEntityChildren()` + +**PlayerEntity** (`worlds/entities/PlayerEntity.ts`): +- Extends `Entity` with player-specific behavior +- Links to a `Player` instance +- Player input processing in tick + +**DefaultPlayerEntity** (`worlds/entities/DefaultPlayerEntity.ts`): +- Extends `PlayerEntity` with cosmetic slots and default configuration + +### Block System +- `ChunkLattice` - Spatial grid of `Chunk` instances using packed bigint keys +- `Chunk` - 16x16x16 block array with rotations +- `Block` - Individual block with rotation support (24 rotation variants) +- `BlockType` - Block type definition with collider options and texture +- `BlockTypeRegistry` - Per-world registry of block types + +### Physics Integration +**Simulation** (`worlds/physics/Simulation.ts`): +- Wraps RAPIER physics engine +- Default gravity: `{ x: 0, y: -32, z: 0 }` (Minecraft-like) +- Default tick rate: 60Hz +- Raycasting, intersection queries +- Collision event dispatch (entity-entity, entity-block) +- Debug rendering support +- `ColliderMap` for mapping RAPIER collider handles to game objects +- `CollisionGroupsBuilder` for configuring collision filtering + +**RigidBody** (`worlds/physics/RigidBody.ts`): +- Wrapper around RAPIER rigid bodies +- Types: Dynamic, Fixed, KinematicPosition, KinematicVelocity + +--- + +## 6. Player System + +### Player Lifecycle +1. `Connection` opens -> `PlayerManager._onConnectionOpened()` creates `Player` +2. Player persistence data loaded +3. `worldSelectionHandler?.(player)` called for custom routing +4. `player.joinWorld(world)` -> emits `PlayerEvent.JOINED_WORLD` +5. On disconnect: 30s reconnect window -> `PlayerEvent.LEFT_WORLD` on close +6. On reconnect within window: `PlayerEvent.RECONNECTED_WORLD` + +### Player (`players/Player.ts:108`) +- Extends `EventRouter`, implements `protocol.Serializable` +- Owns: `PlayerCamera`, `PlayerUI`, `Connection` reference +- Input: received via `InputPacket`, stored as `PlayerInput` object +- Sequence number tracking for unreliable UDP input packets +- Interact system: client sends ray origin/direction, server performs raycast +- Persistence: `getPersistedData()`, `setPersistedData()` +- Notifications: `scheduleNotification()`, `unscheduleNotification()` + +### PlayerCamera (`players/PlayerCamera.ts:156`) +- Extends `EventRouter`, implements `protocol.Serializable` +- Modes: FIRST_PERSON, THIRD_PERSON, SPECTATOR +- Can attach to entity or position +- Target entity/position for continuous look-at +- View model support (first-person arms/weapons) +- Orientation (pitch/yaw) set from client input +- `facingDirection` and `facingQuaternion` computed properties + +### PlayerUI (`players/PlayerUI.ts:64`) +- Extends `EventRouter` +- `load(htmlUri)` - Replace UI +- `append(htmlUri)` - Append to existing UI +- `sendData(data)` - Push data to client UI +- `lockPointer()` / `freezePointerLock()` - Pointer lock control +- Bidirectional data: server sends via `sendData`, client sends via `UI_DATA_SEND` packet + +### PlayerManager (`players/PlayerManager.ts:69`) +- Global singleton +- Listens to global `ConnectionEvent` events +- `worldSelectionHandler` callback for custom world routing +- Tracks players by `Connection` reference + +--- + +## 7. Module Organization & Dependency Graph + +``` +GameServer (root singleton) + |-- BlockTextureRegistry (singleton) + |-- ModelRegistry (singleton) + |-- PlayerManager (singleton) + | |-- Player (per-connection) + | | |-- PlayerCamera + | | |-- PlayerUI + | | |-- Connection (transport) + | |-- PersistenceManager (singleton) + | + |-- WorldManager (singleton) + | |-- World (per-instance) + | |-- AudioManager + | |-- BlockTypeRegistry + | |-- ChatManager + | |-- ChunkLattice -> Chunk[] + | |-- EntityManager -> Entity[] + | |-- NetworkSynchronizer + | |-- ParticleEmitterManager + | |-- SceneUIManager + | |-- Simulation (RAPIER) + | |-- WorldLoop -> Ticker + | + |-- Socket (singleton) + | |-- Connection[] (WebSocket/WebTransport) + | + |-- WebServer (singleton) + +Cross-cutting: + EventRouter (event bus, used by almost every class) + ErrorHandler (static utility) + Telemetry/Sentry (static utility) + Serializer (static utility) + PlatformGateway (singleton) +``` + +### Directory Structure +``` +server/src/ + GameServer.ts # Root singleton + startServer() + index.ts # Barrel exports + playground.ts # Local dev test server + assets/ # AssetsLibrary + errors/ # ErrorHandler + events/ # EventRouter, Events (type union) + metrics/ # Telemetry (Sentry integration) + models/ # ModelRegistry (glTF processing) + networking/ # Connection, Socket, WebServer, NetworkSynchronizer, Serializer, PlatformGateway + ssl/ # Embedded SSL certificates + persistence/ # PersistenceManager + players/ # Player, PlayerCamera, PlayerManager, PlayerUI + shared/ + classes/ # Ajv, IterationMap, Matrix2/3/4, Quaternion, Ticker, Vector2/3 + helpers/ # msgpackr configuration + types/ # Outline, RgbColor, math types (Vector2Like, Vector3Like, QuaternionLike, etc.) + textures/ # BlockTextureRegistry + worlds/ + World.ts # World container + WorldLoop.ts # Tick loop + WorldManager.ts # World factory/registry + audios/ # Audio, AudioManager + blocks/ # Block, BlockType, BlockTypeRegistry, Chunk, ChunkLattice + chat/ # ChatManager + entities/ # Entity, EntityManager, PlayerEntity, DefaultPlayerEntity, EntityModelAnimation, EntityModelNodeOverride + controllers/ # BaseEntityController, DefaultPlayerEntityController, SimpleEntityController, PathfindingEntityController + particles/ # ParticleEmitter, ParticleEmitterManager + physics/ # Simulation, RigidBody, Collider, ColliderMap, CollisionGroupsBuilder + ui/ # SceneUI, SceneUIManager +``` + +--- + +## 8. Error Handling + +### ErrorHandler (`errors/ErrorHandler.ts`) +Three severity levels: +1. `warning(message)` - Logs warning, no throw +2. `error(message)` - Logs error, no throw (returns `void`, causing `undefined` returns) +3. `fatalError(message)` - Logs error, **throws** (`never` return type) + +**Pattern**: `ErrorHandler.error()` returns `void`, so callers that `return ErrorHandler.error(...)` get an implicit `undefined` return. This is intentional for methods that return `void | T`. + +**Production crash protection** (`ErrorHandler.enableCrashProtection()` at `ErrorHandler.ts:108`): +- `unhandledRejection` -> logs error +- `uncaughtException` -> logs error, exits after 1s delay +- `console.log` disabled in production (replaced with no-op) + +**Formatting**: Color-coded console output with timestamps, stack traces, and error counts. + +### Error propagation patterns: +- **Guard clauses**: Early returns with `ErrorHandler.error()` or `ErrorHandler.warning()` (e.g., `PlayerCamera._requirePlayerWorld()`) +- **Fatal assertions**: `ErrorHandler.fatalError()` for impossible states (e.g., serializing unspawned entities) +- **Try/catch in event emission**: `EventRouter.emit()` wraps listener invocation in try/catch +- **Connection errors**: Caught and logged, connection continues operating + +--- + +## 9. Type Safety + +### Typed Events +All events use mapped types via `EventPayloads` interface. Method overloading on `EventRouter` provides type inference: +```typescript +// Typed overload +public emit(eventType: TEventType, payload: EventPayloads[TEventType]): boolean; +// Fallback for dynamic event types +public emit(eventType: string, payload: any): boolean; +``` + +### Type-Like Interfaces +Math types use "Like" suffix interfaces (`Vector3Like`, `QuaternionLike`, `SpdMatrix3`) as structural contracts, allowing plain objects to be used interchangeably with class instances. + +### Enum Usage +All event types and state enums use TypeScript `enum`: +- `GameServerEvent`, `ConnectionEvent`, `PlayerEvent`, `EntityEvent`, `WorldEvent`, etc. +- `PlayerCameraMode`, `RigidBodyType`, `ColliderShape`, `EntityModelAnimationBlendMode`, etc. + +### Generics +- `IterationMap` - Generic key-value container +- `EventRouter.emit()` - Event type constraints +- `EntityManager.getEntity(id)` - Generic entity type narrowing +- `SyncQueue` and `SingletonSyncQueue` - Network sync queue types +- `Connection.onPacket()` - Typed packet handlers + +--- + +## 10. Async Patterns + +### Promise-based initialization +`startServer()` uses a `.then()` chain (not async/await) for the boot sequence. This is likely historical. + +### Async/await usage +- `PlatformGateway` methods: `getPlayerCosmetics()`, `getPlayerSession()`, `getGlobalData()`, etc. +- `PersistenceManager` methods: `getPlayerData()`, `setGlobalData()` +- `PlayerManager._onConnectionOpened()`: async for loading persistence data +- `Connection.bindWt()`: async for WebTransport stream setup + +### Error handling in async +- `void this._someAsyncMethod()` pattern used to fire-and-forget without unhandled rejection +- `.catch()` on promises that might fail silently +- `Promise.resolve(initResult)` to normalize sync/async init callbacks + +### No async in hot paths +The tick loop (`WorldLoop._tick`) is entirely synchronous. All async operations (persistence, platform calls) happen outside the tick cycle. + +--- + +## 11. Performance Optimizations + +### IterationMap (`shared/classes/IterationMap.ts`) +Custom data structure maintaining both a Map (O(1) lookup) and Array (fast iteration). Uses lazy synchronization (`_isDirty` flag) to avoid rebuilding the array on every mutation. + +### Network Sync Optimizations +- **Packet caching** (`Connection._cachedPacketsSerializedBuffer`): Serialized packets are cached by array identity so the same packet isn't re-encoded per player +- **Conditional queue clearing**: Only clears queues if `size > 0` to avoid unnecessary GC +- **Unreliable channel for position/rotation**: Entity spatial updates sent via UDP datagrams +- **Sync rate reduction**: Network sync at 30Hz while physics runs at 60Hz +- **Compression threshold**: gzip only for packets > 64KB + +### Entity Update Thresholds +Position and rotation changes below thresholds are not emitted: +- Position: `0.04^2` squared distance (1/25 of a block) +- Rotation: `cos(0.052/2)` (~3 degrees) + +### Environmental Entity Optimization +`isEnvironmental` entities are excluded from the active tick set entirely, reducing per-tick overhead for static world objects. + +--- + +## 12. Key Architectural Decisions + +1. **EventRouter as base class**: Nearly every domain class extends `EventRouter`, providing ubiquitous event emission. `Connection`, `Socket`, `WebServer`, `Player`, `PlayerCamera`, `PlayerUI`, `World`, `WorldLoop`, `ChunkLattice`, `BaseEntityController`, `Simulation` all extend it. + +2. **World-scoped events**: Events can be emitted to a specific world's event router, allowing per-world event isolation. The `NetworkSynchronizer` uses `final()` listeners on the world to intercept state changes. + +3. **Singleton-heavy architecture**: All global services are singletons. This simplifies access patterns but creates tight coupling. + +4. **Protocol separation**: Wire format is handled by `@hytopia.com/server-protocol` package. The server uses `Serializer` as a translation layer between domain objects and protocol schemas. + +5. **Dual transport**: Both WebSocket and WebTransport are supported simultaneously per connection, with transparent fallback and reconnection across transports. + +6. **Fixed timestep with network decimation**: Physics at 60Hz, network at 30Hz, with unreliable UDP for high-frequency spatial updates. + +7. **Composition for World**: Instead of inheritance, World composes 10+ sub-systems, each receiving the World reference. This keeps each system focused but creates circular references. + +8. **Abstract controller pattern**: Entity behavior is decoupled via the Strategy pattern through `BaseEntityController`, enabling custom controllers without modifying entity code. + +--- + +## 13. Summary of Patterns by Category + +| Category | Patterns | +|----------|----------| +| Creational | Singleton (eager + lazy), Factory (WorldManager.createWorld) | +| Structural | Composition (World), Facade (GameServer), Adapter (Connection wraps WS/WT) | +| Behavioral | Observer (EventRouter), Strategy (BaseEntityController), Command (Packet handlers) | +| Architectural | Manager pattern, Registry pattern, Barrel exports, Fixed-timestep game loop | +| Data | IterationMap (hybrid Map+Array), SyncQueue (broadcast + per-player), Protocol schemas (short keys for wire efficiency) | +| Networking | Dual transport, Packet caching, Reliable/unreliable channel split, Reconnect window, Session auth | +| Error Handling | Tiered severity (warning/error/fatal), Guard clauses, Production crash protection | +| Performance | Environmental entity optimization, Update thresholds, Lazy sync arrays, Conditional queue clearing | From 8be8c26f2208dbbb05064c080e057740f2156c9c Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:57:48 +1100 Subject: [PATCH 03/19] docs: add client architecture analysis report Comprehensive analysis of client/src/ covering: - Multi-scene Three.js rendering with 6-pass post-processing pipeline - Chunk/voxel system (16x16x16 chunks, 2x2x2 batch grouping) - Web Worker architecture for geometry generation - Entity system (~1700 line Entity.ts with 7-pass per-frame update) - 22+ Manager pattern instances with Game as service locator - Dual transport networking with msgpack serialization - glTF instanced mesh rendering pipeline - Working variables pattern for GC pressure reduction - Manual matrix management throughout - Mobile support with virtual joystick Co-Authored-By: Claude Opus 4.6 --- analysis-output/client-architecture.md | 625 +++++++++++++++++++++++++ 1 file changed, 625 insertions(+) create mode 100644 analysis-output/client-architecture.md diff --git a/analysis-output/client-architecture.md b/analysis-output/client-architecture.md new file mode 100644 index 00000000..827164e5 --- /dev/null +++ b/analysis-output/client-architecture.md @@ -0,0 +1,625 @@ +# Hytopia Client Architecture Analysis + +## 1. Entry Point and Initialization Flow + +### main.ts (client/src/main.ts) +- Side-effect imports: heartbeat service, UI globals (hytopia), profanity filter +- Async IIFE: measures refresh rate via `PerformanceMetricsManager.measureRefreshRate()`, then calls `Game.instance.start()` +- Refresh rate measurement fires-and-forgets with `void` + `.catch(console.error)` -- does not block game start + +### Game.ts (client/src/Game.ts) +- **Singleton pattern** via private static `_instance` and static `get instance()` (lines 89-95) +- Central orchestrator: constructor instantiates ALL 22+ managers in explicit dependency order +- **Constructor ordering matters**: NetworkManager, SettingsManager, PerformanceMetricsManager, InputManager, Camera, Renderer, ChunkWorkerClient are initialized first (lines 61-67), then all other managers +- Some managers receive `this` (the Game instance), others do not (PerformanceMetricsManager, ChunkWorkerClient, CustomTextureManager, LightLevelManager, SkyDistanceVolumeManager) +- `start()` method (line 123): starts renderer animation loop, connects to network, initializes block texture atlas +- All managers exposed via public readonly getters + +**Design pattern**: Service Locator / God Object -- Game acts as both a container and a service locator. Every manager accesses siblings through `this._game.otherManager`. + +--- + +## 2. Rendering Architecture + +### Renderer (client/src/core/Renderer.ts) +- Wraps Three.js `WebGLRenderer` with multi-scene architecture: + - `_scene`: main 3D scene (chunks, entities) + - `_viewModelScene`: first-person view model (rendered after depth clear for weapon/tool overlay) + - `_overlayScene`: screen-space overlays (underwater effect quad) + - `_uiScene`: CSS2D UI elements (nametags) +- **Post-processing pipeline** (lines 206-213): + 1. `RenderPass` (main scene) + 2. `SelectiveOutlinePass` (entity outlines) + 3. `RenderPass` (view model, no clear, depth clear) + 4. `WhiteCoreBloomPass` (custom bloom) + 5. `SMAAPass` (anti-aliasing) + 6. `OutputPass` (final output) +- Post-processing passes are conditionally enabled via `settingsManager.qualityPerfTradeoff` +- **Animation loop** (`_animate`, line 249): uses `requestAnimationFrame`, manual FPS cap via elapsed time comparison +- Frame update order: measure delta -> FPS cap check -> performance update -> settings update -> fog update -> emit Animate event -> update managers (arrows, blocks, camera, audio, skybox, gltf, ui) -> underwater effect -> view model sync -> render +- **Manual matrix management**: all scenes have `matrixAutoUpdate = false` and `matrixWorldAutoUpdate = false` (lines 526-533) +- **Custom transparent sort** (lines 599-617): sorts by groupOrder, renderOrder, then by AABB farthest-point distance to camera +- **Skybox**: custom `SkyboxMaterial` extending `ShaderMaterial` with cube environment mapping and color tinting (lines 66-103) +- Color space: explicit `SRGBColorSpace` output, ambient light colors converted from sRGB to linear +- Fog: Three.js `Fog` with smooth interpolation toward target colors and near/far values +- WebGL context loss handling: modal alert + throws Error (lines 620-624) + +### Camera (client/src/core/Camera.ts) +- Dual camera system: `_gameCamera` (attached to player) and `_spectatorCamera` (free-fly) +- Three camera modes: `FIRST_PERSON`, `THIRD_PERSON`, `SPECTATOR` (enum CameraMode) +- **Manual matrix control**: both cameras have `matrixAutoUpdate = false`, `matrixWorldAutoUpdate = false` +- Third-person features: radial zoom (MIN_ZOOM=3, MAX_ZOOM=10), shoulder rotation offset, film offset, collision with blocks via raycasting +- Camera collision smoothing: different speeds for moving in (20.0) vs out (10.0) to reduce jarring jumps (lines 634-635) +- First-person features: forward offset, view model anchoring with base camera offset caching +- Model pitch/yaw with camera: model can be configured to rotate with camera orientation +- FOV, zoom, and film offset use lerp interpolation with `CAMERA_LERP_TIME = 0.2` +- **Working variables pattern**: module-level pre-allocated `Vector3`, `Euler`, `Quaternion` objects reused to avoid GC pressure (lines 16-25) + +### DebugPanel (client/src/core/DebugPanel.ts) +- Uses Three.js `Stats` module and `lil-gui` for overlay +- Toggled with backtick/F3 key or 5-finger touch +- Tracks: player position, camera position, server info, WebGL stats, entity stats, chunk stats, glTF stats, scene UI stats, arrow stats, audio stats +- All stats classes follow same pattern: static properties + static `reset()` method + +### DebugRenderer (client/src/core/DebugRenderer.ts) +- Renders physics debug data (raycasts, collision shapes) received from server +- Raycasts shown as arrows with blink-then-fade animation (lines 58-66) +- Physics debug wireframes via `LineSegments` with vertex colors + +### PerformanceMetricsManager (client/src/core/PerformanceMetricsManager.ts) +- Uses Three.js `Clock` for delta time +- Refresh rate estimation: samples 30 frames, trims outliers (10%), snaps to common rates (30-360Hz) +- Static `measureRefreshRate()` -- result stored in static `_refreshRate` +- Tracks FPS (updated every 1s), delta time, frame count, memory usage (JS heap) + +--- + +## 3. Chunk/Voxel System + +### ChunkConstants (client/src/chunks/ChunkConstants.ts) +- `CHUNK_SIZE = 16` (16x16x16 blocks per chunk) +- `BATCH_SIZE = 2` -- 2x2x2 chunks batched for reduced draw calls +- `BATCH_WORLD_SIZE = 32` -- 32 blocks per batch dimension +- Typed IDs: `ChunkId` and `BatchId` as template literal types `${number},${number},${number}` + +### Chunk (client/src/chunks/Chunk.ts) +- Represents a 16x16x16 voxel chunk +- Stores blocks as `Uint8Array` (256 possible block type IDs) +- Block rotations stored as sparse `Map` (blockIndex to rotationIndex) +- Light sources lazily computed and cached (`_lightSources`) +- **Coordinate utilities** -- all static methods: + - `globalCoordinateToChunkId`, `globalCoordinateToOriginCoordinate`, `globalCoordinateToLocalCoordinate` + - Uses bitwise AND for fast modulo with power-of-2 chunk size: `x & ~(CHUNK_SIZE - 1)` (line 61) + - `worldPositionToGlobalCoordinate`: floor-based conversion +- Batch system: chunks grouped into 2x2x2 batches for mesh batching + +### ChunkRegistry (client/src/chunks/ChunkRegistry.ts) +- Registry pattern: stores chunks by ChunkId, tracks batch metadata +- Used by both main thread (ChunkManager) and web worker (ChunkWorker) + +### ChunkManager (client/src/chunks/ChunkManager.ts) +- Event-driven: listens to `BlocksPacket`, `ChunksPacket`, `Animate`, `ChunkBatchBuilt` +- On blocks update: updates registry, sends `blocks_update` message to worker +- On chunks packet: registers/removes chunks, determines affected batches, sorts by proximity to player, sends batch build messages to worker +- View distance: per-frame check of batch distance to camera (2D, ignoring Y) (lines 68-88) + +### ChunkMeshManager (client/src/chunks/ChunkMeshManager.ts) +- Manages Three.js meshes for chunk batches +- Three mesh types per batch: liquid, opaque solid, transparent solid +- Each mesh type stored in separate `Map` +- `createOrUpdateMesh`: creates `BufferGeometry` with position, normal, uv, color, lightLevel, foamLevel attributes +- Meshes have `matrixAutoUpdate = false` (fixed position) +- View distance culling: adds/removes meshes from scene graph (not just visibility toggle) for GPU performance +- Dirty tracking for `solidMeshesInScene` array (used by camera collision raycasting) + +### LightLevelManager / LightLevelVolume (client/src/chunks/LightLevelManager.ts, LightLevelVolume.ts) +- Block-based point light system +- Light level volume computed in worker, results stored per-chunk +- Entities query light level from volume for ambient occlusion + +### SkyDistanceVolume / SkyDistanceVolumeManager +- Sky distance tracking for ambient lighting on entities +- Computed in worker alongside light levels + +--- + +## 4. Worker Thread Architecture + +### ChunkWorker (client/src/workers/ChunkWorker.ts) +- **Web Worker** for CPU-intensive geometry generation +- Runs in separate thread via `new Worker(new URL('./ChunkWorker.ts', import.meta.url), { type: 'module' })` +- Maintains its own copies of: `ChunkRegistry`, `BlockTypeRegistry`, `BlockTextureAtlasManager` +- **Message queue with yielding**: `MAX_CONSECUTIVE_PROCESS_COUNT = 100` to allow message events between batches +- Generates geometry for: chunk batches (liquid/opaque/transparent), block entities, light level volumes, sky distance volumes +- Geometry data: positions, normals, UVs, indices, colors, light levels, foam levels +- Ambient occlusion: per-vertex AO sampling from neighboring blocks +- Sky light: per-vertex sampling from 4 coordinates with weighted blending + +### ChunkWorkerClient (client/src/workers/ChunkWorkerClient.ts) +- Main-thread proxy for ChunkWorker +- Posts typed messages to worker, receives results and emits via EventRouter +- String literal URL requirement: `'./ChunkWorker.ts'` must be literal for Vite bundling (line 17 comment) + +### ChunkWorkerConstants (client/src/workers/ChunkWorkerConstants.ts) +- Defines all message types between main thread and worker: + - To worker: `init`, `chunk_update`, `chunk_remove`, `chunk_batch_build`, `blocks_update`, `block_type`, `block_type_update`, `block_entity_build`, `block_texture_atlas_metadata`, `block_texture_atlas_updated` + - From worker: `chunk_batch_built`, `block_entity_built`, `block_texture_atlas_updated`, `block_texture_atlas_metadata`, `light_level_volume_built`, `sky_distance_volume_built` + +--- + +## 5. Entity System + +### Entity (client/src/entities/Entity.ts) +- **Largest file** (~1700 lines) -- handles both glTF and block entities +- Two entity types: glTF model entities (3D models) and block entities (voxel-like) +- **Transform interpolation**: position, rotation, scale interpolated toward targets with configurable time +- Server tick ordering: tracks `_lastPositionUpdateServerTick` and `_lastRotationUpdateServerTick` for unreliable/unordered packet handling +- **Multi-pass update** (called by EntityManager): + 1. `update()`: interpolate position/rotation/scale + 2. `applyViewDistance()`: 2D distance check + 3. `applyFrustumCulling()`: AABB frustum test + 4. `updateAnimationAndLocalMatrix()`: animation mixer update + local matrix + 5. `updateWorldMatrices()`: world matrix propagation + 6. `updateLightLevel()` / `updateSkyLight()`: lighting updates +- **Frame skipping optimization**: far entities skip updates for up to `MAX_UPDATE_SKIP_FRAMES = 4` frames (line 52) +- **glTF model management**: + - Automatic selection of optimized model variants: base, named-nodes, no-animations + - `_buildGLTFModel()`: loads via `GLTFManager`, applies materials, animations, node overrides + - Animation system: `AnimationMixer` with fade in/out blending (`DEFAULT_ANIMATION_BLEND_TIME_S = 0.1`) + - Additive animation blending support + - Model node overrides: per-node position/rotation/scale overrides with interpolation + - Node visibility: camera-based hidden/shown nodes +- **Block entity model**: geometry built by ChunkWorker, texture from atlas +- Custom textures: loaded via `CustomTextureManager` +- Light level: shared uniform data (`LightLevelUniformData`) for shader-based lighting +- Entity-entity attachment: parent-child hierarchy with model-ready listeners +- `_entityRoot`: Three.js `Group` as root, `matrixAutoUpdate = false` +- Custom `userData` for entity ID and effective visibility tracking +- **Emissive support**: per-entity and per-node emissive color/intensity + +### EntityManager (client/src/entities/EntityManager.ts) +- Manages all entities in a `Map` +- Separate tracking for dynamic entities (`_dynamicEntities: Set`) +- **Seven-pass animation frame update** (lines 165-233): + 1. Local position/rotation update + 2. View distance culling + 3. Frustum culling + 4. Animation + local matrix + 5. World matrices + 6. Light level (conditional on light-emitting blocks) + 7. Sky light +- Entity creation: distinguishes Static Environment Entities (cheaper path) from dynamic entities +- Outline system: per-entity outline options, max `MAX_OUTLINES` concurrent outlines +- Static entities delegated to `StaticEntityManager` + +### StaticEntity (client/src/entities/StaticEntity.ts) +- Extends Entity for static environmental entities +- Lower CPU cost path -- no per-frame updates except light level + +### StaticEntityManager (client/src/entities/StaticEntityManager.ts) +- Manages static environmental entities with batch operations +- Delegates to GLTFManager for instanced mesh rendering + +### EntityConstants (client/src/entities/EntityConstants.ts) +- `EntityId` type alias for number +- `MAX_OUTLINES` constant + +### EntityStats (client/src/entities/EntityStats.ts) +- Static class with static numeric properties and `reset()` method +- Tracks: count, staticEnvironmentCount, inViewDistanceCount, frustumCulledCount, updateSkipCount, animationPlayCount, localMatrixUpdateCount, worldMatrixUpdateCount, lightLevelUpdateCount, customTextureCount + +--- + +## 6. Manager Pattern Analysis + +Every subsystem follows the **Manager Pattern** consistently: + +| Manager | Constructor Receives | Singleton? | EventRouter Listener? | +|---------|---------------------|------------|----------------------| +| Game | (none - is singleton) | Yes (static) | No | +| ArrowManager | Game | No | Yes | +| AudioManager | Game | No | Yes | +| BlockMaterialManager | Game | No | Yes | +| BlockTextureAtlasManager | Game | No | Yes | +| BlockTypeManager | Game | No | Yes | +| BridgeManager | Game | No | Yes | +| ChunkManager | Game | No | Yes | +| ChunkMeshManager | Game | No | No | +| CustomTextureManager | (none) | No | No | +| DebugRenderer | Game | No | Yes | +| EntityManager | Game | No | Yes | +| GLTFManager | Game | No | No | +| InputManager | Game | No | Yes | +| LightLevelManager | (none) | No | No | +| MobileManager | Game | No | No | +| NetworkManager | Game | No | No | +| ParticlesManager | Game | No | Yes | +| PerformanceMetricsManager | (none) | No | No | +| PlayerManager | Game | No | Yes | +| Renderer | Game | No | Yes | +| SettingsManager | Game | No | No | +| SkyDistanceVolumeManager | (none) | No | No | +| UIManager | Game | No | Yes | + +**Observations**: +- No dependency injection framework -- all dependencies resolved through Game reference +- Most managers store `_game: Game` as first private field +- Pattern is consistent but creates tight coupling through Game +- Some managers are standalone (no Game reference): PerformanceMetricsManager, CustomTextureManager, LightLevelManager, SkyDistanceVolumeManager, ChunkWorkerClient + +--- + +## 7. Event System + +### EventRouter (client/src/events/EventRouter.ts) +- **Singleton** via `public static readonly instance = new EventRouter()` (line 3) +- Simple pub-sub: `on()`, `off()`, `offAll()`, `emit()` +- Uses `Map>` for storage +- Error handling: catches and logs errors in listeners (lines 36-38) +- No event priority, no async support, no event cancellation +- Type parameter `TPayload` for generic typing but no runtime enforcement + +**Event naming convention**: `NAMESPACE.EVENT_NAME` (e.g., `NETWORK_MANAGER.ENTITIES_PACKET`, `RENDERER.ANIMATE`, `CAMERA.GAME_CAMERA_ORIENTATION_CHANGE`) + +**Event type definitions**: Each emitter defines its own enum + namespace: +```typescript +export enum NetworkManagerEventType { + AudiosPacket = 'NETWORK_MANAGER.AUDIOS_PACKET', + // ... +} +export namespace NetworkManagerEventPayload { + export interface IAudiosPacket { deserializedAudios: DeserializedAudios; serverTick: number; } + // ... +} +``` + +--- + +## 8. UI System + +### UIManager (client/src/ui/UIManager.ts) +- Manages server-provided HTML UI loaded into `#ui-container` div +- Scene UIs: 3D-positioned UI elements (nametags, health bars) +- **Monkey-patches `EventTarget.prototype.addEventListener`** (lines 20-31) to track elements with click listeners for interact detection +- Reliable click fallback for Safari/WebKit bugs (lines 87-125) +- HTML templates loaded via fetch, scripts re-executed via DOM replacement +- CDN URL template replacement: `{{CDN_ASSETS_URL}}` +- Pending SceneUI queue for templates not yet registered + +### SceneUI (client/src/ui/SceneUI.ts) +- 3D-positioned UI using `CSS2DObject` (custom Three.js CSS2D renderer) +- Can attach to entities or fixed world positions +- Template-based rendering via `hytopia.getSceneUITemplateRenderer()` +- View distance culling for performance + +### Modal (client/src/ui/Modal.ts) +- `modalAlert()` and `modalPrompt()` -- custom modal implementations +- Used for server connection, error messages + +### Nametag (client/src/ui/templates/Nametag.ts) +- Built-in SceneUI template for player/entity nametags + +### hytopia global (client/src/ui/globals/hytopia.ts) +- Global `hytopia` object exposed on window +- Provides API for game developers: `onData()`, `offAllData()`, `emitData()`, `sendData()` +- SceneUI template registration: `registerSceneUITemplate()`, `hasSceneUITemplateRenderer()` + +--- + +## 9. Input System + +### InputManager (client/src/input/InputManager.ts) +- **Keyboard mapping**: `CODE_TO_KEY_MAP` maps physical key codes to logical keys (layout-independent via `event.code`) +- **Supported inputs**: 20 letter keys, 10 digit keys, space, shift, tab, mouse left, mouse right +- State tracking: `InputState` (boolean per key) + `ContinuousInputState` (camera pitch/yaw, joystick direction) +- **Network batching**: continuous inputs (camera, joystick) batched at 60Hz desktop / 30Hz mobile via `setInterval` (lines 291-302) +- Discrete inputs (key press/release) sent immediately +- Pointer lock: handles unadjusted movement, Linux fallback, Safari quirks +- **Interact system** (lines 343-385): detects tap vs hold vs drag, sends ray origin+direction to server +- Tap detection: max 200ms duration, max 30px movement + +--- + +## 10. Audio System + +### AudioManager (client/src/audio/AudioManager.ts) +- Manages `Audio` instances in `Map` +- Max 64 active audio nodes (`MAX_ACTIVE_NODES = 64`) to prevent browser limits +- LRU cleanup: oldest non-playing nodes disposed first +- Orphaned audio cleanup: every 5 seconds, disposes nodes for despawned entities +- Spatial audio via Three.js `AudioListener` attached to active camera + +### Audio (client/src/audio/Audio.ts) +- Wraps Three.js `PositionalAudio` / `Audio` +- Entity attachment: positional audio follows entity position +- Properties: volume, playback rate, detune, distortion, cutoff distance, reference distance +- Start tick synchronization for server-coordinated playback + +### AudioStats (client/src/audio/AudioStats.ts) +- Static properties: count, matrixUpdateCount, matrixUpdateSkipCount + +--- + +## 11. Network Layer + +### NetworkManager (client/src/network/NetworkManager.ts) +- **Dual transport**: WebTransport (HTTP/3) with WebSocket fallback +- WebTransport: bidirectional reliable stream + unreliable datagrams +- WebSocket: reliable-only binary frames +- **Packet serialization**: msgpackr with FLOAT32_OPTIONS.ALWAYS +- **Compression**: gzip detection via magic number bytes (0x1f, 0x8b), decompressed with fflate +- **Batched packets**: supports both legacy single-packet and batched array format +- Reliable vs unreliable routing: camera movements and joystick sent unreliable, all other inputs reliable +- Reliable packet queue: max 32 pending, drops oldest on overflow +- Heartbeat: 5-second interval +- Sync: 2-second interval for RTT measurement +- RTT: exponential moving average with smoothing factor 0.5 +- Server version comparison for feature detection (`isServerAtLeastVersion`) +- Reconnection: iframe-based page reload for full context reset + +### Deserializer (client/src/network/Deserializer.ts) +- Static class mapping protocol short keys to readable property names +- Protocol uses single-letter keys (`i`, `p`, `r`, `m`, etc.) for bandwidth efficiency +- Colors: RGB arrays [0-255] -> Three.js Color [0-1] +- Vectors: protocol arrays -> `Vector3Like` objects +- Quaternions: protocol arrays -> `QuaternionLike` objects +- `'in' operator` pattern for distinguishing "not sent" (undefined) from "explicitly null" + +### Assets (client/src/network/Assets.ts) +- Static loaders: AudioLoader, TextureLoader, CubeTextureLoader, KTX2Loader, GLTFLoader +- Three.js Cache enabled globally +- CDN URL construction from server hostname +- **Optimized glTF resolution** (`getEffectiveGLTFlUri`): tries `.optimized/` variants with fallbacks +- URL existence check with error caching + +### Servers (client/src/network/Servers.ts) +- Server discovery: URL parameter `join=hostname` or modal prompt +- Health check with 8-second timeout using AbortController +- Version compatibility: minimum version check + client compat redirect table +- Local dev support: auto-tries `local.hytopiahosting.com:8080` and `localhost:8080` + +--- + +## 12. glTF/3D Asset Pipeline + +### GLTFManager (client/src/gltf/GLTFManager.ts) +- **Instanced mesh rendering**: merges identical meshes across entities into `InstancedMesh` +- Threshold: `USE_INSTANCED_MESH_THRESHOLD = 8` clones before instancing +- Transparent mesh threshold: `USE_INSTANCED_MESH_THRESHOLD_TRANSPARENT = 4` +- Hysteresis: different thresholds for creating (4x) vs deleting (8x) instanced meshes +- **Custom material**: `InstancedMeshBasicMaterial` extending `EmissiveMeshBasicMaterial` + - Per-instance: opacity, map index (texture array), light level, sky light, emissive color + - Custom shader injection via string replacement of GLSL code +- **Instanced textures**: `InstancedTexture` (DataArrayTexture) for per-instance texture variation +- Material conversion: `MeshStandardMaterial` -> `EmissiveMeshBasicMaterial` for GPU performance +- Directional face shading: `FACE_SHADE_TOP`, `FACE_SHADE_SIDE`, `FACE_SHADE_BOTTOM` based on world normal Y + +### EmissiveMeshBasicMaterial (client/src/gltf/EmissiveMeshBasicMaterial.ts) +- Extends `MeshBasicMaterial` with custom emissive support +- Shader processing hook system for GLSL injection + +--- + +## 13. Block System + +### BlockConstants (client/src/blocks/BlockConstants.ts) +- Block type ID as `BlockId` (number, max 255 from Uint8Array) +- Face definitions: top, bottom, left, right, front, back +- Face shading: top=1.0, side=0.85, bottom=0.7 +- Block rotation matrices: 24 rotations (cube symmetry group) +- Light level constants: MAX_LIGHT_LEVEL, SKY_LIGHT_MAX_DISTANCE, SKY_LIGHT_BRIGHTNESS_LUT +- Water surface Y offset +- Buffer geometry data types + +### BlockType (client/src/blocks/BlockType.ts) +- Data class: id, name, isLiquid, textureUri, lightLevel, trimesh data +- Trimesh: custom collision geometry for non-cube blocks + +### BlockTypeManager / BlockTypeRegistry +- Manager: handles network events for block type updates, delegates to registry +- Registry: stores block types by ID, provides lookup + +### BlockMaterialManager (client/src/blocks/BlockMaterialManager.ts) +- Creates and manages materials for chunk mesh rendering +- Four material variants: opaque, transparent, opaque non-lit, transparent non-lit +- Liquid material: custom `ShaderMaterial` with wave animation, foam, ambient occlusion +- Solid materials: `MeshBasicMaterial` with custom shader hooks for lighting + +### BlockTextureAtlasManager (client/src/blocks/BlockTextureAtlasManager.ts) +- Texture atlas for all block textures +- Packed into single texture for minimal draw calls +- Metadata stored in JSON, loaded by worker and main thread + +--- + +## 14. Particles System + +### ParticlesManager (client/src/particles/ParticlesManager.ts) +- Manages particle emitters from server +- Supports attachment to entities/nodes + +### ParticleEmitter / ParticleEmitterCore +- GPU-based particle system +- Properties: color, opacity, size interpolation over lifetime +- Orientation modes: billboard, billboardY, fixed, velocity-aligned +- Variance support for randomized particle properties + +--- + +## 15. Players System + +### PlayerManager (client/src/players/PlayerManager.ts) +- Tracks connected players by ID +- Handles player join/leave events from server + +### Player (client/src/players/Player.ts) +- Data class: id, username, profilePictureUrl + +--- + +## 16. Settings System + +### SettingsManager (client/src/settings/SettingsManager.ts) +- **Quality presets**: ULTRA, HIGH, MEDIUM, LOW, LOWEST -- each defines resolution multiplier, view distance, fog, post-processing, environmental animations, FPS cap +- Dynamic quality adjustment based on FPS (auto-downgrade) +- Client settings: mouse sensitivity, touch sensitivity, zoom sensitivity, distant block view mode +- Emits `ClientSettingsEventType.Update` on changes + +--- + +## 17. Mobile Support + +### MobileManager (client/src/mobile/MobileManager.ts) +- Static `isMobile` detection using `is-mobile` library + touch/pointer checks +- Virtual joystick via `nipplejs` library +- Move zone: left 40% of screen +- Camera zone: right 60% of screen +- Pinch-to-zoom support for camera +- Force-based walk/run detection: `WALK_FORCE_THRESHOLD = 0.1`, `RUN_FORCE_THRESHOLD = 0.75` + +--- + +## 18. Three.js Extensions + +### CSS2DRenderer (client/src/three/CSS2DRenderer.ts) +- Custom CSS2D rendering for HTML elements positioned in 3D space +- Used for nametags and scene UIs + +### SelectiveOutlinePass (client/src/three/postprocessing/SelectiveOutlinePass.ts) +- Custom outline post-processing pass +- Supports per-entity outline color, thickness, opacity, occlusion + +### WhiteCoreBloomPass (client/src/three/postprocessing/WhiteCoreBloomPass.ts) +- Custom bloom pass with dynamic threshold based on ambient light intensity + +### utils.ts (client/src/three/utils.ts) +- `lerp()`, `slerp()`, `lerpColor()`: epsilon-based convergence detection (EPSILON = 0.000001) +- `updateAABB()`: cached AABB computation for transparent sort +- `getTransparentSortKey()`: frame-cached distance key for sort stability + +--- + +## 19. Bridge System + +### BridgeManager (client/src/bridge/BridgeManager.ts) +- iframe <-> parent window communication via `postMessage` +- **Parent -> Client**: pointer lock, chat messages, quality presets, volume, sensitivity, debug toggle +- **Client -> Parent**: game ready, chat messages, key events, player updates, reconnect URL +- Typed message system with `BridgeMessageType` enum and `BridgeMessageDataMap` interface + +--- + +## 20. Services + +### Heartbeat (client/src/services/hytopia/heartbeat.ts) +- Matchmaking heartbeat service (side-effect import) + +### Profanity Filter (client/src/services/hytopia/profanityFilter.ts) +- Initialized on import for chat message filtering + +### Translations (client/src/services/hytopia/translations.ts) +- Internationalization support + +--- + +## 21. Key Design Patterns Summary + +### 1. Singleton Pattern +- `Game` class: lazy singleton via static getter +- `EventRouter`: eager singleton via static readonly property + +### 2. Manager/Service Locator Pattern +- All subsystems are "*Manager" classes instantiated by Game +- Cross-manager access through Game reference: `this._game.entityManager` + +### 3. Event-Driven Architecture +- Central `EventRouter` for decoupled communication +- Network packets -> deserialized -> emitted as typed events -> consumed by managers +- Renderer emits `Animate` event each frame for update orchestration + +### 4. Working Variables / Object Pooling +- Module-level pre-allocated `Vector3`, `Quaternion`, `Euler`, `Box3`, `Color` objects +- Pattern: `const vec3 = new Vector3()` at module scope, reused across method calls +- Prevents garbage collection pressure in hot paths + +### 5. Manual Matrix Management +- `matrixAutoUpdate = false` everywhere +- Matrices updated explicitly only when transforms change +- Multi-pass update in EntityManager ensures correct parent-child ordering + +### 6. Worker Thread Pattern +- CPU-intensive geometry generation offloaded to Web Worker +- Main thread and worker maintain parallel data structures (chunk registry, block types) +- Typed message passing with discriminated union types + +### 7. Dirty Flag Pattern +- `_solidMeshesInSceneDirty` in ChunkMeshManager +- `_needsMatrixUpdate`, `_needsMatrixWorldUpdate` in Entity +- `_needsLightLevelUpdate`, `_needsSkyLightUpdate` in Entity + +### 8. Interpolation Pattern +- Position, rotation, scale, fog, skybox color all use lerp/slerp +- Configurable interpolation time with server-controlled overrides +- Epsilon-based convergence detection to stop interpolation + +### 9. View Distance + Frustum Culling +- Two-stage visibility: view distance check (2D, per-frame) then frustum culling +- Scene graph add/remove instead of visibility toggle for GPU optimization + +### 10. Protocol Short Keys +- Network protocol uses minimal single/two-letter keys (`i`, `p`, `r`, `m`, `sv`, etc.) +- Deserializer maps to readable names: `entity.i` -> `id`, `entity.p` -> `position` +- `'in' operator` distinguishes "field not sent" from "field sent as null" + +--- + +## 22. State Management + +- **No global state store** -- state distributed across manager instances +- Entity state: per-entity fields with dirty tracking +- Network state: last server tick, connection ID, RTT +- Settings state: quality presets, client settings in SettingsManager +- UI state: loaded HTML, scene UIs, pending templates + +--- + +## 23. Memory Management Patterns + +- **Audio node pooling**: max 64 active nodes, LRU eviction +- **Geometry disposal**: explicit `geometry.dispose()` in ChunkMeshManager, Entity +- **Material disposal**: explicit in skybox replacement +- **Texture caching**: Three.js Cache.enabled, error cache for URL existence checks +- **WeakMap usage**: `_modelNodeOverrideBaseTransforms` uses WeakMap for automatic cleanup +- **Pending promise tracking**: sets of pending promises (`_pendingGltfs`, `_pendingEffectiveUris`) for race condition handling + +--- + +## 24. File Organization + +``` +client/src/ + main.ts -- Entry point + Game.ts -- Singleton orchestrator + style.css -- Global styles + arrows/ -- Arrow rendering (Arrow, ArrowManager, ArrowConstants, ArrowStats) + audio/ -- Audio system (Audio, AudioManager, AudioStats) + blocks/ -- Block/voxel system (BlockType, managers, constants, utils) + bridge/ -- iframe communication (BridgeManager) + chunks/ -- Chunk system (Chunk, managers, constants, stats, volumes) + core/ -- Core rendering (Camera, Renderer, DebugPanel, DebugRenderer, PerformanceMetricsManager) + entities/ -- Entity system (Entity, EntityManager, StaticEntity, StaticEntityManager, constants, stats) + events/ -- Event system (EventRouter) + gltf/ -- glTF pipeline (GLTFManager, EmissiveMeshBasicMaterial, GLTFStats) + input/ -- Input handling (InputManager) + mobile/ -- Mobile support (MobileManager, utils) + network/ -- Networking (NetworkManager, Deserializer, Assets, Servers) + particles/ -- Particle system (ParticleEmitter, ParticleEmitterCore, ParticlesManager, constants) + players/ -- Player tracking (Player, PlayerManager) + services/hytopia/ -- External services (heartbeat, profanityFilter, translations) + settings/ -- Settings system (SettingsManager) + textures/ -- Custom textures (CustomTextureManager) + three/ -- Three.js extensions (CSS2DRenderer, postprocessing, utils) + types/ -- TypeScript declarations (fetch.d.ts) + ui/ -- UI system (UIManager, Modal, SceneUI, SceneUIStats, globals, templates) + workers/ -- Web workers (ChunkWorker, ChunkWorkerClient, ChunkWorkerConstants, BlockTextureAtlasManager) +``` From d93c2ee9437954cdb323a230aa15abec9043f127 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:57:57 +1100 Subject: [PATCH 04/19] docs: add protocol layer analysis report Comprehensive analysis of protocol/ covering: - 26 packets (2 bidirectional, 6 inbound, 18 outbound) - Tuple wire format [id, data, tick?] for minimal msgpack overhead - 44 schema files with extreme property name minification - Co-located TypeScript types + JSON Schema with JSONSchemaType - Custom binary framing protocol (4-byte big-endian, 32MB cap) - Load-time Ajv schema compilation via shared singleton - Schema composition via object spread (no \$ref) - Delta encoding via all-optional properties - Fail-fast duplicate packet ID detection Co-Authored-By: Claude Opus 4.6 --- analysis-output/protocol-architecture.md | 558 +++++++++++++++++++++++ 1 file changed, 558 insertions(+) create mode 100644 analysis-output/protocol-architecture.md diff --git a/analysis-output/protocol-architecture.md b/analysis-output/protocol-architecture.md new file mode 100644 index 00000000..5cc2ce10 --- /dev/null +++ b/analysis-output/protocol-architecture.md @@ -0,0 +1,558 @@ +# Hytopia Protocol Layer Architecture Analysis + +## 1. Package Structure & Entry Points + +### Package Configuration +**`protocol/package.json:1-26`** - Published as `@hytopia.com/server-protocol` (v1.4.56). Dependencies: `ajv@^8.17.1` for runtime JSON Schema validation. Dev dependency `quicktype@^23.0.170` suggests codegen from JSON Schema was explored but the codebase uses hand-written TypeScript types co-located with schemas. + +### TypeScript Configuration +**`protocol/tsconfig.json:1-14`** - Targets ESNext with strict mode enabled (`strict`, `noImplicitReturns`, `noImplicitAny`, `noImplicitThis`, `strictNullChecks`). Include paths: `packets/**/*.ts`, `schemas/**/*.json`, `shared/**/*.ts`. Module resolution: `node` with ESNext modules. + +### Entry Point Chain +**`protocol/index.ts:1-3`** - Dual export pattern: +```typescript +import * as protocol from './exports'; +export * from './exports'; +export default protocol; +``` +Consumers can use either `import protocol from '@hytopia.com/server-protocol'` (namespace object) or `import { PacketId, createPacket } from '@hytopia.com/server-protocol'` (named destructuring). + +**`protocol/exports.ts:1-6`** - Barrel file re-exporting all public API: +- `./packets/bidirectional` - Connection, Heartbeat +- `./packets/inbound` - Client-to-server packets +- `./packets/outbound` - Server-to-client packets +- `./packets/PacketCore` - Core types and utilities +- `./packets/PacketDefinitions` - Registry and validation +- `./schemas` - All schema types and validators + +--- + +## 2. Packet System Architecture + +### 2.1 PacketId Enum & Bandwidth Optimization +**`protocol/packets/PacketCore.ts:23-59`** - Packet IDs use deliberate numeric ranges to optimize msgpack wire size: + +| Range | Category | Msgpack Bytes | Count | +|-------|----------|---------------|-------| +| 0-31 | Standard Inbound | 1 byte | 5 defined | +| 32-127 | Standard Outbound | 1 byte | 16 defined | +| 116-127 | Standard Bidirectional | 1 byte | 2 defined | +| 128-191 | Debug Inbound | 2 bytes | 1 defined | +| 192-255 | Debug Outbound | 2 bytes | 2 defined | + +The comment at `PacketCore.ts:8-21` explicitly states the rationale: standard packets fit in 1 byte for minimal overhead on high-frequency game traffic, while debug packets accept 2 bytes since they are infrequent. + +### 2.2 Core Type Definitions +**`protocol/packets/PacketCore.ts:65-87`** + +```typescript +// Packet definition = compile-time contract +interface IPacketDefinition { + id: TId; + schema: JSONSchemaType; + validate: ValidateFunction; +} + +// Wire packet = tuple [packetId, data, optionalWorldTick] +type IPacket = [ + TId, // packet id + TSchema, // packet data + WorldTick?, // world tick (optional) +]; + +// WorldTick = number (game server tick counter) +type WorldTick = number; +``` + +**Key design pattern**: Packets are represented as fixed-position tuples `[id, data, tick?]`, not objects. This minimizes serialization overhead since arrays are more compact than objects in msgpack. + +**Utility types** (`PacketCore.ts:79-87`): +- `AnyPacket = IPacket` - type-erased packet +- `AnyPacketDefinition = IPacketDefinition` - type-erased definition +- `AnySchema = unknown` - raw schema data +- `Serializable` interface with `serialize(): AnySchema` method + +### 2.3 Packet Factory Function +**`protocol/packets/PacketCore.ts:89-105`** - `createPacket()`: +```typescript +function createPacket( + packetDef: IPacketDefinition, + data: TSchema, + worldTick?: WorldTick, +): IPacket +``` +1. Validates `data` against the packet definition's compiled Ajv validator +2. Throws with Ajv error text if validation fails +3. Returns tuple `[id, data]` or `[id, data, worldTick]` + +Validation happens at packet **creation** time, not at send time -- fail-fast pattern. + +### 2.4 Packet Definition Factory +**`protocol/packets/PacketCore.ts:165-174`** - `definePacket()`: +```typescript +function definePacket( + id: TId, + schema: JSONSchemaType, +): IPacketDefinition +``` +Compiles the JSON Schema into an Ajv `ValidateFunction` at module load time via `Ajv.instance.compile(schema)`. This means all schemas are compiled once and cached by the shared Ajv singleton. + +### 2.5 Binary Framing Protocol +**`protocol/packets/PacketCore.ts:4-5,107-187`** - Custom length-prefixed framing for WebSocket binary transport: + +- `FRAME_HEADER_SIZE = 4` bytes (uint32 big-endian length prefix) +- `MAX_FRAME_BUFFER_SIZE = 32 * 1024 * 1024` (32MB cap) + +**`framePacketBuffer()`** (`PacketCore.ts:176-187`): Prepends 4-byte big-endian uint32 length header to a packet buffer. + +**`createPacketBufferUnframer()`** (`PacketCore.ts:107-163`): Returns a stateful closure that: +1. Maintains a growable internal buffer (512KB initial, doubles as needed, 32MB cap) +2. Appends incoming chunks +3. Extracts complete frames by reading the 4-byte length prefix +4. Calls `onMessage` callback with zero-copy `subarray()` views +5. Uses `copyWithin()` to shift remaining data after processing (avoids allocation) +6. Discards data if buffer exceeds 32MB (logs error, resets) + +This is a streaming binary framing protocol operating over WebSocket binary frames. + +### 2.6 Packet Registration & Validation +**`protocol/packets/PacketDefinitions.ts:1-37`** + +Auto-registration pattern at module load: +```typescript +const registeredPackets = new Map(); + +const allPackets = { ...bidirectionalPackets, ...inboundPackets, ...outboundPackets }; +for (const packet of Object.values(allPackets)) { + if ('id' in packet && 'schema' in packet) { + const definition = packet as AnyPacketDefinition; + if (registeredPackets.has(definition.id)) { + throw new Error(`Packet with id ${definition.id} is already registered.`); + } + registeredPackets.set(definition.id, definition); + } +} +``` + +Duplicate ID detection at load time via `throw`. The duck-typing check (`'id' in packet && 'schema' in packet`) filters out type exports from the barrel modules. + +**`isValidPacket()`** (`PacketDefinitions.ts:24-37`): Runtime validation for incoming packets: +1. Checks structural integrity (array, numeric id >= 0, data present, optional numeric tick >= 0) +2. Looks up packet definition by ID from registry +3. Runs Ajv validation on the data payload +4. Returns type guard `packet is AnyPacket` + +--- + +## 3. Packet Definition Patterns + +### 3.1 Bidirectional Packets (2 packets) + +**`protocol/packets/bidirectional/Connection.ts:1-11`**: +```typescript +export type ConnectionPacket = IPacket; +export const connectionPacketDefinition = definePacket(PacketId.CONNECTION, connectionSchema); +``` + +**`protocol/packets/bidirectional/Heartbeat.ts:1-11`**: Same pattern. + +### 3.2 Inbound Packets (6 packets, client-to-server) + +All follow identical structure (`ChatMessageSend.ts:1-11`, `DebugConfig.ts:1-11`, `Input.ts:1-11`, `StateRequest.ts:1-11`, `SyncRequest.ts:1-11`, `UIDataSend.ts:1-11`): + +```typescript +import { definePacket, PacketId } from '../PacketCore'; +import type { IPacket } from '../PacketCore'; +import { schemaNameSchema } from '../../schemas/SchemaName'; +import type { SchemaNameSchema } from '../../schemas/SchemaName'; + +export type XxxPacket = IPacket; +export const xxxPacketDefinition = definePacket(PacketId.XXX, schemaNameSchema); +``` + +No inbound packet includes `WorldTick` in its type -- tick is server-side only. + +### 3.3 Outbound Packets (18 packets, server-to-client) + +All outbound packets include `WorldTick` in their type via intersection: + +```typescript +export type AudiosPacket = IPacket & [WorldTick]; +``` + +This pattern (`& [WorldTick]`) appends the world tick as a required third element in the tuple. Every outbound packet carries the server tick for client-side interpolation/reconciliation. + +### 3.4 Universal Packet File Template + +Every packet definition file follows this exact 11-12 line template: +1. Import `definePacket` and `PacketId` from `PacketCore` +2. Import type `IPacket` from `PacketCore` +3. Import concrete schema (value) from `schemas/` +4. Import concrete schema type from `schemas/` +5. (Outbound only) Import type `WorldTick` from `PacketCore` +6. Export type alias binding `IPacket` +7. Export const definition via `definePacket(PacketId.X, schema)` + +No packet file contains any business logic. All are pure wiring. + +### 3.5 Index Barrel Files + +- `protocol/packets/bidirectional/index.ts:1-2` - Re-exports Connection, Heartbeat +- `protocol/packets/inbound/index.ts:1-6` - Re-exports all 6 inbound packets +- `protocol/packets/outbound/index.ts:1-18` - Re-exports all 18 outbound packets + +All use `export * from './PacketName'` pattern. + +--- + +## 4. Schema Validation Architecture + +### 4.1 Shared Ajv Singleton +**`protocol/shared/Ajv.ts:1-8`**: +```typescript +import Ajv from "ajv"; +export default class extends Ajv { + public static readonly instance = new Ajv(); +} +``` +Anonymous class extending Ajv with a static singleton. The comment explains: "per ajv docs, this is for performance around compiled validate function caching & more." All `definePacket()` calls use `Ajv.instance.compile(schema)`, so all compiled validators share the same Ajv instance for optimal caching. + +### 4.2 Schema Design Patterns + +#### Pattern: Extreme Property Name Minification +Every schema uses 1-3 character property keys with inline comments documenting meaning: + +```typescript +// From Entity.ts:16-39 +export type EntitySchema = { + i: number; // entity id + bh?: VectorSchema; // block half extents + bt?: string; // block texture uri + e?: boolean; // environmental + // ... +} +``` + +This is a bandwidth optimization -- property names are serialized over the wire via msgpack, so shorter keys = smaller payloads. The inline comments serve as documentation. + +**Abbreviation conventions observed**: +- `i` = id (universal) +- `p` = position (spatial schemas), player id (chat), play (animation), paused (particle), pointer (UI) +- `r` = rotation (entity), remove/removed (with `rm`), rate (particle), restart (animation) +- `rm` = removed/remove (universal deletion flag) +- `n` = name (universal) +- `m` = model uri (entity), message (chat), mode (camera) +- `v` = volume (audio), velocity (particle), view distance (sceneUI), vertices (debug) +- `c` = coordinate (block), color (various), colors (debug) +- `s` = skyboxUri (world), start (audio), stop (animation), state (sceneUI) +- `o` = offset (many schemas), opacity (outline/particle) +- `e` = entity attachment id (audio/light/particle/sceneUI), environmental (entity) +- `t` = type (light), texture uri (particle), template id (sceneUI), timestep (world) + +#### Pattern: Singular/Plural Schema Pairs +Most domain schemas come in pairs -- a singular schema for one item and a plural schema wrapping it in an array: + +| Singular | Plural | Relationship | +|----------|--------|-------------| +| `Audio.ts` | `Audios.ts` | `AudiosSchema = AudioSchema[]` | +| `Block.ts` | `Blocks.ts` | `BlocksSchema = BlockSchema[]` | +| `BlockType.ts` | `BlockTypes.ts` | `BlockTypesSchema = BlockTypeSchema[]` | +| `ChatMessage.ts` | `ChatMessages.ts` | `ChatMessagesSchema = ChatMessageSchema[]` | +| `Chunk.ts` | `Chunks.ts` | `ChunksSchema = ChunkSchema[]` | +| `Entity.ts` | `Entities.ts` | `EntitiesSchema = EntitySchema[]` | +| `Light.ts` | `Lights.ts` | `LightsSchema = LightSchema[]` | +| `ParticleEmitter.ts` | `ParticleEmitters.ts` | `ParticleEmittersSchema = ParticleEmitterSchema[]` | +| `PhysicsDebugRaycast.ts` | `PhysicsDebugRaycasts.ts` | `PhysicsDebugRaycastsSchema = PhysicsDebugRaycastSchema[]` | +| `Player.ts` | `Players.ts` | `PlayersSchema = PlayerSchema[]` | +| `SceneUI.ts` | `SceneUIs.ts` | `SceneUIsSchema = SceneUISchema[]` | +| `UIData.ts` | `UIDatas.ts` | `UIDatasSchema = UIDataSchema[]` | + +The plural file always follows the same 10-line template: +```typescript +import { singularSchema } from './Singular'; +import type { JSONSchemaType } from 'ajv'; +import type { SingularSchema } from './Singular'; + +export type PluralsSchema = SingularSchema[]; + +export const pluralsSchema: JSONSchemaType = { + type: 'array', + items: { ...singularSchema }, +} +``` + +Note the spread `{ ...singularSchema }` pattern for embedding the singular schema into the array items -- this avoids schema `$ref` and keeps everything inline. + +#### Pattern: Co-located Type + Schema +Every schema file exports BOTH a TypeScript type AND a matching JSON Schema: +```typescript +export type FooSchema = { ... }; +export const fooSchema: JSONSchemaType = { ... }; +``` +The `JSONSchemaType` generic from Ajv ensures the schema structurally matches the TypeScript type at compile time. This provides bidirectional type safety: TypeScript checks the schema matches the type, and Ajv validates runtime data matches the schema. + +#### Pattern: Nullable Optional Properties +Optional properties use Ajv's `nullable: true` pattern: +```typescript +// TypeScript +type AudioSchema = { i: number; a?: string; } + +// JSON Schema +properties: { + i: { type: 'number' }, + a: { type: 'string', nullable: true }, +} +``` +Required fields are listed in `required: [...]`. Optional fields are not in `required` and have `nullable: true`. + +#### Pattern: additionalProperties Control +- Most schemas: `additionalProperties: false` (strict, no extra properties allowed) +- Exception: `UIData.ts:7-11` uses `additionalProperties: true` for arbitrary key-value data +- Exception: `Camera.ts` and `Input.ts` have no `required` array (all properties optional) -- camera and input states are delta-based + +### 4.3 Primitive Schema Types + +**Tuple schemas** for compact spatial data: + +**`Vector.ts:1-17`**: `VectorSchema = [number, number, number]` -- 3-element fixed tuple for x/y/z +**`Quaternion.ts:1-19`**: `QuaternionSchema = [number, number, number, number]` -- 4-element fixed tuple for x/y/z/w +**`RgbColor.ts:1-18`**: `RgbColorSchema = [number, number, number]` with `minimum: 0, maximum: 255` constraints +**`VectorBoolean.ts:1-17`**: `VectorBooleanSchema = [boolean, boolean, boolean]` -- 3-element boolean tuple +**`HexColor.ts:1-7`**: `HexColorSchema = string` with regex `pattern: '^[0-9A-Fa-f]{6}$'` + +All use `minItems`/`maxItems` to enforce fixed lengths, and Ajv `items` as a tuple array (per-position type definitions). + +### 4.4 Schema Composition via Spread + +Schemas reference other schemas via object spread, not JSON Schema `$ref`: +```typescript +// From Entity.ts:45-46 +bh: { ...vectorSchema, nullable: true }, +``` + +This flattens the referenced schema inline. Used for: `vectorSchema`, `quaternionSchema`, `rgbColorSchema`, `outlineSchema`, `modelAnimationSchema`, `modelNodeOverrideSchema`, `hexColorSchema`. + +For array-of-objects composition, the items also use spread: +```typescript +// From Entity.ts:51 +ma: { type: 'array', items: { ...modelAnimationSchema }, nullable: true }, +``` + +### 4.5 Null/Empty Schemas + +Three schemas use null payload for signal-only packets: +- `HeartbeatSchema = null` (`Heartbeat.ts:3-8`) - `{ type: 'null', nullable: true }` +- `SyncRequestSchema = null` (`SyncRequest.ts:3-8`) - `{ type: 'null', nullable: true }` +- `NotificationPermissionRequestSchema = null` (`NotificationPermissionRequest.ts:3-8`) - `{ type: 'null', nullable: true }` + +One uses empty object: +- `StateRequestSchema = {}` (`StateRequest.ts:3-12`) - `{ type: 'object', properties: {}, additionalProperties: false }` + +--- + +## 5. Complex Schema Analysis + +### 5.1 Entity Schema (Most Complex) +**`protocol/schemas/Entity.ts:1-70`** - 23 optional properties + 1 required (`i`). Composes: +- `VectorSchema` (5 usages: bh, p, sv, position, scale) +- `QuaternionSchema` (1 usage: r) +- `RgbColorSchema` (2 usages: ec, t) +- `ModelAnimationSchema[]` (1 usage: ma) +- `ModelNodeOverrideSchema[]` (1 usage: mo) +- `OutlineSchema` (1 usage: ol) + +### 5.2 ParticleEmitter Schema +**`protocol/schemas/ParticleEmitter.ts:1-94`** - 38 optional properties + 1 required (`i`). Most properties are numeric with variance counterparts (e.g., `l` lifetime + `lv` lifetime variance, `ss` size start + `ssv` size start variance). + +### 5.3 Input Schema +**`protocol/schemas/Input.ts:1-95`** - 36 optional boolean key-press flags + 6 optional numeric/vector fields. No required properties. Uses string literal keys for number keys (`'0'` through `'9'`). + +### 5.4 Camera Schema +**`protocol/schemas/Camera.ts:1-47`** - 17 optional properties, no required. All camera state is delta-encoded. + +--- + +## 6. Type Safety Flow + +### Compile-time safety chain: +1. `type FooSchema = { i: number; ... }` -- TypeScript type definition +2. `const fooSchema: JSONSchemaType` -- Ajv generic ensures schema matches type +3. `definePacket(PacketId.FOO, fooSchema)` -- returns `IPacketDefinition` +4. `type FooPacket = IPacket` -- wire format type +5. `createPacket(fooPacketDefinition, data)` -- type-checks data against FooSchema + +### Runtime safety chain: +1. `Ajv.instance.compile(schema)` at module load -- pre-compiles validator +2. `createPacket()` calls `packetDef.validate(data)` -- validates outgoing data +3. `isValidPacket()` calls `packetDef.validate(packet[1])` -- validates incoming data + +The dual TypeScript + Ajv approach ensures both compile-time type correctness and runtime data integrity for untrusted client input. + +--- + +## 7. Naming Conventions + +### File Naming +- **PascalCase** for all TypeScript files: `PacketCore.ts`, `PacketDefinitions.ts`, `Connection.ts`, `ChatMessageSend.ts` +- **Singular** for individual schemas: `Audio.ts`, `Block.ts`, `Entity.ts` +- **Plural** for collection schemas: `Audios.ts`, `Blocks.ts`, `Entities.ts` +- Packet files named after their packet concept: `ChatMessageSend.ts` (inbound action), `ChatMessages.ts` (outbound state) + +### Export Naming +- Types: `PascalCase` with `Schema` suffix for schemas, `Packet` suffix for packets: `AudioSchema`, `AudiosPacket` +- Schema constants: `camelCase` with `Schema` suffix: `audioSchema`, `audiosSchema` +- Packet definitions: `camelCase` with `PacketDefinition` suffix: `audiosPacketDefinition`, `connectionPacketDefinition` +- Enum values: `SCREAMING_SNAKE_CASE`: `SYNC_REQUEST`, `CHAT_MESSAGE_SEND` + +### Property Naming in Schemas +- 1-3 character abbreviated keys for wire format: `i`, `p`, `rm`, `bh`, `bt` +- Inline `//` comments document each key's meaning on the same line as the type definition +- Alphabetical ordering is NOT enforced -- properties are grouped by semantic meaning + +### Import Conventions +- Value imports: `import { fooSchema } from './Foo'` +- Type-only imports: `import type { FooSchema } from './Foo'` +- Ajv type imports: `import type { JSONSchemaType } from 'ajv'` +- Consistent separation of value and type imports (type-only import syntax used throughout) + +--- + +## 8. Directory Structure Summary + +``` +protocol/ + index.ts -- Dual re-export entry point + exports.ts -- Barrel file for all public API + package.json -- NPM package config + tsconfig.json -- TypeScript strict config + shared/ + Ajv.ts -- Singleton Ajv instance + packets/ + PacketCore.ts -- Core types, factories, framing (187 lines, largest file) + PacketDefinitions.ts -- Auto-registration, isValidPacket (37 lines) + bidirectional/ + index.ts -- Barrel (2 exports) + Connection.ts -- CONNECTION packet (11 lines) + Heartbeat.ts -- HEARTBEAT packet (11 lines) + inbound/ + index.ts -- Barrel (6 exports) + ChatMessageSend.ts + DebugConfig.ts + Input.ts + StateRequest.ts + SyncRequest.ts + UIDataSend.ts + outbound/ + index.ts -- Barrel (18 exports) + Audios.ts + BlockTypes.ts + Blocks.ts + Camera.ts + ChatMessages.ts + Chunks.ts + Entities.ts + Lights.ts + NotificationPermissionRequest.ts + ParticleEmitters.ts + PhysicsDebugRaycasts.ts + PhysicsDebugRender.ts + Players.ts + SceneUIs.ts + SyncResponse.ts + UI.ts + UIDatas.ts + World.ts + schemas/ + index.ts -- Barrel (44 exports) + -- Primitives -- + Vector.ts -- [number, number, number] + VectorBoolean.ts -- [boolean, boolean, boolean] + Quaternion.ts -- [number, number, number, number] + RgbColor.ts -- [number, number, number] with 0-255 range + HexColor.ts -- string with hex regex + -- Domain Schemas (singular) -- + Audio.ts, Block.ts, BlockType.ts, Camera.ts, ChatMessage.ts, + Chunk.ts, Connection.ts, DebugConfig.ts, Entity.ts, Heartbeat.ts, + Input.ts, Light.ts, ModelAnimation.ts, ModelNodeOverride.ts, + NotificationPermissionRequest.ts, Outline.ts, ParticleEmitter.ts, + PhysicsDebugRaycast.ts, PhysicsDebugRender.ts, Player.ts, + SceneUI.ts, StateRequest.ts, SyncRequest.ts, SyncResponse.ts, + UI.ts, UIData.ts, World.ts + -- Collection Schemas (plural) -- + Audios.ts, Blocks.ts, BlockTypes.ts, ChatMessages.ts, Chunks.ts, + Entities.ts, Lights.ts, ParticleEmitters.ts, PhysicsDebugRaycasts.ts, + Players.ts, SceneUIs.ts, UIDatas.ts +``` + +--- + +## 9. Unique Protocol Layer Patterns + +### 9.1 Msgpack-Aware ID Sizing +The `PacketId` enum is explicitly designed around msgpack's variable-length integer encoding. IDs 0-127 fit in 1 byte, 128-255 in 2 bytes. High-frequency game packets use the 1-byte range; debug packets use the 2-byte range. + +### 9.2 Tuple-Based Wire Format +Packets are tuples `[id, data, tick?]`, not objects. This saves 3 property name strings per packet over the wire in msgpack encoding. + +### 9.3 Inline Schema Composition (No $ref) +All schema composition uses JavaScript spread (`{ ...vectorSchema, nullable: true }`) rather than JSON Schema `$ref`. This keeps schemas self-contained after module evaluation and avoids Ajv's `$ref` resolution overhead. + +### 9.4 Dual Type Safety +TypeScript `type` + Ajv `JSONSchemaType` ensures the schema is structurally correct at compile time. The `ValidateFunction` then validates data at runtime. This two-layer approach is particularly important because the protocol boundary crosses trust boundaries (client-to-server). + +### 9.5 Load-Time Compilation +All Ajv validators are compiled at module import time via `definePacket()`. There is zero runtime compilation cost during gameplay. + +### 9.6 Zero-Copy Framing +The `createPacketBufferUnframer` returns `subarray()` views instead of copies, with a documented constraint that the callback must process immediately before the buffer is modified. + +### 9.7 Delta-Encoded Optional Fields +Schemas like `Camera`, `Entity`, and `Input` have all-optional properties, enabling delta encoding: only changed fields need to be sent. The `rm?: boolean` field on many schemas signals entity/resource removal. + +### 9.8 Fail-Fast Registration +`PacketDefinitions.ts:16-18` throws at module load if two packets register with the same ID, preventing subtle runtime bugs. + +--- + +## 10. Import Style Consistency + +A minor inconsistency exists in how `JSONSchemaType` is imported: +- Most files: `import type { JSONSchemaType } from 'ajv'` (type-only import) +- `Connection.ts:1`: `import { JSONSchemaType } from 'ajv'` (value import) +- `ModelAnimation.ts:1`: `import { JSONSchemaType } from 'ajv'` (value import) + +Since `JSONSchemaType` is only used as a type, these should all be type-only imports. The inconsistency has no runtime effect due to TypeScript's type erasure but breaks the otherwise uniform convention. + +Similarly, `Camera.ts:3-4` imports its schema from the barrel `../../schemas` instead of the specific file `../../schemas/Camera`, unlike all other outbound packets which import from the specific schema file. + +--- + +## 11. Schema Validation Constraints Summary + +| Constraint Type | Example | Files | +|----------------|---------|-------| +| `minimum`/`maximum` | `RgbColor: 0-255`, `Audio.v: 0-1`, `Light.pe: 0-1` | RgbColor.ts, Audio.ts, Light.ts | +| `pattern` (regex) | `HexColor: ^[0-9A-Fa-f]{6}$` | HexColor.ts | +| `minItems`/`maxItems` | `Vector: 3/3`, `Quaternion: 4/4`, `Chunk.b: 4096/4096` | Vector.ts, Quaternion.ts, Chunk.ts | +| `additionalProperties: false` | All schemas except UIData | Nearly all files | +| `additionalProperties: true` | Arbitrary user data | UIData.ts | +| `nullable: true` | All optional properties | Pervasive | +| `type: 'null'` | Signal-only packets | Heartbeat.ts, SyncRequest.ts, NotificationPermissionRequest.ts | + +--- + +## 12. Data Flow Summary + +``` +Server Wire (msgpack + framing) Client + | | + |-- createPacket(def, data) -----> [id, data, tick] -----> framePacketBuffer() -->| + | | + |<-- isValidPacket(packet) <----- [id, data] <--------- unframer() ---| + | | + | validate(data) at both ends | +``` + +Outbound: Server creates typed packet -> validates -> serializes to tuple -> frames with 4-byte header -> sends over WebSocket. + +Inbound: Client sends binary -> unframer extracts complete messages -> deserialize -> `isValidPacket()` validates structure + schema -> process. From c70568d28e0641f05d3b1493ca15b9b1add8a6e2 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:58:08 +1100 Subject: [PATCH 05/19] docs: add coding style conventions report Exhaustive style guide extracted from 25+ files covering: - Naming: PascalCase classes with role suffixes, camelCase methods with verb prefixes, _underscore private fields/methods, UPPER_SNAKE_CASE constants - Import/export: default exports for classes, named for enums/types, import type usage - File organization: consistent 9-section class structure - One-liner getters, named setX() methods (no property setters) - TSDoc: comprehensive on server (with Category tags), minimal on client - Error messages: ClassName.methodName(): Description format - Arrow functions for event handlers - 7 documented inconsistencies between server and client conventions Co-Authored-By: Claude Opus 4.6 --- analysis-output/style-conventions.md | 885 +++++++++++++++++++++++++++ 1 file changed, 885 insertions(+) create mode 100644 analysis-output/style-conventions.md diff --git a/analysis-output/style-conventions.md b/analysis-output/style-conventions.md new file mode 100644 index 00000000..d071708a --- /dev/null +++ b/analysis-output/style-conventions.md @@ -0,0 +1,885 @@ +# Hytopia Codebase Style Conventions + +Comprehensive analysis extracted from 25+ files across `server/src/`, `client/src/`, and `protocol/`. + +--- + +## 1. Naming Conventions + +### 1.1 Class Names: PascalCase with Role Suffixes + +Classes use PascalCase. Domain objects have no suffix; coordination/infrastructure classes use suffixes like `Manager`, `Registry`, `Builder`, `Handler`, `Loop`. + +```typescript +// DO: PascalCase, role-specific suffixes +class GameServer { } // server/src/GameServer.ts:104 +class PlayerManager { } // server/src/players/PlayerManager.ts:69 +class EntityManager { } // server/src/worlds/entities/EntityManager.ts:26 +class BlockTypeRegistry { } // server/src/worlds/blocks/BlockTypeRegistry.ts +class CollisionGroupsBuilder { } // server/src/worlds/physics/CollisionGroupsBuilder.ts +class ErrorHandler { } // server/src/errors/ErrorHandler.ts:30 +class WorldLoop { } // server/src/worlds/WorldLoop.ts:71 +class NetworkSynchronizer { } // server/src/networking/NetworkSynchronizer.ts +class EventRouter { } // server/src/events/EventRouter.ts:20 + +// DON'T: avoid generic suffixes like "Service", "Helper", "Util" +// No: class PlayerService { } +// No: class EntityHelper { } +``` + +### 1.2 Method Names: camelCase with Verb Prefixes + +Methods use camelCase. Standard verb prefixes: `get`, `set`, `is`, `has`, `create`, `load`, `emit`, `on`, `off`, `register`, `unregister`, `serialize`, `tick`. + +```typescript +// DO: verb-first camelCase +public getConnectedPlayers(): Player[] // server/src/players/PlayerManager.ts:128 +public getConnectedPlayersByWorld(world): Player[] // server/src/players/PlayerManager.ts:140 +public getDefaultWorld(): World // server/src/worlds/WorldManager.ts:134 +public setAmbientLightColor(color): void // server/src/worlds/World.ts:585 +public setPersistedData(data): void // server/src/players/Player.ts:451 +public isValidPacket(packet): boolean // protocol/packets/PacketDefinitions.ts:24 +public hasListeners(eventType): boolean // server/src/events/EventRouter.ts:138 +public createWorld(options): World // server/src/worlds/WorldManager.ts:93 +public loadMap(map): void // server/src/worlds/World.ts:509 +public registerEntity(entity): number // server/src/worlds/entities/EntityManager.ts:61 + +// DON'T +// No: public playerGet() +// No: public ambientLightColorSet() +``` + +### 1.3 Private Properties: Underscore Prefix + +All private fields use `_` prefix. This is universal across both server and client. + +```typescript +// DO: underscore prefix for private fields +private static _instance: GameServer; // server/src/GameServer.ts:106 +private _blockTextureRegistry = ...; // server/src/GameServer.ts:109 +private _world: World | undefined; // server/src/players/Player.ts:176 +private _connectionPlayers: Map = new Map(); // server/src/players/PlayerManager.ts:91 +private _entities: Map = new Map(); // server/src/worlds/entities/EntityManager.ts:31 +private _ws: WebSocket | undefined; // client/src/network/NetworkManager.ts:81 +private _game: Game; // client/src/entities/EntityManager.ts:45 + +// DON'T: no underscore for public; no double underscore +// No: private blockTextureRegistry = ...; +// No: private __instance; +``` + +### 1.4 Private Methods: Underscore Prefix + +All private methods also use `_` prefix. + +```typescript +// DO +private _leaveWorld() { } // server/src/players/Player.ts:469 +private _onChatMessageSendPacket = () => { }; // server/src/players/Player.ts:483 +private _onConnectionOpened() { } // server/src/players/PlayerManager.ts:159 +private _onClose = (): void => { }; // server/src/networking/Connection.ts:456 +private _cleanupConnections(): void { } // server/src/networking/Connection.ts:470 +private _logMessage(options): void { } // server/src/errors/ErrorHandler.ts:125 +private _tick = (tickDeltaMs): void => { } // server/src/worlds/WorldLoop.ts:149 +private _setupEventListeners() { } // client/src/entities/EntityManager.ts:59 (implied) + +// DON'T +// No: private leaveWorld() { } +// No: private handleConnectionOpened() { } +``` + +### 1.5 Constants: UPPER_SNAKE_CASE + +Module-level constants use `SCREAMING_SNAKE_CASE`. + +```typescript +// DO +const RECONNECT_WINDOW_MS = 30 * 1000; // server/src/networking/Connection.ts:15 +const FRAME_HEADER_SIZE = 4; // protocol/packets/PacketCore.ts:4 +const MAX_FRAME_BUFFER_SIZE = 32 * 1024 * 1024; // protocol/packets/PacketCore.ts:5 +const DEFAULT_ANIMATION_BLEND_TIME_S = 0.1; // client/src/entities/Entity.ts:43 +const TRANSFORM_INTERPOLATION_TIME_S = 0.04; // client/src/entities/Entity.ts:46 +const MAX_UPDATE_SKIP_FRAMES = 4; // client/src/entities/Entity.ts:52 +const NEAR_DISTANCE_SQUARED = 16 * 16; // client/src/entities/Entity.ts:53 +export const ENTITY_POSITION_UPDATE_THRESHOLD_SQ = 0.04 * 0.04; // server/src/worlds/entities/Entity.ts:35 +export const DEFAULT_ENTITY_RIGID_BODY_OPTIONS = { ... }; // server/src/worlds/entities/Entity.ts:29 +const HEARTBEAT_INTERVAL_MS = 5000; // client/src/network/NetworkManager.ts:31 +const DEBUG_QUERY_STRINGS = 'debug'; // client/src/Game.ts:27 +``` + +**Exception**: `debug` constant at client/src/Game.ts:27 uses camelCase-ish lowercase -- this is an inconsistency. + +### 1.6 Enums: PascalCase Names, UPPER_SNAKE_CASE Members + +Enum names are PascalCase. Most enum members use `UPPER_SNAKE_CASE`. Protocol wire enums use `UPPER_SNAKE_CASE` for numeric IDs. + +```typescript +// DO: PascalCase enum name, UPPER_SNAKE_CASE members +export enum GameServerEvent { + START = 'GAMESERVER.START', + STOP = 'GAMESERVER.STOP', +} // server/src/GameServer.ts:20 + +export enum PlayerEvent { + CHAT_MESSAGE_SEND = 'PLAYER.CHAT_MESSAGE_SEND', + JOINED_WORLD = 'PLAYER.JOINED_WORLD', +} // server/src/players/Player.ts:52 + +export enum WorldLoopEvent { + START = 'WORLD_LOOP.START', + TICK_START = 'WORLD_LOOP.TICK_START', +} // server/src/worlds/WorldLoop.ts:17 + +export enum PacketId { + SYNC_REQUEST = 0, + INPUT = 1, + ENTITIES = 38, +} // protocol/packets/PacketCore.ts:23 + +export enum ColliderShape { + NONE = 'none', + BALL = 'ball', + BLOCK = 'block', +} // server/src/worlds/physics/Collider.ts:31 +``` + +**Exception**: `CoefficientCombineRule` uses PascalCase members (e.g., `Average = 0`) at Collider.ts:19-24. This is inconsistent with the rest of the codebase. + +### 1.7 Enum Event String Format: `NAMESPACE.EVENT_NAME` + +Event enum string values consistently follow `UPPER_CASE_NAMESPACE.UPPER_CASE_EVENT_NAME` pattern, using `.` as separator: + +```typescript +// Pattern: 'NAMESPACE.EVENT_NAME' +'GAMESERVER.START' // GameServer.ts +'PLAYER.CHAT_MESSAGE_SEND' // Player.ts +'PLAYER_MANAGER.PLAYER_CONNECTED' // PlayerManager.ts +'WORLD.START' // World.ts +'WORLD_LOOP.TICK_START' // WorldLoop.ts +'ENTITY.SPAWN' // Entity.ts +'CONNECTION.OPENED' // Connection.ts +'PLAYER_CAMERA.SET_MODE' // PlayerCamera.ts +'BLOCK_TYPE.INTERACT' // BlockType.ts +``` + +### 1.8 Event Payload Interfaces: `{ClassName}EventPayloads` + +Each class with events exports a paired event payloads interface using computed property keys: + +```typescript +// DO: interface naming and structure +export interface GameServerEventPayloads { + [GameServerEvent.START]: { startedAtMs: number } + [GameServerEvent.STOP]: { stoppedAtMs: number } +} // server/src/GameServer.ts:31 + +export interface PlayerEventPayloads { + [PlayerEvent.JOINED_WORLD]: { player: Player, world: World } +} // server/src/players/Player.ts:68 + +export interface WorldLoopEventPayloads { + [WorldLoopEvent.TICK_START]: { worldLoop: WorldLoop, tickDeltaMs: number } +} // server/src/worlds/WorldLoop.ts:31 +``` + +### 1.9 File Naming: PascalCase.ts + +Files are named after their primary exported class/type, using PascalCase. + +``` +GameServer.ts, Player.ts, PlayerManager.ts, World.ts, WorldLoop.ts, +EventRouter.ts, Events.ts, Connection.ts, ErrorHandler.ts, +EntityManager.ts, Entity.ts, Collider.ts, Vector3.ts, +PacketCore.ts, PacketDefinitions.ts +``` + +Barrel files use lowercase `index.ts` (e.g., `protocol/index.ts`, `server/src/index.ts`). + +### 1.10 Interfaces and Types + +- **Interfaces**: Used for object shapes, options, event payloads, schemas. Named as nouns with optional suffixes: `Options`, `Payloads`, `Schema`, `Like`. +- **Types**: Used for union types, aliases, computed types. + +```typescript +// Interface: shape/contract +export interface WorldOptions { ... } // server/src/worlds/World.ts:69 +export interface BaseEntityOptions { ... } // server/src/worlds/entities/Entity.ts:49 +export interface WorldMap { ... } // server/src/worlds/World.ts:33 +export interface IPacketDefinition { ... } // protocol/packets/PacketCore.ts:65 +interface RgbColor { r: number; g: number; b: number; } // server/src/shared/types/RgbColor.ts:8 +interface Vector3Like { x: number; y: number; z: number; } // server/src/shared/types/math/Vector3Like.ts:7 + +// Type: alias/union +export type EntityOptions = BlockEntityOptions | ModelEntityOptions; // server/src/worlds/entities/Entity.ts:151 +export type PlayerInput = InputSchema; // server/src/players/Player.ts:42 +export type AnyPacket = IPacket; // protocol/packets/PacketCore.ts:79 +export type WorldTick = number; // protocol/packets/PacketCore.ts:71 +``` + +**Pattern**: "Like" suffix for lightweight shape interfaces used as parameter types (e.g., `Vector3Like`, `QuaternionLike`). + +--- + +## 2. Import/Export Patterns + +### 2.1 Import Ordering + +Imports follow this consistent order (no explicit delimiter, but grouped logically): + +1. External packages (`@dimforge/rapier3d-simd-compat`, `eventemitter3`, `ws`, `zlib`) +2. Internal modules via path alias `@/` (server) or relative `./`/`../` (client/protocol) +3. Type-only imports last (using `import type`) + +```typescript +// DO: external -> internal -> type-only +import RAPIER from '@dimforge/rapier3d-simd-compat'; // external +import BlockTextureRegistry from '@/textures/BlockTextureRegistry'; // internal +import EventRouter from '@/events/EventRouter'; // internal +import type World from '@/worlds/World'; // type-only +import type { RaycastHit } from '@/worlds/physics/Simulation'; // type-only + // server/src/GameServer.ts:1-10 +``` + +### 2.2 Default Exports for Classes + +Every class is exported as `export default class ClassName`. This is universal across server, client, and protocol. + +```typescript +// DO: default export for classes +export default class GameServer { } // server/src/GameServer.ts:104 +export default class Player { } // server/src/players/Player.ts:108 +export default class EventRouter { } // server/src/events/EventRouter.ts:20 +export default class World { } // server/src/worlds/World.ts:217 +export default class NetworkManager { } // client/src/network/NetworkManager.ts:80 +export default class Game { } // client/src/Game.ts:29 + +// DON'T: named export for primary class +// No: export class GameServer { } +``` + +### 2.3 Named Exports for Enums, Constants, Types, Functions + +Enums, constants, standalone types, and functions use named exports alongside the default class. + +```typescript +// DO: named exports alongside default export +export enum GameServerEvent { ... } // alongside export default class GameServer +export interface GameServerEventPayloads { } // alongside export default class GameServer +export const SUPPORTED_INPUTS = [...]; // alongside export default class Player +export function startServer(init) { } // alongside export default class GameServer + // server/src/GameServer.ts +``` + +### 2.4 Barrel File (index.ts) Re-export Pattern + +The server `index.ts` re-exports with explicit grouping by concept, using comments as section headers. Both default exports and named exports are re-exported. + +```typescript +// DO: grouped re-exports with comments +// AudioManager +export { default as AudioManager } from '@/worlds/audios/AudioManager'; + +// Player +export { default as Player, PlayerEvent, SUPPORTED_INPUTS } from '@/players/Player'; +export type { PlayerEventPayloads, PlayerInput } from '@/players/Player'; + // server/src/index.ts +``` + +### 2.5 Protocol index.ts: Namespace Re-export + +Protocol uses a dual pattern: re-export all from `exports.ts` plus a default namespace export. + +```typescript +import * as protocol from './exports'; +export * from './exports'; +export default protocol; // protocol/index.ts +``` + +### 2.6 `import type` Usage + +Type-only imports consistently use `import type` syntax. This is enforced across the codebase. + +```typescript +// DO: use import type for type-only imports +import type Connection from '@/networking/Connection'; // server/src/players/Player.ts:10 +import type Vector3Like from '@/shared/types/math/Vector3Like'; // throughout +import type { EntityOptions } from '@/worlds/entities/Entity'; // throughout +import type { JSONSchemaType } from 'ajv'; // protocol/schemas/Entity.ts:7 +``` + +--- + +## 3. Type Annotation Patterns + +### 3.1 Explicit Return Types on Public Methods + +Public methods have explicit return type annotations. Private methods sometimes omit them. + +```typescript +// DO: explicit return types on public +public getConnectedPlayers(): Player[] { ... } // PlayerManager.ts:128 +public get playerCount(): number { ... } // PlayerManager.ts:117 +public emit(eventType: string, payload: any): boolean { // EventRouter.ts:46 +public static serializePackets(packets): Buffer | void { // Connection.ts:155 +public async scheduleNotification(type, scheduledFor): Promise { // Player.ts:342 + +// Private methods often omit return type when obvious +private _leaveWorld() { ... } // Player.ts:469 +private _onConnectionDisconnected(connection: Connection) { ... } // PlayerManager.ts:173 +``` + +### 3.2 Generic Type Parameters: TPrefix + +Generic type parameters use `T` prefix convention. + +```typescript +// DO: T-prefixed generic params +public emit(...) // EventRouter.ts:44 +public on(...) // EventRouter.ts:225 +export function createPacket(...) // PacketCore.ts:89 +export interface IPacketDefinition // PacketCore.ts:65 +type EventListener = (payload: TPayload) => void; // client/src/events/EventRouter.ts:1 +``` + +### 3.3 Nullable Patterns + +The codebase consistently uses `| undefined` (not `| null`) for optional values. Optional properties use `?:` syntax. Explicit `undefined` used for runtime checks. + +```typescript +// DO: | undefined and ?: +private _world: World | undefined; // Player.ts:176 +public readonly profilePictureUrl: string | undefined; // Player.ts:131 +public get fogColor(): RgbColor | undefined { ... } // World.ts:407 +public get tag(): string | undefined { ... } // World.ts:490 +worldSelectionHandler?: (player: Player) => Promise; // PlayerManager.ts:88 + +// DON'T: avoid | null (used only where protocols/3rd-party require it) +``` + +### 3.4 `as const` and `satisfies` + +Used together for compile-time validation of constant arrays: + +```typescript +export const SUPPORTED_INPUTS = [ + 'w', 'a', 's', 'd', ... +] as const satisfies readonly (keyof InputSchema)[]; // Player.ts:24-34 +``` + +### 3.5 Method Overload Pattern for Type-Safe Events + +The EventRouter uses a consistent triple-overload pattern: typed overload, string fallback, and implementation: + +```typescript +public emit(eventType: TEventType, payload: EventPayloads[TEventType]): boolean; +public emit(eventType: string, payload: any): boolean; +public emit(eventType: string, payload: any): boolean { ... } + // EventRouter.ts:44-57 +``` + +--- + +## 4. Code Organization Within Files + +### 4.1 File Structure Order (Server Classes) + +``` +1. Imports (external -> internal -> type-only) +2. Module-level constants (SCREAMING_SNAKE_CASE) +3. Event enum (export enum XxxEvent { ... }) +4. Event payloads interface (export interface XxxEventPayloads { ... }) +5. Options/Config interfaces (export interface XxxOptions { ... }) +6. export default class Xxx { + a. Static private fields + b. Public readonly fields + c. Private fields (with @internal JSDoc) + d. Constructor (often private for singletons) + e. Static getter (instance) for singletons + f. Public getters (one-liner format for simple getters) + g. Public methods (alphabetical or logical grouping) + h. Internal public methods (marked @internal) + i. Private methods (underscore-prefixed) +} +``` + +**Evidence**: This exact pattern is followed in `GameServer.ts`, `Player.ts`, `World.ts`, `WorldLoop.ts`, `Connection.ts`, `PlayerManager.ts`, `WorldManager.ts`. + +### 4.2 One-Liner Getters + +Simple property getters are written on a single line: + +```typescript +// DO: single-line getters +public get id(): number { return this._id; } // World.ts:337 +public get input(): PlayerInput { return this._input; } // Player.ts:205 +public get isDuplicate(): boolean { return this._isDuplicate; } // Connection.ts:122 +public get world(): World { return this._world; } // WorldLoop.ts:131 +public get entityCount(): number { return this._entities.size; } // EntityManager.ts:49 +public get arrowManager(): ArrowManager { return this._arrowManager; } // Game.ts:97 + +// DON'T: multi-line for trivial getters +// No: +// public get id(): number { +// return this._id; +// } +``` + +### 4.3 Private Backing Fields with Public Getters (No Setters) + +The dominant pattern is: private `_field` + public getter, no public setter. State mutation is done through named `setX()` methods or internal methods. + +```typescript +// DO: named setter methods +private _ambientLightColor: RgbColor; +public get ambientLightColor(): RgbColor { return this._ambientLightColor; } +public setAmbientLightColor(color: RgbColor) { ... } // World.ts:222,344,585 + +private _interactEnabled: boolean = true; +public get isInteractEnabled(): boolean { return this._interactEnabled; } +public setInteractEnabled(enabled: boolean) { ... } // Player.ts:164,215,419 + +// DON'T: public setters +// No: public set ambientLightColor(color: RgbColor) { ... } +``` + +### 4.4 `@internal` JSDoc Annotation + +Internal implementation details are marked with `/** @internal */`. This is the sole JSDoc tag used for visibility control. + +```typescript +/** @internal */ +private static _instance: GameServer; // GameServer.ts:106 + +/** @internal */ +private _blockTextureRegistry = ...; // GameServer.ts:109 + +/** @internal */ +public constructor(connection, session) { ... } // Player.ts:182 + +/** @internal */ +public registerEntity(entity): number { ... } // EntityManager.ts:61 +``` + +--- + +## 5. Comment/Documentation Patterns + +### 5.1 TSDoc Format + +The server package uses comprehensive TSDoc with a consistent structured format: + +```typescript +/** + * Brief description of what it is. + * + * When to use: [use case]. + * Do NOT use for: [anti-use case]. + * + * @remarks + * Additional details, initialization order, caveats. + * Pattern: [recommended pattern]. + * Anti-pattern: [what to avoid]. + * + *

Events

+ * [event documentation, if applicable] + * + * @example + * ```typescript + * // code example + * ``` + * + * @param paramName - Description. + * @returns Description. + * + * **Requires:** [preconditions] + * **Side effects:** [mutations, emissions, I/O] + * + * @see `RelatedClass` + * + * **Category:** CategoryName + * @public + */ +``` + +**Evidence**: Consistent across `GameServer.ts:39-65`, `EventRouter.ts:7-19`, `World.ts:187-216`, `WorldLoop.ts:48-70`, `Connection.ts:49-63`, `Player.ts:91-107`. + +### 5.2 Category Tags + +Every public TSDoc block ends with `**Category:** CategoryName`. Categories seen: `Core`, `Events`, `Players`, `Entities`, `Physics`, `Networking`, `Blocks`, `Persistence`, `Utilities`, `Types`, `Math`. + +```typescript + * **Category:** Core + * @public +``` + +### 5.3 Inline Comments + +Inline comments use `//` style. They explain *why*, not *what*. Often placed above a line or at end of line. + +```typescript +// If an input packet has a sequence number, meaning it was sent +// over an unreliable, unordered UDP connection... // Player.ts:518-520 + +// Lazy init if none exist +this._defaultWorld ??= this.createWorld({ ... }); // WorldManager.ts:135 + +// prevent movement/actions if keys were pressed at time of disconnect +player.resetInputs(); // PlayerManager.ts:177 + +// TODO: Seperate and expand on global server vs worlds/room chat? +const message = packet[1].m; // Player.ts:488 +``` + +### 5.4 Client-Side: Minimal TSDoc + +The client-side code has almost no TSDoc. Methods are rarely documented. This is a clear asymmetry with the server. + +```typescript +// Client: no TSDoc, just code +export default class Game { // Game.ts:29 + public static get instance(): Game { ... } // Game.ts:89 - no doc + public async start(): Promise { ... } // Game.ts:123 - no doc +} + +export default class EntityManager { // client/src/entities/EntityManager.ts:44 + public constructor(game: Game) { ... } // no doc + public getEntity(id: number) { ... } // no doc +} +``` + +--- + +## 6. Error Handling Patterns + +### 6.1 ErrorHandler Static Methods + +Errors are handled through the static `ErrorHandler` class with three severity levels: + +```typescript +// DO: use ErrorHandler, not raw throw +ErrorHandler.warning('message'); // Logs warning, continues execution +ErrorHandler.error('message'); // Logs error, continues execution (returns void) +ErrorHandler.fatalError('message'); // Logs error, throws Error (returns never) + +// server/src/errors/ErrorHandler.ts +``` + +### 6.2 Error Message Format: `ClassName.methodName(): Description` + +Error messages follow a consistent format: `ClassName.methodName(): Human-readable description.` + +```typescript +// DO: class.method(): message format +ErrorHandler.error(`Connection._deserialize(): Invalid packet format. Packet: ${JSON.stringify(packet)}`); +// Connection.ts:506 + +ErrorHandler.error(`Connection.send(): Packet send failed. Error: ${error as Error}`); +// Connection.ts:429 + +ErrorHandler.warning(`PlayerManager._onConnectionClosed(): Connection ${connection.id} not in the PlayerManager._connectionPlayers map.`); +// PlayerManager.ts:211 + +ErrorHandler.fatalError(`EntityManager.registerEntity(): Entity ${entity.name} is already assigned the id ${entity.id}!`); +// EntityManager.ts:63 + +ErrorHandler.fatalError(`Failed to initialize the game engine, exiting. Error: ${error}`); +// GameServer.ts:87 +``` + +### 6.3 Try/Catch Usage + +Try/catch is used sparingly, mainly around I/O and event emission: + +```typescript +// EventRouter: catch individual listener errors +try { + this._emitter.emit(eventType, payload); +} catch (error) { + console.error(`EventRouter.emit(): Error emitting event "${eventType}":`, error); +} + // EventRouter.ts:49-54 + +// Connection: catch transport errors +try { + this._ws?.close(); + this._wt?.close(); +} catch (error) { + ErrorHandler.error(`Connection.disconnect(): ... Error: ${error as Error}`); +} + // Connection.ts:328-333 +``` + +### 6.4 Error Return as void + +Non-fatal `ErrorHandler.error()` and `ErrorHandler.warning()` return `void`, allowing early returns: + +```typescript +return ErrorHandler.error(`EventRouter.final(): Listener ... already exists.`); +// EventRouter.ts:121 + +return ErrorHandler.warning('Player.scheduleNotification(): Player must be in a world...'); +// Player.ts:344 +``` + +--- + +## 7. Formatting Patterns + +### 7.1 ESLint-Enforced Rules (from `server/eslint.config.js`) + +| Rule | Setting | +|------|---------| +| Indent | 2 spaces, `SwitchCase: 1` | +| Quotes | Single quotes, `avoidEscape: true` | +| Semicolons | Always | +| Trailing commas | Always in multiline | +| Object curly spacing | `{ spaces }` | +| Array bracket spacing | `[ spaces ]` (note: config has conflicting rules, see inconsistency below) | +| Arrow parens | `as-needed` (omit when single param) | +| Newline before return | Required | +| Linebreak style | Unix (LF) | +| `no-explicit-any` | OFF (any is allowed) | +| `no-unused-vars` | Error, except `_` prefixed args | +| `no-floating-promises` | Error | +| `await-thenable` | Error | +| `prefer-optional-chain` | Error | +| TSDoc syntax | Warn | + +**Note**: The ESLint config has a conflicting duplicate rule -- `array-bracket-spacing` is set to `'never'` on line 31 and then overridden to `'always'` on line 31 (second occurrence). The actual code uses `['always']` style (spaces inside brackets): + +```typescript +super([ x, y, z ]); // Vector3.ts:26 +return [ ...this._emitter.listeners(eventType), ... ]; // EventRouter.ts:154-157 +``` + +### 7.2 Blank Line Patterns + +- Blank line before `return` (enforced by ESLint `newline-before-return`) +- Blank line between methods +- No blank line after opening brace of class/method +- Blank line between logical sections within methods + +```typescript +public static get instance(): GameServer { + if (!this._instance) { + this._instance = new GameServer(); + } + + return this._instance; // blank line before return +} // GameServer.ts:140-146 +``` + +### 7.3 Ternary vs If/Else + +- Ternaries for simple assignments and nullish coalescing +- If/else for multi-line logic or side effects + +```typescript +// DO: ternary for simple value selection +this._ambientLightColor = options.ambientLightColor ?? { r: 255, g: 255, b: 255 }; +this._fogFar = options.fogFar ?? 550; // World.ts:303-309 + +// DO: if/else for complex logic +if (init.length > 0) { + initResult = init(GameServer.instance.worldManager.getDefaultWorld()); +} else { + initResult = (init as () => void | Promise)(); +} // GameServer.ts:74-79 +``` + +### 7.4 Early Return Pattern + +Extensively used to reduce nesting: + +```typescript +public joinWorld(world: World) { + if (this._world === world) { + return; // early return + } + // ... rest of logic +} // Player.ts:295-321 + +public start(): void { + if (this._loop.isStarted) return; // inline early return + // ... +} // World.ts:767 +``` + +### 7.5 Arrow Functions for Callbacks/Event Handlers + +Private event handler methods consistently use arrow function assignment (preserving `this` context): + +```typescript +// DO: arrow function assignment for event handlers +private _onChatMessageSendPacket = (packet: protocol.ChatMessageSendPacket) => { ... }; +private _onInputPacket = (packet: protocol.InputPacket) => { ... }; +private _onClose = (): void => { ... }; +private _tick = (tickDeltaMs: number): void => { ... }; +private _onTickError = (error: Error) => { ... }; + // Player.ts:483-577, Connection.ts:434-468, WorldLoop.ts:149-205 +``` + +### 7.6 Enum Value Alignment + +Enum values and event payload property values are right-aligned with whitespace padding for readability: + +```typescript +export enum PlayerEvent { + CHAT_MESSAGE_SEND = 'PLAYER.CHAT_MESSAGE_SEND', + INTERACT = 'PLAYER.INTERACT', + JOINED_WORLD = 'PLAYER.JOINED_WORLD', + LEFT_WORLD = 'PLAYER.LEFT_WORLD', + RECONNECTED_WORLD = 'PLAYER.RECONNECTED_WORLD', +} // Player.ts:52-60 + +export interface PlayerEventPayloads { + [PlayerEvent.CHAT_MESSAGE_SEND]: { player: Player, message: string } + [PlayerEvent.JOINED_WORLD]: { player: Player, world: World } +} // Player.ts:68-89 +``` + +### 7.7 Semicolons in Event Payload Interfaces + +Event payload interface members do NOT have trailing semicolons -- they are bare (no separator). Each line has a JSDoc comment above it. + +```typescript +export interface GameServerEventPayloads { + /** Emitted when the game server starts. */ + [GameServerEvent.START]: { startedAtMs: number } // no semicolon + + /** Emitted when the game server stops. */ + [GameServerEvent.STOP]: { stoppedAtMs: number } // no semicolon +} // GameServer.ts:31-37 +``` + +--- + +## 8. Architectural Patterns (Style-Relevant) + +### 8.1 Singleton Pattern + +Two variants are used: + +**Variant 1: Lazy static getter** (private constructor): +```typescript +private static _instance: GameServer; +private constructor() { } +public static get instance(): GameServer { + if (!this._instance) { this._instance = new GameServer(); } + return this._instance; +} // GameServer.ts:106-146, Game.ts:30-95 +``` + +**Variant 2: Eager static readonly** (private/no constructor): +```typescript +public static readonly instance: PlayerManager = new PlayerManager(); +private constructor() { ... } // PlayerManager.ts:75-76 + +public static readonly instance: WorldManager = new WorldManager(); + // WorldManager.ts:63 + +public static readonly instance: PersistenceManager = new PersistenceManager(); + // PersistenceManager.ts:28 +``` + +**Variant 3: Static readonly instance** (no private constructor): +```typescript +public static readonly globalInstance: EventRouter = new EventRouter(); + // EventRouter.ts:26 +``` + +### 8.2 Options Object Pattern + +Configuration is passed via a single `options` object parameter to constructors: + +```typescript +constructor(options: WorldOptions) { ... } // World.ts:299 +public createWorld(options: Omit) // WorldManager.ts:93 +``` + +### 8.3 Protocol Schema Short Keys + +Protocol schemas use single-letter or two-letter keys for wire efficiency: + +```typescript +export type EntitySchema = { + i: number; // entity id + bh?: VectorSchema; // block half extents + bt?: string; // block texture uri + e?: boolean; // environmental + m?: string; // model uri + p?: VectorSchema; // position + r?: QuaternionSchema; // rotation + rm?: boolean; // removed +} // protocol/schemas/Entity.ts:15-39 +``` + +--- + +## 9. Client vs Server Style Differences + +| Aspect | Server | Client | +|--------|--------|--------| +| Path aliases | `@/` prefix | Relative `./`/`../` | +| TSDoc | Comprehensive, structured | Minimal or absent | +| EventRouter | Full-featured (EventEmitter wrapper, final listeners, world scope) | Lightweight (Map) | +| Singleton access | `.instance` getter or `readonly instance` | `.instance` getter | +| Event enum naming | `{Class}Event` | `{Class}EventType` | +| Event payload convention | Interface with computed keys | Namespace with individual interfaces | +| Constructor visibility | `public` or `private` + `@internal` | `public` | + +Client-specific pattern -- namespaced event payloads: + +```typescript +// Client uses namespace + interfaces (differs from server) +export namespace NetworkManagerEventPayload { + export interface IAudiosPacket { ... } + export interface IBlocksPacket { ... } +} // client/src/network/NetworkManager.ts:59-78 + +// Server uses single interface with computed keys (preferred pattern) +export interface PlayerEventPayloads { + [PlayerEvent.JOINED_WORLD]: { ... } +} +``` + +--- + +## 10. Known Inconsistencies + +1. **ESLint `array-bracket-spacing`**: Duplicate rule (lines 31 vs 31) -- first says `'never'`, second says `'always'`. Code uses `'always'`. + +2. **`CoefficientCombineRule` enum members**: Uses PascalCase (`Average`, `Min`, `Multiply`, `Max`) while all other enums use UPPER_SNAKE_CASE. (Collider.ts:19-24) + +3. **Client event naming**: Uses `EventType` suffix (`NetworkManagerEventType`, `RendererEventType`, `WorkerEventType`, `ClientSettingsEventType`) while server uses `Event` suffix (`PlayerEvent`, `WorldEvent`, `EntityEvent`). + +4. **Client event payloads**: Uses `namespace` + `I`-prefixed interfaces (`NetworkManagerEventPayload.IAudiosPacket`) while server uses single interface with computed keys. + +5. **Client TSDoc**: Almost entirely absent compared to server's exhaustive documentation. + +6. **`DEBUG_QUERY_STRINGS`**: Uses lowercase value `'debug'` -- minor inconsistency in constant naming. (Game.ts:27) + +7. **Interface default export for simple types**: `RgbColor` and `Vector3Like` use `interface ... export default` pattern rather than `export default interface`. + +--- + +## 11. Summary of Critical Rules + +1. Classes: `PascalCase` + role suffix (`Manager`, `Registry`, `Builder`, `Handler`, `Loop`) +2. Methods: `camelCase` with verb prefix (`get`, `set`, `is`, `has`, `create`, `load`, `emit`, `on`) +3. Private fields/methods: `_underscore` prefix, always +4. Constants: `SCREAMING_SNAKE_CASE` +5. Enums: `PascalCase` name, `UPPER_SNAKE_CASE` members +6. Event strings: `'NAMESPACE.EVENT_NAME'` format +7. Default exports for classes, named exports for everything else +8. `import type` for type-only imports +9. One-liner getters for simple property access +10. Named `setX()` methods instead of property setters +11. `@internal` JSDoc for implementation details +12. `ErrorHandler.method()` for error handling, not raw `throw` +13. Error messages: `ClassName.methodName(): Description` +14. Arrow functions for event handlers and callbacks +15. 2-space indentation, single quotes, always semicolons, trailing commas +16. Blank line before `return` statements +17. Early returns to reduce nesting +18. Options object pattern for configuration From 4447e38e061bb7b92811ef1abe86e43b536e1d57 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:58:18 +1100 Subject: [PATCH 06/19] docs: add code smells analysis report 36 findings across 40+ files analyzed: - 4 Critical: NetworkSynchronizer God Class (1561 lines, 60+ identical handlers), Client Entity (2900 lines), 70+ identical setter+emit patterns - 18 Major: RigidBody/Collider ~1750 lines each, GLTFManager 1812 lines, tickWithPlayerInput 225 lines, monkey-patching applyImpulse, event listener leaks, fatalError for validation, 60+ client TODOs - 14 Minor: as any casts, console.log leftovers, hardcoded URIs, empty catches - 14 concrete standards for new code to prevent these smells Co-Authored-By: Claude Opus 4.6 --- analysis-output/code-smells.md | 795 +++++++++++++++++++++++++++++++++ 1 file changed, 795 insertions(+) create mode 100644 analysis-output/code-smells.md diff --git a/analysis-output/code-smells.md b/analysis-output/code-smells.md new file mode 100644 index 00000000..08f02fa4 --- /dev/null +++ b/analysis-output/code-smells.md @@ -0,0 +1,795 @@ +# Hytopia Codebase -- Code Smells Analysis + +**Analyzed files:** 40+ files across `server/src/`, `client/src/`, and `protocol/` +**Date:** 2026-03-08 + +--- + +## Table of Contents + +1. [God Classes](#1-god-classes) +2. [Long Methods / Excessive Repetition](#2-long-methods--excessive-repetition) +3. [Feature Envy](#3-feature-envy) +4. [Primitive Obsession / Data Clumps](#4-primitive-obsession--data-clumps) +5. [Type Safety Issues](#5-type-safety-issues) +6. [Magic Numbers and Strings](#6-magic-numbers-and-strings) +7. [DRY Violations](#7-dry-violations) +8. [Memory Leak Risks](#8-memory-leak-risks) +9. [Error Handling Issues](#9-error-handling-issues) +10. [Console / Debug Leftovers](#10-console--debug-leftovers) +11. [Dead Code / TODOs](#11-dead-code--todos) +12. [Coupling Issues](#12-coupling-issues) +13. [Mutable State Issues](#13-mutable-state-issues) +14. [Inconsistencies](#14-inconsistencies) +15. [Promise Anti-patterns](#15-promise-anti-patterns) + +--- + +## 1. God Classes + +### CRITICAL: `NetworkSynchronizer` -- 1561 lines + +**File:** `server/src/networking/NetworkSynchronizer.ts` +**Responsibilities:** Event subscription, state queuing, sync serialization, packet batching, per-player routing, singleton/collection sync abstraction, entity model sync, camera sync. + +This is the most extreme God Class in the codebase. It has: +- **60+ private event handler methods** (lines ~400-1305), each following the exact same pattern: create-or-get a sync object, set a single property on it +- ~15 `_createOrGetQueued*Sync` methods (lines ~1320-1447) +- 2 generic sync collection methods +- 2 sync output collection methods +- A `synchronize()` method that orchestrates everything + +**Suggested improvement:** Extract domain-specific synchronizers (AudioSyncHandler, EntitySyncHandler, CameraSyncHandler, ParticleEmitterSyncHandler, etc.) that each handle their own event subscriptions and sync queuing. The NetworkSynchronizer becomes an orchestrator that registers handlers and calls `synchronize()`. + +**Standard for:** EXISTING codebase should refactor. NEW code must not add to this file. + +--- + +### CRITICAL: Client `Entity` -- 2900 lines + +**File:** `client/src/entities/Entity.ts` +**Responsibilities:** 3D model management, animation playback, interpolation, block entity rendering, material/texture management, frustum culling, bounding box computation, light level tracking, parent-child attachment, node overrides, custom textures, emissive color management. + +This is the largest file in the entire codebase. It has: +- 50+ private fields (lines 175-265) +- Multiple concerns mixed: rendering, animation, spatial queries, material management + +**Suggested improvement:** Extract `EntityAnimationManager`, `EntityMaterialManager`, `EntityCullingManager`, `EntityInterpolator` as separate classes. The Entity becomes a coordinator. + +**Standard for:** EXISTING codebase should refactor. NEW code must not add rendering logic here. + +--- + +### MAJOR: `RigidBody` and `Collider` -- 1760 and 1750 lines each + +**Files:** `server/src/worlds/physics/RigidBody.ts`, `server/src/worlds/physics/Collider.ts` + +Both classes are very large wrappers around RAPIER physics. They expose dozens of getters/setters that largely proxy to the underlying physics engine. + +**Suggested improvement:** These are somewhat acceptable as facade wrappers, but the setter-per-property pattern with event emission could be generated or handled via a generic property-change observer pattern to reduce boilerplate. + +**Standard for:** Acceptable for EXISTING code. NEW physics features should extract sub-concerns (e.g., `ColliderShapeFactory`). + +--- + +### MAJOR: `GLTFManager` -- 1812 lines + +**File:** `client/src/gltf/GLTFManager.ts` + +Handles model loading, caching, texture compression, mesh optimization, instancing, material pipeline, and resource cleanup -- all in one file. + +**Standard for:** EXISTING codebase should consider splitting. NEW code should not add to this file. + +--- + +## 2. Long Methods / Excessive Repetition + +### CRITICAL: NetworkSynchronizer event handlers -- 60+ identical-pattern methods + +**File:** `server/src/networking/NetworkSynchronizer.ts:400-977` + +```typescript +// This exact pattern repeats 60+ times: +private _onParticleEmitterSetColorEnd = (payload: EventPayloads[ParticleEmitterEvent.SET_COLOR_END]) => { + const particleEmitterSync = this._createOrGetQueuedParticleEmitterSync(payload.particleEmitter); + particleEmitterSync.ce = payload.colorEnd ? Serializer.serializeRgbColor(payload.colorEnd) : undefined; +}; +``` + +Each handler: (1) gets a sync object, (2) assigns one property. This is the textbook case for a data-driven approach (a mapping table from event type to sync property + optional serializer). + +**Suggested improvement:** Replace with a registration table: +```typescript +// Instead of 60 handler methods: +const particleEmitterMappings = [ + { event: ParticleEmitterEvent.SET_COLOR_END, key: 'ce', field: 'colorEnd', serializer: Serializer.serializeRgbColor }, + // ... etc +]; +``` + +**Standard for:** EXISTING codebase should refactor. NEW code must NOT add more handler methods to this file. + +--- + +### MAJOR: `DefaultPlayerEntityController.tickWithPlayerInput` -- ~225 lines + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:565-789` + +This single method handles: +- Input parsing +- Swimming state +- Movement animations (3 branches, each with animation loops) +- Movement rotation calculation (joystick + WASD) +- Interaction handling +- Horizontal velocity calculation +- Swimming physics +- Jumping logic +- Platform velocity +- External impulse decay +- Final impulse application +- Character rotation + +**Suggested improvement:** Decompose into `_handleAnimations()`, `_calculateMovementVelocity()`, `_handleSwimmingPhysics()`, `_handleJumping()`, `_applyPhysicsImpulses()`, `_applyRotation()`. + +**Standard for:** Both EXISTING and NEW code. Methods should be under ~40 lines. + +--- + +### MAJOR: Animation management repetition in DefaultPlayerEntityController + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:597-630` + +The same pattern repeats 3 times for ground/swim/idle animations: +```typescript +entity.stopAllModelAnimations(animation => animations.includes(animation.name) || animation.loopMode === EntityModelAnimationLoopMode.ONCE); +for (const animation of animations) { + entity.getModelAnimation(animation)?.setLoopMode(EntityModelAnimationLoopMode.LOOP); + entity.getModelAnimation(animation)?.play(); +} +``` + +**Suggested improvement:** Extract a `_playAnimationSet(entity, animations)` helper. + +**Standard for:** Both EXISTING and NEW code. + +--- + +### MAJOR: Setter method boilerplate in Entity, PlayerCamera, ParticleEmitter + +**Files:** Multiple server files + +Every property setter follows an identical pattern: +```typescript +public setSomething(value: T) { + if (this._something === value) return; + this._something = value; + if (this.isSpawned) { + this.emitWithWorld(this._world!, EventType.SET_SOMETHING, { entity: this, something: value }); + } +} +``` + +This pattern repeats 20+ times in `Entity.ts`, 15+ times in `PlayerCamera.ts`, 30+ times in `ParticleEmitter.ts`. + +**Suggested improvement:** Consider a generic `_setAndEmit()` helper or decorator-based approach. + +**Standard for:** Acceptable pattern for EXISTING code but NEW code should consider reducing boilerplate. + +--- + +## 3. Feature Envy + +### MAJOR: NetworkSynchronizer reaching into every domain object + +**File:** `server/src/networking/NetworkSynchronizer.ts` + +This class imports and directly accesses internals of: Audio, BlockType, Chunk, Entity, EntityModelAnimation, EntityModelNodeOverride, ParticleEmitter, Player, PlayerCamera, PlayerUI, SceneUI, World, Simulation. It subscribes to events from all of them. + +The synchronizer knows more about each domain object's internal structure than the objects themselves -- it accesses `.id`, `.serialize()`, `.position`, `.isSpawned`, etc. + +**Standard for:** The event-based pattern is intentional and performance-critical. NEW code should use the `serialize()` pattern consistently rather than having the synchronizer know individual fields. + +--- + +### MINOR: Serializer accessing many domain object internals + +**File:** `server/src/networking/Serializer.ts` + +The Serializer directly accesses dozens of properties from Audio, Entity, ParticleEmitter, PlayerCamera, World, etc. This is somewhat expected for a serializer, but it creates tight coupling. + +**Standard for:** Acceptable for EXISTING code. Each domain object already has a `serialize()` method that delegates to `Serializer` -- this is a reasonable pattern. + +--- + +## 4. Primitive Obsession / Data Clumps + +### MAJOR: `RgbColor` used as raw `{ r, g, b }` everywhere + +**Files:** Throughout server and client + +`RgbColor` is a type alias for `{ r: number, g: number, b: number }` and is compared manually: +```typescript +// Entity.ts:973-978 +if ((!emissiveColor && !this._emissiveColor) || (emissiveColor && this._emissiveColor && + emissiveColor.r === this._emissiveColor.r && + emissiveColor.g === this._emissiveColor.g && + emissiveColor.b === this._emissiveColor.b)) { + return; +} +``` + +This equality check pattern repeats for `setEmissiveColor` and `setTintColor` in the same file (lines 973-978 and 1273-1277). + +**Suggested improvement:** Create an `RgbColor.equals(a, b)` utility or make `RgbColor` a proper value object. + +**Standard for:** NEW code must use a comparison utility rather than inline field-by-field comparison. + +--- + +### MAJOR: Coordinate string keys `"x,y,z"` for Map lookups + +**Files:** `server/src/networking/NetworkSynchronizer.ts:1329`, `server/src/worlds/World.ts:538-564` + +```typescript +// NetworkSynchronizer.ts:1329 +const id = `${globalCoordinate.x},${globalCoordinate.y},${globalCoordinate.z}`; +``` + +String keys from coordinates are used for map lookups, creating GC pressure from string allocations. + +**Suggested improvement:** Use a packed numeric key (e.g., `ChunkLattice` already uses `bigint` packed keys). Apply consistently. + +**Standard for:** NEW code should use numeric/packed keys for hot-path map lookups. + +--- + +### MINOR: Vector3Like `{ x, y, z }` inline construction + +Inline `{ x: 0, y: 0, z: 0 }` appears dozens of times. This is fine for a data-oriented architecture but watch for excessive object allocation in hot paths. + +**Standard for:** Acceptable pattern. The `DefaultPlayerEntityController` already uses reusable vectors (`_reusableImpulse`, `_reusableTargetVelocities`), which is the correct approach for hot paths. + +--- + +## 5. Type Safety Issues + +### MAJOR: `type?: any` in WorldMap entity options + +**File:** `server/src/worlds/World.ts:51` + +```typescript +rigidBodyOptions?: Omit, 'type'> & { type?: any } +``` + +An explicit `any` cast to work around JSON map imports where `type` is a string, not a `RigidBodyType` enum. The code even has a comment acknowledging it. + +**Suggested improvement:** Create a `WorldMapRigidBodyType` that accepts `string | RigidBodyType` with validation at parse time. + +**Standard for:** NEW code must not introduce `any` in public/exported types. + +--- + +### MAJOR: EventRouter method overloads with `any` payload + +**File:** `server/src/events/EventRouter.ts:45-46, 72-73, 94-95, 118-119` + +```typescript +public emit(eventType: string, payload: any): boolean; +``` + +All EventRouter methods have a `string, any` overload alongside the typed overload. This allows emitting untyped events that bypass the type system. + +**Suggested improvement:** The dual overload pattern is intentional for extensibility, but could be tightened with `unknown` instead of `any` on the fallback overload. + +**Standard for:** Acceptable architectural decision. NEW code should prefer the typed overload. + +--- + +### MINOR: `as any` in client code + +**File:** `client/src/particles/ParticleEmitterCore.ts:403, 418` + +```typescript +(this._options as any)[key] = options[key as keyof ParticleEmitterCoreOptions]; +``` + +**Standard for:** NEW code should use properly typed index access or a mapping approach. + +--- + +### MINOR: Accessing internal Three.js properties + +**File:** `client/src/entities/Entity.ts:72-78` + +```typescript +interface AnimationActionEx extends AnimationAction { + _propertyBindings: PropertyMixer[]; +} +``` + +Accessing private/undocumented Three.js internals. The code has a TODO acknowledging the risk. + +**Standard for:** Avoid in NEW code. File a Three.js issue or find a public API workaround. + +--- + +## 6. Magic Numbers and Strings + +### MAJOR: Animation name strings hardcoded across the codebase + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:199-248` + +```typescript +public idleLoopedAnimations: string[] = [ 'idle-upper', 'idle-lower' ]; +public runLoopedAnimations: string[] = [ 'run-upper', 'run-lower' ]; +public walkLoopedAnimations: string[] = [ 'walk-upper', 'walk-lower' ]; +public swimLoopedAnimations: string[] = [ 'swim-forward' ]; +public jumpOneshotAnimations: string[] = [ 'jump-loop' ]; +``` + +While these are configurable defaults, the animation name strings are scattered as defaults. If the standard player model changes its animation names, multiple locations must update. + +**Standard for:** Acceptable since they are configurable overrides. NEW controllers should follow the same pattern. + +--- + +### MINOR: Physics constants as class statics + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:135-158` + +These are well-structured as `private static readonly` constants. Good pattern to follow. + +--- + +### MINOR: Step audio URI hardcoded + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:414-421` + +```typescript +this._stepAudio = new Audio({ + uri: 'audio/sfx/step/stone/stone-step-04.mp3', + ... +}); +``` + +The step audio URI is hardcoded and not configurable via constructor options. + +**Standard for:** NEW code should make asset URIs configurable. + +--- + +## 7. DRY Violations + +### CRITICAL: Identical setter+emit pattern repeated 70+ times + +**Files:** `Entity.ts`, `PlayerCamera.ts`, `ParticleEmitter.ts`, `World.ts` + +Every property setter across these files follows the exact same template: +1. Check if value changed +2. Update internal field +3. If spawned/loaded, emit event with `emitWithWorld()` + +**ParticleEmitter.ts** is the worst offender with 30+ nearly identical setter methods across 1323 lines. + +**Suggested improvement:** A generic `_setProperty(field, eventType, payload)` or decorator-based property change notification. + +**Standard for:** NEW code should reduce boilerplate with a helper if adding many property setters. + +--- + +### MAJOR: Server Entity + Client Entity duplication + +**Files:** `server/src/worlds/entities/Entity.ts` and `client/src/entities/Entity.ts` + +Both define entity concepts with overlapping field names, options, and behaviors. While server and client have different concerns, the data shapes (EntityData, EntityOptions) are nearly identical, leading to drift risk. + +**Standard for:** This is a fundamental client-server architecture choice. The `protocol/` layer bridges them. NEW code should ensure protocol schemas remain the single source of truth. + +--- + +### MAJOR: Color comparison logic duplicated + +**File:** `server/src/worlds/entities/Entity.ts:973-978` and `Entity.ts:1273-1277` + +The exact same RGB color equality check is copied for `setEmissiveColor` and `setTintColor`: +```typescript +if ((!color && !this._color) || (color && this._color && + color.r === this._color.r && + color.g === this._color.g && + color.b === this._color.b)) { + return; +} +``` + +**Suggested improvement:** Extract `rgbColorEquals(a, b)` utility. + +**Standard for:** NEW code must use a shared utility for value equality checks. + +--- + +## 8. Memory Leak Risks + +### MAJOR: Event listeners not cleaned up on some code paths + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:427` + +The `entity.on(EntityEvent.BLOCK_COLLISION, ...)` listener is added in `attach()` but there's no corresponding `off()` in `detach()`. The `detach()` method is inherited from `BaseEntityController` and may not clean up the lambda. + +**Suggested improvement:** Store the listener reference and remove it in `detach()`. + +**Standard for:** Both EXISTING and NEW code. Event listeners must be paired with cleanup. + +--- + +### MAJOR: `Connection._cachedPacketsSerializedBuffer` as static Map + +**File:** `server/src/networking/Connection.ts:65` + +```typescript +private static _cachedPacketsSerializedBuffer: Map = new Map(); +``` + +This uses `AnyPacket[]` (array references) as map keys. Since new arrays are created each tick, old entries should be cleaned up. The code does call `clearCachedPacketsSerializedBuffers()` (line 134-138), but if any code path skips this call, the map grows unboundedly. + +**Standard for:** Acceptable since cleanup is called. NEW code using caches must ensure cleanup paths exist. + +--- + +### MINOR: `_animationFadeTimeouts` Map in client Entity + +**File:** `client/src/entities/Entity.ts:177` + +```typescript +private _animationFadeTimeouts: Map = new Map(); +``` + +Timeout handles stored but need verification that all timeouts are cleared when the entity is removed. + +**Standard for:** NEW code must clear all timers in disposal/cleanup methods. + +--- + +## 9. Error Handling Issues + +### MAJOR: Debug packet handler logs raw packet + +**File:** `server/src/players/Player.ts:511` + +```typescript +private _onDebugConfigPacket = (packet: protocol.DebugConfigPacket) => { + console.log(packet); +}; +``` + +A debug handler that just `console.log`s the raw packet with no processing. In production, `console.log` is disabled (ErrorHandler.ts:13), so this is a no-op. But it's dead code that signals an unfinished feature. + +**Standard for:** NEW code must not include placeholder handlers. Either implement or remove. + +--- + +### MAJOR: Empty catch blocks in Connection + +**File:** `server/src/networking/Connection.ts:274, 490` + +```typescript +} catch { + this._wtBinding = false; + return; +} +// and +setTimeout(() => { try { wtToClose.close(); } catch { /* NOOP */ } }, 50); +``` + +Silent error swallowing during WebTransport binding. While some "best effort close" silencing is acceptable, the first catch discards all binding errors silently. + +**Standard for:** NEW code must at minimum log errors to ErrorHandler, even in catch blocks. + +--- + +### MINOR: `ErrorHandler.fatalError` used for validation in Serializer + +**File:** `server/src/networking/Serializer.ts:49-51, 133, 224, 376` + +```typescript +if (audio.id === undefined) { + ErrorHandler.fatalError(`Serializer.serializeAudio(): Audio ${audio.uri} is not playing!`); +} +``` + +Using `fatalError` (which calls `process.exit`) for what should be defensive validation. If an audio object somehow reaches the serializer without an ID, the entire server crashes. + +**Suggested improvement:** Use `ErrorHandler.error()` for recoverable validation failures and skip the serialization. + +**Standard for:** NEW code should use `fatalError` only for truly unrecoverable states (corrupted memory, impossible invariant violations). + +--- + +## 10. Console / Debug Leftovers + +### MINOR: `console.log` in Player debug handler + +**File:** `server/src/players/Player.ts:511` + +```typescript +console.log(packet); +``` + +**Standard for:** NEW code must not use `console.log` for production code. Use `console.info` or `ErrorHandler`. + +--- + +### MINOR: `console.log` in AssetsLibrary + +**File:** `server/src/assets/AssetsLibrary.ts:98, 102` + +```typescript +console.log(`AssetsLibrary.syncAsset(): Copied model from asset library...`); +``` + +Development-only logging that persists. Acceptable since it's gated by sync operations. + +**Standard for:** NEW code should use `console.info` instead of `console.log` (since log is disabled in production). + +--- + +### MINOR: PathfindingEntityController debug logging + +**File:** `server/src/worlds/entities/controllers/PathfindingEntityController.ts:432` + +```typescript +console.log(`PathfindingEntityController._calculatePath: Path found after ${openSetIterations}...`); +``` + +Debug-level logging left in production code. + +**Standard for:** NEW code should use a debug flag or remove before merging. + +--- + +## 11. Dead Code / TODOs + +### MAJOR: 60+ TODOs across the client codebase + +**Files:** Primarily `client/src/entities/Entity.ts`, `client/src/gltf/GLTFManager.ts`, `client/src/particles/`, `client/src/workers/` + +The client has a very high density of `TODO` comments indicating known issues: +- `client/src/entities/Entity.ts`: 16 TODOs (error handling, optimization, design concerns) +- `client/src/gltf/GLTFManager.ts`: 14 TODOs (error handling, naming, design) +- `client/src/particles/`: 7 TODOs +- `client/src/workers/`: 6 TODOs +- `client/src/textures/`: 4 TODOs (error handling) + +Many of these are about **missing error handling** (e.g., "TODO: Better error handling?", "TODO: Proper error handling"). + +**Standard for:** NEW code must not have TODOs for error handling. Handle errors before merging. + +--- + +### MINOR: Two TODOs in server code + +**Files:** `server/src/players/Player.ts:488`, `server/src/worlds/physics/Simulation.ts:138` + +Only 2 TODOs in the entire server codebase -- significantly cleaner than the client side. + +**Standard for:** Server code sets a good standard. NEW code should aim for zero TODOs at merge time. + +--- + +## 12. Coupling Issues + +### MAJOR: NetworkSynchronizer imports 15+ domain classes + +**File:** `server/src/networking/NetworkSynchronizer.ts:1-30` + +The synchronizer imports types and event enums from every major domain: Audio, BlockType, ChatManager, ChunkLattice, Entity, EntityModelAnimation, EntityModelNodeOverride, ParticleEmitter, Player, PlayerCamera, PlayerUI, SceneUI, Simulation, World. + +This creates a "hub" that must change whenever any domain object changes. + +**Standard for:** Inevitable for a synchronizer. NEW domain objects should follow the existing pattern but consider plugin-style registration. + +--- + +### MAJOR: Monkey-patching in DefaultPlayerEntityController + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:405-412` + +```typescript +this._internalApplyImpulse = entity.applyImpulse.bind(entity); +entity.applyImpulse = (impulse: Vector3Like) => { + const mass = entity.mass || 1; + this._externalVelocity.x += impulse.x / mass; + // ... +}; +``` + +The controller replaces `entity.applyImpulse` with a wrapped version. This is fragile: +- Other code calling `applyImpulse` gets the wrapped version silently +- Multiple controllers on the same entity would chain unpredictably +- No cleanup in `detach()` + +**Suggested improvement:** Use a composition pattern (e.g., `entity.addImpulseInterceptor()`) instead of monkey-patching. + +**Standard for:** NEW code must never monkey-patch methods on other objects. + +--- + +## 13. Mutable State Issues + +### MAJOR: Public mutable arrays for animation names + +**File:** `server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:199-248` + +```typescript +public idleLoopedAnimations: string[] = [ 'idle-upper', 'idle-lower' ]; +public runLoopedAnimations: string[] = [ 'run-upper', 'run-lower' ]; +``` + +All animation name arrays are `public` and mutable. External code can push to, splice from, or replace these arrays at any time, potentially causing subtle bugs. + +**Suggested improvement:** Make these `readonly` or provide setter methods. + +**Standard for:** NEW code should use `readonly` arrays for configuration data. + +--- + +### MINOR: `_input` mutated via `Object.assign` + +**File:** `server/src/players/Player.ts:527` + +```typescript +Object.assign(this._input, input); +``` + +Merges untrusted client input directly into the input object. No validation or sanitization of the incoming input packet. + +**Standard for:** NEW code should validate input before assignment. + +--- + +## 14. Inconsistencies + +### MAJOR: `isSpawned` uses `!!this._world` but still requires `!` assertions + +**File:** `server/src/worlds/entities/Entity.ts:733` + +```typescript +public get isSpawned(): boolean { return !!this._world; } +// But then everywhere: +this._world!.entityManager... +this.emitWithWorld(this._world!, ...); +``` + +The code even has a comment: `// this should prob be changed to type predicate so we don't have to assert (!) in areas.` + +Using `isSpawned` as a guard doesn't narrow the type of `_world`. Every subsequent access requires `!`. + +**Suggested improvement:** Make `isSpawned` a type predicate: `public get isSpawned(): this is Entity & { _world: World }` (would require refactoring private field access), or use a `_requireSpawned()` pattern that returns the world. + +**Standard for:** NEW code should use type predicates or assertion functions instead of `!` assertions after guards. + +--- + +### MINOR: Inconsistent event emission patterns + +Server `Entity` uses `emitWithWorld(this._world!, ...)` for most events but `emit(...)` for `TICK`. +`World` uses `emit(...)` directly. `Player` uses `emitWithWorld(...)`. +`GameServer` uses `EventRouter.globalInstance.emit(...)`. + +The scoping is intentional (local vs world vs global), but the lack of documentation about when to use which pattern creates confusion. + +**Standard for:** NEW code must document which scope is intended. Use `emitWithWorld` for events that the world's NetworkSynchronizer needs to observe. + +--- + +### MINOR: Naming inconsistency for packed data keys + +Protocol schemas use cryptic single-letter keys (`i`, `m`, `p`, `e`, `n`, `r`, etc.) across all schemas. This is intentional for bandwidth optimization but creates readability challenges when debugging. + +**Standard for:** Acceptable. NEW protocol fields should follow the existing abbreviated key convention. + +--- + +## 15. Promise Anti-patterns + +### MINOR: `.catch(() => {})` silencing in Connection + +**File:** `server/src/networking/Connection.ts:250` + +```typescript +wt.closed.catch(() => { /* NOOP */}).finally(() => wt.userData.onclose?.()); +``` + +Silently catches all WebTransport close errors. + +**Standard for:** Acceptable for transport close cleanup. NEW code should log at debug level. + +--- + +### MINOR: `startServer` chaining without structured error handling + +**File:** `server/src/GameServer.ts:67-89` + +```typescript +RAPIER.init().then(() => { + return GameServer.instance.blockTextureRegistry.preloadAtlas(); +}).then(() => { + return GameServer.instance.modelRegistry.preloadModels(); +}).then(() => { ... }).catch(error => { + ErrorHandler.fatalError(...); +}); +``` + +The `.then()` chain is acceptable but modern `async/await` would be clearer and allow per-step error handling. + +**Standard for:** NEW code should prefer `async/await` over `.then()` chains. + +--- + +## Summary by Severity + +### Critical (4 findings) +| # | Finding | Location | Action | +|---|---------|----------|--------| +| 1 | God Class: NetworkSynchronizer (1561 lines, 60+ handlers) | `server/src/networking/NetworkSynchronizer.ts` | Refactor to domain-specific sync handlers | +| 2 | God Class: Client Entity (2900 lines) | `client/src/entities/Entity.ts` | Extract animation, material, culling managers | +| 3 | 60+ identical event handler methods | `NetworkSynchronizer.ts:400-977` | Replace with data-driven mapping table | +| 4 | 70+ identical setter+emit patterns | Entity, PlayerCamera, ParticleEmitter | Create generic setter helper | + +### Major (18 findings) +| # | Finding | Location | +|---|---------|----------| +| 1 | RigidBody (1760 lines) / Collider (1750 lines) | `server/src/worlds/physics/` | +| 2 | GLTFManager (1812 lines) | `client/src/gltf/GLTFManager.ts` | +| 3 | DefaultPlayerEntityController.tickWithPlayerInput (225 lines) | Server controllers | +| 4 | Animation management code duplicated 3x | DefaultPlayerEntityController | +| 5 | `type?: any` in WorldMap | `server/src/worlds/World.ts:51` | +| 6 | EventRouter `any` payload overloads | `server/src/events/EventRouter.ts` | +| 7 | Inline RgbColor equality checks duplicated | `Entity.ts:973, 1273` | +| 8 | String coordinate keys for map lookups | NetworkSynchronizer, World | +| 9 | Server/Client Entity data shape duplication | Cross-package | +| 10 | Event listeners not cleaned up in DefaultPlayerEntityController | `attach()` without matching `detach()` cleanup | +| 11 | Debug packet handler is dead code | `Player.ts:511` | +| 12 | Empty catch blocks in Connection | `Connection.ts:274, 490` | +| 13 | `fatalError` used for validation in Serializer | `Serializer.ts` | +| 14 | 60+ TODOs about missing error handling (client) | `client/src/` | +| 15 | NetworkSynchronizer coupled to 15+ domain classes | Imports | +| 16 | Monkey-patching `applyImpulse` | DefaultPlayerEntityController | +| 17 | Public mutable animation arrays | DefaultPlayerEntityController | +| 18 | `isSpawned` doesn't narrow types, causes `!` assertions | `Entity.ts:733` | + +### Minor (14 findings) +| # | Finding | Location | +|---|---------|----------| +| 1 | `as any` in client code | ParticleEmitterCore, ChunkWorker | +| 2 | Accessing private Three.js internals | Client Entity | +| 3 | Hardcoded step audio URI | DefaultPlayerEntityController | +| 4 | `_animationFadeTimeouts` cleanup | Client Entity | +| 5 | `console.log` in Player debug handler | Player.ts:511 | +| 6 | `console.log` in AssetsLibrary | AssetsLibrary.ts | +| 7 | PathfindingEntityController debug logging | PathfindingEntityController | +| 8 | 2 TODOs in server code | Player.ts, Simulation.ts | +| 9 | `Object.assign` for input without validation | Player.ts:527 | +| 10 | Inconsistent event emission scope patterns | Multiple files | +| 11 | Cryptic single-letter protocol keys | Protocol schemas | +| 12 | `.catch(() => {})` silencing | Connection.ts | +| 13 | `.then()` chain instead of async/await | GameServer.ts | +| 14 | Inline vector `{ x: 0, y: 0, z: 0 }` construction | Multiple files | + +--- + +## Standards for New Code (Summary) + +1. **No file over 800 lines.** If a class exceeds this, split responsibilities. +2. **No method over 40 lines.** Decompose into well-named private helpers. +3. **No `any` in exported types.** Use `unknown` if truly needed; prefer proper types. +4. **No monkey-patching.** Use composition, interceptors, or event hooks. +5. **No inline value equality checks.** Create and use utility functions. +6. **No data-driven boilerplate.** If 5+ methods follow the same pattern, use a mapping table. +7. **No TODO for error handling.** Handle errors before merging. +8. **No `console.log` in production.** Use `console.info` or `ErrorHandler`. +9. **Pair every event listener with cleanup.** Store references; remove in `detach()`/`dispose()`. +10. **Use type predicates or assertion functions** instead of `!` after boolean guards. +11. **Use `async/await`** instead of `.then()` chains. +12. **Make configuration arrays `readonly`.** +13. **Validate external input** before merging into internal state. +14. **Use `fatalError` only for unrecoverable invariant violations**, not for validation. From a2de034e61bd80711a5937119ba0750fa0c2bda7 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:58:26 +1100 Subject: [PATCH 07/19] docs: add SOLID principles and clean code analysis report Overall score: 6.6/10 across 13 principles: - Strongest: LSP (8), KISS (8), YAGNI (8), Composition over Inheritance (8) - Weakest: DIP (4/10) - heavy singleton usage, no DI, no abstractions - Coupling (5/10) - fully connected singleton dependency graph - Law of Demeter (5/10) - deep chains through managers - SRP (6/10) - Entity ~1200 lines mixing physics/visuals/animations - Top recommendations: introduce interfaces + constructor injection, decompose Entity, break up long methods Co-Authored-By: Claude Opus 4.6 --- analysis-output/principles-analysis.md | 592 +++++++++++++++++++++++++ 1 file changed, 592 insertions(+) create mode 100644 analysis-output/principles-analysis.md diff --git a/analysis-output/principles-analysis.md b/analysis-output/principles-analysis.md new file mode 100644 index 00000000..819afa62 --- /dev/null +++ b/analysis-output/principles-analysis.md @@ -0,0 +1,592 @@ +# Hytopia Game Engine: Software Engineering Principles Analysis + +## Overall Scorecard + +| Principle | Score (1-10) | Verdict | +|-----------|:---:|---------| +| Single Responsibility (SRP) | 6 | Mixed -- managers are focused, but some classes (Entity, NetworkSynchronizer) are overloaded | +| Open/Closed (OCP) | 7 | Good event/controller extension points; some areas require source modification | +| Liskov Substitution (LSP) | 8 | Clean inheritance hierarchies with proper specialization | +| Interface Segregation (ISP) | 7 | Options interfaces are well-scoped; some fat config objects exist | +| Dependency Inversion (DIP) | 4 | Heavy use of concrete singletons; no DI container; limited abstractions | +| DRY | 6 | Some duplication in serialization, event patterns, and manager boilerplate | +| KISS | 8 | Pragmatic game engine design; complexity is justified where it exists | +| YAGNI | 8 | Minimal unused abstractions; code is feature-driven | +| Law of Demeter | 5 | Frequent deep property chains through managers and world accessors | +| Composition vs Inheritance | 8 | Strong preference for composition; inheritance used appropriately | +| Separation of Concerns | 7 | Clear server/client/protocol boundaries; some bleed in NetworkSynchronizer | +| Cohesion | 7 | Modules are generally focused; World is a facade with appropriate delegation | +| Coupling | 5 | Tight coupling through singletons and direct references; event system helps | + +**Overall Score: 6.6 / 10** + +--- + +## 1. Single Responsibility Principle (SRP) + +**Score: 6/10** + +### Good Adherence + +**WorldLoop** (`server/src/worlds/WorldLoop.ts:71`) -- Focused exclusively on tick scheduling and lifecycle events. Delegates physics to Simulation, entity ticking to EntityManager, and networking to NetworkSynchronizer. Clean single responsibility. + +**EntityManager** (`server/src/worlds/entities/EntityManager.ts:26`) -- Manages entity registration, querying, and lifecycle. Does not mix in physics, rendering, or serialization concerns. Each query method (getEntitiesByTag, getPlayerEntitiesByPlayer, etc.) is well-scoped. + +**BaseEntityController** (`server/src/worlds/entities/controllers/BaseEntityController.ts:76`) -- Abstract base with clear lifecycle hooks (attach, spawn, tick, despawn, detach). Each method has a single purpose. Extension is the expected usage pattern. + +**Serializer** (`server/src/networking/Serializer.ts:38`) -- Pure static class focused solely on converting domain objects to protocol schemas. No side effects, no state. Each method handles exactly one domain type. + +**PlayerCamera** (`server/src/players/PlayerCamera.ts`) -- Focused on camera configuration and orientation state for a single player. + +### Violations + +**Entity** (`server/src/worlds/entities/Entity.ts`) -- ~1,200+ lines. This class handles: +- Physics body management (rigid body creation, collider management, impulse application) +- Model and animation state management +- Parent-child entity relationships +- Serialization coordination +- Event emission for collisions, spawning, despawning +- Visual property management (opacity, tint, emissive color) + +This is the most significant SRP violation in the codebase. Entity serves as both a domain model and a physics proxy, making it the primary "God Class" candidate. + +**NetworkSynchronizer** (`server/src/networking/NetworkSynchronizer.ts:67`) -- Subscribes to ~14 different event types across the entire domain model. It knows about audios, blocks, block types, chunks, entities, animations, particles, players, cameras, UIs, scene UIs, physics debug rendering, and world state. This is a cross-cutting concern by nature, but the sheer scope means any change to any domain object's events requires touching this file. + +**DefaultPlayerEntityController** (`server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:133`) -- ~790 lines mixing movement physics, animation management, platform detection, swimming mechanics, audio management, and collision handling. The `tickWithPlayerInput` method at line 565 is a ~225-line method handling movement calculation, animation state machines, physics impulse computation, and rotation updates all in one function. + +**Client Game** (`client/src/Game.ts:29`) -- Instantiates and holds references to 22 different managers. While it acts as a composition root (which is its purpose), it creates all dependencies directly with `new` rather than through a factory or DI pattern. + +### Recommendations + +1. Extract physics-related behavior from `Entity` into a separate `EntityPhysicsProxy` or delegate to `RigidBody`/`Collider` wrappers. +2. Break `DefaultPlayerEntityController.tickWithPlayerInput` into smaller methods: `_calculateMovementVelocity()`, `_applyAnimations()`, `_applyRotation()`, `_resolvePhysicsImpulse()`. +3. Consider splitting `NetworkSynchronizer` into per-domain synchronizers (EntityNetSync, AudioNetSync, etc.) composed by a coordinator. + +--- + +## 2. Open/Closed Principle (OCP) + +**Score: 7/10** + +### Good Adherence + +**EventRouter system** (`server/src/events/EventRouter.ts:20`) -- Excellent extension point. Every significant class extends `EventRouter`, allowing external behavior to be added via event listeners without modifying source: +``` +World extends EventRouter +Entity extends EventRouter (via emitWithWorld) +BaseEntityController extends EventRouter +Simulation extends EventRouter +BlockType extends EventRouter +``` + +**Controller hierarchy** (`server/src/worlds/entities/controllers/`) -- `BaseEntityController` defines lifecycle hooks (`attach`, `spawn`, `tick`, `tickWithPlayerInput`, `despawn`, `detach`) that subclasses override. New movement/AI behaviors can be added without touching existing code: +- `DefaultPlayerEntityController` -- player movement +- `SimpleEntityController` -- basic entity movement +- `PathfindingEntityController` -- AI navigation + +**WorldManager.worldSelectionHandler** (`server/src/players/PlayerManager.ts:88`) -- Hook-based extension for world routing: +```typescript +public worldSelectionHandler?: (player: Player) => Promise; +``` +This allows custom lobby/routing logic without modifying PlayerManager. + +**Entity callbacks** (`server/src/worlds/entities/Entity.ts`) -- `onTick`, `onInteract`, `onBlockCollision`, `onEntityCollision` callbacks allow behavior injection without subclassing. + +### Violations + +**Serializer** (`server/src/networking/Serializer.ts:38`) -- Adding a new serializable type requires adding a new static method and modifying the switch/dispatch in NetworkSynchronizer. No plugin or registration mechanism. + +**NetworkManager._onMessage** (`client/src/network/NetworkManager.ts:376`) -- Giant switch statement on PacketId (lines 419-536). Adding a new packet type requires modifying this method. A registry-based packet handler dispatch would be more extensible. + +**Simulation._onCollisionEvent** (`server/src/worlds/physics/Simulation.ts:548`) -- Uses `instanceof` chains to dispatch collision events. Adding a new collision participant type requires modifying this method. + +### Recommendations + +1. Replace the `_onMessage` switch statement with a packet handler registry: `packetHandlers.register(PacketId.ENTITIES, handler)`. +2. Consider a `Serializable` registration pattern where types self-register their serializers. + +--- + +## 3. Liskov Substitution Principle (LSP) + +**Score: 8/10** + +### Good Adherence + +**PlayerEntity extends Entity** (`server/src/worlds/entities/PlayerEntity.ts:62`) -- PlayerEntity properly extends Entity, adding player-specific behavior (nametag, input processing) without breaking Entity's contract. Calls `super.spawn()` and `super.tick()` correctly. Overrides thresholds for more sensitive position/rotation updates, which is behavioral refinement, not contract violation. + +**DefaultPlayerEntityController extends BaseEntityController** (`server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:133`) -- Properly calls `super.attach(entity)`, `super.tickWithPlayerInput(...)`, etc. to emit base class events while adding specialized behavior. + +**World extends EventRouter** (`server/src/worlds/World.ts:217`) -- Also implements `protocol.Serializable`. The EventRouter contract is preserved -- listeners registered on a World receive events exactly as they would on any EventRouter. + +**Client Entity / StaticEntity** -- Both implement the same display contract but StaticEntity is optimized for environmental objects. They are stored in the same `_entities` map and support the same query interface. + +### Minor Concerns + +**Entity type narrowing** -- `EntityManager.getEntity(id: number): T | undefined` uses generic type assertion (`as T | undefined`) which bypasses runtime type checking. The caller must know the concrete type, which works but shifts type safety to the caller. + +**BlockType as EventRouter** -- BlockType extends EventRouter to emit collision events, but it represents a *type definition* (like a class/template) rather than an *instance*. Multiple blocks of the same type share one BlockType object, so listeners receive events for all blocks of that type. This is documented but could surprise consumers expecting per-instance events. + +### Recommendations + +1. Consider narrowing `getEntity` with runtime validation or a type-discriminated union. +2. Document the shared-BlockType event model prominently in the API. + +--- + +## 4. Interface Segregation Principle (ISP) + +**Score: 7/10** + +### Good Adherence + +**Options interfaces are well-decomposed:** +- `BaseEntityOptions` -- common entity config +- `BlockEntityOptions` -- block-specific (extends BaseEntityOptions) +- `ModelEntityOptions` -- model-specific (extends BaseEntityOptions) +- `EntityOptions = BlockEntityOptions | ModelEntityOptions` -- discriminated union + +This pattern ensures consumers only provide what's relevant to their entity type. + +**WorldOptions** (`server/src/worlds/World.ts:69`) -- All optional except `id`, `name`, `skyboxUri`. Consumers configure only what they need. + +**FilterOptions / RaycastOptions** (`server/src/worlds/physics/Simulation.ts:81-151`) -- Physics query filters are composed from small optional fields. Clean ISP. + +**DefaultPlayerEntityControllerOptions** (`server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:23`) -- All 30+ fields are optional, defaulting to sensible values. Consumers override only what they need. + +### Violations + +**Player class** (`server/src/players/Player.ts:108`) -- Exposes both public gameplay API (joinWorld, disconnect, scheduleNotification) and internal networking methods (reconnected, loadInitialPersistedData, serialize). The `@internal` annotation hides them from docs but they're still part of the class surface. A cleaner separation would be a public `Player` interface and an internal `PlayerInternal` class. + +**WorldMap interface** (`server/src/worlds/World.ts:33`) -- The `entities` field uses `Omit & { rigidBodyOptions?: ... & { type?: any } }` with an `any` cast, acknowledged in comments. This weakens type safety at the boundary. + +### Recommendations + +1. Split Player into a public-facing interface and an internal implementation class. +2. Define a proper `WorldMapEntityOptions` type instead of using `any` cast for rigidBodyOptions.type. + +--- + +## 5. Dependency Inversion Principle (DIP) + +**Score: 4/10** + +This is the weakest area of the codebase. + +### Violations + +**Pervasive singleton pattern** -- Nearly every manager/registry is a concrete singleton accessed via `.instance`: +``` +GameServer.instance (server/src/GameServer.ts:140) +WorldManager.instance (server/src/worlds/WorldManager.ts:63) +PlayerManager.instance (server/src/players/PlayerManager.ts:75) +PersistenceManager.instance (server/src/persistence/PersistenceManager.ts:28) +ModelRegistry.instance (server/src/models/ModelRegistry.ts) +BlockTextureRegistry.instance (server/src/textures/BlockTextureRegistry.ts) +Socket.instance (server/src/networking/Socket.ts) +WebServer.instance (server/src/networking/WebServer.ts) +PlatformGateway.instance (server/src/networking/PlatformGateway.ts) +``` + +This means: +- Modules depend on concrete implementations, not abstractions +- No dependency injection; all dependencies are resolved via global state +- Unit testing individual classes in isolation is difficult +- Swapping implementations (e.g., mock persistence) requires modifying the singleton + +**Client-side Game class** (`client/src/Game.ts:59-87`) -- Directly instantiates all 22 managers with `new`. No factory, no DI container, no abstractions: +```typescript +this._networkManager = new NetworkManager(this); +this._entityManager = new EntityManager(this); +this._audioManager = new AudioManager(this); +// ... 19 more +``` + +**Direct RAPIER dependency** (`server/src/worlds/physics/Simulation.ts`) -- The physics simulation directly imports and uses `@dimforge/rapier3d-simd-compat`. There's no physics abstraction layer. Public API types like `RaycastOptions` expose `RAPIER.QueryFilterFlags` and `RAPIER.RigidBody` directly, as acknowledged by the TODO at line 138: `// TODO: Clean this up to hide RAPIER types from the public API.` + +### Partial Adherence + +**Constructor injection via composition** -- Some classes receive dependencies via constructor: +```typescript +// World receives its managers via constructor, but creates them itself +this._audioManager = new AudioManager(this); +this._simulation = new Simulation(this, options.tickRate, options.gravity); +``` + +While World creates its own dependencies, it at least passes `this` (the world) into them, creating a bidirectional reference rather than using globals. + +**EventRouter as indirect coupling** -- The event system provides *some* DIP by decoupling producers from consumers. Subsystems communicate through events rather than direct method calls, especially for cross-cutting concerns like NetworkSynchronizer. + +### Recommendations + +1. Define interfaces for critical services: `IPlayerManager`, `IPersistenceManager`, `ISimulation`. +2. Pass dependencies via constructor injection rather than using `.instance` globals. +3. Wrap RAPIER types behind engine-owned interfaces for the public API. +4. Consider a simple service locator or DI container for the server's root-level services. + +--- + +## 6. DRY (Don't Repeat Yourself) + +**Score: 6/10** + +### Good Adherence + +**Shared types** (`server/src/shared/types/`) -- Vector3Like, QuaternionLike, RgbColor are defined once and imported everywhere. No re-definition of math types. + +**Protocol schemas** (`protocol/schemas/`) -- Single source of truth for network data shapes shared between server serialization and client deserialization. + +**Serializer utility methods** -- `serializeVector`, `serializeQuaternion`, `serializeRgbColor` are reused across all entity/world/particle serialization methods. + +### Violations + +**Animation state management duplication** (`server/src/worlds/entities/controllers/DefaultPlayerEntityController.ts:597-630`) -- The animation play/stop pattern is repeated 6 times with minor variations: +```typescript +// Pattern repeated for walk, run, idle, swim, swimIdle +entity.stopAllModelAnimations(animation => animations.includes(animation.name) || ...); +for (const animation of animations) { + entity.getModelAnimation(animation)?.setLoopMode(EntityModelAnimationLoopMode.LOOP); + entity.getModelAnimation(animation)?.play(); +} +``` +This could be extracted into a `_playAnimationSet(entity, animations)` helper. + +**Event declaration boilerplate** -- Every class follows the same pattern of enum + interface + implementation: +```typescript +// Repeated in ~15+ files +export enum XEvent { ... } +export interface XEventPayloads { ... } +export default class X extends EventRouter { ... } +``` +While this provides consistency (a good thing), the declaration overhead per class is significant and could benefit from a code generator or type helper. + +**Manager query patterns** -- `EntityManager`, `PlayerManager`, and `WorldManager` all implement similar filter-by-property patterns: +```typescript +// EntityManager:160 +public getEntitiesByTag(tag: string): Entity[] { + const entities: Entity[] = []; + this._entities.forEach(entity => { if (entity.tag === tag) entities.push(entity); }); + return entities; +} + +// WorldManager:151 +public getWorldsByTag(tag: string): World[] { + const worlds: World[] = []; + this._worlds.forEach(world => { if (world.tag === tag) worlds.push(world); }); + return worlds; +} +``` + +**Client/Server EventRouter implementations** -- Two completely separate EventRouter implementations: +- Server: `server/src/events/EventRouter.ts` -- Full-featured with `emitWithWorld`, `emitWithGlobal`, `final`, typed overloads +- Client: `client/src/events/EventRouter.ts` -- Minimal (44 lines), no typed events, no scoped emission + +These serve different needs but share the same conceptual interface. The protocol layer could define a shared base. + +**Serialization/Deserialization symmetry** -- The server's `Serializer` and client's `Deserializer` manually mirror each other field-by-field. Adding a new field to an entity requires updating both files plus the protocol schema. + +### Recommendations + +1. Extract the animation play/stop pattern into a reusable helper method. +2. Create a generic `filterByProperty` utility for manager query methods. +3. Consider generating serializer/deserializer code from protocol schemas to avoid manual mirroring. + +--- + +## 7. KISS (Keep It Simple, Stupid) + +**Score: 8/10** + +### Good Adherence + +**Flat class hierarchy** -- The inheritance tree is shallow. Entity -> PlayerEntity is the deepest game-logic chain (2 levels). Controllers use a single abstract base. No deep inheritance hierarchies. + +**Simple data flow** -- The server tick loop (`WorldLoop._tick`, line 149) is straightforward: +1. Tick entities +2. Step physics +3. Check/emit updates +4. Synchronize network + +Each step is a single method call. No complex state machines or middleware chains. + +**Options pattern** -- All configuration uses plain TypeScript interfaces with optional fields and sensible defaults. No builder pattern, no fluent API, no configuration DSL. Simple and effective. + +**Protocol schemas** -- Use compact single-letter keys (`i`, `p`, `r`, `m`) for bandwidth efficiency. Simple arrays for vectors (`[x, y, z]`) and quaternions (`[x, y, z, w]`). No nested wrapping. + +### Minor Complexity + +**DefaultPlayerEntityController.tickWithPlayerInput** -- While the individual pieces are simple, the 225-line method combines many concerns. The complexity is inherent to the problem (player movement is genuinely complex) but the method would benefit from decomposition for readability. + +**NetworkSynchronizer sync queues** -- The dual `broadcast` + `perPlayer` queue pattern with IterationMap nesting adds conceptual complexity, but it's solving a real performance problem (avoiding per-player packet construction for shared state). + +### Recommendations + +1. The codebase is admirably pragmatic. Resist adding abstraction layers unless they solve a concrete problem. + +--- + +## 8. YAGNI (You Aren't Gonna Need It) + +**Score: 8/10** + +### Good Adherence + +**No unused abstractions** -- The codebase has no abstract factories, no strategy registries, no unused hooks. Every abstraction (`BaseEntityController`, `EventRouter`, `CollisionGroupsBuilder`) is actively used. + +**Features match requirements** -- The entity system supports exactly what's needed: model entities, block entities, player entities, environmental entities. No speculative entity types or unused entity categories. + +**Protocol is compact** -- Packet definitions include only what's actively transmitted. No "reserved" fields or placeholder packet types. + +### Minor Concerns + +**Events declared but rarely used** -- Some event types exist in enums but may have few or no external listeners (e.g., `SimulationEvent.STEP_START`, `SimulationEvent.STEP_END`). These are primarily for telemetry/debugging, which is a valid use case. + +**Tag system** -- Both `Entity.tag` and `World.tag` provide generic string-based filtering. This is a simple, useful feature, but `getEntitiesByTagSubstring` (`EntityManager:180`) might be YAGNI -- substring matching on tags suggests the tag system may need structured metadata instead. + +### Recommendations + +1. Monitor usage of `getEntitiesByTagSubstring`. If used primarily as a "starts with" check, consider structured tags or a separate category system. + +--- + +## 9. Law of Demeter + +**Score: 5/10** + +### Violations + +**Deep property chains through World** -- The World class is a facade exposing many sub-managers, which leads to long chains: +```typescript +// WorldLoop._tick (server/src/worlds/WorldLoop.ts:164-186) +this._world.entityManager.tickEntities(tickDeltaMs); +this._world.simulation.step(tickDeltaMs); +this._world.entityManager.checkAndEmitUpdates(); +this._world.networkSynchronizer.shouldSynchronize(); +this._world.networkSynchronizer.synchronize(); +this._world.chunkLattice.chunkCount; +``` + +**Player.interact chain** (`server/src/players/Player.ts:542-561`): +```typescript +const playerEntity = this.world.entityManager.getPlayerEntitiesByPlayer(this)[0]; +const raycastHit = this.world.simulation.raycast(interactOrigin, interactDirection, this._maxInteractDistance, { + filterExcludeRigidBody: playerEntity?.rawRigidBody, +}); +// ... then: +raycastHit.hitEntity.interact(this, raycastHit); +raycastHit.hitBlock.blockType.interact(this, raycastHit); +``` + +**Client EntityManager multi-pass** (`client/src/entities/EntityManager.ts:160-233`): +```typescript +this._game.settingsManager.qualityPerfTradeoff.viewDistance.enabled +this._game.camera.activeCamera.position +this._game.performanceMetricsManager.frameCount +this._game.renderer.viewDistance +``` + +### Partial Adherence + +The **event system** mitigates some Demeter violations by allowing indirect communication. For example, `NetworkSynchronizer` subscribes to events rather than directly calling `entity.getPosition()` each tick. + +### Recommendations + +1. Consider adding convenience methods on World that delegate to sub-managers: `world.tickEntities(delta)` instead of `world.entityManager.tickEntities(delta)`. +2. For the client Game class, consider extracting commonly-needed values into the animation callback payload rather than reaching into nested managers. + +--- + +## 10. Composition vs Inheritance + +**Score: 8/10** + +### Good Adherence -- Composition Preferred + +**Entity-Controller composition** -- Entities delegate behavior to controllers via composition rather than inheritance: +```typescript +// Entity has a controller field, not "MovableEntity extends Entity" +entity.setController(new DefaultPlayerEntityController({ ... })); +``` +This allows swapping controllers at runtime and sharing controllers across entities. + +**World composes its subsystems** (`server/src/worlds/World.ts:299-325`): +```typescript +this._audioManager = new AudioManager(this); +this._blockTypeRegistry = new BlockTypeRegistry(this); +this._chatManager = new ChatManager(this); +this._chunkLattice = new ChunkLattice(this); +this._entityManager = new EntityManager(this); +this._simulation = new Simulation(this, ...); +``` +World is a composition root for world-scoped systems, not an inheritance hierarchy. + +**Client StaticEntityManager as a peer** -- Rather than making StaticEntity a subclass that overrides Entity behavior, the client uses a separate `StaticEntityManager` that manages static entities through a different code path optimized for immutable objects. + +### Appropriate Inheritance + +**PlayerEntity extends Entity** -- Genuine "is-a" relationship. A player entity *is* an entity with additional player-specific behavior. Clean two-level hierarchy. + +**DefaultPlayerEntityController extends BaseEntityController** -- Genuine specialization. The base provides lifecycle hooks and event emission; the subclass adds player movement logic. + +**World, Simulation, WorldLoop all extend EventRouter** -- Mixin-like pattern where EventRouter provides event capability to any class that needs it. + +### Recommendations + +1. The current balance is well-chosen. The engine avoids the common game engine trap of deep inheritance hierarchies. + +--- + +## 11. Separation of Concerns + +**Score: 7/10** + +### Good Adherence + +**Three-layer architecture (server/client/protocol):** +- `protocol/` -- Shared data schemas and packet definitions. No business logic. +- `server/src/` -- Game logic, physics, networking, persistence. No rendering. +- `client/src/` -- Rendering, input, deserialization, visual effects. No game logic. + +This is a textbook separation for a client-server game engine. + +**Server domain boundaries:** +``` +worlds/ -- World state, blocks, entities, physics +players/ -- Player lifecycle, input, camera, UI +networking/ -- Serialization, connections, web server +persistence/ -- Data storage abstraction +events/ -- Event routing infrastructure +``` + +**Client domain boundaries:** +``` +core/ -- Renderer, Camera, DebugRenderer +entities/ -- Entity display, animation +chunks/ -- Block mesh, terrain rendering +network/ -- Connection, deserialization +input/ -- Keyboard, mouse, touch +ui/ -- HUD, modals, scene UI +``` + +### Violations + +**NetworkSynchronizer crosses all boundaries** (`server/src/networking/NetworkSynchronizer.ts`) -- Imports from audios, blocks, chat, chunks, entities, animations, particles, players, cameras, UIs, scene UIs, physics, and worlds. This is inherent to its role but means it's a change bottleneck. + +**Player._onInputPacket** (`server/src/players/Player.ts:515-531`) -- Mixes network packet handling with domain logic (camera orientation, interact trigger). The packet handler should deserialize and delegate, not perform game logic. + +**Entity spans physics and domain** -- The server's `Entity` class directly creates and manages RAPIER rigid bodies and colliders, mixing domain model concerns with physics engine integration. + +### Recommendations + +1. Extract input packet processing from Player into a dedicated InputHandler class. +2. Introduce a PhysicsBody wrapper that hides RAPIER behind an engine-owned interface. + +--- + +## 12. Cohesion Analysis + +**Score: 7/10** + +### High Cohesion + +**Serializer** (`server/src/networking/Serializer.ts`) -- All methods convert domain objects to protocol schemas. Every method relates to the single purpose of network serialization. Very cohesive. + +**ChunkLattice** -- Manages block placement in a 3D grid. All methods relate to block CRUD operations within the chunk coordinate system. + +**PlayerCamera** -- All fields and methods relate to camera configuration for a single player. + +**WorldLoop** -- Start/stop/tick lifecycle management for a world. Highly focused. + +**CollisionGroupsBuilder** -- Builder pattern for constructing collision group bitmasks. Single, focused purpose. + +### Mixed Cohesion + +**World** (`server/src/worlds/World.ts`) -- Acts as a facade exposing ~10 sub-managers plus lighting/fog/skybox configuration. The lighting methods (setAmbientLightColor, setDirectionalLightIntensity, etc.) could be extracted into a `WorldEnvironment` class, but the current design is defensible as a convenience API. + +**Entity** -- Low cohesion due to mixing physics management, visual properties, animation state, parent-child relationships, and event emission. See SRP analysis. + +### Low Cohesion + +**Client Game** (`client/src/Game.ts`) -- 22 manager fields with no behavioral methods beyond `start()`. It's a pure composition root / service locator, which is its intended role, but it has low cohesion by definition. + +--- + +## 13. Coupling Analysis + +**Score: 5/10** + +### Tight Coupling + +**Singleton coupling** -- The most significant coupling issue. Any class that calls `GameServer.instance`, `PlayerManager.instance`, `WorldManager.instance`, etc. is tightly coupled to both the concrete class and the singleton lifecycle: + +Dependency graph for a typical server-side request: +``` +PlayerManager.instance + -> EventRouter.globalInstance + -> WorldManager.instance + -> PersistenceManager.instance + -> PlatformGateway.instance +``` + +Changing any of these classes requires considering all consumers. + +**Entity <-> World circular reference** -- Entity holds a reference to World, and World's EntityManager holds references to entities. This bidirectional coupling is common in game engines but makes it impossible to reason about either in isolation. + +**Client Game <-> All Managers** -- Every client manager receives the Game instance and stores it. All managers can access any other manager through `this._game.someManager`. This creates an N-to-N coupling graph where every manager can depend on every other manager. + +### Loose Coupling + +**Event-driven communication** -- The EventRouter pattern provides loose coupling for cross-cutting concerns. The NetworkSynchronizer listens to events rather than being called directly by entities/blocks/players. This means domain objects don't know about networking. + +**Protocol as a boundary** -- The protocol layer cleanly separates server serialization from client deserialization. Neither side depends on the other's implementation. + +**Controller pattern** -- Entity behavior is decoupled from entity state via the controller composition pattern. Controllers can be developed and tested independently. + +### Dependency Graph Complexity + +The server has ~15 singleton/static instances that form a fully-connected dependency graph at the top level. The world-scoped classes (EntityManager, AudioManager, ChunkLattice, etc.) are better -- they're scoped to a World instance and reference each other through the World facade. + +### Recommendations + +1. Replace singleton access with constructor injection for testability. +2. Consider making the client Game class pass specific dependencies to managers rather than `this` (the entire Game). +3. Define explicit interfaces at module boundaries (e.g., `IWorld` that EntityManager depends on rather than the concrete `World`). + +--- + +## Summary of Top Recommendations + +### Critical (Score < 5) + +1. **DIP (4/10)**: Introduce interfaces for key services and use constructor injection instead of singleton access. This is the single most impactful architectural improvement for testability and flexibility. + +2. **Coupling (5/10)**: The singleton pattern creates a fully-connected dependency graph. Moving to explicit dependency injection would naturally reduce coupling. + +### Important (Score 5-6) + +3. **Law of Demeter (5/10)**: Add facade methods to reduce deep property chains. `world.tickEntities(delta)` instead of `world.entityManager.tickEntities(delta)`. + +4. **SRP (6/10)**: Decompose `Entity` and `DefaultPlayerEntityController` into focused sub-components. Entity's physics management should be delegated. + +5. **DRY (6/10)**: Extract repeated animation state management patterns and consider generating serialization code from protocol schemas. + +### Maintain (Score 7+) + +6. The **event-driven architecture**, **controller composition pattern**, **options interfaces**, and **three-layer separation** are strong. These patterns should be preserved and extended. + +7. The codebase's **pragmatic approach** (KISS 8/10, YAGNI 8/10) is a significant strength. Resist adding abstraction for its own sake -- only add what solves concrete problems. + +--- + +## Architecture Pattern Summary + +| Pattern | Usage | Assessment | +|---------|-------|------------| +| Singleton | GameServer, all managers | Overused; testability concern | +| Composition | Entity + Controller | Excellent; right approach | +| Observer/Event | EventRouter throughout | Well-implemented; primary extension mechanism | +| Facade | World, GameServer | Appropriate for game engine API surface | +| Strategy | EntityController hierarchy | Clean pattern, well-executed | +| Delta Synchronization | NetworkSynchronizer | Sophisticated and performant | +| Builder | CollisionGroupsBuilder | Appropriate for bitmask construction | +| Registry | BlockTypeRegistry, ModelRegistry | Simple and effective | From aa2cb7fdedae120bf8159509621c25d296fef39e Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 18:21:30 +1100 Subject: [PATCH 08/19] docs: compile CODING_STANDARDS.md from 6 analysis reports Master guidelines document for reviewing AI-generated pull requests, compiled from server architecture, client architecture, protocol layer, style conventions, code smells, and SOLID principles analyses. Includes: hard rules, naming conventions, code organization, type safety, error handling, architecture patterns, performance patterns, formatting rules, and a comprehensive PR review checklist. Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 861 ++++++++++++++++++ .../progress.md | 9 + 2 files changed, 870 insertions(+) create mode 100644 CODING_STANDARDS.md create mode 100644 ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md new file mode 100644 index 00000000..0fabf774 --- /dev/null +++ b/CODING_STANDARDS.md @@ -0,0 +1,861 @@ +# Hytopia Coding Standards & AI PR Review Guidelines + +Compiled from a 6-agent analysis of ~49k lines of TypeScript across `server/src/`, `client/src/`, and `protocol/`. + +--- + +## Quick Reference: Hard Rules + +These are non-negotiable. Any PR violating these should be rejected. + +| # | Rule | Rationale | +|---|------|-----------| +| 1 | No file over 800 lines | Prevents God Classes (NetworkSynchronizer: 1561, client Entity: 2900) | +| 2 | No method over 40 lines | `tickWithPlayerInput` at 225 lines is a cautionary tale | +| 3 | No `any` in exported types | Use `unknown` or proper types; `any` breaks the type system | +| 4 | No monkey-patching | Fragile, unchainable, no cleanup path | +| 5 | No `console.log` in production code | Use `ErrorHandler` or `console.info` | +| 6 | No TODO for error handling | Handle errors before merging | +| 7 | Pair every event listener with cleanup | Store references; remove in `detach()`/`dispose()` | +| 8 | No inline value equality checks | Create and use utility functions | +| 9 | Use `async/await` over `.then()` chains | Clearer error handling, better stack traces | +| 10 | Configuration arrays must be `readonly` | Prevents external mutation of internal state | + +--- + +## 1. Naming Conventions + +### 1.1 Classes + +**PascalCase** with role-specific suffixes. The suffix communicates architectural role. + +```typescript +// DO: Role suffix from this approved list +class PlayerManager { } // Owns a collection of domain objects +class BlockTypeRegistry { } // Stores type/definition registrations +class CollisionGroupsBuilder { } // Constructs complex objects step-by-step +class ErrorHandler { } // Processes specific concerns +class WorldLoop { } // Manages a lifecycle/loop +class EventRouter { } // Routes/dispatches events +class NetworkSynchronizer { } // Coordinates sync operations + +// DON'T: Generic suffixes +class PlayerService { } // "Service" is meaningless +class EntityHelper { } // "Helper" is meaningless +class MathUtil { } // "Util" is meaningless +``` + +### 1.2 Methods + +**camelCase** with verb prefix. The verb communicates the operation type. + +| Verb | Usage | Example | +|------|-------|---------| +| `get` | Retrieve value/query | `getConnectedPlayers()` | +| `set` | Mutate + emit event | `setAmbientLightColor(color)` | +| `is` / `has` | Boolean check | `isValidPacket()`, `hasListeners()` | +| `create` | Factory/instantiation | `createWorld(options)` | +| `load` | Async resource loading | `loadMap(map)` | +| `register` / `unregister` | Add/remove from registry | `registerEntity(entity)` | +| `emit` / `on` / `off` | Event system | `emit(eventType, payload)` | +| `serialize` | Convert to wire format | `serialize()` | +| `tick` | Per-frame update | `tickWithPlayerInput(input)` | + +```typescript +// DO +public getConnectedPlayers(): Player[] { } +public setAmbientLightColor(color: RgbColor): void { } +public isSpawned(): boolean { } + +// DON'T +public playerGet(): Player[] { } // wrong verb position +public ambientLightColorSet(): void { } // wrong verb position +public spawned(): boolean { } // missing verb prefix +``` + +### 1.3 Fields & Methods: Private Visibility + +**All private fields and methods use `_` prefix.** This is universal across both server and client. + +```typescript +// DO +private _world: World | undefined; +private _entities: Map = new Map(); +private _leaveWorld() { } +private _onConnectionOpened() { } + +// DON'T +private world: World | undefined; +private leaveWorld() { } +``` + +### 1.4 Constants + +**SCREAMING_SNAKE_CASE** for module-level constants. + +```typescript +const RECONNECT_WINDOW_MS = 30 * 1000; +const FRAME_HEADER_SIZE = 4; +const DEFAULT_ANIMATION_BLEND_TIME_S = 0.1; +export const ENTITY_POSITION_UPDATE_THRESHOLD_SQ = 0.04 * 0.04; +``` + +### 1.5 Enums + +**PascalCase** name, **UPPER_SNAKE_CASE** members. + +```typescript +// DO +export enum PlayerEvent { + CHAT_MESSAGE_SEND = 'PLAYER.CHAT_MESSAGE_SEND', + JOINED_WORLD = 'PLAYER.JOINED_WORLD', + LEFT_WORLD = 'PLAYER.LEFT_WORLD', +} + +// DON'T: PascalCase members +export enum PlayerEvent { + ChatMessageSend = 'PLAYER.CHAT_MESSAGE_SEND', // wrong +} +``` + +### 1.6 Event String Format + +`'NAMESPACE.EVENT_NAME'` using dot separator. Namespace matches the class name in UPPER_SNAKE_CASE. + +```typescript +'GAMESERVER.START' +'PLAYER.JOINED_WORLD' +'ENTITY.SPAWN' +'WORLD_LOOP.TICK_START' +'CONNECTION.OPENED' +``` + +### 1.7 Event Payload Interfaces + +Each event-emitting class exports a paired `{ClassName}EventPayloads` interface with computed property keys: + +```typescript +export interface PlayerEventPayloads { + [PlayerEvent.CHAT_MESSAGE_SEND]: { player: Player, message: string } + [PlayerEvent.JOINED_WORLD]: { player: Player, world: World } +} +``` + +### 1.8 Files + +**PascalCase.ts** matching the primary exported class/type. Barrel files use lowercase `index.ts`. + +### 1.9 Interfaces & Types + +- **Interfaces**: Object shapes, options, payloads, schemas. Suffixes: `Options`, `Payloads`, `Schema`, `Like`. +- **Types**: Union types, aliases, computed types. +- **`Like` suffix**: Lightweight structural interfaces for parameters (e.g., `Vector3Like`, `QuaternionLike`). + +### 1.10 Generic Type Parameters + +Use `T` prefix: `TEventType`, `TPayload`, `TId`, `TSchema`. + +--- + +## 2. Import & Export Patterns + +### 2.1 Import Order + +1. External packages (`@dimforge/rapier3d-simd-compat`, `eventemitter3`, `ws`) +2. Internal modules via `@/` (server) or relative `./`/`../` (client/protocol) +3. Type-only imports last (`import type`) + +```typescript +import RAPIER from '@dimforge/rapier3d-simd-compat'; +import BlockTextureRegistry from '@/textures/BlockTextureRegistry'; +import EventRouter from '@/events/EventRouter'; +import type World from '@/worlds/World'; +import type { RaycastHit } from '@/worlds/physics/Simulation'; +``` + +### 2.2 Export Rules + +- **`export default class`** for the primary class in each file +- **Named exports** for enums, constants, types, functions alongside the default + +```typescript +export enum GameServerEvent { ... } +export interface GameServerEventPayloads { ... } +export default class GameServer { ... } +``` + +### 2.3 Type-Only Imports + +Always use `import type` for type-only imports. This is enforced codebase-wide. + +```typescript +// DO +import type Connection from '@/networking/Connection'; +import type { JSONSchemaType } from 'ajv'; + +// DON'T +import { JSONSchemaType } from 'ajv'; // value import for a type +``` + +--- + +## 3. Code Organization + +### 3.1 File Structure + +``` +1. Imports (external → internal → type-only) +2. Module-level constants (SCREAMING_SNAKE_CASE) +3. Event enum (export enum XxxEvent { ... }) +4. Event payloads interface (export interface XxxEventPayloads { ... }) +5. Options/Config interfaces (export interface XxxOptions { ... }) +6. export default class Xxx { + a. Static private fields + b. Public readonly fields + c. Private fields (with @internal JSDoc) + d. Constructor + e. Static getter (instance) for singletons + f. Public getters (one-liner format) + g. Public methods + h. Internal public methods (marked @internal) + i. Private methods (underscore-prefixed) +} +``` + +### 3.2 One-Liner Getters + +Simple property access getters go on a single line: + +```typescript +// DO +public get id(): number { return this._id; } +public get world(): World { return this._world; } +public get entityCount(): number { return this._entities.size; } + +// DON'T: Multi-line for trivial getters +public get id(): number { + return this._id; +} +``` + +### 3.3 Private Backing + Public Getter + Named Setter + +No public property setters. State mutation through named `setX()` methods: + +```typescript +// DO +private _ambientLightColor: RgbColor; +public get ambientLightColor(): RgbColor { return this._ambientLightColor; } +public setAmbientLightColor(color: RgbColor) { + if (this._ambientLightColor === color) return; + this._ambientLightColor = color; + // emit event... +} + +// DON'T: Public setter +public set ambientLightColor(color: RgbColor) { ... } +``` + +### 3.4 `@internal` JSDoc + +Use `/** @internal */` for implementation details that are public for technical reasons but not part of the API: + +```typescript +/** @internal */ +public constructor(connection: Connection, session: PlayerSession) { ... } + +/** @internal */ +public registerEntity(entity: Entity): number { ... } +``` + +--- + +## 4. Type Safety + +### 4.1 Nullable Patterns + +Use `| undefined` (not `| null`). Optional properties use `?:` syntax. + +```typescript +// DO +private _world: World | undefined; +public readonly profilePictureUrl: string | undefined; +worldSelectionHandler?: (player: Player) => Promise; + +// DON'T +private _world: World | null; +``` + +### 4.2 Explicit Return Types + +Public methods must have explicit return type annotations. Private methods may omit them when obvious. + +```typescript +// DO +public getConnectedPlayers(): Player[] { ... } +public get playerCount(): number { ... } +public async scheduleNotification(type: string): Promise { ... } +``` + +### 4.3 `as const` and `satisfies` + +Use together for compile-time validation of constant arrays: + +```typescript +export const SUPPORTED_INPUTS = [ + 'w', 'a', 's', 'd', 'sp', 'sh', ... +] as const satisfies readonly (keyof InputSchema)[]; +``` + +### 4.4 Type Predicates Over `!` Assertions + +Use type predicates or assertion functions instead of non-null assertions after guards. + +```typescript +// DO: assertion function +private _requireWorld(): World { + if (!this._world) throw new Error('Entity not spawned'); + return this._world; +} +// Usage: const world = this._requireWorld(); + +// DON'T: repeated ! assertions +if (this.isSpawned) { + this._world!.entityManager... // fragile + this.emitWithWorld(this._world!, ...); // fragile +} +``` + +### 4.5 No `any` in Public API + +```typescript +// DO +public emit(eventType: string, payload: unknown): boolean; + +// DON'T +public emit(eventType: string, payload: any): boolean; +``` + +--- + +## 5. Error Handling + +### 5.1 ErrorHandler Usage + +Use the static `ErrorHandler` class, not raw `throw`: + +| Method | When to Use | Behavior | +|--------|-------------|----------| +| `ErrorHandler.warning(msg)` | Unexpected but non-breaking state | Log warning, continue | +| `ErrorHandler.error(msg)` | Error that can be recovered from | Log error, return void | +| `ErrorHandler.fatalError(msg)` | Truly unrecoverable invariant violation | Log error, throw | + +### 5.2 Error Message Format + +`ClassName.methodName(): Human-readable description.` + +```typescript +// DO +ErrorHandler.error(`Connection._deserialize(): Invalid packet format. Packet: ${JSON.stringify(packet)}`); +ErrorHandler.warning(`PlayerManager._onConnectionClosed(): Connection ${connection.id} not in map.`); +ErrorHandler.fatalError(`EntityManager.registerEntity(): Entity ${entity.name} already has id ${entity.id}!`); + +// DON'T +ErrorHandler.error('Invalid packet'); // no context +throw new Error('bad data'); // raw throw +``` + +### 5.3 `fatalError` Restrictions + +Use `fatalError` only for impossible invariant violations. Not for: +- Input validation (use `error` or `warning`) +- Missing data (use `error` and skip) +- Configuration problems (use `error` with fallback) + +### 5.4 Early Return Pattern + +Prefer early returns to reduce nesting: + +```typescript +// DO +public joinWorld(world: World) { + if (this._world === world) return; + if (!world.isStarted) return ErrorHandler.warning('World not started'); + // ... rest of logic +} + +// DON'T +public joinWorld(world: World) { + if (this._world !== world) { + if (world.isStarted) { + // ... deeply nested logic + } + } +} +``` + +### 5.5 Try/Catch + +Use sparingly, mainly around I/O, event emission, and transport operations: + +```typescript +try { + this._emitter.emit(eventType, payload); +} catch (error) { + console.error(`EventRouter.emit(): Error emitting event "${eventType}":`, error); +} +``` + +### 5.6 No Empty Catch Blocks + +```typescript +// DON'T +} catch { /* NOOP */ } + +// DO: at minimum log +} catch (error) { + ErrorHandler.warning(`Connection.bindWt(): WebTransport binding failed: ${error}`); +} +``` + +--- + +## 6. Architecture Patterns + +### 6.1 Singleton Pattern + +Two approved variants: + +**Lazy (when initialization order matters):** +```typescript +private static _instance: GameServer; +private constructor() { } +public static get instance(): GameServer { + if (!this._instance) this._instance = new GameServer(); + return this._instance; +} +``` + +**Eager (for most managers):** +```typescript +public static readonly instance: PlayerManager = new PlayerManager(); +private constructor() { ... } +``` + +### 6.2 EventRouter Pattern + +Every significant class extends `EventRouter` and follows the event declaration template: + +```typescript +export enum XxxEvent { + SOMETHING_HAPPENED = 'XXX.SOMETHING_HAPPENED', +} + +export interface XxxEventPayloads { + [XxxEvent.SOMETHING_HAPPENED]: { xxx: Xxx, detail: string } +} + +export default class Xxx extends EventRouter { + // ... + public doSomething() { + // ... + this.emitWithWorld(this._world!, XxxEvent.SOMETHING_HAPPENED, { xxx: this, detail: '...' }); + } +} +``` + +**Event emission scopes:** +- `emit()` — local only +- `emitWithGlobal()` — local + global singleton (for cross-world events) +- `emitWithWorld()` — local + world EventRouter (for NetworkSynchronizer to observe) + +### 6.3 Controller/Strategy Pattern + +Entity behavior is delegated via composition, not inheritance: + +```typescript +// DO: Compose behavior +entity.setController(new DefaultPlayerEntityController({ ... })); + +// DON'T: Inherit behavior +class SwimmingEntity extends Entity { ... } +``` + +Controllers extend `BaseEntityController` and implement lifecycle hooks: +`attach()` → `spawn()` → `tick()` / `tickWithPlayerInput()` → `despawn()` → `detach()` + +### 6.4 Options Object Pattern + +Configuration via single options parameter with optional fields and defaults: + +```typescript +export interface WorldOptions { + id?: number; + name: string; + skyboxUri?: string; + tickRate?: number; // defaults to 60 + gravity?: Vector3Like; // defaults to { x: 0, y: -32, z: 0 } +} + +constructor(options: WorldOptions) { + this._tickRate = options.tickRate ?? 60; + this._gravity = options.gravity ?? { x: 0, y: -32, z: 0 }; +} +``` + +### 6.5 Composition Over Inheritance + +`World` is the prime example — composes 10+ sub-systems rather than inheriting from them: + +```typescript +// DO: Composition +this._audioManager = new AudioManager(this); +this._entityManager = new EntityManager(this); +this._simulation = new Simulation(this, options.tickRate, options.gravity); + +// DON'T: Deep inheritance +class World extends AudioCapable extends EntityCapable extends PhysicsCapable { } +``` + +### 6.6 Protocol Schema Pattern + +Co-located TypeScript type + JSON Schema with extreme key minification: + +```typescript +export type EntitySchema = { + i: number; // entity id + p?: VectorSchema; // position + r?: QuaternionSchema; // rotation + rm?: boolean; // removed +} + +export const entitySchema: JSONSchemaType = { + type: 'object', + properties: { + i: { type: 'number' }, + p: { ...vectorSchema, nullable: true }, + r: { ...quaternionSchema, nullable: true }, + rm: { type: 'boolean', nullable: true }, + }, + required: ['i'], + additionalProperties: false, +} +``` + +--- + +## 7. Formatting Rules + +### ESLint-Enforced + +| Rule | Setting | +|------|---------| +| Indent | 2 spaces, `SwitchCase: 1` | +| Quotes | Single quotes, `avoidEscape: true` | +| Semicolons | Always | +| Trailing commas | Always in multiline | +| Object curly spacing | `{ spaces }` | +| Array bracket spacing | `[ spaces ]` | +| Arrow parens | As-needed (omit for single param) | +| Blank line before return | Required | +| Linebreak style | Unix (LF) | +| Unused vars | Error, except `_` prefixed | +| Floating promises | Error | +| Await thenable | Error | + +### Manual Conventions + +- Arrow functions for event handlers and callbacks (preserves `this`): + ```typescript + private _onClose = (): void => { ... }; + private _tick = (tickDeltaMs: number): void => { ... }; + ``` + +- Ternaries for simple value selection; if/else for complex logic with side effects. + +- Enum value alignment with whitespace padding: + ```typescript + export enum PlayerEvent { + CHAT_MESSAGE_SEND = 'PLAYER.CHAT_MESSAGE_SEND', + JOINED_WORLD = 'PLAYER.JOINED_WORLD', + } + ``` + +--- + +## 8. Performance Patterns + +These patterns are established in the codebase. New code in hot paths must follow them. + +### 8.1 Pre-allocated Working Variables + +Module-level reusable objects to avoid GC pressure: + +```typescript +// DO: module-level working variables +const _workingVec3 = new Vector3(); +const _workingQuat = new Quaternion(); + +class Entity { + tick() { + _workingVec3.set(this._x, this._y, this._z); // reuse, no allocation + } +} + +// DON'T: allocate in hot paths +class Entity { + tick() { + const pos = new Vector3(this._x, this._y, this._z); // GC pressure + } +} +``` + +### 8.2 Manual Matrix Management + +`matrixAutoUpdate = false` everywhere. Update matrices explicitly: + +```typescript +object.matrixAutoUpdate = false; +object.matrixWorldAutoUpdate = false; +// Update manually only when transforms change +object.updateMatrix(); +object.updateMatrixWorld(); +``` + +### 8.3 Packed Numeric Keys for Map Lookups + +Use numeric/packed keys in hot-path maps, not string concatenation: + +```typescript +// DO +const key = (x << 20) | (y << 10) | z; // packed numeric key + +// DON'T +const key = `${x},${y},${z}`; // string allocation every lookup +``` + +### 8.4 Update Thresholds + +Only emit updates when changes exceed thresholds: + +```typescript +const POSITION_THRESHOLD_SQ = 0.04 * 0.04; +const ROTATION_THRESHOLD = Math.cos(0.052 / 2); + +if (positionDeltaSq > POSITION_THRESHOLD_SQ) { + this.emit(EntityEvent.SET_POSITION, { ... }); +} +``` + +### 8.5 Environmental Entity Optimization + +Static world objects (`isEnvironmental = true`) skip per-tick updates entirely. Use this flag for non-interactive decoration. + +--- + +## 9. Client vs Server Differences + +New code must follow the conventions of the layer it's in. + +| Aspect | Server | Client | +|--------|--------|--------| +| Path aliases | `@/` prefix | Relative `./`/`../` | +| TSDoc | Comprehensive, structured | Minimal (improvement needed) | +| EventRouter | Full-featured (emitWithWorld, final) | Lightweight (Map\) | +| Singleton access | `.instance` getter or `readonly instance` | `.instance` getter | +| Event enum suffix | `Event` (e.g., `PlayerEvent`) | `EventType` (e.g., `NetworkManagerEventType`) | +| Event payload style | Interface with computed keys | Namespace with `I`-prefixed interfaces | + +--- + +## 10. Known Improvement Areas + +These are documented weaknesses in the current codebase. New code should not make them worse. Improvements are welcome. + +### 10.1 Dependency Inversion (Score: 4/10) + +The codebase relies heavily on concrete singletons (`GameServer.instance`, `PlayerManager.instance`, etc.) instead of interfaces and injection. This limits testability. + +**For new code**: Prefer constructor injection where practical. If you must use a singleton, at minimum define an interface for the dependency. + +### 10.2 Law of Demeter (Score: 5/10) + +Deep property chains are common: +```typescript +// This pattern exists but should not be extended +this._world.entityManager.getPlayerEntitiesByPlayer(this)[0]; +this._game.settingsManager.qualityPerfTradeoff.viewDistance.enabled; +``` + +**For new code**: Add convenience methods on the owning class rather than reaching through multiple levels. + +### 10.3 God Classes + +| Class | Lines | Status | +|-------|-------|--------| +| Client `Entity.ts` | ~2900 | Should extract AnimationManager, MaterialManager, CullingManager | +| `NetworkSynchronizer` | ~1561 | Should extract per-domain sync handlers | +| `GLTFManager` | ~1812 | Should split loading, caching, instancing | +| `RigidBody` / `Collider` | ~1750 each | Acceptable as physics wrappers | + +**For new code**: Do not add more responsibilities to these files. Extract into new focused classes. + +### 10.4 DRY Violations + +- 60+ identical event handler methods in `NetworkSynchronizer` (use data-driven mapping) +- 70+ identical setter+emit patterns across Entity, PlayerCamera, ParticleEmitter +- Inline RGB color equality checks duplicated + +**For new code**: If you see 5+ methods following the same pattern, use a mapping table or generic helper. + +--- + +## 11. AI PR Review Checklist + +Use this checklist when reviewing AI-generated pull requests. + +### Naming & Style +- [ ] Classes use PascalCase + approved role suffix +- [ ] Methods use camelCase + verb prefix +- [ ] Private fields/methods use `_` prefix +- [ ] Constants use SCREAMING_SNAKE_CASE +- [ ] Enum members use UPPER_SNAKE_CASE +- [ ] Event strings follow `'NAMESPACE.EVENT_NAME'` format +- [ ] Files use PascalCase.ts matching primary export +- [ ] Generic params use `T` prefix + +### Structure +- [ ] No file exceeds 800 lines +- [ ] No method exceeds 40 lines +- [ ] Imports follow: external → internal → type-only order +- [ ] `import type` used for type-only imports +- [ ] Class uses `export default class` +- [ ] Class body follows the 9-section structure (static → fields → constructor → getters → methods → private) +- [ ] One-liner getters for simple property access +- [ ] Named `setX()` methods, no property setters + +### Type Safety +- [ ] No `any` in exported types or interfaces +- [ ] No `as any` except with documented justification +- [ ] Public methods have explicit return types +- [ ] Uses `| undefined` not `| null` +- [ ] No non-null assertions (`!`) after boolean guards — uses type predicates or assertion functions +- [ ] `as const satisfies` for constant arrays + +### Error Handling +- [ ] Uses `ErrorHandler.warning/error/fatalError`, not raw `throw` +- [ ] Error messages follow `ClassName.methodName(): Description` format +- [ ] `fatalError` only for unrecoverable invariant violations +- [ ] No empty catch blocks +- [ ] No `console.log` (use `ErrorHandler` or `console.info`) + +### Architecture +- [ ] Event listeners paired with cleanup in `detach()`/`dispose()` +- [ ] No monkey-patching of methods on other objects +- [ ] Options object pattern for configuration +- [ ] Composition over inheritance +- [ ] Controller pattern for entity behavior + +### Performance (if touching hot paths) +- [ ] Pre-allocated working variables for hot loops +- [ ] `matrixAutoUpdate = false` for Three.js objects +- [ ] No string key concatenation in hot-path maps +- [ ] Update thresholds for position/rotation changes +- [ ] Environmental entity flag for static objects + +### Protocol (if touching network layer) +- [ ] Schema uses 1-3 char property keys with inline comments +- [ ] Co-located TypeScript type + JSON Schema with `JSONSchemaType` +- [ ] `additionalProperties: false` on schemas +- [ ] Packet validation at creation time (fail-fast) +- [ ] Tuple wire format `[id, data, tick?]` + +### General Quality +- [ ] No TODOs for error handling +- [ ] No stale comments referencing values that change +- [ ] Configuration arrays are `readonly` +- [ ] External input validated before merging into internal state +- [ ] `async/await` used instead of `.then()` chains +- [ ] No data-driven boilerplate (5+ identical patterns → use mapping table) +- [ ] Doesn't add responsibilities to known God Classes + +--- + +## 12. ESLint Configuration Reference + +The server ESLint config (`server/eslint.config.js`) enforces: + +```javascript +// Key rules +'indent': ['error', 2, { SwitchCase: 1 }], +'quotes': ['error', 'single', { avoidEscape: true }], +'semi': ['error', 'always'], +'comma-dangle': ['error', 'always-multiline'], +'object-curly-spacing': ['error', 'always'], +'array-bracket-spacing': ['error', 'always'], +'arrow-parens': ['error', 'as-needed'], +'newline-before-return': 'error', +'linebreak-style': ['error', 'unix'], +'@typescript-eslint/no-explicit-any': 'off', // any is allowed (legacy) +'@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }], +'@typescript-eslint/no-floating-promises': 'error', +'@typescript-eslint/await-thenable': 'error', +'@typescript-eslint/prefer-optional-chain': 'error', +``` + +**Note**: While `no-explicit-any` is off in ESLint, the coding standard prohibits `any` in new exported types. The ESLint rule is kept off for legacy compatibility only. + +--- + +## Appendix A: Codebase Architecture Overview + +``` +GameServer (root singleton) +├── BlockTextureRegistry (singleton) +├── ModelRegistry (singleton) +├── PlayerManager (singleton) +│ ├── Player (per-connection) +│ │ ├── PlayerCamera +│ │ ├── PlayerUI +│ │ └── Connection (transport) +│ └── PersistenceManager (singleton) +├── WorldManager (singleton) +│ └── World (per-instance, composition root) +│ ├── AudioManager +│ ├── BlockTypeRegistry +│ ├── ChatManager +│ ├── ChunkLattice → Chunk[] +│ ├── EntityManager → Entity[] +│ ├── NetworkSynchronizer +│ ├── ParticleEmitterManager +│ ├── SceneUIManager +│ ├── Simulation (RAPIER) +│ └── WorldLoop → Ticker +├── Socket (singleton) → Connection[] +└── WebServer (singleton) + +Cross-cutting: + EventRouter (event bus, used by almost every class) + ErrorHandler (static utility) + Serializer (static utility) + PlatformGateway (singleton) +``` + +## Appendix B: Design Principles Scorecard + +| Principle | Score | Key Finding | +|-----------|:-----:|-------------| +| LSP | 8/10 | Clean inheritance hierarchies | +| KISS | 8/10 | Pragmatic game engine design | +| YAGNI | 8/10 | Minimal unused abstractions | +| Composition > Inheritance | 8/10 | Strong preference for composition | +| OCP | 7/10 | Good event/controller extension points | +| ISP | 7/10 | Well-scoped option interfaces | +| Separation of Concerns | 7/10 | Clear server/client/protocol boundaries | +| Cohesion | 7/10 | Focused modules, World as facade | +| SRP | 6/10 | Entity and NetworkSynchronizer overloaded | +| DRY | 6/10 | Serialization and event pattern duplication | +| Law of Demeter | 5/10 | Deep chains through managers | +| Coupling | 5/10 | Fully connected singleton graph | +| DIP | 4/10 | No DI, 15+ singletons, RAPIER leaks to API | +| **Overall** | **6.6/10** | | diff --git a/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md b/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md new file mode 100644 index 00000000..22b8e7f6 --- /dev/null +++ b/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md @@ -0,0 +1,9 @@ +# Progress + +- [x] Server architecture analysis (server-architecture.md) +- [x] Client architecture analysis (client-architecture.md) +- [x] Protocol layer analysis (protocol-architecture.md) +- [x] Style conventions analysis (style-conventions.md) +- [x] Code smells analysis (code-smells.md) +- [x] SOLID principles analysis (principles-analysis.md) +- [x] Compile final CODING_STANDARDS.md from all 6 reports From 54a2ced3614c29b0439a5b3166b019f818017d40 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 18:39:15 +1100 Subject: [PATCH 09/19] docs: lower pattern duplication threshold from 5 to 3 Three identical patterns is enough to warrant a mapping table or generic helper. Five was too lenient. Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md index 0fabf774..81138cf5 100644 --- a/CODING_STANDARDS.md +++ b/CODING_STANDARDS.md @@ -20,6 +20,7 @@ These are non-negotiable. Any PR violating these should be rejected. | 8 | No inline value equality checks | Create and use utility functions | | 9 | Use `async/await` over `.then()` chains | Clearer error handling, better stack traces | | 10 | Configuration arrays must be `readonly` | Prevents external mutation of internal state | +| 11 | 3+ identical patterns → mapping table or helper | Prevents boilerplate sprawl | --- @@ -706,7 +707,7 @@ this._game.settingsManager.qualityPerfTradeoff.viewDistance.enabled; - 70+ identical setter+emit patterns across Entity, PlayerCamera, ParticleEmitter - Inline RGB color equality checks duplicated -**For new code**: If you see 5+ methods following the same pattern, use a mapping table or generic helper. +**For new code**: If you see 3+ methods following the same pattern, use a mapping table or generic helper. --- @@ -776,7 +777,7 @@ Use this checklist when reviewing AI-generated pull requests. - [ ] Configuration arrays are `readonly` - [ ] External input validated before merging into internal state - [ ] `async/await` used instead of `.then()` chains -- [ ] No data-driven boilerplate (5+ identical patterns → use mapping table) +- [ ] No data-driven boilerplate (3+ identical patterns → use mapping table) - [ ] Doesn't add responsibilities to known God Classes --- From 6514d3825f67f9d8a565b04503b926b586e2dceb Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 18:43:55 +1100 Subject: [PATCH 10/19] docs: add contribution process, backwards compat, and data-driven rules New sections from mega-sappy guidelines + ArkDev code analysis: - PR requirements (testing, AI review, backwards compat assessment) - Multi-layer review process (static checks, AI, human, game regression) - Backwards compatibility rules (defaults are sacred, opt-in vs automatic) - Data-driven design principles (no magic numbers, config over hardcode) - Quality gate stack (typecheck, lint, unit, perf, AI, human, manual, regression) - Extended PR review checklist with new categories Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 177 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md index 81138cf5..0e5cf79a 100644 --- a/CODING_STANDARDS.md +++ b/CODING_STANDARDS.md @@ -21,6 +21,161 @@ These are non-negotiable. Any PR violating these should be rejected. | 9 | Use `async/await` over `.then()` chains | Clearer error handling, better stack traces | | 10 | Configuration arrays must be `readonly` | Prevents external mutation of internal state | | 11 | 3+ identical patterns → mapping table or helper | Prevents boilerplate sprawl | +| 12 | No magic numbers | Extract to named constants or config | +| 13 | Data-driven over hardcoded | Game values belong in config, not source code | +| 14 | Defaults are sacred — change with extreme care | A default change silently affects every existing game | +| 15 | Backwards compatibility required | Existing games must not break on SDK upgrade | +| 16 | Prefer config/data files over code constants | Convention-over-configuration; separate data from logic | + +--- + +## 0. Contribution & Review Process + +These rules apply to all contributions — human or AI. + +### 0.1 PR Requirements + +Every PR must include: +- **Problem/benefit statement**: What problem does this solve or what value does it add? +- **Testing done**: What manual and automated testing was performed? On which platforms (desktop, mobile, iOS, Android)? +- **AI tools used**: Which AI models/harnesses were used for development and review? +- **Backwards compatibility assessment**: Does this change have any chance of breaking existing games? If someone upgrades to this SDK version, do they need to change code or assets on their end? +- **Opt-in vs automatic**: If the change is 100% an upgrade in all situations, it should apply automatically with no code changes. If it has tradeoffs (e.g., performance cost for visual improvement), it should be opt-in. + +### 0.2 Review Layers + +PRs must pass through multiple review layers before merge: + +1. **Static type checks** — `npm run typecheck` must pass locally and in CI (GitHub Actions) +2. **Linting** — `npm run lint` must pass +3. **Unit tests** — pragmatic, high signal-to-noise tests that catch regressions +4. **Performance tests** — run locally at minimum; CI integration where viable +5. **AI code review** — at least 1 additional AI review with a **fresh context** (even if same model). Preferably 2 different tools (e.g., Claude Code + Codex). Codex can be hooked to GitHub for automatic review. +6. **Human code review** — a human with code knowledge reviewing for: wrong architecture, code smells, bad practices, suspicious hardcoding. Not checking syntax — checking design. +7. **Manual testing** — the PR submitter must have manually tested. Standard guides/tools for testing modified SDK code in a game must be provided. Test on multiple platforms. +8. **Game regression testing** — where possible, run PRs against existing games (e.g., Hatch A Zoo, VoxFire) to verify no breakage. PR creators should provide evidence of testing against published games. + +### 0.3 Backwards Compatibility + +This is critical. A change that "works" but breaks existing games is worse than no change. + +``` +Questions every PR must answer: +1. Does this change any default value? + → If yes, what existing behavior changes silently? +2. Does this change any public API signature? + → If yes, what existing code breaks on upgrade? +3. Does this change any wire protocol? + → If yes, what client/server version combinations break? +4. Is this opt-in or automatic? + → Automatic changes must be universally beneficial with zero downsides + → Changes with tradeoffs must be opt-in +``` + +**Real examples of backwards compatibility failures:** +- Changing default particle alpha → broke smoke grenade visuals in existing games +- Changing player controller defaults → existing games behaved differently on upgrade +- Changing character model conventions → existing games needed asset updates + +**Rule**: Default values are part of the API contract. Changing a default is a breaking change, even if the parameter is "optional." + +### 0.4 Best Solution / Robustness Check + +A PR might solve a real problem but: +- Is it the **best** approach, or just the first approach that worked? +- Does it account for **different game scenarios**? (e.g., a feature that works for 10 entities but kills performance with 1000) +- Should it be **opt-in** with per-entity/per-world granularity? +- Does it **scale** to large games? + +Reviewers should ask: "Would this work in Hatch A Zoo with hundreds of entities?" + +### 0.5 Quality Gate Stack + +| Layer | Tool/Method | When | +|-------|------------|------| +| Type safety | `npm run typecheck` | Every commit, CI | +| Lint | `npm run lint` | Every commit, CI | +| Unit tests | `npm run test` | Every commit, CI | +| Perf tests | `npm run test:perf` (local) | Before PR, ideally CI | +| AI review | Fresh-context AI review (1-2 tools) | Before PR | +| Human review | Architecture/design review | Before merge | +| Manual test | Desktop + mobile platforms | Before PR | +| Game regression | Run against existing games | Before merge (where possible) | + +--- + +## 0b. Data-Driven Design Principles + +### No Magic Numbers + +Every numeric literal in game logic must be a named constant or config value. + +```typescript +// DO: Named constant +const RECONNECT_WINDOW_MS = 30 * 1000; +const MAX_ACTIVE_AUDIO_NODES = 64; +private static readonly WALK_FORCE_THRESHOLD = 0.1; + +// DO: Config-driven +const cooldownMs = weaponConfig.cooldownMs; +const damage = weaponConfig.damage * playerDamageMultiplier; + +// DON'T: Magic numbers in logic +if (distance < 16) { ... } // what is 16? +setTimeout(callback, 5000); // why 5000? +this._health -= 25; // where does 25 come from? +``` + +### Data-Driven Over Hardcoded + +Game values (damage, cooldowns, speeds, costs, drop rates, animation names) belong in configuration files, not source code. Source code reads config; it doesn't define game balance. + +```typescript +// DO: Read from config +const damage = catalog.getWeapon(weaponId).damage; +const spawnRate = balanceConfig.enemySpawnRatePerSecond; + +// DON'T: Hardcode game values +const SWORD_DAMAGE = 25; // belongs in config +const SPAWN_RATE = 0.5; // belongs in config +``` + +### Convention Over Configuration + +Establish conventions that eliminate boilerplate configuration: +- File naming conventions that auto-register content +- Default values that cover 90% of use cases +- Predictable patterns that don't require explicit wiring + +### Separate UI, Logic, and Data + +Three concerns, three layers. Never mix them: +- **Data**: Config files, schemas, generated catalogs +- **Logic**: Runtime systems, state management, game rules +- **UI**: Presentation, HUD, modals, scene UI + +### Defaults Are Sacred + +Changing a default value is a **breaking change** in disguise. It silently alters behavior for every existing consumer. + +```typescript +// DANGEROUS: Changing this default +export interface ParticleEmitterOptions { + opacity?: number; // was 1.0, someone changes to 0.8 + // → Every game's particles suddenly become semi-transparent +} + +// SAFE: Add new option with backwards-compatible default +export interface ParticleEmitterOptions { + opacity?: number; // stays 1.0 + fadeOnDeath?: boolean; // NEW, defaults to false (opt-in) +} +``` + +Before changing any default: +1. List every place the default is consumed +2. Assess impact on existing games +3. If any game would behave differently → it's a breaking change → requires migration path or opt-in --- @@ -771,6 +926,28 @@ Use this checklist when reviewing AI-generated pull requests. - [ ] Packet validation at creation time (fail-fast) - [ ] Tuple wire format `[id, data, tick?]` +### Data-Driven Design +- [ ] No magic numbers — all numeric literals in game logic are named constants or config values +- [ ] Game values (damage, cooldowns, speeds, costs) come from config, not hardcoded in source +- [ ] UI, logic, and data concerns are separated +- [ ] No new defaults changed without backwards compatibility assessment +- [ ] New features with tradeoffs are opt-in, not automatic + +### Backwards Compatibility +- [ ] No default values changed silently +- [ ] No public API signatures broken +- [ ] Existing games work without code changes on upgrade +- [ ] If breaking change is necessary, migration path documented +- [ ] Change scales to large games (100+ entities, multiple worlds) + +### PR Process +- [ ] PR description includes problem statement, testing done, AI tools used +- [ ] At least 1 fresh-context AI review completed +- [ ] Manual testing done on relevant platforms +- [ ] `npm run typecheck` and `npm run lint` pass +- [ ] Unit tests pass; new tests added for new behavior +- [ ] Existing game regression considered + ### General Quality - [ ] No TODOs for error handling - [ ] No stale comments referencing values that change From 7f0a832f0f9031b62e6e4fe437b2a4e570387712 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 18:45:27 +1100 Subject: [PATCH 11/19] docs: remove game-specific references from coding standards Strip out damage/weapon/catalog examples that are specific to mega-sappy game logic, keep rules SDK-generic. Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md index 0e5cf79a..dcc10a2f 100644 --- a/CODING_STANDARDS.md +++ b/CODING_STANDARDS.md @@ -116,28 +116,26 @@ const RECONNECT_WINDOW_MS = 30 * 1000; const MAX_ACTIVE_AUDIO_NODES = 64; private static readonly WALK_FORCE_THRESHOLD = 0.1; -// DO: Config-driven -const cooldownMs = weaponConfig.cooldownMs; -const damage = weaponConfig.damage * playerDamageMultiplier; - // DON'T: Magic numbers in logic if (distance < 16) { ... } // what is 16? setTimeout(callback, 5000); // why 5000? -this._health -= 25; // where does 25 come from? +if (count > 64) { ... } // where does 64 come from? ``` ### Data-Driven Over Hardcoded -Game values (damage, cooldowns, speeds, costs, drop rates, animation names) belong in configuration files, not source code. Source code reads config; it doesn't define game balance. +Tunable values (tick rates, thresholds, timeouts, capacities) should be configurable rather than buried in source code. ```typescript -// DO: Read from config -const damage = catalog.getWeapon(weaponId).damage; -const spawnRate = balanceConfig.enemySpawnRatePerSecond; +// DO: Configurable via options +constructor(options: WorldOptions) { + this._tickRate = options.tickRate ?? 60; + this._gravity = options.gravity ?? { x: 0, y: -32, z: 0 }; +} -// DON'T: Hardcode game values -const SWORD_DAMAGE = 25; // belongs in config -const SPAWN_RATE = 0.5; // belongs in config +// DON'T: Bury tunable values in logic +this._tickRate = 60; // not configurable +this._gravity = { x: 0, y: -32, z: 0 }; // not overridable ``` ### Convention Over Configuration @@ -927,8 +925,8 @@ Use this checklist when reviewing AI-generated pull requests. - [ ] Tuple wire format `[id, data, tick?]` ### Data-Driven Design -- [ ] No magic numbers — all numeric literals in game logic are named constants or config values -- [ ] Game values (damage, cooldowns, speeds, costs) come from config, not hardcoded in source +- [ ] No magic numbers — all numeric literals are named constants or config values +- [ ] Tunable values (thresholds, rates, capacities) are configurable via options, not hardcoded - [ ] UI, logic, and data concerns are separated - [ ] No new defaults changed without backwards compatibility assessment - [ ] New features with tradeoffs are opt-in, not automatic From fa1e236b85477e544445e526dc824baabfd50ba9 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 19:00:17 +1100 Subject: [PATCH 12/19] docs: split coding standards into two focused documents CODING_STANDARDS.md now focuses purely on code quality (naming, types, architecture, performance). Process/governance content extracted into new CONTRIBUTING.md (PR requirements, backwards compatibility, review layers, testing expectations). Hard rules consolidated from 16 to 13. Section numbering cleaned up. Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 406 ++++++++++---------------------------------- CONTRIBUTING.md | 132 ++++++++++++++ 2 files changed, 217 insertions(+), 321 deletions(-) create mode 100644 CONTRIBUTING.md diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md index dcc10a2f..ae085019 100644 --- a/CODING_STANDARDS.md +++ b/CODING_STANDARDS.md @@ -1,179 +1,30 @@ -# Hytopia Coding Standards & AI PR Review Guidelines +# Hytopia Coding Standards -Compiled from a 6-agent analysis of ~49k lines of TypeScript across `server/src/`, `client/src/`, and `protocol/`. +Extracted from ~49k lines of TypeScript across `server/src/`, `client/src/`, and `protocol/`. + +For contribution process, PR requirements, and review workflow, see [CONTRIBUTING.md](CONTRIBUTING.md). --- -## Quick Reference: Hard Rules +## Hard Rules -These are non-negotiable. Any PR violating these should be rejected. +Non-negotiable. Any PR violating these should be rejected. | # | Rule | Rationale | |---|------|-----------| -| 1 | No file over 800 lines | Prevents God Classes (NetworkSynchronizer: 1561, client Entity: 2900) | -| 2 | No method over 40 lines | `tickWithPlayerInput` at 225 lines is a cautionary tale | -| 3 | No `any` in exported types | Use `unknown` or proper types; `any` breaks the type system | -| 4 | No monkey-patching | Fragile, unchainable, no cleanup path | -| 5 | No `console.log` in production code | Use `ErrorHandler` or `console.info` | -| 6 | No TODO for error handling | Handle errors before merging | -| 7 | Pair every event listener with cleanup | Store references; remove in `detach()`/`dispose()` | -| 8 | No inline value equality checks | Create and use utility functions | -| 9 | Use `async/await` over `.then()` chains | Clearer error handling, better stack traces | -| 10 | Configuration arrays must be `readonly` | Prevents external mutation of internal state | -| 11 | 3+ identical patterns → mapping table or helper | Prevents boilerplate sprawl | -| 12 | No magic numbers | Extract to named constants or config | -| 13 | Data-driven over hardcoded | Game values belong in config, not source code | -| 14 | Defaults are sacred — change with extreme care | A default change silently affects every existing game | -| 15 | Backwards compatibility required | Existing games must not break on SDK upgrade | -| 16 | Prefer config/data files over code constants | Convention-over-configuration; separate data from logic | - ---- - -## 0. Contribution & Review Process - -These rules apply to all contributions — human or AI. - -### 0.1 PR Requirements - -Every PR must include: -- **Problem/benefit statement**: What problem does this solve or what value does it add? -- **Testing done**: What manual and automated testing was performed? On which platforms (desktop, mobile, iOS, Android)? -- **AI tools used**: Which AI models/harnesses were used for development and review? -- **Backwards compatibility assessment**: Does this change have any chance of breaking existing games? If someone upgrades to this SDK version, do they need to change code or assets on their end? -- **Opt-in vs automatic**: If the change is 100% an upgrade in all situations, it should apply automatically with no code changes. If it has tradeoffs (e.g., performance cost for visual improvement), it should be opt-in. - -### 0.2 Review Layers - -PRs must pass through multiple review layers before merge: - -1. **Static type checks** — `npm run typecheck` must pass locally and in CI (GitHub Actions) -2. **Linting** — `npm run lint` must pass -3. **Unit tests** — pragmatic, high signal-to-noise tests that catch regressions -4. **Performance tests** — run locally at minimum; CI integration where viable -5. **AI code review** — at least 1 additional AI review with a **fresh context** (even if same model). Preferably 2 different tools (e.g., Claude Code + Codex). Codex can be hooked to GitHub for automatic review. -6. **Human code review** — a human with code knowledge reviewing for: wrong architecture, code smells, bad practices, suspicious hardcoding. Not checking syntax — checking design. -7. **Manual testing** — the PR submitter must have manually tested. Standard guides/tools for testing modified SDK code in a game must be provided. Test on multiple platforms. -8. **Game regression testing** — where possible, run PRs against existing games (e.g., Hatch A Zoo, VoxFire) to verify no breakage. PR creators should provide evidence of testing against published games. - -### 0.3 Backwards Compatibility - -This is critical. A change that "works" but breaks existing games is worse than no change. - -``` -Questions every PR must answer: -1. Does this change any default value? - → If yes, what existing behavior changes silently? -2. Does this change any public API signature? - → If yes, what existing code breaks on upgrade? -3. Does this change any wire protocol? - → If yes, what client/server version combinations break? -4. Is this opt-in or automatic? - → Automatic changes must be universally beneficial with zero downsides - → Changes with tradeoffs must be opt-in -``` - -**Real examples of backwards compatibility failures:** -- Changing default particle alpha → broke smoke grenade visuals in existing games -- Changing player controller defaults → existing games behaved differently on upgrade -- Changing character model conventions → existing games needed asset updates - -**Rule**: Default values are part of the API contract. Changing a default is a breaking change, even if the parameter is "optional." - -### 0.4 Best Solution / Robustness Check - -A PR might solve a real problem but: -- Is it the **best** approach, or just the first approach that worked? -- Does it account for **different game scenarios**? (e.g., a feature that works for 10 entities but kills performance with 1000) -- Should it be **opt-in** with per-entity/per-world granularity? -- Does it **scale** to large games? - -Reviewers should ask: "Would this work in Hatch A Zoo with hundreds of entities?" - -### 0.5 Quality Gate Stack - -| Layer | Tool/Method | When | -|-------|------------|------| -| Type safety | `npm run typecheck` | Every commit, CI | -| Lint | `npm run lint` | Every commit, CI | -| Unit tests | `npm run test` | Every commit, CI | -| Perf tests | `npm run test:perf` (local) | Before PR, ideally CI | -| AI review | Fresh-context AI review (1-2 tools) | Before PR | -| Human review | Architecture/design review | Before merge | -| Manual test | Desktop + mobile platforms | Before PR | -| Game regression | Run against existing games | Before merge (where possible) | - ---- - -## 0b. Data-Driven Design Principles - -### No Magic Numbers - -Every numeric literal in game logic must be a named constant or config value. - -```typescript -// DO: Named constant -const RECONNECT_WINDOW_MS = 30 * 1000; -const MAX_ACTIVE_AUDIO_NODES = 64; -private static readonly WALK_FORCE_THRESHOLD = 0.1; - -// DON'T: Magic numbers in logic -if (distance < 16) { ... } // what is 16? -setTimeout(callback, 5000); // why 5000? -if (count > 64) { ... } // where does 64 come from? -``` - -### Data-Driven Over Hardcoded - -Tunable values (tick rates, thresholds, timeouts, capacities) should be configurable rather than buried in source code. - -```typescript -// DO: Configurable via options -constructor(options: WorldOptions) { - this._tickRate = options.tickRate ?? 60; - this._gravity = options.gravity ?? { x: 0, y: -32, z: 0 }; -} - -// DON'T: Bury tunable values in logic -this._tickRate = 60; // not configurable -this._gravity = { x: 0, y: -32, z: 0 }; // not overridable -``` - -### Convention Over Configuration - -Establish conventions that eliminate boilerplate configuration: -- File naming conventions that auto-register content -- Default values that cover 90% of use cases -- Predictable patterns that don't require explicit wiring - -### Separate UI, Logic, and Data - -Three concerns, three layers. Never mix them: -- **Data**: Config files, schemas, generated catalogs -- **Logic**: Runtime systems, state management, game rules -- **UI**: Presentation, HUD, modals, scene UI - -### Defaults Are Sacred - -Changing a default value is a **breaking change** in disguise. It silently alters behavior for every existing consumer. - -```typescript -// DANGEROUS: Changing this default -export interface ParticleEmitterOptions { - opacity?: number; // was 1.0, someone changes to 0.8 - // → Every game's particles suddenly become semi-transparent -} - -// SAFE: Add new option with backwards-compatible default -export interface ParticleEmitterOptions { - opacity?: number; // stays 1.0 - fadeOnDeath?: boolean; // NEW, defaults to false (opt-in) -} -``` - -Before changing any default: -1. List every place the default is consumed -2. Assess impact on existing games -3. If any game would behave differently → it's a breaking change → requires migration path or opt-in +| 1 | No file over 800 lines | Prevents God Classes | +| 2 | No method over 40 lines | Decompose into focused helpers | +| 3 | No `any` in exported types | Use `unknown` or proper types | +| 4 | No magic numbers | Extract to named constants or options | +| 5 | No monkey-patching | Fragile, unchainable, no cleanup path | +| 6 | No `console.log` in production code | Use `ErrorHandler` or `console.info` | +| 7 | No TODO for error handling | Handle errors before merging | +| 8 | Pair every event listener with cleanup | Store references; remove in `detach()`/`dispose()` | +| 9 | No inline value equality checks | Create and use utility functions | +| 10 | 3+ identical patterns → mapping table or helper | Prevents boilerplate sprawl | +| 11 | Configuration arrays must be `readonly` | Prevents external mutation | +| 12 | Use `async/await` over `.then()` chains | Clearer error handling, better stack traces | +| 13 | Tunable values must be configurable via options | Not buried in source code | --- @@ -181,7 +32,7 @@ Before changing any default: ### 1.1 Classes -**PascalCase** with role-specific suffixes. The suffix communicates architectural role. +**PascalCase** with role-specific suffixes. ```typescript // DO: Role suffix from this approved list @@ -201,7 +52,7 @@ class MathUtil { } // "Util" is meaningless ### 1.2 Methods -**camelCase** with verb prefix. The verb communicates the operation type. +**camelCase** with verb prefix. | Verb | Usage | Example | |------|-------|---------| @@ -227,9 +78,9 @@ public ambientLightColorSet(): void { } // wrong verb position public spawned(): boolean { } // missing verb prefix ``` -### 1.3 Fields & Methods: Private Visibility +### 1.3 Private Fields & Methods -**All private fields and methods use `_` prefix.** This is universal across both server and client. +**All private fields and methods use `_` prefix.** ```typescript // DO @@ -259,22 +110,16 @@ export const ENTITY_POSITION_UPDATE_THRESHOLD_SQ = 0.04 * 0.04; **PascalCase** name, **UPPER_SNAKE_CASE** members. ```typescript -// DO export enum PlayerEvent { CHAT_MESSAGE_SEND = 'PLAYER.CHAT_MESSAGE_SEND', JOINED_WORLD = 'PLAYER.JOINED_WORLD', LEFT_WORLD = 'PLAYER.LEFT_WORLD', } - -// DON'T: PascalCase members -export enum PlayerEvent { - ChatMessageSend = 'PLAYER.CHAT_MESSAGE_SEND', // wrong -} ``` ### 1.6 Event String Format -`'NAMESPACE.EVENT_NAME'` using dot separator. Namespace matches the class name in UPPER_SNAKE_CASE. +`'NAMESPACE.EVENT_NAME'` using dot separator: ```typescript 'GAMESERVER.START' @@ -286,7 +131,7 @@ export enum PlayerEvent { ### 1.7 Event Payload Interfaces -Each event-emitting class exports a paired `{ClassName}EventPayloads` interface with computed property keys: +Each event-emitting class exports a paired `{ClassName}EventPayloads` interface: ```typescript export interface PlayerEventPayloads { @@ -340,7 +185,7 @@ export default class GameServer { ... } ### 2.3 Type-Only Imports -Always use `import type` for type-only imports. This is enforced codebase-wide. +Always use `import type` for type-only imports. ```typescript // DO @@ -378,8 +223,6 @@ import { JSONSchemaType } from 'ajv'; // value import for a type ### 3.2 One-Liner Getters -Simple property access getters go on a single line: - ```typescript // DO public get id(): number { return this._id; } @@ -445,7 +288,6 @@ private _world: World | null; Public methods must have explicit return type annotations. Private methods may omit them when obvious. ```typescript -// DO public getConnectedPlayers(): Player[] { ... } public get playerCount(): number { ... } public async scheduleNotification(type: string): Promise { ... } @@ -463,8 +305,6 @@ export const SUPPORTED_INPUTS = [ ### 4.4 Type Predicates Over `!` Assertions -Use type predicates or assertion functions instead of non-null assertions after guards. - ```typescript // DO: assertion function private _requireWorld(): World { @@ -528,8 +368,6 @@ Use `fatalError` only for impossible invariant violations. Not for: ### 5.4 Early Return Pattern -Prefer early returns to reduce nesting: - ```typescript // DO public joinWorld(world: World) { @@ -548,19 +386,7 @@ public joinWorld(world: World) { } ``` -### 5.5 Try/Catch - -Use sparingly, mainly around I/O, event emission, and transport operations: - -```typescript -try { - this._emitter.emit(eventType, payload); -} catch (error) { - console.error(`EventRouter.emit(): Error emitting event "${eventType}":`, error); -} -``` - -### 5.6 No Empty Catch Blocks +### 5.5 No Empty Catch Blocks ```typescript // DON'T @@ -578,7 +404,7 @@ try { ### 6.1 Singleton Pattern -Two approved variants: +Two approved variants. Note: the codebase is singleton-heavy (DIP score: 4/10). New code should prefer constructor injection where practical. If you must use a singleton, define an interface for the dependency. **Lazy (when initialization order matters):** ```typescript @@ -611,10 +437,6 @@ export interface XxxEventPayloads { export default class Xxx extends EventRouter { // ... - public doSomething() { - // ... - this.emitWithWorld(this._world!, XxxEvent.SOMETHING_HAPPENED, { xxx: this, detail: '...' }); - } } ``` @@ -659,8 +481,6 @@ constructor(options: WorldOptions) { ### 6.5 Composition Over Inheritance -`World` is the prime example — composes 10+ sub-systems rather than inheriting from them: - ```typescript // DO: Composition this._audioManager = new AudioManager(this); @@ -739,12 +559,10 @@ export const entitySchema: JSONSchemaType = { ## 8. Performance Patterns -These patterns are established in the codebase. New code in hot paths must follow them. +New code in hot paths must follow these established patterns. ### 8.1 Pre-allocated Working Variables -Module-level reusable objects to avoid GC pressure: - ```typescript // DO: module-level working variables const _workingVec3 = new Vector3(); @@ -766,8 +584,6 @@ class Entity { ### 8.2 Manual Matrix Management -`matrixAutoUpdate = false` everywhere. Update matrices explicitly: - ```typescript object.matrixAutoUpdate = false; object.matrixWorldAutoUpdate = false; @@ -778,8 +594,6 @@ object.updateMatrixWorld(); ### 8.3 Packed Numeric Keys for Map Lookups -Use numeric/packed keys in hot-path maps, not string concatenation: - ```typescript // DO const key = (x << 20) | (y << 10) | z; // packed numeric key @@ -790,8 +604,6 @@ const key = `${x},${y},${z}`; // string allocation every lookup ### 8.4 Update Thresholds -Only emit updates when changes exceed thresholds: - ```typescript const POSITION_THRESHOLD_SQ = 0.04 * 0.04; const ROTATION_THRESHOLD = Math.cos(0.052 / 2); @@ -824,145 +636,99 @@ New code must follow the conventions of the layer it's in. ## 10. Known Improvement Areas -These are documented weaknesses in the current codebase. New code should not make them worse. Improvements are welcome. - -### 10.1 Dependency Inversion (Score: 4/10) - -The codebase relies heavily on concrete singletons (`GameServer.instance`, `PlayerManager.instance`, etc.) instead of interfaces and injection. This limits testability. - -**For new code**: Prefer constructor injection where practical. If you must use a singleton, at minimum define an interface for the dependency. +Documented weaknesses. New code should not make them worse. Improvements are welcome. -### 10.2 Law of Demeter (Score: 5/10) +| Area | Score | Issue | Guidance for new code | +|------|:-----:|-------|----------------------| +| Dependency Inversion | 4/10 | 15+ concrete singletons, no DI | Prefer constructor injection; define interfaces | +| Coupling | 5/10 | Fully connected singleton graph | Don't add new singleton dependencies | +| Law of Demeter | 5/10 | Deep chains through managers | Add convenience methods instead of reaching through | +| SRP | 6/10 | Entity, NetworkSynchronizer overloaded | Don't add responsibilities to God Classes | +| DRY | 6/10 | 60+ identical handlers, 70+ identical setters | Use mapping tables for 3+ identical patterns | -Deep property chains are common: -```typescript -// This pattern exists but should not be extended -this._world.entityManager.getPlayerEntitiesByPlayer(this)[0]; -this._game.settingsManager.qualityPerfTradeoff.viewDistance.enabled; -``` - -**For new code**: Add convenience methods on the owning class rather than reaching through multiple levels. +**God Classes — do not add to these files:** -### 10.3 God Classes - -| Class | Lines | Status | -|-------|-------|--------| -| Client `Entity.ts` | ~2900 | Should extract AnimationManager, MaterialManager, CullingManager | -| `NetworkSynchronizer` | ~1561 | Should extract per-domain sync handlers | -| `GLTFManager` | ~1812 | Should split loading, caching, instancing | +| Class | Lines | Should extract | +|-------|-------|----------------| +| Client `Entity.ts` | ~2900 | AnimationManager, MaterialManager, CullingManager | +| `NetworkSynchronizer` | ~1561 | Per-domain sync handlers | +| `GLTFManager` | ~1812 | Loading, caching, instancing | | `RigidBody` / `Collider` | ~1750 each | Acceptable as physics wrappers | -**For new code**: Do not add more responsibilities to these files. Extract into new focused classes. - -### 10.4 DRY Violations - -- 60+ identical event handler methods in `NetworkSynchronizer` (use data-driven mapping) -- 70+ identical setter+emit patterns across Entity, PlayerCamera, ParticleEmitter -- Inline RGB color equality checks duplicated - -**For new code**: If you see 3+ methods following the same pattern, use a mapping table or generic helper. - --- -## 11. AI PR Review Checklist - -Use this checklist when reviewing AI-generated pull requests. +## 11. Code Review Checklist ### Naming & Style -- [ ] Classes use PascalCase + approved role suffix -- [ ] Methods use camelCase + verb prefix -- [ ] Private fields/methods use `_` prefix -- [ ] Constants use SCREAMING_SNAKE_CASE -- [ ] Enum members use UPPER_SNAKE_CASE -- [ ] Event strings follow `'NAMESPACE.EVENT_NAME'` format -- [ ] Files use PascalCase.ts matching primary export -- [ ] Generic params use `T` prefix +- [ ] Classes: PascalCase + approved role suffix +- [ ] Methods: camelCase + verb prefix +- [ ] Private fields/methods: `_` prefix +- [ ] Constants: SCREAMING_SNAKE_CASE +- [ ] Enums: UPPER_SNAKE_CASE members +- [ ] Event strings: `'NAMESPACE.EVENT_NAME'` +- [ ] Files: PascalCase.ts matching primary export +- [ ] Generics: `T` prefix ### Structure - [ ] No file exceeds 800 lines - [ ] No method exceeds 40 lines -- [ ] Imports follow: external → internal → type-only order -- [ ] `import type` used for type-only imports -- [ ] Class uses `export default class` -- [ ] Class body follows the 9-section structure (static → fields → constructor → getters → methods → private) -- [ ] One-liner getters for simple property access -- [ ] Named `setX()` methods, no property setters +- [ ] Imports: external → internal → type-only +- [ ] `import type` for type-only imports +- [ ] `export default class` for primary class +- [ ] Class body follows 9-section structure +- [ ] One-liner getters, named `setX()` methods ### Type Safety -- [ ] No `any` in exported types or interfaces -- [ ] No `as any` except with documented justification +- [ ] No `any` in exported types +- [ ] No `as any` without documented justification - [ ] Public methods have explicit return types -- [ ] Uses `| undefined` not `| null` -- [ ] No non-null assertions (`!`) after boolean guards — uses type predicates or assertion functions +- [ ] `| undefined` not `| null` +- [ ] Type predicates or assertion functions, not `!` assertions - [ ] `as const satisfies` for constant arrays ### Error Handling -- [ ] Uses `ErrorHandler.warning/error/fatalError`, not raw `throw` -- [ ] Error messages follow `ClassName.methodName(): Description` format -- [ ] `fatalError` only for unrecoverable invariant violations +- [ ] `ErrorHandler.warning/error/fatalError`, not raw `throw` +- [ ] Error messages: `ClassName.methodName(): Description` +- [ ] `fatalError` only for unrecoverable invariants - [ ] No empty catch blocks -- [ ] No `console.log` (use `ErrorHandler` or `console.info`) +- [ ] No `console.log` ### Architecture -- [ ] Event listeners paired with cleanup in `detach()`/`dispose()` -- [ ] No monkey-patching of methods on other objects +- [ ] Event listeners paired with cleanup +- [ ] No monkey-patching - [ ] Options object pattern for configuration - [ ] Composition over inheritance -- [ ] Controller pattern for entity behavior +- [ ] Doesn't add to known God Classes -### Performance (if touching hot paths) -- [ ] Pre-allocated working variables for hot loops +### Performance (hot paths) +- [ ] Pre-allocated working variables - [ ] `matrixAutoUpdate = false` for Three.js objects - [ ] No string key concatenation in hot-path maps -- [ ] Update thresholds for position/rotation changes +- [ ] Update thresholds for position/rotation - [ ] Environmental entity flag for static objects -### Protocol (if touching network layer) -- [ ] Schema uses 1-3 char property keys with inline comments -- [ ] Co-located TypeScript type + JSON Schema with `JSONSchemaType` -- [ ] `additionalProperties: false` on schemas -- [ ] Packet validation at creation time (fail-fast) +### Protocol (network layer) +- [ ] 1-3 char property keys with inline comments +- [ ] Co-located type + JSON Schema with `JSONSchemaType` +- [ ] `additionalProperties: false` +- [ ] Validation at creation time (fail-fast) - [ ] Tuple wire format `[id, data, tick?]` -### Data-Driven Design -- [ ] No magic numbers — all numeric literals are named constants or config values -- [ ] Tunable values (thresholds, rates, capacities) are configurable via options, not hardcoded -- [ ] UI, logic, and data concerns are separated -- [ ] No new defaults changed without backwards compatibility assessment -- [ ] New features with tradeoffs are opt-in, not automatic - -### Backwards Compatibility -- [ ] No default values changed silently -- [ ] No public API signatures broken -- [ ] Existing games work without code changes on upgrade -- [ ] If breaking change is necessary, migration path documented -- [ ] Change scales to large games (100+ entities, multiple worlds) - -### PR Process -- [ ] PR description includes problem statement, testing done, AI tools used -- [ ] At least 1 fresh-context AI review completed -- [ ] Manual testing done on relevant platforms -- [ ] `npm run typecheck` and `npm run lint` pass -- [ ] Unit tests pass; new tests added for new behavior -- [ ] Existing game regression considered - -### General Quality +### Quality +- [ ] No magic numbers +- [ ] Tunable values configurable via options - [ ] No TODOs for error handling -- [ ] No stale comments referencing values that change +- [ ] No stale comments referencing changeable values - [ ] Configuration arrays are `readonly` -- [ ] External input validated before merging into internal state -- [ ] `async/await` used instead of `.then()` chains -- [ ] No data-driven boilerplate (3+ identical patterns → use mapping table) -- [ ] Doesn't add responsibilities to known God Classes +- [ ] External input validated before internal use +- [ ] `async/await` not `.then()` chains +- [ ] No boilerplate (3+ identical patterns → mapping table) --- ## 12. ESLint Configuration Reference -The server ESLint config (`server/eslint.config.js`) enforces: - ```javascript -// Key rules 'indent': ['error', 2, { SwitchCase: 1 }], 'quotes': ['error', 'single', { avoidEscape: true }], 'semi': ['error', 'always'], @@ -972,18 +738,16 @@ The server ESLint config (`server/eslint.config.js`) enforces: 'arrow-parens': ['error', 'as-needed'], 'newline-before-return': 'error', 'linebreak-style': ['error', 'unix'], -'@typescript-eslint/no-explicit-any': 'off', // any is allowed (legacy) +'@typescript-eslint/no-explicit-any': 'off', // legacy; standard prohibits in new exported types '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }], '@typescript-eslint/no-floating-promises': 'error', '@typescript-eslint/await-thenable': 'error', '@typescript-eslint/prefer-optional-chain': 'error', ``` -**Note**: While `no-explicit-any` is off in ESLint, the coding standard prohibits `any` in new exported types. The ESLint rule is kept off for legacy compatibility only. - --- -## Appendix A: Codebase Architecture Overview +## Appendix A: Architecture Overview ``` GameServer (root singleton) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..f4562f09 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,132 @@ +# Contributing to Hytopia + +For code style, naming conventions, and technical standards, see [CODING_STANDARDS.md](CODING_STANDARDS.md). + +--- + +## PR Requirements + +Every pull request must include: + +1. **Description** — What changed, why, and what it affects +2. **Test evidence** — How you verified it works (screenshots, test output, repro steps) +3. **Breaking change flag** — If defaults, public API signatures, or wire format changed, say so explicitly + +--- + +## Backwards Compatibility + +### Defaults Are Sacred + +Changing a default value is a breaking change. Existing games depend on current defaults without specifying them explicitly. + +```typescript +// A game using the SDK today: +const world = new World({ name: 'My World' }); +// This implicitly depends on: +// tickRate = 60 +// gravity = { x: 0, y: -32, z: 0 } +// ambientLightColor defaults +// particle alpha = 1.0 + +// If you change gravity default to -9.8, every existing game +// that doesn't explicitly set gravity will break silently. +``` + +**Rules:** +- Never change a default value without a deprecation path +- If a default must change, require explicit opt-in via options +- Document the migration in the PR description +- Consider adding a console warning for one release cycle + +### Public API Contract + +These are part of the API contract and must not change without a major version bump: +- Method signatures on exported classes +- Event string values (`'PLAYER.JOINED_WORLD'`) +- Options interface field names and their defaults +- Wire format packet structure +- Constructor parameter shapes + +--- + +## Review Process + +### Quality Gate Stack + +PRs pass through these layers in order. A failure at any layer blocks merge. + +| Layer | What | Automated? | +|-------|------|:----------:| +| 1. Type checks | `tsc --noEmit` catches type errors | Yes (CI) | +| 2. Lint | ESLint enforces style rules | Yes (CI) | +| 3. Unit tests | Verify isolated behavior | Yes (CI) | +| 4. Performance tests | No regressions in hot paths | Yes (CI) | +| 5. AI review | Fresh-context automated review against CODING_STANDARDS.md | Yes | +| 6. Human review | Maintainer reviews architecture, intent, edge cases | No | +| 7. Manual testing | Run affected systems, verify behavior | No | +| 8. Game regression | Test against existing games to catch silent breakage | No | + +### AI Review (Layer 5) + +Automated review runs with fresh context on every PR. The reviewer: +- Checks the diff against CODING_STANDARDS.md hard rules +- Flags backwards compatibility concerns +- Identifies missing error handling or cleanup +- Catches naming convention violations + +Fresh context is critical — the reviewer must not carry assumptions from previous reviews. + +### Human Review (Layer 6) + +Human reviewers focus on what automation cannot catch: +- Does the change make architectural sense? +- Are there edge cases the tests don't cover? +- Will this be maintainable in 6 months? +- Does the PR description accurately reflect the change? + +--- + +## Testing Expectations + +### Unit Tests + +New public methods should have corresponding tests. Tests verify behavior, not implementation: + +```typescript +// DO: Test behavior +it('rejects invalid packet format', () => { ... }); +it('emits JOINED_WORLD when player enters', () => { ... }); + +// DON'T: Test implementation details +it('calls _internalMethod three times', () => { ... }); +``` + +### Performance Tests + +Changes to hot paths (tick loops, serialization, network sync) must include before/after benchmarks. Key metrics: +- Tick processing time (ms per tick) +- Serialization throughput (entities per second) +- Memory allocation rate in hot paths (should be zero) + +### Game Regression + +Before merging changes that affect defaults, physics, networking, or entity behavior: +1. Run at least one existing game against the branch +2. Verify no visual or behavioral differences +3. Note any intentional changes in the PR description + +--- + +## Robustness Checklist + +Before submitting a PR, verify: + +- [ ] No new God Class additions (see CODING_STANDARDS.md section 10) +- [ ] Event listeners have matching cleanup +- [ ] Error paths use ErrorHandler, not raw throw +- [ ] No defaults were changed (or change is flagged as breaking) +- [ ] Public API types don't use `any` +- [ ] Hot path changes don't allocate (no `new` in tick loops) +- [ ] Options pattern used for configurable values +- [ ] Configuration arrays marked `readonly` From 89e91e6112bb0536a1a304963139e7821e1f177a Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 19:00:48 +1100 Subject: [PATCH 13/19] docs: update progress tracking Co-Authored-By: Claude Opus 4.6 --- .../analysis/code-standards-guidelines-44f2a42/progress.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md b/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md index 22b8e7f6..7b131276 100644 --- a/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md +++ b/ai-memory/analysis/code-standards-guidelines-44f2a42/progress.md @@ -7,3 +7,9 @@ - [x] Code smells analysis (code-smells.md) - [x] SOLID principles analysis (principles-analysis.md) - [x] Compile final CODING_STANDARDS.md from all 6 reports +- [x] Add mega-sappy guidelines (backwards compat, data-driven, review layers) +- [x] Remove game-specific content (damage/weapon examples) +- [x] Critical review and restructure into two documents +- [x] CODING_STANDARDS.md — code quality only (13 hard rules, clean numbering) +- [x] CONTRIBUTING.md — process/governance (PR reqs, review layers, backwards compat) +- [x] Update PR #13 description From 76338477d6131c112fe97eb49d4f23f8e3590ab8 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 19:02:43 +1100 Subject: [PATCH 14/19] =?UTF-8?q?docs:=20clarify=20God=20Class=20guidance?= =?UTF-8?q?=20=E2=80=94=20no=20new=20responsibilities,=20not=20untouchable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md index ae085019..906bce7e 100644 --- a/CODING_STANDARDS.md +++ b/CODING_STANDARDS.md @@ -646,7 +646,7 @@ Documented weaknesses. New code should not make them worse. Improvements are wel | SRP | 6/10 | Entity, NetworkSynchronizer overloaded | Don't add responsibilities to God Classes | | DRY | 6/10 | 60+ identical handlers, 70+ identical setters | Use mapping tables for 3+ identical patterns | -**God Classes — do not add to these files:** +**God Classes — do not add new responsibilities to these files:** | Class | Lines | Should extract | |-------|-------|----------------| @@ -698,7 +698,7 @@ Documented weaknesses. New code should not make them worse. Improvements are wel - [ ] No monkey-patching - [ ] Options object pattern for configuration - [ ] Composition over inheritance -- [ ] Doesn't add to known God Classes +- [ ] Doesn't add new responsibilities to known God Classes ### Performance (hot paths) - [ ] Pre-allocated working variables From c82738380c2b70c069bfeea6d59cfe0a63159e36 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 20:07:57 +1100 Subject: [PATCH 15/19] docs: add Code Clarity section with refactoring guidance Section 6 covers: extract method over comments, explanatory variables, parameter objects, guard clauses over nesting, and when comments are appropriate (why, not what). Checklist updated with Code Clarity category. Sections renumbered 6-13 to accommodate. Co-Authored-By: Claude Opus 4.6 --- CODING_STANDARDS.md | 148 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 130 insertions(+), 18 deletions(-) diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md index 906bce7e..f62b0535 100644 --- a/CODING_STANDARDS.md +++ b/CODING_STANDARDS.md @@ -400,9 +400,114 @@ public joinWorld(world: World) { --- -## 6. Architecture Patterns +## 6. Code Clarity -### 6.1 Singleton Pattern +Prefer code that explains itself over code that needs comments to explain it. + +### 6.1 Extract Method Over Comments + +If a comment explains *what* a block does, extract it into a method whose name says the same thing. + +```typescript +// DON'T: Comment as section header +public joinWorld(world: World) { + // Check if player is allowed to join + if (!world.isStarted || world.isFull() || !this._isAuthenticated) { + return; + } + + // Remove from current world + if (this._world) { + this._world.entityManager.unregister(this._entity); + this._world = undefined; + } + + // Add to new world + world.entityManager.register(this._entity); + this._world = world; +} + +// DO: Method names are the documentation +public joinWorld(world: World) { + if (!this._canJoinWorld(world)) return; + this._leaveCurrentWorld(); + this._enterWorld(world); +} +``` + +### 6.2 Explanatory Variables + +Break complex expressions into named intermediates. The variable name documents the intent. + +```typescript +// DON'T: Dense expression +if (entity.position.distanceToSquared(player.position) < RANGE_SQ + && entity.health > 0 && !entity.isEnvironmental) { + +// DO: Named conditions +const isInRange = entity.position.distanceToSquared(player.position) < RANGE_SQ; +const isAlive = entity.health > 0; +const isInteractable = !entity.isEnvironmental; +if (isInRange && isAlive && isInteractable) { +``` + +### 6.3 Introduce Parameter Object + +When 3+ parameters travel together, group them into an interface. + +```typescript +// DON'T +function spawnEntity(x: number, y: number, z: number, rx: number, ry: number, rz: number) { } + +// DO +function spawnEntity(options: EntitySpawnOptions) { } +``` + +This is already the codebase convention (see 7.4 Options Object Pattern) — apply it consistently. + +### 6.4 Replace Nested Conditionals with Guard Clauses + +```typescript +// DON'T: Nested logic +public processPacket(packet: Packet) { + if (packet) { + if (packet.isValid()) { + if (this._isConnected) { + // ... actual logic buried 3 levels deep + } + } + } +} + +// DO: Guard clauses, then logic +public processPacket(packet: Packet) { + if (!packet) return; + if (!packet.isValid()) return ErrorHandler.warning('...'); + if (!this._isConnected) return; + // ... actual logic at top level +} +``` + +### 6.5 When Comments Are Appropriate + +Comments explain *why*, never *what*. Good uses: + +```typescript +// Rapier requires bodies to be removed before the world steps +this._bodiesToRemove.forEach(body => this._rapierWorld.removeRigidBody(body)); + +// Wire format uses 1-char keys to minimize packet size +export type EntitySchema = { i: number; p?: VectorSchema; } + +// Intentionally no-op: base class hook for subclasses to override +public onTick(deltaMs: number): void { } +``` + +--- + +## 7. Architecture Patterns + +### 7.1 Singleton Pattern Two approved variants. Note: the codebase is singleton-heavy (DIP score: 4/10). New code should prefer constructor injection where practical. If you must use a singleton, define an interface for the dependency. @@ -422,7 +527,7 @@ public static readonly instance: PlayerManager = new PlayerManager(); private constructor() { ... } ``` -### 6.2 EventRouter Pattern +### 7.2 EventRouter Pattern Every significant class extends `EventRouter` and follows the event declaration template: @@ -445,7 +550,7 @@ export default class Xxx extends EventRouter { - `emitWithGlobal()` — local + global singleton (for cross-world events) - `emitWithWorld()` — local + world EventRouter (for NetworkSynchronizer to observe) -### 6.3 Controller/Strategy Pattern +### 7.3 Controller/Strategy Pattern Entity behavior is delegated via composition, not inheritance: @@ -460,7 +565,7 @@ class SwimmingEntity extends Entity { ... } Controllers extend `BaseEntityController` and implement lifecycle hooks: `attach()` → `spawn()` → `tick()` / `tickWithPlayerInput()` → `despawn()` → `detach()` -### 6.4 Options Object Pattern +### 7.4 Options Object Pattern Configuration via single options parameter with optional fields and defaults: @@ -479,7 +584,7 @@ constructor(options: WorldOptions) { } ``` -### 6.5 Composition Over Inheritance +### 7.5 Composition Over Inheritance ```typescript // DO: Composition @@ -491,7 +596,7 @@ this._simulation = new Simulation(this, options.tickRate, options.gravity); class World extends AudioCapable extends EntityCapable extends PhysicsCapable { } ``` -### 6.6 Protocol Schema Pattern +### 7.6 Protocol Schema Pattern Co-located TypeScript type + JSON Schema with extreme key minification: @@ -518,7 +623,7 @@ export const entitySchema: JSONSchemaType = { --- -## 7. Formatting Rules +## 8. Formatting Rules ### ESLint-Enforced @@ -557,11 +662,11 @@ export const entitySchema: JSONSchemaType = { --- -## 8. Performance Patterns +## 9. Performance Patterns New code in hot paths must follow these established patterns. -### 8.1 Pre-allocated Working Variables +### 9.1 Pre-allocated Working Variables ```typescript // DO: module-level working variables @@ -582,7 +687,7 @@ class Entity { } ``` -### 8.2 Manual Matrix Management +### 9.2 Manual Matrix Management ```typescript object.matrixAutoUpdate = false; @@ -592,7 +697,7 @@ object.updateMatrix(); object.updateMatrixWorld(); ``` -### 8.3 Packed Numeric Keys for Map Lookups +### 9.3 Packed Numeric Keys for Map Lookups ```typescript // DO @@ -602,7 +707,7 @@ const key = (x << 20) | (y << 10) | z; // packed numeric key const key = `${x},${y},${z}`; // string allocation every lookup ``` -### 8.4 Update Thresholds +### 9.4 Update Thresholds ```typescript const POSITION_THRESHOLD_SQ = 0.04 * 0.04; @@ -613,13 +718,13 @@ if (positionDeltaSq > POSITION_THRESHOLD_SQ) { } ``` -### 8.5 Environmental Entity Optimization +### 9.5 Environmental Entity Optimization Static world objects (`isEnvironmental = true`) skip per-tick updates entirely. Use this flag for non-interactive decoration. --- -## 9. Client vs Server Differences +## 10. Client vs Server Differences New code must follow the conventions of the layer it's in. @@ -634,7 +739,7 @@ New code must follow the conventions of the layer it's in. --- -## 10. Known Improvement Areas +## 11. Known Improvement Areas Documented weaknesses. New code should not make them worse. Improvements are welcome. @@ -657,7 +762,7 @@ Documented weaknesses. New code should not make them worse. Improvements are wel --- -## 11. Code Review Checklist +## 12. Code Review Checklist ### Naming & Style - [ ] Classes: PascalCase + approved role suffix @@ -693,6 +798,13 @@ Documented weaknesses. New code should not make them worse. Improvements are wel - [ ] No empty catch blocks - [ ] No `console.log` +### Code Clarity +- [ ] No comments explaining *what* — extract method instead +- [ ] Complex conditions use explanatory variables +- [ ] No nesting deeper than 2 levels — use guard clauses +- [ ] 3+ params that travel together → parameter object +- [ ] Comments explain *why*, not *what* + ### Architecture - [ ] Event listeners paired with cleanup - [ ] No monkey-patching @@ -726,7 +838,7 @@ Documented weaknesses. New code should not make them worse. Improvements are wel --- -## 12. ESLint Configuration Reference +## 13. ESLint Configuration Reference ```javascript 'indent': ['error', 2, { SwitchCase: 1 }], From e3aa5ef90c077d27210dc4ddb53bc5ca8a2a8f1f Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 20:30:19 +1100 Subject: [PATCH 16/19] docs: add Performance Impact section to CONTRIBUTING.md Covers server/desktop/mobile/high-player-count considerations, what to ask yourself before submitting runtime changes, and when before/after measurements are required in the PR. Checklist and PR requirements updated to include performance. Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f4562f09..f6ea1f77 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,6 +11,7 @@ Every pull request must include: 1. **Description** — What changed, why, and what it affects 2. **Test evidence** — How you verified it works (screenshots, test output, repro steps) 3. **Breaking change flag** — If defaults, public API signatures, or wire format changed, say so explicitly +4. **Performance impact** — For runtime code changes: what targets were tested, any before/after numbers --- @@ -50,6 +51,45 @@ These are part of the API contract and must not change without a major version b --- +## Performance Impact + +Every change should be considered from a performance perspective. Hytopia runs on both desktop and mobile, on both client and server — what's cheap on a desktop GPU can be a bottleneck on a mobile browser, and what's fine for one player can collapse at 50. + +### Think Across All Targets + +| Target | Constraints to consider | +|--------|------------------------| +| Server | Tick budget (~16ms at 60Hz), memory per-world, scales with player count | +| Desktop client | GPU draw calls, texture memory, physics step time | +| Mobile client | Thermal throttling, limited GPU/RAM, battery drain, smaller bandwidth | +| High player count | Per-player serialization cost, event fan-out, network packet size | + +### What to Ask Yourself + +Before submitting a PR that touches runtime code: + +- **Does this scale?** Will it still work with 50 players? 200 entities? What's the growth curve — linear, quadratic, constant? +- **Does this allocate?** Any `new`, string concatenation, or array creation in a per-tick or per-frame path adds GC pressure. Worse on mobile. +- **Does this add draw calls?** New visual elements, materials, or render passes affect mobile frame rate disproportionately. +- **Does this add network traffic?** Extra packets or larger payloads affect mobile users on limited connections. Check if the data can be delta-compressed or batched. +- **Does this affect startup time?** New asset loading, initialization, or validation that runs on connect/join impacts mobile users most. + +### When Performance Evidence Is Required + +If your change touches any of these, include before/after measurements in the PR: + +- Tick loop or frame loop code +- Serialization / deserialization +- Network packet handling +- Entity creation, destruction, or sync +- Asset loading or caching +- Physics simulation setup +- Anything called per-entity or per-player per-tick + +"It works on my machine" is not sufficient — consider the lowest-spec target. + +--- + ## Review Process ### Quality Gate Stack @@ -122,7 +162,7 @@ Before merging changes that affect defaults, physics, networking, or entity beha Before submitting a PR, verify: -- [ ] No new God Class additions (see CODING_STANDARDS.md section 10) +- [ ] No new God Class additions (see CODING_STANDARDS.md section 11) - [ ] Event listeners have matching cleanup - [ ] Error paths use ErrorHandler, not raw throw - [ ] No defaults were changed (or change is flagged as breaking) @@ -130,3 +170,6 @@ Before submitting a PR, verify: - [ ] Hot path changes don't allocate (no `new` in tick loops) - [ ] Options pattern used for configurable values - [ ] Configuration arrays marked `readonly` +- [ ] Considered performance on mobile, not just desktop +- [ ] Changes that scale with player/entity count have been stress-tested +- [ ] No unnecessary network traffic added (batching, delta compression considered) From 34ec1b12e807cf41bde48596c7414b8fe5042f64 Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 20:31:32 +1100 Subject: [PATCH 17/19] docs: add games-tested requirement to PRs Contributors should list which games they tested against (SDK examples, their own game, etc.) and on what targets. Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f6ea1f77..0c64020e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -10,8 +10,9 @@ Every pull request must include: 1. **Description** — What changed, why, and what it affects 2. **Test evidence** — How you verified it works (screenshots, test output, repro steps) -3. **Breaking change flag** — If defaults, public API signatures, or wire format changed, say so explicitly -4. **Performance impact** — For runtime code changes: what targets were tested, any before/after numbers +3. **Games tested** — Which games you tested against (SDK examples, your own game, etc.) and on what targets (desktop, mobile) +4. **Breaking change flag** — If defaults, public API signatures, or wire format changed, say so explicitly +5. **Performance impact** — For runtime code changes: what targets were tested, any before/after numbers --- @@ -152,9 +153,10 @@ Changes to hot paths (tick loops, serialization, network sync) must include befo ### Game Regression Before merging changes that affect defaults, physics, networking, or entity behavior: -1. Run at least one existing game against the branch +1. Run at least one existing game against the branch — SDK examples, your own game, or both 2. Verify no visual or behavioral differences -3. Note any intentional changes in the PR description +3. List which games you tested in the PR (e.g. "Tested with `examples/payload-game` and my own game") +4. Note any intentional changes in the PR description --- From 6e0986b62a32645255ef5b992584d3e7875cf5ff Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Sun, 8 Mar 2026 20:31:52 +1100 Subject: [PATCH 18/19] docs: expand games-tested to include device and repro steps PR should specify which games, which devices (desktop/mobile/OS), and what actions were taken in-game to exercise the change. Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0c64020e..d0a7c53a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -10,7 +10,7 @@ Every pull request must include: 1. **Description** — What changed, why, and what it affects 2. **Test evidence** — How you verified it works (screenshots, test output, repro steps) -3. **Games tested** — Which games you tested against (SDK examples, your own game, etc.) and on what targets (desktop, mobile) +3. **Games tested** — Which games (SDK examples, your own game, etc.), on what devices (desktop browser, mobile browser, specific OS), and what you did in-game to exercise the change 4. **Breaking change flag** — If defaults, public API signatures, or wire format changed, say so explicitly 5. **Performance impact** — For runtime code changes: what targets were tested, any before/after numbers @@ -155,8 +155,11 @@ Changes to hot paths (tick loops, serialization, network sync) must include befo Before merging changes that affect defaults, physics, networking, or entity behavior: 1. Run at least one existing game against the branch — SDK examples, your own game, or both 2. Verify no visual or behavioral differences -3. List which games you tested in the PR (e.g. "Tested with `examples/payload-game` and my own game") -4. Note any intentional changes in the PR description +3. In the PR, describe: + - **Which games** — e.g. `examples/payload-game`, your own game + - **Which devices** — e.g. Chrome desktop, Safari iOS, Android Chrome + - **What you did** — e.g. "spawned 20 entities, walked around, triggered physics collisions, tested on mobile with 3 players" +4. Note any intentional behavior changes in the PR description --- From a97f69c4f84c6e19b9f7c698387e369ae486093b Mon Sep 17 00:00:00 2001 From: web3dev1337 <160291380+web3dev1337@users.noreply.github.com> Date: Mon, 9 Mar 2026 08:41:09 +1100 Subject: [PATCH 19/19] docs: split AI review into general (bugs) and standards (style) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Layer 5 is now general AI review — bugs, logic errors, edge cases, merge readiness. Use multiple tools (Claude Code, Codex) for independent perspectives. Layer 6 is standards compliance against CODING_STANDARDS.md. Both require fresh context. Review stack renumbered to 9 layers. Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d0a7c53a..fbf13e39 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -103,22 +103,37 @@ PRs pass through these layers in order. A failure at any layer blocks merge. | 2. Lint | ESLint enforces style rules | Yes (CI) | | 3. Unit tests | Verify isolated behavior | Yes (CI) | | 4. Performance tests | No regressions in hot paths | Yes (CI) | -| 5. AI review | Fresh-context automated review against CODING_STANDARDS.md | Yes | -| 6. Human review | Maintainer reviews architecture, intent, edge cases | No | -| 7. Manual testing | Run affected systems, verify behavior | No | -| 8. Game regression | Test against existing games to catch silent breakage | No | +| 5. AI review (general) | Bug detection, logic errors, edge cases, merge readiness | Yes | +| 6. AI review (standards) | CODING_STANDARDS.md compliance check | Yes | +| 7. Human review | Maintainer reviews architecture, intent, edge cases | No | +| 8. Manual testing | Run affected systems, verify behavior | No | +| 9. Game regression | Test against existing games to catch silent breakage | No | -### AI Review (Layer 5) +### AI Review — General (Layer 5) -Automated review runs with fresh context on every PR. The reviewer: -- Checks the diff against CODING_STANDARDS.md hard rules -- Flags backwards compatibility concerns -- Identifies missing error handling or cleanup -- Catches naming convention violations +A general-purpose AI review with fresh context. Use multiple tools (e.g. Claude Code, Codex) for independent perspectives. The reviewer checks: +- Bugs, logic errors, off-by-one mistakes +- Edge cases and failure modes +- Missing validation or error handling +- Whether the change is actually ready to merge -Fresh context is critical — the reviewer must not carry assumptions from previous reviews. +Prompt should be open-ended: *"Review this PR for bugs, edge cases, and merge readiness"* — not limited to style. -### Human Review (Layer 6) +### AI Review — Standards (Layer 6) + +A separate, focused check against CODING_STANDARDS.md: +- Hard rule violations +- Naming convention mismatches +- Backwards compatibility concerns +- Missing cleanup or event listener pairing + +This is intentionally separate from the general review so style concerns don't crowd out bug detection. + +### Fresh Context for Both + +Each AI review must run with fresh context — no carry-over from previous reviews. This prevents the reviewer from developing blind spots about the codebase. + +### Human Review (Layer 7) Human reviewers focus on what automation cannot catch: - Does the change make architectural sense?