Implement Phase 8: Rig Relocation and Component Modularization

- Moved the rig scene from `res://master_rig.tscn` to `res://scenes/stickman_rig.tscn`, preserving its UID. - Split the `StickmanRig` controller into three components: `StickmanLocomotion`, `StickmanRagdoll`, and `StickmanActionRunner`. - Updated the architecture to ensure `StickmanRig` acts as the public facade for the new components. - Created a new test suite `test_phase8_rig_modularization.gd` to verify the integrity of the relocated rig and its components. - Added checks for scene loading, component resolution, action queue delegation, and state authority.
2026-09-18 14:11:53 -04:00
parent 9a22322dba
commit db4ff8d939
29 changed files with 2820 additions and 2747 deletions
@@ -0,0 +1,501 @@
+# Phase 8 — Rig Relocation and Runtime Component Modularization
+
+Status: architecture proposed; awaiting user approval before implementation.
+
+Source proposal: `plans/PHASE_8_RIG_MODULARIZATION.md`.
+
+## 1. Outcome and architectural correction
+
+Move the authored rig scene from `res://master_rig.tscn` to
+`res://scenes/stickman_rig.tscn` while preserving its existing scene UID
+(`uid://dr5ef3l2var2s`). Split the 3,159-line `StickmanRig` controller into three child
+components, but keep `StickmanRig` as the only public facade used by the factory, stage,
+harnesses, scene I/O, and existing gameplay code.
+
+One boundary in the source plan should change before implementation: navigation and
+`walk_to()` belong in `StickmanLocomotion`, not `StickmanActionRunner`. Walking, terrain
+following, ledge momentum, landing, animation pause/resume, and root displacement share one
+state machine and one `AnimationPlayer`. Splitting path following from terrain motion would
+create two writers for the rig transform and require a per-frame synchronization protocol.
+`StickmanActionRunner` should orchestrate actions and call the locomotion API; it should not
+implement movement.
+
+The extraction is structural only. It must not change movement tuning, recovery math, action
+payloads, signal timing, saved scene data, or editor UI.
+
+## 2. Targets and ownership
+
+| Target | Ownership after Phase 8 |
+| --- | --- |
+| `scenes/stickman_rig.tscn` | Renamed rig scene. Keeps the existing `Master`, `Body`, `Skeleton2D`, `IK_Targets`, `AnimationTargets`, `AnimationPlayer`, and `AnimationTree` paths. Adds a nonvisual `RigComponents` branch. |
+| `scripts/stickman_rig.gd` (`StickmanRig`) | Public compatibility facade; authoritative `RigState`; facing profile, bend flags, IK modification resolution, body z-order, root mirror, head pin, shared node lookup, component wiring, deterministic physics scheduling, and public signal forwarding. |
+| `scripts/stickman_locomotion.gd` (`StickmanLocomotion`) | Navigation agent, walking, terrain probes/following, airborne and landing states, root movement, gait/foot contacts, animated collision proxies, airborne sweeps, and impact reactions. |
+| `scripts/stickman_ragdoll.gd` (`StickmanRagdoll`) | Momentum tracking, ragdoll definitions and procedural bodies/joints, rest detection, pre-ragdoll/captured-pose data, recovery snap/tween, and ragdoll impulse application. |
+| `scripts/stickman_action_runner.gd` (`StickmanActionRunner`) | Authored/runtime action queues, reactive injection, action phase machine, wait timing, speech bubble and overlay arbitration, and action/queue/speech signals. |
+| `scripts/stickman_factory.gd` | Loads the relocated scene; return type and `spawn*` API stay unchanged. |
+| `scripts/stk_rig_adapter.gd` | No node-path changes. Comments reference the relocated scene. It continues to adapt the root before `_ready()`. |
+| `scripts/create_animations.gd`, harnesses, tests, active docs | Update live scene-path literals/comments and component-aware tests. |
+
+`source_stk_path` remains on the root facade because it is instance provenance consumed by
+stage serialization, not behavior owned by a component.
+
+No layout, theme, visible node, or user-facing UI change is required.
+
+## 3. Scene structure
+
+The existing visual and skeleton hierarchy must remain byte-for-byte equivalent apart from the
+new component branch and external component script resources. Adapter and harness node paths
+therefore remain valid.
+
+```text
+Master: Node2D [StickmanRig]
+├── RigComponents: Node
+│   ├── Locomotion: Node [StickmanLocomotion]
+│   ├── Ragdoll: Node [StickmanRagdoll]
+│   └── ActionRunner: Node [StickmanActionRunner]
+├── Body: Node2D
+│   └── Body / Head / 8 limb visual containers   (unchanged)
+├── Skeleton2D                                  (unchanged)
+│   └── Torso/Head + 8 limb Bone2D nodes
+├── RayCast_Ground                              (unchanged; legacy)
+├── IK_Targets                                  (unchanged)
+├── AnimationTargets                            (unchanged)
+├── AnimationPlayer                             (unchanged)
+└── AnimationTree                               (unchanged)
+
+world/stage parent
+└── RagdollBodyContainer                        (runtime only; unchanged location)
+    ├── 10 RigidBody2D
+    └── 9 PinJoint2D
+```
+
+The components are authored children rather than created in `_ready()`. Missing component nodes
+produce one `StickmanRig:` warning per missing node and disable only that subsystem. The root must
+never manufacture a fallback component silently because that would hide a broken scene.
+
+## 4. Dependency direction and state authority
+
+```mermaid
+flowchart LR
+    C[StickmanRig facade/coordinator]
+    L[StickmanLocomotion]
+    R[StickmanRagdoll]
+    A[StickmanActionRunner]
+    U[Stage / Factory / Harness / Scene IO]
+
+    U -->|existing public API| C
+    C -->|configure + deterministic ticks| L
+    C -->|configure + commands| R
+    C -->|configure + commands| A
+    A -->|walk/speech-independent movement query| L
+    A -->|ragdoll_requested / recovery_requested| C
+    R -->|state_transition_requested| C
+    R -->|ground query + recovery reanchor| L
+    L -->|arrived / facing_requested| C
+    C -->|forwarded public signals| U
+```
+
+Rules:
+
+1. `StickmanRig.state` is the only high-level state authority. Components receive the current
+   state during commands/ticks and never write it directly.
+2. `StickmanActionRunner` has no reference to `StickmanRagdoll`; it requests transitions through
+   root-connected signals. This allows queue tests without constructing physics bodies.
+3. `StickmanRagdoll` may synchronously call only the narrow locomotion recovery/query API; it
+   must not inspect locomotion private fields.
+4. `StickmanLocomotion` requests facing changes by signal so the facade remains the profile and
+   mirror authority.
+5. The components do not implement `_physics_process()`. The root is the sole scheduler. This
+   preserves the current update order and avoids relying on Godot sibling processing order.
+
+## 5. Static component contracts
+
+The signatures below are the implementation contract. Internal helper names may be retained
+during mechanical extraction, but cross-component calls must use only these methods/signals.
+
+### 5.1 `StickmanLocomotion`
+
+```gdscript
+class_name StickmanLocomotion
+extends Node
+
+signal arrived(target: Vector2)
+signal facing_requested(profile: int)
+
+func configure(rig: StickmanRig) -> void
+func tick_movement(delta: float, rig_state: int) -> void
+func tick_post_skeleton(delta: float, rig_state: int) -> void
+
+func walk_to(target: Vector2, speed: float = -1.0) -> bool
+func cancel_walk(restore_idle_pose: bool = true) -> void
+func is_walking() -> bool
+func consume_walk_completed() -> bool
+
+func begin_terrain_fall() -> void
+func set_terrain_following_enabled(enabled: bool) -> void
+func is_action_blocked() -> bool
+func prepare_for_ragdoll() -> void
+func restore_after_recovery() -> void
+
+func probe_ground_y(
+    world_x: float,
+    top_y: float,
+    bottom_y: float,
+    exclude: Array[RID]
+) -> float
+func reanchor_for_recovery(feet_world: Vector2, foot_offset: Vector2) -> Vector2
+
+func set_collision_bodies_enabled(enabled: bool) -> void
+func apply_collision_reaction(
+    part_name: String,
+    normal: Vector2,
+    impact_speed: float,
+    contact_position: Vector2 = Vector2.INF
+) -> void
+```
+
+`tick_movement()` owns the old clear-pelvis → walking → terrain → settle sequence.
+`tick_post_skeleton()` owns impact-reaction application and collision-proxy synchronization,
+which must remain after the facade's head pin. `reanchor_for_recovery()` moves the root and
+returns the mirror-aware rig-local shift used by `StickmanRagdoll` to rebase captured body data.
+
+The locomotion component is the source of truth for `TerrainMotion`, navigation, terrain/air
+tuning constants, collision proxy tables, and reaction constants. Compatibility aliases may
+remain on `StickmanRig` for one phase, but they must point to component constants rather than
+duplicate values.
+
+### 5.2 `StickmanRagdoll`
+
+```gdscript
+class_name StickmanRagdoll
+extends Node
+
+signal state_transition_requested(next_state: int)
+signal rest_state_changed(at_rest: bool)
+
+func configure(rig: StickmanRig, locomotion: StickmanLocomotion) -> void
+func tick_pre_movement(delta: float, rig_state: int) -> void
+
+func enter_ragdoll() -> bool
+func request_recovery() -> bool
+func snap_to_standing() -> bool
+func cancel_recovery() -> void
+func apply_velocity_boost(velocity: Vector2) -> void
+func is_at_rest() -> bool
+func initialize_spawn_pose_from_skeleton() -> void
+```
+
+`enter_ragdoll()` and recovery methods return whether they actually accepted the transition.
+They perform the physical handoff before emitting `state_transition_requested`; the root then
+commits `state` and emits the legacy `state_changed` signal exactly once. The component owns the
+existing `RAGDOLL_BODIES`, `RAGDOLL_JOINTS`, per-profile joint bands, recovery overrides,
+captured dictionaries, tween generation, and procedural container lifetime.
+
+The root provides narrow internal skeleton services used by recovery:
+
+```gdscript
+func _get_skeleton() -> Skeleton2D
+func _get_body_container() -> Node2D
+func _get_animation_player() -> AnimationPlayer
+func _get_ik_marker(name: String) -> Marker2D
+func _get_limb_bones() -> Dictionary
+func _get_bend_modifications() -> Dictionary
+func _get_head_bone() -> Bone2D
+func _get_head_look_at() -> SkeletonModification2DLookAt
+func _set_limb_mods_enabled(enabled: bool) -> void
+func _restore_head_lookat_state() -> void
+func _rearm_ik_stack() -> void
+```
+
+These methods are internal collaboration points, not new consumer APIs.
+
+### 5.3 `StickmanActionRunner`
+
+```gdscript
+class_name StickmanActionRunner
+extends Node
+
+signal action_started(action: Dictionary, index: int)
+signal action_finished(action: Dictionary, index: int)
+signal queue_finished
+signal queue_changed
+signal speech_finished
+signal ragdoll_requested(enabled: bool)
+signal recovery_requested
+
+var action_queue: Array[Dictionary] = []
+
+func configure(rig: StickmanRig, locomotion: StickmanLocomotion) -> void
+func tick(delta: float, rig_state: int, ragdoll_at_rest: bool) -> void
+
+func speak(text: String, duration: float) -> void
+func queue_action(action: Dictionary) -> void
+func clear_queue() -> void
+func get_queue() -> Array[Dictionary]
+func remove_action(index: int) -> void
+func insert_action(index: int, action: Dictionary) -> void
+func queue_size() -> int
+func enqueue_reactive(actions: Array[Dictionary]) -> void
+func clear_reactive_actions() -> void
+func start_queue() -> void
+func stop_queue() -> void
+func is_queue_running() -> bool
+func sync_facing(profile: int) -> void
+```
+
+`RunnerState` and `ActionPhase` are component-owned. Root keeps enum aliases/definitions only
+where required for source compatibility. The component owns both primary and overlay speech,
+creates `SpeechBubble` under `Master` (not under `RigComponents`), and counter-mirrors it through
+`sync_facing()`.
+
+## 6. Root compatibility facade
+
+All current consumer-facing members remain available on `StickmanRig` with unchanged names,
+parameters, return types, and signal cardinality.
+
+| Existing root surface | Delegates to / remains in |
+| --- | --- |
+| `set_facing_profile`, `get_facing_profile`, bend getters/setters, bend joint queries | Root |
+| `state`, `state_changed`, `set_ragdoll`, `toggle_ragdoll`, `is_in_ragdoll`, `request_recovery`, `snap_to_standing`, `apply_ragdoll_velocity_boost`, `is_ragdoll_at_rest`, `initialize_spawn_pose_from_skeleton` | Root facade → Ragdoll |
+| `walk_to`, `is_walking`, `arrived`, `begin_terrain_fall`, `snap_to_terrain`, `set_terrain_following_enabled` | Root facade → Locomotion |
+| `set_collision_bodies_enabled`, `apply_collision_reaction` | Root facade → Locomotion |
+| `action_queue`, queue mutation/query methods, reactive methods, `start_queue`, `stop_queue`, `is_queue_running`, `speak` | Root facade → ActionRunner |
+| `action_started`, `action_finished`, `queue_finished`, `queue_changed`, `speech_finished` | Child signal forwarded by root |
+| `facing_profile`, four bend exports, `rest_timeout`, `auto_recover`, `walk_speed` | Remain root exports; values synchronize to the owning component during `_ready()` and in setters afterward. |
+| `source_stk_path` | Root |
+
+`action_queue` must be a delegating property because tests, scene loading, and older callers may
+read or assign it directly:
+
+```gdscript
+var action_queue: Array[Dictionary]:
+    get:
+        return _action_runner.action_queue if _action_runner != null else []
+    set(value):
+        if _action_runner != null:
+            _action_runner.action_queue = value
+        else:
+            _pending_action_queue = value
+```
+
+Direct assignment keeps today's behavior and does not emit `queue_changed`; mutation methods do.
+Pre-`_ready()` export/property writes are stored on the facade and pushed to components after
+configuration, matching the existing pre-ready setter guard.
+
+Private root fields are not compatibility API. Existing tests that inspect `_ragdoll_bodies`,
+`_terrain_motion`, `_run_queue`, or other private internals must move to the owning component.
+Do not keep duplicate mutable private state on the root merely to satisfy tests.
+
+## 7. Physics order and state transitions
+
+The root retains a single `_physics_process()` scheduler:
+
+```text
+Ragdoll.tick_pre_movement
+  ├── cache root momentum
+  └── update ragdoll rest detection
+Locomotion.tick_movement
+  ├── clear one-frame pelvis correction
+  ├── update walk/navigation
+  ├── update terrain/air/landing
+  └── settle walk markers
+ActionRunner.tick
+  ├── update speech clocks
+  └── update action state machine
+StickmanRig core
+  └── pin mirrored head rotation
+Locomotion.tick_post_skeleton
+  ├── apply impact reactions
+  └── sync animated collision proxies
+```
+
+Ragdoll transition flow:
+
+```mermaid
+stateDiagram-v2
+    [*] --> ANIMATED
+    ANIMATED --> RAGDOLL: root.set_ragdoll(true)\nlocomotion cleanup → build physics → request state
+    RECOVERING --> RAGDOLL: root.set_ragdoll(true)\ncancel tween → rebuild physics
+    RAGDOLL --> RECOVERING: request_recovery()\ncapture → destroy physics → reanchor → snap
+    RECOVERING --> ANIMATED: recovery tween finished
+    RAGDOLL --> ANIMATED: snap_to_standing()
+    RECOVERING --> ANIMATED: snap_to_standing()
+```
+
+The facade rejects illegal transition requests before delegating. A component signal can request
+a state commit, but `_set_state(next_state)` on the facade is the only function that mutates the
+root `state` and emits `state_changed`.
+
+## 8. Practical data flows
+
+### 8.1 Authored queue action
+
+```gdscript
+rig.queue_action({"type": "walk_to", "target": Vector2(640, 320), "speed": 300.0})
+rig.queue_action({"type": "speak", "text": "Made it!", "duration": 2.0})
+rig.start_queue()
+```
+
+```text
+StickmanRig.queue_action
+  → ActionRunner.action_queue
+  → root forwards queue_changed
+ActionRunner begins walk_to
+  → Locomotion.walk_to
+  → Locomotion arrived(target)
+  → root forwards arrived(target)
+  → ActionRunner observes consume_walk_completed() and emits action_finished
+```
+
+The queue dictionary shape and deep-copy behavior remain unchanged.
+
+### 8.2 Ragdoll recovery landing handoff
+
+```text
+Ragdoll captures torso world center + body pose
+  → Locomotion.probe_ground_y(x, top, bottom, ragdoll_rids)
+  → Ragdoll computes feet_world
+  → Locomotion.reanchor_for_recovery(feet_world, FOOT_OFFSET)
+      moves Master and returns mirror-aware local_shift
+  → Ragdoll rebases captured positions by local_shift
+  → Ragdoll snap-solves skeleton and requests RECOVERING
+```
+
+The existing capture payload stays internal and unchanged:
+
+```gdscript
+{
+    "torso": {"pos": Vector2(...), "rot": 0.42, "half": 195.0},
+    "head": {"pos": Vector2(...), "rot": -0.10, "half": 100.0},
+    # eight limb entries...
+}
+```
+
+## 9. Relocation policy
+
+Implementation moves the scene once with its UID intact, then updates live references in the
+same change:
+
+- `scripts/stickman_factory.gd`
+- `scripts/physics_test_harness.gd`
+- `tests/test_phase6a_airborne_landing.gd`
+- any additional literal discovered by `rg --fixed-strings 'res://master_rig.tscn'`
+- active comments in `stickman_rig.gd`, `stk_rig_adapter.gd`, `test_harness.gd`, and
+  `create_animations.gd`
+- implementation-facing comments in the changed scripts
+
+After the user verifies the implementation in Godot, the documentation phase updates active
+project docs (`README.md`, `AGENTS.md`, `RIGGING.md`, and the source plan). Historical specs
+should not receive a blind search/replace because many statements describe the path that existed
+when that phase shipped. Add a short relocation note where a historical file is still used as
+live guidance; do not rewrite historical line references or decisions.
+
+Acceptance requires:
+
+```powershell
+rg --fixed-strings 'res://master_rig.tscn' scripts scenes tests project.godot
+```
+
+to return no live code/scene/test matches. Human-readable historical mentions may remain when
+clearly historical.
+
+## 10. Migration sequence
+
+1. **Characterize first.** Run the current focused rig suites and record baselines before moving
+   code. Add no behavior changes to the extraction patch.
+2. **Add component nodes/scripts without relocating the scene.** Wire configuration, root signal
+   forwarding, and the central scheduler. The scene still loads from the old path during this
+   step.
+3. **Extract action/speech orchestration.** It has the smallest physics surface. Keep all root
+   queue methods as delegates and migrate private runner tests.
+4. **Extract locomotion plus animated collision.** Move navigation, terrain/air/landing, proxy,
+   and flinch state together so only one component writes the animated root/markers.
+5. **Extract ragdoll/recovery.** Connect the narrow ground-query/reanchor API and retain the exact
+   instant-handoff ordering.
+6. **Slim and verify the facade.** Remove duplicate mutable state/constants, leaving aliases only
+   where compatibility requires them. Verify every root public method and signal.
+7. **Relocate the scene last.** Move to `scenes/stickman_rig.tscn`, preserve the UID, update every
+   live preload/path/comment, force one headless editor rescan for new `class_name` scripts, then
+   run static checks and all focused suites.
+8. **Stop for editor verification.** Per `spec-pipeline`, do not update README, AGENTS, RIGGING,
+   the source plan, or historical documentation until the user confirms the feature works in
+   Godot. Documentation cleanup is the manually triggered Phase 4.
+
+## 11. Verification plan
+
+### Static and scene-load checks
+
+```powershell
+& "C:\Godot4\Godot_v4.7.1-stable_win64_console.exe" --headless --editor --path "C:\Godot4\stickman" --quit
+& "C:\Godot4\Godot_v4.7.1-stable_win64_console.exe" --headless --check-only --script "res://scripts/stickman_rig.gd" --path "C:\Godot4\stickman"
+& "C:\Godot4\Godot_v4.7.1-stable_win64_console.exe" --headless --check-only --script "res://scripts/stickman_locomotion.gd" --path "C:\Godot4\stickman"
+& "C:\Godot4\Godot_v4.7.1-stable_win64_console.exe" --headless --check-only --script "res://scripts/stickman_ragdoll.gd" --path "C:\Godot4\stickman"
+& "C:\Godot4\Godot_v4.7.1-stable_win64_console.exe" --headless --check-only --script "res://scripts/stickman_action_runner.gd" --path "C:\Godot4\stickman"
+```
+
+Add `tests/test_phase8_rig_modularization.gd` covering:
+
+1. relocated scene loads by new path and retains UID/node paths;
+2. all three typed component children resolve;
+3. root public methods delegate and root signals emit once;
+4. missing optional/component dependency warns instead of crashing;
+5. queue/speech execution works while no ragdoll bodies exist;
+6. locomotion walking/terrain ticks work while no ragdoll bodies exist;
+7. ragdoll enter/recover/snap keeps state order and ten-body/nine-joint cardinality;
+8. root exports synchronize before and after `_ready()`;
+9. direct `action_queue` assignment retains compatibility semantics; and
+10. repeated spawn/free leaves no world-level `RagdollBodyContainer` orphan.
+
+Run the focused regressions most coupled to the extraction:
+
+- `tests/test_phase3c_walk_recovery.gd`
+- `tests/test_phase4_final_bugs.gd`
+- `tests/test_phase4b_walk.gd`
+- `tests/test_phase4b_terrain.gd`
+- `tests/test_phase45_collision_bodies.gd`
+- `tests/test_phase6a_airborne_landing.gd`
+- `tests/test_phase5_file_features.gd`
+- `tests/test_cli_scene.gd`
+
+Tests should inspect `$RigComponents/Locomotion`, `$RigComponents/Ragdoll`, or
+`$RigComponents/ActionRunner` for subsystem internals instead of requiring stale private mirrors
+on `StickmanRig`.
+
+Manual Godot verification after automated checks:
+
+- factory/harness loads an `.stk` and all body geometry follows the same bones;
+- LEFT/RIGHT/FORWARD, head mirror pin, bend menus, and z-order remain unchanged;
+- queue `walk_to → speak → wait → ragdoll → recover` completes with one signal per event;
+- walking over ramps, ledge fall, hard landing, prop collision/flinch, and queue landing barrier;
+- ragdoll knock-up, automatic/manual recovery, recovery interruption, and snap-to-standing;
+- Sandbox EDIT/DIRECT/PLAY round trip and scene save/load provenance/queue persistence.
+
+## 12. Risks and guardrails
+
+| Risk | Guardrail |
+| --- | --- |
+| Godot child processing order changes behavior | Components have no physics callbacks; root calls ordered tick methods. |
+| Walk and terrain both write the root | They live in one locomotion component and retain the current internal order. |
+| State duplicated between root and ragdoll | Root is sole `RigState` authority; component requests transitions. |
+| Signals double-fire through forwarding | Child emits subsystem signal; root forwarding is the only consumer-facing emission. Add cardinality tests. |
+| Recovery reaches through locomotion internals | Only `probe_ground_y()` and `reanchor_for_recovery()` cross that boundary. |
+| Adapter runs before component `_ready()` | Components do not cache geometry until root `_ready()`; adapter node paths remain unchanged. |
+| Scene move breaks preload before scripts update | Relocation and all live path updates land atomically; UID is asserted. |
+| New `class_name` cache is stale in CLI | Run one `--headless --editor --quit` rescan before `--check-only`. |
+| Private-test migration masks regressions | Add behavior assertions through root API/signals plus component-local focused assertions. |
+| Broad doc replacement falsifies old specs | Update active docs; annotate historical specs instead of blind replacement. |
+
+## 13. Decisions requiring approval
+
+1. **Approve movement ownership change:** `StickmanLocomotion` owns navigation and `walk_to()`;
+   `StickmanActionRunner` only orchestrates it. This intentionally differs from the source plan.
+2. **Approve animated collision ownership:** collision proxies, airborne sweeps, and impact
+   reactions move with locomotion, avoiding a fourth component in this phase.
+3. **Approve compatibility scope:** preserve all root public APIs/signals/exports, including direct
+   `action_queue`; migrate tests off private root fields rather than keeping duplicate private
+   mirrors.
+4. **Approve documentation policy:** after editor verification, update active documentation and
+   preserve historical specs with relocation notes instead of blanket rewriting their historical
+   references.
+
+Implementation must not begin until these decisions are approved.