From e8d1da8f8996ba3ee383db5993b07cf3f2b10f4d Mon Sep 17 00:00:00 2001 From: Gabriel Kaszewski Date: Tue, 28 Jul 2026 20:19:35 +0200 Subject: [PATCH] feat: lives and death move to the new domain (commit C of the swap) fixes the regression commit B introduced: coins and skills lived in the Attempt ledger, but nothing discarded it, because LivesStateHandler still called the old store and HandlePlayerDeath was never invoked. Dying kept everything earned that level. Discard-on-death is the ledger's purpose, so it could not be separated from it. new AttemptFailed(remaining, runOver) event, raised after the life is spent and the ledger discarded. DeathScreen and GameOverScreen react to that instead of being wired to HealthComponent.Death by scene connections which fired *before* the life was charged - on the last life game-over saw one life left and showed nothing, then the death screen saw zero and refused to reload. 18 scene connections deleted; the race is now unrepresentable rather than fixed. DeathScreen also had two read paths for lives - a signal and a direct store read - because neither was reliable alone. Now one. Hud reads both facts from signals; only health still polls, and that belongs to the component rather than to progression. GameManager no longer touches GameStateStore or SaveSystem at all. --- Autoloads/EventBus.cs | 10 ++++ Autoloads/GameManager.cs | 18 +----- Autoloads/ProgressionEventRepublisher.cs | 4 ++ objects/level/base_level.tscn | 2 - progression.tests/ProgressionServiceTests.cs | 44 ++++++++++++++ progression/ProgressionEvent.cs | 12 ++++ progression/ProgressionService.cs | 5 +- scripts/Events/LivesStateHandler.cs | 13 +++-- scripts/UI/DeathScreen.cs | 60 +++++++++----------- scripts/UI/GameOverScreen.cs | 15 +++-- scripts/UI/Hud.cs | 21 ++++--- scripts/UI/MainMenu.cs | 5 +- scripts/UI/PauseMenu.cs | 2 +- 13 files changed, 139 insertions(+), 72 deletions(-) diff --git a/Autoloads/EventBus.cs b/Autoloads/EventBus.cs index 7bcb112..e2deec3 100644 --- a/Autoloads/EventBus.cs +++ b/Autoloads/EventBus.cs @@ -161,6 +161,16 @@ public partial class EventBus : Node /// [Signal] public delegate void RunEndedEventHandler(); + /// + /// A death has been settled. The life is already spent and the attempt already discarded, so + /// is safe to display directly — unlike reading Lives from a + /// handler racing the one that writes it. + /// + [Signal] public delegate void AttemptFailedEventHandler(int remainingLives, bool runOver); + + public static void EmitAttemptFailed(int remainingLives, bool runOver) + => Instance?.EmitSignal(SignalName.AttemptFailed, remainingLives, runOver); + public static void EmitRunEnded() => Instance?.EmitSignal(SignalName.RunEnded); diff --git a/Autoloads/GameManager.cs b/Autoloads/GameManager.cs index 1650565..ea5b81c 100644 --- a/Autoloads/GameManager.cs +++ b/Autoloads/GameManager.cs @@ -15,9 +15,9 @@ namespace Mr.BrickAdventures.Autoloads; /// of an inspector array, so reordering levels silently invalidated every save. /// /// -/// Transitional: coins, lives and skills are still owned by GameStateStore and persisted by -/// SaveSystem. Only level progression has moved. The two are kept apart deliberately — nothing -/// is tracked in both places at once. +/// Progression is persisted by 's stores, which save themselves at the +/// points that matter — a First Clear and a death — so there is no separate "save the game" step to +/// forget. /// /// public partial class GameManager : Node @@ -30,7 +30,6 @@ public partial class GameManager : Node private PlayerController _player; private SpeedRunManager SpeedRunManager => SpeedRunManager.Instance; - private GameStateStore Store => GameStateStore.Instance; private ProgressionRoot Progression => ProgressionRoot.Instance; public static GameManager Instance { get; private set; } @@ -60,10 +59,8 @@ public partial class GameManager : Node { if (Progression == null) return; - Store?.ResetAll(); Progression.StartPlaythrough(ProgressionRoot.SoleSlot); SpeedRunManager?.StartTimer(); - SaveSystem.Instance.SaveGame(); EventBus.EmitGameStarted(); GoTo(Progression.Campaign.First); @@ -73,10 +70,6 @@ public partial class GameManager : Node { if (Progression == null) return; - // Coins and skills still come from the old save; level progression from the new one. - if (!SaveSystem.Instance.LoadGame()) - GD.PrintErr("GameManager: no legacy save to continue; starting fresh."); - if (!Progression.LoadPlaythrough(ProgressionRoot.SoleSlot)) { var why = Progression.PeekPlaythrough(ProgressionRoot.SoleSlot); @@ -101,10 +94,8 @@ public partial class GameManager : Node { if (Progression == null) return; - Store?.ResetAll(); Progression.StartPlaythrough(ProgressionRoot.SoleSlot); SpeedRunManager?.StartTimer(); - SaveSystem.Instance.SaveGame(); GoTo(Progression.Campaign.First); } @@ -122,9 +113,6 @@ public partial class GameManager : Node // position if this was a First Clear. Progression.FinishLevel(time); - // Still needed for lives, which move with the death path in the next commit. - SaveSystem.Instance.SaveGame(); - if (Progression.CurrentLevel is { } next && !next.Equals(finished)) GoTo(next); } diff --git a/Autoloads/ProgressionEventRepublisher.cs b/Autoloads/ProgressionEventRepublisher.cs index e60f13b..60a0042 100644 --- a/Autoloads/ProgressionEventRepublisher.cs +++ b/Autoloads/ProgressionEventRepublisher.cs @@ -51,6 +51,10 @@ public sealed class ProgressionEventRepublisher EventBus.EmitRunEnded(); break; + case AttemptFailed failed: + EventBus.EmitAttemptFailed(failed.Remaining.Value, failed.RunOver); + break; + case AchievementUnlocked unlocked: EventBus.EmitAchievementUnlocked(unlocked.Achievement.Value); break; diff --git a/objects/level/base_level.tscn b/objects/level/base_level.tscn index fa9de44..a860055 100644 --- a/objects/level/base_level.tscn +++ b/objects/level/base_level.tscn @@ -110,8 +110,6 @@ tile_set = ExtResource("11_0vx27") [node name="Killzone" parent="." unique_id=319936093 instance=ExtResource("13_0vx27")] position = Vector2(215, 324) -[connection signal="Death" from="Brick Player/HealthComponent" to="UI Layer/DeathScreen" method="OnPlayerDeath"] -[connection signal="Death" from="Brick Player/HealthComponent" to="UI Layer/GameOverScreen" method="OnPlayerDeath"] [editable path="Brick Player"] [editable path="UI Layer"] diff --git a/progression.tests/ProgressionServiceTests.cs b/progression.tests/ProgressionServiceTests.cs index 63f89a2..b8c64ee 100644 --- a/progression.tests/ProgressionServiceTests.cs +++ b/progression.tests/ProgressionServiceTests.cs @@ -230,6 +230,50 @@ public class ProgressionServiceTests Assert.False(service.CanEnter(Second)); } + [Fact] + public void A_settled_death_announces_the_outcome_so_no_screen_has_to_read_lives_itself() + { + // DeathScreen and GameOverScreen were both wired to HealthComponent.Death by scene + // connections that fire before lives are decremented, so on the last life neither saw zero. + // Announcing the settled result means the value is already correct when they hear about it. + var service = ServiceWithSkills(); + + service.HandlePlayerDeath(); + + var failed = service.DrainEvents().OfType().Single(); + Assert.Equal(2, failed.Remaining.Value); + Assert.False(failed.RunOver); + } + + [Fact] + public void Dying_on_the_last_life_announces_the_run_is_over_in_the_same_breath() + { + var service = new ProgressionService( + Profile.Empty(), new InMemoryPlaythroughStore(), new InMemoryProfileStore(), + TwoLevels(), Skills(), Lives.Of(1)); + service.StartPlaythrough(Id); + service.EnterLevel(First); + service.DrainEvents(); + + service.HandlePlayerDeath(); + + var events = service.DrainEvents(); + Assert.True(events.OfType().Single().RunOver); + Assert.Single(events.OfType()); + } + + [Fact] + public void A_repeated_death_event_announces_nothing_further() + { + var service = ServiceWithSkills(); + service.HandlePlayerDeath(); + service.DrainEvents(); + + service.HandlePlayerDeath(); + + Assert.Empty(service.DrainEvents().OfType()); + } + [Fact] public void A_collected_skill_is_granted_free_and_only_once() { diff --git a/progression/ProgressionEvent.cs b/progression/ProgressionEvent.cs index e973f6e..c94d705 100644 --- a/progression/ProgressionEvent.cs +++ b/progression/ProgressionEvent.cs @@ -10,6 +10,18 @@ public abstract record ProgressionEvent; /// Raised when a Playthrough loses its last life. public sealed record RunEnded : ProgressionEvent; +/// +/// An Attempt ended in death and has been settled: the life is already spent and the ledger already +/// discarded when this is raised. +/// +/// +/// Exists so no screen has to read Lives for itself. The death and game-over screens were wired +/// straight to HealthComponent.Death by scene connections, which fire before the life is charged +/// — so on the last life the game-over screen saw one life left and showed nothing, and the death +/// screen then saw zero and refused to reload. Carrying the settled values removes the race entirely. +/// +public sealed record AttemptFailed(Lives Remaining, bool RunOver) : ProgressionEvent; + /// /// Raised the first time an Achievement is unlocked, and only then — so a subscriber that shows a /// popup or pushes to Steam cannot fire twice however often it is told. diff --git a/progression/ProgressionService.cs b/progression/ProgressionService.cs index 59bc293..af26d2b 100644 --- a/progression/ProgressionService.cs +++ b/progression/ProgressionService.cs @@ -246,10 +246,13 @@ public sealed class ProgressionService if (!_playthrough.SettleDeath(_attempt)) return; - // The ledger was discarded, so it must stop contributing to TotalCoins. + // The ledger was discarded, so it must stop contributing to TotalCoins or to owned skills. _attempt = null; _playthroughs.Save(_playthrough); AnnounceChanges(); + + // Announced last, once lives and the discard have both landed. + _events.Raise(new AttemptFailed(_playthrough.Lives, _playthrough.IsRunOver)); } /// diff --git a/scripts/Events/LivesStateHandler.cs b/scripts/Events/LivesStateHandler.cs index 77179b1..86285a9 100644 --- a/scripts/Events/LivesStateHandler.cs +++ b/scripts/Events/LivesStateHandler.cs @@ -4,8 +4,14 @@ using Mr.BrickAdventures.Autoloads; namespace Mr.BrickAdventures.scripts.Events; /// -/// Handles player death events and updates lives in GameStateStore. +/// Charges a death to the Playthrough. /// +/// +/// One call now does what three did: spend a life, discard the Attempt's ledger, and announce the +/// settled result. It is also idempotent — overlapping hazards in a kill zone, or a hazard re-firing +/// during the two seconds before the scene reloads, cost one life. See +/// docs/adr/0005-playthrough-settles-attempts.md. +/// public partial class LivesStateHandler : Node { public override void _Ready() @@ -16,14 +22,11 @@ public partial class LivesStateHandler : Node public override void _ExitTree() { if (EventBus.Instance != null) - { EventBus.Instance.PlayerDied -= OnPlayerDied; - } } private void OnPlayerDied(Vector2 position) { - GameStateStore.Instance?.RemoveLife(); - GameStateStore.Instance?.ResetSession(); + ProgressionRoot.Instance?.HandlePlayerDeath(); } } diff --git a/scripts/UI/DeathScreen.cs b/scripts/UI/DeathScreen.cs index ecf1d0e..67071b3 100644 --- a/scripts/UI/DeathScreen.cs +++ b/scripts/UI/DeathScreen.cs @@ -1,7 +1,6 @@ using Godot; using Mr.BrickAdventures.Autoloads; using Mr.BrickAdventures.scripts.Resources; -using Mr.BrickAdventures.scripts.State; namespace Mr.BrickAdventures.scripts.UI; @@ -14,46 +13,49 @@ public partial class DeathScreen : Control [Export] public float TimeoutTime { get; set; } = 2.0f; [Export] public Godot.Collections.Array NodesToDisable { get; set; } = new(); - private GameManager GameManager => GameManager.Instance; private Timer _timer; + private bool _runOver; public override void _Ready() { - // Subscribe to lives changed event for reactive updates - EventBus.Instance.LivesChanged += OnLivesChanged; + EventBus.Instance.AttemptFailed += OnAttemptFailed; } public override void _ExitTree() { if (EventBus.Instance != null) - { - EventBus.Instance.LivesChanged -= OnLivesChanged; - } + EventBus.Instance.AttemptFailed -= OnAttemptFailed; } - private void OnLivesChanged(int lives) + /// + /// Shows the death screen. + /// + /// + /// Driven by the settled outcome rather than by a scene connection to + /// HealthComponent.Death, so the life is already spent and the count already correct. The old + /// arrangement read Lives from a handler racing the one that wrote it, then read it a second + /// time on timeout — two read paths for one fact, because neither was reliable alone. + /// + private void OnAttemptFailed(int remainingLives, bool runOver) { - // Update the label when lives change - LivesLeftLabel.Text = $" x {lives}"; - } + _runOver = runOver; + LivesLeftLabel.Text = $" x {remainingLives}"; - private void SetLabels() - { if (CurrentLevel != null) - { CurrentLevelLabel.Text = CurrentLevel.LevelName; - } - // Read current lives from store - var lives = GameStateStore.Instance?.Player.Lives ?? 0; - LivesLeftLabel.Text = $" x {lives}"; + ToggleNodes(); + Show(); + SetupTimer(); } private void SetupTimer() { - _timer = new Timer(); - _timer.WaitTime = TimeoutTime; - _timer.OneShot = true; + _timer = new Timer + { + WaitTime = TimeoutTime, + OneShot = true, + }; _timer.Timeout += OnTimeout; AddChild(_timer); _timer.Start(); @@ -69,21 +71,11 @@ public partial class DeathScreen : Control } } - public void OnPlayerDeath() - { - if (GameManager == null) return; - - ToggleNodes(); - SetLabels(); - Show(); - SetupTimer(); - } - private void OnTimeout() { - var lives = GameStateStore.Instance?.Player.Lives ?? 0; - if (lives == 0) return; + // Game over is the other screen's job; this one only retries. + if (_runOver) return; GetTree().ReloadCurrentScene(); } -} \ No newline at end of file +} diff --git a/scripts/UI/GameOverScreen.cs b/scripts/UI/GameOverScreen.cs index 1a98455..738a1ef 100644 --- a/scripts/UI/GameOverScreen.cs +++ b/scripts/UI/GameOverScreen.cs @@ -21,12 +21,17 @@ public partial class GameOverScreen : Control if (InputDeviceManager.Instance != null) InputDeviceManager.Instance.DeviceChanged += OnDeviceChanged; + + EventBus.Instance.RunEnded += OnRunEnded; } public override void _ExitTree() { if (InputDeviceManager.Instance != null) InputDeviceManager.Instance.DeviceChanged -= OnDeviceChanged; + + if (EventBus.Instance != null) + EventBus.Instance.RunEnded -= OnRunEnded; } private void OnDeviceChanged(int device) @@ -38,7 +43,6 @@ public partial class GameOverScreen : Control private void OnMainMenuClicked() { - GameStateStore.Instance?.ResetAll(); SpeedRunManager.Instance?.StopTimer(); GetTree().ChangeSceneToPacked(MainMenuScene); } @@ -48,10 +52,13 @@ public partial class GameOverScreen : Control GameManager.Instance?.RestartGame(); } - public void OnPlayerDeath() + /// + /// Shown when the run is actually over. Previously this read Lives from a scene connection that + /// fired before the life was charged, so on the last life it saw one remaining and showed nothing — + /// leaving the death screen up forever. + /// + private void OnRunEnded() { - if (GameStateStore.Instance?.Player.Lives != 0) return; - GameOverPanel.Show(); if (InputDeviceManager.Instance?.IsPointerless == true) RestartButton.GrabFocus(); diff --git a/scripts/UI/Hud.cs b/scripts/UI/Hud.cs index 47680f3..ebdcd86 100644 --- a/scripts/UI/Hud.cs +++ b/scripts/UI/Hud.cs @@ -12,29 +12,36 @@ public partial class Hud : Control [Export] public ProgressBar HealthBar { get; set; } [Export] public Label LivesLabel { get; set; } - private GameStateStore Store => GameStateStore.Instance; - public override void _Ready() { if (EventBus.Instance != null) + { EventBus.Instance.CoinsChanged += OnCoinsChanged; + EventBus.Instance.LivesChanged += OnLivesChanged; + } - SetCoinsLabel(ProgressionRoot.Instance?.TotalCoins.Value ?? 0); + var progression = ProgressionRoot.Instance; + SetCoinsLabel(progression?.TotalCoins.Value ?? 0); + SetLivesLabel(progression?.Lives.Value ?? 0); } public override void _ExitTree() { if (EventBus.Instance != null) + { EventBus.Instance.CoinsChanged -= OnCoinsChanged; + EventBus.Instance.LivesChanged -= OnLivesChanged; + } } - // Lives still polls the old store; it moves with the death path in the next commit. + // Health belongs to the component on the player, not to progression, so it stays a direct read. public override void _Process(double delta) { SetHealthBar(); - SetLivesLabel(); } + private void OnLivesChanged(int lives) => SetLivesLabel(lives); + private void OnCoinsChanged(int totalCoins) => SetCoinsLabel(totalCoins); private void SetCoinsLabel(int totalCoins) @@ -42,9 +49,9 @@ public partial class Hud : Control CoinsLabel.Text = Tr("COINS_LABEL") + ": " + totalCoins; } - private void SetLivesLabel() + private void SetLivesLabel(int lives) { - LivesLabel.Text = Tr("LIVES_LABEL") + ": " + (Store?.Player.Lives ?? 0); + LivesLabel.Text = Tr("LIVES_LABEL") + ": " + lives; } private void SetHealthBar() diff --git a/scripts/UI/MainMenu.cs b/scripts/UI/MainMenu.cs index 49345e6..e53d0be 100644 --- a/scripts/UI/MainMenu.cs +++ b/scripts/UI/MainMenu.cs @@ -15,7 +15,6 @@ public partial class MainMenu : Control [Export] public Control SettingsControl { get; set; } [Export] public Control CreditsControl { get; set; } - private SaveSystem SaveSystem => SaveSystem.Instance; private GameManager GameManager => GameManager.Instance; private UIManager UIManager => UIManager.Instance; @@ -34,7 +33,7 @@ public partial class MainMenu : Control ExitButton.Pressed += OnExitPressed; VersionLabel.Text = $"v. {ProjectSettings.GetSetting("application/config/version")}"; - ContinueButton.Disabled = !SaveSystem.CheckSaveExists(); + ContinueButton.Disabled = !(ProgressionRoot.Instance?.HasSavedPlaythrough ?? false); if (InputDeviceManager.Instance != null) InputDeviceManager.Instance.DeviceChanged += OnDeviceChanged; @@ -66,7 +65,7 @@ public partial class MainMenu : Control private void GrabFirstFocus() { - if (SaveSystem.CheckSaveExists()) + if (ProgressionRoot.Instance?.HasSavedPlaythrough ?? false) ContinueButton.GrabFocus(); else NewGameButton.GrabFocus(); diff --git a/scripts/UI/PauseMenu.cs b/scripts/UI/PauseMenu.cs index e8bb83a..928b4da 100644 --- a/scripts/UI/PauseMenu.cs +++ b/scripts/UI/PauseMenu.cs @@ -83,7 +83,7 @@ public partial class PauseMenu : Control private void OnMainMenuPressed() { GameManager.ResumeGame(); - GameStateStore.Instance?.ResetSession(); + ProgressionRoot.Instance?.AbandonAttempt(); // The run is over once you leave to the menu; otherwise the timer keeps ticking there. SpeedRunManager.Instance?.StopTimer(); GetTree().ChangeSceneToPacked(MainMenuScene);