From a89321c7f18eac2077f5ab1b7e0c0e4b3f1108b1 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 21 Feb 2026 12:57:05 +1030 Subject: [PATCH 1/3] Add AGENTS.md, code review guidelines, and architecture docs - AGENTS.md: tool-agnostic AI coding agent configuration with project overview, build commands, and development principles - docs/Development/Guidelines.md: code review guidelines distilled from PR feedback (general principles, code style, architecture, network patterns, testing) - docs/Development/Architecture.md: GUI inheritance hierarchy, layer responsibilities, and network infrastructure reference - CONTRIBUTING.md: new sections pointing to guidelines and AGENTS.md - docs/_sidebar.md: new entries under Development Co-Authored-By: Claude Opus 4.6 --- AGENTS.md | 124 ++++++++++++++++ CONTRIBUTING.md | 10 ++ docs/Development/Architecture.md | 243 +++++++++++++++++++++++++++++++ docs/Development/Guidelines.md | 85 +++++++++++ docs/_sidebar.md | 3 + 5 files changed, 465 insertions(+) create mode 100644 AGENTS.md create mode 100644 docs/Development/Architecture.md create mode 100644 docs/Development/Guidelines.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..136af39a126 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,124 @@ +# Forge - AI Coding Agent Configuration + +## Project Overview + +Forge is an open-source, cross-platform Magic: The Gathering rules engine written in Java. Started in 2007, it provides a complete MTG game experience with AI opponents, multiple game modes, and network play support. + +**License:** GPL-3.0 | **Repository:** https://github.com/Card-Forge/forge + +Forge is maintained by a small group of part-time volunteers. Contributions should be well-documented, self-contained, and respectful of reviewer time: small PRs, minimal diffs, and clear descriptions. + +## Tech Stack + +- **Language:** Java 17+ | **Build:** Maven 3.8.1+ | **Testing:** TestNG +- **GUI:** Java Swing (desktop), Libgdx (mobile) +- **Libraries:** Guava, JGraphT, Sentry + +## Project Structure + +``` +forge/ +├── forge-core/ # Core engine: card definitions, utilities +├── forge-game/ # Rules engine: combat, mana, zones, phases +├── forge-ai/ # AI opponent: decision algorithms +├── forge-gui/ # Shared GUI and resources (platform-neutral) +├── forge-gui-desktop/ # Desktop GUI (Swing) +├── forge-gui-mobile/ # Mobile GUI (Libgdx) +├── forge-gui-android/ # Android backend (Libgdx) +├── forge-gui-ios/ # iOS backend (Libgdx) +└── docs/ # Project documentation + └── Development/ + ├── Guidelines.md # Code review guidelines + └── Architecture.md # GUI and network architecture reference +``` + +For detailed architecture documentation (GUI hierarchy, layer responsibilities, network infrastructure), see [docs/Development/Architecture.md](docs/Development/Architecture.md). + +**Card scripting:** Card definitions use a text-based scripting DSL and live in `forge-gui/res/cardsfolder/`. See the [Card Scripting API](https://github.com/Card-Forge/forge/wiki/Card-scripting-API) wiki for reference. + +## Before Making Changes + +You MUST read and follow these documents before writing or planning code: + +- **[docs/Development/Guidelines.md](docs/Development/Guidelines.md)** — Code review guidelines distilled from PR feedback. These are the most common cause of PR rejection when violated. Covers general principles, code style, architecture rules, network patterns, and testing requirements. +- **[docs/Development/Architecture.md](docs/Development/Architecture.md)** — GUI inheritance hierarchy, layer responsibilities, and network infrastructure. Read before modifying any GUI or network code. + +Do not treat these as optional reference material. Review the relevant sections before designing an approach, and verify your implementation against them before submitting. + +## Build & Test Commands + +```bash +# Incremental build (default for iterative development) +mvn -pl forge-gui -am install -DskipTests + +# Desktop build (for testing) +mvn -pl forge-gui-desktop -am install -DskipTests + +# Quick compile check (fastest, syntax checking only) +mvn -pl forge-gui -am compile + +# Compile a single module (fastest feedback loop) +mvn -pl forge-game compile + +# Run a single test (IMPORTANT: use verify, not test) +mvn -pl forge-gui-desktop -am verify -Dtest="TestClassName#methodName" -Dsurefire.failIfNoSpecifiedTests=false + +# Checkstyle (checks RedundantImport and UnusedImports only) +mvn checkstyle:check + +# Install dependencies (also happens implicitly during compile/install) +mvn dependency:resolve + +# Full distributable installer (Windows/Linux) +mvn -U -B clean -P windows-linux install -DskipTests + +# Full distributable installer (Mac) +mvn -U -B clean -P osx install -DskipTests +``` + +**Key notes:** +- Always use incremental builds (`-pl forge-gui -am`) for development iteration. Full installer builds are slow. +- Use `-pl ` to scope commands to a single module for faster feedback. +- Use `verify` (not `test`) when running individual tests. +- Desktop GUI tests require a display. CI uses Xvfb for headless testing. + +## CI Environment + +**Workflow:** `.github/workflows/test-build.yaml` runs on `ubuntu-latest` with Java 17 and 21 (Temurin), Xvfb virtual framebuffer, on every push and pull request. + +**CI command:** `mvn -U -B clean test` + +**What CI checks:** +- Unit tests (`testDeckLoaderHasPrecons`, `testGameResultInitialization`, `testConfigurationParsing`, etc.) +- Checkstyle: `RedundantImport` and `UnusedImports` only (see `checkstyle.xml`) + +**Note:** Fork PRs require maintainer approval before CI workflows run. This is a GitHub Actions security feature, not a test failure. + +## Development Principles + +These are the most critical principles. See [docs/Development/Guidelines.md](docs/Development/Guidelines.md) for the complete set. + +1. **Ask clarifying questions before starting work.** When a task has ambiguities in scope, approach, or trade-offs, surface options and let the maintainers decide before proceeding. +2. **Search for existing code before creating new functionality.** Before implementing, search the codebase for existing mechanisms that solve the same or similar problem. Enhance existing code rather than creating parallel implementations. +3. **Avoid over-engineering.** Solve the problem with the simplest approach that works. Don't introduce new classes, event types, or abstractions when existing infrastructure can be reused. Prefer modifying 3 files over creating 10 new ones. +4. **Prototype the minimal version.** Before building new infrastructure, ask: "Can this be done by composing existing methods with a few lines at the call site?" If yes, do that. +5. **Read code before modifying it.** Understand existing code before suggesting changes. Search for all callers of a method before changing its behavior. +6. **Trace execution contexts.** Before implementing, enumerate the runtime contexts where the affected code will execute: local single-player, local multiplayer, network host, network client, AI opponent. Verify the change is appropriate in each. +7. **Minimal diff.** Prefer small, focused changes. Do not make cosmetic fixes (whitespace, formatting, style) to code that isn't otherwise being changed for functional reasons. + +## Code Review Guidelines + +See [docs/Development/Guidelines.md](docs/Development/Guidelines.md) for the complete code review guidelines. These are distilled from PR reviewer feedback and cover general principles, code style, architecture rules, network-specific patterns, and testing requirements. Following them avoids the most common causes of PR rejection. + +## Git Conventions + +- Do not commit files listed in `.gitignore`. +- Write focused commits with clear, descriptive messages. +- Fork PRs require maintainer approval before CI workflows run. This is a GitHub Actions security feature, not a test failure. + +## Further Reading + +- **[docs/Development/Guidelines.md](docs/Development/Guidelines.md)** - Complete code review guidelines distilled from PR feedback +- **[docs/Development/Architecture.md](docs/Development/Architecture.md)** - GUI hierarchy, layer responsibilities, and network architecture +- **[CONTRIBUTING.md](CONTRIBUTING.md)** - Contributor setup guide (IDE, SDK, platform builds) +- **[Card Scripting API](https://github.com/Card-Forge/forge/wiki/Card-scripting-API)** - Card scripting reference diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fdc81309a1f..4c787fa610b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,6 +58,16 @@ Visit [this page](https://github.com/Card-Forge/forge/wiki/Card-scripting-API) f Card scripting resources are found in the forge-gui/res/ path. +## Code Review Guidelines & Architecture + +Before submitting a PR, review [docs/Development/Guidelines.md](docs/Development/Guidelines.md) — coding standards distilled from PR reviewer feedback. Following these guidelines helps avoid the most common causes of PR rejection. + +For GUI hierarchy, layer responsibilities, and network infrastructure, see [docs/Development/Architecture.md](docs/Development/Architecture.md). This is essential reading before modifying GUI or network code. + +## Using AI Coding Agents + +See [AGENTS.md](AGENTS.md) for AI-assisted development configuration. It contains project context, build commands, and cross-references to the guidelines and architecture docs. It is designed to work with any AI coding tool (Claude Code, Cursor, Windsurf, Copilot, Codex, etc.). + ## General Notes Art files need to be copyright-free and they should be in the public domain. diff --git a/docs/Development/Architecture.md b/docs/Development/Architecture.md new file mode 100644 index 00000000000..b9197b21ed6 --- /dev/null +++ b/docs/Development/Architecture.md @@ -0,0 +1,243 @@ +# Architecture reference + +Reference documentation for Forge architecture (currently GUI and network infrastructure only). + +For code review guidelines (general principles, code style, network patterns, testing), see [Guidelines.md](Guidelines.md). + +## Contents + +- [GUI Architecture](#gui-architecture) + - [Inheritance Hierarchy](#inheritance-hierarchy) + - [Layer Responsibilities](#layer-responsibilities) + - [Where Does My Code Go?](#where-does-my-code-go--decision-checklist) + - [Red Flags](#red-flags--signs-youre-in-the-wrong-layer) +- [Network Architecture](#network-architecture) + - [Server Lifecycle](#server-lifecycle) + - [GUI Creation and the guis Map](#gui-creation-and-the-guis-map) + - [Protocol Pipeline](#protocol-pipeline) + - [Client Side](#client-side) + +--- + +## GUI Architecture + +### Inheritance Hierarchy + +``` +IGuiGame (interface, forge-gui) + +-- AbstractGuiGame (abstract, forge-gui; also implements IMayViewCards) + |-- CMatchUI (forge-gui-desktop) -- Swing desktop implementation + |-- NetGuiGame (forge-gui) -- server-side network proxy + +-- MatchController (forge-gui-mobile) -- libgdx mobile implementation +``` + +### Layer Responsibilities + +##### `IGuiGame` — Interface Contract (forge-gui) +Defines the method signatures that any GUI implementation must provide. This is the +contract the game engine programs against. Changes here affect all platforms. No default +methods — every method must be implemented (or stubbed) by the concrete class. + +##### `AbstractGuiGame` — Shared Game-UI State (forge-gui) +Implements `IGuiGame` and `IMayViewCards`. Platform-agnostic state management and +convenience methods. Contains: +- Player tracking (current player, local players, game controllers) +- Game state flags (pause, speed, daytime) +- Card visibility rules (`mayView`, `mayFlip`) +- UI state tracking (highlighted cards, selectable cards) +- Auto-pass / auto-yield state management +- Await-next-input timer mechanism (`awaitNextInput`/`cancelAwaitNextInput`) +- Choice/input convenience wrappers (`one()`, `many()`, `getInteger()`, etc.) +- Concede/spectator logic +- No-op stubs for optional interface methods (`refreshField`, `refreshCardDetails`, etc.) +- No-op stubs for optional network methods overridden in subclasses + +**What does NOT belong here:** Anything that manages Swing/libgdx components, implements +platform-specific rendering, or uses platform-specific APIs. Simple text messages and +lightweight UI logic that is identical across all platforms *may* live here to avoid +duplication (prefer one implementation over two identical copies in subclasses). But if +the display differs per platform or uses platform APIs, it belongs in a subclass. + +##### `CMatchUI` — Desktop Match Screen (forge-gui-desktop) +The Swing-based desktop implementation. Extends `AbstractGuiGame`. This is where desktop-specific display logic, +Swing component management, and screen coordination belong. Implements `ICDoc` +(controller) and `IMenuProvider`. Owns references to desktop panel controllers (`CAntes`, +`CCombat`, `CDependencies`, `CDetailPicture`, `CDev`, `CDock`, `CLog`, `CPrompt`, +`CStack`). + +##### `MatchController` — Mobile Match Screen (forge-gui-mobile) +The libgdx-based mobile implementation. Extends `AbstractGuiGame`. Uses the singleton pattern +(`MatchController.instance`). Mobile-specific display and interaction logic belongs +here. + +##### `V*` Views (forge-gui-desktop: `forge.screens.match.views`) +Pure Swing UI components (`VField`, `VHand`, `VPrompt`, `VStack`, etc.). Each panel +view implements `IVDoc` and defines how a panel *looks* — layout, Swing components, +rendering. Views hold a reference to their corresponding controller. + +Note: `VMatchUI` is the top-level match screen view and implements `IVTopLevelUI` (not +`IVDoc`), so it follows a different pattern from the per-panel views. + +##### `C*` Controllers (forge-gui-desktop: `forge.screens.match.controllers`) +Per-panel controllers (`CField`, `CHand`, `CPrompt`, `CLog`, etc.). Each implements +`ICDoc` and manages the behavior of its corresponding `V*` view. Controllers hold a +reference to `CMatchUI` and their `V*` view. + +Exception: `CDetailPicture` is a composite controller that manages `CDetail` and +`CPicture` together. It does not itself implement `ICDoc`. + +### Where Does My Code Go? — Decision Checklist + +Before adding or modifying GUI code, work through this checklist top-to-bottom. +The first matching rule wins: + +1. **Does it define a new capability the game engine needs from the UI?** + Add the method signature to `IGuiGame`. Provide a concrete implementation in + `AbstractGuiGame` if the logic is shared across platforms, otherwise leave it + unimplemented there so each platform subclass (`CMatchUI`, `MatchController`) must + provide its own. + +2. **Is it shared game-UI state that both desktop and mobile need identically?** + (e.g., tracking which cards are selectable, auto-yield flags, player controller mappings) + `AbstractGuiGame`. + +3. **Is it a convenience wrapper that delegates to abstract methods?** + (e.g., `one()` calls `getChoices()`, `confirm()` calls overloaded `confirm()`) + `AbstractGuiGame` — this is the template method pattern already used there. + +4. **Does it involve network protocol or tracker synchronization?** + The network-aware subclasses `CMatchUI`/`NetGuiGame`, or `AbstractGuiGame` with + no-op stubs that subclasses override. + +5. **Does it use platform-specific APIs, or does the display differ between desktop + and mobile?** + `CMatchUI` (desktop) or `MatchController` (mobile). However, simple text messages + and lightweight UI logic that is identical across all platforms may live in + `AbstractGuiGame` to avoid duplication — see rule #2. + +6. **Does it coordinate multiple desktop panels or manage screen-level concerns?** + (e.g., targeting overlay, floating zones, keyboard shortcuts, menus) + `CMatchUI`. + +7. **Does it control the behavior of a specific desktop UI panel?** + The corresponding `C*` controller (e.g., `CPrompt`, `CField`, `CLog`). + +8. **Does it define how a desktop panel looks — layout, Swing components, rendering?** + The corresponding `V*` view (e.g., `VPrompt`, `VField`, `VLog`). + +### Red Flags — Signs You're in the Wrong Layer + +Some of these anti-patterns already exist in the codebase as technical debt. Do not add new instances of them. + +- **Adding `javax.swing.*` or `java.awt.*` imports to anything in `forge-gui/`.** + The `forge-gui` module is shared across platforms. Swing imports mean desktop-specific + code that belongs in `forge-gui-desktop`. + +- **Adding platform-specific display logic to `AbstractGuiGame`.** + Code that uses Swing/libgdx APIs or differs between desktop and mobile belongs in + `CMatchUI` or `MatchController`. Simple text messages identical across platforms are + acceptable in `AbstractGuiGame` to avoid duplication. + +- **Checking `GuiBase.getInterface().isLibgdxPort()` in `AbstractGuiGame` to branch + on platform.** + Platform-specific branches should be handled by overriding methods in the appropriate + subclass, not by runtime platform checks in the shared base. (The `isLibgdxPort()` + checks in `setCurrentPlayer()` and `mayView()` already violate this — do not extend + the pattern.) + +- **Putting game-state logic (auto-yield decisions, controller management) in a `V*` + view class.** + Views are for layout and rendering. State logic goes in the corresponding `C*` + controller or `CMatchUI`. + +- **Moving local-only logic into a shared layer without checking network and side-effect implications.** + When refactoring code from a subclass into a shared class (`AbstractGuiGame`), check two things: (1) whether the shared layer serializes state changes over the network — what was a harmless local update may become constant network traffic; (2) whether methods in the shared class have side effects that interfere with the new logic (e.g., a display update method that cancels the timer calling it). Trace the full call chain within the class. + +--- + +## Network Architecture + +This section covers the network play infrastructure. + +### Server Lifecycle + +`FServerManager` is the server-side singleton that manages the Netty server, client +connections, and the bridge between the game engine and remote clients. + +- **Startup:** `FServerManager.listen()` binds a Netty server socket. When a client + connects, `channelActive()` fires and a `RemoteClient` is created, keyed by `Channel` + in the `clients` map (`Map`). +- **Login:** The client sends a `LoginEvent` with username, avatar, and sleeve index. + `handleLogin()` calls `ServerGameLobby.connectPlayer()` which fills the first `OPEN` + slot, changing it to `REMOTE`. +- **RemoteClient:** Wraps a Netty `Channel` with the client's username and slot index. + The slot index is assigned at login and identifies the player for the lifetime of the + connection. `RemoteClient` implements `IToClient` for sending messages. + +### GUI Creation and the guis Map + +When a game starts, `GameLobby.startGame()` builds the **guis map** +(`Map`) — the authoritative mapping from player to GUI: + +- For each lobby slot, `ServerGameLobby.getGui(index)` delegates to + `FServerManager.getGui(index)`. +- `LOCAL` slots get a new `CMatchUI` (desktop) or `MatchController` (mobile). +- `REMOTE` slots get a `new NetGuiGame(client, index)` — finds the `RemoteClient` matching + the slot index. +- `AI` slots get no GUI entry. + +The guis map is stored in `HostedMatch.startMatch()` and never rebuilt during the game. +`HostedMatch.getGuiForPlayer(Player)` looks up by `player.getRegisteredPlayer()` — this +uses **identity equality** (Object default), so the `RegisteredPlayer` instance must be +the same object used as the key, not a copy. + +`NetGuiGame` is the server-side proxy: every `IGuiGame` method call is serialized via +`GameProtocolSender` and sent to the remote client over the Netty channel. It stores its +`slotIndex` for reliable identification (player names are unreliable — see the +stable identifiers guideline in [Guidelines.md](Guidelines.md)). + +### Protocol Pipeline + +Messages flow through the Netty pipeline as serialized Java objects: + +``` +Server (game thread) Client + | | + |-- NetGuiGame.send(method, args) | + | +-- GameProtocolSender | + | +-- RemoteClient.send() | + | +-- Channel.writeAndFlush() | + | -- [Netty wire] --> | + | GameProtocolHandler.channelRead() + | |-- beforeCall() [IO thread] + | +-- method.invoke() [EDT] +``` + +**Critical threading detail:** `GameProtocolHandler.channelRead()` calls `beforeCall()` +synchronously on the Netty IO thread, then dispatches the actual GUI method to the EDT +via `FThreads.invokeInEdtNowOrLater()`. This means `beforeCall()` for message N+1 can +execute before the EDT has processed message N's GUI method. Code in `beforeCall()` must +not read state that is set by a previous message's EDT-dispatched method — use fields set +within `beforeCall()` itself instead. + +Replies (for `sendAndWait` calls like `getChoices`, `confirm`) flow back through the +`ReplyPool`. The server blocks on `ReplyPool.get()` until the client responds. + +### Client Side + +- **`FGameClient`:** Connects to the server, sends `LoginEvent`, manages the Netty + channel. Holds references to the local `IGuiGame` and lobby listeners. +- **`GameClientHandler`:** Extends `GameProtocolHandler`. Its `beforeCall()` + builds local state that the client needs for display: on `openView`, it creates a local + `Match`, `Game`, and `Tracker` from lobby data. These local objects exist only so the + client's `CMatchUI` can function — the server remains the source of truth. +- **Lobby:** `ClientGameLobby` mirrors the server lobby state. The server sends + `LobbyUpdateEvent`s to keep it in sync. Slot data (names, decks, avatars) is used by + `GameClientHandler` to create `RegisteredPlayer` objects for the local Match. + +--- + +## Further Reading + +- **[AGENTS.md](../../AGENTS.md)** - AI coding agent configuration (project overview, build commands) +- **[Guidelines.md](Guidelines.md)** - Code review guidelines distilled from PR feedback diff --git a/docs/Development/Guidelines.md b/docs/Development/Guidelines.md new file mode 100644 index 00000000000..e5364380d3e --- /dev/null +++ b/docs/Development/Guidelines.md @@ -0,0 +1,85 @@ +# Code review guidelines + +These guidelines are distilled from PR review feedback. Following them avoids the most common causes of PR rejection. + +For architecture reference (GUI hierarchy, layer responsibilities, network infrastructure), see [Architecture.md](Architecture.md). + +## Contents + +- [General Principles](#general-principles) +- [Code Style](#code-style) +- [Architecture](#architecture) +- [Network-Specific Guidelines](#network-specific-guidelines) + - [Design](#design) + - [Implementation](#implementation) + - [Verification](#verification) +- [Testing](#testing) + +## General Principles + +- **Keep it simple:** Code should be simple, easy to follow, and use as few lines as possible while still achieving the desired functionality. +- **Minimal diff:** Prefer small, focused changes over large refactors. The fewer lines changed, the easier to review and less risk of introducing bugs. Do not make cosmetic fixes (whitespace, formatting, style) to code that isn't otherwise being changed for functional reasons — it creates diff noise and draws reviewer scrutiny to unrelated code. +- **Search before creating / avoid duplication:** Before implementing new functionality, search the codebase for existing mechanisms that solve the same or a similar problem. Before creating a new helper method, search the file for existing functions with equivalent logic. Before adding a new system (e.g., a timer, a polling loop, a status display), search broadly for existing mechanisms that already do the same thing. Enhance the existing mechanism rather than creating a parallel one. **Exception:** More specific guidelines (particularly in [Network-Specific Guidelines](#network-specific-guidelines)) may require intentional duplication — e.g., computing timers or derived state independently on each side of a client-server boundary to avoid network traffic. When a domain-specific principle conflicts with this general rule, the more specific principle wins. +- **Avoid over-engineering:** Solve the problem at hand with the simplest approach that works. Don't introduce new classes, event types, or abstractions when existing infrastructure can be reused. Prefer modifying 3 files over creating 10 new ones. If a feature can be built by composing existing mechanisms do that instead of building new framework. Don't create helper methods that are only called from one place — they add indirection without value and grow the code. Inline the logic at the call site instead. Similarly, don't introduce callback interfaces or delegate patterns when the caller can access the data directly (e.g., `matchUI.getGameView().getPlayers().size()` is better than adding a new `getPlayerCount()` to a callback interface). +- **Trace changes across execution contexts:** Before implementing, enumerate the runtime contexts where the affected code will execute: local single-player, local multiplayer, network host, network client, AI opponent. Verify the change is appropriate (or correctly no-op'd) in each. If a flag or property is set on one side of a client-server boundary, check whether it needs to be set on the other side too. +- **Trace callers before modifying:** Before changing a method's behavior, search for all call sites and understand the contexts they run in. A method called from one place is safe to change; a method called from network callbacks, UI threads, and game logic simultaneously needs careful consideration. For network code, trace the full path: who sends, what serializes it, what deserializes it, who receives. +- **Check for dead code:** When a change removes callers or replaces a mechanism, check whether any methods, fields, or branches become unreachable. Remove dead code in the same change rather than leaving it for later. +- **Don't write workarounds for existing bugs — flag them instead:** If your new code needs special handling because existing code doesn't match its declared types or contracts (e.g., a `Set` arriving where a `List` is declared), stop and flag the inconsistency rather than adding defensive code. The comment you'd write to explain the workaround is the sign you should be asking instead. +- **Preserve existing guards when changing types:** When refactoring code to use different types (e.g., `Player` to `PlayerView` in game events), don't silently remove conditional logic or performance guards that depended on the old type. If the new type lacks the data needed for a guard, carry the data forward — e.g., pre-compute it and store it as an additional field in the event or data structure. Dropping a guard because the data "isn't available anymore" is a regression, not a simplification. **However**, if the data is still reachable from view types available at the use-site, prefer querying it there rather than pre-computing it into the event — simpler records are better than records bloated with derived fields. + +## Code Style + +- **Check hotkey conflicts:** When assigning keyboard shortcuts, search for `VK_F[key]` and `getKeyStroke` in the codebase to ensure no conflicts with hardcoded menu accelerators (e.g., F1=Help, F11=Fullscreen). +- **Wrap parseInt/parseLong in try-catch:** System property parsing should handle `NumberFormatException` gracefully with fallback to defaults. +- **Add @Override annotations:** When implementing interface methods, always add `@Override` annotation. +- **Meaningful toString():** Classes used in logging/debugging should override `toString()` rather than inheriting from Object. +- **Intuitive naming:** Names of files, classes, and methods should communicate purpose without needing to read the implementation. Prefer `hasNetGame()` over `checkNet()`, `cancelAwaitNextInput()` over `resetTimer()`, `NetGuiGame` over `NetGG`. Names should describe *what* they answer or do, not *how* — a reader skimming a call site or class hierarchy should understand the intent immediately. +- **`this` in anonymous classes/lambdas:** Inside anonymous inner classes (e.g., `TimerTask`, `Runnable`) and lambdas nested within them, `this` refers to the anonymous class, not the enclosing class. Use `EnclosingClass.this` (e.g., `AbstractGuiGame.this`) when passing the outer instance. +- **Inline comments should be durable:** Comments must make sense to any contributor reading the code in isolation. Don't reference conversations, branch names, internal documents, or session-specific debugging labels (e.g., `// Path A`, `// this was the bug`). When giving examples in comments, keep them generic so they don't go stale. +- **Use semantic actions, not UI simulation:** When triggering game actions programmatically (e.g., from keyboard shortcuts or automated logic), call the semantic method (`passPriority()`, `concede()`) rather than simulating a UI click (`btnOK.doClick()`). UI simulation can trigger unintended side effects because the button's current meaning depends on context. +- **View-type overloads should delegate, not duplicate:** When both an engine-type overload (e.g., `updateZone(Player, ZoneType)`) and a view-type overload (e.g., `updateZone(PlayerView, ZoneType)`) exist, the engine-type one must delegate to the view-type one — not duplicate its logic. The view-type overload is the canonical implementation; the engine-type one is a convenience wrapper that converts and forwards. +- **Localize user-facing strings:** Use `Localizer.getInstance().getMessage()` for all user-facing text. Add new keys to `forge-gui/res/languages/en-US.properties`. Never hardcode English strings in Java code that will be displayed to the user. + +## Architecture + +- **Demand-driven computation:** Expensive operations (iterating all cards, getting all abilities) should only be performed when actually needed, not proactively or on every update cycle. Consider the performance cost of helper methods that might be called frequently (e.g., on every priority pass or network update). +- **Guard computation behind feature checks:** When a feature is gated behind a user preference or toggle, the associated computation must also be gated — not run unconditionally with results discarded. If a preference is OFF, no work should be done. This applies especially to expensive operations (iterating cards, computing heuristics) added to hot paths like `Player` or `PhaseHandler`. +- **Preference-gated features must preserve original behavior when OFF:** When a preference-gated feature modifies an existing method, gate the **entire modified flow** behind a single check at the top — not individual steps scattered throughout. When the preference is OFF, the method must execute exactly as it did before the feature was added: same loop bounds, same assignments, same code paths. A common mistake is changing the original logic (e.g., reducing a loop range, removing an assignment) and relying on the new feature code to compensate, then gating only the new code. This leaves the original flow broken when the feature is OFF. +- **Don't expand interfaces for trivial access:** Before adding a new method to an interface (especially `IGuiGame`), check whether the data is already reachable through existing object graphs. Adding an interface method forces all implementations to change and adds long-term maintenance cost. Only add interface methods when the caller genuinely cannot reach the data otherwise, or when the method represents a meaningful new capability. +- **Keep engine clean:** GUI-specific logic (UI hints, styling) belongs in View classes, not in forge-game engine classes like Player.java or PhaseHandler.java. +- **Fix bugs at the closest layer:** Errors and bug fixes should be solved in the closest layer that is effective. For example, a network serialization issue should be fixed in the network layer, not by adding guards in the game engine. A card rules bug belongs in forge-game, not worked around in forge-gui. Fixing at the source keeps the codebase clean and avoids defensive code proliferating through unrelated layers. +- **Platform-neutral code for platform-neutral features:** If a feature is intended to work across platforms (desktop and mobile), implement the *state and logic* in shared code (e.g., `AbstractGuiGame`, `forge-gui`) rather than in platform-specific classes. Display that uses platform-specific APIs (Swing components, libgdx widgets) belongs in platform subclasses (`CMatchUI`, `MatchController`). However, simple text messages and lightweight UI logic that is identical across platforms may live in `AbstractGuiGame` to avoid duplication — prefer one implementation in the base class over two identical copies in subclasses. **Code smell:** If you find yourself writing the same algorithm with the same state fields in both `CMatchUI` and `MatchController`, that's a strong signal it belongs in `AbstractGuiGame` instead. +- **Check for mobile GUI:** Desktop-only features must check `GuiBase.getInterface().isLibgdxPort()` and return early/disable for mobile. Users switching between desktop and mobile share preferences. +- **Zone transitions involve two players, not one:** When a card changes zones, the "from" zone and "to" zone may belong to different players (e.g., a stolen creature dying moves from the controller's battlefield to the owner's graveyard). Don't assume the card's owner or current controller is the correct player for both zones — use each zone's actual player reference. +- **Reuse `TrackableProperty` constants across view types:** Each `TrackableObject` instance has its own property map, so different view classes (e.g., `SpellAbilityView` and `StackItemView`) can share the same `TrackableProperty` enum constant when the semantics and type match. Don't create prefixed duplicates (e.g., `SA_Foo`) when an unprefixed constant (`Foo`) already exists with the same `TrackableType`. The enum name is just a key — it doesn't imply ownership by a particular view class. +- **Isolate network code:** Network-specific functionality should be in dedicated classes (`NetGuiGame`, `NetGameController`) rather than added to core classes like `AbstractGuiGame`. Keep core game classes free of network dependencies so they remain usable in non-network contexts. +- **Use `GuiBase.isNetworkplay(game)` for network detection:** There is only one signature — `isNetworkplay(IGuiGame game)`. When a game reference is available, pass it; the method delegates to `game.isNetGame()` for a per-instance answer. When no game is available, pass `null`; the method falls back to `IGuiBase.hasNetGame()` which iterates registered game instances. **Important:** `isNetGame()` must return `true` for *all* game instances in a network match — both the server-side proxy (`NetGuiGame`) and the host's local GUI (`CMatchUI`/`MatchController`). The host's local GUI gets its flag set via `FServerManager.getGui()` calling `setNetGame()`. If adding a new code path gated on `isNetGame()`, test it from the host's perspective, not just the remote client's. + +## Network-Specific Guidelines + +### Design + +- **Account for client-server asymmetry:** A network match has three execution contexts, not two: (1) the **host's local GUI** (`CMatchUI`) — sees full game state but displays locally; (2) the **server-side proxy** (`NetGuiGame`) — serializes `IGuiGame` calls over the wire to the remote client; (3) the **remote client** (`CMatchUI` receiving protocol calls) — has only a proxy view of game state. When branching on network status, verify the behavior is correct in all three contexts. The host's local GUI is the most commonly forgotten — it participates in the match alongside `NetGuiGame` but is a separate `IGuiGame` instance. +- **Design from the client's perspective first** — the client is the constrained side. Ask: "What does the client need to know, and how will it receive that information?" If a feature requires data the client doesn't have, the server must explicitly provide it (via protocol messages or lobby initialization). Don't assume that because something is reachable on the server, it's also reachable on the client. +- **Use stable identifiers for player lookup:** Display names are not stable identifiers — the game engine may transform them (e.g. deduplicating identical names). Use slot indices or GUI type for player identification in network code rather than matching on `Player.getName()`. +- **Distinguish events from continuous state:** When code runs during network play, ask: "Will this generate network traffic? Is that once, or on every tick?" One-time state transitions (e.g., "player is now waiting") should be sent as a single network event. Locally-derived continuous state (timers, counters, animations) must be computed on each side independently — never stream tick-by-tick updates over the wire. This intentional duplication overrides the general [Search before creating](#general-principles) principle: independent client-side and server-side implementations of the same logic is the correct pattern when sharing would create recurring network traffic. +- **Prefer forwarding game events over adding protocol methods:** When the client needs to react to a game occurrence (sound effects, log entries, UI animations), the preferred direction is to forward the `GameEvent` itself so the client can process it locally using its own `IGameEventVisitor` implementations, settings, and locale. This lets each client derive its own reactions rather than receiving pre-digested instructions from the host. Avoid adding new per-feature `ProtocolMethod` entries (e.g., `hearSoundEffect`, `showLogEntry`) that duplicate what event visitors already do on the host — each one adds to protocol maintenance burden. `GameEvent` records reference view types (`PlayerView`, `CardView`) and are network-serializable. Some protocol methods (e.g., `updateLives`) predate this pattern and duplicate what event visitors already handle. + +### Implementation + +- **Preserve message ordering:** The client's `GameClientHandler.beforeCall()` assumes messages arrive in the same sequence as during normal game start. When resending game state (e.g. during reconnection), follow the same message order as `HostedMatch.startGame()` — the client builds local objects (Match, Game, Tracker) based on what previous messages have established. Sending messages out of order causes the client to read uninitialized state and fail silently or throw exceptions. +- **Serialization compatibility:** Changes to objects serialized over the network (anything in `TrackableProperty`, lobby messages) must maintain backwards compatibility or include version-aware migration logic. +- **Thread safety:** Network callbacks execute on Netty threads, not the game thread. Access to shared state (e.g., `gameControllers`, `gameView`, tracker collections) from network callbacks must be synchronized or delegated to the game thread via `FThreads.invokeInEdtAndWait()` or equivalent. +- **Game event record design:** `GameEvent` records should have a single canonical constructor accepting view types (`PlayerView`, `CardView`, `SpellAbilityView`, etc.). Callers are responsible for converting engine types at the fire-site (e.g., `CardView.get(card)`, `PlayerView.get(player)`). Don't add convenience constructor overloads that accept engine types and auto-convert — they hide the conversion, complicate the record, and blur the boundary between engine and view layers. + +### Verification + +- **Test serialization of new objects:** When adding or modifying objects that may be transmitted over the network (game state, card data, player info), verify they are serializable. Netty will throw `NotSerializableException` at runtime for non-serializable objects — these are easy to miss in local testing but break network play immediately. Run at least one network game after changing data model classes. + +## Testing + +- **Headless CI compatibility:** Test classes must not depend on GUI components (`FOptionPane`, `JOptionPane`, etc.) that fail in headless CI environments. Use headless alternatives or skip GUI-dependent tests in CI. +- **Multi-process test isolation:** Tests spawning subprocesses must handle classpath/JAR discovery robustly across different environments (local dev vs CI). +- **Gate slow tests:** Stress and integration tests (batch tests, multiplayer tests) should be gated so they are skipped during CI's default `mvn clean test`. Only run them explicitly for local validation. +- **Always-run tests:** Unit tests (deck loader, game result, configuration parsing) must NOT be gated — they should always pass in CI without extra flags. +- **Manual network testing shares preferences:** When testing network play locally, both host and client instances read the same preferences file, so they have identical player names. The game engine then deduplicates these names (e.g. "2nd Player"), which breaks any code matching `Player.getName()` against the original login username. Use slot index or GUI type, not names — see [Use stable identifiers for player lookup](#design). diff --git a/docs/_sidebar.md b/docs/_sidebar.md index 8cd697a3a79..ec88d127bed 100644 --- a/docs/_sidebar.md +++ b/docs/_sidebar.md @@ -43,12 +43,15 @@ - [Tutorial: creating a custom card](Card-scripting-API/Creating-a-Custom-Card.md) - Development + - [Contributing](../CONTRIBUTING.md) - [IntelliJ Setup](Development/IntelliJ-setup/IntelliJ-setup.md) - [Snapshots & Releases](Development/Snapshots-and-Releases.md) - [Android Builds](Development/Android-Builds.md) - [Dev Mode](Development/DevMode.md) - [Ownership](Development/ownership.md) - [Docker Container](docker-setup.md) + - [Code Review Guidelines](Development/Guidelines.md) + - [Architecture Reference](Development/Architecture.md) - Customization & Themes - [Skins](Skins.md) From e32ac6e92dd0c45d11078e9e782e2837eee8410d Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Wed, 25 Feb 2026 06:36:19 +1030 Subject: [PATCH 2/3] Streamline Guidelines.md and add missing items Add missing items (check platform counterparts, null-guard contracts, only write meaningful tests, domain-specific section). Reduce verbosity throughout. Remove completed game event record design guideline. Co-Authored-By: Claude Opus 4.6 --- docs/Development/Guidelines.md | 40 +++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/docs/Development/Guidelines.md b/docs/Development/Guidelines.md index e5364380d3e..b4dd2122928 100644 --- a/docs/Development/Guidelines.md +++ b/docs/Development/Guidelines.md @@ -9,6 +9,7 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra - [General Principles](#general-principles) - [Code Style](#code-style) - [Architecture](#architecture) +- [Domain-Specific (MTG Rules)](#domain-specific-mtg-rules) - [Network-Specific Guidelines](#network-specific-guidelines) - [Design](#design) - [Implementation](#implementation) @@ -17,15 +18,14 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra ## General Principles -- **Keep it simple:** Code should be simple, easy to follow, and use as few lines as possible while still achieving the desired functionality. +- **Keep it simple / avoid over-engineering:** Simplest approach that works. Don't introduce new classes, abstractions, or helper methods called from one place when existing infrastructure suffices. Inline logic at the call site. Prefer modifying 3 files over creating 10. Don't introduce callback interfaces or delegate patterns when the caller can access the data directly (e.g., `matchUI.getGameView().getPlayers().size()` is better than adding a new `getPlayerCount()` to a callback interface). - **Minimal diff:** Prefer small, focused changes over large refactors. The fewer lines changed, the easier to review and less risk of introducing bugs. Do not make cosmetic fixes (whitespace, formatting, style) to code that isn't otherwise being changed for functional reasons — it creates diff noise and draws reviewer scrutiny to unrelated code. -- **Search before creating / avoid duplication:** Before implementing new functionality, search the codebase for existing mechanisms that solve the same or a similar problem. Before creating a new helper method, search the file for existing functions with equivalent logic. Before adding a new system (e.g., a timer, a polling loop, a status display), search broadly for existing mechanisms that already do the same thing. Enhance the existing mechanism rather than creating a parallel one. **Exception:** More specific guidelines (particularly in [Network-Specific Guidelines](#network-specific-guidelines)) may require intentional duplication — e.g., computing timers or derived state independently on each side of a client-server boundary to avoid network traffic. When a domain-specific principle conflicts with this general rule, the more specific principle wins. -- **Avoid over-engineering:** Solve the problem at hand with the simplest approach that works. Don't introduce new classes, event types, or abstractions when existing infrastructure can be reused. Prefer modifying 3 files over creating 10 new ones. If a feature can be built by composing existing mechanisms do that instead of building new framework. Don't create helper methods that are only called from one place — they add indirection without value and grow the code. Inline the logic at the call site instead. Similarly, don't introduce callback interfaces or delegate patterns when the caller can access the data directly (e.g., `matchUI.getGameView().getPlayers().size()` is better than adding a new `getPlayerCount()` to a callback interface). -- **Trace changes across execution contexts:** Before implementing, enumerate the runtime contexts where the affected code will execute: local single-player, local multiplayer, network host, network client, AI opponent. Verify the change is appropriate (or correctly no-op'd) in each. If a flag or property is set on one side of a client-server boundary, check whether it needs to be set on the other side too. +- **Search before creating / avoid duplication:** Before implementing new functionality, search the codebase for existing mechanisms that solve the same or a similar problem. Enhance what exists rather than creating parallel systems. **Exception:** More specific guidelines (particularly in [Network-Specific Guidelines](#network-specific-guidelines)) may require intentional duplication — e.g., computing timers or derived state independently on each side of a client-server boundary to avoid network traffic. +- **Trace execution contexts:** Enumerate where the code runs: local single-player, local multiplayer, network host, network client, AI. Verify correctness in each. - **Trace callers before modifying:** Before changing a method's behavior, search for all call sites and understand the contexts they run in. A method called from one place is safe to change; a method called from network callbacks, UI threads, and game logic simultaneously needs careful consideration. For network code, trace the full path: who sends, what serializes it, what deserializes it, who receives. -- **Check for dead code:** When a change removes callers or replaces a mechanism, check whether any methods, fields, or branches become unreachable. Remove dead code in the same change rather than leaving it for later. -- **Don't write workarounds for existing bugs — flag them instead:** If your new code needs special handling because existing code doesn't match its declared types or contracts (e.g., a `Set` arriving where a `List` is declared), stop and flag the inconsistency rather than adding defensive code. The comment you'd write to explain the workaround is the sign you should be asking instead. -- **Preserve existing guards when changing types:** When refactoring code to use different types (e.g., `Player` to `PlayerView` in game events), don't silently remove conditional logic or performance guards that depended on the old type. If the new type lacks the data needed for a guard, carry the data forward — e.g., pre-compute it and store it as an additional field in the event or data structure. Dropping a guard because the data "isn't available anymore" is a regression, not a simplification. **However**, if the data is still reachable from view types available at the use-site, prefer querying it there rather than pre-computing it into the event — simpler records are better than records bloated with derived fields. +- **Check for dead code:** Remove dead code *caused by your change* in the same commit — don't hunt pre-existing dead code. +- **Don't write workarounds — flag them:** If existing code doesn't match its contracts, flag the inconsistency rather than adding defensive code. +- **Preserve existing guards when changing types:** Don't silently remove guards when refactoring types. Query the data at the use-site if reachable; otherwise carry it forward as an additional field. ## Code Style @@ -39,38 +39,41 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra - **Use semantic actions, not UI simulation:** When triggering game actions programmatically (e.g., from keyboard shortcuts or automated logic), call the semantic method (`passPriority()`, `concede()`) rather than simulating a UI click (`btnOK.doClick()`). UI simulation can trigger unintended side effects because the button's current meaning depends on context. - **View-type overloads should delegate, not duplicate:** When both an engine-type overload (e.g., `updateZone(Player, ZoneType)`) and a view-type overload (e.g., `updateZone(PlayerView, ZoneType)`) exist, the engine-type one must delegate to the view-type one — not duplicate its logic. The view-type overload is the canonical implementation; the engine-type one is a convenience wrapper that converts and forwards. - **Localize user-facing strings:** Use `Localizer.getInstance().getMessage()` for all user-facing text. Add new keys to `forge-gui/res/languages/en-US.properties`. Never hardcode English strings in Java code that will be displayed to the user. +- **Null-guard changed contracts:** When tightening a condition, trace downstream callers that assumed non-null. ## Architecture -- **Demand-driven computation:** Expensive operations (iterating all cards, getting all abilities) should only be performed when actually needed, not proactively or on every update cycle. Consider the performance cost of helper methods that might be called frequently (e.g., on every priority pass or network update). -- **Guard computation behind feature checks:** When a feature is gated behind a user preference or toggle, the associated computation must also be gated — not run unconditionally with results discarded. If a preference is OFF, no work should be done. This applies especially to expensive operations (iterating cards, computing heuristics) added to hot paths like `Player` or `PhaseHandler`. -- **Preference-gated features must preserve original behavior when OFF:** When a preference-gated feature modifies an existing method, gate the **entire modified flow** behind a single check at the top — not individual steps scattered throughout. When the preference is OFF, the method must execute exactly as it did before the feature was added: same loop bounds, same assignments, same code paths. A common mistake is changing the original logic (e.g., reducing a loop range, removing an assignment) and relying on the new feature code to compensate, then gating only the new code. This leaves the original flow broken when the feature is OFF. -- **Don't expand interfaces for trivial access:** Before adding a new method to an interface (especially `IGuiGame`), check whether the data is already reachable through existing object graphs. Adding an interface method forces all implementations to change and adds long-term maintenance cost. Only add interface methods when the caller genuinely cannot reach the data otherwise, or when the method represents a meaningful new capability. +- **Gate expensive work behind checks:** Expensive operations (iterating all cards, getting all abilities) should only be performed when actually needed, not proactively or on every update cycle. If a feature is preference-gated, the computation must also be gated. +- **Preference-gated features:** Gate the **entire modified flow** behind a single check at the top. When OFF, the original code path must execute unchanged — same loop bounds, same assignments, same paths. Common mistake: changing the original logic (e.g., reducing a loop range) and relying on the new feature code to compensate, then gating only the new code — this leaves the original flow broken when OFF. +- **Don't expand interfaces for trivial access:** Check whether data is reachable through existing object graphs before adding methods to `IGuiGame` or similar interfaces. - **Keep engine clean:** GUI-specific logic (UI hints, styling) belongs in View classes, not in forge-game engine classes like Player.java or PhaseHandler.java. - **Fix bugs at the closest layer:** Errors and bug fixes should be solved in the closest layer that is effective. For example, a network serialization issue should be fixed in the network layer, not by adding guards in the game engine. A card rules bug belongs in forge-game, not worked around in forge-gui. Fixing at the source keeps the codebase clean and avoids defensive code proliferating through unrelated layers. +- **Check platform counterparts:** When fixing a bug in `CMatchUI` (desktop), check `MatchController` (mobile) for the same issue, and vice versa. - **Platform-neutral code for platform-neutral features:** If a feature is intended to work across platforms (desktop and mobile), implement the *state and logic* in shared code (e.g., `AbstractGuiGame`, `forge-gui`) rather than in platform-specific classes. Display that uses platform-specific APIs (Swing components, libgdx widgets) belongs in platform subclasses (`CMatchUI`, `MatchController`). However, simple text messages and lightweight UI logic that is identical across platforms may live in `AbstractGuiGame` to avoid duplication — prefer one implementation in the base class over two identical copies in subclasses. **Code smell:** If you find yourself writing the same algorithm with the same state fields in both `CMatchUI` and `MatchController`, that's a strong signal it belongs in `AbstractGuiGame` instead. - **Check for mobile GUI:** Desktop-only features must check `GuiBase.getInterface().isLibgdxPort()` and return early/disable for mobile. Users switching between desktop and mobile share preferences. -- **Zone transitions involve two players, not one:** When a card changes zones, the "from" zone and "to" zone may belong to different players (e.g., a stolen creature dying moves from the controller's battlefield to the owner's graveyard). Don't assume the card's owner or current controller is the correct player for both zones — use each zone's actual player reference. -- **Reuse `TrackableProperty` constants across view types:** Each `TrackableObject` instance has its own property map, so different view classes (e.g., `SpellAbilityView` and `StackItemView`) can share the same `TrackableProperty` enum constant when the semantics and type match. Don't create prefixed duplicates (e.g., `SA_Foo`) when an unprefixed constant (`Foo`) already exists with the same `TrackableType`. The enum name is just a key — it doesn't imply ownership by a particular view class. - **Isolate network code:** Network-specific functionality should be in dedicated classes (`NetGuiGame`, `NetGameController`) rather than added to core classes like `AbstractGuiGame`. Keep core game classes free of network dependencies so they remain usable in non-network contexts. - **Use `GuiBase.isNetworkplay(game)` for network detection:** There is only one signature — `isNetworkplay(IGuiGame game)`. When a game reference is available, pass it; the method delegates to `game.isNetGame()` for a per-instance answer. When no game is available, pass `null`; the method falls back to `IGuiBase.hasNetGame()` which iterates registered game instances. **Important:** `isNetGame()` must return `true` for *all* game instances in a network match — both the server-side proxy (`NetGuiGame`) and the host's local GUI (`CMatchUI`/`MatchController`). The host's local GUI gets its flag set via `FServerManager.getGui()` calling `setNetGame()`. If adding a new code path gated on `isNetGame()`, test it from the host's perspective, not just the remote client's. +## Domain-Specific (MTG Rules) + +- **Zone transitions involve two players, not one:** When a card changes zones, the "from" zone and "to" zone may belong to different players (e.g., a stolen creature dying moves from the controller's battlefield to the owner's graveyard). Don't assume the card's owner or current controller is the correct player for both zones — use each zone's actual player reference. +- **Reuse `TrackableProperty` constants across view types:** Each `TrackableObject` instance has its own property map, so different view classes (e.g., `SpellAbilityView` and `StackItemView`) can share the same `TrackableProperty` enum constant when the semantics and type match. Don't create prefixed duplicates (e.g., `SA_Foo`) when an unprefixed constant (`Foo`) already exists with the same `TrackableType`. The enum name is just a key — it doesn't imply ownership by a particular view class. + ## Network-Specific Guidelines ### Design - **Account for client-server asymmetry:** A network match has three execution contexts, not two: (1) the **host's local GUI** (`CMatchUI`) — sees full game state but displays locally; (2) the **server-side proxy** (`NetGuiGame`) — serializes `IGuiGame` calls over the wire to the remote client; (3) the **remote client** (`CMatchUI` receiving protocol calls) — has only a proxy view of game state. When branching on network status, verify the behavior is correct in all three contexts. The host's local GUI is the most commonly forgotten — it participates in the match alongside `NetGuiGame` but is a separate `IGuiGame` instance. - **Design from the client's perspective first** — the client is the constrained side. Ask: "What does the client need to know, and how will it receive that information?" If a feature requires data the client doesn't have, the server must explicitly provide it (via protocol messages or lobby initialization). Don't assume that because something is reachable on the server, it's also reachable on the client. -- **Use stable identifiers for player lookup:** Display names are not stable identifiers — the game engine may transform them (e.g. deduplicating identical names). Use slot indices or GUI type for player identification in network code rather than matching on `Player.getName()`. -- **Distinguish events from continuous state:** When code runs during network play, ask: "Will this generate network traffic? Is that once, or on every tick?" One-time state transitions (e.g., "player is now waiting") should be sent as a single network event. Locally-derived continuous state (timers, counters, animations) must be computed on each side independently — never stream tick-by-tick updates over the wire. This intentional duplication overrides the general [Search before creating](#general-principles) principle: independent client-side and server-side implementations of the same logic is the correct pattern when sharing would create recurring network traffic. -- **Prefer forwarding game events over adding protocol methods:** When the client needs to react to a game occurrence (sound effects, log entries, UI animations), the preferred direction is to forward the `GameEvent` itself so the client can process it locally using its own `IGameEventVisitor` implementations, settings, and locale. This lets each client derive its own reactions rather than receiving pre-digested instructions from the host. Avoid adding new per-feature `ProtocolMethod` entries (e.g., `hearSoundEffect`, `showLogEntry`) that duplicate what event visitors already do on the host — each one adds to protocol maintenance burden. `GameEvent` records reference view types (`PlayerView`, `CardView`) and are network-serializable. Some protocol methods (e.g., `updateLives`) predate this pattern and duplicate what event visitors already handle. +- **Use stable identifiers for player lookup:** Slot indices or GUI type, not `Player.getName()` (names get deduplicated). +- **Distinguish events from continuous state:** One-time transitions → single network event. Continuous state (timers, animations) → compute independently on each side. Never stream tick-by-tick updates. This intentional duplication overrides [Search before creating](#general-principles) — independent client/server implementations is correct when sharing would create recurring network traffic. +- **Prefer forwarding game events over adding protocol methods:** Forward `GameEvent` records for client-side processing via `IGameEventVisitor`. Avoid new per-feature `ProtocolMethod` entries. ### Implementation -- **Preserve message ordering:** The client's `GameClientHandler.beforeCall()` assumes messages arrive in the same sequence as during normal game start. When resending game state (e.g. during reconnection), follow the same message order as `HostedMatch.startGame()` — the client builds local objects (Match, Game, Tracker) based on what previous messages have established. Sending messages out of order causes the client to read uninitialized state and fail silently or throw exceptions. +- **Preserve message ordering:** Follow the same message order as `HostedMatch.startGame()`. Out-of-order messages cause silent failures. - **Serialization compatibility:** Changes to objects serialized over the network (anything in `TrackableProperty`, lobby messages) must maintain backwards compatibility or include version-aware migration logic. - **Thread safety:** Network callbacks execute on Netty threads, not the game thread. Access to shared state (e.g., `gameControllers`, `gameView`, tracker collections) from network callbacks must be synchronized or delegated to the game thread via `FThreads.invokeInEdtAndWait()` or equivalent. -- **Game event record design:** `GameEvent` records should have a single canonical constructor accepting view types (`PlayerView`, `CardView`, `SpellAbilityView`, etc.). Callers are responsible for converting engine types at the fire-site (e.g., `CardView.get(card)`, `PlayerView.get(player)`). Don't add convenience constructor overloads that accept engine types and auto-convert — they hide the conversion, complicate the record, and blur the boundary between engine and view layers. ### Verification @@ -78,6 +81,7 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra ## Testing +- **Only write tests that catch real problems:** No wiring tests. Tests should catch integration issues, regressions, or non-obvious edge cases. - **Headless CI compatibility:** Test classes must not depend on GUI components (`FOptionPane`, `JOptionPane`, etc.) that fail in headless CI environments. Use headless alternatives or skip GUI-dependent tests in CI. - **Multi-process test isolation:** Tests spawning subprocesses must handle classpath/JAR discovery robustly across different environments (local dev vs CI). - **Gate slow tests:** Stress and integration tests (batch tests, multiplayer tests) should be gated so they are skipped during CI's default `mvn clean test`. Only run them explicitly for local validation. From 6f00295a96fc7b6606ef854b6febb5593f55e8d5 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Wed, 25 Feb 2026 07:10:02 +1030 Subject: [PATCH 3/3] Address PR feedback: clarify scope, mobile UI, and review comments - Clarify Architecture.md covers in-match GUI only - Describe mobile UI as a full LibGDX implementation with feature parity - Reconcile isLibgdxPort guideline with Architecture red flags - Move hotkey conflicts to end of Code Style, add default prefs check - Add AI disclosure requirement to CONTRIBUTING.md - Remove multi-process test isolation and manual network testing bullets Co-Authored-By: Claude Opus 4.6 --- CONTRIBUTING.md | 4 +++- docs/Development/Architecture.md | 12 ++++++++---- docs/Development/Guidelines.md | 22 +++++++--------------- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4c787fa610b..33c64ed251e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -66,7 +66,9 @@ For GUI hierarchy, layer responsibilities, and network infrastructure, see [docs ## Using AI Coding Agents -See [AGENTS.md](AGENTS.md) for AI-assisted development configuration. It contains project context, build commands, and cross-references to the guidelines and architecture docs. It is designed to work with any AI coding tool (Claude Code, Cursor, Windsurf, Copilot, Codex, etc.). +See [AGENTS.md](AGENTS.md) for AI-assisted development configuration. It contains project context, build commands, and cross-references to the guidelines and architecture docs. It is designed to work with any AI coding tool (Claude Code, Cursor, Windsurf, Copilot, Codex, etc.). + +Substantial contributions made with the assistance of AI must be disclosed. Include the AI tool as a co-author on commits and/or expressly note AI involvement in the PR description. ## General Notes diff --git a/docs/Development/Architecture.md b/docs/Development/Architecture.md index b9197b21ed6..721c93a0b3d 100644 --- a/docs/Development/Architecture.md +++ b/docs/Development/Architecture.md @@ -21,6 +21,8 @@ For code review guidelines (general principles, code style, network patterns, te ## GUI Architecture +This section covers the **in-match GUI** — the classes responsible for displaying and controlling an active game. It does not cover the full application UI (menus, lobby screens, deck editors, etc.). + ### Inheritance Hierarchy ``` @@ -65,10 +67,12 @@ Swing component management, and screen coordination belong. Implements `ICDoc` `CCombat`, `CDependencies`, `CDetailPicture`, `CDev`, `CDock`, `CLog`, `CPrompt`, `CStack`). -##### `MatchController` — Mobile Match Screen (forge-gui-mobile) -The libgdx-based mobile implementation. Extends `AbstractGuiGame`. Uses the singleton pattern -(`MatchController.instance`). Mobile-specific display and interaction logic belongs -here. +##### `MatchController` — LibGDX Match Screen (forge-gui-mobile) +The libgdx-based implementation. Extends `AbstractGuiGame`. Uses the singleton pattern +(`MatchController.instance`). Despite the module name, this is not just a mobile port — it +is a fully featured LibGDX implementation that also runs on desktop (via `forge-gui-mobile-dev`) +and aims for feature parity with the Swing UI. It is also the exclusive home of Adventure Mode +and Planar Conquest. LibGDX-specific display and interaction logic belongs here. ##### `V*` Views (forge-gui-desktop: `forge.screens.match.views`) Pure Swing UI components (`VField`, `VHand`, `VPrompt`, `VStack`, etc.). Each panel diff --git a/docs/Development/Guidelines.md b/docs/Development/Guidelines.md index b4dd2122928..b66004f8b67 100644 --- a/docs/Development/Guidelines.md +++ b/docs/Development/Guidelines.md @@ -29,9 +29,8 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra ## Code Style -- **Check hotkey conflicts:** When assigning keyboard shortcuts, search for `VK_F[key]` and `getKeyStroke` in the codebase to ensure no conflicts with hardcoded menu accelerators (e.g., F1=Help, F11=Fullscreen). -- **Wrap parseInt/parseLong in try-catch:** System property parsing should handle `NumberFormatException` gracefully with fallback to defaults. - **Add @Override annotations:** When implementing interface methods, always add `@Override` annotation. +- **Wrap parseInt/parseLong in try-catch:** System property parsing should handle `NumberFormatException` gracefully with fallback to defaults. - **Meaningful toString():** Classes used in logging/debugging should override `toString()` rather than inheriting from Object. - **Intuitive naming:** Names of files, classes, and methods should communicate purpose without needing to read the implementation. Prefer `hasNetGame()` over `checkNet()`, `cancelAwaitNextInput()` over `resetTimer()`, `NetGuiGame` over `NetGG`. Names should describe *what* they answer or do, not *how* — a reader skimming a call site or class hierarchy should understand the intent immediately. - **`this` in anonymous classes/lambdas:** Inside anonymous inner classes (e.g., `TimerTask`, `Runnable`) and lambdas nested within them, `this` refers to the anonymous class, not the enclosing class. Use `EnclosingClass.this` (e.g., `AbstractGuiGame.this`) when passing the outer instance. @@ -40,6 +39,7 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra - **View-type overloads should delegate, not duplicate:** When both an engine-type overload (e.g., `updateZone(Player, ZoneType)`) and a view-type overload (e.g., `updateZone(PlayerView, ZoneType)`) exist, the engine-type one must delegate to the view-type one — not duplicate its logic. The view-type overload is the canonical implementation; the engine-type one is a convenience wrapper that converts and forwards. - **Localize user-facing strings:** Use `Localizer.getInstance().getMessage()` for all user-facing text. Add new keys to `forge-gui/res/languages/en-US.properties`. Never hardcode English strings in Java code that will be displayed to the user. - **Null-guard changed contracts:** When tightening a condition, trace downstream callers that assumed non-null. +- **Check hotkey conflicts:** When assigning keyboard shortcuts, search for `VK_F[key]` and `getKeyStroke` in the codebase to ensure no conflicts with hardcoded menu accelerators (e.g., F1=Help, F11=Fullscreen), and check default shortcut preferences in `ForgePreferences` for collisions. ## Architecture @@ -50,12 +50,7 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra - **Fix bugs at the closest layer:** Errors and bug fixes should be solved in the closest layer that is effective. For example, a network serialization issue should be fixed in the network layer, not by adding guards in the game engine. A card rules bug belongs in forge-game, not worked around in forge-gui. Fixing at the source keeps the codebase clean and avoids defensive code proliferating through unrelated layers. - **Check platform counterparts:** When fixing a bug in `CMatchUI` (desktop), check `MatchController` (mobile) for the same issue, and vice versa. - **Platform-neutral code for platform-neutral features:** If a feature is intended to work across platforms (desktop and mobile), implement the *state and logic* in shared code (e.g., `AbstractGuiGame`, `forge-gui`) rather than in platform-specific classes. Display that uses platform-specific APIs (Swing components, libgdx widgets) belongs in platform subclasses (`CMatchUI`, `MatchController`). However, simple text messages and lightweight UI logic that is identical across platforms may live in `AbstractGuiGame` to avoid duplication — prefer one implementation in the base class over two identical copies in subclasses. **Code smell:** If you find yourself writing the same algorithm with the same state fields in both `CMatchUI` and `MatchController`, that's a strong signal it belongs in `AbstractGuiGame` instead. -- **Check for mobile GUI:** Desktop-only features must check `GuiBase.getInterface().isLibgdxPort()` and return early/disable for mobile. Users switching between desktop and mobile share preferences. -- **Isolate network code:** Network-specific functionality should be in dedicated classes (`NetGuiGame`, `NetGameController`) rather than added to core classes like `AbstractGuiGame`. Keep core game classes free of network dependencies so they remain usable in non-network contexts. -- **Use `GuiBase.isNetworkplay(game)` for network detection:** There is only one signature — `isNetworkplay(IGuiGame game)`. When a game reference is available, pass it; the method delegates to `game.isNetGame()` for a per-instance answer. When no game is available, pass `null`; the method falls back to `IGuiBase.hasNetGame()` which iterates registered game instances. **Important:** `isNetGame()` must return `true` for *all* game instances in a network match — both the server-side proxy (`NetGuiGame`) and the host's local GUI (`CMatchUI`/`MatchController`). The host's local GUI gets its flag set via `FServerManager.getGui()` calling `setNetGame()`. If adding a new code path gated on `isNetGame()`, test it from the host's perspective, not just the remote client's. - -## Domain-Specific (MTG Rules) - +- **Check for mobile GUI:** Desktop-only features in platform-neutral code (e.g., `PlayerControllerHuman`, event handlers) should check `GuiBase.getInterface().isLibgdxPort()` and return early for mobile. However, in `AbstractGuiGame` and its subclasses, prefer subclass overrides over runtime platform checks — see [Architecture Red Flags](Architecture.md#red-flags--signs-youre-in-the-wrong-layer). - **Zone transitions involve two players, not one:** When a card changes zones, the "from" zone and "to" zone may belong to different players (e.g., a stolen creature dying moves from the controller's battlefield to the owner's graveyard). Don't assume the card's owner or current controller is the correct player for both zones — use each zone's actual player reference. - **Reuse `TrackableProperty` constants across view types:** Each `TrackableObject` instance has its own property map, so different view classes (e.g., `SpellAbilityView` and `StackItemView`) can share the same `TrackableProperty` enum constant when the semantics and type match. Don't create prefixed duplicates (e.g., `SA_Foo`) when an unprefixed constant (`Foo`) already exists with the same `TrackableType`. The enum name is just a key — it doesn't imply ownership by a particular view class. @@ -63,6 +58,7 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra ### Design +- **Isolate network code:** Network-specific functionality should be in dedicated classes (`NetGuiGame`, `NetGameController`) rather than added to core classes like `AbstractGuiGame`. Keep core game classes free of network dependencies so they remain usable in non-network contexts. - **Account for client-server asymmetry:** A network match has three execution contexts, not two: (1) the **host's local GUI** (`CMatchUI`) — sees full game state but displays locally; (2) the **server-side proxy** (`NetGuiGame`) — serializes `IGuiGame` calls over the wire to the remote client; (3) the **remote client** (`CMatchUI` receiving protocol calls) — has only a proxy view of game state. When branching on network status, verify the behavior is correct in all three contexts. The host's local GUI is the most commonly forgotten — it participates in the match alongside `NetGuiGame` but is a separate `IGuiGame` instance. - **Design from the client's perspective first** — the client is the constrained side. Ask: "What does the client need to know, and how will it receive that information?" If a feature requires data the client doesn't have, the server must explicitly provide it (via protocol messages or lobby initialization). Don't assume that because something is reachable on the server, it's also reachable on the client. - **Use stable identifiers for player lookup:** Slot indices or GUI type, not `Player.getName()` (names get deduplicated). @@ -74,16 +70,12 @@ For architecture reference (GUI hierarchy, layer responsibilities, network infra - **Preserve message ordering:** Follow the same message order as `HostedMatch.startGame()`. Out-of-order messages cause silent failures. - **Serialization compatibility:** Changes to objects serialized over the network (anything in `TrackableProperty`, lobby messages) must maintain backwards compatibility or include version-aware migration logic. - **Thread safety:** Network callbacks execute on Netty threads, not the game thread. Access to shared state (e.g., `gameControllers`, `gameView`, tracker collections) from network callbacks must be synchronized or delegated to the game thread via `FThreads.invokeInEdtAndWait()` or equivalent. - -### Verification - +- **Use `GuiBase.isNetworkplay(game)` for network detection:** There is only one signature — `isNetworkplay(IGuiGame game)`. When a game reference is available, pass it; the method delegates to `game.isNetGame()` for a per-instance answer. When no game is available, pass `null`; the method falls back to `IGuiBase.hasNetGame()` which iterates registered game instances. **Important:** `isNetGame()` must return `true` for *all* game instances in a network match — both the server-side proxy (`NetGuiGame`) and the host's local GUI (`CMatchUI`/`MatchController`). The host's local GUI gets its flag set via `FServerManager.getGui()` calling `setNetGame()`. If adding a new code path gated on `isNetGame()`, test it from the host's perspective, not just the remote client's. - **Test serialization of new objects:** When adding or modifying objects that may be transmitted over the network (game state, card data, player info), verify they are serializable. Netty will throw `NotSerializableException` at runtime for non-serializable objects — these are easy to miss in local testing but break network play immediately. Run at least one network game after changing data model classes. ## Testing - **Only write tests that catch real problems:** No wiring tests. Tests should catch integration issues, regressions, or non-obvious edge cases. - **Headless CI compatibility:** Test classes must not depend on GUI components (`FOptionPane`, `JOptionPane`, etc.) that fail in headless CI environments. Use headless alternatives or skip GUI-dependent tests in CI. -- **Multi-process test isolation:** Tests spawning subprocesses must handle classpath/JAR discovery robustly across different environments (local dev vs CI). -- **Gate slow tests:** Stress and integration tests (batch tests, multiplayer tests) should be gated so they are skipped during CI's default `mvn clean test`. Only run them explicitly for local validation. -- **Always-run tests:** Unit tests (deck loader, game result, configuration parsing) must NOT be gated — they should always pass in CI without extra flags. -- **Manual network testing shares preferences:** When testing network play locally, both host and client instances read the same preferences file, so they have identical player names. The game engine then deduplicates these names (e.g. "2nd Player"), which breaks any code matching `Player.getName()` against the original login username. Use slot index or GUI type, not names — see [Use stable identifiers for player lookup](#design). +- **Gate slow tests:** Stress and integration tests should be gated so they are skipped during CI's default `mvn clean test`. Only run them explicitly for local validation. +- **Always-run tests:** Unit tests (deck loader, game result, configuration parsing) must NOT be gated — they should always pass in CI without extra flags. \ No newline at end of file