NEO-122: Address follow-up review doc drift and QA numbering.

Reconcile plan with economy HUD layout refactor, fix manual QA steps,
update regression notes for NEO-74/75/110, and strike through resolved
review suggestions.
pull/161/head
VinPropane 2026-06-07 13:36:21 -04:00
parent 64da7611d9
commit ba75aeed79
7 changed files with 35 additions and 18 deletions

View File

@ -369,7 +369,7 @@ On **Linux** (including GitHub Actions), the path must be **`gdUnit4`** with a c
**CI:** The **GDScript** workflow fails the job if Godot prints **`SCRIPT ERROR:`** or **`ERROR: Failed to load script`** while running GdUnit. GdUnit alone can still exit **0** when a test file fails to parse (that suite is skipped); the workflow guards against that.
**Scope:** Unit tests cover **`player.gd`**, **`player_locomotion_wasd.gd`** (pure WASD seam helpers), **`position_authority_client.gd`**, **`inventory_client.gd`**, **`item_definitions_client.gd`** (NEO-72), **`skill_progression_client.gd`**, **`gig_progression_client.gd`** (NEO-86), **`quest_progress_client.gd`**, **`quest_definitions_client.gd`** (NEO-122), **`prototype_interactable_picker.gd`**, extended **`interaction_request_client_test.gd`**, **`gather_feedback_refresh_test.gd`** (NEO-73), **`craft_client.gd`**, **`recipe_definitions_client.gd`**, **`craft_feedback_refresh_test.gd`** (NEO-74), **`prototype_economy_hud_section.gd`** (NEO-75), **`combat_targets_client.gd`**, **`combat_feedback_refresh_test.gd`** (NEO-85), **`gig_feedback_refresh_test.gd`** (NEO-86), **`isometric_follow_camera.gd`** (eye math + effective zoom distance), **`camera_state.gd`**, and **`zoom_band_config.gd`**. **`main.gd`** and scene wiring are **not** automated here—use manual checks above.
**Scope:** Unit tests cover **`player.gd`**, **`player_locomotion_wasd.gd`** (pure WASD seam helpers), **`position_authority_client.gd`**, **`inventory_client.gd`**, **`item_definitions_client.gd`** (NEO-72), **`skill_progression_client.gd`**, **`gig_progression_client.gd`** (NEO-86), **`quest_progress_client.gd`**, **`quest_definitions_client.gd`** (NEO-122), **`prototype_interactable_picker.gd`**, extended **`interaction_request_client_test.gd`**, **`gather_feedback_refresh_test.gd`** (NEO-73), **`craft_client.gd`**, **`recipe_definitions_client.gd`**, **`craft_recipe_panel_test.gd`** (NEO-122 follow-up), **`craft_feedback_refresh_test.gd`** (NEO-74), **`prototype_economy_hud_section.gd`** (NEO-75), **`combat_targets_client.gd`**, **`combat_feedback_refresh_test.gd`** (NEO-85), **`gig_feedback_refresh_test.gd`** (NEO-86), **`isometric_follow_camera.gd`** (eye math + effective zoom distance), **`camera_state.gd`**, and **`zoom_band_config.gd`**. **`main.gd`** and scene wiring are **not** automated here—use manual checks above.
**Camera zoom:** Input actions **`camera_zoom_in`** / **`camera_zoom_out`** (mouse wheel, **`=`** / **`-`**, keypad **+** / ****) are defined in **`project.godot`**. **`isometric_follow_camera.gd`** also maps **macOS trackpad** two-finger scroll and pinch to the same discrete zoom bands using **accumulated delta** (one band step per ~1.25 pan units, **150 ms** cooldown between trackpad steps) so small gestures do not traverse all bands at once. On layouts where **`+`** requires **Shift**, remap or add a binding under **Project → Project Settings → Input Map** if needed.

View File

@ -37,7 +37,7 @@ Player casts **`prototype_pulse`** (**25** damage, ~**3 s** cooldown).
## Checklist
1. Start server: `cd server/NeonSprawl.Server && dotnet run`.
2. Run Godot main scene (**F5**). Confirm boot lines: **`Encounter: … not started (0/3)`** and **`Loot: —`** on **`EncounterProgressLabel`** / **`EncounterCompleteLabel`** (combat HUD block, below **`PlayerCombatHpLabel`**).
2. Run Godot main scene (**F5**). Confirm boot lines: **`Encounter: … not started (0/3)`** and **`Loot: —`** on **`EncounterProgressLabel`** / **`EncounterCompleteLabel`** (in **`HudRootScroll`**, below **`PlayerCombatHpLabel`**). Scroll **`HudRootScroll`** if the debug stack is taller than the viewport.
3. Defeat **`prototype_npc_melee`**: walk to orange marker, **Tab** lock, press **1** until **`Cast: … — defeated!`**. Verify **`EncounterProgressLabel`** shows **`1/3`** (may lag one GET behind cast — wait ~1 s or defeat next NPC).
4. Defeat **`prototype_npc_ranged`**: lock at SE marker, cast until defeated. Verify **`2/3`**.
5. Defeat **`prototype_npc_elite`**: lock at origin, cast until defeated (**8** pulses). Verify **`completed (3/3)`** and **`EncounterCompleteLabel`** lists **`scrap_metal_bulk ×10`** and **`contract_handoff_token ×1`** (or raw ids if defs still loading).

View File

@ -29,7 +29,7 @@
## Checklist
1. Start server: `cd server/NeonSprawl.Server && dotnet run`.
2. Run Godot main scene (**F5**). Confirm **`QuestProgressLabel`** shows four **`not started`** lines and **`QuestAcceptFeedbackLabel`** shows the idle hint (below encounter loot labels).
2. Run Godot main scene (**F5**). Confirm **`QuestProgressLabel`** shows four **`not started`** lines and **`QuestAcceptFeedbackLabel`** shows the idle hint (in **`HudRootScroll`**, below encounter loot labels). Economy panel pinned bottom-left (not off-screen).
3. Press **Q**. Verify gather intro becomes **`active step 1`** and accept feedback shows success.
4. Gather scrap until gather quest completes (resource node **R**). Verify counter updates on **`QuestProgressLabel`** after each successful gather (may lag one GET).
5. Press **Shift+Q** before gather is complete — verify **`Quest accept: denied — prerequisite_incomplete`** for refine (or **`no eligible quest`** if refine is not yet eligible).
@ -37,8 +37,9 @@
7. Open **Economy HUD** recipe panel — **Refine Scrap (Standard)** is the **first** row (scroll starts at top). Craft when **≥5** scrap in bag.
8. Craft **`refine_scrap_standard`** with Economy HUD open. Verify refine quest **`completed`** after successful craft.
9. Press **Q** again on already-completed gather. Verify deny feedback on **`QuestAcceptFeedbackLabel`** without Bruno.
9. Optional: stop server while Godot running, press **Q** — verify **`Quests: error — …`** or **`Quest accept: failed — …`** visible on HUD.
10. Optional: block **`GET /game/world/quest-definitions`** only while progress GET succeeds — verify **`definitions error — …`** under **`QuestProgressLabel`** with raw quest ids as fallback.
10. Optional: stop server while Godot running, press **Q** — verify **`Quests: error — …`** or **`Quest accept: failed — …`** visible on HUD.
11. Optional: block **`GET /game/world/quest-definitions`** only while progress GET succeeds — verify **`definitions error — …`** under **`QuestProgressLabel`** with raw quest ids as fallback.
12. Toggle **Economy HUD** off/on — debug labels in **`HudRootScroll`** remain; recipe repopulate + scroll reset still works.
## Notes
@ -48,6 +49,7 @@
## Acceptance
- [ ] Steps 18 completable in one session without Bruno/curl.
- [ ] Steps 19 completable in one session without Bruno/curl.
- [ ] Quest status visible after **Q** accept and updates after gather/craft without manual refresh key.
- [ ] Failed GET/accept shows visible HUD error copy.
- [ ] Economy HUD craft panel visible for step 78 (recipes first row, no overlap with quest labels).

View File

@ -16,8 +16,8 @@
1. Start server: `cd server/NeonSprawl.Server && dotnet run`.
2. Run Godot main scene (**F5**). Confirm **`SkillProgressionLabel`** shows **`salvage`** and **`refine`** rows after boot.
3. Scroll the recipe panel below skills**eight** prototype recipes with **Craft** buttons.
4. On a fresh player (empty bag), click **Craft** on **`refine_scrap_standard`**. **`CraftFeedbackLabel`** shows **`Craft: denied — insufficient_materials`**. Press **I** — inventory unchanged.
3. Scroll the recipe panel in **Economy HUD** (`BodyScroll/Body/CraftRecipePanel`)**eight** prototype recipes with **Craft** buttons. **Refine Scrap (Standard)** is the first row (NEO-122 layout).
4. On a fresh player (empty bag), click **Craft** on **`refine_scrap_standard`**. **`CraftFeedbackLabel`** (in **`HudRootScroll`**) shows **`Craft: denied — insufficient_materials`**. Press **I** — inventory unchanged.
5. Gather **≥5** **`scrap_metal_bulk`** (see [NEO-73 manual QA](NEO-73.md) **R** at resource nodes).
6. Click **Craft** on **`refine_scrap_standard`**. **`CraftFeedbackLabel`** shows **`Craft: +1 …`** (refined plate output). **`InventoryLabel`** updates without **I**. **`refine`** xp increases on **`SkillProgressionLabel`**.
7. Gather **≥1** more scrap if needed; ensure **≥2** **`refined_plate_stock`** and **≥1** **`scrap_metal_bulk`** in bag (craft **`refine_scrap_standard`** again or gather).
@ -28,3 +28,4 @@
- Craft quantity is fixed at **1** per button (no spinner in this story).
- Success copy comes from POST **`outputsGranted`**; inventory and skill rows refresh via GET after success only.
- **NEO-122 layout:** debug feedback labels live in **`HudRootScroll`**; economy block (recipes, inventory, skills) is pinned bottom-left — scroll economy **`BodyScroll`** for inventory/skills below recipes.

View File

@ -26,13 +26,13 @@
## Checklist
1. Start server: `cd server/NeonSprawl.Server && dotnet run`.
2. Run Godot main scene (**F5**). Confirm **`Economy HUD`** toggle is **on**; **`InventoryLabel`** shows empty bag; **`SkillProgressionLabel`** shows **`salvage`** and **`refine`** rows.
2. Run Godot main scene (**F5**). Confirm **`Economy HUD`** toggle is **on**; **`InventoryLabel`** shows empty bag; **`SkillProgressionLabel`** shows **`salvage`** and **`refine`** rows. Scroll **`BodyScroll`** if inventory/skills are below the recipe list (NEO-122 recipes-first order).
3. **WASD** to **`prototype_resource_node_alpha`** anchor **(12, 6)**. Press **R** once. **`GatherFeedbackLabel`** shows **`+1`** scrap delta; inventory scrap updates without **I**; **`salvage`** xp increases.
4. Walk to **`prototype_subsurface_vein_beta`** **(18, 6)**; press **R** (**+2** scrap). Repeat at **`prototype_bio_mat_gamma`** **(12, 0)** (**+3**) and **`prototype_urban_bulk_delta`** **(18, 0)** (**+5**). Inventory shows **11** scrap total.
5. In the recipe panel, click **Craft** on **`refine_scrap_standard`**. **`CraftFeedbackLabel`** shows success; inventory shows **6** scrap and **1** **`refined_plate_stock`**; **`refine`** xp increases.
6. Click **Craft** on **`refine_scrap_standard`** again. Inventory shows **1** scrap and **2** plates.
7. Click **Craft** on **`make_field_stim_mk0`**. Success feedback shows **`field_stim_mk0`** output; **`InventoryLabel`** lists **`Field Stim Mk0`** (or agreed display name) with quantity **≥ 1**; **`refine`** xp increases again.
8. Uncheck **`Economy HUD`** — inventory, skills, and recipe panel hide; gather/craft feedback labels above remain visible. Check **`Economy HUD`** again — panel repopulates with current data.
8. Uncheck **`Economy HUD`** — inventory, skills, and recipe panel hide; gather/craft feedback labels in **`HudRootScroll`** remain visible. Check **`Economy HUD`** again — panel repopulates with current data.
## Notes

View File

@ -58,7 +58,7 @@
- [x] Player sees quest status change after accept without Bruno.
- [x] Objective progress updates after gather/craft/encounter in Godot session.
- [x] Failed GET/accept surfaces visible HUD error (match interaction client pattern).
- [ ] `docs/manual-qa/NEO-122.md` Godot steps exercisable (human QA pending).
- [x] `docs/manual-qa/NEO-122.md` Godot steps exercisable (human QA passed 2026-06-07).
## Implementation reconciliation (shipped)
@ -69,6 +69,17 @@
- **Tests:** `client/test/quest_progress_client_test.gd` (12 cases across both suites), `client/test/quest_definitions_client_test.gd`.
- **Docs:** `client/README.md` quest HUD section; `docs/manual-qa/NEO-122.md`.
### NEO-122 follow-up (economy HUD layout — manual QA step 7)
During NEO-122 QA, **`refine_scrap_standard`** was unreachable when the economy block sat below the viewport inside the tall **`HudRoot`** stack. Follow-up commits (postfirst review) refactored prototype HUD layout **without changing quest contracts**:
- **`HudRootScroll`** — **`ScrollContainer`** wrapping debug/combat/quest labels; height clamped above the economy panel.
- **`EconomyHudSection`** — reparented to **`UICanvas`**; pinned bottom-left via **`prototype_economy_hud_section.gd`** **`_apply_viewport_layout`** (explicit **`position`/`size`**, opaque panel fill, **`z_index`**).
- **`BodyScroll/Body`** — single economy scroll; child order **Recipes → Inventory → Skills → Gig XP** so craft rows appear first.
- **`craft_recipe_panel.gd`** — flattened to **`VBoxContainer`** + **`RecipeRows`** (no nested scroll); populate resets economy body scroll to recipes via parent **`scroll_body_to_recipes()`**.
- **Tests:** `client/test/craft_recipe_panel_test.gd`; viewport layout tests in **`prototype_economy_hud_section_test.gd`**.
- **Regression:** spot-check [NEO-74](../manual-qa/NEO-74.md), [NEO-75](../manual-qa/NEO-75.md), [NEO-110](../manual-qa/NEO-110.md) after merge (collapsed economy toggle, feedback labels in **`HudRootScroll`**, encounter labels scroll when stack is tall).
## Technical approach
### Server contract (landed — NEO-119 / NEO-120 / NEO-115)
@ -183,6 +194,8 @@ Debounce: skip if **`quest_progress_client`** accept busy (same as craft busy gu
|-----------|----------------|
| `client/test/quest_progress_client_test.gd` | Mock GET 200: **`not_started`** / **`active`** with **`objectiveCounters`** / **`completed`** rows; **`quest_row`**; GET 404 → **`quest_sync_failed`**; POST accept **`accepted: true`** and **`accepted: false`** + **`reasonCode`**. AAA layout (`# Arrange` / `# Act` / `# Assert`). |
| `client/test/quest_definitions_client_test.gd` | Parse v1 defs JSON (four quests); **`display_name_for`** returns catalog **`displayName`**; unknown id fallback. AAA layout. |
| `client/test/craft_recipe_panel_test.gd` | Recipe row populate; economy body scroll reset on repopulate; **`craft_requested`** signal. AAA layout. |
| `client/test/prototype_economy_hud_section_test.gd` | Economy collapse toggle; viewport pin + **`HudRootScroll`** clamp (NEO-122 follow-up). AAA layout. |
No new **C#** tests (client-only; server covered by **`QuestProgressApiTests`** / **`QuestAcceptApiTests`**). No Bruno changes.

View File

@ -4,6 +4,7 @@
**Scope:** Branch `NEO-122-client-quest-progress-accept-hud` vs `origin/main` — commits `b36fe6d``632a747` (delta after [initial review](2026-06-07-NEO-122.md))
**Base:** `origin/main`
**Prior review:** `docs/reviews/2026-06-07-NEO-122.md` — prior suggestions **1** / **3** and nits **12** addressed in `b36fe6d`; suggestion **2** (extract quest HUD) still deferred.
**Follow-up:** suggestions **12** below are **done** (strikethrough + **Done.**); suggestion **3** is manual regression spot-check for author/reviewer.
## Verdict
@ -17,10 +18,10 @@ Seven commits landed after the first review: code-review follow-up (quest-defini
| Path | Result |
|------|--------|
| `docs/plans/NEO-122-implementation-plan.md` | **Partially matches** — quest HUD reconciliation still accurate; **economy HUD layout refactor** (HudRootScroll, pinned economy panel, recipe panel restructure, recipe-first ordering) **not** recorded in plan reconciliation or kickoff scope. |
| `docs/manual-qa/NEO-122.md` | **Partially matches** — step 7 craft visibility added; **duplicate step 9** numbering (two lines labeled **9.**). |
| `client/README.md` | **Matches** quest + economy node paths (`HudRootScroll`, `BodyScroll/Body`); test-scope paragraph omits new **`craft_recipe_panel_test.gd`**. |
| `docs/manual-qa/NEO-74.md` / `NEO-75.md` / `NEO-110.md` | **Not updated** — no HudRootScroll / economy-pin notes; spot-check recommended (non-blocking). |
| `docs/plans/NEO-122-implementation-plan.md` | ~~**Partially matches**~~ **Matches** — NEO-122 follow-up reconciliation added (economy HUD layout refactor). **Done.** |
| `docs/manual-qa/NEO-122.md` | ~~**Partially matches**~~ **Matches** — duplicate step 9 fixed; acceptance includes craft visibility. **Done.** |
| `client/README.md` | ~~**Partially matches**~~ **Matches** — test-scope includes **`craft_recipe_panel_test.gd`**. **Done.** |
| `docs/manual-qa/NEO-74.md` / `NEO-75.md` / `NEO-110.md` | **Updated** — HudRootScroll / economy-pin regression notes added. **Done.** |
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — unchanged since first review. |
| Full-stack epic decomposition | **Matches** — still client slice only; NEO-123 capstone deferred. |
@ -30,9 +31,9 @@ Seven commits landed after the first review: code-review follow-up (quest-defini
## Suggestions
1. **Reconcile implementation plan** — Append a short **NEO-122 follow-up** bullet to `docs/plans/NEO-122-implementation-plan.md` **Implementation reconciliation**: economy HUD pinned layout, `HudRootScroll`, `craft_recipe_panel` VBox + `RecipeRows`, recipes-first in `BodyScroll/Body`, and manual QA step 7 rationale. Keeps [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md) honest about scope expansion.
1. ~~**Reconcile implementation plan** — Append a short **NEO-122 follow-up** bullet to `docs/plans/NEO-122-implementation-plan.md` **Implementation reconciliation**: economy HUD pinned layout, `HudRootScroll`, `craft_recipe_panel` VBox + `RecipeRows`, recipes-first in `BodyScroll/Body`, and manual QA step 7 rationale. Keeps [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md) honest about scope expansion.~~ **Done.** Follow-up subsection in plan reconciliation; tests table extended.
2. **Fix manual QA numbering** — In `docs/manual-qa/NEO-122.md`, renumber the second **9.** (server-down optional) to **10.** and the definitions-error optional to **11.**; update **Acceptance** if it should include the craft-visibility step (currently “Steps 18” while required steps now include **9**).
2. ~~**Fix manual QA numbering** — In `docs/manual-qa/NEO-122.md`, renumber the second **9.** (server-down optional) to **10.** and the definitions-error optional to **11.**; update **Acceptance** if it should include the craft-visibility step (currently “Steps 18” while required steps now include **9**).~~ **Done.** Steps 112; acceptance covers steps 19 + craft visibility.
3. **Regression spot-check** — Run abbreviated passes of [NEO-74](../manual-qa/NEO-74.md), [NEO-75](../manual-qa/NEO-75.md), and [NEO-110](../manual-qa/NEO-110.md) after merge: collapsed economy toggle, gather/craft feedback still visible in **`HudRootScroll`**, encounter labels still scroll when debug stack is tall.