Add docs to the logger
This commit is contained in:
Michal Pikulski
@@ -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<string>(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<string>(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
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Persistent popup window for class filtering with multi-selection support
|
||||
/// </summary>
|
||||
public class ClassFilterWindow : EditorWindow
|
||||
{
|
||||
private HashSet<string> availableTags;
|
||||
private HashSet<string> selectedTags;
|
||||
private System.Action<HashSet<string>> onApply;
|
||||
private Vector2 scrollPosition;
|
||||
private string searchFilter = "";
|
||||
|
||||
public static void ShowWindow(CustomLogWindow parent, HashSet<string> available, HashSet<string> selected,
|
||||
System.Action<HashSet<string>> applyCallback)
|
||||
{
|
||||
var window = CreateInstance<ClassFilterWindow>();
|
||||
window.titleContent = new GUIContent("Class Filter");
|
||||
window.availableTags = available;
|
||||
window.selectedTags = new HashSet<string>(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<string>(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);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Persistent popup window for method filtering with multi-selection support
|
||||
/// </summary>
|
||||
public class MethodFilterWindow : EditorWindow
|
||||
{
|
||||
private HashSet<string> availableTags;
|
||||
private HashSet<string> selectedTags;
|
||||
private System.Action<HashSet<string>> onApply;
|
||||
private Vector2 scrollPosition;
|
||||
private string searchFilter = "";
|
||||
|
||||
public static void ShowWindow(CustomLogWindow parent, HashSet<string> available, HashSet<string> selected,
|
||||
System.Action<HashSet<string>> applyCallback)
|
||||
{
|
||||
var window = CreateInstance<MethodFilterWindow>();
|
||||
window.titleContent = new GUIContent("Method Filter");
|
||||
window.availableTags = available;
|
||||
window.selectedTags = new HashSet<string>(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<string>(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);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Persistent popup window for log level filtering with multi-selection support
|
||||
/// </summary>
|
||||
public class LevelFilterWindow : EditorWindow
|
||||
{
|
||||
private HashSet<LogLevel> selectedLevels;
|
||||
private System.Action<HashSet<LogLevel>> onApply;
|
||||
|
||||
public static void ShowWindow(CustomLogWindow parent, HashSet<LogLevel> selected,
|
||||
System.Action<HashSet<LogLevel>> applyCallback)
|
||||
{
|
||||
var window = CreateInstance<LevelFilterWindow>();
|
||||
window.titleContent = new GUIContent("Log Level Filter");
|
||||
window.selectedLevels = new HashSet<LogLevel>(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>
|
||||
{
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Popup window for configuring time range filters
|
||||
/// </summary>
|
||||
public class TimeRangeFilterWindow : EditorWindow
|
||||
{
|
||||
private bool enableTimeFilter;
|
||||
private float minTimestamp;
|
||||
private float maxTimestamp;
|
||||
private float currentMaxTimestamp;
|
||||
private System.Action<bool, float, float> onApply;
|
||||
|
||||
public static void ShowWindow(CustomLogWindow parent, bool enabled, float min, float max, float currentMax,
|
||||
System.Action<bool, float, float> applyCallback)
|
||||
{
|
||||
var window = CreateInstance<TimeRangeFilterWindow>();
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
120
docs/custom_log_console.md
Normal file
120
docs/custom_log_console.md
Normal file
@@ -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
|
||||
|
||||
|
||||
|
||||
### 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<LogEntry> 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
|
||||
|
||||
---
|
||||
BIN
docs/media/custom_console.png
Normal file
BIN
docs/media/custom_console.png
Normal file
Binary file not shown.
|
After Width: | Height: | Size: 146 KiB |
@@ -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<ManagedBehaviour> 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<ManagedBehaviour> 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<saveId, serializedData>`
|
||||
|
||||
**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<ManagedBehaviour>(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.
|
||||
Reference in New Issue
Block a user