diff --git a/Assets/Editor/CustomLogWindow.cs b/Assets/Editor/CustomLogWindow.cs index 5e936742..c4cb45b3 100644 --- a/Assets/Editor/CustomLogWindow.cs +++ b/Assets/Editor/CustomLogWindow.cs @@ -49,11 +49,6 @@ namespace Editor #region UI State - private bool showClassFilter = true; - private bool showMethodFilter = false; - private bool showLevelFilter = true; - private bool showTimeFilter = false; - // Colors for log levels private readonly Color debugColor = Color.white; private readonly Color infoColor = Color.white; @@ -123,7 +118,6 @@ namespace Editor private void OnGUI() { DrawToolbar(); - DrawFilters(); DrawLogEntries(); } @@ -144,11 +138,46 @@ namespace Editor // Auto-scroll toggle autoScroll = GUILayout.Toggle(autoScroll, "Auto-scroll", EditorStyles.toolbarButton, GUILayout.Width(80)); + GUILayout.Space(10); + + // Class Filter Button + string classLabel = selectedClassFilters.Count == 0 ? "Classes: All" : + selectedClassFilters.Count == activeClassTags.Count ? "Classes: All" : + $"Classes: {selectedClassFilters.Count}"; + if (GUILayout.Button(new GUIContent(classLabel, "Filter by class"), EditorStyles.toolbarDropDown, GUILayout.Width(100))) + { + ShowClassFilterMenu(); + } + + // Method Filter Button + string methodLabel = selectedMethodFilters.Count == 0 ? "Methods: All" : + selectedMethodFilters.Count == activeMethodTags.Count ? "Methods: All" : + $"Methods: {selectedMethodFilters.Count}"; + if (GUILayout.Button(new GUIContent(methodLabel, "Filter by method"), EditorStyles.toolbarDropDown, GUILayout.Width(110))) + { + ShowMethodFilterMenu(); + } + + // Log Level Filter Button + string levelLabel = selectedLevelFilters.Count == 4 ? "Levels: All" : + selectedLevelFilters.Count == 0 ? "Levels: None" : + $"Levels: {selectedLevelFilters.Count}"; + if (GUILayout.Button(new GUIContent(levelLabel, "Filter by log level"), EditorStyles.toolbarDropDown, GUILayout.Width(90))) + { + ShowLevelFilterMenu(); + } + + // Time Range Filter Button + if (GUILayout.Button(new GUIContent("⏱", "Time range filter"), EditorStyles.toolbarButton, GUILayout.Width(25))) + { + ShowTimeRangeWindow(); + } + GUILayout.FlexibleSpace(); - // Log count - var filteredCount = GetFilteredLogs().Count(); - GUILayout.Label($"{filteredCount} / {allLogs.Count} logs", GUILayout.Width(100)); + // Search box + GUILayout.Label("Search:", GUILayout.Width(50)); + searchText = GUILayout.TextField(searchText, EditorStyles.toolbarSearchField, GUILayout.Width(150)); GUILayout.Space(10); @@ -158,135 +187,55 @@ namespace Editor ExportLogs(); } - GUILayout.Space(10); + GUILayout.Space(5); - // Search box - GUILayout.Label("Search:", GUILayout.Width(50)); - searchText = GUILayout.TextField(searchText, EditorStyles.toolbarSearchField, GUILayout.Width(200)); + // Log count + var filteredCount = GetFilteredLogs().Count(); + GUILayout.Label($"{filteredCount}/{allLogs.Count}", GUILayout.Width(80)); EditorGUILayout.EndHorizontal(); } - private void DrawFilters() + private void ShowClassFilterMenu() { - EditorGUILayout.BeginVertical(EditorStyles.helpBox); - - // Class Filters - showClassFilter = EditorGUILayout.Foldout(showClassFilter, $"Class Filters ({selectedClassFilters.Count} selected)", true); - if (showClassFilter) - { - EditorGUI.indentLevel++; - - EditorGUILayout.BeginHorizontal(); - if (GUILayout.Button("All", GUILayout.Width(50))) + ClassFilterWindow.ShowWindow(this, activeClassTags, selectedClassFilters, + (newSelection) => { - selectedClassFilters = new HashSet(activeClassTags); - } - if (GUILayout.Button("None", GUILayout.Width(50))) + selectedClassFilters = newSelection; + Repaint(); + }); + } + + private void ShowMethodFilterMenu() + { + MethodFilterWindow.ShowWindow(this, activeMethodTags, selectedMethodFilters, + (newSelection) => { - selectedClassFilters.Clear(); - } - EditorGUILayout.EndHorizontal(); - - EditorGUILayout.BeginVertical(); - foreach (var tag in activeClassTags.OrderBy(t => t)) + selectedMethodFilters = newSelection; + Repaint(); + }); + } + + private void ShowLevelFilterMenu() + { + LevelFilterWindow.ShowWindow(this, selectedLevelFilters, + (newSelection) => { - bool isSelected = selectedClassFilters.Contains(tag); - bool newSelection = EditorGUILayout.ToggleLeft(tag, isSelected); - - if (newSelection && !isSelected) - selectedClassFilters.Add(tag); - else if (!newSelection && isSelected) - selectedClassFilters.Remove(tag); - } - EditorGUILayout.EndVertical(); - - EditorGUI.indentLevel--; - } - - EditorGUILayout.Space(5); - - // Method Filters - showMethodFilter = EditorGUILayout.Foldout(showMethodFilter, $"Method Filters ({selectedMethodFilters.Count} selected)", true); - if (showMethodFilter) - { - EditorGUI.indentLevel++; - - EditorGUILayout.BeginHorizontal(); - if (GUILayout.Button("All", GUILayout.Width(50))) + selectedLevelFilters = newSelection; + Repaint(); + }); + } + + private void ShowTimeRangeWindow() + { + TimeRangeFilterWindow.ShowWindow(this, enableTimeFilter, minTimestamp, maxTimestamp, currentMaxTimestamp, + (enabled, min, max) => { - selectedMethodFilters = new HashSet(activeMethodTags); - } - if (GUILayout.Button("None", GUILayout.Width(50))) - { - selectedMethodFilters.Clear(); - } - EditorGUILayout.EndHorizontal(); - - EditorGUILayout.BeginVertical(); - foreach (var tag in activeMethodTags.OrderBy(t => t)) - { - bool isSelected = selectedMethodFilters.Contains(tag); - bool newSelection = EditorGUILayout.ToggleLeft(tag, isSelected); - - if (newSelection && !isSelected) - selectedMethodFilters.Add(tag); - else if (!newSelection && isSelected) - selectedMethodFilters.Remove(tag); - } - EditorGUILayout.EndVertical(); - - EditorGUI.indentLevel--; - } - - EditorGUILayout.Space(5); - - // Log Level Filters - showLevelFilter = EditorGUILayout.Foldout(showLevelFilter, "Log Level Filters", true); - if (showLevelFilter) - { - EditorGUI.indentLevel++; - foreach (LogLevel level in Enum.GetValues(typeof(LogLevel))) - { - bool isSelected = selectedLevelFilters.Contains(level); - bool newSelection = EditorGUILayout.ToggleLeft(level.ToString(), isSelected); - - if (newSelection && !isSelected) - selectedLevelFilters.Add(level); - else if (!newSelection && isSelected) - selectedLevelFilters.Remove(level); - } - EditorGUI.indentLevel--; - } - - EditorGUILayout.Space(5); - - // Time Range Filter - showTimeFilter = EditorGUILayout.Foldout(showTimeFilter, "Time Range Filter", true); - if (showTimeFilter) - { - EditorGUI.indentLevel++; - - enableTimeFilter = EditorGUILayout.Toggle("Enable Time Filter", enableTimeFilter); - - if (enableTimeFilter && currentMaxTimestamp > 0) - { - EditorGUILayout.BeginHorizontal(); - EditorGUILayout.LabelField($"Min: {minTimestamp:F2}s", GUILayout.Width(100)); - EditorGUILayout.LabelField($"Max: {maxTimestamp:F2}s", GUILayout.Width(100)); - EditorGUILayout.EndHorizontal(); - - EditorGUILayout.MinMaxSlider( - ref minTimestamp, - ref maxTimestamp, - 0, - currentMaxTimestamp); - } - - EditorGUI.indentLevel--; - } - - EditorGUILayout.EndVertical(); + enableTimeFilter = enabled; + minTimestamp = min; + maxTimestamp = max; + Repaint(); + }); } private void DrawLogEntries() @@ -508,5 +457,373 @@ namespace Editor #endregion } + + /// + /// Persistent popup window for class filtering with multi-selection support + /// + public class ClassFilterWindow : EditorWindow + { + private HashSet availableTags; + private HashSet selectedTags; + private System.Action> onApply; + private Vector2 scrollPosition; + private string searchFilter = ""; + + public static void ShowWindow(CustomLogWindow parent, HashSet available, HashSet selected, + System.Action> applyCallback) + { + var window = CreateInstance(); + window.titleContent = new GUIContent("Class Filter"); + window.availableTags = available; + window.selectedTags = new HashSet(selected); + window.onApply = applyCallback; + + var parentRect = parent.position; + window.position = new Rect(parentRect.x + 50, parentRect.y + 50, 300, 400); + window.ShowUtility(); + } + + private void OnGUI() + { + EditorGUILayout.Space(5); + + // Control buttons + EditorGUILayout.BeginHorizontal(); + if (GUILayout.Button("All", GUILayout.Width(60))) + { + if (availableTags != null) + selectedTags = new HashSet(availableTags); + } + if (GUILayout.Button("None", GUILayout.Width(60))) + { + selectedTags.Clear(); + } + GUILayout.FlexibleSpace(); + searchFilter = EditorGUILayout.TextField(searchFilter, EditorStyles.toolbarSearchField, GUILayout.Width(150)); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(5); + + // Scrollable list of toggles + scrollPosition = EditorGUILayout.BeginScrollView(scrollPosition); + + if (availableTags != null && availableTags.Count > 0) + { + var filteredTags = string.IsNullOrEmpty(searchFilter) + ? availableTags.OrderBy(t => t) + : availableTags.Where(t => t.Contains(searchFilter, StringComparison.OrdinalIgnoreCase)).OrderBy(t => t); + + foreach (var tag in filteredTags) + { + bool isSelected = selectedTags.Contains(tag); + bool newSelection = EditorGUILayout.ToggleLeft(tag, isSelected); + + if (newSelection != isSelected) + { + if (newSelection) + selectedTags.Add(tag); + else + selectedTags.Remove(tag); + } + } + } + else + { + EditorGUILayout.HelpBox("No classes available", MessageType.Info); + } + + EditorGUILayout.EndScrollView(); + + EditorGUILayout.Space(5); + + // Apply/Close buttons + EditorGUILayout.BeginHorizontal(); + GUILayout.FlexibleSpace(); + + if (GUILayout.Button("Apply", GUILayout.Width(80))) + { + onApply?.Invoke(selectedTags); + Close(); + } + + if (GUILayout.Button("Close", GUILayout.Width(80))) + { + Close(); + } + + GUILayout.FlexibleSpace(); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(5); + } + } + + /// + /// Persistent popup window for method filtering with multi-selection support + /// + public class MethodFilterWindow : EditorWindow + { + private HashSet availableTags; + private HashSet selectedTags; + private System.Action> onApply; + private Vector2 scrollPosition; + private string searchFilter = ""; + + public static void ShowWindow(CustomLogWindow parent, HashSet available, HashSet selected, + System.Action> applyCallback) + { + var window = CreateInstance(); + window.titleContent = new GUIContent("Method Filter"); + window.availableTags = available; + window.selectedTags = new HashSet(selected); + window.onApply = applyCallback; + + var parentRect = parent.position; + window.position = new Rect(parentRect.x + 50, parentRect.y + 50, 300, 400); + window.ShowUtility(); + } + + private void OnGUI() + { + EditorGUILayout.Space(5); + + // Control buttons + EditorGUILayout.BeginHorizontal(); + if (GUILayout.Button("All", GUILayout.Width(60))) + { + if (availableTags != null) + selectedTags = new HashSet(availableTags); + } + if (GUILayout.Button("None", GUILayout.Width(60))) + { + selectedTags.Clear(); + } + GUILayout.FlexibleSpace(); + searchFilter = EditorGUILayout.TextField(searchFilter, EditorStyles.toolbarSearchField, GUILayout.Width(150)); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(5); + + // Scrollable list of toggles + scrollPosition = EditorGUILayout.BeginScrollView(scrollPosition); + + if (availableTags != null && availableTags.Count > 0) + { + var filteredTags = string.IsNullOrEmpty(searchFilter) + ? availableTags.OrderBy(t => t) + : availableTags.Where(t => t.Contains(searchFilter, StringComparison.OrdinalIgnoreCase)).OrderBy(t => t); + + foreach (var tag in filteredTags) + { + bool isSelected = selectedTags.Contains(tag); + bool newSelection = EditorGUILayout.ToggleLeft(tag, isSelected); + + if (newSelection != isSelected) + { + if (newSelection) + selectedTags.Add(tag); + else + selectedTags.Remove(tag); + } + } + } + else + { + EditorGUILayout.HelpBox("No methods available", MessageType.Info); + } + + EditorGUILayout.EndScrollView(); + + EditorGUILayout.Space(5); + + // Apply/Close buttons + EditorGUILayout.BeginHorizontal(); + GUILayout.FlexibleSpace(); + + if (GUILayout.Button("Apply", GUILayout.Width(80))) + { + onApply?.Invoke(selectedTags); + Close(); + } + + if (GUILayout.Button("Close", GUILayout.Width(80))) + { + Close(); + } + + GUILayout.FlexibleSpace(); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(5); + } + } + + /// + /// Persistent popup window for log level filtering with multi-selection support + /// + public class LevelFilterWindow : EditorWindow + { + private HashSet selectedLevels; + private System.Action> onApply; + + public static void ShowWindow(CustomLogWindow parent, HashSet selected, + System.Action> applyCallback) + { + var window = CreateInstance(); + window.titleContent = new GUIContent("Log Level Filter"); + window.selectedLevels = new HashSet(selected); + window.onApply = applyCallback; + + var parentRect = parent.position; + window.position = new Rect(parentRect.x + 50, parentRect.y + 50, 250, 180); + window.ShowUtility(); + } + + private void OnGUI() + { + EditorGUILayout.Space(5); + + // Control buttons + EditorGUILayout.BeginHorizontal(); + if (GUILayout.Button("All", GUILayout.Width(60))) + { + selectedLevels = new HashSet + { + LogLevel.Debug, LogLevel.Info, LogLevel.Warning, LogLevel.Error + }; + } + if (GUILayout.Button("None", GUILayout.Width(60))) + { + selectedLevels.Clear(); + } + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(10); + + // Log level toggles + foreach (LogLevel level in Enum.GetValues(typeof(LogLevel))) + { + bool isSelected = selectedLevels.Contains(level); + bool newSelection = EditorGUILayout.ToggleLeft(level.ToString(), isSelected); + + if (newSelection != isSelected) + { + if (newSelection) + selectedLevels.Add(level); + else + selectedLevels.Remove(level); + } + } + + EditorGUILayout.Space(10); + + // Apply/Close buttons + EditorGUILayout.BeginHorizontal(); + GUILayout.FlexibleSpace(); + + if (GUILayout.Button("Apply", GUILayout.Width(80))) + { + onApply?.Invoke(selectedLevels); + Close(); + } + + if (GUILayout.Button("Close", GUILayout.Width(80))) + { + Close(); + } + + GUILayout.FlexibleSpace(); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(5); + } + } + + /// + /// Popup window for configuring time range filters + /// + public class TimeRangeFilterWindow : EditorWindow + { + private bool enableTimeFilter; + private float minTimestamp; + private float maxTimestamp; + private float currentMaxTimestamp; + private System.Action onApply; + + public static void ShowWindow(CustomLogWindow parent, bool enabled, float min, float max, float currentMax, + System.Action applyCallback) + { + var window = CreateInstance(); + window.titleContent = new GUIContent("Time Range Filter"); + window.enableTimeFilter = enabled; + window.minTimestamp = min; + window.maxTimestamp = max; + window.currentMaxTimestamp = currentMax; + window.onApply = applyCallback; + + // Position near the parent window + var parentRect = parent.position; + window.position = new Rect(parentRect.x + 100, parentRect.y + 100, 350, 120); + + window.ShowUtility(); + } + + private void OnGUI() + { + EditorGUILayout.Space(10); + + enableTimeFilter = EditorGUILayout.Toggle("Enable Time Filter", enableTimeFilter); + + EditorGUILayout.Space(5); + + if (enableTimeFilter && currentMaxTimestamp > 0) + { + EditorGUILayout.BeginHorizontal(); + EditorGUILayout.LabelField($"Min: {minTimestamp:F2}s", GUILayout.Width(100)); + EditorGUILayout.LabelField($"Max: {maxTimestamp:F2}s", GUILayout.Width(100)); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.MinMaxSlider( + ref minTimestamp, + ref maxTimestamp, + 0, + currentMaxTimestamp); + + EditorGUILayout.Space(5); + + EditorGUILayout.BeginHorizontal(); + if (GUILayout.Button("Reset Range")) + { + minTimestamp = 0; + maxTimestamp = currentMaxTimestamp; + } + EditorGUILayout.EndHorizontal(); + } + else if (enableTimeFilter) + { + EditorGUILayout.HelpBox("No logs available for time filtering", MessageType.Info); + } + + EditorGUILayout.Space(10); + + EditorGUILayout.BeginHorizontal(); + GUILayout.FlexibleSpace(); + + if (GUILayout.Button("Apply", GUILayout.Width(80))) + { + onApply?.Invoke(enableTimeFilter, minTimestamp, maxTimestamp); + Close(); + } + + if (GUILayout.Button("Cancel", GUILayout.Width(80))) + { + Close(); + } + + GUILayout.FlexibleSpace(); + EditorGUILayout.EndHorizontal(); + + EditorGUILayout.Space(10); + } + } } - diff --git a/docs/custom_log_console.md b/docs/custom_log_console.md new file mode 100644 index 00000000..98015d7f --- /dev/null +++ b/docs/custom_log_console.md @@ -0,0 +1,120 @@ +ο»Ώ# Custom Log Console + +## Overview + +A centralized logging system with an advanced filtering console that automatically tags log entries with class and method names. Provides powerful filtering capabilities beyond Unity's default console, including multi-select filters, text search, time-range filtering, and log export. + +## Using the Logging System in Code + +All logging automatically captures the calling class and method name using `CallerMemberName` and `CallerFilePath` attributes. Simply call the static logging methods: + +```csharp +using Core; + +public class MyClass : ManagedBehaviour +{ + internal override void OnManagedStart() + { + Logging.Debug("Initialization complete"); + Logging.Info("Player spawned at position"); + Logging.Warning("Missing configuration, using defaults"); + Logging.Error("Failed to load required asset"); + } +} +``` + +**Output format:** +``` +[ClassName][MethodName] Your message +``` + +**Available methods:** +- `Logging.Debug(string message)` - Detailed diagnostic information +- `Logging.Info(string message)` - General informational messages +- `Logging.Warning(string message)` - Non-critical issues +- `Logging.Error(string message)` - Critical errors + +**Note:** All logs are broadcast via the `Logging.OnLogEntryAdded` event and stored in a central buffer accessible via `Logging.GetRecentLogs()`. + +--- + +## Opening the Console Window + +**Menu:** `AppleHills > Custom Log Console` + +You can open multiple independent console instances with different filter configurations to monitor separate systems simultaneously + +--- + +## Console Interface + +![Custom Log Console Interface](media/custom_console.png) + +### Toolbar Controls + +#### πŸ”΅ Basic Controls (Blue outline) +- **Clear** - Clears all log entries and resets tag lists +- **Auto-scroll** - Automatically scrolls to newest entries when enabled + +#### Filter Buttons (Persistent Popups) + +All filter buttons open persistent popup windows that remain open during multi-selection. Changes apply when you click "Apply" or dismiss with "Close". + +- **πŸ”΄ Classes Filter (Red outline)** + - Multi-select which classes to display + - Includes search box for quick filtering + - All/None quick actions + +- **🟒 Methods Filter (Green outline)** + - Multi-select which methods to display + - Includes search box for quick filtering + - All/None quick actions + +- **🟑 Levels Filter (Yellow outline)** + - Toggle Debug, Info, Warning, Error levels + - All/None quick actions + +- **⏱ Time Filter** + - Opens utility window with MinMaxSlider + - Filter logs by timestamp range + - Enable/disable toggle with reset option + +#### Search & Export +- **Search** - Full-text search across class names, method names, and message content +- **Export** - Save filtered logs to .txt file with timestamp +- **Count** - Shows `filtered/total` log count + +--- + +## Visual Indicators + +**Color Coding:** +- White: Debug/Info (normal operation) +- Yellow: Warning (non-critical issues) +- Red: Error (critical failures) + +**Alternating Rows:** Light/dark grey backgrounds improve readability for dense log output. + +--- + +## Technical Details + +**Event Broadcasting:** +```csharp +Logging.OnLogEntryAdded += (LogEntry entry) => { /* handle */ }; +``` + +**Manual Log Retrieval:** +```csharp +List recentLogs = Logging.GetRecentLogs(); +``` + +**LogEntry Structure:** +- `ClassName` - Captured from calling file path +- `MethodName` - Captured from `CallerMemberName` +- `Message` - User-provided message text +- `Level` - Debug/Info/Warning/Error enum +- `Timestamp` - Time.realtimeSinceStartup +- `FullFormattedMessage` - Complete formatted string + +--- \ No newline at end of file diff --git a/docs/media/custom_console.png b/docs/media/custom_console.png new file mode 100644 index 00000000..bea85472 Binary files /dev/null and b/docs/media/custom_console.png differ diff --git a/docs/wip/managed_behavior.md b/docs/wip/managed_behavior.md deleted file mode 100644 index f2a8e84c..00000000 --- a/docs/wip/managed_behavior.md +++ /dev/null @@ -1,849 +0,0 @@ -ο»Ώο»Ώ# ManagedBehaviour System - Architecture Review - -**Date:** November 10, 2025 -**Reviewer:** Senior System Architect -**Status:** βœ… **IMPLEMENTATION COMPLETE** -**Implementation Date:** November 10, 2025 - ---- - -## Executive Summary - -The ManagedBehaviour system is a **well-designed lifecycle orchestration framework** that successfully provides guaranteed execution order and lifecycle management for Unity MonoBehaviours. The core architecture is sound, but there are **several code quality issues** that should be addressed to improve maintainability, reduce cognitive overhead, and enhance developer experience. - -**Overall Assessment:** βœ… Good foundation, needs refinement -**Complexity Rating:** Medium-High (could be simplified) -**Developer-Friendliness:** Medium (confusion points exist) - ---- - -## βœ… IMPLEMENTATION COMPLETED (November 10, 2025) - -### Critical Issue Resolved: The `new` Keyword Pattern - -**Problem:** 16+ singleton classes used the fragile `private new void Awake()` pattern that required developers to remember to call `base.Awake()` or registration would silently fail. - -**Solution Implemented:** -1. **Sealed Awake()** - Changed from `protected virtual` to `private` - cannot be overridden -2. **New Early Hook** - Added `OnManagedAwake()` that fires during registration for early setup (singletons, GetComponent) -3. **Renamed Late Hook** - Renamed old `OnManagedAwake()` β†’ `OnManagedStart()` for clarity (mirrors Unity's Awakeβ†’Start pattern) -4. **Bulk Migration** - Updated all 40 affected files to use new pattern - -### Before & After: - -**Before (Fragile):** -```csharp -private new void Awake() -{ - base.Awake(); // CRITICAL: Must call or breaks! - _instance = this; -} - -protected override void OnManagedAwake() -{ - // Late initialization -} -``` - -**After (Foolproof):** -```csharp -protected override void OnManagedAwake() -{ - _instance = this; // Early - automatic registration happened first -} - -protected override void OnManagedStart() -{ - // Late initialization - all managers guaranteed ready -} -``` - -### Impact: -- βœ… **40 files modified** (2 core, 38 components) -- βœ… **Zero compilation errors** -- βœ… **Eliminated all fragile `new` keyword patterns** -- βœ… **Removed all "CRITICAL" comment warnings** -- βœ… **Clearer mental model** (Awakeβ†’Start pattern familiar to Unity devs) - -### Status: **READY FOR TESTING** - ---- - -## 1. System Architecture Analysis - -### Core Components - -``` -CustomBoot (Static Bootstrap) - ↓ Creates -LifecycleManager (Singleton Orchestrator) - ↓ Registers & Broadcasts to -ManagedBehaviour (Abstract Base Class) - ↓ Inherited by -Concrete Game Components (AudioManager, InputManager, etc.) -``` - -### Lifecycle Flow - -**Boot Phase:** -1. `[RuntimeInitializeOnLoadMethod]` β†’ `CustomBoot.Initialise()` -2. `LifecycleManager.CreateInstance()` (before bootstrap) -3. Components register via `Awake()` β†’ `LifecycleManager.Register()` -4. Bootstrap completes β†’ `OnBootCompletionTriggered()` -5. `BroadcastManagedAwake()` β†’ All components receive `OnManagedAwake()` in priority order - -**Scene Transition Phase:** -1. `BeginSceneLoad(sceneName)` - Batching mode activated -2. New components register during scene load β†’ Added to pending batch -3. `BroadcastSceneReady()` β†’ Process batched components, then broadcast `OnSceneReady()` - -**Save/Load Phase:** -- Scene saves: `BroadcastSceneSaveRequested()` β†’ `OnSceneSaveRequested()` -- Global saves: `BroadcastGlobalSaveRequested()` β†’ `OnGlobalSaveRequested()` -- Restores: Similar pattern with `OnSceneRestoreRequested()` and `OnGlobalRestoreRequested()` - -### βœ… What Works Well - -1. **Guaranteed Execution Order**: Priority-based sorted lists ensure deterministic execution -2. **Separation of Concerns**: Bootstrap, scene lifecycle, and save/load are clearly separated -3. **Automatic Registration**: Components auto-register in `Awake()`, reducing boilerplate -4. **Late Registration Support**: Components that spawn after boot/scene load are handled correctly -5. **Scene Batching**: Smart batching during scene load prevents premature initialization -6. **Auto-registration Features**: `AutoRegisterPausable` and `AutoRegisterForSave` reduce manual wiring - ---- - -## 2. Problematic Code & Complexity Issues - -### βœ… RESOLVED: The `new` Keyword Pattern - -**Status:** βœ… **FIXED - Implementation Complete (Nov 10, 2025)** - -**Location:** All singleton components inheriting from ManagedBehaviour (WAS in 16+ files) - -**Original Problem:** -```csharp -// OLD pattern that was used in 16+ files -private new void Awake() -{ - base.Awake(); // CRITICAL: Register with LifecycleManager! - _instance = this; -} -``` - -**Issues That Existed:** -1. **Misleading Syntax**: `new` keyword hides the base method rather than overriding it -2. **Fragile**: If a derived class forgot `base.Awake()`, registration silently failed -3. **Inconsistent**: Some files used `private new`, creating confusion -4. **Comment Dependency**: Required "CRITICAL" comments because pattern was error-prone - -**Solution Implemented:** -- βœ… Changed `ManagedBehaviour.Awake()` to `private` (sealed, cannot override) -- βœ… Added new `OnManagedAwake()` hook for early initialization -- βœ… Renamed old `OnManagedAwake()` β†’ `OnManagedStart()` for late initialization -- βœ… Migrated all 40 affected files to new pattern -- βœ… Removed all "CRITICAL" comments -- βœ… Zero compilation errors - -**New Pattern:** -```csharp -protected override void OnManagedAwake() -{ - _instance = this; // Early - singleton setup -} - -protected override void OnManagedStart() -{ - // Late - manager dependencies safe to access -} -``` - -**Result:** Pattern is now foolproof - developers cannot mess up registration. - ---- - -### 🟑 MEDIUM: Invoke Methods Bloat - -**Location:** `ManagedBehaviour.cs` lines 89-99 - -```csharp -// Public wrappers to invoke protected lifecycle methods -public void InvokeManagedAwake() => OnManagedAwake(); -public void InvokeSceneUnloading() => OnSceneUnloading(); -public void InvokeSceneReady() => OnSceneReady(); -public string InvokeSceneSaveRequested() => OnSceneSaveRequested(); -public void InvokeSceneRestoreRequested(string data) => OnSceneRestoreRequested(data); -public void InvokeSceneRestoreCompleted() => OnSceneRestoreCompleted(); -public string InvokeGlobalSaveRequested() => OnGlobalSaveRequested(); -public void InvokeGlobalRestoreRequested(string data) => OnGlobalRestoreRequested(data); -public void InvokeManagedDestroy() => OnManagedDestroy(); -public void InvokeGlobalLoadCompleted() => OnGlobalLoadCompleted(); -public void InvokeGlobalSaveStarted() => OnGlobalSaveStarted(); -``` - -**Problems:** -1. **Code Duplication**: 11 one-liner wrapper methods -2. **Maintenance Burden**: Every new lifecycle hook requires a public wrapper -3. **Leaky Abstraction**: Exposes internal lifecycle to external callers (should only be LifecycleManager) - -**Alternative Solutions:** -1. **Make lifecycle methods internal**: Use `internal virtual` instead of `protected virtual` - LifecycleManager can call directly (same assembly) -2. **Reflection**: Use reflection to invoke methods (performance cost, but cleaner API) -3. **Interface Segregation**: Break into multiple interfaces (IBootable, ISceneAware, ISaveable) - more flexible but more complex - -**Recommendation:** Use `internal virtual` for lifecycle methods. LifecycleManager and ManagedBehaviour are in the same assembly (`Core.Lifecycle` namespace), so `internal` access is perfect. This eliminates all 11 wrapper methods. - ---- - -### 🟑 MEDIUM: OnDestroy Pattern Confusion - -**Location:** Multiple derived classes - -**Current State:** Inconsistent override patterns - -```csharp -// Pattern 1: Most common (correct) -protected override void OnDestroy() -{ - base.OnDestroy(); // Unregisters from LifecycleManager - - // Custom cleanup - if (SceneManagerService.Instance != null) - SceneManagerService.Instance.SceneLoadCompleted -= OnSceneLoadCompleted; -} - -// Pattern 2: SaveLoadManager (also correct, but verbose) -protected override void OnDestroy() -{ - base.OnDestroy(); // Important: call base to unregister from LifecycleManager - - if (_instance == this) - _instance = null; -} -``` - -**Problems:** -1. **Manual Cleanup Required**: Developers must remember to call `base.OnDestroy()` -2. **No OnManagedDestroy Usage**: `OnManagedDestroy()` exists but is rarely used (only 1 reference in SceneManagerService) -3. **Redundant Comments**: Multiple files have comments reminding to call base (fragile pattern) - -**Root Cause:** `OnManagedDestroy()` is called from within `ManagedBehaviour.OnDestroy()`, but most developers override `OnDestroy()` directly instead of using `OnManagedDestroy()`. - -**Recommendation:** -- Make `OnDestroy()` sealed in `ManagedBehaviour` (or private) -- Encourage use of `OnManagedDestroy()` for all cleanup logic -- Document the difference clearly: `OnManagedDestroy()` = custom cleanup, `OnDestroy()` = framework cleanup (hands-off) - ---- - -### 🟑 MEDIUM: AutoRegisterPausable Mechanism - -**Location:** `ManagedBehaviour.cs` line 57, `LifecycleManager.cs` lines 599-609 - -```csharp -// ManagedBehaviour -public virtual bool AutoRegisterPausable => false; - -// LifecycleManager -private void HandleAutoRegistrations(ManagedBehaviour component) -{ - if (component.AutoRegisterPausable && component is AppleHills.Core.Interfaces.IPausable pausable) - { - if (GameManager.Instance != null) - { - GameManager.Instance.RegisterPausableComponent(pausable); - LogDebug($"Auto-registered IPausable: {component.gameObject.name}"); - } - } -} -``` - -**Problems:** -1. **Tight Coupling**: `LifecycleManager` has a direct dependency on `GameManager` and `IPausable` interface -2. **Hidden Dependency**: Not obvious from LifecycleManager that it depends on GameManager existing -3. **Single Purpose**: Only handles one type of auto-registration (pausable), but there could be more -4. **Unregister in Base Class**: `ManagedBehaviour.OnDestroy()` also handles pausable unregistration (split responsibility) - -**Alternative Approaches:** -1. **Event-Based**: Fire an event after `OnManagedAwake()` that GameManager listens to -2. **Reflection/Attributes**: Use attributes like `[AutoRegisterPausable]` and scan for them -3. **Remove Feature**: Components can register themselves in `OnManagedAwake()` (one line of code) - -**Recommendation:** Consider removing `AutoRegisterPausable`. It saves one line of code (`GameManager.Instance.RegisterPausableComponent(this)`) but adds complexity. Most components that implement `IPausable` will want to register anyway, and explicit is better than implicit. - ---- - -### 🟑 MEDIUM: Priority Property Repetition - -**Location:** `ManagedBehaviour.cs` lines 13-50 - -```csharp -// 6 nearly identical priority properties -public virtual int ManagedAwakePriority => 100; -public virtual int SceneUnloadingPriority => 100; -public virtual int SceneReadyPriority => 100; -public virtual int SavePriority => 100; -public virtual int RestorePriority => 100; -public virtual int DestroyPriority => 100; -``` - -**Problems:** -1. **Repetitive**: 6 properties that do essentially the same thing -2. **Overhead**: Most components only care about 1-2 priorities (usually `ManagedAwakePriority`) -3. **Cognitive Load**: Developers must understand all 6 priorities even if they only use one - -**Is This Over-Engineered?** -- **Pro**: Provides fine-grained control over each lifecycle phase -- **Con**: In practice, most components use default (100) for everything except `ManagedAwakePriority` -- **Con**: Save/Restore priorities are rarely customized (mostly manager-level components) - -**Recommendation:** Consider consolidating to 2-3 priorities: -- `Priority` (general, affects ManagedAwake, SceneReady, Save, Restore) -- `UnloadPriority` (affects SceneUnloading, Destroy - reverse order) -- Alternatively: Use attributes like `[LifecyclePriority(Phase.ManagedAwake, 20)]` for granular control only when needed - ---- - -### 🟒 MINOR: GetPriorityForList Helper Method - -**Location:** `LifecycleManager.cs` lines 639-649 - -```csharp -private int GetPriorityForList(ManagedBehaviour component, List list) -{ - if (list == managedAwakeList) return component.ManagedAwakePriority; - if (list == sceneUnloadingList) return component.SceneUnloadingPriority; - if (list == sceneReadyList) return component.SceneReadyPriority; - if (list == saveRequestedList) return component.SavePriority; - if (list == restoreRequestedList) return component.RestorePriority; - if (list == destroyList) return component.DestroyPriority; - return 100; -} -``` - -**Problems:** -1. **Brittle**: Relies on reference equality checks (works but fragile) -2. **Default Value**: Returns 100 if no match (could hide bugs) - -**Recommendation:** Use a dictionary or enum-based lookup. Better yet, if priorities are consolidated (see above), this method becomes simpler or unnecessary. - ---- - -### 🟒 MINOR: InsertSorted Performance - -**Location:** `LifecycleManager.cs` lines 620-638 - -```csharp -private void InsertSorted(List list, ManagedBehaviour component, int priority) -{ - // Simple linear insertion for now (can optimize with binary search later if needed) - int index = 0; - for (int i = 0; i < list.Count; i++) - { - int existingPriority = GetPriorityForList(list[i], list); - if (priority < existingPriority) - { - index = i; - break; - } - index = i + 1; - } - list.Insert(index, component); -} -``` - -**Problems:** -1. **O(n) insertion**: Linear search for insertion point -2. **Comment Admits It**: "can optimize with binary search later if needed" - -**Is This a Problem?** -- Probably not: Registration happens during Awake/scene load (not runtime-critical) -- Typical projects have 10-100 managed components per scene (O(n) is fine) -- Premature optimization warning: Don't fix unless proven bottleneck - -**Recommendation:** Leave as-is unless profiling shows it's a problem. Add a comment explaining why linear is acceptable. - ---- - -### 🟒 MINOR: SaveId Generation Logic - -**Location:** `ManagedBehaviour.cs` lines 70-78 - -```csharp -public virtual string SaveId -{ - get - { - string sceneName = gameObject.scene.IsValid() ? gameObject.scene.name : "UnknownScene"; - string componentType = GetType().Name; - return $"{sceneName}/{gameObject.name}/{componentType}"; - } -} -``` - -**Problems:** -1. **Runtime Allocation**: Allocates a new string every time it's accessed -2. **Mutable Path Components**: GameObject name can change at runtime (breaks save system) -3. **Collision Risk**: Two objects with same name + type in same scene = collision - -**Is This a Problem?** -- Yes: Save IDs should be stable and unique -- No: Most components override this for singletons (e.g., `"PlayerController"`) - -**Recommendation:** -1. Cache the SaveId in Awake (don't regenerate on every call) -2. Add validation warnings if GameObject name changes after registration -3. Consider GUID-based IDs for instance-based components (prefabs spawned at runtime) - ---- - -## 3. Code Style Issues - -### 🎨 Region Overuse - -**Location:** Both `ManagedBehaviour.cs` and `LifecycleManager.cs` - -**Current State:** -- `ManagedBehaviour.cs`: 6 regions (Priority Properties, Configuration, Public Accessors, Private Fields, Unity Lifecycle, Managed Lifecycle) -- `LifecycleManager.cs`: 8 regions (Singleton, Lifecycle Lists, Tracking Dictionaries, State Flags, Unity Lifecycle, Registration, Broadcast Methods, Auto-Registration, Helper Methods) - -**Opinion:** -- **Pro**: Helps organize long files -- **Con**: Regions are a code smell suggesting the file is doing too much -- **Modern Practice**: Prefer smaller, focused classes over heavily regioned classes - -**Recommendation:** Regions are acceptable for these orchestrator classes, but consider: -- Moving priority properties to a separate struct/class (`LifecyclePriorities`) -- Moving auto-registration logic to a separate service - ---- - -### 🎨 Documentation Quality - -**Overall:** βœ… Excellent! - -**Strengths:** -- Comprehensive XML comments on all public/protected members -- Clear "GUARANTEE" and "TIMING" sections in lifecycle hook docs -- Good use of examples and common patterns -- Warnings about synchronous vs async behavior - -**Minor Issues:** -1. Some comments are overly verbose (e.g., `OnSceneRestoreCompleted` has a paragraph explaining async guarantees) -2. "IMPORTANT:" and "GUARANTEE:" prefixes could be standardized - -**Recommendation:** Keep the thorough documentation style. Consider extracting complex documentation into markdown files (like you have in `docs/`) and linking to them. - ---- - -### 🎨 Naming Conventions - -**Mostly Consistent:** -- Protected methods: `OnManagedAwake()`, `OnSceneReady()` βœ… -- Invoke wrappers: `InvokeManagedAwake()`, `InvokeSceneReady()` βœ… -- Priority properties: `ManagedAwakePriority`, `SceneReadyPriority` βœ… - -**Inconsistencies:** -- `OnBootCompletionTriggered()` (passive voice) vs `BroadcastManagedAwake()` (active voice) -- `currentSceneReady` (camelCase field) vs `_instance` (underscore prefix) - -**Recommendation:** Minor cleanup pass for consistency (not critical). - ---- - -## 4. Missing Features / Potential Enhancements - -### πŸ”§ No Update/FixedUpdate Management - -**Observation:** The system manages initialization and shutdown, but not per-frame updates. - -**Question:** Should `ManagedBehaviour` provide ordered Update loops? - -**Trade-offs:** -- **Pro**: Could guarantee update order (e.g., InputManager before PlayerController) -- **Con**: Adds performance overhead (one dispatch loop per frame) -- **Con**: Unity's native Update is very optimized (hard to beat) - -**Recommendation:** Don't add unless there's a proven need. Most update-order issues can be solved with ScriptExecutionOrder settings. - ---- - -### πŸ”§ No Pause/Resume Lifecycle Hooks - -**Observation:** `IPausable` exists and is auto-registered, but there are no lifecycle hooks for pause/resume events. - -**Current State:** Components must implement `IPausable` interface and handle pause/resume manually. - -**Potential Enhancement:** -```csharp -protected virtual void OnGamePaused() { } -protected virtual void OnGameResumed() { } -``` - -**Recommendation:** Consider adding if pause/resume is a common pattern. Alternatively, components can subscribe to GameManager events in `OnManagedAwake()`. - ---- - -### πŸ”§ Limited Error Recovery - -**Observation:** If a component throws in `OnManagedAwake()`, the error is logged but the system continues. - -**Current State:** -```csharp -catch (Exception ex) -{ - Debug.LogError($"[LifecycleManager] Error in OnManagedAwake for {component.gameObject.name}: {ex}"); -} -``` - -**Trade-offs:** -- **Pro**: Resilient - one component failure doesn't crash the game -- **Con**: Silent failures can be hard to debug -- **Con**: Components might be in invalid state if initialization failed - -**Recommendation:** Consider adding: -1. A flag on component: `HasInitialized` / `InitializationFailed` -2. An event: `OnComponentInitializationFailed(component, exception)` -3. Editor-only hard failures (#if UNITY_EDITOR throw; #endif) - ---- - -## 5. Behavioral Correctness - -### βœ… Execution Order: CORRECT - -**Boot Components:** -1. All components register during their `Awake()` (sorted by AwakePriority) -2. Bootstrap completes -3. `OnBootCompletionTriggered()` β†’ `BroadcastManagedAwake()` -4. Components receive `OnManagedAwake()` in priority order (20, 25, 30, 100, etc.) - -**Late Registration (Component enabled after boot):** -1. Component's `Awake()` calls `Register()` -2. If boot complete: `OnManagedAwake()` called immediately -3. Component is added to all lifecycle lists - -**Scene Load:** -1. `BeginSceneLoad("SceneName")` - batching mode ON -2. Scene loads β†’ New components register β†’ Added to pending batch -3. `BroadcastSceneReady("SceneName")` -4. Batched components processed in priority order β†’ `OnManagedAwake()` called -5. All components (batched + existing) receive `OnSceneReady()` - -**Assessment:** βœ… Logic is sound and well-tested - ---- - -### βœ… Save/Load: CORRECT - -**Scene Save (During Transition):** -1. `BroadcastSceneSaveRequested()` iterates all components with `AutoRegisterForSave == true` -2. Calls `OnSceneSaveRequested()` β†’ Collects returned data -3. Returns `Dictionary` - -**Scene Restore:** -1. `BroadcastSceneRestoreRequested(saveData)` distributes data by SaveId -2. Calls `OnSceneRestoreRequested(data)` for matching components -3. Calls `BroadcastSceneRestoreCompleted()` β†’ All components receive `OnSceneRestoreCompleted()` - -**Global Save/Load:** Same pattern but uses `OnGlobalSaveRequested()` / `OnGlobalRestoreRequested()` - -**Assessment:** βœ… Separation of scene vs global state is clean - ---- - -### ⚠️ Unregister Timing: POTENTIAL ISSUE - -**Scenario:** Component is destroyed during `BroadcastManagedAwake()` - -**Current Protection:** -```csharp -var componentsCopy = new List(managedAwakeList); -foreach (var component in componentsCopy) -{ - if (component == null) continue; // Null check protects against destroyed objects - component.InvokeManagedAwake(); -} -``` - -**Protection Mechanism:** Copies list before iteration + null checks - -**Assessment:** βœ… Handles collection modification correctly - -**Minor Issue:** If a component is destroyed, it still remains in `managedAwakeList` copy (but null check prevents execution). The real list is cleaned up when `Unregister()` is called from `OnDestroy()`. - ---- - -### ⚠️ AutoRegisterPausable Unregister: ASYMMETRY - -**Registration:** -- Happens in `LifecycleManager.HandleAutoRegistrations()` (after `OnManagedAwake()`) - -**Unregistration:** -- Happens in `ManagedBehaviour.OnDestroy()` directly -```csharp -if (AutoRegisterPausable && this is IPausable pausable) -{ - GameManager.Instance?.UnregisterPausableComponent(pausable); -} -``` - -**Issue:** Registration and unregistration logic is split between two classes. - -**Recommendation:** Move unregistration to LifecycleManager for symmetry. Call `InvokeManagedDestroy()` before automatic cleanup. - ---- - -## 6. Developer Experience Issues - -### πŸ˜• Confusion Point: Awake vs OnManagedAwake - -**Common Question:** "When should I use `Awake()` vs `OnManagedAwake()`?" - -**Answer:** -- `Awake()`: Set singleton instance, early initialization (before bootstrap) -- `OnManagedAwake()`: Initialization that depends on other systems (after bootstrap) - -**Problem:** Requires understanding of bootstrap sequencing (not obvious to new developers) - -**Recommendation:** -1. Improve docs with flowchart diagram -2. Add validation: If component accesses `GameManager.Instance` in `Awake()`, warn that it should be in `OnManagedAwake()` - ---- - -### πŸ˜• Confusion Point: OnDestroy vs OnManagedDestroy - -**Current Usage:** Only 1 file uses `OnManagedDestroy()` (SceneManagerService) - -**Most files override `OnDestroy()`:** -```csharp -protected override void OnDestroy() -{ - base.OnDestroy(); // Must remember this! - // Custom cleanup -} -``` - -**Problem:** The `OnManagedDestroy()` hook exists but isn't being used as intended. - -**Recommendation:** -1. Make `OnDestroy()` sealed (force use of `OnManagedDestroy()`) -2. Or deprecate `OnManagedDestroy()` entirely (seems redundant) - ---- - -### πŸ˜• Confusion Point: SaveId Customization - -**Default Behavior:** `"SceneName/GameObjectName/ComponentType"` - -**Comment Says:** "Override ONLY for special cases (e.g., singletons like 'PlayerController', or custom IDs)" - -**Reality:** Many components don't realize they need custom SaveIds until save data collides. - -**Recommendation:** -1. Add editor validation: Detect duplicate SaveIds and show warnings -2. Better yet: Generate GUIDs for components that don't override SaveId -3. Document the collision risks more prominently - ---- - -## 7. Recommendations Summary - -### βœ… High Priority - IMPLEMENTED - -1. **βœ… COMPLETE: Eliminated the `new` keyword pattern:** - - βœ… Made `ManagedBehaviour.Awake()` private and sealed - - βœ… Added `protected virtual void OnManagedAwake()` hook for early initialization - - βœ… Renamed old `OnManagedAwake()` β†’ `OnManagedStart()` for clarity - - βœ… Removed all 16 instances of `private new void Awake()` pattern - - βœ… Removed all "CRITICAL: Register with LifecycleManager!" comments - - **Result:** Pattern is now foolproof - developers can't mess up registration - -2. **⏸️ DEFERRED: Seal `OnDestroy()` or deprecate `OnManagedDestroy()`:** - - Current implementation left unchanged - - Reason: Low usage of OnManagedDestroy() suggests it may not be needed - - Recommendation: Monitor usage, consider deprecation in future cleanup - -3. **⏸️ DEFERRED: Fix AutoRegister asymmetry:** - - Current implementation left unchanged - - Reason: Works correctly, low priority for immediate refactor - - Recommendation: Revisit if adding more auto-registration features - ---- - -### 🟑 Medium Priority - NOT IMPLEMENTED - -4. **Replace Invoke wrappers with `internal virtual` methods:** - - Status: Not implemented in this pass - - Reason: Would require changing method visibility, needs broader testing - - Impact: Low - existing pattern works, just verbose - -5. **Consolidate priority properties:** - - Status: Not implemented in this pass - - Reason: Requires analyzing usage patterns across all components - - Impact: Medium - would simplify API but needs careful migration - -6. **Cache SaveId:** - - Status: Not implemented in this pass - - Reason: Performance impact unclear, needs profiling first - - Impact: Low - regeneration is cheap for most use cases - ---- - -### 🟒 Low Priority (Nice to Have) - -7. **Improve developer documentation:** - - Add flowchart for lifecycle phases - - Create visual diagram of execution order - - Add common pitfalls section - -8. **Add editor validation:** - - Warn if SaveId collisions detected - - Warn if base.OnDestroy() not called - - Warn if GameManager accessed in Awake() - -9. **Performance optimization:** - - Binary search for InsertSorted (only if profiling shows need) - - Cache priority lookups - ---- - -## 8. Final Verdict & Implementation Results - -### What You Asked For: - -1. βœ… **Thorough analysis of the code** - Complete -2. βœ… **Summary of logic and expected behavior** - Confirmed correct -3. βœ… **Problematic code identification** - 7 issues found (3 high, 4 medium) -4. βœ… **Code style improvements** - Documentation, regions, naming reviewed -5. βœ… **Implementation of critical fixes** - High priority issue resolved - -### Overall Assessment: - -**Architecture:** βœ… Solid -**Implementation:** βœ… **Significantly Improved** (was ⚠️) -**Developer Experience:** βœ… **Much Better** (was ⚠️) - -The system **behaves as expected** and provides real value (guaranteed execution order, clean lifecycle hooks, save/load integration). The most critical code smell - **the fragile `new` keyword pattern** - has been **completely eliminated**. - -### Implementation Summary (November 10, 2025): - -**Files Modified:** 40 total -- 2 Core lifecycle files (ManagedBehaviour.cs, LifecycleManager.cs) -- 14 Core/Manager components -- 8 UI components -- 5 Sound components -- 4 Puzzle components -- 3 Level/Minigame components -- 2 Input components -- 2 Cinematic components -- 1 Data component -- 1 Dialogue component -- 1 Movement component -- 1 Interaction component - -**Key Changes:** -1. βœ… Sealed `Awake()` method - now private, cannot be overridden -2. βœ… New `OnManagedAwake()` hook - fires during registration for early initialization -3. βœ… Renamed `OnManagedAwake()` β†’ `OnManagedStart()` - clearer naming, mirrors Unity pattern -4. βœ… Removed all `private new void Awake()` patterns across 16 singleton classes -5. βœ… Updated all lifecycle method calls in LifecycleManager - -**Zero Compilation Errors** - All changes validated successfully - -### Is It Over-Engineered? - -**No, and it's now cleaner.** - -βœ… Sealed Awake = brilliant (prevents misuse) -βœ… OnManagedAwake/OnManagedStart split = clear mental model -⚠️ 6 priority properties = still granular but acceptable -⚠️ 11 invoke wrappers = still present but low impact -βœ… Batching system = justified -βœ… Priority-sorted lists = justified - -### Tight, Developer-Friendly, Not Over-Engineered Code: - -You're now at **95%** (was 80%). The critical fragility is gone. The remaining 5% is nice-to-have optimizations that don't impact correctness or developer experience significantly. - ---- - -## 9. Implementation Notes & Observations - -### What Went Well: - -1. **Pattern Consistency**: All 40 files followed similar patterns, making bulk updates straightforward -2. **Zero Breaking Changes**: All existing functionality preserved -3. **Improved Clarity**: New naming (`OnManagedStart`) is clearer than old naming -4. **Developer Safety**: Impossible to forget registration now - it's automatic and sealed - -### Discovered During Implementation: - -1. **Some components had `base.OnManagedAwake()` calls**: These were remnants from UI page inheritance patterns, safely removed -2. **AppleAudioSource had both `Awake` override AND `OnManagedAwake`**: Component setup was duplicated, now properly split -3. **ObjectiveStepBehaviour had orphaned `base.Awake()` call**: Cleaned up during migration -4. **All singleton setup moved to `OnManagedAwake()`**: Ensures instance is available immediately after registration - -### Testing Recommendations: - -Before committing, verify: -- βœ… Code compiles without errors (VERIFIED) -- ⏳ Game boots successfully -- ⏳ All singletons initialize properly (check console for warnings) -- ⏳ Scene transitions work correctly -- ⏳ Save/Load system functions -- ⏳ Pause system works -- ⏳ Input handling works -- ⏳ No null reference exceptions in lifecycle hooks - -### Migration Guide for Future Components: - -**Old Pattern (Don't Use):** -```csharp -private new void Awake() -{ - base.Awake(); // CRITICAL! - _instance = this; - // setup code -} - -protected override void OnManagedAwake() -{ - // late initialization -} -``` - -**New Pattern (Use This):** -```csharp -protected override void OnManagedAwake() -{ - _instance = this; // Early - singletons, GetComponent -} - -protected override void OnManagedStart() -{ - // Late - depends on other managers - SomeManager.Instance.DoSomething(); -} -``` - ---- - -## 10. Next Steps - -**Immediate:** -1. βœ… Code review - Verify changes compile -2. ⏳ **Playtesting** - Run full game loop to verify no regressions -3. ⏳ Commit changes with descriptive message -4. ⏳ Update team documentation - -**Future Cleanup (Optional):** -1. Consider removing `InvokeManagedAwake()` wrappers (use `internal virtual`) -2. Analyze priority usage, possibly consolidate properties -3. Add editor validation for SaveId collisions -4. Create visual diagram of lifecycle flow - -**Status:** βœ… **READY FOR TESTING** - -The refactoring is complete and represents a significant improvement to code quality and developer experience. The most critical issue (fragile `new` keyword pattern) has been completely eliminated across the entire codebase.