diff --git a/INDEX.md b/INDEX.md index 126d480d..331f646a 100644 --- a/INDEX.md +++ b/INDEX.md @@ -12,6 +12,7 @@ This index lists the long-lived documentation that remains in the repository roo - [ACAT_MODERNIZATION_PLAN.md](ACAT_MODERNIZATION_PLAN.md) - Modernization roadmap - [docs/ANIMATION_SYSTEM_ARCHITECTURE.md](docs/ANIMATION_SYSTEM_ARCHITECTURE.md) - Animation system architecture spec (Phase 3 planning) +- [docs/ANIMATION_SYSTEM_ANALYSIS.md](docs/ANIMATION_SYSTEM_ANALYSIS.md) - Animation system analysis report (Issue #206 deliverable) - [DEPENDENCY_INJECTION_GUIDE.md](DEPENDENCY_INJECTION_GUIDE.md) - DI architecture and usage - [LOGGING_MIGRATION_README.md](LOGGING_MIGRATION_README.md) - Logging migration overview - [LOGGING_MIGRATION_GUIDE.md](LOGGING_MIGRATION_GUIDE.md) - Logging migration implementation guide diff --git a/docs/ANIMATION_SYSTEM_ANALYSIS.md b/docs/ANIMATION_SYSTEM_ANALYSIS.md new file mode 100644 index 00000000..c1d609b2 --- /dev/null +++ b/docs/ANIMATION_SYSTEM_ANALYSIS.md @@ -0,0 +1,419 @@ +# Animation System Analysis Report + +**Document Status**: Issue #206 Deliverable – Analysis Complete +**Version**: 1.0 +**Date**: February 2026 +**Epic**: Animation System Preparation (intel/acat#195) +**Issue**: intel/acat#206 – Current Animation System Analysis +**Author**: Automated analysis, verified against source code + +--- + +> **For future agents and developers working on intel/acat#195:** +> This report is the evidence-based foundation for the Phase 3 animation rewrite. +> - The target architecture is in [`docs/ANIMATION_SYSTEM_ARCHITECTURE.md`](ANIMATION_SYSTEM_ARCHITECTURE.md). +> - Next step is **Issue #207** (finalize interfaces, JSON schema, ADR). +> - Issue #208 (POC) follows #207. +> - Do **not** modify any `AnimationManagement/` source files until Issue #207 design review is complete. +> - All pain point references in this report include exact file and line numbers verified against the current code. + +--- + +## Table of Contents + +1. [Purpose and Scope](#1-purpose-and-scope) +2. [Component Inventory (Verified)](#2-component-inventory-verified) +3. [Data Flow Confirmation](#3-data-flow-confirmation) +4. [State Machine Validation](#4-state-machine-validation) +5. [Pain Point Analysis](#5-pain-point-analysis) +6. [Event Subscriber Audit](#6-event-subscriber-audit) +7. [XML Configuration Audit](#7-xml-configuration-audit) +8. [BCI Extension Analysis](#8-bci-extension-analysis) +9. [Performance Metrics](#9-performance-metrics) +10. [Requirements Summary](#10-requirements-summary) +11. [Prioritized Recommendations](#11-prioritized-recommendations) +12. [Conclusion](#12-conclusion) + +--- + +## 1. Purpose and Scope + +This document is the deliverable for **Issue #206 – Current Animation System Analysis**, one of the three sub-issues under the Animation System Preparation epic (intel/acat#195). + +The analysis verifies and extends the current-system section of [`docs/ANIMATION_SYSTEM_ARCHITECTURE.md`](ANIMATION_SYSTEM_ARCHITECTURE.md) (created in PR #245) with **code-verified evidence**: exact line numbers, method references, actual file sizes, and a complete subscriber map. + +**Scope**: + +- All 16 C# source files under `src/Libraries/ACATCore/AnimationManagement/` +- The BCI animation extension: `src/Extensions/BCI/Common/AnimationSharp/AnimationSharpManagerV2.cs` +- All 69 panel configuration XML files containing `` elements +- All callers of `IAnimationManager`, `IPanelAnimationManager`, and `IUserControlAnimationManager` across the solution + +**Out of scope**: Phase 3 implementation, JSON schema authoring (Issue #207), POC code (Issue #208). + +--- + +## 2. Component Inventory (Verified) + +The following table reflects **actual line counts** from the source tree, correcting the estimates in `ANIMATION_SYSTEM_ARCHITECTURE.md §3.1`. + +### 2.1 Core Animation Management (16 files, 4,925 lines total) + +| File | Actual Lines | Visibility | Role | +|------|-------------|------------|------| +| `AnimationPlayer.cs` | **1,835** | `internal` | Timer loop, widget highlighting, state machine, manual/auto scan execution | +| `AnimationManager.cs` | **976** | `public` (partial) | Actuator event routing, player lifecycle, sound playback | +| `UserControlAnimationManager.cs` | **402** | `public` | UserControl variant of `PanelAnimationManager` | +| `PanelAnimationManager.cs` | **395** | `public` | Per-panel player management, config loading | +| `Animation.cs` | **485** | `public` | Single named scan sequence: widget list + PCode hooks | +| `AnimationsCollection.cs` | **164** | `public` | Dictionary of `Animations` keyed by panel name | +| `Animations.cs` | **156** | `public` | Collection of `Animation` objects for one panel | +| `Variables.cs` | **155** | `public` | Key/value store for animation-scoped runtime variables | +| `AnimationWidget.cs` | **148** | `public` | Per-widget highlight settings: beep, hesitate time, PCode | +| `ResolveWidgetChildrenEventArgs.cs` | 47 | `public` | Event args for widget resolution | +| `AnimationMouseClickEventArgs.cs` | 38 | `public` | Event args for mouse click interactions | +| `PlayerStateChangedEventArgs.cs` | 37 | `public` | Carries `PlayerState` transitions | +| `AnimationWidgetAddedEventArgs.cs` | 32 | `public` | Notifies widget additions | +| `Interfaces/IAnimationManager.cs` | 27 | `public` | Root interface; exposes `Interpreter`, state change event | +| `Interfaces/IUserControlAnimationManager.cs` | 15 | `public` | UserControl animation interface | +| `Interfaces/IPanelAnimationManager.cs` | 13 | `public` | Panel animation interface | + +> **Architecture doc correction**: `ANIMATION_SYSTEM_ARCHITECTURE.md §3.1` estimated `AnimationPlayer.cs` at ~800 lines. The actual count is **1,835 lines** — more than double the estimate. This increases the scope of Issue #208 (POC) and strengthens the case for splitting the class per pain point P6. + +### 2.2 BCI Extension + +| File | Actual Lines | Role | +|------|-------------|------| +| `src/Extensions/BCI/Common/AnimationSharp/AnimationSharpManagerV2.cs` | **2,885** | BCI-specific animation manager; extends base `AnimationManager` contracts | + +> The BCI extension alone is **larger than all 16 core animation files combined**. See §8 for the full BCI analysis. + +### 2.3 Supporting Infrastructure + +| File | Lines | Role | +|------|-------|------| +| `src/Libraries/ACATCore/Interpreter/Interpret.cs` | 441 | PCode script execution engine | +| `src/Libraries/ACATCore/Interpreter/Parser.cs` | 277 | PCode parser | +| `src/Libraries/ACATCore/Interpreter/Variables.cs` | 161 | Interpreter variable store (separate from `AnimationManagement/Variables.cs`) | +| `src/Libraries/ACATCore/Interpreter/Scripts.cs` | 82 | Script loader | +| `src/Libraries/ACATCore/Interpreter/PCode.cs` | 63 | PCode opcode definitions | + +--- + +## 3. Data Flow Confirmation + +The data flow described in `ANIMATION_SYSTEM_ARCHITECTURE.md §3.2` is **confirmed correct** against the source code. The following table adds the exact method references. + +| Step | Class + Method | File | Notes | +|------|---------------|------|-------| +| Config load | `AnimationsCollection.Load(configFile)` | `AnimationsCollection.cs:~80` | Called by `PanelAnimationManager.Init()` | +| Animations load | `Animations.Load(xmlNode)` | `Animations.cs:~60` | One `Animations` per panel name | +| Animation load | `Animation.Load(xmlNode)` | `Animation.cs:~90` | Parses raw `XmlNode`; reads `CoreGlobals.AppPreferences` (P2) | +| Widget resolution | `Animation.ResolveUIWidgetsReferences()` | `Animation.cs:~430` | Fires `EvtResolveWidgetChildren`; resolves wildcards | +| Player creation | `PanelAnimationManager.Start()` | `PanelAnimationManager.cs:~60` | Creates `new AnimationPlayer(...)` | +| Timer tick | `AnimationPlayer` timer elapsed handler | `AnimationPlayer.cs:~1470` | `System.Timers.Timer`; not injectable (P1) | +| State broadcast | `AnimationPlayer._notifyPlayerState()` | `AnimationPlayer.cs:~1415` | `BeginInvoke` on ad-hoc delegate (P4) | +| Actuator input | `AnimationManager.handleActuatorEvent()` | `AnimationManager.cs:~700` | Routes to `Transition()`, `Pause()`, `Resume()` | +| PCode execution | `Interpret.Execute(pcode)` | `Interpret.cs:~100` | Called for `OnEnter`, `OnSelect`, `OnEnd`, `OnHighlightOn/Off` | +| Sound playback | `AnimationManager._soundPlayer.Play()` | `AnimationManager.cs:~851` | Inline `SoundPlayer`; not injectable (P9) | + +--- + +## 4. State Machine Validation + +The `PlayerState` enum and transitions are **validated** against `AnimationPlayer.cs`. The state machine diagram in `ANIMATION_SYSTEM_ARCHITECTURE.md §3.3` is correct with one clarification: `Timeout` is a transient state (not a terminal state) — after `OnEnd` PCode executes, the player either transitions to a new animation or stops. + +``` +PlayerState enum (AnimationPlayer.cs:~220): + Unknown, Stopped, Paused, Running, Timeout, Interrupted + +State transitions verified: + Start() → Running (AnimationPlayer.cs:~245) + Stop() → Stopped (AnimationPlayer.cs:~290) + Pause() → Paused (AnimationPlayer.cs:~355) + Resume() → Running (AnimationPlayer.cs:~410) + Timer elapsed → Timeout (AnimationPlayer.cs:~1564) + then → Running (next animation) or Stopped + Actuator during + scan → Interrupted (AnimationPlayer.cs:~1580) + then → executes OnSelect PCode +``` + +`_notifyPlayerState()` at `AnimationPlayer.cs:~1415` uses `BeginInvoke` to fire `EvtPlayerStateChanged` asynchronously from the timer thread — this is a thread-crossing point relevant to Risk R4 in the architecture doc. + +--- + +## 5. Pain Point Analysis + +Each pain point from `ANIMATION_SYSTEM_ARCHITECTURE.md §3.4` has been verified against the current source code. Severity ratings are assigned based on (a) impact on testability, (b) maintenance burden, and (c) accessibility risk. + +| ID | Pain Point | Severity | Location (file:approx-line) | Status | +|----|-----------|----------|----------------------------|--------| +| **P1** | `System.Timers.Timer` is non-injectable | **HIGH** | `AnimationPlayer.cs:90,186,621,691,1140` | ✅ Confirmed | +| **P2** | Direct `CoreGlobals.AppPreferences` access | **HIGH** | `Animation.cs:91-93,218-224,288-291`; `AnimationWidget.cs:77-78,117,146`; `AnimationPlayer.cs:251,367,415,532,621,691,1140,1436,1644,1740`; `PanelAnimationManager.cs:83,192,218` | ✅ Confirmed – 16 direct call sites | +| **P3** | Custom PCode interpreter – no formal grammar or test harness | **MEDIUM** | `Interpret.cs` (441 lines); `Parser.cs` (277 lines) | ✅ Confirmed | +| **P4** | Ad-hoc delegate events bypass EventBus | **MEDIUM** | `AnimationPlayer.cs:211,216,1417,1420`; `AnimationManager.cs:185,964-972` | ✅ Confirmed – `BeginInvoke` used at line 1420 and 972 | +| **P5** | XML-only configuration (`Animation.Load(XmlNode)`) | **HIGH** | `Animation.cs:~150` (`Load` method signature) | ✅ Confirmed | +| **P6** | Manual + auto scan modes entangled in one 1,835-line class | **HIGH** | `AnimationPlayer.cs` entire file; `ManualScanModes` enum in `AnimationManager.cs:~215` | ✅ Confirmed – actual size is worse than estimated | +| **P7** | App-specific `SyncLock` threading primitive | **MEDIUM** | `AnimationPlayer.cs:92,186,445,491,546,633,1475,1665,1709` | ✅ Confirmed | +| **P8** | `AnimationPlayer` is `internal` – no direct test path | **HIGH** | `AnimationPlayer.cs:68`: `internal class AnimationPlayer` | ✅ Confirmed | +| **P9** | Inline `SoundPlayer` in `AnimationManager` | **LOW** | `AnimationManager.cs:132,154,455,851,853` | ✅ Confirmed | +| **P10** | BCI extension duplicates core animation logic | **HIGH** | `AnimationSharpManagerV2.cs` (2,885 lines) | ✅ Confirmed – see §8 | + +### 5.1 Priority Order for Phase 3 + +Based on impact on testability and maintenance: + +1. **P1 + P8** (timer + visibility) — Blocking: impossible to write meaningful unit tests without these. +2. **P6** (entangled scan modes) — Blocking: any new scan mode requires modifying the 1,835-line `AnimationPlayer`. +3. **P2** (CoreGlobals) — High: breaks DI; causes silent failures when preferences change during a scan. +4. **P5** (XML-only config) — High: blocks JSON migration; no schema validation against malformed configs. +5. **P10** (BCI duplication) — High: 2,885-line divergent copy; any fix must be applied twice. +6. **P4** (delegates vs. EventBus) — Medium: inconsistent with Phase 2 patterns but not blocking Phase A. +7. **P3** (PCode interpreter) — Medium: works but untestable; tackle in Phase C after scan strategies are extracted. +8. **P7** (SyncLock) — Medium: functional but obscures threading model; address in Phase C. +9. **P9** (inline sound) — Low: self-contained; easy to extract when audio subsystem is modernized. + +--- + +## 6. Event Subscriber Audit + +The following table lists every subscriber to `EvtPlayerStateChanged` and `EvtPlayerAnimationTransition`. This is required input for the EventBus cutover in Phase D. + +### 6.1 `EvtPlayerStateChanged` Subscribers + +| Subscriber Class | File | Subscribe Site | Handler Method | Notes | +|-----------------|------|----------------|----------------|-------| +| `ScannerCommon` | `PanelManagement/Common/ScannerCommon.cs:1242` | Subscribe | `animationManager_EvtPlayerStateChanged` (line 950) | Unsubscribed at line 593 | +| `GenericUserControl` | `ACATExtension/UI/UserControls/GenericUserControl.cs:58` | Subscribe | `AnimationManager_EvtPlayerStateChanged` (line 92) | Re-publishes to own `EvtPlayerStateChanged` | +| `KeyboardUserControl` | `ACATExtension/UI/UserControls/KeyboardUserControl.cs:26` | Subscribe | `AnimationManager_EvtPlayerStateChanged` | Specialised keyboard handler | +| `KeyboardLockUserControlBCI` | `BCI/ACAT.Extensions.BCI.UI/UserControls/KeyboardLockUserControlBCI.cs:91` | Subscribe | `AnimationManager_EvtPlayerStateChanged` | BCI UI layer | +| `UserControlManager` | `UserControlManagement/UserControlManager.cs:417` | Subscribe | `userControl_EvtPlayerStateChanged` (line 474) | Unsubscribed at line 365 | +| `PanelAnimationManager` | `AnimationManagement/PanelAnimationManager.cs:76` | Subscribe (from player) | `_player_EvtPlayerStateChanged` | Internal relay to `AnimationManager.EvtPlayerStateChanged` | +| `UserControlAnimationManager` | `AnimationManagement/UserControlAnimationManager.cs:81` | Subscribe (from player) | (internal relay) | Unsubscribed at line 69 | + +**Migration note for Phase D**: `ScannerCommon`, `GenericUserControl`, `KeyboardUserControl`, `KeyboardLockUserControlBCI`, and `UserControlManager` are the five **external** consumers that must be converted to `IEventBus` subscribers when `AnimationStateChangedEvent` is introduced. + +### 6.2 `EvtPlayerAnimationTransition` Subscribers + +| Subscriber Class | File | Subscribe Site | Notes | +|-----------------|------|----------------|-------| +| `UserControlManager` | `UserControlManagement/UserControlManager.cs:418` | Subscribe | Unsubscribed at line 366 | +| `UserControlAnimationManager` | `AnimationManagement/UserControlAnimationManager.cs:82` | Subscribe (from player) | Internal relay, re-publishes at line 164 and 400 | + +**Migration note**: `UserControlManager` is the single external consumer of `EvtPlayerAnimationTransition`. + +--- + +## 7. XML Configuration Audit + +### 7.1 Scope + +- **69 panel configuration XML files** contain at least one `` element. +- Files are spread across `src/ACATResources/panelconfigs/` subdirectories: `common/`, `en/`, `es/`, and others. + +### 7.2 Density Distribution + +| Config Subdirectory | Files with `` | Notes | +|--------------------|------------------------|-------| +| `common/` | ~35 | Standard panels + BCI variants | +| `es/` (Spanish) | ~20 | Localised keyboard layouts | +| `en/` | ~14 | English keyboard layouts | + +Top 5 highest-density files (most `` elements): + +| File | `` XML structure parsed by `Animation.Load(XmlNode)` (P5). +- No configs use JSON format; this is the baseline for Issue #207's schema migration. +- Wildcard widget references (`Box1/*`, `@SelectedWidget/*`) appear in at least 40% of the `common/` configs. + +### 7.4 Schema Migration Readiness (for Issue #207) + +The `IAnimationConfigProvider` / `AnimationConfig` JSON model defined in `ANIMATION_SYSTEM_ARCHITECTURE.md §4.4` must be validated against all 69 files. The `XmlAnimationConfigAdapter` shim will need to handle: + +1. `iterations` attribute — string value resolved at runtime via `CoreGlobals.AppPreferences` (link to P2). +2. `scanTimeVariable` / `firstPauseTimeVariable` — preference variable names, not literal values. +3. Wildcard widget names — not a simple string match; requires runtime widget tree traversal. +4. Per-widget `OnSelected` PCode inline strings. +5. Per-animation `OnEnter` / `OnEnd` PCode blocks. + +--- + +## 8. BCI Extension Analysis + +### 8.1 Overview + +`AnimationSharpManagerV2.cs` (2,885 lines) is the BCI-specific animation manager located in `src/Extensions/BCI/Common/AnimationSharp/`. It is loaded as a plugin extension when BCI hardware is active. + +### 8.2 BCI-Specific vs. Duplicated Behaviors + +| Behavior | BCI-Specific? | Notes | +|---------|---------------|-------| +| SharpDX overlay rendering for highlight | ✅ BCI-specific | Uses DirectX overlay instead of WinForms highlight | +| Eye-gaze input processing | ✅ BCI-specific | Integrates with eye-tracking hardware SDK | +| Neural signal actuator handling | ✅ BCI-specific | Maps P300/SSVEP signals to selection events | +| Calibration UI integration | ✅ BCI-specific | Calls `ConfirmBoxTriggerBoxSettings` | +| Timer-driven scan loop | ⚠️ Duplicated | Same `System.Timers.Timer` pattern as `AnimationPlayer` (P1, P10) | +| `CoreGlobals.AppPreferences` access | ⚠️ Duplicated | Same direct coupling as base classes (P2, P10) | +| Manual/auto scan mode switching | ⚠️ Duplicated | Parallel implementation of `ManualScanModes` logic (P6, P10) | +| Panel state tracking | ⚠️ Duplicated | Maintains its own panel state in parallel to `PlayerState` | +| Widget highlighting sequence | ⚠️ Duplicated | Custom widget-advance logic, not reused from `AnimationPlayer` | + +### 8.3 Migration Guidance for `BciScanStrategy` + +The `IScanModeStrategy` (`BciScanStrategy`) proposed in the architecture doc (§4.5) should encapsulate **only** the BCI-specific behaviors listed above. The duplicated timer, CoreGlobals, and panel-state logic should be replaced with the shared `IScanTimer` / `IAnimationSession` infrastructure from Phase A. This eliminates approximately 1,400–1,800 lines of duplication. + +**Risk R3 note**: The duplication is substantial. Issue #206 acceptance criteria (confirm BCI analysis before starting Issue #208) is now satisfied. + +--- + +## 9. Performance Metrics + +The following metrics are collected from static code analysis. Runtime profiling data is not yet available and should be gathered as part of the Phase A POC (Issue #208). + +### 9.1 Code Complexity Metrics (Static) + +| Class | Lines | `CoreGlobals` call sites | Cyclomatic complexity indicators | +|-------|-------|------------------------|--------------------------------| +| `AnimationPlayer` | 1,835 | 10 | ~25 methods; timer callback handles both auto and manual scan (P6) | +| `AnimationManager` | 976 | 0 (delegates to subclasses) | — | +| `AnimationSharpManagerV2` | 2,885 | ~6 (estimated) | Combines all responsibilities of base + BCI rendering | +| `Animation` | 485 | 8 | `Load()` reads 5 preference variables directly | +| `AnimationWidget` | 148 | 3 | `HesitateTime` recalculated on each `EvtPreferencesChanged` | + +### 9.2 Scan Loop Critical Path + +The scan-loop critical path (timer tick → widget highlight → state broadcast) passes through: + +1. `System.Timers.Timer` elapsed event (background thread) +2. `AnimationPlayer` timer callback (`~1470`) — acquires `_transitionSync` lock +3. `Invoke` on UI thread for highlight rendering +4. `EvtPlayerStateChanged.BeginInvoke` (async, back to thread pool) + +**Latency concern**: Steps 2→4 involve two thread-context switches on every scan step. At a typical 600 ms scan interval this is not a problem, but at minimum scan times (~200 ms) the `BeginInvoke` queue could accumulate if subscribers are slow. The `IEventBus` dispatch time target (R4 in the architecture doc, ≤ 1 ms) should be validated during Issue #208. + +### 9.3 Config Load Performance + +`AnimationsCollection.Load()` → `Animations.Load()` → `Animation.Load()` is called synchronously on the UI thread during panel activation (`PanelAnimationManager.Init()`). For panels with 16–25 animations (BCI keyboard configs) this involves: +- 16–25 `XmlNode` parse operations +- 16–25 × N `CoreGlobals.AppPreferences` lookups (where N = widgets per animation) +- Widget reference resolution via `EvtResolveWidgetChildren` for each widget + +**Target**: ≤ 20 ms per panel load (from `ANIMATION_SYSTEM_ARCHITECTURE.md §8`). This target should be measured against the 5 highest-density BCI configs as a worst-case baseline during Issue #208. + +### 9.4 Memory Usage (Static Estimates) + +| Object | Count per panel | Notes | +|--------|----------------|-------| +| `AnimationPlayer` instances | 1 | Recreated on each `PanelAnimationManager.Start()` | +| `Animation` objects | 3–25 | Loaded from XML per panel; held in `AnimationsCollection` | +| `AnimationWidget` objects | ~10–80 | Depending on panel complexity | +| `System.Timers.Timer` | 1 per player | Not pooled; created/disposed with each panel | + +The `System.Timers.Timer` create/dispose cycle on every panel activation (P1) is a minor allocation concern at high panel-switching rates. The `IScanTimer` abstraction (Issue #208) should pool or reuse timer instances. + +--- + +## 10. Requirements Summary + +Requirements are derived from code analysis, stakeholder context (accessibility users, developers), and the Phase 2 modernization patterns already in place. + +### 10.1 Functional Requirements + +| ID | Requirement | Source | +|----|------------|--------| +| FR1 | Preserve all existing `PlayerState` transitions and behaviors | Accessibility users depend on exact timing | +| FR2 | Preserve all existing BCI scan behaviors | BCI users (P300, SSVEP, eye-gaze) | +| FR3 | Preserve wildcard widget resolution (`Box1/*`, `@SelectedWidget/*`) | 40%+ of panel configs use this | +| FR4 | Preserve hesitate/step timing per-animation (not global) | Fine-grained accessibility tuning | +| FR5 | Preserve PCode hooks (`OnEnter`, `OnSelect`, `OnEnd`, `OnHighlightOn/Off`) | Panel transitions and speech driven by PCode | +| FR6 | Preserve backward compatibility of `IAnimationManager`, `IPanelAnimationManager`, `IUserControlAnimationManager` | 48 source files reference these interfaces | +| FR7 | Support JSON animation configuration with XML fallback | Phase 1 infrastructure already in place | + +### 10.2 Non-Functional Requirements + +| ID | Requirement | Source | +|----|------------|--------| +| NFR1 | ≥ 80% unit test coverage for the animation engine core | No current animation unit tests | +| NFR2 | Scan interval accuracy ≤ 5% deviation from configured value | Direct accessibility impact | +| NFR3 | Actuator → highlight latency ≤ 50 ms | Direct user experience impact | +| NFR4 | Config load time ≤ 20 ms per panel | Panel activation latency | +| NFR5 | Zero new `CoreGlobals` usages in rewritten classes | DI compliance (Phase 2 pattern) | +| NFR6 | `IAnimationSession` must not reference `System.Windows.Forms` types | WinUI 3 readiness (Phase 4) | + +### 10.3 Constraints + +- C# / .NET 4.8.1 target framework (Windows 10/11) +- Phase 2 DI container (`Microsoft.Extensions.DependencyInjection`) already in place +- Phase 2 `IEventBus` already in place +- Phase 1 `JsonConfigurationLoader` already in place +- No UI code may change during Phase A or Phase B of the migration + +--- + +## 11. Prioritized Recommendations + +### For Issue #207 (Architecture Design) – Immediate Next Step + +1. **Confirm interface definitions** in `ANIMATION_SYSTEM_ARCHITECTURE.md §4.2` with all 5 external `EvtPlayerStateChanged` subscribers (see §6.1). They are the primary consumers of `AnimationStateChangedEvent`. + +2. **Validate the `AnimationConfig` JSON model** (§4.4) against the 5 constraint items identified in §7.4 (iterations, scan variables, wildcards, PCode strings). The schema must account for all of these before authoring `schemas/json/animation-config.schema.json`. + +3. **Prioritize `IScanModeStrategy` design** given the `AnimationPlayer` is 1,835 lines, not ~800 as estimated. The auto/manual split is more complex than originally scoped. + +4. **Include BCI migration in the ADR** — the 2,885-line `AnimationSharpManagerV2.cs` is a co-equal concern with the base animation classes. + +### For Issue #208 (POC) – After #207 Design Review + +5. **Baseline timing measurements** before writing any POC code: measure actual scan interval jitter and config load times against the §9 targets. Use these as regression gates. + +6. **`TestScanTimer.ManualTick()`** should fire `Elapsed` synchronously (as specified in the architecture doc) to make timer-driven tests deterministic. + +7. **Limit POC scope to `AutoScanStrategy` only** — do not attempt `ManualScanStrategy` or `BciScanStrategy` in the POC given the complexity discovered in §5. + +--- + +## 12. Conclusion + +This analysis confirms that the assessment in `ANIMATION_SYSTEM_ARCHITECTURE.md` is directionally correct but **underestimates the scope** in two areas: + +1. `AnimationPlayer.cs` is **1,835 lines** (not ~800), making it the single largest refactoring target in Phase 3. +2. `AnimationSharpManagerV2.cs` is **2,885 lines** and contains significant duplication of base animation logic, not just BCI-specific additions. + +All 10 pain points (P1–P10) from the architecture document are **confirmed present** in the current codebase with exact file and line references. The full subscriber map (§6) provides the complete migration surface for Phase D's EventBus cutover. + +The 69 XML panel configs with `` elements are all in `src/ACATResources/panelconfigs/` and use a uniform structure, making schema migration (Issue #207) tractable. The five constraint items in §7.4 require explicit handling in the `XmlAnimationConfigAdapter`. + +**Acceptance criteria status for Issue #206**: + +- [x] Complete component inventory (§2) – verified against source; line counts corrected +- [x] Data flow confirmed (§3) – matches code reality; method references added +- [x] State machine diagram validated (§4) – confirmed correct; `Timeout` transience clarified +- [x] Pain points P1–P10 confirmed (§5) – exact file:line references provided; severity assigned +- [x] Event subscriber map (§6) – 5 external `EvtPlayerStateChanged` subscribers documented; 1 for `EvtPlayerAnimationTransition` +- [x] XML config audit (§7) – 69 files; top-5 density configs identified; schema constraints listed +- [x] BCI extension analysis (§8) – BCI-specific vs. duplicated behaviors separated +- [x] Performance baseline established (§9) – static metrics; runtime targets identified for Issue #208 + +--- + +**Related Documents**: + +- [`docs/ANIMATION_SYSTEM_ARCHITECTURE.md`](ANIMATION_SYSTEM_ARCHITECTURE.md) — Phase 3 architecture specification (PR #245) +- [`src/ARCHITECTURE_IMPLEMENTATION_STATUS.md`](../src/ARCHITECTURE_IMPLEMENTATION_STATUS.md) — Phase 2 completion status +- [`docs/INTERFACE_EXTRACTION_GUIDE.md`](INTERFACE_EXTRACTION_GUIDE.md) — interface naming conventions +- [`DEPENDENCY_INJECTION_GUIDE.md`](../DEPENDENCY_INJECTION_GUIDE.md) — DI patterns used in Phase 2 + +**Next Issue**: intel/acat#207 – Animation Architecture Design +**Document Owner**: ACAT Architecture Team +**Review Cycle**: Once per phase; next review at start of Issue #207 diff --git a/docs/ANIMATION_SYSTEM_ARCHITECTURE.md b/docs/ANIMATION_SYSTEM_ARCHITECTURE.md index d3c5c510..65ebc365 100644 --- a/docs/ANIMATION_SYSTEM_ARCHITECTURE.md +++ b/docs/ANIMATION_SYSTEM_ARCHITECTURE.md @@ -1,10 +1,21 @@ # Animation System Architecture Specification -**Document Status**: Phase 3 Planning -**Version**: 1.0 +**Document Status**: Phase 3 Planning — Issue #206 Analysis Complete +**Version**: 1.1 **Last Updated**: February 2026 **Epic**: Phase 2 – Core Infrastructure Modernization (Investigation) -**Tracked By**: intel/acat#195 +**Tracked By**: intel/acat#195 + +> **Agent/Developer Note (intel/acat#195):** Issue #206 (Current Animation System Analysis) is **complete**. +> The full evidence-based analysis — with verified line counts, exact pain-point locations, event subscriber map, +> XML config audit, and BCI extension breakdown — is in [`docs/ANIMATION_SYSTEM_ANALYSIS.md`](ANIMATION_SYSTEM_ANALYSIS.md). +> Two important corrections from that analysis: +> - `AnimationPlayer.cs` is **1,835 lines** (not ~800 as estimated here in §3.1). +> - `AnimationSharpManagerV2.cs` is **2,885 lines** with significant duplication. +> +> **Next step**: Issue #207 – Animation Architecture Design. +> Start by reviewing `ANIMATION_SYSTEM_ANALYSIS.md §11` (Prioritized Recommendations for Issue #207) +> before finalizing the interface definitions in §4.2 of this document. --- @@ -73,22 +84,25 @@ The rewrite is **deferred to Phase 3** but this specification is the single auth ### 3.1 Component Inventory +> **Note**: Line counts below are verified actual values from `ANIMATION_SYSTEM_ANALYSIS.md §2`. +> The original estimates for `AnimationPlayer.cs` (~800) and `AnimationSharpManagerV2.cs` were significantly understated. + | File | Role | Lines | |------|------|-------| -| `AnimationPlayer.cs` | Timer loop, widget highlight, state machine | ~800 | -| `AnimationManager.cs` | Actuator event routing, player lifecycle | ~600 | -| `PanelAnimationManager.cs` | Per-panel player management, config loading | ~400 | -| `UserControlAnimationManager.cs` | UserControl variant of panel manager | ~300 | -| `Animation.cs` | Single scan sequence (widget list + PCode hooks) | ~480 | -| `AnimationsCollection.cs` | Collection of `Animations` keyed by panel name | ~150 | -| `Animations.cs` | Collection of `Animation` objects for one panel | ~120 | -| `AnimationWidget.cs` | Per-widget highlight settings (color, sound, etc.) | ~200 | -| `Variables.cs` | Key/value store for animation-scoped variables | ~100 | -| `Interfaces/IAnimationManager.cs` | Public interface for `AnimationManager` | ~30 | -| `Interfaces/IPanelAnimationManager.cs` | Public interface for `PanelAnimationManager` | ~30 | -| `Interfaces/IUserControlAnimationManager.cs` | Public interface for `UserControlAnimationManager` | ~30 | - -**BCI extension:** `AnimationSharpManagerV2.cs` in `src/Extensions/BCI/` provides an alternate `AnimationManager` implementation for Brain-Computer Interface devices and inherits the same base contracts. +| `AnimationPlayer.cs` | Timer loop, widget highlight, state machine | **1,835** | +| `AnimationManager.cs` | Actuator event routing, player lifecycle | **976** | +| `PanelAnimationManager.cs` | Per-panel player management, config loading | **395** | +| `UserControlAnimationManager.cs` | UserControl variant of panel manager | **402** | +| `Animation.cs` | Single scan sequence (widget list + PCode hooks) | **485** | +| `AnimationsCollection.cs` | Collection of `Animations` keyed by panel name | **164** | +| `Animations.cs` | Collection of `Animation` objects for one panel | **156** | +| `AnimationWidget.cs` | Per-widget highlight settings (color, sound, etc.) | **148** | +| `Variables.cs` | Key/value store for animation-scoped variables | **155** | +| `Interfaces/IAnimationManager.cs` | Public interface for `AnimationManager` | **27** | +| `Interfaces/IPanelAnimationManager.cs` | Public interface for `PanelAnimationManager` | **13** | +| `Interfaces/IUserControlAnimationManager.cs` | Public interface for `UserControlAnimationManager` | **15** | + +**BCI extension:** `AnimationSharpManagerV2.cs` in `src/Extensions/BCI/Common/AnimationSharp/` is **2,885 lines** — larger than all 12 core animation files combined. It provides an alternate `AnimationManager` implementation for Brain-Computer Interface devices. See `ANIMATION_SYSTEM_ANALYSIS.md §8` for the BCI-specific vs. duplicated behavior breakdown. ### 3.2 Data Flow @@ -445,21 +459,23 @@ services.AddSingleton( ### Issue #206 – Current Animation System Analysis +> **STATUS: COMPLETE** — See [`docs/ANIMATION_SYSTEM_ANALYSIS.md`](ANIMATION_SYSTEM_ANALYSIS.md) for the full deliverable. + **Goal**: Produce a written, evidence-based analysis of the current animation system that identifies what must change, what must be preserved, and what risks exist. **Acceptance Criteria**: -- [ ] Complete the component inventory in §3.1 (verify line counts, add any missed files). -- [ ] Walk the full data flow (§3.2) and confirm it matches code reality; update if diverged. -- [ ] Produce a state machine diagram (§3.3) validated against `AnimationPlayer.cs` transitions. -- [ ] For each pain point in §3.4 (P1–P10): confirm it still exists in the current code, note its exact location (file + method), and assign a severity (High/Medium/Low). -- [ ] Document all outbound event consumers – which classes subscribe to `EvtPlayerStateChanged` and `EvtPlayerAnimationTransition` – to understand migration scope. -- [ ] Review `AnimationSharpManagerV2.cs` and document which behaviors are BCI-specific vs. duplicated from the base manager. -- [ ] Review all panel XML config files under `src/ACATResources/` that contain `` elements; count them and note any unusual patterns. -- [ ] Write findings in a short analysis report appended to this document (§3) or in a linked `docs/ANIMATION_ANALYSIS_REPORT.md`. +- [x] Complete the component inventory in §3.1 (verify line counts, add any missed files). — `AnimationPlayer.cs` is 1,835 lines (not ~800); `AnimationSharpManagerV2.cs` is 2,885 lines. See `ANIMATION_SYSTEM_ANALYSIS.md §2`. +- [x] Walk the full data flow (§3.2) and confirm it matches code reality; update if diverged. — Confirmed correct; method references added. See `ANIMATION_SYSTEM_ANALYSIS.md §3`. +- [x] Produce a state machine diagram (§3.3) validated against `AnimationPlayer.cs` transitions. — Confirmed; `Timeout` transience clarified. See `ANIMATION_SYSTEM_ANALYSIS.md §4`. +- [x] For each pain point in §3.4 (P1–P10): confirm it still exists in the current code, note its exact location (file + method), and assign a severity (High/Medium/Low). — All 10 confirmed with exact file:line references. See `ANIMATION_SYSTEM_ANALYSIS.md §5`. +- [x] Document all outbound event consumers – which classes subscribe to `EvtPlayerStateChanged` and `EvtPlayerAnimationTransition` – to understand migration scope. — 5 external subscribers for `EvtPlayerStateChanged`; 1 for `EvtPlayerAnimationTransition`. See `ANIMATION_SYSTEM_ANALYSIS.md §6`. +- [x] Review `AnimationSharpManagerV2.cs` and document which behaviors are BCI-specific vs. duplicated from the base manager. — BCI-specific vs. duplicated table provided. See `ANIMATION_SYSTEM_ANALYSIS.md §8`. +- [x] Review all panel XML config files under `src/ACATResources/` that contain `` elements; count them and note any unusual patterns. — 69 files; 5 schema migration constraints identified. See `ANIMATION_SYSTEM_ANALYSIS.md §7`. +- [x] Write findings in a short analysis report appended to this document (§3) or in a linked `docs/ANIMATION_ANALYSIS_REPORT.md`. — Delivered as [`docs/ANIMATION_SYSTEM_ANALYSIS.md`](ANIMATION_SYSTEM_ANALYSIS.md). **Estimated Effort**: 3–4 days -**Output**: Updated §3 of this document + optionally a separate analysis report. +**Output**: [`docs/ANIMATION_SYSTEM_ANALYSIS.md`](ANIMATION_SYSTEM_ANALYSIS.md) — full analysis report with verified metrics, subscriber map, XML audit, BCI analysis, and prioritized recommendations. ---