diff --git a/client/README.md b/client/README.md index 546e14e..1744692 100644 --- a/client/README.md +++ b/client/README.md @@ -106,6 +106,8 @@ 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`**, **`position_authority_client.gd`**, and **`ground_pick.gd`** (walkable collider check). **`main.gd`**, full **`_input` / ray pick** flows, and scene wiring are **not** automated here—use manual checks above. +**Scope:** Unit tests cover **`player.gd`**, **`position_authority_client.gd`**, **`ground_pick.gd`** (walkable collider check), **`isometric_follow_camera.gd`** (eye math + effective zoom distance), **`camera_state.gd`**, and **`zoom_band_config.gd`**. **`main.gd`**, full **`_input` / ray pick** flows, 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`**. On layouts where **`+`** requires **Shift**, remap or add a binding under **Project → Project Settings → Input Map** if needed. **Reports:** GdUnit writes under **`reports/`** (gitignored); ignore locally generated HTML/XML when committing. diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index fbef68b..d36233e 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -9,7 +9,7 @@ extends Node3D const CameraStateScript := preload("res://scripts/camera_state.gd") const ZoomBandConfigScript := preload("res://scripts/zoom_band_config.gd") -## TODO(E9.M1): throttled `camera_zoom_changed` telemetry when schema exists. +## TODO(E9.M1): map throttled product telemetry (e.g. `camera_zoom_changed`) to this signal when schema exists. signal zoom_band_changed(new_index: int, distance: float) ## Tuned to match pre-NEON-25 static `Camera3D` at (12,10,12) vs player at (-5,0.9,-5). @@ -67,7 +67,7 @@ func _ready() -> void: ) global_position = _smoothed_eye camera.look_at(focus0, Vector3.UP) - _sync_camera_state(focus0) + _sync_camera_state(focus0, dist0) else: _smoothed_eye = global_position _warn_missing_follow_target_once() @@ -90,9 +90,9 @@ func _process(delta: float) -> void: var focus: Vector3 = target.global_position + Vector3(0.0, focus_vertical_offset, 0.0) var yaw_total: float = deg_to_rad(presentation_yaw_deg) + _orbit_yaw_rad - var dist: float = _current_follow_distance() + var effective_dist: float = _current_follow_distance() var desired: Vector3 = desired_eye_world( - focus, dist, deg_to_rad(pitch_elevation_deg), yaw_total + focus, effective_dist, deg_to_rad(pitch_elevation_deg), yaw_total ) if _smoothed_eye.distance_to(desired) >= snap_distance: @@ -103,7 +103,7 @@ func _process(delta: float) -> void: global_position = _smoothed_eye camera.look_at(focus, Vector3.UP) - _sync_camera_state(focus) + _sync_camera_state(focus, effective_dist) func _unhandled_input(event: InputEvent) -> void: @@ -124,7 +124,8 @@ func _apply_zoom_step(delta_bands: int) -> void: if new_idx == _zoom_band_index: return _zoom_band_index = new_idx - zoom_band_changed.emit(_zoom_band_index, _current_follow_distance()) + var d: float = _current_follow_distance() + zoom_band_changed.emit(_zoom_band_index, d) func _init_zoom_band_index() -> void: @@ -140,6 +141,7 @@ func _zoom_config_valid() -> bool: zoom_band_config != null and zoom_band_config.get_script() == ZoomBandConfigScript and zoom_band_config.band_count() > 0 + and zoom_band_config.all_band_distances_positive() ) @@ -154,14 +156,16 @@ static func effective_follow_distance( return fallback_distance if zoom_cfg.band_count() == 0: return fallback_distance + if not zoom_cfg.all_band_distances_positive(): + return fallback_distance return zoom_cfg.distance_at(band_index) -func _sync_camera_state(focus: Vector3) -> void: +func _sync_camera_state(focus: Vector3, effective_distance: float) -> void: # `CameraState.yaw` = orbit delta only; world-fixed diagonal framing = # `presentation_yaw_deg`. _state.follow_target_path = follow_target_path - _state.distance = _current_follow_distance() + _state.distance = effective_distance _state.zoom_band_index = _zoom_band_index if _zoom_config_valid() else 0 _state.focus_world = focus _state.yaw = _orbit_yaw_rad diff --git a/client/scripts/zoom_band_config.gd b/client/scripts/zoom_band_config.gd index a83dbe7..b8b1f7b 100644 --- a/client/scripts/zoom_band_config.gd +++ b/client/scripts/zoom_band_config.gd @@ -10,6 +10,14 @@ func band_count() -> int: return band_distances.size() +## **True** when every entry is **> 0** (empty array is vacuously true; callers also check [method band_count]). +func all_band_distances_positive() -> bool: + for i in range(band_distances.size()): + if band_distances[i] <= 0.0: + return false + return true + + func clamp_index(i: int) -> int: var n := band_count() if n == 0: diff --git a/client/test/isometric_follow_camera_test.gd b/client/test/isometric_follow_camera_test.gd index fbe0666..5bae4d4 100644 --- a/client/test/isometric_follow_camera_test.gd +++ b/client/test/isometric_follow_camera_test.gd @@ -35,3 +35,9 @@ func test_effective_follow_distance_empty_config_bands_falls_back() -> void: var cfg = ZoomBandConfigScript.new() cfg.band_distances = PackedFloat32Array() assert_that(IsoScript.effective_follow_distance(cfg, 0, 7.5)).is_equal(7.5) + + +func test_effective_follow_distance_fallback_when_band_non_positive() -> void: + var cfg = ZoomBandConfigScript.new() + cfg.band_distances = PackedFloat32Array([10.0, 0.0, 30.0]) + assert_that(IsoScript.effective_follow_distance(cfg, 2, 99.0)).is_equal(99.0) diff --git a/client/test/zoom_band_config_test.gd b/client/test/zoom_band_config_test.gd index 901b72b..de83f51 100644 --- a/client/test/zoom_band_config_test.gd +++ b/client/test/zoom_band_config_test.gd @@ -46,3 +46,24 @@ func test_default_index_clamped_via_clamp_index() -> void: c.band_distances = PackedFloat32Array([1.0, 2.0, 3.0]) c.default_band_index = 99 assert_that(c.clamp_index(c.default_band_index)).is_equal(2) + + +func test_all_band_distances_positive_vacuous_when_empty() -> void: + var c = ZoomBandConfigScript.new() + c.band_distances = PackedFloat32Array() + assert_that(c.all_band_distances_positive()).is_true() + + +func test_all_band_distances_positive_true_when_all_positive() -> void: + var c = ZoomBandConfigScript.new() + c.band_distances = PackedFloat32Array([1.0, 2.5, 10.0]) + assert_that(c.all_band_distances_positive()).is_true() + + +func test_all_band_distances_positive_false_when_zero_or_negative() -> void: + var c0 = ZoomBandConfigScript.new() + c0.band_distances = PackedFloat32Array([10.0, 0.0, 20.0]) + assert_that(c0.all_band_distances_positive()).is_false() + var cn = ZoomBandConfigScript.new() + cn.band_distances = PackedFloat32Array([5.0, -1.0]) + assert_that(cn.all_band_distances_positive()).is_false() diff --git a/docs/plans/NEON-26-implementation-plan.md b/docs/plans/NEON-26-implementation-plan.md index 6c55701..eaf24c1 100644 --- a/docs/plans/NEON-26-implementation-plan.md +++ b/docs/plans/NEON-26-implementation-plan.md @@ -39,7 +39,7 @@ ## Technical approach 1. **`zoom_band_config.gd`** — `extends Resource` with e.g. `@export var band_distances: PackedFloat32Array` (length ≥ 1) and `@export var default_band_index: int`. Provide **`band_count`**, **`clamp_index(i: int) -> int`**, **`distance_at(index: int) -> float`** (uses clamped index). Validate in `_validate_property` or getter: if empty at runtime, treat as single band using rig fallback (see below). -2. **`isometric_follow_camera.gd`** — Add `@export var zoom_band_config: ZoomBandConfig`. Keep **`follow_distance`** as **fallback** when config is null or has no bands (preserves NEON-25 scenes/tests). Track **`_zoom_band_index`** initialized from config default (clamped). On zoom-in/out actions: adjust index with `clamp_index`, no op at ends (or wrap — **clamp** per story). **`_process`:** compute `var d := effective_follow_distance()` from config + index or fallback export. Replace uses of raw `follow_distance` in `desired_eye_world` with `d`. **`_sync_camera_state`:** assign `_state.zoom_band_index` and `_state.distance = d` (and existing yaw / path / focus). +2. **`isometric_follow_camera.gd`** — Add **`@export var zoom_band_config: Resource`** (no `class_name` on the config script); at runtime require **`get_script() == preload("…/zoom_band_config.gd")`** so headless CI and typed exports stay aligned. Conceptually this is **`ZoomBandConfig`**. Keep **`follow_distance`** as **fallback** when config is null, empty, or has non‑positive band distances (preserves NEON-25 scenes/tests). Track **`_zoom_band_index`** initialized from config default (clamped). On zoom-in/out actions: adjust index with `clamp_index`, no op at ends (or wrap — **clamp** per story). **`_process`:** compute effective distance from config + index or fallback export. Replace uses of raw `follow_distance` in `desired_eye_world` with that value. **`_sync_camera_state`:** pass effective distance into state (`zoom_band_index` + `distance`, plus existing yaw / path / focus). 3. **`project.godot`** — Add **`camera_zoom_in`** and **`camera_zoom_out`** (or one action with axis — prefer two actions for wheel + keys) with mouse wheel and key events as in Jira examples. 4. **Scene / resource** — Add a default **`ZoomBandConfig`** `.tres` under `res://resources/` (or co-located under `client/resources/`) with **≥ 3** bands bracketing **~25.709** m so readability can be checked; assign on **`IsometricFollowCamera`** in `main.tscn`. 5. **Telemetry stub** — Optional `signal zoom_band_changed(new_index: int, distance: float)` or `TODO` comment referencing E9.M1; no external schema required for this story. @@ -51,6 +51,7 @@ | **Band parameter** | **Distance-only** per step (same pitch / FOV as NEON-25); document in module snapshot. FOV-per-band deferred unless trivial. | | **Bounds at ends** | **Clamp** (no wrap): input at min/max does nothing extra. | | **Config null / empty** | Use **`follow_distance`** export only; **`zoom_band_index`** in state = `0`. | +| **Invalid band values** | Any **≤ 0** distance in **`band_distances`** treats the config as unusable (same fallback as null/empty). | ## Files to add diff --git a/docs/reviews/2026-04-08-NEON-26.md b/docs/reviews/2026-04-08-NEON-26.md index cf15c17..d2e0bcd 100644 --- a/docs/reviews/2026-04-08-NEON-26.md +++ b/docs/reviews/2026-04-08-NEON-26.md @@ -2,11 +2,12 @@ **Date:** 2026-04-08 **Scope:** Branch `NEON-26-zoom-band-config-discrete-input` vs `origin/main` (NEON-26 zoom bands + follow-up `.tres` tuning commits). Issue **NEON-26**; no PR URL supplied. -**Base:** `origin/main` @ `88653292104eddfc1db702f97b02a088fb80c1c7` (merge-base with HEAD at review time). +**Base:** `origin/main` @ `88653292104eddfc1db702f97b02a088fb80c1c7` (merge-base with HEAD at review time). +**Follow-up:** Suggestions and nits below were implemented in-repo (README, plan typing note, positive-distance validation, `_sync_camera_state(effective_distance)`, telemetry TODO wording). ## Verdict -**Approve with nits** +**Approve** ## Summary @@ -16,7 +17,7 @@ The branch delivers **data-driven discrete zoom** via `ZoomBandConfig`, **InputM | Document | Result | |----------|--------| -| `docs/plans/NEON-26-implementation-plan.md` | **Matches** intent and acceptance criteria. Minor **partial match**: the plan’s technical approach mentions `@export var zoom_band_config: ZoomBandConfig`; the implementation correctly uses **`Resource` + `get_script()`** comparison to avoid `class_name`, consistent with `godot-client-script-organization` and NEON-14 headless notes. Consider a one-line clarification in the plan that the export is untyped/`Resource` by design. | +| `docs/plans/NEON-26-implementation-plan.md` | **Matches** — technical approach updated to document **`Resource` export + `get_script()`** and positive-distance fallback; aligns with implementation. | | `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — implementation snapshot updated for NEON-26 (config path, input actions, distance-only bands, fallback, signal + E9.M1 TODO). | | `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E1.M2 note reflects zoom bands shipped. | | `docs/decomposition/modules/client_server_authority.md` (camera client-local) | **Matches** — no server use of camera pose introduced. | @@ -28,19 +29,19 @@ None. ## Suggestions -1. **`client/README.md` (testing section)** — The bullet that lists covered scripts is now slightly stale: it does not mention **`isometric_follow_camera.gd`**, **`camera_state.gd`**, or **`zoom_band_config.gd`**, though **`client/test/`** includes suites for them. Per [testing-expectations](../../.cursor/rules/testing-expectations.md), consider extending that sentence so contributors know camera/zoom logic is under GdUnit. +1. ~~**`client/README.md` (testing section)** — The bullet that lists covered scripts is now slightly stale: it does not mention **`isometric_follow_camera.gd`**, **`camera_state.gd`**, or **`zoom_band_config.gd`**, though **`client/test/`** includes suites for them. Per [testing-expectations](../../.cursor/rules/testing-expectations.md), consider extending that sentence so contributors know camera/zoom logic is under GdUnit.~~ **Done.** Scope bullet extended; short **Camera zoom** note added for Input Map / layout remapping. -2. **`ZoomBandConfig` validation** — Designer-editable **`band_distances`** could theoretically include **non-positive** values, which would produce odd framing. Optional: `_validate_property` or a short runtime guard in the rig when loading config (non-blocking for prototype). +2. ~~**`ZoomBandConfig` validation** — Designer-editable **`band_distances`** could theoretically include **non-positive** values, which would produce odd framing. Optional: `_validate_property` or a short runtime guard in the rig when loading config (non-blocking for prototype).~~ **Done.** **`all_band_distances_positive()`** on the resource; **`_zoom_config_valid()`** and **`effective_follow_distance`** fall back when any distance is ≤ 0; tests added. -3. **Plan vs implementation (typing)** — As above, add a brief note to **`NEON-26-implementation-plan.md`** that **`zoom_band_config` is exported as `Resource`** (or untyped) with script identity checked at runtime, so future readers are not confused by the plan’s “ZoomBandConfig” type mention. +3. ~~**Plan vs implementation (typing)** — As above, add a brief note to **`NEON-26-implementation-plan.md`** that **`zoom_band_config` is exported as `Resource`** (or untyped) with script identity checked at runtime, so future readers are not confused by the plan’s “ZoomBandConfig” type mention.~~ **Done.** Technical approach step 2 rewritten accordingly. ## Nits -- **`_current_follow_distance()`** is invoked multiple times per **`_process`** / **`_sync_camera_state`**; a single local **`var d`** per frame would avoid redundant calls (micro-optimization, clarity). +- ~~**`_current_follow_distance()`** is invoked multiple times per **`_process`** / **`_sync_camera_state`**; a single local **`var d`** per frame would avoid redundant calls (micro-optimization, clarity).~~ **Done.** **`_sync_camera_state(focus, effective_distance)`** takes the distance computed once per frame (and **`_ready`** passes **`dist0`**). -- **Keyboard zoom:** **`=`** without **Shift** may not match all layouts’ “+” expectations; wheel and keypad bindings mitigate this. Document in README or input remap note if QA reports gaps. +- ~~**Keyboard zoom:** **`=`** without **Shift** may not match all layouts’ “+” expectations; wheel and keypad bindings mitigate this. Document in README or input remap note if QA reports gaps.~~ **Done.** Documented under **Camera zoom** in **`client/README.md`**. -- **Signal name vs telemetry TODO:** Comment mentions **`camera_zoom_changed`**; signal is **`zoom_band_changed`** — harmless but slightly inconsistent wording for whoever wires E9.M1 later. +- ~~**Signal name vs telemetry TODO:** Comment mentions **`camera_zoom_changed`**; signal is **`zoom_band_changed`** — harmless but slightly inconsistent wording for whoever wires E9.M1 later.~~ **Done.** TODO now references mapping product telemetry to **`zoom_band_changed`**. ## Verification