Gabriel Kaszewski
313a5315b1
three bug-encoding invariants, one red/green cycle each: - Coins.TrySubtract fails instead of clamping - LevelTime rejects zero (continued-run zero best-times) - SkillLevel has no zero (unowned is absence, not Level=1) plan: drop signed NetCoins; add Attempt outcome as idempotency guard
291 lines
15 KiB
Markdown
291 lines
15 KiB
Markdown
# 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:**
|
||
|
||
```sh
|
||
grep -rn "using Godot" progression progression.tests --include=*.cs # must print nothing
|
||
```
|
||
|
||
That single check is what ADR-0004 buys you. Scope it to `--include=*.cs` — an unscoped `grep -r` also matches
|
||
prose in the `.csproj` comments and reports a false violation.
|
||
|
||
---
|
||
|
||
## 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`, plus an outcome.
|
||
|
||
- `RecordCoinsFound(Coins)`
|
||
- `RecordPurchase(SkillId, Coins cost)`
|
||
- `Outcome` — `InProgress | Died | Cleared`
|
||
- `bool TryEndInDeath()` / `bool TryEndInCompletion()` — return false if the Attempt already ended
|
||
- No `Discard` method: discarding is throwing the object away.
|
||
|
||
`CoinsFound` and `CoinsSpent` are both non-negative. There is no signed net value — settlement is
|
||
`bank.Add(found).TrySubtract(spent)` in `Playthrough.Commit`, which in that order cannot go negative given
|
||
affordability was checked at purchase time.
|
||
|
||
> Corrects an earlier draft of this plan, which had `NetCoins` "may be negative". That would have forced `Coins`
|
||
> to permit negatives and defeated the point of the type.
|
||
|
||
**The outcome is the idempotency guard, and it is the reason this slice matters.** An Attempt is the thing that
|
||
dies or is cleared, and it can only do either once. `Lives` flooring at zero does not solve this: losing two lives
|
||
from a single death takes you from 3 to 1, which no clamp catches.
|
||
|
||
Tests worth writing:
|
||
|
||
- **An Attempt cannot end twice.** Second `TryEndInDeath` returns false and changes nothing.
|
||
- **An Attempt that died cannot then be cleared**, and vice versa.
|
||
- Spending more than found is representable — the shortfall is settled at Commit, not rejected here.
|
||
- Recording anything after the Attempt ended is refused.
|
||
|
||
Two real call sites motivate this. Death: overlapping hazards in a kill zone, or a hazard re-triggering during the
|
||
two seconds before `DeathScreen` reloads the scene — the player node still exists with zero health. Completion:
|
||
`ExitDoorComponent:46` and `ConsoleManager.GoToNextLevelCommand:99` both call `OnLevelComplete()`.
|
||
|
||
`HealthComponent.ApplyHealthChange` currently absorbs repeat deaths via its `delta == 0` early return, but that is
|
||
incidental and `Health` is a public `[Export]` setter that bypasses it. Do not rely on the engine side.
|
||
|
||
---
|
||
|
||
## 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.
|
||
- **Two deaths in one Attempt cost one life.** Pairs with slice 2's outcome guard; assert it here at the
|
||
Playthrough level too, because this is the invariant a player would actually notice.
|
||
- **`Commit` on an already-cleared level does not advance and does not bank** (ADR-0003).
|
||
- **`Commit` settles as `bank.Add(found).TrySubtract(spent)`**, in that order, so an Attempt that overspent
|
||
reduces the Bank correctly and can never drive it negative. 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.
|