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..33c64ed251e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,6 +58,18 @@ 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.). + +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 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..721c93a0b3d --- /dev/null +++ b/docs/Development/Architecture.md @@ -0,0 +1,247 @@ +# 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 + +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 + +``` +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` — 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 +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..b66004f8b67 --- /dev/null +++ b/docs/Development/Guidelines.md @@ -0,0 +1,81 @@ +# 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) +- [Domain-Specific (MTG Rules)](#domain-specific-mtg-rules) +- [Network-Specific Guidelines](#network-specific-guidelines) + - [Design](#design) + - [Implementation](#implementation) + - [Verification](#verification) +- [Testing](#testing) + +## General Principles + +- **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. 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:** 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 + +- **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. +- **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. +- **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 + +- **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 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. + +## Network-Specific Guidelines + +### 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). +- **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:** 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. +- **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. +- **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 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)