From dd113af7861fa4055f0057ce7246becd8c636949 Mon Sep 17 00:00:00 2001 From: Gabriel Kaszewski Date: Tue, 28 Jul 2026 18:11:01 +0200 Subject: [PATCH] docs: progression CONTEXT, ADRs 0001-0004, phase A plan ignore .idea/ .superpowers/ bin/ obj/; drop 3 identical csproj backups --- .gitignore | 8 + CONTEXT.md | 91 ++++++ docs/adr/0001-three-progression-scopes.md | 15 + docs/adr/0002-attempt-is-a-ledger.md | 16 ++ docs/adr/0003-only-first-clear-commits.md | 15 + .../0004-domain-does-not-reference-godot.md | 21 ++ docs/plans/phase-a-progression-domain.md | 261 ++++++++++++++++++ 7 files changed, 427 insertions(+) create mode 100644 CONTEXT.md create mode 100644 docs/adr/0001-three-progression-scopes.md create mode 100644 docs/adr/0002-attempt-is-a-ledger.md create mode 100644 docs/adr/0003-only-first-clear-commits.md create mode 100644 docs/adr/0004-domain-does-not-reference-godot.md create mode 100644 docs/plans/phase-a-progression-domain.md diff --git a/.gitignore b/.gitignore index bb2a7a0..dcc3c46 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,11 @@ /android/ builds/ .worktrees/ + +# IDE / local tooling +.idea/ +.superpowers/ + +# .NET build output +bin/ +obj/ diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..2568377 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,91 @@ +# Mr. Brick Adventures + +A 2D platformer. This context covers **player state and progression** — what the player has, what they have done, +and at what point each of those becomes permanent. Gameplay behaviour (movement, combat, hazards) lives in the +composable components and is deliberately outside this context. + +## Language + +### Scopes + +Three lifetimes. Every fact in this context belongs to exactly one. + +**Attempt**: +One try at a level, from entering it to either completing it or losing a life. Holds a ledger of everything that +happened during the try — coins found, coins spent, skills bought. Discarded whole if the player dies. +_Avoid_: Session, run, life + +**Playthrough**: +One game from New Game until game over, identified by a `PlaythroughId`. Holds lives, banked coins, owned skills +and their levels, campaign position, and which levels it has cleared. Several exist at once and the player picks +between them. +_Avoid_: Save, save slot, progress, game, run + +**Profile**: +Everything shared across all Playthroughs, one per installation. Achievements, best time per Level, and lifetime +statistics. Never reset by starting a new Playthrough. +_Avoid_: Global state, meta, persistent data, account + +### Operations + +**Commit**: +Applying an Attempt's ledger to the Playthrough. Only a First Clear commits — nothing else, ever. +_Avoid_: Save, persist, flush, sync + +**First Clear**: +Completing a Level this Playthrough has not cleared before. The only event that commits an Attempt and advances +campaign position. +_Avoid_: Level complete, finish (both ambiguous — they also describe a Replay) + +**Replay**: +Completing a Level this Playthrough has already cleared. Records Best Time, achievements and statistics to the +Profile; commits nothing and never advances campaign position. +_Avoid_: Practice, retry, second run + +**Bank**: +The coins a Playthrough holds, as opposed to coins found in the current Attempt. Coins are banked by committing, +never on pickup. +_Avoid_: Total coins, saved coins + +**Discard**: +Throwing away an Attempt's ledger — on death, or at the end of a Replay. Purchases made during the Attempt are +undone by discarding, not refunded; under the ledger they never happened. +_Avoid_: Reset, rollback, revert + +### Things + +**Level**: +One playable stage, identified by a stable `LevelId` — never by position in a list. Knows its scene and display +name. Ids must survive inserting and reordering levels, because Playthroughs and Best Times both reference them. +_Avoid_: Stage, scene, level index + +**Cleared Levels**: +The Levels a Playthrough has completed at least once. Gates both campaign progression and level select; whether a +Level is reachable is computed from this, never stored alongside it. +_Avoid_: Unlocked levels, completed levels, available levels + +**Skill**: +An ability the player can own and upgrade. `SkillData` is the authored asset describing one (name, icon, upgrade +table) and is immutable at runtime; the level the player has reached in it belongs to the Playthrough. +_Avoid_: Ability, power-up, upgrade (for the skill itself) + +**Movement Ability**: +A composable behaviour attached to the player controller (jump, wall-jump, gravity). Unrelated to Skill — +authored per movement preset, not owned, not upgraded, not persisted. +_Avoid_: Skill, movement skill + +**Best Time**: +The fastest recorded completion of a Level, from a First Clear or a Replay alike. Belongs to the Profile, so it +survives New Game and is shared by every Playthrough. Stored alongside, but distinct from, the ghost replay +frames that produced it. +_Avoid_: Personal best, high score, record + +## Rules + +- Reads may cross scopes freely — affordability spans Attempt and Playthrough. Writes must not. +- Derived facts are computed, never stored. Level reachability is derived from Cleared Levels; total coins from + Attempt plus Bank. Storing either alongside its source creates the second source of truth this context exists + to prevent. +- Achievements are mirrored to Steam, which is per-account and cannot be un-earned. That is why they are Profile + and not Playthrough: a per-Playthrough achievement list would contradict Steam permanently. +- Nothing in this context may reference Godot. Its correctness must be checkable without a scene tree. diff --git a/docs/adr/0001-three-progression-scopes.md b/docs/adr/0001-three-progression-scopes.md new file mode 100644 index 0000000..da228c9 --- /dev/null +++ b/docs/adr/0001-three-progression-scopes.md @@ -0,0 +1,15 @@ +# Player progression has three scopes, not two + +Progression state was two scopes — `PlayerState` (persisted) and `SessionState` (transient) — with achievements +and best times leaking out as special cases: `PlayerState.Reset()` deliberately skipped `UnlockedAchievements`, +and best times lived in the ghost replay files rather than the save. We split progression into **Attempt**, +**Playthrough** and **Profile** (defined in `CONTEXT.md`), because introducing save slots makes the third scope +mandatory rather than a preference: Steam achievements are per-account and cannot be un-earned, so a +per-Playthrough achievement list would permanently contradict Steam the moment a second slot exists. Best times +follow the same argument — a completionist's record is not something starting a new game should reset. + +## Considered options + +Keeping one persisted root with a hand-maintained list of fields that survive New Game. Rejected: that is exactly +what the old `Reset()` did, and it had already drifted — achievements survived by omission rather than by design, +and nothing would have caught the next missed field. diff --git a/docs/adr/0002-attempt-is-a-ledger.md b/docs/adr/0002-attempt-is-a-ledger.md new file mode 100644 index 0000000..f4ba486 --- /dev/null +++ b/docs/adr/0002-attempt-is-a-ledger.md @@ -0,0 +1,16 @@ +# Attempt is a separate aggregate holding a ledger + +**Playthrough** and **Attempt** are separate aggregate roots rather than one root containing the other, and an +Attempt records a *ledger* — coins found, coins spent, skills bought — rather than just a purse of coins found. +Separate roots keeps each aggregate small and gives the Attempt its own lifecycle, since it is never persisted. +The ledger exists so that the rule "nothing survives an Attempt unless you complete the level" applies to +purchases too: under the previous model `RemoveCoins` drained session coins and then debited banked coins +immediately, so buying a skill mid-level and then dying lost both the coins and the skill. + +## Consequences + +- Affordability is a cross-aggregate **read** (`attempt.CoinsFound + playthrough.Bank >= cost`). Reads may cross + scopes; writes may not. +- Purchasing writes only the Attempt. The only operation touching both roots is Commit, which has exactly one + call site. +- Discarding an Attempt undoes purchases rather than refunding them — under the ledger they never happened. diff --git a/docs/adr/0003-only-first-clear-commits.md b/docs/adr/0003-only-first-clear-commits.md new file mode 100644 index 0000000..ac109c7 --- /dev/null +++ b/docs/adr/0003-only-first-clear-commits.md @@ -0,0 +1,15 @@ +# Only a First Clear commits an Attempt + +The game is moving from a purely sequential campaign to one where completionists can select and replay levels +they have already beaten. Coins and skills bank only on a **First Clear** — the first completion of a Level +within a Playthrough — while a **Replay** records Best Time, achievements and statistics to the Profile and +discards the Attempt. We chose this over committing on every completion because grinding the easiest level would +otherwise be the optimal way to buy every skill. + +## Consequences + +- A Replay must never advance campaign position. Sharing one code path between First Clear and Replay without an + explicit branch is the obvious way to reintroduce this bug. +- `ClearedLevels` answers "is this a First Clear?" for free, since it already deduplicates. +- Coins found during a Replay are lost on completion, not just on death. This is intentional and worth surfacing + in the UI. diff --git a/docs/adr/0004-domain-does-not-reference-godot.md b/docs/adr/0004-domain-does-not-reference-godot.md new file mode 100644 index 0000000..243ede0 --- /dev/null +++ b/docs/adr/0004-domain-does-not-reference-godot.md @@ -0,0 +1,21 @@ +# The progression domain does not reference Godot + +The progression domain lives in its own assembly with no Godot reference. Aggregates raise plain C# event records +as part of their operations, and a single adapter republishes those as `EventBus` signals. This is the only +enforceable version of the goal — correctness checkable without a scene tree, with the compiler enforcing the rule +rather than discipline. The leak it removes was already visible: `PlayerState` documented itself as a POCO "for +predictable state management" while holding `List`, and `SkillData : Resource`, which is why saves had +to store skill *names* and re-resolve them through `SkillManager` on load. + +Republishing rather than replacing `EventBus` keeps existing scene wiring and subscribers intact, and makes +"state changed ⇒ signal fired" un-bypassable — the original defect, where `ConsoleManager` mutated state directly +and re-emitted signals by hand, sometimes without the invariants the store's own operations applied. + +## Consequences + +- `GameStateStore` stops being a `Node`. The autoload becomes a composition root that owns the aggregates, so the + aggregates' lifetime is no longer tied to the scene tree and most `Store?.` null-guards disappear. +- Skill progression cannot live on `SkillData`. A `Resource` is one cached instance per process, so with save + slots it would leak levels between Playthroughs. +- Godot types (`Vector2`, `Mathf`, `GD.Print`) are not available in the domain; where they appear today they are + replaced by domain values or moved to adapters. diff --git a/docs/plans/phase-a-progression-domain.md b/docs/plans/phase-a-progression-domain.md new file mode 100644 index 0000000..cc22b03 --- /dev/null +++ b/docs/plans/phase-a-progression-domain.md @@ -0,0 +1,261 @@ +# Phase A — Progression domain + +Replace the current state layer with the model in [`CONTEXT.md`](../../CONTEXT.md), governed by +[ADR-0001..0004](../adr/). **No new player-facing features.** Save files are wiped once; the game is unreleased so +this costs nothing (ADR context). + +Phase B — slot picker UI, level select, Replay surfacing, best times out of the ghost files — is out of scope +here. Phase A does put the *persistence format* in place for slots, so Phase B is UI work rather than another +model change. + +## Two kinds of slice + +| | verified by | `/tdd` | +|---|---|---| +| **Slices 1–6** — pure domain, no Godot | `dotnet test` | yes — red/green/refactor throughout | +| **Slices 7–13** — adapters and wiring | compiler + playing the game | no — write the test where one is possible, otherwise verify by hand | + +Don't try to TDD slices 7–13. You can't red-green an autoload swap or a `.tscn` connection deletion, and forcing +it produces tests that assert the mock rather than the behaviour. The value of slices 1–6 is that they absorb +*all* the logic worth testing, so the untestable slices become mechanical. + +Each slice ends with the build green and the game runnable. Don't start the next one until it is. + +--- + +## Slice 0 — scaffolding + +No tests. Pure setup. + +- `progression/Mr.BrickAdventures.Progression.csproj` — plain `net8.0` library, **no Godot reference**, root + namespace `Mr.BrickAdventures.Progression`. +- `progression.tests/Mr.BrickAdventures.Progression.Tests.csproj` — xunit (NUnit is equally fine), references the + domain library **only**. +- Add both to `Mr. Brick Adventures.sln`. Add a `ProjectReference` from the game project to the domain library. +- Set `true` in the domain library too, not just the game project — + Godot needs it on referenced assemblies for hot reload, and omitting it produces confusing load failures. + +**Done when:** `dotnet test` runs and reports zero tests; Godot still builds; the game still launches and plays. + +**Guard rail for the whole phase:** `grep -r "using Godot" progression/` must stay empty. That single check is +what ADR-0004 buys you. + +--- + +## Slice 1 — value objects · TDD + +`Coins`, `Lives`, `LevelId`, `SkillId`, `SkillLevel`, `StatName`, `LevelTime`, `PlaythroughId`. + +Invariants to drive out with tests: + +- `Coins` cannot be negative. Subtraction that would go negative is a failure the caller must handle, not a + silent clamp — the current `Mathf.Max(0, ...)` clamp in one path and absence of it in another is exactly the + bug this removes. +- `Lives` floors at zero, and reaching zero is observable. +- `LevelId` is a non-empty stable string. Not an int, not an index. +- `SkillLevel` starts at 1 and has no zero value — "unowned" is represented by *absence*, never by level 0. See + the `SkillData.Level = 1` ambiguity this replaces. +- `LevelTime` is positive; comparison is what "best" means. +- `StatName` is a closed set (the five in `StatNames` today), so a typo cannot invent a statistic. + +**Done when:** no value object can hold invalid state, and every clamp/guard currently scattered across +`GameStateStore` and `ConsoleManager` exists in exactly one place. + +--- + +## Slice 2 — Attempt · TDD + +The ledger from ADR-0002: `CoinsFound`, `CoinsSpent`, `SkillsBought`. + +- `RecordCoinsFound(Coins)` +- `RecordPurchase(SkillId, Coins cost)` +- `NetCoins` — may be negative, when purchases drew against the Bank +- No `Discard` method: discarding is throwing the object away. + +Tests worth writing: + +- Spending more than found is representable — the shortfall is settled at Commit, not at purchase time. +- Net coins after found 10, spent 30 is −20. +- Nothing on Attempt persists or mutates anything else. + +--- + +## Slice 3 — Playthrough · TDD + +Aggregate root. `PlaythroughId`, `Lives`, `Bank`, `OwnedSkills : SkillId → SkillLevel`, `CurrentLevel : LevelId`, +`ClearedLevels`. + +Operations: `LoseLife`, `Commit(Attempt)`, `CanAfford(Attempt, Coins)`, `HasCleared(LevelId)`, +`ReachableLevels(order)`, `IsRunOver`. + +Raises: `LifeLost(remaining)`, `RunEnded`, `BankChanged(total)`, `SkillSetChanged`. + +This is where the report's bugs become tests. Write these first, watch them fail: + +- **Losing the last life sets lives to zero *and* raises `RunEnded` in one operation.** This is the last-life + soft-lock — today two screens read `Lives` before the handler writes it. Make it one atomic operation and the + race cannot be expressed. +- **`Commit` on an already-cleared level does not advance and does not bank** (ADR-0003). +- **`Commit` applies `NetCoins`**, so an Attempt that overspent reduces the Bank correctly, and a discarded + Attempt leaves the Bank untouched — the "buy mid-level, die, lose coins *and* skill" bug. +- **`ReachableLevels` is computed** from `ClearedLevels` plus level order, never stored. `UnlockedLevels` and + `IsLevelUnlocked` are deleted, not ported. +- **`Bank` cannot go negative** even through Commit. + +--- + +## Slice 4 — Profile · TDD + +Aggregate root, one per installation. `Achievements`, `BestTimes : LevelId → LevelTime`, `Statistics`. + +- `RecordTime(LevelId, LevelTime)` — only replaces a slower time. Recording a worse time is a no-op, not an error. +- `UnlockAchievement(id)` — idempotent, raises `AchievementUnlocked` only on the transition. +- `Record(StatName, int)`. +- `RelockAchievement(id)` exists for the console only, and is documented as **not** reaching Steam — that + asymmetry is unavoidable (Steam achievements cannot be un-earned) so make it explicit rather than surprising. + +--- + +## Slice 5 — ports and in-memory adapters · TDD + +Interfaces in the domain: `IPlaythroughStore`, `IProfileStore`, `ILevelCatalogue`, `IGhostStore`. + +In-memory implementations live in the **test project**. That's what makes the seam real rather than hypothetical — +two adapters, one of which ships and one of which doesn't. + +- Round-trip: save a Playthrough, load it, assert equality. Same for Profile. +- Loading an absent Playthrough and loading a *corrupt* one are different outcomes. Today both surface as + `false` from `LoadGame`, which is why a version bump silently overwrites saves. +- `ILevelCatalogue` returns ordered `LevelId`s and resolves one to a scene reference — as an opaque handle, since + the domain can't name `PackedScene`. + +--- + +## Slice 6 — use cases · TDD + +`StartPlaythrough`, `LoadPlaythrough`, `EnterLevel`, `FinishLevel`, `HandlePlayerDeath`, `PurchaseSkill`, +`UpgradeSkill`. + +Four of these carry the orchestration that is currently spread across `GameManager`, the `Events/` handlers and +the UI. Each of the remaining report bugs gets a test here: + +- **`EnterLevel` always announces `LevelStarted`.** Today only `TryToGoToNextLevel` emits it, so ghost recording + never starts on the first level or after Continue, and a continued run records completion times of zero. +- **`FinishLevel` branches First Clear vs Replay** and a Replay never advances `CurrentLevel` (ADR-0003 flags this + as the obvious regression). +- **`HandlePlayerDeath` loses a life and discards the Attempt as one operation.** +- **`PurchaseSkill` writes only the Attempt**, checks affordability across Attempt + Bank, and rejects rather than + throwing when short. +- **`UpgradeSkill` reads the level from the Playthrough**, and cannot index past the upgrade table. + +**Done when:** every bug listed in the architecture review has a test that failed before the fix and passes after. +That is the deliverable of Phase A — the point where "hidden bugs" stops being a category. + +--- + +> Everything above is pure C# with no Godot. Everything below touches the engine. Switch verification mode here. + +--- + +## Slice 7 — level catalogue adapter + +`resources/levels/` already holds six `LevelResource` files (`village_1..5`, `forest_1`) but there are eight level +scenes and only two scenes reference a resource at all. Complete and adopt them. + +- Add `LevelId` and explicit order to `LevelResource`; create the two missing entries (`tutorial_1`, + `village_6`). +- A catalogue resource lists them in order. Adapter implements `ILevelCatalogue`. +- Delete `GameManager.LevelScenes` and all array-index addressing. +- `LevelResource.ScenePath` is currently never read — either it becomes how the catalogue resolves scenes, or it + goes. Don't leave it. + +**Verify:** game boots into the first level; order matches the catalogue; `DeathScreen`'s level-name label still +populates. + +## Slice 8 — JSON store adapters + +- `user://profile.json` and `user://playthrough_{id}.json`. The slot format lands now even though the UI only + ever creates one; Phase B then adds a picker and nothing about the format changes. +- Write to a temp file and rename, so a crash mid-save can't corrupt a slot. With N slots this stops being + theoretical. +- Keep a version field, but replace the equality check with a migration hook that can return + "unmigratable" — distinct from "absent". + +**Verify:** play, quit, continue. Then hand-corrupt a file and confirm you get an error rather than a silent new +game. + +## Slice 9 — composition root and republisher + +- New autoload owns the `Profile` and the active `Playthrough`. It is the only `Node` in this area. +- Republisher subscribes to domain events and emits the existing `EventBus` signals (ADR-0004), so `Hud`, + `DeathScreen` and the rest keep working unchanged. +- Delete `GameStateStore`. Register the new autoload in `project.godot` in its place. + +**Verify:** HUD shows coins and lives; a coin pickup updates them. + +## Slice 10 — swap the call sites + +Mechanical and compiler-driven — sealing the aggregates turns every old write into a compile error, which is the +cheapest way to find them all. + +- `Events/` handlers become thin adapters that call use cases. `CoinStateHandler` already has this shape. +- `ConsoleManager` calls use cases instead of writing fields and hand-re-emitting signals. +- `AchievementManager` reads and writes the `Profile` instead of reaching into `PlayerState`. +- `SkillManager` reconciles `ActiveComponents` from `SkillSetChanged` — `ApplyUnlockedSkills` becomes the only + path, so the four two-call pairs collapse to one call each and a skill can no longer end up owned but inert. +- Delete `SaveSystem`, `PlayerState`, `SessionState`, `SaveDataDto`. + +**Verify:** compiler first, then play one level start to finish including a purchase. + +## Slice 11 — skill level off the asset + +- `ISkill.Initialize(Node owner, SkillData data, int level)`. Update `BrickThrowComponent:90`, + `MagneticSkillComponent:97`, `BrickArmorSkillComponent:25` to use the passed level rather than `Data.Level`. + `BrickArmorSkillComponent` indexes `Upgrades[Level - 1]` unguarded — fix while you're there. +- Delete `SkillData.Level`. `SkillData` becomes immutable authored data (CONTEXT.md). +- `MarketplaceButton` and `Marketplace` read the level from the Playthrough. + +**Verify:** buy a skill, upgrade it, quit, continue — level persists. Game over, New Game — level is gone. Both +are currently wrong. + +**Landmine to note:** five skills (`brick_armor`, `brick_shield`, `double_jump`, `ground_pound_skill`, +`wall_jump`) have no `upgrades/` entries, so `Upgrades.Count == 0`. `TryUnlockSkill` would throw on +`Upgrades[0].Cost`; it's currently unreachable only because `MarketplaceButton` hides zero-upgrade skills. Decide +whether those skills are purchasable at all and make the domain say so. + +## Slice 12 — one death path + +- Delete the 18 `Death` → `OnPlayerDeath` connections across `objects/level/base_level.tscn` and the eight level + scenes. +- `DeathScreen` and `GameOverScreen` subscribe to republished `LivesChanged` / `RunEnded`. +- `DeathScreen` currently reads `Player.Lives` directly *and* subscribes to `LivesChanged` — it keeps only the + subscription. + +**Verify:** die with lives remaining → scene reloads. Die on the last life → game over appears. The second case +does not work today. + +## Slice 13 — delete the dead surface + +- The four never-emitted signals (`LevelRestarted`, `EnemyDamaged`, `SkillActivated`, `SkillDeactivated`). +- Of the eleven emitted-but-unheard: keep the ones the republisher now feeds, delete the speculative ones. +- Rename `EventBus.SkillUnlocked` — it collides with `SkillUnlockerComponent.SkillUnlocked`, and every apparent + subscriber is bound to the component's. +- `SaveSystem.DeleteSave`, `SpeedRunManager.StopTimer`, `AchievementManager.IsAchievementUnlocked`, + `Constants.CoinsGroup` — zero callers each. +- `SpeedRunManager`: `StopTimer` carries a "save personal best" comment for logic that never existed. Best times + are the Profile's job now; delete the comment and the dead method. + +--- + +## Order dependencies + +``` +0 ──> 1 ──> 2 ──┐ + 3 ──┼──> 5 ──> 6 ──> 9 ──> 10 ──> 11 ──> 12 ──> 13 + 4 ──┘ ↑ + 7 ───┘ + 8 ───┘ +``` + +2, 3 and 4 are independent of each other after 1. 7 and 8 can be done any time after 5, and both must precede 9. +11, 12 and 13 each require 10.