From 321905e68eb75801f2faf5d3c4639e379350b9d4 Mon Sep 17 00:00:00 2001 From: Gabriel Kaszewski Date: Thu, 19 Mar 2026 01:41:14 +0100 Subject: [PATCH] refactor: fix bugs and improve architecture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix double-execution bug in LevelStateHandler (coins/skills were committed twice per level) - Fix DamageComponent to track multiple targets via HashSet instead of single node - Fix HealthComponent: update health immediately, decouple from PlayerController via signal wiring in PlayerController - Remove dead loop in SkillManager.RemoveSkill; simplify O(n²) throw skill loop - Replace GetNode(path) with .Instance across managers; add Instance to StatisticsManager/SpeedRunManager - GameManager.GetPlayer() now uses EventBus.PlayerSpawned instead of scanning all scene nodes - UIManager.UiStack: remove [Export], use private List - PlayerState: extract DefaultLives constant, simplify CreateDefault() --- Autoloads/GameManager.cs | 53 ++++++--------------- Autoloads/SkillManager.cs | 33 +++---------- Autoloads/SpeedRunManager.cs | 5 ++ Autoloads/StatisticsManager.cs | 4 ++ Autoloads/UIManager.cs | 13 +++--- scripts/Events/LevelStateHandler.cs | 14 +----- scripts/Events/StatisticsEventHandler.cs | 3 +- scripts/State/PlayerState.cs | 18 +++----- scripts/components/DamageComponent.cs | 27 +++++------ scripts/components/HealthComponent.cs | 59 ++++++------------------ scripts/components/PlayerController.cs | 17 +++++-- 11 files changed, 85 insertions(+), 161 deletions(-) diff --git a/Autoloads/GameManager.cs b/Autoloads/GameManager.cs index 73ceb8d..6ea7457 100644 --- a/Autoloads/GameManager.cs +++ b/Autoloads/GameManager.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using Godot; using Godot.Collections; using Mr.BrickAdventures.scripts.components; @@ -21,7 +20,6 @@ public partial class GameManager : Node private set => _player = value; } - private List _sceneNodes = []; private PlayerController _player; private SpeedRunManager _speedRunManager; @@ -32,38 +30,24 @@ public partial class GameManager : Node public static GameManager Instance { get; private set; } - public override void _EnterTree() + public override void _Ready() { - GetTree().NodeAdded += OnNodeAdded; - GetTree().NodeRemoved += OnNodeRemoved; + Instance = this; + _speedRunManager = SpeedRunManager.Instance; + EventBus.Instance.PlayerSpawned += OnPlayerSpawned; } public override void _ExitTree() { if (Instance == this) Instance = null; - GetTree().NodeAdded -= OnNodeAdded; - GetTree().NodeRemoved -= OnNodeRemoved; - _sceneNodes.Clear(); + if (EventBus.Instance != null) + EventBus.Instance.PlayerSpawned -= OnPlayerSpawned; } - public override void _Ready() + private void OnPlayerSpawned(PlayerController player) { - Instance = this; - _speedRunManager = GetNode(Constants.SpeedRunManagerPath); - } - - private void OnNodeAdded(Node node) - { - _sceneNodes.Add(node); - } - - private void OnNodeRemoved(Node node) - { - _sceneNodes.Remove(node); - if (node == _player) - { - _player = null; - } + _player = player; + player.TreeExiting += () => { if (_player == player) _player = null; }; } #region Coin Operations @@ -187,7 +171,7 @@ public partial class GameManager : Node { Store?.ResetAll(); GetTree().ChangeSceneToPacked(LevelScenes[0]); - GetNode(Constants.SaveSystemPath).SaveGame(); + SaveSystem.Instance.SaveGame(); } public void QuitGame() => GetTree().Quit(); @@ -209,13 +193,13 @@ public partial class GameManager : Node Store?.ResetAll(); _speedRunManager?.StartTimer(); GetTree().ChangeSceneToPacked(LevelScenes[0]); - GetNode(Constants.SaveSystemPath).SaveGame(); + SaveSystem.Instance.SaveGame(); EventBus.EmitGameStarted(); } public void ContinueGame() { - var save = GetNode(Constants.SaveSystemPath); + var save = SaveSystem.Instance; if (!save.LoadGame()) { GD.PrintErr("Failed to load game. Starting a new game instead."); @@ -249,7 +233,7 @@ public partial class GameManager : Node Store.ResetSession(); TryToGoToNextLevel(); - GetNode(Constants.SaveSystemPath).SaveGame(); + SaveSystem.Instance.SaveGame(); } #endregion @@ -259,18 +243,7 @@ public partial class GameManager : Node public PlayerController GetPlayer() { if (_player != null && IsInstanceValid(_player)) return _player; - _player = null; - - foreach (var node in _sceneNodes) - { - if (node is not PlayerController player) continue; - - _player = player; - return _player; - } - - GD.PrintErr("PlayerController not found in the scene tree."); return null; } diff --git a/Autoloads/SkillManager.cs b/Autoloads/SkillManager.cs index abaf4be..49afed9 100644 --- a/Autoloads/SkillManager.cs +++ b/Autoloads/SkillManager.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.Linq; using Godot; using Godot.Collections; -using Mr.BrickAdventures; using Mr.BrickAdventures.scripts.components; using Mr.BrickAdventures.scripts.interfaces; using Mr.BrickAdventures.scripts.Resources; @@ -11,8 +10,8 @@ namespace Mr.BrickAdventures.Autoloads; public partial class SkillManager : Node { - private GameManager _gameManager; private PlayerController _player; + private GameManager GameManager => GameManager.Instance; [Export] public Array AvailableSkills { get; set; } = []; @@ -28,7 +27,6 @@ public partial class SkillManager : Node public override void _Ready() { Instance = this; - _gameManager = GetNode(Constants.GameManagerPath); } public override void _ExitTree() @@ -85,21 +83,11 @@ public partial class SkillManager : Node if (skillData.Type == SkillType.Throw) { - var unlocked = _gameManager.GetUnlockedSkills(); - foreach (var sd in unlocked) + // Remove any other active throw skill before adding the new one + foreach (var sd in AvailableSkills) { - SkillData data = null; - foreach (var s in AvailableSkills) - { - if (s == sd) - { - data = s; - break; - } - } - // Remove other throw skills if a new one is added - if (data is { Type: SkillType.Throw }) - RemoveSkill(data.Name); + if (sd != skillData && sd.Type == SkillType.Throw) + RemoveSkill(sd.Name); } } @@ -146,14 +134,6 @@ public partial class SkillManager : Node if (IsInstanceValid(inst)) inst.QueueFree(); - var skills = _gameManager.GetUnlockedSkills(); - foreach (var s in skills) - { - if (s.Name == skillName) - { - break; - } - } ActiveComponents.Remove(skillName); var sd = GetSkillByName(skillName); if (sd != null) EmitSignalSkillRemoved(sd); @@ -174,10 +154,11 @@ public partial class SkillManager : Node public void ApplyUnlockedSkills() { if (_player == null || !IsInstanceValid(_player)) return; + if (GameManager == null) return; foreach (var sd in AvailableSkills) { - if (_gameManager.IsSkillUnlocked(sd)) + if (GameManager.IsSkillUnlocked(sd)) { CallDeferred(MethodName.AddSkill, sd); } diff --git a/Autoloads/SpeedRunManager.cs b/Autoloads/SpeedRunManager.cs index c44a1ad..edfdfda 100644 --- a/Autoloads/SpeedRunManager.cs +++ b/Autoloads/SpeedRunManager.cs @@ -5,6 +5,11 @@ namespace Mr.BrickAdventures.Autoloads; public partial class SpeedRunManager : Node { + public static SpeedRunManager Instance { get; private set; } + + public override void _EnterTree() => Instance = this; + public override void _ExitTree() { if (Instance == this) Instance = null; } + public bool IsRunning { get; private set; } = false; public bool IsVisible { get; private set; } = false; diff --git a/Autoloads/StatisticsManager.cs b/Autoloads/StatisticsManager.cs index c2669f3..079e058 100644 --- a/Autoloads/StatisticsManager.cs +++ b/Autoloads/StatisticsManager.cs @@ -9,6 +9,10 @@ namespace Mr.BrickAdventures.Autoloads; /// public partial class StatisticsManager : Node { + public static StatisticsManager Instance { get; private set; } + + public override void _Ready() => Instance = this; + public override void _ExitTree() { if (Instance == this) Instance = null; } /// /// Gets the statistics dictionary from the store. diff --git a/Autoloads/UIManager.cs b/Autoloads/UIManager.cs index cd576be..93b5844 100644 --- a/Autoloads/UIManager.cs +++ b/Autoloads/UIManager.cs @@ -1,11 +1,10 @@ using Godot; -using Godot.Collections; namespace Mr.BrickAdventures.Autoloads; public partial class UIManager : Node { - [Export] public Array UiStack { get; set; } = new(); + private readonly System.Collections.Generic.List UiStack = new(); [Signal] public delegate void ScreenPushedEventHandler(Control screen); [Signal] public delegate void ScreenPoppedEventHandler(Control screen); @@ -34,19 +33,19 @@ public partial class UIManager : Node return; } - var top = (Control)UiStack[^1]; + var top = UiStack[^1]; UiStack.RemoveAt(UiStack.Count - 1); top.Hide(); top.SetProcessInput(false); EmitSignalScreenPopped(top); top.AcceptEvent(); - if (UiStack.Count > 0) ((Control)UiStack[^1]).GrabFocus(); + if (UiStack.Count > 0) UiStack[^1].GrabFocus(); } - public Control TopScreen() => UiStack.Count > 0 ? (Control)UiStack[^1] : null; - - public bool IsScreenOnTop(Control screen) => UiStack.Count > 0 && (Control)UiStack[^1] == screen; + public Control TopScreen() => UiStack.Count > 0 ? UiStack[^1] : null; + + public bool IsScreenOnTop(Control screen) => UiStack.Count > 0 && UiStack[^1] == screen; public bool IsVisibleOnStack(Control screen) => UiStack.Contains(screen) && screen.Visible; diff --git a/scripts/Events/LevelStateHandler.cs b/scripts/Events/LevelStateHandler.cs index 5925e6f..a79fd06 100644 --- a/scripts/Events/LevelStateHandler.cs +++ b/scripts/Events/LevelStateHandler.cs @@ -23,17 +23,7 @@ public partial class LevelStateHandler : Node private void OnLevelCompleted(int levelIndex, Node currentScene, double completionTime) { - var store = GameStateStore.Instance; - if (store == null) return; - - // Mark level complete and unlock next - store.MarkLevelComplete(levelIndex); - - // Commit session data to persistent state - store.CommitSessionCoins(); - store.CommitSessionSkills(); - - // Reset session for next level - store.ResetSession(); + // State mutations (commit coins/skills, reset session) are handled by GameManager.OnLevelComplete + // before this event fires. This handler is reserved for future level-specific side-effects. } } diff --git a/scripts/Events/StatisticsEventHandler.cs b/scripts/Events/StatisticsEventHandler.cs index 420d6b9..9a41c3a 100644 --- a/scripts/Events/StatisticsEventHandler.cs +++ b/scripts/Events/StatisticsEventHandler.cs @@ -1,5 +1,4 @@ using Godot; -using Mr.BrickAdventures; using Mr.BrickAdventures.Autoloads; namespace Mr.BrickAdventures.scripts.Events; @@ -14,7 +13,7 @@ public partial class StatisticsEventHandler : Node public override void _Ready() { - _statisticsManager = GetNode(Constants.StatisticsManagerPath); + _statisticsManager = StatisticsManager.Instance; // Subscribe to events EventBus.Instance.CoinCollected += OnCoinCollected; diff --git a/scripts/State/PlayerState.cs b/scripts/State/PlayerState.cs index 8388217..6680b12 100644 --- a/scripts/State/PlayerState.cs +++ b/scripts/State/PlayerState.cs @@ -9,6 +9,8 @@ namespace Mr.BrickAdventures.scripts.State; /// public class PlayerState { + private const int DefaultLives = 3; + /// /// Saved coins (not including current session). /// @@ -17,7 +19,7 @@ public class PlayerState /// /// Remaining lives. /// - public int Lives { get; set; } = 3; + public int Lives { get; set; } = DefaultLives; /// /// Indices of completed levels. @@ -45,17 +47,9 @@ public class PlayerState public List UnlockedAchievements { get; set; } = new(); /// - /// Creates a fresh default player state. + /// Creates a fresh default player state (same as new PlayerState()). /// - public static PlayerState CreateDefault() => new() - { - Coins = 0, - Lives = 3, - CompletedLevels = new List(), - UnlockedLevels = new List { 0 }, - UnlockedSkills = new List(), - Statistics = new Dictionary() - }; + public static PlayerState CreateDefault() => new(); /// /// Resets this state to default values. @@ -63,7 +57,7 @@ public class PlayerState public void Reset() { Coins = 0; - Lives = 3; + Lives = DefaultLives; CompletedLevels.Clear(); UnlockedLevels.Clear(); UnlockedLevels.Add(0); diff --git a/scripts/components/DamageComponent.cs b/scripts/components/DamageComponent.cs index eb9992b..5d63b48 100644 --- a/scripts/components/DamageComponent.cs +++ b/scripts/components/DamageComponent.cs @@ -11,7 +11,7 @@ public partial class DamageComponent : Node [Export] public StatusEffectDataResource StatusEffectData { get; set; } [Export] public Timer DamageTimer { get; set; } - private Node _currentTarget = null; + private readonly System.Collections.Generic.HashSet _currentTargets = new(); private KnockbackComponent _knockbackComponent = null; [Signal] public delegate void EffectInflictedEventHandler(Node2D target, StatusEffectDataResource effect); @@ -35,10 +35,11 @@ public partial class DamageComponent : Node public override void _Process(double delta) { - if (_currentTarget == null) return; + if (_currentTargets.Count == 0) return; if (DamageTimer != null) return; - - ProcessEntityAndApplyDamage(_currentTarget as Node2D); + + foreach (var target in _currentTargets) + ProcessEntityAndApplyDamage(target as Node2D); } public void DealDamage(HealthComponent target) => target.DecreaseHealth(Damage); @@ -56,28 +57,28 @@ public partial class DamageComponent : Node private void OnAreaBodyExited(Node2D body) { - if (body != _currentTarget) return; - _currentTarget = null; - DamageTimer?.Stop(); + _currentTargets.Remove(body); + if (_currentTargets.Count == 0) + DamageTimer?.Stop(); } private void OnAreaBodyEntered(Node2D body) { - _currentTarget = body; + _currentTargets.Add(body); if (!CheckIfProcessingIsOn()) return; - DamageTimer?.Start(); + if (_currentTargets.Count == 1) + DamageTimer?.Start(); ProcessEntityAndApplyDamage(body); } - + private void OnDamageTimerTimeout() { - if (_currentTarget == null) return; - - ProcessEntityAndApplyDamage(_currentTarget as Node2D); + foreach (var target in _currentTargets) + ProcessEntityAndApplyDamage(target as Node2D); } private void ProcessEntityAndApplyDamage(Node2D body) diff --git a/scripts/components/HealthComponent.cs b/scripts/components/HealthComponent.cs index 685dd2a..7321541 100644 --- a/scripts/components/HealthComponent.cs +++ b/scripts/components/HealthComponent.cs @@ -1,6 +1,4 @@ -using System.Threading.Tasks; using Godot; -using Mr.BrickAdventures; using Mr.BrickAdventures.Autoloads; namespace Mr.BrickAdventures.scripts.components; @@ -20,27 +18,16 @@ public partial class HealthComponent : Node2D public override void _Ready() { - _floatingTextManager = GetNode(Constants.FloatingTextManagerPath); + _floatingTextManager = FloatingTextManager.Instance; } - public void SetHealth(float newValue) - { - _ = ApplyHealthChange(newValue); - } - - public void IncreaseHealth(float delta) - { - _ = ApplyHealthChange(Health + delta); - } - - public void DecreaseHealth(float delta) - { - _ = ApplyHealthChange(Health - delta); - } + public void SetHealth(float newValue) => ApplyHealthChange(newValue); + public void IncreaseHealth(float delta) => ApplyHealthChange(Health + delta); + public void DecreaseHealth(float delta) => ApplyHealthChange(Health - delta); public float GetDelta(float newValue) => newValue - Health; - private async Task ApplyHealthChange(float newHealth, bool playSfx = true) + private void ApplyHealthChange(float newHealth, bool playSfx = true) { newHealth = Mathf.Clamp(newHealth, 0.0f, MaxHealth); var delta = newHealth - Health; @@ -53,39 +40,19 @@ public partial class HealthComponent : Node2D else _floatingTextManager?.ShowHeal(delta, GlobalPosition); - if (playSfx) - { - if (delta > 0f && HealSfx != null) - { - HealSfx.Play(); - } - else if (delta < 0f && HurtSfx != null) - { - HurtSfx.Play(); - await HurtSfx.ToSignal(HurtSfx, AudioStreamPlayer2D.SignalName.Finished); - } - } - Health = newHealth; + if (playSfx) + { + if (delta > 0f) + HealSfx?.Play(); + else + HurtSfx?.Play(); + } + if (Health <= 0f) - { EmitSignalDeath(); - // Emit global event if this is the player - if (Owner is PlayerController) - EventBus.EmitPlayerDied(GlobalPosition); - } else - { EmitSignalHealthChanged(delta, Health); - // Emit global events if this is the player - if (Owner is PlayerController) - { - if (delta < 0f) - EventBus.EmitPlayerDamaged(Mathf.Abs(delta), Health, GlobalPosition); - else - EventBus.EmitPlayerHealed(delta, Health, GlobalPosition); - } - } } } \ No newline at end of file diff --git a/scripts/components/PlayerController.cs b/scripts/components/PlayerController.cs index 9cab451..6b617d2 100644 --- a/scripts/components/PlayerController.cs +++ b/scripts/components/PlayerController.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using Godot; -using Mr.BrickAdventures; using Mr.BrickAdventures.Autoloads; namespace Mr.BrickAdventures.scripts.components; @@ -34,8 +33,19 @@ public partial class PlayerController : CharacterBody2D public override void _Ready() { - var skillManager = GetNodeOrNull(Constants.SkillManagerPath); - skillManager?.RegisterPlayer(this); + SkillManager.Instance?.RegisterPlayer(this); + + // Wire HealthComponent signals to global EventBus events + var health = GetNodeOrNull("HealthComponent"); + if (health != null) + { + health.Death += () => EventBus.EmitPlayerDied(GlobalPosition); + health.HealthChanged += (delta, total) => + { + if (delta < 0f) EventBus.EmitPlayerDamaged(Mathf.Abs(delta), total, GlobalPosition); + else EventBus.EmitPlayerHealed(delta, total, GlobalPosition); + }; + } _inputHandler = GetNode("PlayerInputHandler"); foreach (var child in MovementAbilitiesContainer.GetChildren()) @@ -49,6 +59,7 @@ public partial class PlayerController : CharacterBody2D _ = ConnectJumpAndGravityAbilities(); EmitSignalMovementAbilitiesChanged(); + EventBus.EmitPlayerSpawned(this); } public override void _PhysicsProcess(double delta)