NEON-26: address code review follow-ups (zoom validation, docs)

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.

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

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:

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)

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()

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
 

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