681 lines
25 KiB
Markdown
681 lines
25 KiB
Markdown
|
|
# ManagedBehaviour System - Architecture Review
|
||
|
|
|
||
|
|
**Date:** November 10, 2025
|
||
|
|
**Reviewer:** Senior System Architect
|
||
|
|
**Status:** Analysis Complete - Awaiting Implementation Decisions
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## 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)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## 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
|
||
|
|
|
||
|
|
### 🔴 CRITICAL: The `new` Keyword Pattern
|
||
|
|
|
||
|
|
**Location:** All singleton components inheriting from ManagedBehaviour
|
||
|
|
|
||
|
|
```csharp
|
||
|
|
// Current pattern in 16+ files
|
||
|
|
private new void Awake()
|
||
|
|
{
|
||
|
|
base.Awake(); // CRITICAL: Register with LifecycleManager!
|
||
|
|
_instance = this;
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Problems:**
|
||
|
|
1. **Misleading Syntax**: `new` keyword hides the base method rather than overriding it
|
||
|
|
2. **Fragile**: If a derived class forgets `base.Awake()`, registration silently fails
|
||
|
|
3. **Inconsistent**: Some files use `private new`, some use `protected override` (confusing)
|
||
|
|
4. **Comment Dependency**: Requires "CRITICAL" comments because the pattern is error-prone
|
||
|
|
|
||
|
|
**Why It's Used:**
|
||
|
|
- `ManagedBehaviour.Awake()` is marked `protected virtual`
|
||
|
|
- Singletons need to set `_instance` in `Awake()` before `OnManagedAwake()` is called
|
||
|
|
- They use `new` to hide the base `Awake()` while still calling it
|
||
|
|
|
||
|
|
**Recommendation:** Change `ManagedBehaviour.Awake()` to be `private` and non-virtual. Introduce a new virtual hook like `OnBeforeRegister()` or `OnEarlyAwake()` that runs before registration. This eliminates the need for the `new` keyword pattern.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 🟡 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 (Fix These)
|
||
|
|
|
||
|
|
1. **Eliminate the `new` keyword pattern:**
|
||
|
|
- Make `ManagedBehaviour.Awake()` private and non-virtual
|
||
|
|
- Add `protected virtual void OnPreRegister()` hook for singletons to set instances
|
||
|
|
- Reduces fragility and removes "CRITICAL" comment dependency
|
||
|
|
|
||
|
|
2. **Seal `OnDestroy()` or deprecate `OnManagedDestroy()`:**
|
||
|
|
- Current dual pattern confuses developers
|
||
|
|
- Choose one approach and enforce it
|
||
|
|
|
||
|
|
3. **Fix AutoRegister asymmetry:**
|
||
|
|
- Move unregistration to LifecycleManager for symmetry
|
||
|
|
- Or remove AutoRegisterPausable entirely (explicit > implicit)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 🟡 Medium Priority (Should Do)
|
||
|
|
|
||
|
|
4. **Replace Invoke wrappers with `internal virtual` methods:**
|
||
|
|
- Eliminates 11 one-liner methods
|
||
|
|
- Cleaner API surface
|
||
|
|
|
||
|
|
5. **Consolidate priority properties:**
|
||
|
|
- Most components only customize one priority
|
||
|
|
- Reduce to 2-3 priorities or use attributes
|
||
|
|
|
||
|
|
6. **Cache SaveId:**
|
||
|
|
- Don't regenerate on every access
|
||
|
|
- Validate uniqueness in editor
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 🟢 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
|
||
|
|
|
||
|
|
### 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
|
||
|
|
|
||
|
|
### Overall Assessment:
|
||
|
|
|
||
|
|
**Architecture:** ✅ Solid
|
||
|
|
**Implementation:** ⚠️ Needs refinement
|
||
|
|
**Developer Experience:** ⚠️ Can be improved
|
||
|
|
|
||
|
|
The system **behaves as expected** and provides real value (guaranteed execution order, clean lifecycle hooks, save/load integration). However, there are **code smell issues** that increase complexity and cognitive load:
|
||
|
|
|
||
|
|
- The `new` keyword pattern is fragile
|
||
|
|
- Invoke wrapper bloat
|
||
|
|
- Dual OnDestroy patterns
|
||
|
|
- AutoRegister coupling
|
||
|
|
|
||
|
|
These are **fixable without major refactoring**. The core architecture doesn't need to change.
|
||
|
|
|
||
|
|
### Is It Over-Engineered?
|
||
|
|
|
||
|
|
**No, but it's close to the line.**
|
||
|
|
|
||
|
|
- 6 priority properties = probably too granular (most are unused)
|
||
|
|
- 11 invoke wrappers = definitely unnecessary (use `internal virtual`)
|
||
|
|
- AutoRegisterPausable = debatable (saves 1 line of code, adds coupling)
|
||
|
|
- Batching system = justified (prevents race conditions during scene load)
|
||
|
|
- Priority-sorted lists = justified (core value proposition)
|
||
|
|
|
||
|
|
### Tight, Developer-Friendly, Not Over-Engineered Code:
|
||
|
|
|
||
|
|
You're **80% there**. The fixes I've outlined will get you to **95%**. The remaining 5% is personal preference (e.g., regions vs no regions).
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Next Steps
|
||
|
|
|
||
|
|
**Before I implement anything:**
|
||
|
|
|
||
|
|
1. Which of these issues do you want fixed? (All high priority? Some medium?)
|
||
|
|
2. Do you want me to make the changes, or just provide guidance?
|
||
|
|
3. Any architectural decisions you want to discuss first? (e.g., keep or remove AutoRegisterPausable?)
|
||
|
|
|
||
|
|
I'm ready to execute once you provide direction. 🚀
|