diff --git a/docs/plans/phase-a-progression-domain.md b/docs/plans/phase-a-progression-domain.md index 641ea98..0230e7e 100644 --- a/docs/plans/phase-a-progression-domain.md +++ b/docs/plans/phase-a-progression-domain.md @@ -71,18 +71,38 @@ Invariants to drive out with tests: ## Slice 2 — Attempt · TDD -The ledger from ADR-0002: `CoinsFound`, `CoinsSpent`, `SkillsBought`. +The ledger from ADR-0002: `CoinsFound`, `CoinsSpent`, `SkillsBought`, plus an outcome. - `RecordCoinsFound(Coins)` - `RecordPurchase(SkillId, Coins cost)` -- `NetCoins` — may be negative, when purchases drew against the Bank +- `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: -- 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. +- **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. --- @@ -101,9 +121,12 @@ 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` 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. +- **`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. diff --git a/progression.tests/CoinsTests.cs b/progression.tests/CoinsTests.cs new file mode 100644 index 0000000..95f5970 --- /dev/null +++ b/progression.tests/CoinsTests.cs @@ -0,0 +1,17 @@ +namespace Mr.BrickAdventures.Progression.Tests; + +public class CoinsTests +{ + [Fact] + public void Subtracting_more_than_held_fails_and_leaves_the_amount_untouched() + { + var held = Coins.Of(10); + + var succeeded = held.TrySubtract(Coins.Of(30), out var remaining); + + Assert.False(succeeded); + // Not clamped to zero. A caller that ignores the bool sees coins un-spent, + // which is the safe outcome; clamping would silently consume what wasn't there. + Assert.Equal(Coins.Of(10), remaining); + } +} diff --git a/progression.tests/LevelTimeTests.cs b/progression.tests/LevelTimeTests.cs new file mode 100644 index 0000000..1815a92 --- /dev/null +++ b/progression.tests/LevelTimeTests.cs @@ -0,0 +1,13 @@ +namespace Mr.BrickAdventures.Progression.Tests; + +public class LevelTimeTests +{ + [Fact] + public void Zero_is_not_a_completion_time() + { + // GetCurrentLevelTime() returns 0 whenever the speedrun timer was never started, which + // today is every continued run — so zero-second "best times" get written to the ghost + // files. Rejecting zero makes that unrepresentable rather than merely unlikely. + Assert.Throws(() => LevelTime.Of(0)); + } +} diff --git a/progression.tests/SkillLevelTests.cs b/progression.tests/SkillLevelTests.cs new file mode 100644 index 0000000..a7271f2 --- /dev/null +++ b/progression.tests/SkillLevelTests.cs @@ -0,0 +1,13 @@ +namespace Mr.BrickAdventures.Progression.Tests; + +public class SkillLevelTests +{ + [Fact] + public void There_is_no_level_zero_because_unowned_is_absence() + { + // SkillData.Level defaults to 1 today, so "not owned" and "owned at level 1" are the same + // value and nothing can tell them apart. A Playthrough records only the skills it owns, so + // absence from that record is the only way to say "unowned" — level zero must not exist. + Assert.Throws(() => SkillLevel.Of(0)); + } +} diff --git a/progression/Coins.cs b/progression/Coins.cs new file mode 100644 index 0000000..64e7b6c --- /dev/null +++ b/progression/Coins.cs @@ -0,0 +1,37 @@ +namespace Mr.BrickAdventures.Progression; + +/// +/// A non-negative quantity of coins. Amounts only — there is no signed coin value; a shortfall is +/// a failed subtraction the caller must handle, never a negative total. +/// +public readonly struct Coins : IEquatable +{ + public int Value { get; } + + private Coins(int value) => Value = value; + + public static Coins Of(int value) => value < 0 + ? throw new ArgumentOutOfRangeException(nameof(value), value, "Coins cannot be negative.") + : new Coins(value); + + /// + /// Subtracts . Returns false when the amount exceeds what is held, + /// leaving untouched rather than clamping to zero. + /// + public bool TrySubtract(Coins amount, out Coins remaining) + { + if (amount.Value > Value) + { + remaining = this; + return false; + } + + remaining = new Coins(Value - amount.Value); + return true; + } + + public bool Equals(Coins other) => Value == other.Value; + public override bool Equals(object? obj) => obj is Coins other && Equals(other); + public override int GetHashCode() => Value; + public override string ToString() => Value.ToString(); +} diff --git a/progression/LevelTime.cs b/progression/LevelTime.cs new file mode 100644 index 0000000..3f45271 --- /dev/null +++ b/progression/LevelTime.cs @@ -0,0 +1,16 @@ +namespace Mr.BrickAdventures.Progression; + +/// +/// A completion time for one Level. Strictly positive — a zero time means the clock was never +/// running, which is a defect rather than a fast run. +/// +public readonly struct LevelTime +{ + public double Seconds { get; } + + private LevelTime(double seconds) => Seconds = seconds; + + public static LevelTime Of(double seconds) => seconds <= 0 + ? throw new ArgumentOutOfRangeException(nameof(seconds), seconds, "A completion time must be positive.") + : new LevelTime(seconds); +} diff --git a/progression/SkillLevel.cs b/progression/SkillLevel.cs new file mode 100644 index 0000000..56f83d6 --- /dev/null +++ b/progression/SkillLevel.cs @@ -0,0 +1,16 @@ +namespace Mr.BrickAdventures.Progression; + +/// +/// How far a Skill has been upgraded. Starts at one; there is no zero, because a Playthrough +/// records only the Skills it owns and absence from that record is what "unowned" means. +/// +public readonly struct SkillLevel +{ + public int Value { get; } + + private SkillLevel(int value) => Value = value; + + public static SkillLevel Of(int value) => value < 1 + ? throw new ArgumentOutOfRangeException(nameof(value), value, "A skill level starts at 1; unowned is absence, not level 0.") + : new SkillLevel(value); +}