docs: progression CONTEXT, ADRs 0001-0004, phase A plan
ignore .idea/ .superpowers/ bin/ obj/; drop 3 identical csproj backups
This commit is contained in:
261
docs/plans/phase-a-progression-domain.md
Normal file
261
docs/plans/phase-a-progression-domain.md
Normal file
@@ -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 `<EnableDynamicLoading>true</EnableDynamicLoading>` 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.
|
||||
Reference in New Issue
Block a user