Rework of base interactables and managed behaviors

2025-11-04 11:11:27 +01:00
parent 00e1746ac4
commit f88bd0e2c9
60 changed files with 11175 additions and 1340 deletions
--- a/docs/scene_event_trimming_analysis.md
+++ b/docs/scene_event_trimming_analysis.md
@@ -0,0 +1,399 @@
+# Scene Management Event Orchestration - Trimming Analysis
+
+**Date:** November 4, 2025  
+**Context:** Post-lifecycle migration - all components now use lifecycle hooks
+
+---
+
+## Executive Summary
+
+After migrating components to use ManagedBehaviour lifecycle hooks, **SceneManagerService events can be significantly trimmed**. Most components no longer need these events since they use lifecycle hooks instead.
+
+---
+
+## Current Event Usage Analysis
+
+### SceneManagerService Events (6 total)
+
+| Event | Current Subscribers | Can Be Removed? | Notes |
+|-------|-------------------|-----------------|-------|
+| **SceneLoadStarted** | 1 (LoadingScreen) | ⚠️ **KEEP** | Loading screen needs this |
+| **SceneLoadProgress** | 0 | ✅ **REMOVE** | No subscribers |
+| **SceneLoadCompleted** | 2 (LoadingScreen, PauseMenu) | ⚠️ **KEEP** | Still needed |
+| **SceneUnloadStarted** | 0 | ✅ **REMOVE** | No subscribers |
+| **SceneUnloadProgress** | 0 | ✅ **REMOVE** | No subscribers |
+| **SceneUnloadCompleted** | 0 | ✅ **REMOVE** | No subscribers |
+
+---
+
+## Detailed Event Analysis
+
+### 1. SceneLoadStarted ⚠️ **KEEP**
+
+**Current Usage:**
+```csharp
+// SceneManagerService.cs line 102
+SceneLoadStarted += _ => _loadingScreen.ShowLoadingScreen(() => GetAggregateLoadProgress());
+```
+
+**Why Keep:**
+- LoadingScreenController needs to know when loading begins
+- Shows loading screen with progress callback
+- No lifecycle equivalent (this is orchestration, not component lifecycle)
+
+**Action:** ✅ **KEEP**
+
+---
+
+### 2. SceneLoadProgress ✅ **REMOVE**
+
+**Current Usage:** None - no subscribers found
+
+**Why Remove:**
+- Not used anywhere in codebase
+- Progress is reported via IProgress<float> parameter in async methods
+- Loading screen gets progress via callback, not event
+
+**Action:** ✅ **REMOVE** - Dead code
+
+---
+
+### 3. SceneLoadCompleted ⚠️ **KEEP (but maybe simplify)**
+
+**Current Usage:**
+```csharp
+// SceneManagerService.cs line 103
+SceneLoadCompleted += _ => _loadingScreen.HideLoadingScreen();
+
+// PauseMenu.cs line 45
+SceneManagerService.Instance.SceneLoadCompleted += SetPauseMenuByLevel;
+```
+
+**Analysis:**
+- **LoadingScreen subscriber:** Internal orchestration - keeps loading screen hidden until scene ready
+- **PauseMenu subscriber:** Legitimate use case - needs to react to EVERY scene load to set visibility
+
+**Why PauseMenu Can't Use Lifecycle:**
+- OnSceneReady() fires only for the scene PauseMenu is IN
+- PauseMenu is in BootstrapScene (persistent)
+- Needs to know about OTHER scenes loading to update visibility
+- Event subscription is correct pattern here
+
+**Action:** ⚠️ **KEEP** - Still has legitimate uses
+
+---
+
+### 4. SceneUnloadStarted ✅ **REMOVE**
+
+**Current Usage:** None - no subscribers found
+
+**Why Remove:**
+- Components now use OnSceneUnloading() lifecycle hook
+- LifecycleManager.BroadcastSceneUnloading() replaces this
+- Event is fired but nobody listens
+
+**Action:** ✅ **REMOVE** - Dead code
+
+---
+
+### 5. SceneUnloadProgress ✅ **REMOVE**
+
+**Current Usage:** None - no subscribers found
+
+**Why Remove:**
+- Not used anywhere
+- Progress reported via IProgress<float> parameter
+
+**Action:** ✅ **REMOVE** - Dead code
+
+---
+
+### 6. SceneUnloadCompleted ✅ **REMOVE**
+
+**Current Usage:** None - no subscribers found
+
+**Why Remove:**
+- Not used anywhere
+- Components use OnSceneUnloading() before unload
+- No need to know AFTER unload completes
+
+**Action:** ✅ **REMOVE** - Dead code
+
+---
+
+## Trimming Recommendations
+
+### Option A: Aggressive Trimming (Recommended)
+
+**Remove these events entirely:**
+- ❌ SceneLoadProgress
+- ❌ SceneUnloadStarted
+- ❌ SceneUnloadProgress
+- ❌ SceneUnloadCompleted
+
+**Keep these events:**
+- ✅ SceneLoadStarted (loading screen orchestration)
+- ✅ SceneLoadCompleted (loading screen + PauseMenu cross-scene awareness)
+
+**Result:**
+- **4 events removed** (67% reduction)
+- **2 events kept** (essential for orchestration)
+- Clean separation: Lifecycle hooks for components, events for orchestration
+
+---
+
+### Option B: Conservative Approach
+
+**Remove only provably unused:**
+- ❌ SceneLoadProgress
+- ❌ SceneUnloadProgress
+
+**Mark as deprecated but keep:**
+- ⚠️ SceneUnloadStarted (deprecated - use OnSceneUnloading)
+- ⚠️ SceneUnloadCompleted (deprecated - use lifecycle)
+
+**Result:**
+- **2 events removed** (33% reduction)
+- **4 events kept** (backward compatibility)
+- Gradual migration path
+
+---
+
+### Option C: Nuclear Option (Not Recommended)
+
+**Remove ALL events, replace with callbacks:**
+```csharp
+// Instead of events, use callbacks in SwitchSceneAsync
+public async Task SwitchSceneAsync(
+    string newSceneName, 
+    IProgress<float> progress = null,
+    Action onLoadStarted = null,
+    Action onLoadCompleted = null
+)
+```
+
+**Why NOT Recommended:**
+- Breaks existing loading screen integration
+- Removes flexibility
+- Makes simple orchestration harder
+
+---
+
+## Recommended Implementation
+
+### Step 1: Remove Dead Events
+
+**File:** `SceneManagerService.cs`
+
+**Remove these declarations:**
+```csharp
+// REMOVE:
+public event Action<string, float> SceneLoadProgress;
+public event Action<string> SceneUnloadStarted;
+public event Action<string, float> SceneUnloadProgress;
+public event Action<string> SceneUnloadCompleted;
+```
+
+**Remove these invocations:**
+```csharp
+// REMOVE from LoadSceneAsync:
+SceneLoadProgress?.Invoke(sceneName, op.progress);
+
+// REMOVE from UnloadSceneAsync:
+SceneUnloadStarted?.Invoke(sceneName);
+SceneUnloadProgress?.Invoke(sceneName, op.progress);
+SceneUnloadCompleted?.Invoke(sceneName);
+```
+
+---
+
+### Step 2: Simplify Remaining Events
+
+**Keep these (minimal set):**
+```csharp
+/// <summary>
+/// Fired when a scene starts loading.
+/// Used by loading screen orchestration.
+/// </summary>
+public event Action<string> SceneLoadStarted;
+
+/// <summary>
+/// Fired when a scene finishes loading.
+/// Used by loading screen orchestration and cross-scene components (e.g., PauseMenu).
+/// For component initialization, use OnSceneReady() lifecycle hook instead.
+/// </summary>
+public event Action<string> SceneLoadCompleted;
+```
+
+---
+
+### Step 3: Update Documentation
+
+**Add to SceneManagerService class documentation:**
+```csharp
+/// <summary>
+/// Singleton service for loading and unloading Unity scenes asynchronously.
+/// 
+/// EVENTS vs LIFECYCLE HOOKS:
+/// - Events (SceneLoadStarted/Completed): For orchestration and cross-scene awareness
+/// - Lifecycle hooks (OnSceneReady, OnSceneUnloading): For component-level scene initialization
+/// 
+/// Most components should use lifecycle hooks, not event subscriptions.
+/// </summary>
+```
+
+---
+
+## Impact Analysis
+
+### Before Trimming
+```csharp
+public class SceneManagerService : ManagedBehaviour
+{
+    // 6 events
+    public event Action<string> SceneLoadStarted;
+    public event Action<string, float> SceneLoadProgress;      // ← UNUSED
+    public event Action<string> SceneLoadCompleted;
+    public event Action<string> SceneUnloadStarted;            // ← UNUSED
+    public event Action<string, float> SceneUnloadProgress;    // ← UNUSED
+    public event Action<string> SceneUnloadCompleted;          // ← UNUSED
+}
+```
+
+### After Trimming
+```csharp
+public class SceneManagerService : ManagedBehaviour
+{
+    // 2 events (essential orchestration only)
+    public event Action<string> SceneLoadStarted;
+    public event Action<string> SceneLoadCompleted;
+}
+```
+
+### Metrics
+- **Events removed:** 4 out of 6 (67%)
+- **Event invocations removed:** ~8 call sites
+- **Lines of code removed:** ~15-20 lines
+- **Cognitive complexity:** Reduced significantly
+- **Maintenance burden:** Lower
+
+---
+
+## Migration Path for Future Components
+
+### ❌ DON'T: Subscribe to scene events
+```csharp
+public class MyComponent : ManagedBehaviour
+{
+    private void Start()
+    {
+        SceneManagerService.Instance.SceneLoadCompleted += OnSceneLoaded;
+    }
+}
+```
+
+### ✅ DO: Use lifecycle hooks
+```csharp
+public class MyComponent : ManagedBehaviour
+{
+    protected override void OnSceneReady()
+    {
+        // Scene is ready, do initialization
+    }
+}
+```
+
+### ✅ EXCEPTION: Cross-scene awareness
+```csharp
+// Only if component needs to know about OTHER scenes loading
+// (e.g., PauseMenu in BootstrapScene reacting to gameplay scenes)
+public class CrossSceneComponent : ManagedBehaviour
+{
+    protected override void OnSceneReady()
+    {
+        SceneManagerService.Instance.SceneLoadCompleted += OnOtherSceneLoaded;
+    }
+    
+    private void OnOtherSceneLoaded(string sceneName)
+    {
+        // React to OTHER scenes loading
+    }
+}
+```
+
+---
+
+## Code Quality Improvements
+
+### Before (Complex Event Mesh)
+```
+Component A ──┐
+Component B ──┼──→ SceneLoadProgress (unused)
+Component C ──┘
+
+Component D ──┐
+Component E ──┼──→ SceneUnloadStarted (unused)
+Component F ──┘
+
+Component G ──┐
+Component H ──┼──→ SceneUnloadProgress (unused)
+Component I ──┘
+
+Component J ──┐
+Component K ──┼──→ SceneUnloadCompleted (unused)
+Component L ──┘
+```
+
+### After (Clean Separation)
+```
+LoadingScreen ──→ SceneLoadStarted    ✓ (orchestration)
+LoadingScreen ──→ SceneLoadCompleted  ✓ (orchestration)
+PauseMenu ─────→ SceneLoadCompleted   ✓ (cross-scene)
+
+All other components → Use lifecycle hooks (OnSceneReady, OnSceneUnloading)
+```
+
+---
+
+## Testing Checklist
+
+After implementing trimming:
+
+- [ ] Verify loading screen shows/hides correctly
+- [ ] Verify PauseMenu visibility updates per scene
+- [ ] Test scene transitions work smoothly
+- [ ] Check no compilation errors
+- [ ] Grep for removed event names (ensure no orphaned subscribers)
+- [ ] Run full playthrough
+
+---
+
+## Recommendation: **Option A (Aggressive Trimming)**
+
+**Rationale:**
+1. ✅ 67% reduction in events (4 removed)
+2. ✅ No breaking changes (removed events had no subscribers)
+3. ✅ Clearer architecture (events for orchestration, lifecycle for components)
+4. ✅ Easier to maintain going forward
+5. ✅ Matches lifecycle system philosophy
+
+**Estimated Time:** 15-20 minutes
+
+**Risk Level:** 🟢 **LOW** - Removed events have zero subscribers
+
+---
+
+## Next Steps
+
+1. **Remove dead events** from SceneManagerService (10 min)
+2. **Update documentation** in SceneManagerService (5 min)
+3. **Grep search** to verify no orphaned subscribers (2 min)
+4. **Test scene transitions** (5 min)
+
+**Total: ~22 minutes**
+
+---
+
+**Analysis Complete**  
+**Recommendation:** Proceed with aggressive trimming (Option A)
+